Add Metabase tests #20

bobbyiliev · 2023-07-31T13:48:48Z

This is the initial PR that integrates the Materialize driver with the Metabase tests.

joacoc

Approving it since I don't have any refutation to anything, only questions to understand how it works and gain some knowledge.

joacoc · 2023-08-16T10:07:05Z

test/metabase/test/data/materialize.clj

+  [driver {:keys [database-name]}]
+  (format "DROP DATABASE IF EXISTS \"%s\";" (ddl.i/format-name driver database-name)))
+
+(defmethod sql.tx/add-fk-sql :materialize [& _] nil)


What would happen if Metabase tries to add a FK in Materialize? Is that a possible scenario?

The key part is this config in the driver itself:

(doseq [[feature supported?] {:foreign-keys (not config/is-test?)

By default Metabase assumes that it needs foreign keys in order to do joins, but this is not the case for Materialize and some other DB drivers that they have. So we disable this only during the tests, as otherwise the Metatabase test suite tries to add FKs on the tables which we don't support.

resources/metabase-plugin.yaml

joacoc · 2023-08-16T10:09:23Z

docker-compose.yml

+    image: postgres:15.3-alpine3.18
+    depends_on:
+      - materialize
+    command: sh -c 'echo "Waiting for materialized to start..." && sleep 15 && psql -h materialize -U mz_system -d materialize -p 6877 -c "ALTER SYSTEM SET max_tables = 1000;"'


Is this only because Metabase create many tables during testing? Does Metabase also creates tables in prod?

This is only for the tests. Metabase creates a lot of tables when initializing the tests. Then it uses the test data to run all the tests.

CONTRIBUTING.md

joacoc · 2023-08-16T10:14:04Z

src/metabase/driver/materialize.clj

@@ -3,29 +3,71 @@
  (:require [clojure


Is it correct to say they we use exactly the Postgres Driver and disable all the troublemaker options (set-timezone, datetime-diff, etc.)?

Yep exactly! We first register the Materialize driver and define Postgres as the parent driver, eg:

(driver/register! :materialize, :parent :postgres)

It is very similar to how the Redshift driver is setup as well.

test/metabase/driver/materialize_test.clj

joacoc · 2023-08-16T10:17:38Z

.github/workflows/tests.yaml

+        run: yarn build-static-viz
+
+      # Use custom deps.edn containing "user/materialize" alias to include driver sources
+      - name: Run tests


The test takes 12 minutes because it tests all the drivers?

No, it only tests Materialize as we add the DRIVERS=materialize before the clojure command. It's just that there are too many tests, around 15k assertions.

.github/workflows/tests.yaml

Co-authored-by: Joaquin Colacci <[email protected]>

metamben · 2023-08-16T18:59:18Z

.github/workflows/tests.yaml

+      - name: Setup Node
+        uses: actions/setup-node@v2
+        with:
+          node-version: "18"


Metabase 46 uses node 16, starting with 47 we use node 18. (For building static-viz this might not matter though.)

Thank you for pointing that out. I've gone ahead and reverted back to node 16 for the time being.

metamben · 2023-08-16T19:07:57Z

CONTRIBUTING.md

+
+```bash
+mkdir -p ~/.clojure
+cat modules/drivers/materialize/.github/deps.edn | sed -e "s|PWD|$PWD|g" > ~/.clojure/deps.edn


I wouldn't recommend executing this. Many Clojure developers customize their personal deps.edn file.

Good catch! I've changed the instructions accordingly.

metamben · 2023-08-16T19:24:30Z

src/metabase/driver/materialize.clj

+            [metabase [config :as config] [driver :as driver] [util :as u]]
+            [metabase.driver.sql-jdbc.execute :as sql-jdbc.execute]
+            [metabase.driver.sql.query-processor :as sql.qp]
+            [metabase.util.honey-sql-2 :as h2x]


Just a question of style: it's unconventional to use hierarchical :requires.

Good point! I've updated this accordingly.

metamben · 2023-08-16T19:33:01Z

src/metabase/driver/materialize.clj

  (sql-jdbc.common/handle-additional-options
   (merge
    {:classname                     "org.postgresql.Driver"
     :subprotocol                   "postgresql"
-     :subname                       (str "//" host ":" port "/" db)
-     :ssl                           true
+     :subname                       (str "//" host ":" port "/" db "?options=--cluster%3D" cluster)


Can these values be sanitized?

Just updated this, let me know if this is what you had in mind

Given that host, port, etc. are user input, we cannot trust them to be safe. We construct a string from them and pass that to the driver. Since we probably cannot fully trust the driver either, we should do everything reasonable to make sure there is nothing suspicious/bad in that string.

Do you have any suggestions for best practices?
I tried to check how other similar drivers handle this like Redshift here and some other partner drivers like Exasol and ClickHouse but I belive that they also just construct a string and pass it to the driver.

It depends on the underlying driver. I'm guessing that

the resulting string should be a valid URI,

host should be a host name or IP address,

port should be an integer,

db should be a valid database name, and

cluster should be a valid cluster name.

Currently out of ideas on a good way to fulfill this request, as this seems to be the way that it's currently handled with all other drivers:

https://github.com/metabase/metabase/blob/master/src/metabase/db/spec.clj#L24-L29

test/metabase/driver/materialize_test.clj

metamben · 2023-08-16T19:38:11Z

test/metabase/test/data/materialize.clj

+
+(set! *warn-on-reflection* true)
+
+(defmethod driver/database-supports? [:materialize :foreign-keys] [_driver _feature _db] (not config/is-test?))


It's odd to have this method defined in two places.

Good catch! Removed the duplicate one!

metamben · 2023-08-16T19:43:05Z

test/metabase/test/data/materialize.clj

+   (merge
+    ((get-method tx/aggregate-column-info ::tx/test-extensions) driver ag-type field)
+    (when (= ag-type :sum)
+      {:base_type :type/BigInteger}))))


Suggested change

(merge

((get-method tx/aggregate-column-info ::tx/test-extensions) driver ag-type field)

(when (= ag-type :sum)

{:base_type :type/BigInteger}))))

(cond-> ((get-method tx/aggregate-column-info ::tx/test-extensions) driver ag-type field)

(= ag-type :sum) (assoc :base_type :type/BigInteger))))

Thank you for this suggestion!

metamben · 2023-08-16T20:09:43Z

test/metabase/test/data/materialize.clj

+                             :type/JSON           "JSON"
+                             :type/Time           "TIME"
+                             :type/UUID           "UUID"}]
+  (defmethod sql.tx/field-base-type->sql-type [:materialize base-type] [_ _] db-type))


Aren't these inherited from the postgres driver?

If I don't specify those, one of the tests is failing with the following error:

clojure.lang.ExceptionInfo: Failed to create :materialize 'office-checkins' test database: No test data type mapping for driver :materialize for base type :type/DateTimeWithZoneOffset; add an impl for field-base-type->sql-type. database-name: "office-checkins" driver: :materialize java.lang.Exception: No test data type mapping for driver :materialize for base type :type/DateTimeWithZoneOffset; add an impl for field-base-type->sql-type.

src/metabase/driver/materialize.clj

Co-authored-by: metamben <[email protected]>

metamben · 2023-08-18T23:15:38Z

src/metabase/driver/materialize.clj

+(defn- validate-connection-details
+  [{:keys [host]}]
+  (when-not (re-matches #"^[a-zA-Z0-9.-]+$" host)
+    (throw (IllegalArgumentException. (str "Invalid host: " host)))))


I assume port should be either an integer or a string encoding one.

Add Metabase tests

db1398c

bobbyiliev force-pushed the metabase-tests branch from 0d9472c to db1398c Compare July 31, 2023 14:19

bobbyiliev added 5 commits July 31, 2023 17:29

Add dpes.edn file

bdf6d9f

Update the dpes.edn file

4bb49f4

Add default test datasets

7f45b5b

dislabe ssl for test container

8e8e088

Try to disable set-timezone

999d214

bobbyiliev force-pushed the metabase-tests branch from da5e6bc to 999d214 Compare August 2, 2023 15:27

bobbyiliev added 14 commits August 2, 2023 18:30

Add init container to increase max_tables

65abc00

Disable setting timezone

feb2aba

Try to fix expression-using-aggregation-test

81eabed

Fixing tests

377406f

Disable percentile-aggregations due to missing func

221afa2

Add json type

5834e27

Fix weeks interval tests

c64e15c

Fix weeks interval tests

a1dbc6f

Upgrade node and clojure versions

99f784e

Upgrade Metabase version

cd06813

Change jdk from 17 to 11

f210deb

Exclude materialize from faulty timezone tests

4c7ba86

Disable :temporal-extract

79acf7a

Re enable :temporal-extract

94f36be

bobbyiliev force-pushed the metabase-tests branch from e41abbd to 94f36be Compare August 14, 2023 20:02

bobbyiliev added 7 commits August 14, 2023 23:30

Re enable :temporal-extract

de8d99d

Disable :temporal-extract during tests

1a5471a

Disable test with failing FK

625a86a

Disable test with failing FK

a68ef44

Fix failing FK tests

68f1f20

Disable test due to inconsistent results

fc1a80f

Fix git diff apply

0e79404

bobbyiliev added 2 commits August 15, 2023 15:49

Add cluster conn option

cd56201

Add CONTRIBUTING.md file

3fbbcaf

bobbyiliev mentioned this pull request Aug 15, 2023

Update the README.md file #21

Closed

Update readme and add build script

f1d7695

bobbyiliev marked this pull request as ready for review August 15, 2023 13:48

bobbyiliev requested review from dehume, joacoc and morsapaes August 15, 2023 13:49

joacoc approved these changes Aug 16, 2023

View reviewed changes

joacoc reviewed Aug 16, 2023

View reviewed changes

.github/workflows/tests.yaml Outdated Show resolved Hide resolved

bobbyiliev and others added 4 commits August 16, 2023 14:55

Update resources/metabase-plugin.yaml

2dd304e

Co-authored-by: Joaquin Colacci <[email protected]>

Small fixes

5d3a884

Update contributing.md file

5140f9e

Small fixes

91f2919

metamben reviewed Aug 16, 2023

View reviewed changes

bobbyiliev added 2 commits August 17, 2023 16:51

PR change requests

9193399

Sanitize the conn details

1922569

bobbyiliev requested a review from metamben August 17, 2023 15:25

Update cluster display name

9aab1bb

metamben reviewed Aug 17, 2023

View reviewed changes

src/metabase/driver/materialize.clj Outdated Show resolved Hide resolved

bobbyiliev and others added 3 commits August 18, 2023 09:44

Update src/metabase/driver/materialize.clj

7f26855

Co-authored-by: metamben <[email protected]>

Add hostname and port validation

dee4f9b

Remove port validation

dc9d4a6

bobbyiliev requested a review from metamben August 18, 2023 16:24

metamben approved these changes Aug 18, 2023

View reviewed changes

Add release version table

51c7c2e

bobbyiliev merged commit 54c7e18 into master Aug 21, 2023

bobbyiliev deleted the metabase-tests branch August 21, 2023 14:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Metabase tests #20

Add Metabase tests #20

bobbyiliev commented Jul 31, 2023 •

edited

Loading

joacoc left a comment

joacoc Aug 16, 2023

bobbyiliev Aug 16, 2023

joacoc Aug 16, 2023

bobbyiliev Aug 16, 2023

joacoc Aug 16, 2023

bobbyiliev Aug 16, 2023

joacoc Aug 16, 2023

bobbyiliev Aug 16, 2023

metamben Aug 16, 2023

bobbyiliev Aug 17, 2023

metamben Aug 16, 2023

bobbyiliev Aug 17, 2023

metamben Aug 16, 2023

bobbyiliev Aug 17, 2023

metamben Aug 16, 2023

bobbyiliev Aug 17, 2023

metamben Aug 17, 2023

bobbyiliev Aug 18, 2023

metamben Aug 18, 2023

bobbyiliev Aug 18, 2023

metamben Aug 16, 2023

bobbyiliev Aug 17, 2023

metamben Aug 16, 2023

bobbyiliev Aug 17, 2023

metamben Aug 16, 2023

bobbyiliev Aug 17, 2023

metamben Aug 18, 2023


		(set! warn-on-reflection true)

		(defmethod driver/database-supports? [:materialize :foreign-keys] [_driver _feature _db] (not config/is-test?))

Add Metabase tests #20

Add Metabase tests #20

Conversation

bobbyiliev commented Jul 31, 2023 • edited Loading

joacoc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobbyiliev commented Jul 31, 2023 •

edited

Loading