Upgrade to Metabase 1.52

starburstdata · Jan 8, 2025 · a7bc1dd · a7bc1dd
1 parent 3be06fb
commit a7bc1dd
Show file tree

Hide file tree

Showing 10 changed files with 178 additions and 91 deletions.
diff --git a/.github/actions/init-dependencies/action.yml b/.github/actions/init-dependencies/action.yml
@@ -20,7 +20,7 @@ inputs:
   node-version:
     description: "Node version to setup"
     required: false
-    default: "16.x"
+    default: "18.x"
   clojure-version:
     description: "Clojure version to setup"
     required: true

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -49,6 +49,11 @@ jobs:
           java-version: ${{ matrix.runners.java_version }}
           java-architecture: ${{ matrix.runners.architecture }}
           clojure-version: ${{ env.CLOJURE_VERSION }}
+      - name: Initialize Metabase
+        run: |
+          cd metabase |
+          yarn build-static-viz |
+          cd ..
       - name: Build
         run: |
           make build

diff --git a/Makefile b/Makefile
@@ -11,80 +11,82 @@ trino_port := 8082
 is_trino_started := $(shell curl --fail --silent --insecure http://localhost:$(trino_port)/v1/info | jq '.starting')
 
 clone_metabase_if_missing:
-ifeq ($(wildcard $(makefile_dir)/metabase/.),)
+ifeq ($(wildcard $(makefile_dir)metabase/.),)
 	@echo "Did not find metabase repo, cloning version $(metabase_version)..."; git clone -b $(metabase_version) --depth 1 https://github.com/metabase/metabase.git
 else
 	@echo "Found metabase repo, skipping initialization."
 endif
 
 checkout_latest_metabase_tag: clone_metabase_if_missing clean
-	cd $(makefile_dir)/metabase; git fetch --all --tags;
-	$(eval latest_metabase_version=$(shell cd $(makefile_dir)/metabase; git tag | egrep 'v[0-9]+\.[0-9]+\.[0-9]+' | sort | tail -n 1))
+	cd $(makefile_dir)metabase; git fetch --all --tags;
+	$(eval latest_metabase_version=$(shell cd $(makefile_dir)metabase; git tag | egrep 'v[0-9]+\.[0-9]+\.[0-9]+' | sort | tail -n 1))
 	@echo "Checking out latest metabase tag: $(latest_metabase_version)"
-	cd $(makefile_dir)/metabase/modules/drivers && git checkout $(latest_metabase_version);
+	cd $(makefile_dir)metabase/modules/drivers && git checkout $(latest_metabase_version);
 	sed -i.bak 's/metabase\": \".*\"/metabase\": \"$(latest_metabase_version)\"/g' app_versions.json; rm  ./app_versions.json.bak
 
 start_trino_if_missing:
 ifeq ($(is_trino_started),)
 	@echo "Trino not started, starting using version $(trino_version)...";
-	cd $(makefile_dir)/resources/docker/trino; docker build -t trino-metabase-test . --build-arg version=$(trino_version); docker run --rm -d  -p $(trino_port):8080/tcp trino-metabase-test:latest
+	cd $(makefile_dir)resources/docker/trino; docker build -t trino-metabase-test . --build-arg version=$(trino_version); docker run --rm -d  -p $(trino_port):8080/tcp trino-metabase-test:latest
 else
 	@echo "Trino started, skipping initialization."
 endif
 
 clean:
 	@echo "Force cleaning Metabase repo..."
-	cd $(makefile_dir)/metabase/modules/drivers && git reset --hard && git clean -f
+	cd $(makefile_dir)metabase/modules/drivers && git reset --hard && git clean -f
 	@echo "Checking out metabase at: $(metabase_version)"
-	cd $(makefile_dir)/metabase/modules/drivers && git fetch --all --tags && git checkout $(metabase_version);
+	cd $(makefile_dir)metabase/modules/drivers && git fetch --all --tags && git checkout $(metabase_version);
 
 link_to_driver:
-ifeq ($(wildcard $(makefile_dir)/metabase/modules/drivers/starburst/src),)
-	@echo "Adding link to driver..."; ln -s ../../../drivers/starburst $(makefile_dir)/metabase/modules/drivers
+ifeq ($(wildcard $(makefile_dir)metabase/modules/drivers/starburst/src),)
+	@echo "Adding link to driver..."; ln -s ../../../drivers/starburst $(makefile_dir)metabase/modules/drivers
 else
 	@echo "Driver found, skipping linking."
 endif
 
 
 front_end:
 	@echo "Building Front End..."
-	cd $(makefile_dir)/metabase/; export WEBPACK_BUNDLE=production && yarn build-release && yarn build-static-viz
+	cd $(makefile_dir)metabase && yarn build-static-viz && \
+	export WEBPACK_BUNDLE=production && yarn build-release && yarn build-static-viz
 
 driver: update_deps_files
 	@echo "Building Starburst driver..."
-	cd $(makefile_dir)/metabase/; ./bin/build-driver.sh starburst
+	cd $(makefile_dir)metabase/; ./bin/build-driver.sh starburst
 
 server: 
 	@echo "Starting metabase..."
-	cd $(makefile_dir)/metabase/; clojure -M:run
+	cd $(makefile_dir)metabase/; clojure -M:run
 
 # This command adds the require starburst driver dependencies to the metabase repo.
 update_deps_files:
-	@if cd $(makefile_dir)/metabase && grep -q starburst deps.edn; \
+	@if cd $(makefile_dir)metabase && grep -q starburst deps.edn; \
 		then \
 			echo "Metabase deps file updated, skipping..."; \
 		else \
 			echo "Updating metabase deps file..."; \
-			cd $(makefile_dir)/metabase/; sed -i.bak 's/\/test\"\]\}/\/test\" \"modules\/drivers\/starburst\/test\"\]\}/g' deps.edn; \
+			cd $(makefile_dir)metabase/; sed -i.bak 's/\/test\"\]\}/\/test\" \"modules\/drivers\/starburst\/test\"\]\}/g' deps.edn; \
 	fi
 
-	@if cd $(makefile_dir)/metabase/modules/drivers && grep -q starburst deps.edn; \
+	@if cd $(makefile_dir)metabase/modules/drivers && grep -q starburst deps.edn; \
 		then \
 			echo "Metabase driver deps file updated, skipping..."; \
 		else \
 			echo "Updating metabase driver deps file..."; \
-			cd $(makefile_dir)/metabase/modules/drivers/; sed -i.bak "s/\}\}\}/\} \metabase\/starburst \{:local\/root \"starburst\"\}\}\}/g" deps.edn; \
+			cd $(makefile_dir)metabase/modules/drivers/; sed -i.bak "s/\}\}\}/\} \metabase\/starburst \{:local\/root \"starburst\"\}\}\}/g" deps.edn; \
 	fi
 
 test: start_trino_if_missing link_to_driver update_deps_files
 	@echo "Testing Starburst driver..."
-	cd $(makefile_dir)/metabase/; DRIVERS=starburst MB_STARBURST_TEST_PORT=$(trino_port) clojure -X:dev:drivers:drivers-dev:test
+	cp test_test.clj ./metabase/.clj-kondo/test/hooks/clojure/
+	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
 	@echo "Testing Starburst driver (explicitPrepare=true)..."
-	cd $(makefile_dir)/metabase/; DRIVERS=starburst MB_STARBURST_TEST_PORT=$(trino_port) clojure -J-DexplicitPrepare=false -X:dev:drivers:drivers-dev:test
+	cd $(makefile_dir)metabase/; DRIVERS=starburst MB_STARBURST_TEST_PORT=$(trino_port) clojure -J-DexplicitPrepare=false -X:dev:drivers:drivers-dev:test
 
 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/
+	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
@@ -19,6 +19,7 @@ build our `.jar` file into the correct dir, run tests, and start the local serve
 * [Clojure](https://clojure.org/guides/install_clojure)
 * [jq](https://stedolan.github.io/jq/download/)
 * [Metabase Prerequisites](https://www.metabase.com/docs/latest/developers-guide/build#install-the-prerequisites)
+* JDK 8 to 21. **JDK 22 will not work**
 
 ##### Quick Start
 Run `make build test` to build and run tests locally. If everything passes, you're good to go!
@@ -64,9 +65,9 @@ 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.
+*CAUTION*: the Metabase test file `./metabase/.clj-kondo/test/hooks/clojure/test_test.clj` is overridden by a modified version on the root directory (see the `Makefile`). This is because the test `check-driver-keywords-test` is not expected to work with the Starburst driver.
 
-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)
+Whenever upgrading the version of Metabase, `./test_test.clj` should be replaced with `./metabase/.clj-kondo/test/hooks/clojure/test_test.clj` with the `check-driver-keywords-test` test removed.
 
 ## References
 * [Encrypting Metabase Database Details](https://www.metabase.com/docs/latest/operations-guide/encrypting-database-details-at-rest.html)

diff --git a/app_versions.json b/app_versions.json
@@ -1,5 +1,5 @@
 {
     "trino": "431",
     "clojure": "1.11.1.1262",
-    "metabase": "v1.50.9"
+    "metabase": "v1.52.2.2"
 }
diff --git a/drivers/starburst/src/metabase/driver/implementation/driver_helpers.clj b/drivers/starburst/src/metabase/driver/implementation/driver_helpers.clj
@@ -34,12 +34,10 @@
                               :native-parameters               true
                               :expression-aggregations         true
                               :binning                         true
-                              :foreign-keys                    true
+                              ::concat-non-string-args          false
                               :datetime-diff                   true
                               :convert-timezone                true
                               :connection/multiple-databases   true
                               :metadata/key-constraints        false
                               :now                             true}]
   (defmethod driver/database-supports? [:starburst feature] [_ _ _] supported?))
-
-(defmethod driver/database-supports? [:starburst :foreign-keys] [_driver _feature _db] (not config/is-test?))
diff --git a/drivers/starburst/test/metabase/driver/starburst_test.clj b/drivers/starburst/test/metabase/driver/starburst_test.clj
@@ -28,13 +28,21 @@
             [metabase.models.table :as table :refer [Table]]
             [metabase.query-processor :as qp]
             [metabase.query-processor.compile :as qp.compile]
+            [metabase.query-processor-test.timezones-test :as timezones-test]
             [metabase.sync :as sync]
             [metabase.test :as mt]
             [metabase.test.fixtures :as fixtures]
+            [metabase.test.data.interface :as tx]
+            [metabase.test.data.sql-jdbc :as sql-jdbc.tx]
             [toucan2.tools.with-temp :as t2.with-temp]
             [toucan2.core :as t2]))
 
 (use-fixtures :once (fixtures/initialize :db))
+(sql-jdbc.tx/add-test-extensions! :starburst)
+
+(defmethod tx/before-run :starburst
+  [_]
+  (alter-var-root #'timezones-test/broken-drivers conj :starburst))
 
 (deftest describe-database-test
   (mt/test-driver :starburst
@@ -125,8 +133,8 @@
                     (mt/with-temporary-setting-values [report-timezone "Asia/Hong_Kong"]
         ;; the `read-column-thunk` for `Types/TIMESTAMP` used to return an `OffsetDateTime`, but since Metabase 1.50 it
         ;; returns a LocalDate
-                      (is (= [[(t/local-date 2014 8 2)
-                               (t/local-date 2014 8 2)]]
+                      (is (= [[(t/local-date "2014-08-02")
+                               (t/local-date "2014-08-02")]]
                              (mt/rows
                               (qp/process-query
                                {:database     (mt/id)
@@ -147,7 +155,7 @@
                       (is (= (str "SELECT COUNT(*) AS \"count\" "
                                   "FROM \"default\".\"test_data_venues\" "
                                   "WHERE \"default\".\"test_data_venues\".\"name\" = 'wow'")
-                             (:query (qp.compile/compile-and-splice-parameters query))
+                             (:query (qp.compile/compile-with-inline-parameters query))
                              (-> (qp/process-query query) :data :native_form :query)))))))
 
 (deftest connection-tests