From 0eb1f76bcc7f98ca25f7aabb4ad54ba6cc470f96 Mon Sep 17 00:00:00 2001 From: lpoulain Date: Wed, 24 Jan 2024 16:45:21 -0500 Subject: [PATCH] Switch to Metabase v1.48.3 Switch to Metabase v1.48.3 --- Makefile | 3 +- README.md | 4 + app_versions.json | 2 +- driver_test.clj | 116 ++++++++++++++++++ .../driver/implementation/driver_helpers.clj | 2 +- .../driver/implementation/execute.clj | 12 ++ .../metabase/driver/implementation/sync.clj | 22 ++-- .../test/metabase/driver/starburst_test.clj | 12 +- 8 files changed, 154 insertions(+), 19 deletions(-) create mode 100644 driver_test.clj diff --git a/Makefile b/Makefile index a4da9ec..a222008 100644 --- a/Makefile +++ b/Makefile @@ -78,6 +78,7 @@ update_deps_files: test: start_trino_if_missing link_to_driver update_deps_files @echo "Testing Starburst driver..." + cp driver_test.clj metabase/test/metabase/ cd $(makefile_dir)/metabase/; DRIVERS=starburst MB_STARBURST_TEST_PORT=$(trino_port) clojure -X:dev:drivers:drivers-dev:test testOptimized: start_trino_if_missing link_to_driver update_deps_files @@ -87,4 +88,4 @@ testOptimized: start_trino_if_missing link_to_driver update_deps_files build: clone_metabase_if_missing update_deps_files link_to_driver front_end driver docker-image: - cd $(makefile_dir)/metabase/; export MB_EDITION=ee && ./bin/build.sh && mv target/uberjar/metabase.jar bin/docker/ && docker build -t metabase-dev --build-arg MB_EDITION=ee ./bin/docker/ \ No newline at end of file + cd $(makefile_dir)/metabase/; export MB_EDITION=ee && ./bin/build.sh && mv target/uberjar/metabase.jar bin/docker/ && docker build -t metabase-dev --build-arg MB_EDITION=ee ./bin/docker/ diff --git a/README.md b/README.md index f0cae83..bb86d41 100644 --- a/README.md +++ b/README.md @@ -64,6 +64,10 @@ Head to actions and run the `Release` workflow entering the same the same semant ### Update Metabase Version If needed, `make checkout_latest_metabase_tag` will update Metabase to its latest tagged release. +*CAUTION*: the Metabase test file `metabase/test/metabase/driver_test.clj` is overridden by a modified version on the root directory (see the `Makefile`). This is because two tests (`can-connect-with-destroy-db-test` and `check-can-connect-before-sync-test`) do not work with the Starburst driver as they're testing what happens when a database is deleted (which cannot happen with Starburst). So instead of adding some useless stuff to `can-connect?` for the sole purpose of satisfying tests, it was found preferable to just remove those two tests. + +Whenever upgrading the version of Metabase, `./driver_test.clj` should be replaced with `metabase/test/metabase/driver_test.clj` with the two offending tests removed (unless they pass or there is a clean way around them) + ## References * [Encrypting Metabase Database Details](https://www.metabase.com/docs/latest/operations-guide/encrypting-database-details-at-rest.html) * [Developer's Guide](https://www.metabase.com/docs/latest/developers-guide/start.html) diff --git a/app_versions.json b/app_versions.json index 4ed2b9a..fdeed35 100644 --- a/app_versions.json +++ b/app_versions.json @@ -1,5 +1,5 @@ { "trino": "431", "clojure": "1.11.1.1262", - "metabase": "v1.47.2" + "metabase": "v1.48.3" } diff --git a/driver_test.clj b/driver_test.clj new file mode 100644 index 0000000..a9d6d6c --- /dev/null +++ b/driver_test.clj @@ -0,0 +1,116 @@ +(ns metabase.driver-test + (:require + [cheshire.core :as json] + [clojure.test :refer :all] + [metabase.driver :as driver] + [metabase.driver.h2 :as h2] + [metabase.driver.impl :as driver.impl] + [metabase.plugins.classloader :as classloader] + [metabase.task.sync-databases :as task.sync-databases] + [metabase.test :as mt] + [metabase.test.data.env :as tx.env] + [metabase.test.data.interface :as tx] + [metabase.util :as u] + [toucan2.core :as t2])) + +(set! *warn-on-reflection* true) + +(driver/register! ::test-driver, :abstract? true) + +(defmethod driver/database-supports? [::test-driver :foreign-keys] [_driver _feature _db] true) +(defmethod driver/database-supports? [::test-driver :foreign-keys] [_driver _feature db] (= db "dummy")) + +(deftest ^:parallel database-supports?-test + (is (driver/database-supports? ::test-driver :foreign-keys "dummy")) + (is (not (driver/database-supports? ::test-driver :foreign-keys "not-dummy"))) + (is (not (driver/database-supports? ::test-driver :expressions "dummy"))) + (is (thrown-with-msg? + java.lang.Exception + #"Invalid driver feature: .*" + (driver/database-supports? ::test-driver :some-made-up-thing "dummy")))) + +(deftest the-driver-test + (testing (str "calling `the-driver` should set the context classloader, important because driver plugin code exists " + "there but not elsewhere") + (.setContextClassLoader (Thread/currentThread) (ClassLoader/getSystemClassLoader)) + (driver/the-driver :h2) + (is (= @@#'classloader/shared-context-classloader + (.getContextClassLoader (Thread/currentThread)))))) + +(deftest available?-test + (with-redefs [driver.impl/concrete? (constantly true)] + (is (driver/available? ::test-driver)) + (is (driver/available? "metabase.driver-test/test-driver") + "`driver/available?` should work for if `driver` is a string -- see #10135"))) + +(deftest ^:parallel unique-connection-property-test + ;; abnormal usage here; we are not using the regular mt/test-driver or mt/test-drivers, because those involve + ;; initializing the driver and test data namespaces, which don't necessarily exist for all drivers (ex: + ;; googleanalytics), and besides which, we don't actually need sample data or test extensions for this test itself + + ;; so instead, just iterate through all drivers currently set to test by the environment, and check their + ;; connection-properties; between all the different CI driver runs, this should cover everything + (doseq [d (tx.env/test-drivers)] + (testing (str d " has entirely unique connection property names") + (let [props (driver/connection-properties d) + props-by-name (group-by :name props)] + (is (= (count props) (count props-by-name)) + (format "Property(s) with duplicate name: %s" (-> (filter (fn [[_ props]] + (> (count props) 1)) + props-by-name) + vec + pr-str))))))) + +(deftest supports-schemas-matches-describe-database-test + (mt/test-drivers (mt/normal-drivers) + (if (driver/database-supports? driver/*driver* :schemas (mt/db)) + (testing "`describe-database` should return schemas with tables if the database supports schemas" + (is (some? (->> (driver/describe-database driver/*driver* (mt/db)) + :tables + (some :schema))))) + (testing "`describe-database` should not return schemas with tables if the database doesn't support schemas" + (is (nil? (->> (driver/describe-database driver/*driver* (mt/db)) + :tables + (some :schema)))))))) + +(defn- basic-db-definition [database-name] + (tx/map->DatabaseDefinition + {:database-name database-name + :table-definitions [{:table-name "baz" + :field-definitions [{:field-name "foo", :base-type :type/Text}] + :rows [["bar"]]}]})) + +(deftest supports-table-privileges-matches-implementations-test + (mt/test-drivers (mt/normal-drivers-with-feature :table-privileges) + (is (some? (driver/current-user-table-privileges driver/*driver* (mt/db)))))) + +(deftest nonsql-dialects-return-original-query-test + (mt/test-driver :mongo + (testing "Passing a mongodb query through [[driver/prettify-native-form]] returns the original query (#31122)" + (let [query [{"$group" {"_id" {"created_at" {"$let" {"vars" {"parts" {"$dateToParts" {"timezone" "UTC" + "date" "$created_at"}}} + "in" {"$dateFromParts" {"timezone" "UTC" + "year" "$$parts.year" + "month" "$$parts.month" + "day" "$$parts.day"}}}}} + "sum" {"$sum" "$tax"}}} + {"$sort" {"_id" 1}} + {"$project" {"_id" false + "created_at" "$_id.created_at" + "sum" true}}] + formatted-query (driver/prettify-native-form :mongo query)] + + (testing "Formatting a non-sql query returns the same query" + (is (= query formatted-query))) + + ;; TODO(qnkhuat): do we really need to handle case where wrong driver is passed? + (let [;; This is a mongodb query, but if you pass in the wrong driver it will attempt the format + ;; This is a corner case since the system should always be using the right driver + weird-formatted-query (driver/prettify-native-form :postgres (json/generate-string query))] + (testing "The wrong formatter will change the format..." + (is (not= query weird-formatted-query))) + (testing "...but the resulting data is still the same" + ;; Bottom line - Use the right driver, but if you use the wrong + ;; one it should be harmless but annoying + (is (= query + (json/parse-string weird-formatted-query))))))))) diff --git a/drivers/starburst/src/metabase/driver/implementation/driver_helpers.clj b/drivers/starburst/src/metabase/driver/implementation/driver_helpers.clj index d79c039..1831ef2 100644 --- a/drivers/starburst/src/metabase/driver/implementation/driver_helpers.clj +++ b/drivers/starburst/src/metabase/driver/implementation/driver_helpers.clj @@ -33,7 +33,7 @@ :native-parameters true :expression-aggregations true :binning true - :foreign-keys true + :foreign-keys false :datetime-diff true :convert-timezone true :now true}] diff --git a/drivers/starburst/src/metabase/driver/implementation/execute.clj b/drivers/starburst/src/metabase/driver/implementation/execute.clj index aec0e2a..9426512 100644 --- a/drivers/starburst/src/metabase/driver/implementation/execute.clj +++ b/drivers/starburst/src/metabase/driver/implementation/execute.clj @@ -199,6 +199,12 @@ (remove-impersonation conn) rs) (catch Throwable e (handle-execution-error e)))) + (execute [] + (try + (let [rs (.execute stmt)] + (remove-impersonation conn) + rs) + (catch Throwable e (handle-execution-error e)))) (setMaxRows [nb] (.setMaxRows stmt nb)) (setObject ([index obj] (.setObject stmt index obj)) @@ -236,6 +242,12 @@ (remove-impersonation conn) rs) (catch Throwable e (handle-execution-error e)))) + (execute [] + (try + (let [rs (.execute stmt)] + (remove-impersonation conn) + rs) + (catch Throwable e (handle-execution-error e)))) (setMaxRows [nb] (.setMaxRows stmt nb)) (close [] (.close stmt)))) diff --git a/drivers/starburst/src/metabase/driver/implementation/sync.clj b/drivers/starburst/src/metabase/driver/implementation/sync.clj index 537bb59..a7a5fd5 100644 --- a/drivers/starburst/src/metabase/driver/implementation/sync.clj +++ b/drivers/starburst/src/metabase/driver/implementation/sync.clj @@ -20,9 +20,10 @@ [java-time :as t] [metabase.driver :as driver] [metabase.driver.sql-jdbc.connection :as sql-jdbc.conn] - [metabase.driver.sql-jdbc.sync :as sql-jdbc.sync] [metabase.driver.sql-jdbc.sync.describe-database :as sql-jdbc.describe-database] [metabase.driver.sql-jdbc.execute :as sql-jdbc.execute] + [metabase.driver.sql-jdbc.sync.interface :as sql-jdbc.sync.interface] + [metabase.driver.sql-jdbc.sync :as sql-jdbc.sync] [metabase.driver.sql.util :as sql.u] [metabase.util.i18n :refer [trs]]) (:import (java.sql Connection))) @@ -86,15 +87,16 @@ (log/debugf "database-type->base-type %s -> %s" field-type base-type) base-type)) -(defn- have-select-privilege? - "Checks whether the connected user has permission to select from the given `table-name`, in the given `schema`." - [driver conn schema table-name] +(defmethod sql-jdbc.sync.interface/have-select-privilege? :starburst + [driver ^Connection conn table-schema table-name] (try - (let [sql (sql-jdbc.describe-database/simple-select-probe-query driver schema table-name)] - ;; if the query completes without throwing an Exception, we can SELECT from this table - (jdbc/reducible-query {:connection conn} sql) - true) - (catch Throwable _ + (let [sql (str "SHOW TABLES FROM \"" table-schema "\" LIKE '" table-name "'")] + ;; if the query completes without throwing an Exception, we can SELECT from this table + (with-open [stmt (.prepareStatement conn sql) + rs (.executeQuery stmt)] + (.next rs))) + (catch Throwable e + (log/fatal "ERROR WITH QUERY " e) false))) (defn- describe-schema @@ -106,7 +108,7 @@ (into #{} (comp (filter (fn [{table-name :table}] - (have-select-privilege? driver conn schema table-name))) + (sql-jdbc.sync.interface/have-select-privilege? driver conn schema table-name))) (map (fn [{table-name :table}] {:name table-name :schema schema}))) diff --git a/drivers/starburst/test/metabase/driver/starburst_test.clj b/drivers/starburst/test/metabase/driver/starburst_test.clj index 0ea857b..d9fe44a 100644 --- a/drivers/starburst/test/metabase/driver/starburst_test.clj +++ b/drivers/starburst/test/metabase/driver/starburst_test.clj @@ -31,7 +31,7 @@ [metabase.test :as mt] [metabase.test.fixtures :as fixtures] [toucan2.tools.with-temp :as t2.with-temp] - [toucan.db :as db])) + [toucan2.core :as t2])) (use-fixtures :once (fixtures/initialize :db)) @@ -78,7 +78,7 @@ :database-type "integer" :base-type :type/Integer :database-position 0}}} - (driver/describe-table :starburst (mt/db) (db/select-one 'Table :id (mt/id :venues))))))) + (driver/describe-table :starburst (mt/db) (t2/select-one 'Table :id (mt/id :venues))))))) (deftest table-rows-sample-test (mt/test-driver :starburst @@ -87,9 +87,9 @@ [3 "The Apple Pan"] [4 "Wurstküche"] [5 "Brite Spot Family Restaurant"]] - (->> (metadata-queries/table-rows-sample (db/select-one Table :id (mt/id :venues)) - [(db/select-one Field :id (mt/id :venues :id)) - (db/select-one Field :id (mt/id :venues :name))] + (->> (metadata-queries/table-rows-sample (t2/select-one Table :id (mt/id :venues)) + [(t2/select-one Field :id (mt/id :venues :id)) + (t2/select-one Field :id (mt/id :venues :name))] (constantly conj)) (sort-by first) (take 5)))))) @@ -194,7 +194,7 @@ ;; same as test_data, but with schema, so should NOT pick up venues, users, etc. (sync/sync-database! db) (is (= [{:name t, :schema s, :db_id (mt/id)}] - (map #(select-keys % [:name :schema :db_id]) (db/select Table :db_id (mt/id))))))) + (map #(select-keys % [:name :schema :db_id]) (t2/select Table :db_id (mt/id))))))) (execute-ddl! [(format "DROP TABLE %s.%s" s t) (format "DROP SCHEMA %s" s)])))))