diff --git a/backend/junit/test/metabase/junit.clj b/backend/junit/test/metabase/junit.clj
index 03e92ad806fa2bc8bbf8241ec5dec6d395d7ea06..e7e73bf0943199895dcdbc573caded413e1b7604 100644
--- a/backend/junit/test/metabase/junit.clj
+++ b/backend/junit/test/metabase/junit.clj
@@ -3,6 +3,7 @@
(:require [clojure
[pprint :as pp]
[string :as str]]
+ [metabase.util :as u]
[pjstadig.print :as p]
[test-report-junit-xml.core :as junit-xml]))
@@ -15,30 +16,31 @@
(str "\n" message))))
(defn- result-output [{:keys [expected actual diffs message], :as event}]
- (with-out-str
- (println (event-description event))
- ;; this code is adapted from `pjstadig.util`
- (p/with-pretty-writer
- (fn []
- (let [print-expected (fn [actual]
- (p/rprint "expected: ")
- (pp/pprint expected)
- (p/rprint " actual: ")
- (pp/pprint actual)
- (p/clear))]
- (if (seq diffs)
- (doseq [[actual [a b]] diffs]
- (print-expected actual)
- (p/rprint " diff:")
- (if a
- (do (p/rprint " - ")
- (pp/pprint a)
- (p/rprint " + "))
- (p/rprint " + "))
- (when b
- (pp/pprint b))
- (p/clear))
- (print-expected actual)))))))
+ (with-redefs [u/colorize? (constantly false)]
+ (with-out-str
+ (println (event-description event))
+ ;; this code is adapted from `pjstadig.util`
+ (p/with-pretty-writer
+ (fn []
+ (let [print-expected (fn [actual]
+ (p/rprint "expected: ")
+ (pp/pprint expected)
+ (p/rprint " actual: ")
+ (pp/pprint actual)
+ (p/clear))]
+ (if (seq diffs)
+ (doseq [[actual [a b]] diffs]
+ (print-expected actual)
+ (p/rprint " diff:")
+ (if a
+ (do (p/rprint " - ")
+ (pp/pprint a)
+ (p/rprint " + "))
+ (p/rprint " + "))
+ (when b
+ (pp/pprint b))
+ (p/clear))
+ (print-expected actual))))))))
(defmulti format-result
{:arglists '([event])}
diff --git a/modules/drivers/mongo/test/metabase/test/data/mongo.clj b/modules/drivers/mongo/test/metabase/test/data/mongo.clj
index dfd86d443e42c322cf731a3312e07a052388f221..24593f7fe88ec8192d7a454b1b8cd4afabbfafef 100644
--- a/modules/drivers/mongo/test/metabase/test/data/mongo.clj
+++ b/modules/drivers/mongo/test/metabase/test/data/mongo.clj
@@ -18,12 +18,12 @@
(defmethod tx/dbdef->connection-details :mongo
[_ _ dbdef]
- {:dbname (tx/escaped-name dbdef)
+ {:dbname (tx/escaped-database-name dbdef)
:host "localhost"})
(defn- destroy-db! [driver dbdef]
(with-open [mongo-connection (mg/connect (tx/dbdef->connection-details driver :server dbdef))]
- (mg/drop-db mongo-connection (tx/escaped-name dbdef))))
+ (mg/drop-db mongo-connection (tx/escaped-database-name dbdef))))
(defmethod tx/create-db! :mongo
[driver {:keys [table-definitions], :as dbdef} & {:keys [skip-drop-db?], :or {skip-drop-db? false}}]
diff --git a/modules/drivers/snowflake/test/metabase/driver/snowflake_test.clj b/modules/drivers/snowflake/test/metabase/driver/snowflake_test.clj
index 35ac91e71ba3ce2a35f223ed7233216dbf0cc632..5fd4143f82c98f4db064c6c724615d4e30dfd9a4 100644
--- a/modules/drivers/snowflake/test/metabase/driver/snowflake_test.clj
+++ b/modules/drivers/snowflake/test/metabase/driver/snowflake_test.clj
@@ -50,8 +50,8 @@
FOREIGN KEY (\"user_id\") REFERENCES \"v2_test-data\".\"PUBLIC\".\"users\" (\"id\");"
"ALTER TABLE \"v2_test-data\".\"PUBLIC\".\"checkins\" ADD CONSTRAINT \"kins_venue_id_venues_621212269\"
FOREIGN KEY (\"venue_id\") REFERENCES \"v2_test-data\".\"PUBLIC\".\"venues\" (\"id\");"])
- (ddl/create-db-ddl-statements :snowflake (-> (mt/get-dataset-definition dataset-defs/test-data)
- (update :database-name #(str "v2_" %)))))))))
+ (ddl/create-db-tables-ddl-statements :snowflake (-> (mt/get-dataset-definition dataset-defs/test-data)
+ (update :database-name #(str "v2_" %)))))))))
;; TODO -- disabled because these are randomly failing, will figure out when I'm back from vacation. I think it's a
;; bug in the JDBC driver -- Cam
diff --git a/modules/drivers/sqlite/test/metabase/test/data/sqlite.clj b/modules/drivers/sqlite/test/metabase/test/data/sqlite.clj
index 94a87f44ce63ad2ffbc007cd631e3fae44b62f14..34b3fb89ee57cfb13108d5d08bdd628dac478756 100644
--- a/modules/drivers/sqlite/test/metabase/test/data/sqlite.clj
+++ b/modules/drivers/sqlite/test/metabase/test/data/sqlite.clj
@@ -8,7 +8,7 @@
(sql-jdbc.tx/add-test-extensions! :sqlite)
(defmethod tx/dbdef->connection-details :sqlite [_ context dbdef]
- {:db (str (tx/escaped-name dbdef) ".sqlite")})
+ {:db (str (tx/escaped-database-name dbdef) ".sqlite")})
(doseq [[base-type sql-type] {:type/BigInteger "BIGINT"
:type/Boolean "BOOLEAN"
diff --git a/src/metabase/db.clj b/src/metabase/db.clj
index 655474e8533f5302e5878b6f00e0292582136eac..683b2ad98b880b97476461bcb1ada506e2314455 100644
--- a/src/metabase/db.clj
+++ b/src/metabase/db.clj
@@ -248,14 +248,21 @@
(when-let [max-pool-size (config/config-int :mb-application-db-max-connection-pool-size)]
{"maxPoolSize" max-pool-size})))
+(defn quoting-style
+ "HoneySQL quoting style to use for application DBs of the given type. Note for H2 application DBs we automatically
+ uppercase all identifiers (since this is H2's default behavior) whereas in the SQL QP we stick with the case we got
+ when we synced the DB."
+ [db-type]
+ (case db-type
+ :postgres :ansi
+ :h2 :h2
+ :mysql :mysql))
+
(defn- create-connection-pool!
"Create a connection pool for the application DB and set it as the default Toucan connection. This is normally called
once during start up; calling it a second time (e.g. from the REPL) will "
[jdbc-spec]
- (db/set-default-quoting-style! (case (db-type)
- :postgres :ansi
- :h2 :h2
- :mysql :mysql))
+ (db/set-default-quoting-style! (quoting-style (db-type)))
;; REPL usage only: kill the old pool if one exists
(u/ignore-exceptions
(when-let [^PoolBackedDataSource pool (:datasource (db/connection))]
diff --git a/src/metabase/db/liquibase.clj b/src/metabase/db/liquibase.clj
index 8723a85c530f120b473dd65139b7bb773bcb3901..8b7d7f8c32804a906bc3fa1661ebcd4af83d323f 100644
--- a/src/metabase/db/liquibase.clj
+++ b/src/metabase/db/liquibase.clj
@@ -32,19 +32,18 @@
;; closing the `LiquibaseConnection`/`Database` closes the parent JDBC `Connection`, so only use it in combination
;; with `with-open` *if* we are opening a new JDBC `Connection` from a JDBC spec. If we're passed in a `Connection`,
;; it's safe to assume the caller is managing its lifecycle.
- (letfn []
- (cond
- (instance? java.sql.Connection jdbc-spec-or-conn)
- (f (-> jdbc-spec-or-conn liquibase-connection database liquibase))
+ (cond
+ (instance? java.sql.Connection jdbc-spec-or-conn)
+ (f (-> jdbc-spec-or-conn liquibase-connection database liquibase))
- (:connection jdbc-spec-or-conn)
- (recur (:connection jdbc-spec-or-conn) f)
+ (:connection jdbc-spec-or-conn)
+ (recur (:connection jdbc-spec-or-conn) f)
- :else
- (with-open [jdbc-conn (jdbc/get-connection jdbc-spec-or-conn)
- liquibase-conn (liquibase-connection jdbc-conn)
- database (database liquibase-conn)]
- (f (liquibase database))))))
+ :else
+ (with-open [jdbc-conn (jdbc/get-connection jdbc-spec-or-conn)
+ liquibase-conn (liquibase-connection jdbc-conn)
+ database (database liquibase-conn)]
+ (f (liquibase database)))))
(defmacro with-liquibase
"Execute body with an instance of a `Liquibase` bound to `liquibase-binding`.
@@ -73,7 +72,7 @@
filter out blank / comment lines. Even though this is not necessary for H2 or Postgres go ahead and do it anyway
because it keeps the code simple and doesn't make a significant performance difference.
- As of 0.31.1 this is only used for printing the migrations without running or using force migrating."
+ As of 0.31.1 this is only used for printing the migrations without running or when force migrating."
[^Liquibase liquibase]
(for [line (str/split-lines (migrations-sql liquibase))
:when (not (or (str/blank? line)
diff --git a/test/metabase/db/schema_migrations_test.clj b/test/metabase/db/schema_migrations_test.clj
new file mode 100644
index 0000000000000000000000000000000000000000..78509fbb05b1bd3ad5baf19f63a69277652631da
--- /dev/null
+++ b/test/metabase/db/schema_migrations_test.clj
@@ -0,0 +1,33 @@
+(ns metabase.db.schema-migrations-test
+ "Tests for the schema migrations defined in the Liquibase YAML files. The basic idea is:
+
+ 1. Create a temporary H2/Postgres/MySQL/MariaDB database
+ 2. Run all migrations up to a certain point
+ 3. Load some arbitrary data
+ 4. run migration(s) after that point (verify that they actually run)
+ 5. verify that data looks like what we'd expect after running migration(s)
+
+ See `metabase.db.schema-migrations-test.impl` for the implementation of this functionality."
+ (:require [clojure.test :refer :all]
+ [metabase.db.schema-migrations-test.impl :as impl]
+ [metabase.models :refer [Database Field Table]]
+ [toucan.db :as db]))
+
+(deftest database-position-test
+ (testing "Migration 165: add `database_position` to Field"
+ (impl/test-migrations 165 [migrate!]
+ ;; create a Database with a Table with 2 Fields
+ (db/simple-insert! Database {:name "DB", :engine "h2", :created_at :%now, :updated_at :%now})
+ (db/simple-insert! Table {:name "Table", :db_id 1, :created_at :%now, :updated_at :%now, :active true})
+ (let [mock-field {:table_id 1, :created_at :%now, :updated_at :%now, :base_type "type/Text", :database_type "VARCHAR"}]
+ (db/simple-insert! Field (assoc mock-field :name "Field 1"))
+ (db/simple-insert! Field (assoc mock-field :name "Field 2")))
+ (testing "sanity check: Fields should not have a `:database_position` column yet"
+ (is (not (contains? (Field 1) :database_position))))
+ ;; now run migration 165
+ (migrate!)
+ (testing "Fields should get `:database_position` equal to their IDs"
+ (doseq [id [1 2]]
+ (testing (format "Field %d" id)
+ (is (= id
+ (db/select-one-field :database_position Field :id id)))))))))
diff --git a/test/metabase/db/schema_migrations_test/impl.clj b/test/metabase/db/schema_migrations_test/impl.clj
new file mode 100644
index 0000000000000000000000000000000000000000..388cbf9c61f430fd664228bbd6651b67c6207acf
--- /dev/null
+++ b/test/metabase/db/schema_migrations_test/impl.clj
@@ -0,0 +1,162 @@
+(ns metabase.db.schema-migrations-test.impl
+ "Tests for the schema migrations defined in the Liquibase YAML files. The basic idea is:
+
+ 1. Create a temporary H2/Postgres/MySQL/MariaDB database
+ 2. Run all migrations up to a certain point
+ 3. Load some arbitrary data
+ 4. run migration(s) after that point (verify that they actually run)
+ 5. verify that data looks like what we'd expect after running migration(s)
+
+ Actual tests using this code live in `metabase.db.schema-migrations-test`."
+ (:require [clojure.java.jdbc :as jdbc]
+ [clojure.test :refer :all]
+ [clojure.tools.logging :as log]
+ [metabase
+ [db :as mdb]
+ [driver :as driver]
+ [test :as mt]
+ [util :as u]]
+ [metabase.db.liquibase :as liquibase]
+ [metabase.driver.sql-jdbc.connection :as sql-jdbc.conn]
+ [metabase.test.data.interface :as tx]
+ [metabase.test.initialize :as initialize]
+ [toucan.db :as db])
+ (:import [liquibase Contexts Liquibase]
+ [liquibase.changelog ChangeSet DatabaseChangeLog]))
+
+(defmulti ^:private do-with-temp-empty-app-db*
+ "Create a new completely empty app DB for `driver`, then call `(f jdbc-spec)` with a spec for that DB. Should clean up
+ before and after running `f` as needed."
+ {:arglists '([driver f])}
+ driver/dispatch-on-initialized-driver
+ :hierarchy #'driver/hierarchy)
+
+(defmethod do-with-temp-empty-app-db* :default
+ [driver f]
+ (log/debugf "Creating empty %s app db..." driver)
+ (let [dbdef {:database-name "schema-migrations-test-db"
+ :table-definitions []}]
+ (try
+ (tx/create-db! driver dbdef)
+ (let [connection-details (tx/dbdef->connection-details driver :db dbdef)
+ jdbc-spec (sql-jdbc.conn/connection-details->spec driver connection-details)]
+ (f jdbc-spec))
+ (finally
+ (log/debugf "Destroying empty %s app db..." driver)
+ (tx/destroy-db! driver dbdef)))))
+
+(defmethod do-with-temp-empty-app-db* :h2
+ [driver f]
+ (log/debug "Creating empty H2 app db...")
+ ;; we don't need to destroy this DB manually because it will just get shutdown immediately when the Connection
+ ;; closes because we're not setting a `DB_CLOSE_DELAY`
+ ;;
+ ;; don't use the usual implementation of `tx/dbdef->connection-details` because it creates a spec that only connects
+ ;; to with `USER=GUEST` which doesn't let us run DDL statements
+ (let [connection-details {:db "mem:schema-migrations-test-db"}
+ jdbc-spec (binding [mdb/*allow-potentailly-unsafe-connections* true]
+ (sql-jdbc.conn/connection-details->spec driver connection-details))]
+ (f jdbc-spec)))
+
+(defn- do-with-temp-empty-app-db [driver f]
+ (do-with-temp-empty-app-db*
+ driver
+ (fn [jdbc-spec]
+ (with-open [conn (jdbc/get-connection jdbc-spec)]
+ (binding [toucan.db/*db-connection* {:connection conn}
+ toucan.db/*quoting-style* (mdb/quoting-style driver)]
+ (f conn))))))
+
+(defmacro ^:private with-temp-empty-app-db
+ "Create a new temporary application DB of `db-type` and execute `body` with `conn-binding` bound to a
+ `java.sql.Connection` to the database. Toucan `*db-connection*` is also bound, which means Toucan functions like
+ `select` or `update!` will operate against this database."
+ [[conn-binding db-type] & body]
+ `(do-with-temp-empty-app-db ~db-type (fn [~(vary-meta conn-binding assoc :tag 'java.sql.Connection)] ~@body)))
+
+(defn- run-migrations-in-range!
+ "Run Liquibase migrations from our migrations YAML file in the range of `start-id` -> `end-id` (inclusive) against a
+ DB with `jdbc-spec`."
+ [^java.sql.Connection conn [start-id end-id]]
+ (liquibase/with-liquibase [liquibase conn]
+ (let [change-log (.getDatabaseChangeLog liquibase)
+ ;; create a new change log that only has the subset of migrations we want to run.
+ subset-change-log (doto (DatabaseChangeLog.)
+ ;; we don't actually use this for anything but if we don't set it then Liquibase barfs
+ (.setPhysicalFilePath (.getPhysicalFilePath change-log)))]
+ ;; add the relevant migrations (change sets) to our subset change log
+ (doseq [^ChangeSet change-set (.getChangeSets change-log)
+ :let [id (Integer/parseUnsignedInt (.getId change-set))]
+ :when (<= start-id id end-id)]
+ (.addChangeSet subset-change-log change-set))
+ ;; now create a new instance of Liquibase that will run just the subset change log
+ (let [subset-liquibase (Liquibase. subset-change-log (.getResourceAccessor liquibase) (.getDatabase liquibase))]
+ (when-let [unrun (not-empty (.listUnrunChangeSets subset-liquibase nil))]
+ (log/debugf "Running migrations %s...%s (inclusive)"
+ (.getId ^ChangeSet (first unrun)) (.getId ^ChangeSet (last unrun))))
+ ;; run the migrations
+ (.update subset-liquibase (Contexts.))))))
+
+(defn- test-migrations-for-driver [driver [start-id end-id] f]
+ (log/debug (u/format-color 'yellow "Testing migrations for driver %s..." driver))
+ (with-temp-empty-app-db [conn driver]
+ ;; sanity check: make sure the DB is actually empty
+ (let [metadata (.getMetaData conn)]
+ (with-open [rs (.getTables metadata nil nil "%" (into-array String ["TABLE"]))]
+ (let [tables (jdbc/result-set-seq rs)]
+ (assert (zero? (count tables))
+ (str "'Empty' application DB is not actually empty. Found tables:\n"
+ (u/pprint-to-str tables))))))
+ (run-migrations-in-range! conn [1 (dec start-id)])
+ (f #(run-migrations-in-range! conn [start-id end-id])))
+ (log/debug (u/format-color 'green "Done testing migrations for driver %s." driver)))
+
+(defn test-migrations*
+ [migration-range f]
+ ;; make sure the normal Metabase application DB is set up before running the tests so things don't get confused and
+ ;; try to initialize it while the mock DB is bound
+ (initialize/initialize-if-needed! :db)
+ (let [[start-id end-id] (if (sequential? migration-range)
+ migration-range
+ [migration-range migration-range])]
+ (testing (format "Migrations %d-%d" start-id end-id)
+ (mt/test-drivers #{:h2 :mysql :postgres}
+ (test-migrations-for-driver driver/*driver* [start-id end-id] f)))))
+
+(defmacro test-migrations
+ "Util macro for running tests for a set of Liquibase schema migration(s).
+
+ Before invoking body, migrations up to `start-id` are automatically ran. In body, you should do the following in
+ this order:
+
+ 1. Load data and check any preconditions before running migrations you're testing. Prefer `toucan.db/simple-insert!`
+ or plain SQL for loading data to avoid dependencies on the current state of the schema that may be present in
+ Toucan `pre-insert` functions and the like.
+
+ 2. Call `(migrate!)` to run migrations in range of `start-id` -> `end-id` (inclusive)
+
+ 3. Check any postconditions after running the migrations.
+
+ e.g.
+
+ ;; example test for migrations 100-105
+ (test-migrations [100 105] [migrate!]
+ ;; (Migrations 1-99 are ran automatically before body is invoked)
+ ;; 1. Load data
+ (create-some-users!)
+ ;; 2. Run migrations 100-105
+ (migrate!)
+ ;; 3. Do some test assertions
+ (is (= ...)))
+
+ For convenience `migration-range` can be either a range of migrations IDs to test (e.g. `[100 105]`) or just a
+ single migration ID (e.g. `100`).
+
+ These run against the current set of test `DRIVERS` (by default H2), so if you want to run against more than H2
+ either set the `DRIVERS` env var or use `mt/set-test-drivers!` from the REPL."
+ {:style/indent 2}
+ [migration-range [migrate!-binding] & body]
+ `(test-migrations*
+ ~migration-range
+ (fn [~migrate!-binding]
+ ~@body)))
diff --git a/test/metabase/test/data/h2.clj b/test/metabase/test/data/h2.clj
index 07d0bac1a8c80e86c3f0c74c29837dc87fe8ff1f..3c93c425392c0f2b969727156cfd9cc6eeb382ea 100644
--- a/test/metabase/test/data/h2.clj
+++ b/test/metabase/test/data/h2.clj
@@ -48,9 +48,10 @@
(defmethod tx/dbdef->connection-details :h2
[_ context dbdef]
- {:db (str "mem:" (tx/escaped-name dbdef) (when (= context :db)
- ;; Return details with the GUEST user added so SQL queries are allowed.
- ";USER=GUEST;PASSWORD=guest"))})
+ {:db (str "mem:" (tx/escaped-database-name dbdef) (when (= context :db)
+ ;; Return details with the GUEST user added so SQL queries are
+ ;; allowed.
+ ";USER=GUEST;PASSWORD=guest"))})
(defmethod sql.tx/pk-sql-type :h2 [_] "BIGINT AUTO_INCREMENT")
@@ -58,7 +59,8 @@
(defmethod sql.tx/drop-db-if-exists-sql :h2 [& _] nil)
-(defmethod sql.tx/create-db-sql :h2 [& _]
+(defmethod sql.tx/create-db-sql :h2
+ [& _]
(str
;; We don't need to actually do anything to create a database here. Just disable the undo
;; log (i.e., transactions) for this DB session because the bulk operations to load data don't need to be atomic
diff --git a/test/metabase/test/data/impl.clj b/test/metabase/test/data/impl.clj
index 46383a1b63d4a02514d030e5a227edbf28870810..0158b829c66bc8873d76d231881963139aa0fa0f 100644
--- a/test/metabase/test/data/impl.clj
+++ b/test/metabase/test/data/impl.clj
@@ -93,10 +93,11 @@
(tu.tz/with-system-timezone-id "UTC"
(tx/create-db! driver database-definition)))
;; Add DB object to Metabase DB
- (let [db (db/insert! Database
- :name database-name
- :engine (name driver)
- :details (tx/dbdef->connection-details driver :db database-definition))]
+ (let [connection-details (tx/dbdef->connection-details driver :db database-definition)
+ db (db/insert! Database
+ :name database-name
+ :engine (name driver)
+ :details connection-details)]
(try
;; sync newly added DB
(u/with-timeout sync-timeout-ms
@@ -112,16 +113,23 @@
(Database (u/get-id db))
(catch Throwable e
(db/delete! Database :id (u/get-id db))
- (throw e))))
+ (throw (ex-info "Failed to create test database"
+ {:driver driver
+ :database-name database-name
+ :connection-details connection-details}
+ e)))))
(catch Throwable e
- (printf "Failed to create %s '%s' test database:\n" driver database-name)
- (println e)
- (if config/is-test?
- (System/exit -1)
- (do
- (println (u/format-color 'red "create-database! failed; destroying %s database %s" driver (pr-str database-name)))
- (tx/destroy-db! driver database-definition)
- (throw e))))))
+ (let [message (format "Failed to create %s '%s' test database" driver database-name)]
+ (println message "\n" e)
+ (if config/is-test?
+ (System/exit -1)
+ (do
+ (println (u/format-color 'red "create-database! failed; destroying %s database %s" driver (pr-str database-name)))
+ (tx/destroy-db! driver database-definition)
+ (throw (ex-info message
+ {:driver driver
+ :database-name database-name}
+ e))))))))
(defmethod get-or-create-database! :default
[driver dbdef]
diff --git a/test/metabase/test/data/interface.clj b/test/metabase/test/data/interface.clj
index 6e37eea80e9163bb4c9e198d5a8ae4ae1434ab19..a41b8eabed3cff1ef9f7623f8d870e660480ae9c 100644
--- a/test/metabase/test/data/interface.clj
+++ b/test/metabase/test/data/interface.clj
@@ -170,7 +170,7 @@
[]
(the-driver-with-test-extensions (or driver/*driver* :h2)))
-(defn escaped-name
+(defn escaped-database-name
"Return escaped version of database name suitable for use as a filename / database name / etc."
^String [^DatabaseDefinition {:keys [database-name]}]
{:pre [(string? database-name)]}
@@ -261,13 +261,18 @@
(defmulti create-db!
"Create a new database from `database-definition`, including adding tables, fields, and foreign key constraints,
- and add the appropriate data. This method should drop existing databases with the same name if applicable, unless
- the skip-drop-db? arg is true. This is to workaround a scenario where the postgres driver terminates the connection
- before dropping the DB and causes some tests to fail. (This refers to creating the actual *DBMS* database itself,
- *not* a Metabase `Database` object.)
+ and load the appropriate data. (This refers to creating the actual *DBMS* database itself, *not* a Metabase
+ `Database` object.)
Optional `options` as third param. Currently supported options include `skip-drop-db?`. If unspecified,
- `skip-drop-db?` should default to `false`."
+ `skip-drop-db?` should default to `false`.
+
+ This method should drop existing databases with the same name if applicable, unless the `skip-drop-db?` arg is
+ truthy. This is to work around a scenario where the Postgres driver terminates the connection before dropping the DB
+ and causes some tests to fail.
+
+ This method is not expected to return anything; use `dbdef->connection-details` to get connection details for this
+ database after you create it."
{:arglists '([driver database-definition & {:keys [skip-drop-db?]}])}
dispatch-on-driver-with-test-extensions
:hierarchy #'driver/hierarchy)
diff --git a/test/metabase/test/data/postgres.clj b/test/metabase/test/data/postgres.clj
index 5e923c9c4bbf2e0ba6fc7a094b5595b0573dd8b1..2737057f5c155dbb141dcb044e9d441a9b0fd185 100644
--- a/test/metabase/test/data/postgres.clj
+++ b/test/metabase/test/data/postgres.clj
@@ -66,9 +66,10 @@
(defmethod ddl/drop-db-ddl-statements :postgres
[driver {:keys [database-name], :as dbdef} & options]
- (assert (string? database-name)
- (format "Expected String database name; got ^%s %s"
- (some-> database-name class .getCanonicalName) (pr-str database-name)))
+ (when-not (string? database-name)
+ (throw (ex-info (format "Expected String database name; got ^%s %s"
+ (some-> database-name class .getCanonicalName) (pr-str database-name))
+ {:driver driver, :dbdef dbdef})))
;; add an additional statement to the front to kill open connections to the DB before dropping
(cons
(kill-connections-to-db-sql database-name)
diff --git a/test/metabase/test/data/sql.clj b/test/metabase/test/data/sql.clj
index 4c4045c23e3710cdb6c03b9da7cddb4e2c019252..fae7f0874aa307f6870bba71eb20e933339af4a6 100644
--- a/test/metabase/test/data/sql.clj
+++ b/test/metabase/test/data/sql.clj
@@ -173,7 +173,6 @@
tx/dispatch-on-driver-with-test-extensions
:hierarchy #'driver/hierarchy)
-
(defmulti create-db-sql
"Return a `CREATE DATABASE` statement."
{:arglists '([driver dbdef])}
@@ -183,7 +182,6 @@
(defmethod create-db-sql :sql/test-extensions [driver {:keys [database-name]}]
(format "CREATE DATABASE %s;" (qualify-and-quote driver database-name)))
-
(defmulti drop-db-if-exists-sql
"Return a `DROP DATABASE` statement."
{:arglists '([driver dbdef])}
@@ -193,7 +191,6 @@
(defmethod drop-db-if-exists-sql :sql/test-extensions [driver {:keys [database-name]}]
(format "DROP DATABASE IF EXISTS %s;" (qualify-and-quote driver database-name)))
-
(defmulti create-table-sql
"Return a `CREATE TABLE` statement."
{:arglists '([driver dbdef tabledef])}
diff --git a/test/metabase/test/data/sql/ddl.clj b/test/metabase/test/data/sql/ddl.clj
index e2219234a8ae48ecba719e1dbd9d98c04c44bcb6..307f610c4dadd914f88b77ce0434574d516f8634 100644
--- a/test/metabase/test/data/sql/ddl.clj
+++ b/test/metabase/test/data/sql/ddl.clj
@@ -23,18 +23,26 @@
(defmethod drop-db-ddl-statements :sql/test-extensions
[driver dbdef & {:keys [skip-drop-db?]}]
(when-not skip-drop-db?
- ;; Exec SQL for creating the DB
- [(sql.tx/drop-db-if-exists-sql driver dbdef)
- (sql.tx/create-db-sql driver dbdef)]))
+ (try
+ [(sql.tx/drop-db-if-exists-sql driver dbdef)]
+ (catch Throwable e
+ (throw (ex-info "Error generating DDL statements for dropping database"
+ {:driver driver}
+ e))))))
+(defn create-db-ddl-statements
+ "DDL statements to create the DB itself (does not include statements to drop the DB if it already exists)."
+ [driver dbdef]
+ [(sql.tx/create-db-sql driver dbdef)])
-(defmulti create-db-ddl-statements
- "Return a default sequence of DDL statements for creating a DB (not including dropping it)."
+(defmulti create-db-tables-ddl-statements
+ "Return a default sequence of DDL statements for creating the tables/columns/etc. inside a Database. DOES NOT INCLUDE
+ STATEMENTS FOR CREATING (OR DROPPING) THE DATABASE ITSELF."
{:arglists '([driver dbdef & {:as options}])}
tx/dispatch-on-driver-with-test-extensions
:hierarchy #'driver/hierarchy)
-(defmethod create-db-ddl-statements :sql/test-extensions
+(defmethod create-db-tables-ddl-statements :sql/test-extensions
[driver {:keys [table-definitions], :as dbdef} & _]
;; Build combined statement for creating tables + FKs + comments
(let [statements (atom [])
@@ -61,7 +69,6 @@
(add! (sql.tx/standalone-column-comment-sql driver dbdef tabledef fielddef)))))
@statements))
-
;; The methods below are currently only used by `:sql-jdbc` drivers, but you can use them to help implement your
;; `:sql` driver test extensions as well because there's nothing JDBC specific about them
diff --git a/test/metabase/test/data/sql_jdbc/execute.clj b/test/metabase/test/data/sql_jdbc/execute.clj
index 5fd4b9efd51f43f15953f0b7da18c1a71a34b4ae..eaf5165dad0dec5957a449290eea507e0ce984cc 100644
--- a/test/metabase/test/data/sql_jdbc/execute.clj
+++ b/test/metabase/test/data/sql_jdbc/execute.clj
@@ -12,7 +12,7 @@
;;; +----------------------------------------------------------------------------------------------------------------+
(defn- jdbc-execute! [db-spec sql]
- (log/tracef "[execute] %s" (pr-str sql))
+ (log/tracef "[execute %s] %s" driver/*driver* (pr-str sql))
(jdbc/execute! db-spec [sql] {:transaction? false, :multi? true}))
(defn default-execute-sql! [driver context dbdef sql & {:keys [execute!]
diff --git a/test/metabase/test/data/sql_jdbc/load_data.clj b/test/metabase/test/data/sql_jdbc/load_data.clj
index c289ba20f496bfa88b5147cc65ace4f3a04609ba..a3eb6ef6ebff570b57e9ff2dd913363d71c254b8 100644
--- a/test/metabase/test/data/sql_jdbc/load_data.clj
+++ b/test/metabase/test/data/sql_jdbc/load_data.clj
@@ -198,12 +198,16 @@
"Default implementation of `create-db!` for SQL drivers."
{:arglists '([driver dbdef & {:keys [skip-drop-db?]}])}
[driver {:keys [table-definitions], :as dbdef} & options]
- ;; first execute statements to drop/create the DB if needed (this will return nothing is `skip-drop-db?` is true)
+ ;; first execute statements to drop the DB if needed (this will do nothing if `skip-drop-db?` is true)
(doseq [statement (apply ddl/drop-db-ddl-statements driver dbdef options)]
(execute/execute-sql! driver :server dbdef statement))
+ ;; now execute statements to create the DB
+ (doseq [statement (ddl/create-db-ddl-statements driver dbdef)]
+ (execute/execute-sql! driver :server dbdef statement))
;; next, get a set of statements for creating the DB & Tables
- (let [statements (apply ddl/create-db-ddl-statements driver dbdef options)]
- ;; exec the combined statement
+ (let [statements (apply ddl/create-db-tables-ddl-statements driver dbdef options)]
+ ;; exec the combined statement. Notice we're now executing in the `:db` context e.g. executing them for a specific
+ ;; DB rather than on `:server` (no DB in particular)
(execute/execute-sql! driver :db dbdef (str/join ";\n" statements)))
;; Now load the data for each Table
(doseq [tabledef table-definitions]
@@ -213,5 +217,10 @@
(defn destroy-db!
"Default impl of `destroy-db!` for SQL drivers."
[driver dbdef]
- (doseq [statement (apply ddl/drop-db-ddl-statements driver dbdef)]
- (execute/execute-sql! driver :server dbdef statement)))
+ (try
+ (doseq [statement (ddl/drop-db-ddl-statements driver dbdef)]
+ (execute/execute-sql! driver :server dbdef statement))
+ (catch Throwable e
+ (throw (ex-info "Error destroying database"
+ {:driver driver, :dbdef dbdef}
+ e)))))