diff --git a/test/metabase/sync/field_values_test.clj b/test/metabase/sync/field_values_test.clj
index 56de2746b243bbbe34a0e86c5ce543572b363643..96678cac862829148beadb27c783f3a38b63dd1e 100644
--- a/test/metabase/sync/field_values_test.clj
+++ b/test/metabase/sync/field_values_test.clj
@@ -1,71 +1,59 @@
(ns metabase.sync.field-values-test
"Tests around the way Metabase syncs FieldValues, and sets the values of `field.has_field_values`."
- (:require [clojure.java.jdbc :as jdbc]
- [clojure.string :as str]
- [expectations :refer :all]
+ (:require [expectations :refer :all]
[metabase
- [db :as mdb]
- [driver :as driver]
- [sync :as sync :refer :all]]
- [metabase.driver.generic-sql :as sql]
+ [sync :as sync]
+ [util :as u]]
[metabase.models
- [database :refer [Database]]
[field :refer [Field]]
- [field-values :as field-values :refer [FieldValues]]]
- [metabase.test
- [data :as data]
- [util :as tu]]
- [toucan.db :as db]
- [toucan.util.test :as tt]))
+ [field-values :as field-values :refer [FieldValues]]
+ [table :refer [Table]]]
+ [metabase.test.data :as data]
+ [metabase.test.data.one-off-dbs :as one-off-dbs]
+ [toucan.db :as db]))
-;;; --------------------------------------------------- Helper Fns ---------------------------------------------------
-
-(defn insert-range-sql
- "Generate SQL to insert a row for each number in `rang`."
- [rang]
- (str "INSERT INTO blueberries_consumed (num) VALUES "
- (str/join ", " (for [n rang]
- (str "(" n ")")))))
-
-(def ^:private ^:dynamic *conn* nil)
-
-(defn- do-with-blueberries-db
- "An empty canvas upon which you may paint your dreams.
-
- Creates a database with a single table, `blueberries_consumed`, with one column, `num`; binds this DB to
- `data/*get-db*` so you can use `data/db` and `data/id` to access it."
- {:style/indent 0}
- [f]
- (let [details {:db (str "mem:" (tu/random-name) ";DB_CLOSE_DELAY=10")}]
- (binding [mdb/*allow-potentailly-unsafe-connections* true]
- (tt/with-temp Database [db {:engine :h2, :details details}]
- (jdbc/with-db-connection [conn (sql/connection-details->spec (driver/engine->driver :h2) details)]
- (jdbc/execute! conn ["CREATE TABLE blueberries_consumed (num INTEGER NOT NULL);"])
- (binding [data/*get-db* (constantly db)]
- (binding [*conn* conn]
- (f))))))))
-
-(defmacro ^:private with-blueberries-db {:style/indent 0} [& body]
- `(do-with-blueberries-db (fn [] ~@body)))
-
-(defn- insert-blueberries-and-sync!
- "With the temp blueberries db from above, insert a `range` of values and re-sync the DB.
-
- (insert-blueberries-and-sync! [0 1 2 3]) ; insert 4 rows"
- [rang]
- (jdbc/execute! *conn* [(insert-range-sql rang)])
- (sync-database! (data/db)))
+;; Test that when we delete FieldValues syncing the Table again will cause them to be re-created
+(defn- venues-price-field-values []
+ (db/select-one-field :values FieldValues, :field_id (data/id :venues :price)))
+(expect
+ {1 [1 2 3 4]
+ 2 nil
+ 3 [1 2 3 4]}
+ (array-map
+ ;; 1. Check that we have expected field values to start with
+ 1 (venues-price-field-values)
+ ;; 2. Delete the Field values, make sure they're gone
+ 2 (do (db/delete! FieldValues :field_id (data/id :venues :price))
+ (venues-price-field-values))
+ ;; 3. Now re-sync the table and make sure they're back
+ 3 (do (sync/sync-table! (Table (data/id :venues)))
+ (venues-price-field-values))))
+
+;; Test that syncing will cause FieldValues to be updated
+(expect
+ {1 [1 2 3 4]
+ 2 [1 2 3]
+ 3 [1 2 3 4]}
+ (array-map
+ ;; 1. Check that we have expected field values to start with
+ 1 (venues-price-field-values)
+ ;; 2. Update the FieldValues, remove one of the values that should be there
+ 2 (do (db/update! FieldValues (db/select-one-id FieldValues :field_id (data/id :venues :price))
+ :values [1 2 3])
+ (venues-price-field-values))
+ ;; 3. Now re-sync the table and make sure the value is back
+ 3 (do (sync/sync-table! (Table (data/id :venues)))
+ (venues-price-field-values))))
-;;; ---------------------------------------------- The Tests Themselves ----------------------------------------------
;; A Field with 50 values should get marked as `auto-list` on initial sync, because it should be 'list', but was
;; marked automatically, as opposed to explicitly (`list`)
(expect
:auto-list
- (with-blueberries-db
+ (one-off-dbs/with-blueberries-db
;; insert 50 rows & sync
- (insert-blueberries-and-sync! (range 50))
+ (one-off-dbs/insert-rows-and-sync! (range 50))
;; has_field_values should be auto-list
(db/select-one-field :has_field_values Field :id (data/id :blueberries_consumed :num))))
@@ -75,31 +63,31 @@
{:values [0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33
34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49]
:human_readable_values {}}
- (with-blueberries-db
- (insert-blueberries-and-sync! (range 50))
+ (one-off-dbs/with-blueberries-db
+ (one-off-dbs/insert-rows-and-sync! (range 50))
(db/select-one [FieldValues :values :human_readable_values], :field_id (data/id :blueberries_consumed :num))))
;; ok, but if the number grows past the threshold & we sync again it should get unmarked as auto-list and set back to
;; `nil` (#3215)
(expect
nil
- (with-blueberries-db
+ (one-off-dbs/with-blueberries-db
;; insert 50 bloobs & sync. has_field_values should be auto-list
- (insert-blueberries-and-sync! (range 50))
+ (one-off-dbs/insert-rows-and-sync! (range 50))
(assert (= (db/select-one-field :has_field_values Field :id (data/id :blueberries_consumed :num))
:auto-list))
;; now insert enough bloobs to put us over the limit and re-sync.
- (insert-blueberries-and-sync! (range 50 (+ 100 field-values/auto-list-cardinality-threshold)))
+ (one-off-dbs/insert-rows-and-sync! (range 50 (+ 100 field-values/auto-list-cardinality-threshold)))
;; has_field_values should have been set to nil.
(db/select-one-field :has_field_values Field :id (data/id :blueberries_consumed :num))))
;; ...its FieldValues should also get deleted.
(expect
nil
- (with-blueberries-db
+ (one-off-dbs/with-blueberries-db
;; do the same steps as the test above...
- (insert-blueberries-and-sync! (range 50))
- (insert-blueberries-and-sync! (range 50 (+ 100 field-values/auto-list-cardinality-threshold)))
+ (one-off-dbs/insert-rows-and-sync! (range 50))
+ (one-off-dbs/insert-rows-and-sync! (range 50 (+ 100 field-values/auto-list-cardinality-threshold)))
;; ///and FieldValues should also have been deleted.
(db/select-one [FieldValues :values :human_readable_values], :field_id (data/id :blueberries_consumed :num))))
@@ -107,13 +95,13 @@
;; anything!
(expect
:list
- (with-blueberries-db
+ (one-off-dbs/with-blueberries-db
;; insert 50 bloobs & sync
- (insert-blueberries-and-sync! (range 50))
+ (one-off-dbs/insert-rows-and-sync! (range 50))
;; change has_field_values to list
(db/update! Field (data/id :blueberries_consumed :num) :has_field_values "list")
;; insert more bloobs & re-sync
- (insert-blueberries-and-sync! (range 50 (+ 100 field-values/auto-list-cardinality-threshold)))
+ (one-off-dbs/insert-rows-and-sync! (range 50 (+ 100 field-values/auto-list-cardinality-threshold)))
;; has_field_values shouldn't change
(db/select-one-field :has_field_values Field :id (data/id :blueberries_consumed :num))))
@@ -130,10 +118,10 @@
166 167 168 169 170 171 172 173 174 175 176 177 178 179 180 181 182 183 184 185 186 187 188
189 190 191 192 193 194 195 196 197 198 199]
:human_readable_values {}}
- (with-blueberries-db
+ (one-off-dbs/with-blueberries-db
;; follow the same steps as the test above...
- (insert-blueberries-and-sync! (range 50))
+ (one-off-dbs/insert-rows-and-sync! (range 50))
(db/update! Field (data/id :blueberries_consumed :num) :has_field_values "list")
- (insert-blueberries-and-sync! (range 50 (+ 100 field-values/auto-list-cardinality-threshold)))
+ (one-off-dbs/insert-rows-and-sync! (range 50 (+ 100 field-values/auto-list-cardinality-threshold)))
;; ... and FieldValues should still be there, but this time updated to include the new values!
(db/select-one [FieldValues :values :human_readable_values], :field_id (data/id :blueberries_consumed :num))))
diff --git a/test/metabase/sync/sync_metadata/fields_test.clj b/test/metabase/sync/sync_metadata/fields_test.clj
index 2dbde44ddefd8c5a51fb82f6980d66a60bd41136..6cff1212d141c39fb8a3e74df16b5044e41c6dcc 100644
--- a/test/metabase/sync/sync_metadata/fields_test.clj
+++ b/test/metabase/sync/sync_metadata/fields_test.clj
@@ -2,24 +2,26 @@
"Tests for the logic that syncs Field models with the Metadata fetched from a DB. (There are more tests for this
behavior in the namespace `metabase.sync-database.sync-dynamic-test`, which is sort of a misnomer.)"
(:require [clojure.java.jdbc :as jdbc]
- [expectations :refer [expect]]
+ [expectations :refer :all]
[metabase
- [db :as mdb]
- [driver :as driver]
[query-processor :as qp]
[sync :as sync]
[util :as u]]
- [metabase.driver.generic-sql :as sql]
[metabase.models
[database :refer [Database]]
[field :refer [Field]]
[table :refer [Table]]]
- [metabase.test.data.interface :as tdi]
+ [metabase.test.data :as data]
+ [metabase.test.data.one-off-dbs :as one-off-dbs]
[toucan
[db :as db]
[hydrate :refer [hydrate]]]
[toucan.util.test :as tt]))
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | Dropping & Undropping Columns |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
(defn- with-test-db-before-and-after-dropping-a-column
"Testing function that performs the following steps:
@@ -29,40 +31,32 @@
4. Executes `(f database)` a second time
5. Returns a map containing results from both calls to `f` for comparison."
[f]
- ;; let H2 connect to DBs that aren't created yet
- (binding [mdb/*allow-potentailly-unsafe-connections* true]
- (let [driver (driver/engine->driver :h2)
- details (tdi/database->connection-details driver :db {:db "mem:deleted_columns_test"
- :database-name "deleted_columns_test"})
- exec! (fn [& statements]
- (doseq [statement statements]
- (jdbc/execute! (sql/connection-details->spec driver details) statement)))]
- ;; first, create a new in-memory test DB and add some data to it
- (exec!
- ;; H2 needs that 'guest' user for QP purposes. Set that up
- "CREATE USER IF NOT EXISTS GUEST PASSWORD 'guest';"
- ;; Keep DB open until we say otherwise :)
- "SET DB_CLOSE_DELAY -1;"
- ;; create table & load data
- "DROP TABLE IF EXISTS \"birds\";"
- "CREATE TABLE \"birds\" (\"species\" VARCHAR PRIMARY KEY, \"example_name\" VARCHAR);"
- "GRANT ALL ON \"birds\" TO GUEST;"
- (str "INSERT INTO \"birds\" (\"species\", \"example_name\") VALUES "
- "('House Finch', 'Marshawn Finch'), "
- "('California Gull', 'Steven Seagull'), "
- "('Chicken', 'Colin Fowl');"))
- ;; now create MB models + sync
- (tt/with-temp Database [database {:engine :h2, :details details, :name "Deleted Columns Test"}]
- (sync/sync-database! database)
- ;; ok, let's see what (f) gives us
- (let [f-before (f database)]
- ;; ok cool! now delete one of those columns...
- (exec! "ALTER TABLE \"birds\" DROP COLUMN \"example_name\";")
- ;; ...and re-sync...
- (sync/sync-database! database)
- ;; ...now let's see how (f) may have changed! Compare to original.
- {:before-drop f-before
- :after-drop (f database)})))))
+ ;; first, create a new in-memory test DB and add some data to it
+ (one-off-dbs/with-blank-db
+ (doseq [statement [ ;; H2 needs that 'guest' user for QP purposes. Set that up
+ "CREATE USER IF NOT EXISTS GUEST PASSWORD 'guest';"
+ ;; Keep DB open until we say otherwise :)
+ "SET DB_CLOSE_DELAY -1;"
+ ;; create table & load data
+ "DROP TABLE IF EXISTS \"birds\";"
+ "CREATE TABLE \"birds\" (\"species\" VARCHAR PRIMARY KEY, \"example_name\" VARCHAR);"
+ "GRANT ALL ON \"birds\" TO GUEST;"
+ (str "INSERT INTO \"birds\" (\"species\", \"example_name\") VALUES "
+ "('House Finch', 'Marshawn Finch'), "
+ "('California Gull', 'Steven Seagull'), "
+ "('Chicken', 'Colin Fowl');")]]
+ (jdbc/execute! one-off-dbs/*conn* [statement]))
+ ;; now sync
+ (sync/sync-database! (data/db))
+ ;; ok, let's see what (f) gives us
+ (let [f-before (f (data/db))]
+ ;; ok cool! now delete one of those columns...
+ (jdbc/execute! one-off-dbs/*conn* ["ALTER TABLE \"birds\" DROP COLUMN \"example_name\";"])
+ ;; ...and re-sync...
+ (sync/sync-database! (data/db))
+ ;; ...now let's see how (f) may have changed! Compare to original.
+ {:before-drop f-before
+ :after-drop (f (data/db))})))
;; make sure sync correctly marks a Field as active = false when it gets dropped from the DB
@@ -110,3 +104,217 @@
:data
:native_form
:query))))
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | PK & FK Syncing |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+(defn- force-sync-table!
+ "Updates the `:fields_hash` to ensure that the sync process will include fields in the sync"
+ [table]
+ (db/update! Table (u/get-id table), :fields_hash "something new")
+ (sync/sync-table! (Table (data/id :venues))))
+
+;; Test PK Syncing
+(expect [:type/PK
+ nil
+ :type/PK
+ :type/Latitude
+ :type/PK]
+ (let [get-special-type (fn [] (db/select-one-field :special_type Field, :id (data/id :venues :id)))]
+ [;; Special type should be :id to begin with
+ (get-special-type)
+ ;; Clear out the special type
+ (do (db/update! Field (data/id :venues :id), :special_type nil)
+ (get-special-type))
+ ;; Calling sync-table! should set the special type again
+ (do (force-sync-table! (data/id :venues))
+ (get-special-type))
+ ;; sync-table! should *not* change the special type of fields that are marked with a different type
+ (do (db/update! Field (data/id :venues :id), :special_type :type/Latitude)
+ (get-special-type))
+ ;; Make sure that sync-table runs set-table-pks-if-needed!
+ (do (db/update! Field (data/id :venues :id), :special_type nil)
+ (force-sync-table! (Table (data/id :venues)))
+ (get-special-type))]))
+
+
+;; Check that Foreign Key relationships were created on sync as we expect
+(expect (data/id :venues :id)
+ (db/select-one-field :fk_target_field_id Field, :id (data/id :checkins :venue_id)))
+
+(expect (data/id :users :id)
+ (db/select-one-field :fk_target_field_id Field, :id (data/id :checkins :user_id)))
+
+(expect (data/id :categories :id)
+ (db/select-one-field :fk_target_field_id Field, :id (data/id :venues :category_id)))
+
+;; Check that sync-table! causes FKs to be set like we'd expect
+(expect [{:special_type :type/FK, :fk_target_field_id true}
+ {:special_type nil, :fk_target_field_id false}
+ {:special_type :type/FK, :fk_target_field_id true}]
+ (let [field-id (data/id :checkins :user_id)
+ get-special-type-and-fk-exists? (fn []
+ (into {} (-> (db/select-one [Field :special_type :fk_target_field_id],
+ :id field-id)
+ (update :fk_target_field_id #(db/exists? Field :id %)))))]
+ [ ;; FK should exist to start with
+ (get-special-type-and-fk-exists?)
+ ;; Clear out FK / special_type
+ (do (db/update! Field field-id, :special_type nil, :fk_target_field_id nil)
+ (get-special-type-and-fk-exists?))
+ ;; Run sync-table and they should be set again
+ (let [table (Table (data/id :checkins))]
+ (sync/sync-table! table)
+ (get-special-type-and-fk-exists?))]))
+
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | mystery narrow-to-min-max test |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+;; TODO - hey, what is this testing? If you wrote this test, please explain what's going on here
+(defn- narrow-to-min-max [row]
+ (-> row
+ (get-in [:type :type/Number])
+ (select-keys [:min :max])
+ (update :min #(u/round-to-decimals 4 %))
+ (update :max #(u/round-to-decimals 4 %))))
+
+(expect
+ [{:min -165.374 :max -73.9533}
+ {:min 10.0646 :max 40.7794}]
+ (tt/with-temp* [Database [database {:details (:details (Database (data/id))), :engine :h2}]
+ Table [table {:db_id (u/get-id database), :name "VENUES"}]]
+ (sync/sync-table! table)
+ (map narrow-to-min-max
+ [(db/select-one-field :fingerprint Field, :id (data/id :venues :longitude))
+ (db/select-one-field :fingerprint Field, :id (data/id :venues :latitude))])))
+
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | tests related to sync's Field hashes |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+(defn- exec! [& statements]
+ (doseq [statement statements]
+ (jdbc/execute! one-off-dbs/*conn* [statement])))
+
+(defmacro ^:private throw-if-called {:style/indent 1} [fn-var & body]
+ `(with-redefs [~fn-var (fn [& args#]
+ (throw (RuntimeException. "Should not be called!")))]
+ ~@body))
+
+;; Validate the changing of a column's type triggers a hash miss and sync
+(expect
+ [ ;; Original column type
+ "SMALLINT"
+ ;; Altered the column, now it's an integer
+ "INTEGER"
+ ;; Original hash and the new one are not equal
+ false
+ ;; Reruning sync shouldn't change the hash
+ true]
+ (one-off-dbs/with-blueberries-db
+ (one-off-dbs/insert-rows-and-sync! (range 50))
+ ;; After this sync, we know about the new table and it's SMALLINT column
+ (let [table-id (data/id :blueberries_consumed)
+ get-table #(Table (data/id :blueberries_consumed))
+ get-field #(Field (data/id :blueberries_consumed :num))
+ {old-hash :fields_hash} (get-table)
+ {old-db-type :database_type} (get-field)]
+ ;; Change the column from SMALLINT to INTEGER. In clojure-land these are both integers, but this change
+ ;; should trigger a hash miss and thus resync the table, since something has changed
+ (exec! "ALTER TABLE blueberries_consumed ALTER COLUMN num INTEGER")
+ (sync/sync-database! (data/db))
+ (let [{new-hash :fields_hash} (get-table)
+ {new-db-type :database_type} (get-field)]
+
+ ;; Syncing again with no change should not call sync-field-instances! or update the hash
+ (throw-if-called metabase.sync.sync-metadata.fields/sync-field-instances!
+ (sync/sync-database! (data/db))
+ [old-db-type
+ new-db-type
+ (= old-hash new-hash)
+ (= new-hash (:fields_hash (get-table)))])))))
+
+(defn- table-md-with-hash [table-id]
+ {:active-fields (count (db/select Field :table_id table-id :active true))
+ :inactive-fields (count (db/select Field :table_id table-id :active false))
+ :fields-hash (:fields_hash (db/select-one Table :id table-id))})
+
+(defn- no-fields-hash [m]
+ (dissoc m :fields-hash))
+
+;; This tests a table that adds a column, ensures sync picked up the new column and the hash changed
+(expect
+ [
+ ;; Only the num column should be found
+ {:active-fields 1, :inactive-fields 0}
+ ;; Add a column, should still be no inactive
+ {:active-fields 2, :inactive-fields 0}
+ ;; Adding a column should make the hashes not equal
+ false
+ ]
+ (one-off-dbs/with-blueberries-db
+ (one-off-dbs/insert-rows-and-sync! (range 50))
+ ;; We should now have a hash value for num as a SMALLINT
+ (let [before-table-md (table-md-with-hash (data/id :blueberries_consumed))
+ _ (exec! "ALTER TABLE blueberries_consumed ADD COLUMN weight FLOAT")
+ _ (sync/sync-database! (data/db))
+ ;; Now that hash will include num and weight
+ after-table-md (table-md-with-hash (data/id :blueberries_consumed))]
+ [(no-fields-hash before-table-md)
+ (no-fields-hash after-table-md)
+ (= (:fields-hash before-table-md)
+ (:fields-hash after-table-md))])))
+
+;; Drops a column, ensures sync finds the drop, updates the hash
+(expect
+ [
+ ;; Test starts with two columns
+ {:active-fields 2, :inactive-fields 0}
+ ;; Dropped the weight column
+ {:active-fields 1, :inactive-fields 1}
+ ;; Hashes should be different without the weight column
+ false]
+ (one-off-dbs/with-blank-db
+ ;; create a DB that has 2 columns this time instead of 1
+ (exec! "CREATE TABLE blueberries_consumed (num SMALLINT NOT NULL, weight FLOAT)")
+ (one-off-dbs/insert-rows-and-sync! (range 50))
+ ;; We should now have a hash value for num as a SMALLINT
+ (let [before-table-md (table-md-with-hash (data/id :blueberries_consumed))
+ _ (exec! "ALTER TABLE blueberries_consumed DROP COLUMN weight")
+ _ (sync/sync-database! (data/db))
+ ;; Now that hash will include num and weight
+ after-table-md (table-md-with-hash (data/id :blueberries_consumed))]
+ [(no-fields-hash before-table-md)
+ (no-fields-hash after-table-md)
+ (= (:fields-hash before-table-md)
+ (:fields-hash after-table-md))])))
+
+;; Drops and readds a column, ensures that the hash is back to it's original value
+(expect
+ [
+ ;; Both num and weight columns should be found
+ {:active-fields 2, :inactive-fields 0}
+ ;; Both columns should still be present
+ {:active-fields 2, :inactive-fields 0}
+ ;; The hashes should be the same
+ true]
+ (one-off-dbs/with-blank-db
+ ;; create a DB that has 2 columns this time instead of 1
+ (exec! "CREATE TABLE blueberries_consumed (num SMALLINT NOT NULL, weight FLOAT)")
+ (one-off-dbs/insert-rows-and-sync! (range 50))
+ ;; We should now have a hash value for num as a SMALLINT
+ (let [before-table-md (table-md-with-hash (data/id :blueberries_consumed))
+ _ (exec! "ALTER TABLE blueberries_consumed DROP COLUMN weight")
+ _ (sync/sync-database! (data/db))
+ _ (exec! "ALTER TABLE blueberries_consumed ADD COLUMN weight FLOAT")
+ _ (sync/sync-database! (data/db))
+ ;; Now that hash will include num and weight
+ after-table-md (table-md-with-hash (data/id :blueberries_consumed))]
+ [(no-fields-hash before-table-md)
+ (no-fields-hash after-table-md)
+ (= (:fields-hash before-table-md)
+ (:fields-hash after-table-md))])) )
diff --git a/test/metabase/sync/util_test.clj b/test/metabase/sync/util_test.clj
new file mode 100644
index 0000000000000000000000000000000000000000..2b0cbbc5f25332366d74cd4f783752afb7537f44
--- /dev/null
+++ b/test/metabase/sync/util_test.clj
@@ -0,0 +1,55 @@
+(ns metabase.sync.util-test
+ "Tests for the utility functions shared by all parts of sync, such as the duplicate ops guard."
+ (:require [expectations :refer :all]
+ [metabase
+ [driver :as driver]
+ [sync :as sync]]
+ [metabase.models.database :refer [Database]]
+ [toucan.util.test :as tt]))
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | Duplicate Sync Prevention |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+;; test that we prevent running simultaneous syncs on the same database
+
+(defonce ^:private calls-to-describe-database (atom 0))
+
+(defrecord ConcurrentSyncTestDriver []
+ clojure.lang.Named
+ (getName [_] "ConcurrentSyncTestDriver"))
+
+(extend ConcurrentSyncTestDriver
+ driver/IDriver
+ (merge driver/IDriverDefaultsMixin
+ {:describe-database (fn [_ _]
+ (swap! calls-to-describe-database inc)
+ (Thread/sleep 1000)
+ {:tables #{}})
+ :describe-table (constantly nil)
+ :details-fields (constantly [])}))
+
+(driver/register-driver! :concurrent-sync-test (ConcurrentSyncTestDriver.))
+
+;; only one sync should be going on at a time
+(expect
+ ;; describe-database gets called twice during a single sync process, once for syncing tables and a second time for
+ ;; syncing the _metabase_metadata table
+ 2
+ (tt/with-temp* [Database [db {:engine :concurrent-sync-test}]]
+ (reset! calls-to-describe-database 0)
+ ;; start a sync processes in the background. It should take 1000 ms to finish
+ (let [f1 (future (sync/sync-database! db))
+ f2 (do
+ ;; wait 200 ms to make sure everything is going
+ (Thread/sleep 200)
+ ;; Start another in the background. Nothing should happen here because the first is already running
+ (future (sync/sync-database! db)))]
+ ;; Start another in the foreground. Again, nothing should happen here because the original should still be
+ ;; running
+ (sync/sync-database! db)
+ ;; make sure both of the futures have finished
+ (deref f1)
+ (deref f2)
+ ;; Check the number of syncs that took place. Should be 2 (just the first)
+ @calls-to-describe-database)))
diff --git a/test/metabase/sync_database_test.clj b/test/metabase/sync_database_test.clj
index e3b3b251311389e538d1e812f91b8a0d612393f5..3fca12bcc7af2b50956147819db3841fb006e887 100644
--- a/test/metabase/sync_database_test.clj
+++ b/test/metabase/sync_database_test.clj
@@ -1,27 +1,30 @@
-(ns metabase.sync-database-test
+(ns ^:deprecated metabase.sync-database-test
"Tests for sync behavior that use a imaginary `SyncTestDriver`. These are kept around mainly because they've already
- been written. For newer sync tests see `metabase.sync.*` test namespaces."
- (:require [clojure.java.jdbc :as jdbc]
- [expectations :refer :all]
+ been written. For newer sync tests see `metabase.sync.*` test namespaces.
+
+ Your new tests almost certainly do not belong in this namespace. Please put them in ones mirroring the location of
+ the specific part of sync you're testing."
+ (:require [expectations :refer :all]
[metabase
- [db :as mdb]
[driver :as driver]
- [sync :as sync :refer :all]
+ [sync :as sync]
[util :as u]]
- [metabase.driver.generic-sql :as sql]
[metabase.models
[database :refer [Database]]
[field :refer [Field]]
- [field-values :as field-values :refer [FieldValues]]
[table :refer [Table]]]
- [metabase.sync.field-values-test :as sync-field-values-test]
- [metabase.test
- [data :as data]
- [util :as tu]]
[metabase.test.mock.util :as mock-util]
+ [metabase.test.util :as tu]
[toucan.db :as db]
[toucan.util.test :as tt]))
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | End-to-end 'MovieDB' Sync Tests |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+;; These tests make up a fake driver and then confirm that sync uses the various methods defined by the driver to
+;; correctly sync appropriate metadata rows (Table/Field/etc.) in the Application DB
+
(def ^:private ^:const sync-test-tables
{"movie" {:name "movie"
:schema "default"
@@ -168,10 +171,10 @@
:base_type :type/Text
:special_type :type/PK})]})]
(tt/with-temp Database [db {:engine :sync-test}]
- (sync-database! db)
+ (sync/sync-database! db)
;; we are purposely running the sync twice to test for possible logic issues which only manifest on resync of a
;; database, such as adding tables that already exist or duplicating fields
- (sync-database! db)
+ (sync/sync-database! db)
(mapv table-details (db/select Table, :db_id (u/get-id db), {:order-by [:name]}))))
@@ -202,331 +205,13 @@
Table [table {:name "movie"
:schema "default"
:db_id (u/get-id db)}]]
- (sync-table! table)
+ (sync/sync-table! table)
(table-details (Table (:id table)))))
-;; test that we prevent running simultaneous syncs on the same database
-
-(defonce ^:private calls-to-describe-database (atom 0))
-
-(defrecord ConcurrentSyncTestDriver []
- clojure.lang.Named
- (getName [_] "ConcurrentSyncTestDriver"))
-
-(extend ConcurrentSyncTestDriver
- driver/IDriver
- (merge driver/IDriverDefaultsMixin
- {:describe-database (fn [_ _]
- (swap! calls-to-describe-database inc)
- (Thread/sleep 1000)
- {:tables #{}})
- :describe-table (constantly nil)
- :details-fields (constantly [])}))
-
-(driver/register-driver! :concurrent-sync-test (ConcurrentSyncTestDriver.))
-
-;; only one sync should be going on at a time
-(expect
- ;; describe-database gets called twice during a single sync process, once for syncing tables and a second time for
- ;; syncing the _metabase_metadata table
- 2
- (tt/with-temp* [Database [db {:engine :concurrent-sync-test}]]
- (reset! calls-to-describe-database 0)
- ;; start a sync processes in the background. It should take 1000 ms to finish
- (let [f1 (future (sync-database! db))
- f2 (do
- ;; wait 200 ms to make sure everything is going
- (Thread/sleep 200)
- ;; Start another in the background. Nothing should happen here because the first is already running
- (future (sync-database! db)))]
- ;; Start another in the foreground. Again, nothing should happen here because the original should still be
- ;; running
- (sync-database! db)
- ;; make sure both of the futures have finished
- (deref f1)
- (deref f2)
- ;; Check the number of syncs that took place. Should be 2 (just the first)
- @calls-to-describe-database)))
-
-
-;; Test that we will remove field-values when they aren't appropriate. Calling `sync-database!` below should cause
-;; them to get removed since the Field isn't `has_field_values` = `list`
-(expect
- [[1 2 3]
- nil]
- (tt/with-temp* [Database [db {:engine :sync-test}]]
- ()
- (sync-database! db)
- (let [table-id (db/select-one-id Table, :schema "default", :name "movie")
- field-id (db/select-one-id Field, :table_id table-id, :name "studio")]
- (tt/with-temp FieldValues [_ {:field_id field-id
- :values "[1,2,3]"}]
- (let [initial-field-values (db/select-one-field :values FieldValues, :field_id field-id)]
- (sync-database! db)
- [initial-field-values
- (db/select-one-field :values FieldValues, :field_id field-id)])))))
-
-
-;; ## Individual Helper Fns
-
-(defn- force-sync-table!
- "Updates the `:fields_hash` to ensure that the sync process will include fields in the sync"
- [table]
- (db/update! Table (u/get-id table), :fields_hash "something new")
- (sync-table! (Table (data/id :venues))))
-
-;; ## TEST PK SYNCING
-(expect [:type/PK
- nil
- :type/PK
- :type/Latitude
- :type/PK]
- (let [get-special-type (fn [] (db/select-one-field :special_type Field, :id (data/id :venues :id)))]
- [;; Special type should be :id to begin with
- (get-special-type)
- ;; Clear out the special type
- (do (db/update! Field (data/id :venues :id), :special_type nil)
- (get-special-type))
- ;; Calling sync-table! should set the special type again
- (do (force-sync-table! (data/id :venues))
- (get-special-type))
- ;; sync-table! should *not* change the special type of fields that are marked with a different type
- (do (db/update! Field (data/id :venues :id), :special_type :type/Latitude)
- (get-special-type))
- ;; Make sure that sync-table runs set-table-pks-if-needed!
- (do (db/update! Field (data/id :venues :id), :special_type nil)
- (force-sync-table! (Table (data/id :venues)))
- (get-special-type))]))
-
-;; ## FK SYNCING
-
-;; Check that Foreign Key relationships were created on sync as we expect
-
-(expect (data/id :venues :id)
- (db/select-one-field :fk_target_field_id Field, :id (data/id :checkins :venue_id)))
-
-(expect (data/id :users :id)
- (db/select-one-field :fk_target_field_id Field, :id (data/id :checkins :user_id)))
-
-(expect (data/id :categories :id)
- (db/select-one-field :fk_target_field_id Field, :id (data/id :venues :category_id)))
-
-;; Check that sync-table! causes FKs to be set like we'd expect
-(expect [{:special_type :type/FK, :fk_target_field_id true}
- {:special_type nil, :fk_target_field_id false}
- {:special_type :type/FK, :fk_target_field_id true}]
- (let [field-id (data/id :checkins :user_id)
- get-special-type-and-fk-exists? (fn []
- (into {} (-> (db/select-one [Field :special_type :fk_target_field_id],
- :id field-id)
- (update :fk_target_field_id #(db/exists? Field :id %)))))]
- [ ;; FK should exist to start with
- (get-special-type-and-fk-exists?)
- ;; Clear out FK / special_type
- (do (db/update! Field field-id, :special_type nil, :fk_target_field_id nil)
- (get-special-type-and-fk-exists?))
- ;; Run sync-table and they should be set again
- (let [table (Table (data/id :checkins))]
- (sync-table! table)
- (get-special-type-and-fk-exists?))]))
-
-
-;;; ## FieldValues Syncing
-
-(let [get-field-values (fn [] (db/select-one-field :values FieldValues, :field_id (data/id :venues :price)))
- get-field-values-id (fn [] (db/select-one-id FieldValues, :field_id (data/id :venues :price)))]
- ;; Test that when we delete FieldValues syncing the Table again will cause them to be re-created
- (expect
- [[1 2 3 4] ; 1
- nil ; 2
- [1 2 3 4]] ; 3
- [ ;; 1. Check that we have expected field values to start with
- (get-field-values)
- ;; 2. Delete the Field values, make sure they're gone
- (do (db/delete! FieldValues :id (get-field-values-id))
- (get-field-values))
- ;; 3. Now re-sync the table and make sure they're back
- (do (sync-table! (Table (data/id :venues)))
- (get-field-values))])
-
- ;; Test that syncing will cause FieldValues to be updated
- (expect
- [[1 2 3 4] ; 1
- [1 2 3] ; 2
- [1 2 3 4]] ; 3
- [ ;; 1. Check that we have expected field values to start with
- (get-field-values)
- ;; 2. Update the FieldValues, remove one of the values that should be there
- (do (db/update! FieldValues (get-field-values-id), :values [1 2 3])
- (get-field-values))
- ;; 3. Now re-sync the table and make sure the value is back
- (do (sync-table! (Table (data/id :venues)))
- (get-field-values))]))
-
-(defn- exec! [conn statements]
- (doseq [statement statements]
- (jdbc/execute! conn [statement])))
-
-(defmacro ^:private with-new-mem-db
- "Setup a in-memory H2 database with a `Database` instance bound to `db-sym` and a connection to that H3 database
- bound to `conn-sym`."
- [db-sym conn-sym & body]
- `(let [details# {:db (str "mem:" (tu/random-name) ";DB_CLOSE_DELAY=10")}]
- (binding [mdb/*allow-potentailly-unsafe-connections* true]
- (tt/with-temp Database [db# {:engine :h2, :details details#}]
- (jdbc/with-db-connection [conn# (sql/connection-details->spec (driver/engine->driver :h2) details#)]
- (let [~db-sym db#
- ~conn-sym conn#]
- ~@body))))))
-
-;; TODO - hey, what is this testing? If you wrote this test, please explain what's going on here
-(defn- narrow-to-min-max [row]
- (-> row
- (get-in [:type :type/Number])
- (select-keys [:min :max])
- (update :min #(u/round-to-decimals 4 %))
- (update :max #(u/round-to-decimals 4 %))))
-
-(expect
- [{:min -165.374 :max -73.9533}
- {:min 10.0646 :max 40.7794}]
- (tt/with-temp* [Database [database {:details (:details (Database (data/id))), :engine :h2}]
- Table [table {:db_id (u/get-id database), :name "VENUES"}]]
- (sync-table! table)
- (map narrow-to-min-max
- [(db/select-one-field :fingerprint Field, :id (data/id :venues :longitude))
- (db/select-one-field :fingerprint Field, :id (data/id :venues :latitude))])))
-
-(defmacro ^{:style/indent 2} throw-if-called [fn-var & body]
- `(with-redefs [~fn-var (fn [& args#]
- (throw (RuntimeException. "Should not be called!")))]
- ~@body))
-
-;; Validate the changing of a column's type triggers a hash miss and sync
-(expect
- [ ;; Original column type
- "SMALLINT"
- ;; Altered the column, now it's an integer
- "INTEGER"
- ;; Original hash and the new one are not equal
- false
- ;; Reruning sync shouldn't change the hash
- true]
- (with-new-mem-db db conn
- (let [get-table #(db/select-one Table :db_id (u/get-id db))]
- ;; create the `blueberries_consumed` table and insert 50 values
- (exec! conn ["CREATE TABLE blueberries_consumed (num SMALLINT NOT NULL);"
- (sync-field-values-test/insert-range-sql (range 50))])
- (sync-database! db)
- ;; After this sync, we know about the new table and it's SMALLINT column
- (let [table-id (u/get-id (get-table))
- get-field #(db/select-one Field :table_id table-id)
- {old-hash :fields_hash} (get-table)
- {old-db-type :database_type} (get-field)]
- ;; Change the column from SMALLINT to INTEGER. In clojure-land these are both integers, but this change
- ;; should trigger a hash miss and thus resync the table, since something has changed
- (exec! conn ["ALTER TABLE blueberries_consumed ALTER COLUMN num INTEGER"])
- (sync-database! db)
- (let [{new-hash :fields_hash} (get-table)
- {new-db-type :database_type} (get-field)]
-
- ;; Syncing again with no change should not call sync-field-instances! or update the hash
- (throw-if-called metabase.sync.sync-metadata.fields/sync-field-instances!
- (sync-database! db)
- [old-db-type
- new-db-type
- (= old-hash new-hash)
- (= new-hash (:fields_hash (get-table)))]))))))
-
-(defn- table-md-with-hash [table-id]
- {:active-fields (count (db/select Field :table_id table-id :active true))
- :inactive-fields (count (db/select Field :table_id table-id :active false))
- :fields-hash (:fields_hash (db/select-one Table :id table-id))})
-
-(defn- no-fields-hash [m]
- (dissoc m :fields-hash))
-
-;; This tests a table that adds a column, ensures sync picked up the new column and the hash changed
-(expect
- [
- ;; Only the num column should be found
- {:active-fields 1, :inactive-fields 0}
- ;; Add a column, should still be no inactive
- {:active-fields 2, :inactive-fields 0}
- ;; Adding a column should make the hashes not equal
- false
- ]
- (with-new-mem-db db conn
- (let [get-table #(db/select-one Table :db_id (u/get-id db))]
- ;; create the `blueberries_consumed` table and insert 50 values
- (exec! conn ["CREATE TABLE blueberries_consumed (num SMALLINT NOT NULL)"
- (sync-field-values-test/insert-range-sql (range 50))])
- (sync-database! db)
- ;; We should now have a hash value for num as a SMALLINT
- (let [table-id (u/get-id (get-table))
- before-table-md (table-md-with-hash table-id)
- _ (exec! conn ["ALTER TABLE blueberries_consumed ADD COLUMN weight FLOAT"])
- _ (sync-database! db)
- ;; Now that hash will include num and weight
- after-table-md (table-md-with-hash table-id)]
- [(no-fields-hash before-table-md)
- (no-fields-hash after-table-md)
- (= (:fields-hash before-table-md)
- (:fields-hash after-table-md))]))))
-
-;; Drops a column, ensures sync finds the drop, updates the hash
-(expect
- [
- ;; Test starts with two columns
- {:active-fields 2, :inactive-fields 0}
- ;; Dropped the weight column
- {:active-fields 1, :inactive-fields 1}
- ;; Hashes should be different without the weight column
- false]
- (with-new-mem-db db conn
- (let [get-table #(db/select-one Table :db_id (u/get-id db))]
- ;; create the `blueberries_consumed` table and insert 50 values
- (exec! conn ["CREATE TABLE blueberries_consumed (num SMALLINT NOT NULL, weight FLOAT)"
- (sync-field-values-test/insert-range-sql (range 50))])
- (sync-database! db)
- ;; We should now have a hash value for num as a SMALLINT
- (let [table-id (u/get-id (get-table))
- before-table-md (table-md-with-hash table-id)
- _ (exec! conn ["ALTER TABLE blueberries_consumed DROP COLUMN weight"])
- _ (sync-database! db)
- ;; Now that hash will include num and weight
- after-table-md (table-md-with-hash table-id)]
- [(no-fields-hash before-table-md)
- (no-fields-hash after-table-md)
- (= (:fields-hash before-table-md)
- (:fields-hash after-table-md))]))))
-
-;; Drops and readds a column, ensures that the hash is back to it's original value
-(expect
- [
- ;; Both num and weight columns should be found
- {:active-fields 2, :inactive-fields 0}
- ;; Both columns should still be present
- {:active-fields 2, :inactive-fields 0}
- ;; The hashes should be the same
- true]
- (with-new-mem-db db conn
- (let [get-table #(db/select-one Table :db_id (u/get-id db))]
- ;; create the `blueberries_consumed` table and insert 50 values
- (exec! conn ["CREATE TABLE blueberries_consumed (num SMALLINT NOT NULL, weight FLOAT)"
- (sync-field-values-test/insert-range-sql (range 50))])
- (sync-database! db)
- ;; We should now have a hash value for num as a SMALLINT
- (let [table-id (u/get-id (get-table))
- before-table-md (table-md-with-hash table-id)
- _ (exec! conn ["ALTER TABLE blueberries_consumed DROP COLUMN weight"])
- _ (sync-database! db)
- _ (exec! conn ["ALTER TABLE blueberries_consumed ADD COLUMN weight FLOAT"])
- _ (sync-database! db)
- ;; Now that hash will include num and weight
- after-table-md (table-md-with-hash table-id)]
- [(no-fields-hash before-table-md)
- (no-fields-hash after-table-md)
- (= (:fields-hash before-table-md)
- (:fields-hash after-table-md))]))) )
+;; !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
+;; !! !!
+;; !! HEY! Your tests probably don't belong in this namespace! Put them in one appropriate to the specific part of !!
+;; !! sync they are testing. !!
+;; !! !!
+;; !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
diff --git a/test/metabase/test/data/one_off_dbs.clj b/test/metabase/test/data/one_off_dbs.clj
new file mode 100644
index 0000000000000000000000000000000000000000..b551a819ccacf3ed95c69816d468532df6f9e5c5
--- /dev/null
+++ b/test/metabase/test/data/one_off_dbs.clj
@@ -0,0 +1,76 @@
+(ns metabase.test.data.one-off-dbs
+ "Test utility functions for using one-off temporary in-memory H2 databases, including completely blank ones and the
+ infamous `blueberries_consumed` database, used by sync tests in several different namespaces."
+ (:require [clojure.java.jdbc :as jdbc]
+ [clojure.string :as str]
+ [metabase
+ [db :as mdb]
+ [driver :as driver]
+ [sync :as sync]]
+ [metabase.driver.generic-sql :as sql]
+ [metabase.models.database :refer [Database]]
+ [metabase.test
+ [data :as data]
+ [util :as tu]]
+ [toucan.util.test :as tt]))
+
+(def ^:dynamic *conn*
+ "Bound to a JDBC connection spec when using one of the `with-db` macros below."
+ nil)
+
+;;; ---------------------------------------- Generic Empty Temp In-Memory DB -----------------------------------------
+
+(defn do-with-blank-db
+ "Impl for `with-blank-db` macro; prefer that to using this directly."
+ [f]
+ (let [details {:db (str "mem:" (tu/random-name) ";DB_CLOSE_DELAY=10")}]
+ (binding [mdb/*allow-potentailly-unsafe-connections* true]
+ (tt/with-temp Database [db {:engine :h2, :details details}]
+ (data/with-db db
+ (jdbc/with-db-connection [conn (sql/connection-details->spec (driver/engine->driver :h2) details)]
+ (binding [*conn* conn]
+ (f))))))))
+
+(defmacro with-blank-db
+ "An empty canvas upon which you may paint your dreams.
+
+ Creates a one-off tempory in-memory H2 database and binds this DB with `data/with-db` so you can use `data/db` and
+ `data/id` to access it. `*conn*` is bound to a JDBC connection spec so you can execute DDL statements to populate it
+ as needed."
+ {:style/indent 0}
+ [& body]
+ `(do-with-blank-db (fn [] ~@body)))
+
+
+;;; ------------------------------------------------- Blueberries DB -------------------------------------------------
+
+(defn do-with-blueberries-db
+ "Impl for `with-blueberries-db` macro; use that instead of using this directly."
+ [f]
+ (with-blank-db
+ (jdbc/execute! *conn* ["CREATE TABLE blueberries_consumed (num SMALLINT NOT NULL);"])
+ (f)))
+
+(defmacro with-blueberries-db
+ "Creates a database with a single table, `blueberries_consumed`, with one column, `num`."
+ {:style/indent 0}
+ [& body]
+ `(do-with-blueberries-db (fn [] ~@body)))
+
+
+;;; ------------------------------------ Helper Fns for Populating Blueberries DB ------------------------------------
+
+(defn- insert-range-sql
+ "Generate SQL to insert a row for each number in `rang`."
+ [rang]
+ (str "INSERT INTO blueberries_consumed (num) VALUES "
+ (str/join ", " (for [n rang]
+ (str "(" n ")")))))
+
+(defn insert-rows-and-sync!
+ "With the temp blueberries db from above, insert a `range` of values and re-sync the DB.
+
+ (insert-rows-and-sync! [0 1 2 3]) ; insert 4 rows"
+ [rang]
+ (jdbc/execute! *conn* [(insert-range-sql rang)])
+ (sync/sync-database! (data/db)))