[Drivers] New features to replace hard-coded driver lists in tests (#…

…42602)

- `:metadata/key-constraints` for those databases which formally track
  PK and FK key constraints. Some databases (eg. Presto/Athena/Starburst
  family) don't support constraints like that, but do support querying
  based on foreign keys (which is the `:foreign-keys` feature).
- `:connection/multiple-databases` for databases where a single
  connection can connect to many databases. This is the case for Athena,
  since it connects to an S3 bucket and exposes all files in the bucket
  as databases through that single connection.
- Also removed a troublesome "all DBs except a huge list" case in the
  SSH tunnel tests with a shorter list of supported drivers.

modules/drivers/athena/src/metabase/driver/athena.clj

@@ -32,10 +32,12 @@
 ;;; |                                          metabase.driver method impls                                          |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
-(doseq [[feature supported?] {:datetime-diff             true
-                              :foreign-keys              true
-                              :nested-fields             false
-                              :test/jvm-timezone-setting false}]
+(doseq [[feature supported?] {:datetime-diff                 true
+                              :foreign-keys                  true
+                              :nested-fields                 false
+                              :connection/multiple-databases true
+                              :metadata/key-constraints      false
+                              :test/jvm-timezone-setting     false}]
   (defmethod driver/database-supports? [:athena feature] [_driver _feature _db] supported?))
 
 ;;; +----------------------------------------------------------------------------------------------------------------+

modules/drivers/presto-jdbc/src/metabase/driver/presto_jdbc.clj

@@ -51,7 +51,8 @@
                               :native-parameters               true
                               :now                             true
                               :set-timezone                    true
-                              :standard-deviation-aggregations true}]
+                              :standard-deviation-aggregations true
+                              :metadata/key-constraints        false}]
   (defmethod driver/database-supports? [:presto-jdbc feature] [_driver _feature _db] supported?))
 
 ;;; Presto API helpers

modules/drivers/sparksql/src/metabase/driver/sparksql.clj

@@ -209,6 +209,7 @@
                               :native-parameters               true
                               :nested-queries                  true
                               :standard-deviation-aggregations true
+                              :metadata/key-constraints        false
                               :test/jvm-timezone-setting       false
                               ;; disabled for now, see issue #40991 to fix this.
                               :window-functions/cumulative     false}]

src/metabase/driver.clj

@@ -460,9 +460,15 @@
 
 (def features
   "Set of all features a driver can support."
-  #{;; Does this database support foreign key relationships?
+  #{;; Does this database support following foreign key relationships while querying?
+    ;; Note that this is different from supporting primary key and foreign key constraints in the schema; see below.
     :foreign-keys
 
+    ;; Does this database track and enforce primary key and foreign key constraints in the schema?
+    ;; SQL query engines like Presto and Athena do not track these, though they can query across FKs.
+    ;; See :foreign-keys above.
+    :metadata/key-constraints
+
     ;; Does this database support nested fields for any and every field except primary key (e.g. Mongo)?
     :nested-fields
 
@@ -598,6 +604,12 @@
     ;; Does the driver support fingerprint the fields. Default is true
     :fingerprint
 
+    ;; Does a connection to this driver correspond to a single database (false), or to multiple databases (true)?
+    ;; Default is false; ie. a single database. This is common for classic relational DBs and some cloud databases.
+    ;; Some have access to many databases from one connection; eg. Athena connects to an S3 bucket which might have
+    ;; many databases in it.
+    :connection/multiple-databases
+
     ;; Does this driver support window functions like cumulative count and cumulative sum? (default: false)
     :window-functions/cumulative
 

src/metabase/driver/sql.clj

@@ -27,6 +27,7 @@
                  :percentile-aggregations
                  :regex
                  :standard-deviation-aggregations
+                 :metadata/key-constraints
                  :window-functions/cumulative
                  :window-functions/offset]]
   (defmethod driver/database-supports? [:sql feature] [_driver _feature _db] true))

test/metabase/driver/sql_jdbc/connection_test.clj

@@ -21,7 +21,6 @@
    [metabase.test.data.interface :as tx]
    [metabase.test.fixtures :as fixtures]
    [metabase.test.util :as tu]
-   [metabase.timeseries-query-processor-test.util :as tqpt]
    [metabase.util :as u]
    [metabase.util.ssh :as ssh]
    [metabase.util.ssh-test :as ssh-test]
@@ -232,9 +231,16 @@
     (doto server (.start))))
 
 (deftest test-ssh-tunnel-connection
-  ;; sqlite cannot be behind a tunnel, h2 is tested below, unsure why others fail
-  (mt/test-drivers (apply disj (sql-jdbc.tu/sql-jdbc-drivers) :sqlite :h2 :oracle :vertica :presto-jdbc :bigquery-cloud-sdk :redshift :athena
-                          (tqpt/timeseries-drivers))
+  ;; TODO: Formerly this test ran against "all JDBC drivers except this big list":
+  ;; (apply disj (sql-jdbc.tu/sql-jdbc-drivers)
+  ;;        :sqlite :h2 :oracle :vertica :presto-jdbc :bigquery-cloud-sdk :redshift :athena
+  ;;        (tqpt/timeseries-drivers))
+  ;; which does not leave very many drivers!
+  ;; That form is not extensible by 3P driver authors who need to exclude their driver as well. Since some drivers
+  ;; (eg. Oracle) do seem to support SSH tunnelling but still fail on this test, it's not clear if this should be
+  ;; controlled by a driver feature, a ^:dynamic override, or something else.
+  ;; For now I'm making this test run against only `#{:postgres :mysql :snowflake}` like the below.
+  (mt/test-drivers #{:postgres :mysql :snowflake}
     (testing "ssh tunnel is established"
       (let [tunnel-db-details (assoc (:details (mt/db))
                                      :tunnel-enabled true
@@ -251,7 +257,7 @@
 
 (deftest test-ssh-tunnel-reconnection
   ;; for now, run against Postgres and mysql, although in theory it could run against many different kinds
-  (mt/test-drivers #{:postgres :mysql}
+  (mt/test-drivers #{:postgres :mysql :snowflake}
     (testing "ssh tunnel is reestablished if it becomes closed, so subsequent queries still succeed"
       (let [tunnel-db-details (assoc (:details (mt/db))
                                      :tunnel-enabled true

test/metabase/driver_test.clj

@@ -84,10 +84,7 @@
 
 (deftest can-connect-with-destroy-db-test
   (testing "driver/can-connect? should fail or throw after destroying a database"
-    (mt/test-drivers (->> (mt/normal-drivers)
-                          ;; athena is a special case because connections aren't made with a single database,
-                          ;; but to an S3 bucket that may contain many databases
-                          (remove #{:athena}))
+    (mt/test-drivers (mt/normal-drivers-without-feature :connection/multiple-databases)
       (let [database-name (mt/random-name)
             dbdef         (basic-db-definition database-name)]
         (mt/dataset dbdef
@@ -119,10 +116,7 @@
 
 (deftest check-can-connect-before-sync-test
   (testing "Database sync should short-circuit and fail if the database at the connection has been deleted (metabase#7526)"
-    (mt/test-drivers (->> (mt/normal-drivers)
-                          ;; athena is a special case because connections aren't made with a single database,
-                          ;; but to an S3 bucket that may contain many databases
-                          (remove #{:athena}))
+    (mt/test-drivers (mt/normal-drivers-without-feature :connection/multiple-databases)
       (let [database-name (mt/random-name)
             dbdef         (basic-db-definition database-name)]
         (mt/dataset dbdef

test/metabase/test/data/dataset_definition_test.clj

@@ -8,17 +8,10 @@
    [toucan2.core :as t2]))
 
 (deftest dataset-with-custom-pk-test
-  (mt/test-drivers (apply disj (mt/sql-jdbc-drivers)
-                          ;; presto doesn't create PK for test data :) not sure why
-                          :presto-jdbc
-                          ;; creating db for athena is expensive and require some extra steps,
-                          ;; so it's not worth testing against, see
-                          ;; the[[metabase.test.data.athena/*allow-database-creation*]]
-                          :athena
-                          ;; there is no PK in sparksql
-                          :sparksql
-                          ;; Timeseries drivers currently support only testing with pre-loaded dataset
-                          (tqpt/timeseries-drivers))
+  (mt/test-drivers (->> (mt/normal-drivers-with-feature :metadata/key-constraints)
+                        (filter (mt/sql-jdbc-drivers))
+                        ;; Timeseries drivers currently support only testing with pre-loaded dataset
+                        (remove (tqpt/timeseries-drivers)))
     (mt/dataset (mt/dataset-definition "custom-pk"
                   ["user"
                    [{:field-name "custom_id" :base-type :type/Integer :pk? true}]
@@ -51,17 +44,10 @@
     [[1 2]]]])
 
 (deftest dataset-with-custom-composite-pk-test
-  (mt/test-drivers (apply disj (mt/sql-jdbc-drivers)
-                          ;; presto doesn't create PK for test data :) not sure why
-                          :presto-jdbc
-                          ;; creating db for athena is expensive and require some extra steps,
-                          ;; so it's not worth testing against, see
-                          ;; the [[metabase.test.data.athena/*allow-database-creation*]]
-                          :athena
-                          ;; there is no PK in sparksql
-                          :sparksql
-                          ;; Timeseries drivers currently support only testing with pre-loaded dataset
-                          (tqpt/timeseries-drivers))
+  (mt/test-drivers (->> (mt/normal-drivers-with-feature :metadata/key-constraints)
+                        (filter (mt/sql-jdbc-drivers))
+                        ;; Timeseries drivers currently support only testing with pre-loaded dataset
+                        (remove (tqpt/timeseries-drivers)))
     (mt/dataset composite-pk
       (let [format-name #(ddl.i/format-name driver/*driver* %)]
         (testing "(artist_id, song_id) is a PK"