Skip to content

CSV Uploads: catch failed connections and fix duplicate inserts #238

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 21, 2024
Merged

CSV Uploads: catch failed connections and fix duplicate inserts #238

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
da1a25f
+ catch sql exceptions from `cloud?`
calherries May 20, 2024
79042cc
fix reference to cloud?
calherries May 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions src/metabase/driver/clickhouse.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,8 @@
(sql-jdbc.common/handle-additional-options details :separator-style :url))))

(def ^:private ^{:arglists '([db-details])} cloud?
"Is this a cloud DB?"
"Returns true if the `db-details` are for a ClickHouse Cloud instance, and false otherwise. If it fails to connect
to the database, it throws a java.sql.SQLException."
(memoize/ttl
(fn [db-details]
(sql-jdbc.execute/do-with-connection-with-options
Expand All @@ -85,13 +86,17 @@
(defmethod sql-jdbc.conn/connection-details->spec :clickhouse
[_ details]
(cond-> (connection-details->spec* details)
(cloud? details)
(try (cloud? details)
(catch java.sql.SQLException _e
false))
;; select_sequential_consistency guarantees that we can query data from any replica in CH Cloud
;; immediately after it is written
(assoc :select_sequential_consistency true)))

(defmethod driver/database-supports? [:clickhouse :uploads] [_driver _feature db]
(cloud? (:details db)))
(try (cloud? (:details db))
(catch java.sql.SQLException _e
false)))

(defmethod driver/can-connect? :clickhouse
[driver details]
Expand Down Expand Up @@ -179,7 +184,9 @@
:quoted true
:dialect (sql.qp/quote-style driver)))
"ENGINE = MergeTree"
(format "ORDER BY (%s)" (str/join ", " (map quote-name primary-key)))]))
(format "ORDER BY (%s)" (str/join ", " (map quote-name primary-key)))
;; disable insert idempotency to allow duplicate inserts
"SETTINGS replicated_deduplication_window = 0"]))
Comment on lines +188 to +189
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested by metabase/metabase#42918


(defmethod driver/create-table! :clickhouse
[driver db-id table-name column-definitions & {:keys [primary-key]}]
Expand Down
11 changes: 11 additions & 0 deletions test/metabase/driver/clickhouse_test.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,17 @@
:clickhouse
{}))))))

(deftest clickhouse-connection-fails-test
(mt/test-driver :clickhouse
(mt/with-temp [:model/Database db {:details (assoc (mt/db) :password "wrongpassword") :engine :clickhouse}]
(testing "Sense check that checking the cloud mode fails with a SQLException."
(is (thrown? java.sql.SQLException (#'clickhouse/cloud? (:details db)))))
(testing "`driver/database-supports?` succeeds even if the connection fails."
(is (false? (driver/database-supports? :clickhouse :uploads db))))
(testing (str "`sql-jdbc.conn/connection-details->spec` succeeds even if the connection fails, "
"and doesn't include the `select_sequential_consistency` parameter.")
(is (nil? (:select_sequential_consistency (sql-jdbc.conn/connection-details->spec :clickhouse (:details db)))))))))

(deftest ^:parallel clickhouse-tls
(mt/test-driver
:clickhouse
Expand Down
Loading