diff --git a/modules/drivers/presto/src/metabase/driver/presto.clj b/modules/drivers/presto/src/metabase/driver/presto.clj
index 1cf1dfabb16c21c6c83653324103fa648c0be5f8..dae2693c9cf67b75f68b682aa7da7f450c24609e 100644
--- a/modules/drivers/presto/src/metabase/driver/presto.clj
+++ b/modules/drivers/presto/src/metabase/driver/presto.clj
@@ -19,7 +19,7 @@
[metabase.driver.sql
[query-processor :as sql.qp]
[util :as sql.u]]
- [metabase.driver.sql-jdbc.sync :as sql-jdbc.sync]
+ [metabase.driver.sql-jdbc.sync.describe-database :as sql-jdbc.describe-database]
[metabase.driver.sql.util.unprepare :as unprepare]
[metabase.query-processor
[context :as context]
@@ -199,12 +199,14 @@
(def ^:private presto-metadata-sync-query-timeout
(u/minutes->ms 2))
-(defmethod sql-jdbc.sync/execute-query-for-sync :presto
- [_ details query]
+(defn- execute-query-for-sync
+ [details query]
(let [result-chan (a/promise-chan)
query (if (string? query)
query
- (first query))]
+ (do
+ (assert (empty? (second query)) "Presto doesn't allow parameterized queries")
+ (first query)))]
(execute-presto-query details query nil (fn [cols rows]
(a/>!! result-chan {:cols cols, :rows rows})))
(let [[val] (a/alts!! [result-chan (a/timeout presto-metadata-sync-query-timeout)])]
@@ -215,7 +217,7 @@
(s/defmethod driver/can-connect? :presto
[driver {:keys [catalog] :as details} :- PrestoConnectionDetails]
- (let [{[[v]] :rows} (sql-jdbc.sync/execute-query-for-sync :presto details
+ (let [{[[v]] :rows} (execute-query-for-sync details
(format "SHOW SCHEMAS FROM %s LIKE 'information_schema'"
(sql.u/quote-name driver :database catalog)))]
(= v "information_schema")))
@@ -228,14 +230,22 @@
"Return a set of all schema names in this `database`."
[driver {{:keys [catalog schema] :as details} :details :as database}]
(let [sql (str "SHOW SCHEMAS FROM " (sql.u/quote-name driver :database catalog))
- {:keys [rows]} (sql-jdbc.sync/execute-query-for-sync :presto details sql)]
+ {:keys [rows]} (execute-query-for-sync details sql)]
(set (map first rows))))
+(defn- have-select-privilege? [driver details schema table-name]
+ (try
+ (let [sql-args (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
+ (execute-query-for-sync details sql-args)
+ true)
+ (catch Throwable _
+ false)))
+
(defn- describe-schema [driver {{:keys [catalog user] :as details} :details :as db} {:keys [schema]}]
(let [sql (str "SHOW TABLES FROM " (sql.u/quote-name driver :schema catalog schema))]
- (set (for [[table-name & _] (:rows (sql-jdbc.sync/execute-query-for-sync :presto details sql))
- :when (sql-jdbc.sync/have-select-privilege? driver details {:table_schem schema
- :table_name table-name})]
+ (set (for [[table-name & _] (:rows (execute-query-for-sync details sql))
+ :when (have-select-privilege? driver details schema table-name)]
{:name table-name
:schema schema}))))
@@ -250,7 +260,7 @@
(defmethod driver/describe-table :presto
[driver {{:keys [catalog] :as details} :details} {schema :schema, table-name :name}]
(let [sql (str "DESCRIBE " (sql.u/quote-name driver :table catalog schema table-name))
- {:keys [rows]} (sql-jdbc.sync/execute-query-for-sync :presto details sql)]
+ {:keys [rows]} (execute-query-for-sync details sql)]
{:schema schema
:name table-name
:fields (set (for [[idx [name type]] (m/indexed rows)]
diff --git a/modules/drivers/presto/test/metabase/test/data/presto.clj b/modules/drivers/presto/test/metabase/test/data/presto.clj
index 3105e8d49fe11c499894a30400ef066fc8eff1d9..e89297395e47f690e1ca6fcfb87cc08472ae71d8 100644
--- a/modules/drivers/presto/test/metabase/test/data/presto.clj
+++ b/modules/drivers/presto/test/metabase/test/data/presto.clj
@@ -9,7 +9,6 @@
[config :as config]
[driver :as driver]]
[metabase.driver.presto :as presto]
- [metabase.driver.sql-jdbc.sync :as sql-jdbc.sync]
[metabase.driver.sql.util :as sql.u]
[metabase.driver.sql.util.unprepare :as unprepare]
[metabase.test.data
@@ -95,7 +94,7 @@
(defmethod tx/create-db! :presto
[driver {:keys [table-definitions database-name] :as dbdef} & {:keys [skip-drop-db?]}]
(let [details (tx/dbdef->connection-details driver :db dbdef)
- execute! (partial sql-jdbc.sync/execute-query-for-sync :presto details)]
+ execute! (partial #'presto/execute-query-for-sync details)]
(doseq [tabledef table-definitions
:let [rows (:rows tabledef)
;; generate an ID for each row because we don't have auto increments
@@ -111,7 +110,7 @@
(defmethod tx/destroy-db! :presto
[driver {:keys [database-name table-definitions], :as dbdef}]
(let [details (tx/dbdef->connection-details driver :db dbdef)
- execute! (partial sql-jdbc.sync/execute-query-for-sync :presto details)]
+ execute! (partial #'presto/execute-query-for-sync details)]
(doseq [{:keys [table-name], :as tabledef} table-definitions]
(println (format "[Presto] destroying %s.%s" (pr-str database-name) (pr-str table-name)))
(execute! (sql.tx/drop-table-if-exists-sql driver dbdef tabledef))
diff --git a/modules/drivers/redshift/test/metabase/driver/redshift_test.clj b/modules/drivers/redshift/test/metabase/driver/redshift_test.clj
index 3ae404cf51718e9a8c3ea8ab2395d4c25589b7e5..9caeb9ce2eef3cf286003e2e6875398172095c55 100644
--- a/modules/drivers/redshift/test/metabase/driver/redshift_test.clj
+++ b/modules/drivers/redshift/test/metabase/driver/redshift_test.clj
@@ -10,17 +10,21 @@
[metabase.driver.sql-jdbc.execute :as execute]
[metabase.plugins.jdbc-proxy :as jdbc-proxy]
[metabase.test.data.redshift :as rstest]
- [metabase.test.fixtures :as fixtures]))
+ [metabase.test.fixtures :as fixtures])
+ (:import metabase.plugins.jdbc_proxy.ProxyDriver))
(use-fixtures :once (fixtures/initialize :plugins))
(use-fixtures :once (fixtures/initialize :db))
(deftest correct-driver-test
- (mt/test-driver
- :redshift
- (is (= "com.amazon.redshift.jdbc.Driver"
- (.getName (class (jdbc-proxy/wrapped-driver (java.sql.DriverManager/getDriver "jdbc:redshift://host:5432/testdb")))))
- "Make sure we're using the correct driver for Redshift")))
+ (mt/test-driver :redshift
+ (testing "Make sure we're using the correct driver for Redshift"
+ (let [driver (java.sql.DriverManager/getDriver "jdbc:redshift://host:5432/testdb")
+ driver (if (instance? metabase.plugins.jdbc_proxy.ProxyDriver driver)
+ (jdbc-proxy/wrapped-driver driver)
+ driver)]
+ (is (= "com.amazon.redshift.jdbc.Driver"
+ (.getName (class driver))))))))
(defn- query->native [query]
(let [native-query (atom nil)]
diff --git a/modules/drivers/snowflake/src/metabase/driver/snowflake.clj b/modules/drivers/snowflake/src/metabase/driver/snowflake.clj
index 121260f356ede53af40e708cd6653ba1206a9051..e17a087f2d97b0bfd92b281b945d9b96f66f40f5 100644
--- a/modules/drivers/snowflake/src/metabase/driver/snowflake.clj
+++ b/modules/drivers/snowflake/src/metabase/driver/snowflake.clj
@@ -25,7 +25,7 @@
[metabase.util
[date-2 :as u.date]
[honeysql-extensions :as hx]
- [i18n :refer [tru]]])
+ [i18n :refer [trs tru]]])
(:import [java.sql ResultSet Types]
[java.time OffsetDateTime ZonedDateTime]
metabase.util.honeysql_extensions.Identifier))
@@ -46,7 +46,6 @@
[_]
:sunday)
-
(defmethod sql-jdbc.conn/connection-details->spec :snowflake
[_ {:keys [account regionid], :as opts}]
(let [host (if regionid
@@ -248,24 +247,31 @@
excluded-schemas (set (sql-jdbc.sync/excluded-schemas driver))]
(qp.store/with-store
(qp.store/fetch-and-store-database! (u/get-id database))
- {:tables (set (for [table (jdbc/query
- (sql-jdbc.conn/db->pooled-connection-spec database)
- (format "SHOW OBJECTS IN DATABASE \"%s\"" db-name))
- :when (and (not (contains? excluded-schemas (:schema_name table)))
- (sql-jdbc.sync/have-select-privilege? driver database
- {:table_name (:name table)
- :table_schem (:schema_name table)}))]
- {:name (:name table)
- :schema (:schema_name table)
- :description (not-empty (:comment table))}))})))
+ (let [spec (sql-jdbc.conn/db->pooled-connection-spec database)
+ sql (format "SHOW OBJECTS IN DATABASE \"%s\"" db-name)]
+ (with-open [conn (jdbc/get-connection spec)]
+ {:tables (into
+ #{}
+ (comp (filter (fn [{schema :schema_name, table-name :name}]
+ (and (not (contains? excluded-schemas schema))
+ (sql-jdbc.sync/have-select-privilege? driver conn schema table-name))))
+ (map (fn [{schema :schema_name, table-name :name, remark :comment}]
+ {:name table-name
+ :schema schema
+ :description (not-empty remark)})))
+ (try
+ (jdbc/reducible-query {:connection conn} sql)
+ (catch Throwable e
+ (throw (ex-info (trs "Error executing query") {:sql sql} e)))))})))))
(defmethod driver/describe-table :snowflake
[driver database table]
- (jdbc/with-db-metadata [metadata (sql-jdbc.conn/db->pooled-connection-spec database)]
- (->> (assoc (select-keys table [:name :schema])
- :fields (sql-jdbc.sync/describe-table-fields metadata driver table (db-name database)))
- ;; find PKs and mark them
- (sql-jdbc.sync/add-table-pks metadata))))
+ (let [spec (sql-jdbc.conn/db->pooled-connection-spec database)]
+ (with-open [conn (jdbc/get-connection spec)]
+ (->> (assoc (select-keys table [:name :schema])
+ :fields (sql-jdbc.sync/describe-table-fields driver conn table (db-name database)))
+ ;; find PKs and mark them
+ (sql-jdbc.sync/add-table-pks (.getMetaData conn))))))
(defmethod driver/describe-table-fks :snowflake
[driver database table]
diff --git a/modules/drivers/sqlite/src/metabase/driver/sqlite.clj b/modules/drivers/sqlite/src/metabase/driver/sqlite.clj
index 1c0c27d32f736a871fc3da4a971c16f801a8aa04..3adaf3a751b1330b4bd5e7ec80f0ab57ae6fcdea 100644
--- a/modules/drivers/sqlite/src/metabase/driver/sqlite.clj
+++ b/modules/drivers/sqlite/src/metabase/driver/sqlite.clj
@@ -82,6 +82,14 @@
[_ database-type]
(database-type->base-type database-type))
+;; The normal SELECT * FROM table WHERE 1 <> 1 LIMIT 0 query doesn't return any information for SQLite views -- it
+;; seems to be the case that the query has to return at least one row
+(defmethod sql-jdbc.sync/fallback-metadata-query :sqlite
+ [driver schema table]
+ (sql.qp/format-honeysql driver {:select [:*]
+ :from [(sql.qp/->honeysql driver (hx/identifier :table schema table))]
+ :limit 1}))
+
;; register the SQLite concatnation operator `||` with HoneySQL as `sqlite-concat`
;; (hsql/format (hsql/call :sqlite-concat :a :b)) -> "(a || b)"
(defmethod hformat/fn-handler "sqlite-concat"
diff --git a/modules/drivers/sqlite/test/metabase/driver/sqlite_test.clj b/modules/drivers/sqlite/test/metabase/driver/sqlite_test.clj
index de820df30221bde3ce942338c544bc1dbfaa1939..bdc060cfb89405ca16490fe60aadd95d2d4f93aa 100644
--- a/modules/drivers/sqlite/test/metabase/driver/sqlite_test.clj
+++ b/modules/drivers/sqlite/test/metabase/driver/sqlite_test.clj
@@ -6,18 +6,13 @@
[query-processor-test :as qp.test]
[sync :as sync]
[test :as mt]]
- [metabase.driver.sql-jdbc
- [connection :as sql-jdbc.conn]
- [execute :as sql-jdbc.execute]]
+ [metabase.driver.sql-jdbc.connection :as sql-jdbc.conn]
[metabase.models
[database :refer [Database]]
- [field :refer [Field]]
[table :refer [Table]]]
- [metabase.query-processor-test :as qp.test]
[metabase.test
[data :as data]
[util :as tu]]
- [metabase.test.data.datasets :as datasets]
[toucan
[db :as db]
[hydrate :refer [hydrate]]]))
@@ -59,8 +54,12 @@
(testing "Make sure we correctly infer complex types in views (#8630, #9276, #12191, #12547, #10681)"
(let [details (mt/dbdef->connection-details :sqlite :db {:database-name "views_test"})
spec (sql-jdbc.conn/connection-details->spec :sqlite details)]
- (mt/with-temp Database [{db-id :id :as database} {:engine :sqlite, :details (assoc details :dbname "viwes_test")}]
- (doseq [statement ["create table if not exists src(id integer, time text);"
+ (mt/with-temp Database [{db-id :id :as database} {:engine :sqlite, :details (assoc details :dbname "views_test")}]
+ (doseq [statement ["drop view if exists v_groupby_test;"
+ "drop table if exists groupby_test;"
+ "drop view if exists v_src;"
+ "drop table if exists src;"
+ "create table if not exists src(id integer, time text);"
"create view if not exists v_src as select id, strftime('%s', time) as time from src;"
"insert into src values(1, '2020-03-01 12:20:35');"]]
(jdbc/execute! spec [statement]))
@@ -77,8 +76,10 @@
:base_type :type/Text}]}]
(->> (hydrate (db/select Table :db_id db-id {:order-by [:name]}) :fields)
(map table-fingerprint))))
- (doseq [statement ["CREATE TABLE IF NOT EXISTS groupby_test (
- id INTEGER primary key unique,
+ (doseq [statement ["drop view if exists v_groupby_test;"
+ "drop table if exists groupby_test;"
+ "CREATE TABLE IF NOT EXISTS groupby_test (
+ id INTEGER primary key unique,
symbol VARCHAR,
dt DATETIME,
value FLOAT);"
diff --git a/src/metabase/driver/sql/query_processor.clj b/src/metabase/driver/sql/query_processor.clj
index 5a407a48adf93c8f0c80a74be2d420f9931ddf6d..95b9fd030088ee19d9bf5cc893d9498a3bb0776a 100644
--- a/src/metabase/driver/sql/query_processor.clj
+++ b/src/metabase/driver/sql/query_processor.clj
@@ -17,6 +17,7 @@
[field :as field :refer [Field]]
[table :refer [Table]]]
[metabase.query-processor
+ [error-type :as qp.error-type]
[interface :as i]
[store :as qp.store]]
[metabase.query-processor.middleware
@@ -24,7 +25,7 @@
[wrap-value-literals :as value-literal]]
[metabase.util
[honeysql-extensions :as hx]
- [i18n :refer [deferred-tru]]
+ [i18n :refer [deferred-tru tru]]
[schema :as su]]
[potemkin.types :as p.types]
[pretty.core :refer [PrettyPrintable]]
@@ -815,7 +816,11 @@
"\n"
(u/pprint-to-str honeysql-form))))
(finally
- (throw e))))))
+ (throw (ex-info (tru "Error compiling HoneySQL form")
+ {:driver driver
+ :form honeysql-form
+ :type qp.error-type/driver}
+ e)))))))
(defn- add-default-select
"Add `SELECT *` to `honeysql-form` if no `:select` clause is present."
diff --git a/src/metabase/driver/sql_jdbc/execute.clj b/src/metabase/driver/sql_jdbc/execute.clj
index 596d5a82547091fe8227545e4a4d60e9dbd9d25e..4e01ea6be940ca518341cf094c4599b0b8cd2cfc 100644
--- a/src/metabase/driver/sql_jdbc/execute.clj
+++ b/src/metabase/driver/sql_jdbc/execute.clj
@@ -11,10 +11,9 @@
[metabase
[driver :as driver]
[util :as u]]
- [metabase.driver.sql-jdbc
- [connection :as sql-jdbc.conn]
- [sync :as sql-jdbc.sync]]
+ [metabase.driver.sql-jdbc.connection :as sql-jdbc.conn]
[metabase.driver.sql-jdbc.execute.old-impl :as execute.old]
+ [metabase.driver.sql-jdbc.sync.interface :as sql-jdbc.sync]
[metabase.mbql.util :as mbql.u]
[metabase.query-processor
[context :as context]
diff --git a/src/metabase/driver/sql_jdbc/sync.clj b/src/metabase/driver/sql_jdbc/sync.clj
index 8f0b3d8701181c358d9f533b94518818ec4a1aa7..0cd279bd902a93f9048070a582ce61bf590530b0 100644
--- a/src/metabase/driver/sql_jdbc/sync.clj
+++ b/src/metabase/driver/sql_jdbc/sync.clj
@@ -1,268 +1,31 @@
(ns metabase.driver.sql-jdbc.sync
"Implementations for sync-related driver multimethods for SQL JDBC drivers, using JDBC DatabaseMetaData."
- (:require [clojure
- [set :as set]
- [string :as str]]
- [clojure.java.jdbc :as jdbc]
- [clojure.tools.logging :as log]
- [medley.core :as m]
- [metabase
- [driver :as driver]
- [util :as u]]
- [metabase.driver.sql-jdbc.connection :as sql-jdbc.conn]
- [metabase.driver.sql.query-processor :as sql.qp]
- [metabase.util.honeysql-extensions :as hx])
- (:import (java.sql Connection DatabaseMetaData ResultSetMetaData)))
-
-;;; +----------------------------------------------------------------------------------------------------------------+
-;;; | Interface (Multimethods) |
-;;; +----------------------------------------------------------------------------------------------------------------+
-
-(defmulti active-tables
- "Return a set of maps containing information about the active tables/views, collections, or equivalent that currently
- exist in a database. Each map should contain the key `:name`, which is the string name of the table. For databases
- that have a concept of schemas, this map should also include the string name of the table's `:schema`.
-
- Two different implementations are provided in this namespace: `fast-active-tables` (the default), and
- `post-filtered-active-tables`. You should be fine using the default, but refer to the documentation for those
- functions for more details on the differences.
-
- `metabase` is an instance of `DatabaseMetaData`."
- {:arglists '([driver database metadata])}
- driver/dispatch-on-initialized-driver
- :hierarchy #'driver/hierarchy)
-
-(declare fast-active-tables)
-
-(defmethod active-tables :sql-jdbc [driver database metadata]
- (fast-active-tables driver database metadata))
-
-
-(defmulti excluded-schemas
- "Return set of string names of schemas to skip syncing tables from."
- {:arglists '([driver])}
- driver/dispatch-on-initialized-driver
- :hierarchy #'driver/hierarchy)
-
-(defmethod excluded-schemas :sql-jdbc [_] nil)
-
-
-;; TODO - why don't we just use JDBC `DatabaseMetaData` to do this? This is wacky
-(defmulti database-type->base-type
- "Given a native DB column type (as a keyword), return the corresponding `Field` `base-type`, which should derive from
- `:type/*`. You can use `pattern-based-database-type->base-type` in this namespace to implement this using regex
- patterns."
- {:arglists '([driver database-type])}
- driver/dispatch-on-initialized-driver
- :hierarchy #'driver/hierarchy)
-
-
-(defmulti column->special-type
- "Attempt to determine the special-type of a field given the column name and native type. For example, the Postgres
- driver can mark Postgres JSON type columns as `:type/SerializedJSON` special type.
-
- `database-type` and `column-name` will be strings."
- {:arglists '([driver database-type column-name])}
- driver/dispatch-on-initialized-driver
- :hierarchy #'driver/hierarchy)
-
-(defmethod column->special-type :sql-jdbc [_ _ _] nil)
-
-
-;;; +----------------------------------------------------------------------------------------------------------------+
-;;; | Common |
-;;; +----------------------------------------------------------------------------------------------------------------+
-
-(defn pattern-based-database-type->base-type
- "Return a `database-type->base-type` function that matches types based on a sequence of pattern / base-type pairs."
- [pattern->type]
- (fn [column-type]
- (let [column-type (name column-type)]
- (loop [[[pattern base-type] & more] pattern->type]
- (cond
- (re-find pattern column-type) base-type
- (seq more) (recur more))))))
-
-(defn- ->spec [db-or-id-or-spec]
- (if (u/id db-or-id-or-spec)
- (sql-jdbc.conn/db->pooled-connection-spec db-or-id-or-spec)
- db-or-id-or-spec))
-
-
-;;; +----------------------------------------------------------------------------------------------------------------+
-;;; | Sync Impl |
-;;; +----------------------------------------------------------------------------------------------------------------+
-
-(defn- db-tables
- "Fetch a JDBC Metadata ResultSet of tables in the DB, optionally limited to ones belonging to a given
- schema."
- [driver ^DatabaseMetaData metadata ^String schema-or-nil ^String db-name-or-nil]
- ;; tablePattern "%" = match all tables
- (with-open [rs (.getTables metadata db-name-or-nil (driver/escape-entity-name-for-metadata driver schema-or-nil) "%"
- (into-array String ["TABLE" "VIEW" "FOREIGN TABLE" "MATERIALIZED VIEW" "EXTERNAL TABLE"]))]
- (into []
- (map (fn [row] (select-keys row [:table_name :remarks :table_schem])))
- (jdbc/reducible-result-set rs {}))))
-
-(defn- simple-select-probe
- "Simple (ie. cheap) SELECT on a given table to test for access and get column metadata."
- [driver schema table]
- ;; Using our SQL compiler here to get portable LIMIT
- (sql.qp/format-honeysql driver
- (sql.qp/apply-top-level-clause driver :limit
- {:select [:*]
- :from [(sql.qp/->honeysql driver (hx/identifier :table schema table))]}
- {:limit 1})))
-
-(defmulti execute-query-for-sync
- "Execute given SQL query. Used during parts of sync where we don't have metadata populated and
- can't use our default query processing pipeline."
- {:arglists '([driver db-or-id-or-spec query])}
- driver/dispatch-on-initialized-driver
- :hierarchy #'driver/hierarchy)
-
-(defmethod execute-query-for-sync :sql-jdbc
- [_ db-or-id-or-spec query]
- (jdbc/query (sql-jdbc.conn/db->pooled-connection-spec db-or-id-or-spec) query))
-
-(defn have-select-privilege?
- "Check if we have select privilege for given table"
- [driver db-or-id-or-spec {:keys [table_name table_schem] :as table}]
- (when (u/ignore-exceptions
- (execute-query-for-sync driver db-or-id-or-spec
- (simple-select-probe driver table_schem table_name)))
- table))
-
-(defn- all-schemas
- [^DatabaseMetaData metadata]
- (with-open [rs (.getSchemas metadata)]
- (set (map :table_schem (jdbc/result-set-seq rs)))))
-
-(defn fast-active-tables
- "Default, fast implementation of `active-tables` best suited for DBs with lots of system tables (like Oracle). Fetch
- list of schemas, then for each one not in `excluded-schemas`, fetch its Tables, and combine the results.
-
- This is as much as 15x faster for Databases with lots of system tables than `post-filtered-active-tables` (4 seconds
- vs 60)."
- [driver, db-or-id-or-spec, ^DatabaseMetaData metadata, & [db-name-or-nil]]
- (->> (set/difference (all-schemas metadata) (excluded-schemas driver))
- (mapcat (fn [schema]
- (db-tables driver metadata schema db-name-or-nil)))
- (filter (partial have-select-privilege? driver db-or-id-or-spec))))
-
-(defn post-filtered-active-tables
- "Alternative implementation of `active-tables` best suited for DBs with little or no support for schemas. Fetch *all*
- Tables, then filter out ones whose schema is in `excluded-schemas` Clojure-side."
- [driver, db-or-id-or-spec, ^DatabaseMetaData metadata, & [db-name-or-nil]]
- (filter (every-pred (partial have-select-privilege? driver db-or-id-or-spec)
- (comp not (partial contains? (excluded-schemas driver)) :table_schem))
- (db-tables driver metadata nil db-name-or-nil)))
-
-(defn get-catalogs
- "Returns a set of all of the catalogs found via `metadata`"
- [^DatabaseMetaData metadata]
- (with-open [rs (.getCatalogs metadata)]
- (set (map :table_cat (jdbc/metadata-result rs)))))
-
-(defn- database-type->base-type-or-warn
- "Given a `database-type` (e.g. `VARCHAR`) return the mapped Metabase type (e.g. `:type/Text`)."
- [driver database-type]
- (or (database-type->base-type driver (keyword database-type))
- (do (log/warn (format "Don't know how to map column type '%s' to a Field base_type, falling back to :type/*."
- database-type))
- :type/*)))
-
-(defn- calculated-special-type
- "Get an appropriate special type for a column with `column-name` of type `database-type`."
- [driver, ^String column-name, ^String database-type]
- (when-let [special-type (column->special-type driver database-type column-name)]
- (assert (isa? special-type :type/*)
- (str "Invalid type: " special-type))
- special-type))
-
-(defn- fields-metadata
- [^DatabaseMetaData metadata, driver, {^String schema :schema, ^String table-name :name :as table}, & [^String db-name-or-nil]]
- (with-open [rs (.getColumns metadata
- db-name-or-nil
- (driver/escape-entity-name-for-metadata driver schema)
- (driver/escape-entity-name-for-metadata driver table-name)
- nil)]
- (let [result (jdbc/result-set-seq rs)]
- ;; In some rare cases `:column_name` is blank (eg. SQLite's views with group by),
- ;; fallback to sniffing the type from a SELECT * query
- (if (some (comp str/blank? :type_name) result)
- (jdbc/with-db-connection [conn (->spec (:db_id table))]
- (let [^Connection conn (:connection conn)
- ^ResultSetMetaData metadata (->> (simple-select-probe driver schema table-name)
- first
- (.executeQuery (.createStatement conn))
- (.getMetaData))]
- (doall
- (for [i (range 1 (inc (.getColumnCount metadata)))]
- {:type_name (.getColumnTypeName metadata i)
- :column_name (.getColumnName metadata i)}))))
- result))))
-
-(defn describe-table-fields
- "Returns a set of column metadata for `schema` and `table-name` using `metadata`. "
- [^DatabaseMetaData metadata, driver, table, & [^String db-name-or-nil]]
- (set
- (for [[idx {database-type :type_name
- column-name :column_name
- remarks :remarks}] (m/indexed (fields-metadata metadata driver table db-name-or-nil))]
- (merge
- {:name column-name
- :database-type database-type
- :base-type (database-type->base-type-or-warn driver database-type)
- :database-position idx}
- (when (not (str/blank? remarks))
- {:field-comment remarks})
- (when-let [special-type (calculated-special-type driver column-name database-type)]
- {:special-type special-type})))))
-
-(defn add-table-pks
- "Using `metadata` find any primary keys for `table` and assoc `:pk?` to true for those columns."
- [^DatabaseMetaData metadata, table]
- (with-open [rs (.getPrimaryKeys metadata nil nil (:name table))]
- (let [pks (set (map :column_name (jdbc/metadata-result rs)))]
- (update table :fields (fn [fields]
- (set (for [field fields]
- (if-not (contains? pks (:name field))
- field
- (assoc field :pk? true)))))))))
-
-
-;;; +----------------------------------------------------------------------------------------------------------------+
-;;; | Default SQL JDBC metabase.driver impls for sync methods |
-;;; +----------------------------------------------------------------------------------------------------------------+
-
-(defn describe-database
- "Default implementation of `driver/describe-database` for SQL JDBC drivers. Uses JDBC DatabaseMetaData."
- [driver db-or-id-or-spec]
- (jdbc/with-db-metadata [metadata (sql-jdbc.conn/db->pooled-connection-spec db-or-id-or-spec)]
- {:tables (set (for [table (active-tables driver db-or-id-or-spec metadata)]
- (let [remarks (:remarks table)]
- {:name (:table_name table)
- :schema (:table_schem table)
- :description (when-not (str/blank? remarks)
- remarks)})))}))
-
-(defn describe-table
- "Default implementation of `driver/describe-table` for SQL JDBC drivers. Uses JDBC DatabaseMetaData."
- [driver db-or-id-or-spec table]
- (jdbc/with-db-metadata [metadata (sql-jdbc.conn/db->pooled-connection-spec db-or-id-or-spec)]
- (->> (assoc (select-keys table [:name :schema]) :fields (describe-table-fields metadata driver table))
- ;; find PKs and mark them
- (add-table-pks metadata))))
-
-(defn describe-table-fks
- "Default implementation of `driver/describe-table-fks` for SQL JDBC drivers. Uses JDBC DatabaseMetaData."
- [driver db-or-id-or-spec table & [^String db-name-or-nil]]
- (jdbc/with-db-metadata [metadata (sql-jdbc.conn/db->pooled-connection-spec db-or-id-or-spec)]
- (with-open [rs (.getImportedKeys metadata db-name-or-nil, ^String (:schema table), ^String (:name table))]
- (set
- (for [result (jdbc/metadata-result rs)]
- {:fk-column-name (:fkcolumn_name result)
- :dest-table {:name (:pktable_name result)
- :schema (:pktable_schem result)}
- :dest-column-name (:pkcolumn_name result)})))))
+ (:require [metabase.driver.sql-jdbc.sync
+ [describe-database :as sync.describe-database]
+ [describe-table :as sync.describe-table]
+ [interface :as i]]
+ [potemkin :as p]))
+
+(comment i/keep-me sync.describe-database/keep-me sync.describe-table/keep-me)
+
+(p/import-vars
+ [i
+ active-tables
+ column->special-type
+ database-type->base-type
+ excluded-schemas
+ fallback-metadata-query
+ have-select-privilege?]
+
+ [sync.describe-table
+ add-table-pks
+ describe-table
+ describe-table-fields
+ describe-table-fks
+ get-catalogs
+ pattern-based-database-type->base-type]
+
+ [sync.describe-database
+ describe-database
+ fast-active-tables
+ post-filtered-active-tables])
diff --git a/src/metabase/driver/sql_jdbc/sync/common.clj b/src/metabase/driver/sql_jdbc/sync/common.clj
new file mode 100644
index 0000000000000000000000000000000000000000..b37cd0642104f1c2c441bd2afd2ab82abffe3f01
--- /dev/null
+++ b/src/metabase/driver/sql_jdbc/sync/common.clj
@@ -0,0 +1,40 @@
+(ns metabase.driver.sql-jdbc.sync.common
+ (:require [metabase.driver.sql-jdbc.execute :as sql-jdbc.execute])
+ (:import [java.sql Connection PreparedStatement ResultSet]))
+
+(defn prepare-statement
+ "Create a PreparedStatement for metadata queries; set `TYPE_FORWARD_ONLY`/`CONCUR_READ_ONLY`/`FETCH_FORWARD` options
+ if possible. These queries return no rows."
+ ^PreparedStatement [driver ^Connection conn ^String sql params]
+ ;; `sql-jdbc.execute/prepared-statement` will set `TYPE_FORWARD_ONLY`/`CONCUR_READ_ONLY`/`FETCH_FORWARD` if
+ ;; possible, although I'm not sure if that will make a difference if we don't actually realize the ResultSet
+ (doto ^PreparedStatement (sql-jdbc.execute/prepared-statement driver conn sql params)
+ (.setMaxRows 0)))
+
+(defn reducible-results
+ "Creates an `IReduceInit` for a function that returns a `ResultSet`, and a function that is called once for each row.
+ `rs-thunk` should return a `ResultSet`; `rs->row-thunk` has the signature
+
+ (rs->row-thunk rs)-> row-thunk
+
+ `rs->row-thunk` is called once with the ResultSet, and should return a thunk; the resulting thunk is called once for
+ each row. Example:
+
+ (reducible-results
+ ;; `rs-thunk` should return a `ResultSet`
+ #(.getSchemas metadata)
+ ;; `rs->row-thunk` is called once with the `ResultSet`, and returns a thunk
+ (fn [rs]
+ ;; the thunk is called once for each row to get results
+ (fn []
+ (.getString rs \"TABLE_SCHEM\"))))"
+ [rs-thunk rs->row-thunk]
+ (reify clojure.lang.IReduceInit
+ (reduce [_ rf init]
+ (with-open [^ResultSet rs (rs-thunk)]
+ (reduce
+ ((take-while some?) rf)
+ init
+ (let [row-thunk (rs->row-thunk rs)]
+ (repeatedly #(when (.next rs)
+ (row-thunk)))))))))
diff --git a/src/metabase/driver/sql_jdbc/sync/describe_database.clj b/src/metabase/driver/sql_jdbc/sync/describe_database.clj
new file mode 100644
index 0000000000000000000000000000000000000000..838c415cc41533e71831e262d15c781e0c55a514
--- /dev/null
+++ b/src/metabase/driver/sql_jdbc/sync/describe_database.clj
@@ -0,0 +1,124 @@
+(ns metabase.driver.sql-jdbc.sync.describe-database
+ "SQL JDBC impl for `describe-database`."
+ (:require [clojure.java.jdbc :as jdbc]
+ [clojure.string :as str]
+ [metabase.driver :as driver]
+ [metabase.driver.sql-jdbc
+ [connection :as sql-jdbc.conn]
+ [execute :as sql-jdbc.execute]]
+ [metabase.driver.sql-jdbc.sync
+ [common :as common]
+ [interface :as i]]
+ [metabase.driver.sql.query-processor :as sql.qp]
+ [metabase.util.honeysql-extensions :as hx])
+ (:import [java.sql Connection DatabaseMetaData ResultSet]))
+
+(defmethod i/excluded-schemas :sql-jdbc [_] nil)
+
+(defn- all-schemas [^DatabaseMetaData metadata]
+ {:pre [(instance? DatabaseMetaData metadata)]}
+ (common/reducible-results
+ #(.getSchemas metadata)
+ (fn [^ResultSet rs]
+ #(.getString rs "TABLE_SCHEM"))))
+
+(defn- syncable-schemas
+ [driver metadata]
+ (eduction (remove (set (i/excluded-schemas driver)))
+ (all-schemas metadata)))
+
+(defn simple-select-probe-query
+ "Simple (ie. cheap) SELECT on a given table to test for access and get column metadata. Doesn't return
+ anything useful (only used to check whether we can execute a SELECT query)
+
+ (simple-select-probe-query :postgres \"public\" \"my_table\")
+ ;; -> [\"SELECT TRUE FROM public.my_table WHERE 1 <> 1 LIMIT 0\"]"
+ [driver schema table]
+ {:pre [(string? table)]}
+ ;; Using our SQL compiler here to get portable LIMIT (e.g. `SELECT TOP n ...` for SQL Server/Oracle)
+ (let [honeysql {:select [[(sql.qp/->honeysql driver true) :_]]
+ :from [(sql.qp/->honeysql driver (hx/identifier :table schema table))]
+ :where [:not= 1 1]}
+ honeysql (sql.qp/apply-top-level-clause driver :limit honeysql {:limit 0})]
+ (sql.qp/format-honeysql driver honeysql)))
+
+(defn- execute-select-probe-query
+ "Execute the simple SELECT query defined above. The main goal here is to check whether we're able to execute a SELECT
+ query against the Table in question -- we don't care about the results themselves -- so the query and the logic
+ around executing it should be as simple as possible. We need to highly optimize this logic because it's executed for
+ every Table on every sync."
+ [driver ^Connection conn [sql & params]]
+ {:pre [(string? sql)]}
+ (with-open [stmt (common/prepare-statement driver conn sql params)]
+ ;; attempting to execute the SQL statement will throw an Exception if we don't have permissions; otherwise it will
+ ;; truthy wheter or not it returns a ResultSet, but we can ignore that since we have enough info to proceed at
+ ;; this point.
+ (.execute stmt)))
+
+(defmethod i/have-select-privilege? :sql-jdbc
+ [driver conn table-schema table-name]
+ ;; Query completes = we have SELECT privileges
+ ;; Query throws some sort of no permissions exception = no SELECT privileges
+ (let [sql-args (simple-select-probe-query driver table-schema table-name)]
+ (try
+ (execute-select-probe-query driver conn sql-args)
+ true
+ (catch Throwable _
+ false))))
+
+(defn- db-tables
+ "Fetch a JDBC Metadata ResultSet of tables in the DB, optionally limited to ones belonging to a given
+ schema. Returns a reducible sequence of results."
+ [driver ^DatabaseMetaData metadata ^String schema-or-nil ^String db-name-or-nil]
+ (common/reducible-results
+ #(.getTables metadata db-name-or-nil (some->> schema-or-nil (driver/escape-entity-name-for-metadata driver)) "%"
+ (into-array String ["TABLE" "VIEW" "FOREIGN TABLE" "MATERIALIZED VIEW" "EXTERNAL TABLE"]))
+ (fn [^ResultSet rs]
+ (fn []
+ {:name (.getString rs "TABLE_NAME")
+ :schema (.getString rs "TABLE_SCHEM")
+ :description (when-let [remarks (.getString rs "REMARKS")]
+ (when-not (str/blank? remarks)
+ remarks))}))))
+
+(defn fast-active-tables
+ "Default, fast implementation of `active-tables` best suited for DBs with lots of system tables (like Oracle). Fetch
+ list of schemas, then for each one not in `excluded-schemas`, fetch its Tables, and combine the results.
+
+ This is as much as 15x faster for Databases with lots of system tables than `post-filtered-active-tables` (4 seconds
+ vs 60)."
+ [driver ^Connection conn & [db-name-or-nil]]
+ {:pre [(instance? Connection conn)]}
+ (let [metadata (.getMetaData conn)]
+ (eduction
+ (comp (mapcat (fn [schema]
+ (db-tables driver metadata schema db-name-or-nil)))
+ (filter (fn [{table-schema :schema, table-name :name}]
+ (i/have-select-privilege? driver conn table-schema table-name))))
+ (syncable-schemas driver metadata))))
+
+(defmethod i/active-tables :sql-jdbc
+ [driver connection]
+ (fast-active-tables driver connection))
+
+(defn post-filtered-active-tables
+ "Alternative implementation of `active-tables` best suited for DBs with little or no support for schemas. Fetch *all*
+ Tables, then filter out ones whose schema is in `excluded-schemas` Clojure-side."
+ [driver ^Connection conn & [db-name-or-nil]]
+ {:pre [(instance? Connection conn)]}
+ (eduction
+ (filter (let [excluded (i/excluded-schemas driver)]
+ (fn [{table-schema :schema, table-name :name}]
+ (and (not (contains? excluded table-schema))
+ (i/have-select-privilege? driver conn table-schema table-name)))))
+ (db-tables driver (.getMetaData conn) nil db-name-or-nil)))
+
+(defn describe-database
+ "Default implementation of `driver/describe-database` for SQL JDBC drivers. Uses JDBC DatabaseMetaData."
+ [driver db-or-id-or-spec]
+ {:tables (with-open [conn (jdbc/get-connection (sql-jdbc.conn/db->pooled-connection-spec db-or-id-or-spec))]
+ ;; try to set the Connection to `READ_UNCOMMITED` if possible, or whatever the next least-locking level
+ ;; is. Not sure how much of a difference that makes since we're not running this inside a transaction,
+ ;; but better safe than sorry
+ (sql-jdbc.execute/set-best-transaction-level! driver conn)
+ (into #{} (i/active-tables driver conn)))})
diff --git a/src/metabase/driver/sql_jdbc/sync/describe_table.clj b/src/metabase/driver/sql_jdbc/sync/describe_table.clj
new file mode 100644
index 0000000000000000000000000000000000000000..497aa87a3b9146056e182d489d55896991d6a532
--- /dev/null
+++ b/src/metabase/driver/sql_jdbc/sync/describe_table.clj
@@ -0,0 +1,193 @@
+(ns metabase.driver.sql-jdbc.sync.describe-table
+ "SQL JDBC impl for `describe-table` and `describe-table-fks`."
+ (:require [clojure.java.jdbc :as jdbc]
+ [clojure.string :as str]
+ [clojure.tools.logging :as log]
+ [medley.core :as m]
+ [metabase
+ [driver :as driver]
+ [util :as u]]
+ [metabase.driver.sql-jdbc.connection :as sql-jdbc.conn]
+ [metabase.driver.sql-jdbc.sync
+ [common :as common]
+ [interface :as i]]
+ [metabase.driver.sql.query-processor :as sql.qp]
+ [metabase.util.honeysql-extensions :as hx])
+ (:import [java.sql Connection DatabaseMetaData ResultSet]))
+
+(defmethod i/column->special-type :sql-jdbc [_ _ _] nil)
+
+(defn pattern-based-database-type->base-type
+ "Return a `database-type->base-type` function that matches types based on a sequence of pattern / base-type pairs.
+ `pattern->type` is a map of regex pattern to MBQL type keyword."
+ [pattern->type]
+ (fn database-type->base-type [column-type]
+ (let [column-type (name column-type)]
+ (some
+ (fn [[pattern base-type]]
+ (when (re-find pattern column-type)
+ base-type))
+ pattern->type))))
+
+(defn get-catalogs
+ "Returns a set of all of the catalogs found via `metadata`"
+ [^DatabaseMetaData metadata]
+ (with-open [rs (.getCatalogs metadata)]
+ (set (map :table_cat (jdbc/metadata-result rs)))))
+
+(defn- database-type->base-type-or-warn
+ "Given a `database-type` (e.g. `VARCHAR`) return the mapped Metabase type (e.g. `:type/Text`)."
+ [driver database-type]
+ (or (i/database-type->base-type driver (keyword database-type))
+ (do (log/warn (format "Don't know how to map column type '%s' to a Field base_type, falling back to :type/*."
+ database-type))
+ :type/*)))
+
+(defn- calculated-special-type
+ "Get an appropriate special type for a column with `column-name` of type `database-type`."
+ [driver ^String column-name ^String database-type]
+ (when-let [special-type (i/column->special-type driver database-type column-name)]
+ (assert (isa? special-type :type/*)
+ (str "Invalid type: " special-type))
+ special-type))
+
+(defmethod i/fallback-metadata-query :sql-jdbc
+ [driver schema table]
+ {:pre [(string? table)]}
+ ;; Using our SQL compiler here to get portable LIMIT (e.g. `SELECT TOP n ...` for SQL Server/Oracle)
+ (let [honeysql {:select [:*]
+ :from [(sql.qp/->honeysql driver (hx/identifier :table schema table))]
+ :where [:not= 1 1]}
+ honeysql (sql.qp/apply-top-level-clause driver :limit honeysql {:limit 0})]
+ (sql.qp/format-honeysql driver honeysql)))
+
+(defn- fallback-fields-metadata-from-select-query
+ "In some rare cases `:column_name` is blank (eg. SQLite's views with group by) fallback to sniffing the type from a
+ SELECT * query."
+ [driver ^Connection conn table-schema table-name]
+ ;; some DBs (:sqlite) don't actually return the correct metadata for LIMIT 0 queries
+ (let [[sql & params] (i/fallback-metadata-query driver table-schema table-name)]
+ (reify clojure.lang.IReduceInit
+ (reduce [_ rf init]
+ (with-open [stmt (common/prepare-statement driver conn sql params)
+ rs (.executeQuery stmt)]
+ (let [metadata (.getMetaData rs)]
+ (reduce
+ ((map (fn [^Integer i]
+ {:name (.getColumnName metadata i)
+ :database-type (.getColumnTypeName metadata i)})) rf)
+ init
+ (range 1 (inc (.getColumnCount metadata))))))))))
+
+(defn- jdbc-fields-metadata
+ "Reducible metadata about the Fields belonging to a Table, fetching using JDBC DatabaseMetaData methods."
+ [driver ^Connection conn db-name-or-nil schema table-name]
+ (common/reducible-results #(.getColumns (.getMetaData conn)
+ db-name-or-nil
+ (some->> schema (driver/escape-entity-name-for-metadata driver))
+ (some->> table-name (driver/escape-entity-name-for-metadata driver))
+ nil)
+ (fn [^ResultSet rs]
+ #(merge
+ {:name (.getString rs "COLUMN_NAME")
+ :database-type (.getString rs "TYPE_NAME")}
+ (when-let [remarks (.getString rs "REMARKS")]
+ (when-not (str/blank? remarks)
+ {:field-comment remarks}))))))
+
+(defn- fields-metadata
+ "Returns reducible metadata for the Fields in a `table`."
+ [driver ^Connection conn {schema :schema, table-name :name} & [^String db-name-or-nil]]
+ {:pre [(instance? Connection conn) (string? table-name)]}
+ (reify clojure.lang.IReduceInit
+ (reduce [_ rf init]
+ ;; 1. Return all the Fields that come back from DatabaseMetaData that include type info.
+ ;;
+ ;; 2. Iff there are some Fields that don't have type info, concatenate
+ ;; `fallback-fields-metadata-from-select-query`, which fetches the same Fields using a different method.
+ ;;
+ ;; 3. Filter out any duplicates between the two methods using `m/distinct-by`.
+ (let [has-fields-without-type-info? (volatile! false)
+ jdbc-metadata (eduction
+ (remove (fn [{:keys [database-type]}]
+ (when (str/blank? database-type)
+ (vreset! has-fields-without-type-info? true)
+ true)))
+ (jdbc-fields-metadata driver conn db-name-or-nil schema table-name))
+ fallback-metadata (reify clojure.lang.IReduceInit
+ (reduce [_ rf init]
+ (reduce
+ rf
+ init
+ (when @has-fields-without-type-info?
+ (fallback-fields-metadata-from-select-query driver conn schema table-name)))))]
+ ;; VERY IMPORTANT! DO NOT REWRITE THIS TO BE LAZY! IT ONLY WORKS BECAUSE AS NORMAL-FIELDS GETS REDUCED,
+ ;; HAS-FIELDS-WITHOUT-TYPE-INFO? WILL GET SET TO TRUE IF APPLICABLE AND THEN FALLBACK-FIELDS WILL RUN WHEN
+ ;; IT'S TIME TO START EVALUATING THAT.
+ (reduce
+ ((comp cat (m/distinct-by :name)) rf)
+ init
+ [jdbc-metadata fallback-metadata])))))
+
+(defn describe-table-fields
+ "Returns a set of column metadata for `table` using JDBC Connection `conn`."
+ [driver conn table & [db-name-or-nil]]
+ (into
+ #{}
+ (map-indexed (fn [i {:keys [database-type], column-name :name, :as col}]
+ (merge
+ (u/select-non-nil-keys col [:name :database-type :field-comment])
+ {:base-type (database-type->base-type-or-warn driver database-type)
+ :database-position i}
+ (when-let [special-type (calculated-special-type driver column-name database-type)]
+ {:special-type special-type}))))
+ (fields-metadata driver conn table db-name-or-nil)))
+
+(defn add-table-pks
+ "Using `metadata` find any primary keys for `table` and assoc `:pk?` to true for those columns."
+ [^DatabaseMetaData metadata table]
+ (let [pks (into #{} (common/reducible-results #(.getPrimaryKeys metadata nil nil (:name table))
+ (fn [^ResultSet rs]
+ #(.getString rs "COLUMN_NAME"))))]
+ (update table :fields (fn [fields]
+ (set (for [field fields]
+ (if-not (contains? pks (:name field))
+ field
+ (assoc field :pk? true))))))))
+
+(defn- describe-table* [driver ^Connection conn table]
+ {:pre [(instance? Connection conn)]}
+ (->> (assoc (select-keys table [:name :schema])
+ :fields (describe-table-fields driver conn table))
+ ;; find PKs and mark them
+ (add-table-pks (.getMetaData conn))))
+
+(defn describe-table
+ "Default implementation of `driver/describe-table` for SQL JDBC drivers. Uses JDBC DatabaseMetaData."
+ [driver db-or-id-or-spec-or-conn table]
+ (if (instance? Connection db-or-id-or-spec-or-conn)
+ (describe-table* driver db-or-id-or-spec-or-conn table)
+ (let [spec (sql-jdbc.conn/db->pooled-connection-spec db-or-id-or-spec-or-conn)]
+ (with-open [conn (jdbc/get-connection spec)]
+ (describe-table* driver conn table)))))
+
+(defn- describe-table-fks*
+ [driver ^Connection conn {^String schema :schema, ^String table-name :name} & [^String db-name-or-nil]]
+ (into
+ #{}
+ (common/reducible-results #(.getImportedKeys (.getMetaData conn) db-name-or-nil schema table-name)
+ (fn [^ResultSet rs]
+ (fn []
+ {:fk-column-name (.getString rs "FKCOLUMN_NAME")
+ :dest-table {:name (.getString rs "PKTABLE_NAME")
+ :schema (.getString rs "PKTABLE_SCHEM")}
+ :dest-column-name (.getString rs "PKCOLUMN_NAME")})))))
+
+(defn describe-table-fks
+ "Default implementation of `driver/describe-table-fks` for SQL JDBC drivers. Uses JDBC DatabaseMetaData."
+ [driver db-or-id-or-spec-or-conn table & [db-name-or-nil]]
+ (if (instance? Connection db-or-id-or-spec-or-conn)
+ (describe-table-fks* driver db-or-id-or-spec-or-conn table db-name-or-nil)
+ (let [spec (sql-jdbc.conn/db->pooled-connection-spec db-or-id-or-spec-or-conn)]
+ (with-open [conn (jdbc/get-connection spec)]
+ (describe-table-fks* driver conn table db-name-or-nil)))))
diff --git a/src/metabase/driver/sql_jdbc/sync/interface.clj b/src/metabase/driver/sql_jdbc/sync/interface.clj
new file mode 100644
index 0000000000000000000000000000000000000000..4f82f6ad972638334e3b89b816c9c0d5d518836d
--- /dev/null
+++ b/src/metabase/driver/sql_jdbc/sync/interface.clj
@@ -0,0 +1,56 @@
+(ns metabase.driver.sql-jdbc.sync.interface
+ (:require [metabase.driver :as driver]))
+
+(defmulti active-tables
+ "Return a reducible sequence of maps containing information about the active tables/views, collections, or equivalent
+ that currently exist in a database. Each map should contain the key `:name`, which is the string name of the table.
+ For databases that have a concept of schemas, this map should also include the string name of the table's `:schema`.
+
+ Two different implementations are provided in this namespace: `fast-active-tables` (the default), and
+ `post-filtered-active-tables`. You should be fine using the default, but refer to the documentation for those
+ functions for more details on the differences.
+
+ `metabase` is an instance of `DatabaseMetaData`."
+ {:arglists '([driver ^java.sql.Connection connection])}
+ driver/dispatch-on-initialized-driver
+ :hierarchy #'driver/hierarchy)
+
+(defmulti excluded-schemas
+ "Return set of string names of schemas to skip syncing tables from."
+ {:arglists '([driver])}
+ driver/dispatch-on-initialized-driver
+ :hierarchy #'driver/hierarchy)
+
+(defmulti have-select-privilege?
+ "Check if we have SELECT privileges for given `table`."
+ {:arglists '([driver ^java.sql.Connection connection ^String table-schema ^String table-name])}
+ driver/dispatch-on-initialized-driver
+ :hierarchy #'driver/hierarchy)
+
+(defmulti database-type->base-type
+ "Given a native DB column type (as a keyword), return the corresponding `Field` `base-type`, which should derive from
+ `:type/*`. You can use `pattern-based-database-type->base-type` in this namespace to implement this using regex
+ patterns."
+ {:arglists '([driver database-type])}
+ driver/dispatch-on-initialized-driver
+ :hierarchy #'driver/hierarchy)
+
+(defmulti column->special-type
+ "Attempt to determine the special-type of a field given the column name and native type. For example, the Postgres
+ driver can mark Postgres JSON type columns as `:type/SerializedJSON` special type.
+
+ `database-type` and `column-name` will be strings."
+ {:arglists '([driver database-type column-name])}
+ driver/dispatch-on-initialized-driver
+ :hierarchy #'driver/hierarchy)
+
+(defmulti fallback-metadata-query
+ "SELECT columns from a given table so we can get column metadata. By default doesn't return any rows. This can be
+ overriden because SQLite is silly and only returns column information for views if the query returns a non-zero
+ number of rows.
+
+ (fallback-metadata-query :postgres \"public\" \"my_table\")
+ ;; -> [\"SELECT * FROM public.my_table WHERE 1 <> 1 LIMIT 0\"]"
+ {:arglists '([driver schema table])}
+ driver/dispatch-on-initialized-driver
+ :hierarchy #'driver/hierarchy)
diff --git a/src/metabase/plugins/jdbc_proxy.clj b/src/metabase/plugins/jdbc_proxy.clj
index a04db1d7a8c5a4e256c4a927bb0f542d5c322f60..852b2d96771c6f7fd8082efa91c53ef8e84eccd9 100644
--- a/src/metabase/plugins/jdbc_proxy.clj
+++ b/src/metabase/plugins/jdbc_proxy.clj
@@ -11,6 +11,7 @@
;;; -------------------------------------------------- Proxy Driver --------------------------------------------------
+;; TODO -- why not use `java.sql.Wrapper` here instead of defining a new protocol that basically does the same thing?
(p.types/defprotocol+ ^:private ProxyDriver
(wrapped-driver [this]
"Get the JDBC driver wrapped by a Metabase JDBC proxy driver."))
@@ -42,17 +43,28 @@
(.jdbcCompliant driver))))
(defn create-and-register-proxy-driver!
- "Create a new JDBC proxy driver to wrap driver with `class-name`. Registers the driver with JDBC, and deregisters the
- class it wraps if that class is already registered."
+ "Create a new JDBC proxy driver to wrap driver with `class-name`, but only if that class WAS NOT loaded by the System
+ ClassLoader. Registers the driver with JDBC, and deregisters the class it wraps if that class is already
+ registered.
+
+ This is necessary because the DriverManager will not recognize any drivers that are *NOT* loaded by the System
+ ClassLoader."
[^String class-name]
- (let [klass (Class/forName class-name true (classloader/the-classloader))
- driver (proxy-driver (.newInstance klass))]
- (log/debug (u/format-color 'blue (trs "Registering JDBC proxy driver for {0}..." klass)))
- (DriverManager/registerDriver driver)
-
- ;; deregister the non-proxy version of the driver so it doesn't try to handle our URLs. Most JDBC drivers register
- ;; themseleves when the classes are loaded
- (doseq [driver (enumeration-seq (DriverManager/getDrivers))
- :when (instance? klass driver)]
- (log/debug (u/format-color 'cyan (trs "Deregistering original JDBC driver {0}..." driver)))
- (DriverManager/deregisterDriver driver))))
+ (let [klass (Class/forName class-name true (classloader/the-classloader))
+ loaded-by-system-classloader? (identical? (.getClassLoader klass) (ClassLoader/getSystemClassLoader))]
+ ;; if the System ClassLoader loaded this class, don't create the proxy driver, because that can break things in
+ ;; some situations -- Oracle for example doesn't seem to behave properly when you do this. This mainly affects dev
+ ;; which merges driver dependencies into the core project deps.
+ (if loaded-by-system-classloader?
+ (log/debug (u/format-color 'cyan (trs "Not creating proxy JDBC driver for class {0} -- original driver was loaded by system ClassLoader"
+ class-name)))
+ (let [driver (proxy-driver (.newInstance klass))]
+ (log/debug (u/format-color 'blue (trs "Registering JDBC proxy driver for {0}..." class-name)))
+ (DriverManager/registerDriver driver)
+
+ ;; deregister the non-proxy version of the driver so it doesn't try to handle our URLs. Most JDBC drivers register
+ ;; themseleves when the classes are loaded
+ (doseq [driver (enumeration-seq (DriverManager/getDrivers))
+ :when (instance? klass driver)]
+ (log/debug (u/format-color 'cyan (trs "Deregistering original JDBC driver {0}..." driver)))
+ (DriverManager/deregisterDriver driver))))))
diff --git a/src/metabase/sync/interface.clj b/src/metabase/sync/interface.clj
index b280d076dc0eaf3744c44fc5dc9e7335a2bc89df..41e5b04000e67e3c3e4612a156e57dbdf76b3f40 100644
--- a/src/metabase/sync/interface.clj
+++ b/src/metabase/sync/interface.clj
@@ -20,7 +20,6 @@
"Schema for the expected output of `describe-database`."
{:tables #{DatabaseMetadataTable}})
-
(def TableMetadataField
"Schema for a given Field as provided in `describe-table`."
{:name su/NonBlankString
diff --git a/src/metabase/sync/sync_metadata/fields/fetch_metadata.clj b/src/metabase/sync/sync_metadata/fields/fetch_metadata.clj
index 622eb2af33b778d4de895a43c4a35fe5aa25402d..81104e479d8a45efc65f07570e747047486bb5e8 100644
--- a/src/metabase/sync/sync_metadata/fields/fetch_metadata.clj
+++ b/src/metabase/sync/sync_metadata/fields/fetch_metadata.clj
@@ -80,7 +80,7 @@
;;; +----------------------------------------------------------------------------------------------------------------+
(s/defn db-metadata :- #{i/TableMetadataField}
- "Fetch metadata about Fields belonging to a given TABLE directly from an external database by calling its
- driver's implementation of `describe-table`."
- [database :- i/DatabaseInstance, table :- i/TableInstance]
+ "Fetch metadata about Fields belonging to a given `table` directly from an external database by calling its driver's
+ implementation of `describe-table`."
+ [database :- i/DatabaseInstance table :- i/TableInstance]
(:fields (fetch-metadata/table-metadata database table)))
diff --git a/src/metabase/util.clj b/src/metabase/util.clj
index e9519c8819e282481c2d699e008c7b505ee00320..232e5aa57b95de56ba1726e056c43e673ac40d4f 100644
--- a/src/metabase/util.clj
+++ b/src/metabase/util.clj
@@ -204,7 +204,7 @@
and [Common Lisp](http://www.lispworks.com/documentation/HyperSpec/Body/m_prog1c.htm#prog1).
Style note: Prefer `doto` when appropriate, e.g. when dealing with Java objects."
- {:style/indent 1}
+ {:style/indent :defn}
[first-form & body]
`(let [~'<> ~first-form]
~@body
diff --git a/test/metabase/driver/postgres_test.clj b/test/metabase/driver/postgres_test.clj
index b90c6686107117e2fb8da83e515844027ab134ff..843999e48c6960fc01d4ebe83385859beafe3568 100644
--- a/test/metabase/driver/postgres_test.clj
+++ b/test/metabase/driver/postgres_test.clj
@@ -531,3 +531,23 @@
(mt/rows
(mt/run-mbql-query venues
{:filter [:= $id "2"]})))))))
+
+(deftest dont-sync-tables-with-no-select-permissions-test
+ (testing "Make sure we only sync databases for which the current user has SELECT permissions"
+ (mt/test-driver :postgres
+ (drop-if-exists-and-create-db! "no-select-test")
+ (let [details (mt/dbdef->connection-details :postgres :db {:database-name "no-select-test"})
+ spec (sql-jdbc.conn/connection-details->spec :postgres details)]
+ (doseq [statement ["CREATE TABLE PUBLIC.table_with_perms (x INTEGER NOT NULL);"
+ "CREATE TABLE PUBLIC.table_with_no_perms (y INTEGER NOT NULL);"
+ "DROP USER IF EXISTS no_select_test_user;"
+ "CREATE USER no_select_test_user WITH PASSWORD '123456';"
+ "GRANT SELECT ON TABLE \"no-select-test\".PUBLIC.table_with_perms TO no_select_test_user;"]]
+ (jdbc/execute! spec [statement])))
+ (let [test-user-details (assoc (mt/dbdef->connection-details :postgres :db {:database-name "no-select-test"})
+ :user "no_select_test_user"
+ :password "123456")]
+ (mt/with-temp Database [database {:engine :postgres, :details test-user-details}]
+ (sync/sync-database! database)
+ (is (= #{"table_with_perms"}
+ (db/select-field :name Table :db_id (:id database)))))))))
diff --git a/test/metabase/driver/sql_jdbc/sync/describe_database_test.clj b/test/metabase/driver/sql_jdbc/sync/describe_database_test.clj
new file mode 100644
index 0000000000000000000000000000000000000000..ddb61a59e19bb4ad79676540a8543028ab3d3f25
--- /dev/null
+++ b/test/metabase/driver/sql_jdbc/sync/describe_database_test.clj
@@ -0,0 +1,106 @@
+(ns metabase.driver.sql-jdbc.sync.describe-database-test
+ (:require [clojure.java.jdbc :as jdbc]
+ [clojure.test :refer :all]
+ [metabase
+ [driver :as driver]
+ [query-processor :as qp]
+ [sync :as sync]
+ [test :as mt]]
+ [metabase.driver.sql-jdbc.connection :as sql-jdbc.conn]
+ [metabase.driver.sql-jdbc.sync
+ [describe-database :as describe-database]
+ [interface :as i]]
+ [metabase.models.table :refer [Table]]
+ [metabase.test.data.one-off-dbs :as one-off-dbs]
+ [toucan.db :as db])
+ (:import java.sql.ResultSet))
+
+(deftest simple-select-probe-query-test
+ (testing "simple-select-probe-query shouldn't actually return any rows"
+ (let [{:keys [name schema]} (Table (mt/id :venues))]
+ (is (= []
+ (mt/rows
+ (qp/process-query
+ (let [[sql] (describe-database/simple-select-probe-query (or driver/*driver* :h2) schema name)]
+ (mt/native-query {:query sql})))))))))
+
+(defn- sql-jdbc-drivers-with-default-describe-database-impl
+ "All SQL JDBC drivers that use the default SQL JDBC implementation of `describe-database`. (As far as I know, this is
+ all of them.)"
+ []
+ (set
+ (filter
+ #(identical? (get-method driver/describe-database :sql-jdbc) (get-method driver/describe-database %))
+ (descendants driver/hierarchy :sql-jdbc))))
+
+(deftest fast-active-tables-test
+ (let [spec (sql-jdbc.conn/db->pooled-connection-spec (mt/db))]
+ (with-open [conn (jdbc/get-connection spec)]
+ ;; We have to mock this to make it work with all DBs
+ (with-redefs [describe-database/all-schemas (constantly #{"PUBLIC"})]
+ (is (= ["CATEGORIES" "CHECKINS" "USERS" "VENUES"]
+ (->> (into [] (describe-database/fast-active-tables (or driver/*driver* :h2) conn))
+ (map :name)
+ sort)))))))
+
+(deftest post-filtered-active-tables-test
+ (let [spec (sql-jdbc.conn/db->pooled-connection-spec (mt/db))]
+ (with-open [conn (jdbc/get-connection spec)]
+ (is (= ["CATEGORIES" "CHECKINS" "USERS" "VENUES"]
+ (->> (into [] (describe-database/post-filtered-active-tables :h2 conn))
+ (map :name)
+ sort))))))
+
+(deftest describe-database-test
+ (is (= {:tables #{{:name "USERS", :schema "PUBLIC", :description nil}
+ {:name "VENUES", :schema "PUBLIC", :description nil}
+ {:name "CATEGORIES", :schema "PUBLIC", :description nil}
+ {:name "CHECKINS", :schema "PUBLIC", :description nil}}}
+ (describe-database/describe-database :h2 (mt/id)))))
+
+(defn- describe-database-with-open-resultset-count
+ "Just like `describe-database`, but instead of returning the database description returns the number of ResultSet
+ objects the sync process left open. Make sure you wrap ResultSets with `with-open`! Otherwise some JDBC drivers like
+ Oracle and Redshift will keep open cursors indefinitely."
+ [driver db]
+ (let [orig-result-set-seq jdbc/result-set-seq
+ resultsets (atom [])]
+ ;; swap out `jdbc/result-set-seq` which is what ultimately gets called on result sets with a function that will
+ ;; stash the ResultSet object in an atom so we can check whether its closed later
+ (with-redefs [jdbc/result-set-seq (fn [^ResultSet rs & more]
+ (swap! resultsets conj rs)
+ (apply orig-result-set-seq rs more))]
+ ;; taking advantage of the fact that `describe-database/describe-database` can accept JBDC connections instead of
+ ;; databases; by doing this we can keep the connection open and check whether resultsets are still open before
+ ;; they would normally get closed
+ (jdbc/with-db-connection [conn (sql-jdbc.conn/db->pooled-connection-spec db)]
+ (describe-database/describe-database driver conn)
+ (reduce + (for [^ResultSet rs @resultsets]
+ (if (.isClosed rs) 0 1)))))))
+
+(defn- count-active-tables-in-db
+ [db-id]
+ (db/count Table
+ :db_id db-id
+ :active true))
+
+(deftest sync-only-accessable
+ (one-off-dbs/with-blank-db
+ (doseq [statement ["set db_close_delay -1;"
+ "drop table if exists \"birds\";"
+ "create table \"birds\" ();"]]
+ (jdbc/execute! one-off-dbs/*conn* [statement]))
+ (sync/sync-database! (mt/db))
+ (is (= 1 (count-active-tables-in-db (mt/id))))
+ ;; We have to mock this as H2 doesn't have the notion of a user connecting to it
+ (with-redefs [i/have-select-privilege? (constantly false)]
+ (sync/sync-database! (mt/db))
+ (is (= 0 (count-active-tables-in-db (mt/id)))
+ "We shouldn't sync tables for which we don't have select privilege"))))
+
+(deftest dont-leak-resultsets-test
+ (mt/test-drivers (sql-jdbc-drivers-with-default-describe-database-impl)
+ (testing (str "make sure that running the sync process doesn't leak cursors because it's not closing the ResultSets. "
+ "See issues #4389, #6028, and #6467 (Oracle) and #7609 (Redshift)")
+ (is (= 0
+ (describe-database-with-open-resultset-count driver/*driver* (mt/db)))))))
diff --git a/test/metabase/driver/sql_jdbc/sync/describe_table_test.clj b/test/metabase/driver/sql_jdbc/sync/describe_table_test.clj
new file mode 100644
index 0000000000000000000000000000000000000000..53c89279a42689b3a015682fa8444f6bae32b32e
--- /dev/null
+++ b/test/metabase/driver/sql_jdbc/sync/describe_table_test.clj
@@ -0,0 +1,73 @@
+(ns metabase.driver.sql-jdbc.sync.describe-table-test
+ (:require [clojure
+ [string :as str]
+ [test :refer :all]]
+ [clojure.java.jdbc :as jdbc]
+ [metabase
+ [driver :as driver]
+ [test :as mt]]
+ [metabase.driver.sql-jdbc.sync
+ [describe-table :as describe-table]
+ [interface :as i]]
+ [metabase.models.table :refer [Table]]))
+
+(defn- sql-jdbc-drivers-with-default-describe-table-impl
+ "All SQL JDBC drivers that use the default SQL JDBC implementation of `describe-table`. (As far as I know, this is
+ all of them.)"
+ []
+ (set
+ (filter
+ #(identical? (get-method driver/describe-table :sql-jdbc) (get-method driver/describe-table %))
+ (descendants driver/hierarchy :sql-jdbc))))
+
+(deftest describe-table-test
+ (is (= {:name "VENUES",
+ :fields
+ #{{:name "ID", :database-type "BIGINT", :base-type :type/BigInteger, :database-position 0, :pk? true}
+ {:name "NAME", :database-type "VARCHAR", :base-type :type/Text, :database-position 1}
+ {:name "CATEGORY_ID", :database-type "INTEGER", :base-type :type/Integer, :database-position 2}
+ {:name "LATITUDE", :database-type "DOUBLE", :base-type :type/Float, :database-position 3}
+ {:name "LONGITUDE", :database-type "DOUBLE", :base-type :type/Float, :database-position 4}
+ {:name "PRICE", :database-type "INTEGER", :base-type :type/Integer, :database-position 5}}}
+ (describe-table/describe-table :h2 (mt/id) {:name "VENUES"}))))
+
+(deftest describe-table-fks-test
+ (is (= #{{:fk-column-name "CATEGORY_ID"
+ :dest-table {:name "CATEGORIES", :schema "PUBLIC"}
+ :dest-column-name "ID"}}
+ (describe-table/describe-table-fks :h2 (mt/id) {:name "VENUES"})))
+ (is (= #{{:fk-column-name "USER_ID", :dest-table {:name "USERS", :schema "PUBLIC"}, :dest-column-name "ID"}
+ {:fk-column-name "VENUE_ID", :dest-table {:name "VENUES", :schema "PUBLIC"}, :dest-column-name "ID"}}
+ (describe-table/describe-table-fks :h2 (mt/id) {:name "CHECKINS"}))))
+
+(deftest database-types-fallback-test
+ (mt/test-drivers (sql-jdbc-drivers-with-default-describe-table-impl)
+ (let [org-result-set-seq jdbc/result-set-seq]
+ (with-redefs [jdbc/result-set-seq (fn [& args]
+ (map #(dissoc % :type_name) (apply org-result-set-seq args)))]
+ (is (= #{{:name "longitude" :base-type :type/Float}
+ {:name "category_id" :base-type :type/Integer}
+ {:name "price" :base-type :type/Integer}
+ {:name "latitude" :base-type :type/Float}
+ {:name "name" :base-type :type/Text}
+ {:name "id" :base-type :type/Integer}}
+ (->> (describe-table/describe-table driver/*driver* (mt/id) (Table (mt/id :venues)))
+ :fields
+ (map (fn [{:keys [name base-type]}]
+ {:name (str/lower-case name)
+ :base-type (if (or (isa? base-type :type/Integer)
+ (isa? base-type :type/Decimal)) ; H2 DBs returns the ID as BigInt, Oracle as Decimal;
+ :type/Integer
+ base-type)}))
+ set)))))))
+
+(deftest calculated-special-type-test
+ (mt/test-drivers (sql-jdbc-drivers-with-default-describe-table-impl)
+ (with-redefs [i/column->special-type (fn [_ _ column-name]
+ (when (= (str/lower-case column-name) "longitude")
+ :type/Longitude))]
+ (is (= [["longitude" :type/Longitude]]
+ (->> (describe-table/describe-table (or driver/*driver* :h2) (mt/id) (Table (mt/id :venues)))
+ :fields
+ (filter :special-type)
+ (map (juxt (comp str/lower-case :name) :special-type))))))))
diff --git a/test/metabase/driver/sql_jdbc/sync_test.clj b/test/metabase/driver/sql_jdbc/sync_test.clj
deleted file mode 100644
index 78aa8dd2f699d7fec00f8eb115b66035cddc82a9..0000000000000000000000000000000000000000
--- a/test/metabase/driver/sql_jdbc/sync_test.clj
+++ /dev/null
@@ -1,131 +0,0 @@
-(ns metabase.driver.sql-jdbc.sync-test
- (:require [clojure
- [string :as str]
- [test :refer :all]]
- [clojure.java.jdbc :as jdbc]
- [metabase
- [driver :as driver]
- [query-processor :as qp]
- [sync :as sync]
- [test :as mt]]
- [metabase.driver.sql-jdbc
- [connection :as sql-jdbc.conn]
- [sync :as sql-jdbc.sync]]
- [metabase.models.table :refer [Table]]
- [metabase.test.data.one-off-dbs :as one-off-dbs]
- [toucan.db :as db])
- (:import java.sql.ResultSet))
-
-(defn- sql-jdbc-drivers-with-default-describe-database-impl
- "All SQL JDBC drivers that use the default SQL JDBC implementation of `describe-database`. (As far as I know, this is
- all of them.)"
- []
- (set
- (filter
- #(identical? (get-method driver/describe-database :sql-jdbc) (get-method driver/describe-database %))
- (descendants driver/hierarchy :sql-jdbc))))
-
-(defn- sql-jdbc-drivers-with-default-describe-table-impl
- "All SQL JDBC drivers that use the default SQL JDBC implementation of `describe-table`. (As far as I know, this is
- all of them.)"
- []
- (set
- (filter
- #(identical? (get-method driver/describe-table :sql-jdbc) (get-method driver/describe-table %))
- (descendants driver/hierarchy :sql-jdbc))))
-
-(defn- describe-database-with-open-resultset-count
- "Just like `describe-database`, but instead of returning the database description returns the number of ResultSet
- objects the sync process left open. Make sure you wrap ResultSets with `with-open`! Otherwise some JDBC drivers like
- Oracle and Redshift will keep open cursors indefinitely."
- [driver db]
- (let [orig-result-set-seq jdbc/result-set-seq
- resultsets (atom [])]
- ;; swap out `jdbc/result-set-seq` which is what ultimately gets called on result sets with a function that will
- ;; stash the ResultSet object in an atom so we can check whether its closed later
- (with-redefs [jdbc/result-set-seq (fn [^ResultSet rs & more]
- (swap! resultsets conj rs)
- (apply orig-result-set-seq rs more))]
- ;; taking advantage of the fact that `sql-jdbc.sync/describe-database` can accept JBDC connections instead of
- ;; databases; by doing this we can keep the connection open and check whether resultsets are still open before
- ;; they would normally get closed
- (jdbc/with-db-connection [conn (sql-jdbc.conn/db->pooled-connection-spec db)]
- (sql-jdbc.sync/describe-database driver conn)
- (reduce + (for [^ResultSet rs @resultsets]
- (if (.isClosed rs) 0 1)))))))
-
-(defn- count-active-tables-in-db
- [db-id]
- (db/count Table
- :db_id db-id
- :active true))
-
-(deftest sync-only-accessable
- (one-off-dbs/with-blank-db
- (doseq [statement ["set db_close_delay -1;"
- "drop table if exists \"birds\";"
- "create table \"birds\" ();"]]
- (jdbc/execute! one-off-dbs/*conn* [statement]))
- (sync/sync-database! (mt/db))
- (is (= 1 (count-active-tables-in-db (mt/id))))
- ;; We have to mock this as H2 doesn't have the notion of a user connecting to it
- (with-redefs [sql-jdbc.sync/have-select-privilege? (constantly false)]
- (sync/sync-database! (mt/db))
- (is (= 0 (count-active-tables-in-db (mt/id)))
- "We shouldn't sync tables for which we don't have select privilege"))))
-
-(deftest dont-leak-resultsets-test
- (mt/test-drivers (sql-jdbc-drivers-with-default-describe-database-impl)
- (testing (str "make sure that running the sync process doesn't leak cursors because it's not closing the ResultSets. "
- "See issues #4389, #6028, and #6467 (Oracle) and #7609 (Redshift)")
- (is (= 0
- (describe-database-with-open-resultset-count driver/*driver* (mt/db)))))))
-
-(deftest simple-select-probe-test
- (let [{:keys [name schema]} (Table (mt/id :venues))]
- (is (= [[1 "Red Medicine" 4 10.0646 -165.374 3]]
- (mt/rows
- (qp/process-query
- (mt/native-query {:query (first (#'sql-jdbc.sync/simple-select-probe (or driver/*driver* :h2) schema name))})))))))
-
-(deftest database-types-fallback-test
- (mt/test-drivers (sql-jdbc-drivers-with-default-describe-table-impl)
- (let [org-result-set-seq jdbc/result-set-seq]
- (with-redefs [jdbc/result-set-seq (fn [& args]
- (map #(dissoc % :type_name) (apply org-result-set-seq args)))]
- (is (= #{{:name "longitude" :base-type :type/Float}
- {:name "category_id" :base-type :type/Integer}
- {:name "price" :base-type :type/Integer}
- {:name "latitude" :base-type :type/Float}
- {:name "name" :base-type :type/Text}
- {:name "id" :base-type :type/Integer}}
- (->> (sql-jdbc.sync/describe-table driver/*driver* (mt/id) (Table (mt/id :venues)))
- :fields
- (map (fn [{:keys [name base-type]}]
- {:name (str/lower-case name)
- :base-type (if (or (isa? base-type :type/Integer)
- (isa? base-type :type/Decimal)) ; H2 DBs returns the ID as BigInt, Oracle as Decimal;
- :type/Integer
- base-type)}))
- set)))))))
-
-(deftest calculated-special-type-test
- (mt/test-drivers (sql-jdbc-drivers-with-default-describe-table-impl)
- (with-redefs [sql-jdbc.sync/column->special-type (fn [_ _ column-name]
- (when (= (str/lower-case column-name) "longitude")
- :type/Longitude))]
- (is (= [["longitude" :type/Longitude]]
- (->> (sql-jdbc.sync/describe-table (or driver/*driver* :h2) (mt/id) (Table (mt/id :venues)))
- :fields
- (filter :special-type)
- (map (juxt (comp str/lower-case :name) :special-type))))))))
-
-(deftest fast-active-tables-test
- (jdbc/with-db-metadata [metadata (sql-jdbc.conn/db->pooled-connection-spec (mt/db))]
- ;; We have to mock this to make it work with all DBs
- (with-redefs [sql-jdbc.sync/all-schemas (constantly #{"PUBLIC"})]
- (is (= ["CATEGORIES" "CHECKINS" "USERS" "VENUES"]
- (->> metadata
- (sql-jdbc.sync/fast-active-tables (or driver/*driver* :h2) (mt/db))
- (map :table_name)
- sort))))))
diff --git a/test/metabase/sync/sync_metadata/comments_test.clj b/test/metabase/sync/sync_metadata/comments_test.clj
index 9372ba69057e3997e72b848e84d5b563c03d8809..e36fdaa76402b485e8098a570871b05d6e0e9ab1 100644
--- a/test/metabase/sync/sync_metadata/comments_test.clj
+++ b/test/metabase/sync/sync_metadata/comments_test.clj
@@ -1,85 +1,83 @@
(ns metabase.sync.sync-metadata.comments-test
"Test for the logic that syncs Table column descriptions with the comments fetched from a DB."
- (:require [metabase
+ (:require [clojure.test :refer :all]
+ [metabase
[driver :as driver]
[sync :as sync]
+ [test :as mt]
[util :as u]]
[metabase.models
[field :refer [Field]]
[table :refer [Table]]]
[metabase.sync.sync-metadata.tables :as sync-tables]
- [metabase.test.data :as data]
- [metabase.test.data
- [datasets :as datasets]
- [interface :as tx]]
+ [metabase.test.data.interface :as tx]
[toucan.db :as db]))
-;; Tests for field comments: ------------------
-
(defn- db->fields [db]
- (let [table-ids (db/select-ids 'Table :db_id (u/get-id db))]
+ (let [table-ids (db/select-ids Table :db_id (u/get-id db))]
(set (map (partial into {}) (db/select ['Field :name :description] :table_id [:in table-ids])))))
-;; test basic field comments sync
(tx/defdataset ^:private basic-field-comments
[["basic_field_comments"
[{:field-name "with_comment", :base-type :type/Text, :field-comment "comment"}
{:field-name "no_comment", :base-type :type/Text}]
[["foo" "bar"]]]])
-(datasets/expect-with-drivers #{:h2 :postgres}
- #{{:name (data/format-name "id"), :description nil}
- {:name (data/format-name "with_comment"), :description "comment"}
- {:name (data/format-name "no_comment"), :description nil}}
- (data/dataset basic-field-comments
- (db->fields (data/db))))
+(deftest basic-field-comments-test
+ (testing "test basic field comments sync"
+ (mt/test-drivers #{:h2 :postgres}
+ (mt/dataset basic-field-comments
+ (is (= #{{:name (mt/format-name "id"), :description nil}
+ {:name (mt/format-name "with_comment"), :description "comment"}
+ {:name (mt/format-name "no_comment"), :description nil}}
+ (db->fields (mt/db))))))))
-;; test changing the description in metabase db so we can check it is not overwritten by comment in source db when resyncing
(tx/defdataset ^:private update-desc
[["update_desc"
[{:field-name "updated_desc", :base-type :type/Text, :field-comment "original comment"}]
[["foo"]]]])
-(datasets/expect-with-drivers #{:h2 :postgres}
- #{{:name (data/format-name "id"), :description nil}
- {:name (data/format-name "updated_desc"), :description "updated description"}}
- (data/dataset update-desc
- (data/with-temp-copy-of-db
- ;; change the description in metabase while the source table comment remains the same
- (db/update-where! Field {:id (data/id "update_desc" "updated_desc")}, :description "updated description")
- ;; now sync the DB again, this should NOT overwrite the manually updated description
- (sync/sync-table! (Table (data/id "update_desc")))
- (db->fields (data/db)))))
+(deftest comment-should-not-overwrite-custom-description-test
+ (testing (str "test changing the description in metabase db so we can check it is not overwritten by comment in "
+ "source db when resyncing"))
+ (mt/test-drivers #{:h2 :postgres}
+ (mt/dataset update-desc
+ (mt/with-temp-copy-of-db
+ ;; change the description in metabase while the source table comment remains the same
+ (db/update-where! Field {:id (mt/id "update_desc" "updated_desc")}, :description "updated description")
+ ;; now sync the DB again, this should NOT overwrite the manually updated description
+ (sync/sync-table! (Table (mt/id "update_desc")))
+ (is (= #{{:name (mt/format-name "id"), :description nil}
+ {:name (mt/format-name "updated_desc"), :description "updated description"}}
+ (db->fields (mt/db))))))))
-;; test adding a comment to the source data that was initially empty, so we can check that the resync picks it up
(tx/defdataset ^:private comment-after-sync
[["comment_after_sync"
[{:field-name "comment_after_sync", :base-type :type/Text}]
[["foo"]]]])
-(datasets/expect-with-drivers #{:h2 :postgres}
- #{{:name (data/format-name "id"), :description nil}
- {:name (data/format-name "comment_after_sync"), :description "added comment"}}
- (data/dataset comment-after-sync
- ;; modify the source DB to add the comment and resync. The easiest way to do this is just destroy the entire DB
- ;; and re-create a modified version. As such, let the SQL JDBC driver know the DB is being "modified" so it can
- ;; destroy its current connection pool
- (driver/notify-database-updated driver/*driver* (data/db))
- (let [modified-dbdef (update
- comment-after-sync
- :table-definitions
- (fn [[tabledef]]
- [(update
- tabledef
- :field-definitions
- (fn [[fielddef]]
- [(assoc fielddef :field-comment "added comment")]))]))]
- (tx/create-db! driver/*driver* modified-dbdef))
- (sync/sync-table! (Table (data/id "comment_after_sync")))
- (db->fields (data/db))))
-
-
-;; Tests for table comments: ------------------
+(deftest sync-comment-on-existing-field-test
+ (testing "test adding a comment to the source data that was initially empty, so we can check that the resync picks it up"
+ (mt/test-drivers #{:h2 :postgres}
+ (mt/dataset comment-after-sync
+ ;; modify the source DB to add the comment and resync. The easiest way to do this is just destroy the entire DB
+ ;; and re-create a modified version. As such, let the SQL JDBC driver know the DB is being "modified" so it can
+ ;; destroy its current connection pool
+ (driver/notify-database-updated driver/*driver* (mt/db))
+ (let [modified-dbdef (update
+ comment-after-sync
+ :table-definitions
+ (fn [[tabledef]]
+ [(update
+ tabledef
+ :field-definitions
+ (fn [[fielddef]]
+ [(assoc fielddef :field-comment "added comment")]))]))]
+ (tx/create-db! driver/*driver* modified-dbdef))
+ (sync/sync-table! (Table (mt/id "comment_after_sync")))
+ (is (= #{{:name (mt/format-name "id"), :description nil}
+ {:name (mt/format-name "comment_after_sync"), :description "added comment"}}
+ (db->fields (mt/db))))))))
(defn- basic-table [table-name comment]
(tx/map->DatabaseDefinition {:database-name (str table-name "_db")
@@ -89,32 +87,35 @@
:table-comment comment}]}))
(defn- db->tables [db]
- (set (map (partial into {}) (db/select ['Table :name :description] :db_id (u/get-id db)))))
+ (set (map (partial into {}) (db/select [Table :name :description] :db_id (u/get-id db)))))
-;; test basic comments on table
-(datasets/expect-with-drivers #{:h2 :postgres}
- #{{:name (data/format-name "table_with_comment"), :description "table comment"}}
- (data/dataset (basic-table "table_with_comment" "table comment")
- (db->tables (data/db))))
+(deftest table-comments-test
+ (testing "test basic comments on table"
+ (mt/test-drivers #{:h2 :postgres}
+ (mt/dataset (basic-table "table_with_comment" "table comment")
+ (is (= #{{:name (mt/format-name "table_with_comment"), :description "table comment"}}
+ (db->tables (mt/db))))))))
-;; test changing the description in metabase on table to check it is not overwritten by comment in source db when
-;; resyncing
-(datasets/expect-with-drivers #{:h2 :postgres}
- #{{:name (data/format-name "table_with_updated_desc"), :description "updated table description"}}
- (data/dataset (basic-table "table_with_updated_desc" "table comment")
- (data/with-temp-copy-of-db
- ;; change the description in metabase while the source table comment remains the same
- (db/update-where! Table {:id (data/id "table_with_updated_desc")}, :description "updated table description")
- ;; now sync the DB again, this should NOT overwrite the manually updated description
- (sync-tables/sync-tables! (data/db))
- (db->tables (data/db)))))
+(deftest dont-overwrite-table-custom-description-test
+ (testing (str "test changing the description in metabase on table to check it is not overwritten by comment in "
+ "source db when resyncing")
+ (mt/test-drivers #{:h2 :postgres}
+ (mt/dataset (basic-table "table_with_updated_desc" "table comment")
+ (mt/with-temp-copy-of-db
+ ;; change the description in metabase while the source table comment remains the same
+ (db/update-where! Table {:id (mt/id "table_with_updated_desc")}, :description "updated table description")
+ ;; now sync the DB again, this should NOT overwrite the manually updated description
+ (sync-tables/sync-tables! (mt/db))
+ (is (= #{{:name (mt/format-name "table_with_updated_desc"), :description "updated table description"}}
+ (db->tables (mt/db)))))))))
-;; test adding a comment to the source table that was initially empty, so we can check that the resync picks it up
-(datasets/expect-with-drivers #{:h2 :postgres}
- #{{:name (data/format-name "table_with_comment_after_sync"), :description "added comment"}}
- (data/dataset (basic-table "table_with_comment_after_sync" nil)
- ;; modify the source DB to add the comment and resync
- (driver/notify-database-updated driver/*driver* (data/db))
- (tx/create-db! driver/*driver* (basic-table "table_with_comment_after_sync" "added comment"))
- (sync-tables/sync-tables! (data/db))
- (db->tables (data/db))))
+(deftest sync-existing-table-comment-test
+ (testing "test adding a comment to the source table that was initially empty, so we can check that the resync picks it up"
+ (mt/test-drivers #{:h2 :postgres}
+ (mt/dataset (basic-table "table_with_comment_after_sync" nil)
+ ;; modify the source DB to add the comment and resync
+ (driver/notify-database-updated driver/*driver* (mt/db))
+ (tx/create-db! driver/*driver* (basic-table "table_with_comment_after_sync" "added comment"))
+ (sync-tables/sync-tables! (mt/db))
+ (is (= #{{:name (mt/format-name "table_with_comment_after_sync"), :description "added comment"}}
+ (db->tables (mt/db))))))))
diff --git a/test/metabase/test/data/impl/verify.clj b/test/metabase/test/data/impl/verify.clj
index 298ce8c23c31e5116efb8f1fad57f1cf89a13edb..9323fdda51d1e0a9f6e5c0f5e49ff2ce9516e5fe 100644
--- a/test/metabase/test/data/impl/verify.clj
+++ b/test/metabase/test/data/impl/verify.clj
@@ -27,7 +27,7 @@
(defn- loaded-fields
"Actual Fields loaded into a Table. Returns set of field names."
[{:keys [driver database actual-schema actual-table-name]}]
- (->> (driver/describe-table driver database {:schema actual-schema, :name actual-table-name})
+ (->> (driver/describe-table driver database {:schema actual-schema, :name actual-table-name, :db_id (:id database)})
:fields
(map :name)
set))