1.5.1 (#244)

* Fix the connection impersonation feature flag resolver * Fix substitutions with null dates * Update CHANGELOG, cleanup.
ClickHouse · Jun 7, 2024 · 6f478d5 · 6f478d5
1 parent 6375feb
commit 6f478d5
Show file tree

Hide file tree

Showing 9 changed files with 107 additions and 29 deletions.
diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml
@@ -10,7 +10,7 @@ on:
   pull_request:
 
 env:
-  METABASE_VERSION: v0.49.12
+  METABASE_VERSION: v0.49.14
 
 jobs:
   check-local-current-version:

diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,6 +1,15 @@
+# 1.5.1
+
+Metabase 0.49.14+ only.
+
+### Bug fixes
+
+* Fixed the issue where the Metabase instance could end up broken if the ClickHouse instance was _stopped_ during the upgrade to 1.5.0. ([#242](https://github.com/ClickHouse/metabase-clickhouse-driver/issues/242))
+* Fixed variables substitution with Nullable Date, Date32, DateTime, and DateTime64 columns, where the generated query could fail with NULL values in the database due to an incorrect cast call. ([#243](https://github.com/ClickHouse/metabase-clickhouse-driver/issues/243))
+
 # 1.5.0
 
-Metabase 0.49.12+ only.
+Metabase 0.49.14+ only.
 
 ### New features
 

diff --git a/README.md b/README.md
@@ -78,7 +78,7 @@ docker run -d -p 3000:3000 \
 | 0.47.7+          | 1.2.5          |
 | 0.48.x           | 1.3.4          |
 | 0.49.x           | 1.4.0          |
-| 0.49.12+         | 1.5.0          |
+| 0.49.14+         | 1.5.1          |
 
 ## Creating a Metabase Docker image with ClickHouse driver
 

diff --git a/docker-compose.yml b/docker-compose.yml
@@ -114,7 +114,7 @@ services:
   ##########################################################################################################
 
   metabase:
-    image: metabase/metabase-enterprise:v1.49.12
+    image: metabase/metabase-enterprise:v1.49.14
     container_name: metabase-with-clickhouse-driver
     environment:
       'MB_HTTP_TIMEOUT': '5000'

diff --git a/resources/metabase-plugin.yaml b/resources/metabase-plugin.yaml
@@ -1,6 +1,6 @@
 info:
   name: Metabase ClickHouse Driver
-  version: 1.5.0
+  version: 1.5.1
   description: Allows Metabase to connect to ClickHouse databases.
 contact-info:
   name: ClickHouse

diff --git a/src/metabase/driver/clickhouse.clj b/src/metabase/driver/clickhouse.clj
@@ -77,16 +77,13 @@
    to the database, it throws a java.sql.SQLException."
   (memoize/ttl
    (fn [db-details]
-     (sql-jdbc.execute/do-with-connection-with-options
-      :clickhouse
-      (connection-details->spec* db-details)
-      nil
-      (fn [^java.sql.Connection conn]
-        (with-open [stmt (.prepareStatement conn "SELECT value='1' FROM system.settings WHERE name='cloud_mode'")
-                    rset (.executeQuery stmt)]
-          (if (.next rset)
-            (.getBoolean rset 1)
-            false)))))
+     (let [spec (connection-details->spec* db-details)]
+       (sql-jdbc.execute/do-with-connection-with-options
+        :clickhouse spec nil
+        (fn [^java.sql.Connection conn]
+          (with-open [stmt (.prepareStatement conn "SELECT value='1' FROM system.settings WHERE name='cloud_mode'")
+                      rset (.executeQuery stmt)]
+            (if (.next rset) (.getBoolean rset 1) false))))))
    ;; cache the results for 48 hours; TTL is here only to eventually clear out old entries
    :ttl/threshold (* 48 60 60 1000)))
 
@@ -101,9 +98,11 @@
     (assoc :select_sequential_consistency true)))
 
 (defmethod driver/database-supports? [:clickhouse :uploads] [_driver _feature db]
-  (try (cloud? (:details db))
-       (catch java.sql.SQLException _e
-         false)))
+  (if (:details db)
+    (try (cloud? (:details db))
+         (catch java.sql.SQLException _e
+           false))
+    false))
 
 (defmethod driver/can-connect? :clickhouse
   [driver details]
@@ -229,7 +228,9 @@
 (defmethod driver/database-supports? [:clickhouse :connection-impersonation]
   [_driver _feature db]
   (if db
-    (clickhouse-version/is-at-least? 24 4 db)
+    (try (clickhouse-version/is-at-least? 24 4 db)
+         (catch Throwable _e
+           false))
     false))
 
 (defmethod driver.sql/set-role-statement :clickhouse

diff --git a/src/metabase/driver/clickhouse_qp.clj b/src/metabase/driver/clickhouse_qp.clj
@@ -100,7 +100,7 @@
 
 (defmethod sql.qp/date [:clickhouse :day]
   [_ _ expr]
-  (h2x/->date expr))
+  [:'toDate expr])
 
 (defmethod sql.qp/date [:clickhouse :week]
   [driver _ expr]

diff --git a/test/metabase/driver/clickhouse_substitution_test.clj b/test/metabase/driver/clickhouse_substitution_test.clj
@@ -253,3 +253,68 @@
                 (testing "next12years"
                   (is (= [[(->iso-str row4)]]
                          (ctd/rows-without-index (qp/process-query (get-mbql "next12years" db)))))))))))))))
+
+(deftest ^:parallel clickhouse-variables-field-filters-null-dates
+  (mt/test-driver
+   :clickhouse
+   (mt/with-clock clock
+     (letfn
+      [(->input-ld
+         [^LocalDate ld]
+         [(t/format "yyyy-MM-dd" ld)])
+       (->input-ldt
+         [^LocalDateTime ldt]
+         [(t/format "yyyy-MM-dd HH:mm:ss" ldt)])
+       (->iso-str-ld
+         [^LocalDate ld]
+         (str (t/format "yyyy-MM-dd" ld) "T00:00:00Z"))
+       (->iso-str-ldt
+         [^LocalDateTime ldt]
+         (t/format "yyyy-MM-dd'T'HH:mm:ss'Z'" ldt))]
+       (let [db         "metabase_tests_field_filters_null_dates"
+             now-ld     (local-date-now)
+             now-ldt    (local-date-time-now)
+             table      ["test_table"
+                         [{:field-name "d"
+                           :base-type {:native "Date"}}
+                          {:field-name "d32"
+                           :base-type {:native "Date32"}}
+                          {:field-name "dt"
+                           :base-type {:native "DateTime"}}
+                          {:field-name "dt64"
+                           :base-type {:native "DateTime64"}}]
+                         [;; row 1
+                          [(->input-ld now-ld) nil (->input-ldt now-ldt) nil]
+                          ;; row 2
+                          [nil (->input-ld now-ld) nil (->input-ldt now-ldt)]]]
+             first-row  [[(->iso-str-ld now-ld) nil (->iso-str-ldt now-ldt) nil]]
+             second-row [[nil (->iso-str-ld now-ld) nil (->iso-str-ldt now-ldt)]]]
+         (data/dataset
+          (tx/dataset-definition db table)
+          (letfn
+           [(get-mbql*
+              [field value]
+              (let [uuid (str (java.util.UUID/randomUUID))]
+                {:database (mt/id)
+                 :type "native"
+                 :native {:collection "test-table"
+                          :template-tags
+                          {:x {:id uuid
+                               :name (str field)
+                               :display-name (str field)
+                               :type "dimension"
+                               :dimension ["field" (mt/id :test-table field) nil]
+                               :required true}}
+                          :query (format "SELECT * FROM `%s`.`test_table` WHERE {{x}}" db)}
+                 :parameters [{:type "date/all-options"
+                               :value value
+                               :target ["dimension" ["template-tag" "x"]]
+                               :id uuid}]}))]
+            (testing "first row (Date field match)"
+              (is (= first-row (ctd/rows-without-index (qp/process-query (get-mbql* :d "2019-11-30"))))))
+            (testing "first row (DateTime field match)"
+              (is (= first-row (ctd/rows-without-index (qp/process-query (get-mbql* :dt "2019-11-30T23:00:00"))))))
+            (testing "second row (Date32 field match)"
+              (is (= second-row (ctd/rows-without-index (qp/process-query (get-mbql* :d32 "2019-11-30"))))))
+            (testing "second row (DateTime64 field match)"
+              (is (= second-row (ctd/rows-without-index (qp/process-query (get-mbql* :dt64 "2019-11-30T23:00:00")))))))))))))
diff --git a/test/metabase/driver/clickhouse_test.clj b/test/metabase/driver/clickhouse_test.clj
@@ -108,17 +108,20 @@
 (deftest clickhouse-connection-fails-test
   (mt/test-driver
    :clickhouse
-   (let [details (merge (tx/dbdef->connection-details
-                         :clickhouse :db
-                         {:database-name "mb_test_connection_fails"})
-                        {:password "wrong"})]
+   (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))))
-     (testing "`driver/database-supports?` succeeds even if the connection fails."
-       (is (false? (driver/database-supports? :clickhouse :uploads details))))
-     (testing (str "`sql-jdbc.conn/connection-details->spec` succeeds even if the connection fails, "
+       ;; nil arg isn't tested here, as it will pick up the defaults, which is the same as the Docker instance credentials.
+       (is (thrown? java.sql.SQLException (#'clickhouse/cloud? (:details db)))))
+     (testing "`driver/database-supports? :uploads` does not throw even if the connection fails."
+       (is (false? (driver/database-supports? :clickhouse :uploads db)))
+       (is (false? (driver/database-supports? :clickhouse :uploads nil))))
+     (testing "`driver/database-supports? :connection-impersonation` does not throw even if the connection fails."
+       (is (false? (driver/database-supports? :clickhouse :connection-impersonation db)))
+       (is (false? (driver/database-supports? :clickhouse :connection-impersonation nil))))
+     (testing (str "`sql-jdbc.conn/connection-details->spec` does not throw 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))))))))
+       (is (nil? (:select_sequential_consistency (sql-jdbc.conn/connection-details->spec :clickhouse db))))
+       (is (nil? (:select_sequential_consistency (sql-jdbc.conn/connection-details->spec :clickhouse nil))))))))
 
 (deftest ^:parallel clickhouse-tls
   (mt/test-driver