From 8f2cc20461f753e5ba77256811bb38c1df55b325 Mon Sep 17 00:00:00 2001
From: Noah Moss <32746338+noahmoss@users.noreply.github.com>
Date: Mon, 6 Dec 2021 12:42:40 -0500
Subject: [PATCH] Backend for DB sync progress UX (#18952)
---
resources/migrations/000_migrations.yaml | 67 ++++++++++++++++++++++
src/metabase/api/activity.clj | 2 +-
src/metabase/api/database.clj | 3 +-
src/metabase/api/search.clj | 10 ++--
src/metabase/api/setup.clj | 12 ++--
src/metabase/models/database.clj | 4 +-
src/metabase/models/table.clj | 5 +-
src/metabase/public_settings.clj | 13 +++++
src/metabase/search/config.clj | 3 +-
src/metabase/sync/sync_metadata.clj | 6 +-
src/metabase/sync/sync_metadata/fks.clj | 3 +
src/metabase/sync/util.clj | 53 +++++++++++++----
test/metabase/api/activity_test.clj | 9 +--
test/metabase/api/database_test.clj | 72 +++++++++++++-----------
test/metabase/api/field_test.clj | 8 +--
test/metabase/api/search_test.clj | 16 +++---
test/metabase/api/table_test.clj | 16 +++---
test/metabase/sync/util_test.clj | 58 +++++++++++++++++--
test/metabase/sync_test.clj | 16 +++---
19 files changed, 281 insertions(+), 95 deletions(-)
diff --git a/resources/migrations/000_migrations.yaml b/resources/migrations/000_migrations.yaml
index d7e37ec3c03..a6bf10be517 100644
--- a/resources/migrations/000_migrations.yaml
+++ b/resources/migrations/000_migrations.yaml
@@ -9795,6 +9795,73 @@ databaseChangeLog:
constraints:
nullable: false
+ - changeSet:
+ id: v42.00-066
+ author: noahmoss
+ comment: >-
+ Added 0.42.0 - new columns for initial DB sync progress UX. Indicates whether a database has succesfully synced
+ at least one time.
+ changes:
+ - addColumn:
+ tableName: metabase_database
+ columns:
+ - column:
+ name: initial_sync_status
+ type: varchar(32)
+ remarks: "String indicating whether a database has completed its initial sync and is ready to use"
+ defaultValue: "complete"
+ constraints:
+ nullable: false
+
+ - changeSet:
+ id: v42.00-067
+ author: noahmoss
+ comment: >-
+ Added 0.42.0 - new columns for initial DB sync progress UX. Indicates whether a table has succesfully synced
+ at least one time.
+ changes:
+ - addColumn:
+ tableName: metabase_table
+ columns:
+ - column:
+ name: initial_sync_status
+ type: varchar(32)
+ remarks: "String indicating whether a table has completed its initial sync and is ready to use"
+ defaultValue: "complete"
+ constraints:
+ nullable: false
+
+ - changeSet:
+ id: v42.00-068
+ author: noahmoss
+ comment: >-
+ Added 0.42.0 - new columns for initial DB sync progress UX. Records the ID of the admin who added a database.
+ May be null for the sample dataset, or for databases added prior to 0.42.0.
+ changes:
+ - addColumn:
+ tableName: metabase_database
+ columns:
+ - column:
+ name: creator_id
+ type: int
+ remarks: "ID of the admin who added the database"
+ constraints:
+ nullable: true
+
+ - changeSet:
+ id: v42.00-069
+ author: noahmoss
+ comment: >-
+ Added 0.42.0 - adds FK constraint for creator_id column, containing the ID of the admin who added a database.
+ changes:
+ - addForeignKeyConstraint:
+ baseTableName: metabase_database
+ baseColumnNames: creator_id
+ referencedTableName: core_user
+ referencedColumnNames: id
+ constraintName: fk_database_creator_id
+ onDelete: SET NULL
+
# >>>>>>>>>> DO NOT ADD NEW MIGRATIONS BELOW THIS LINE! ADD THEM ABOVE <<<<<<<<<<
########################################################################################################################
diff --git a/src/metabase/api/activity.clj b/src/metabase/api/activity.clj
index de9aafae95c..87820c47436 100644
--- a/src/metabase/api/activity.clj
+++ b/src/metabase/api/activity.clj
@@ -96,7 +96,7 @@
"card" [Card :id :name :collection_id :description :display
:dataset_query :dataset]
"dashboard" [Dashboard :id :name :collection_id :description]
- "table" [Table :id :name :db_id :display_name])
+ "table" [Table :id :name :db_id :display_name :initial_sync_status])
{:where [:in :id ids]})))
(by-id [models] (m/index-by :id models))]
(into {} (map (fn [[model models]]
diff --git a/src/metabase/api/database.clj b/src/metabase/api/database.clj
index e093e6f630d..84fe1611864 100644
--- a/src/metabase/api/database.clj
+++ b/src/metabase/api/database.clj
@@ -501,7 +501,8 @@
:details details-or-error
:is_full_sync is-full-sync?
:is_on_demand (boolean is_on_demand)
- :cache_ttl cache_ttl}
+ :cache_ttl cache_ttl
+ :creator_id api/*current-user-id*}
(sync.schedules/schedule-map->cron-strings
(if (:let-user-control-scheduling details)
(sync.schedules/scheduling schedules)
diff --git a/src/metabase/api/search.clj b/src/metabase/api/search.clj
index 9f2fd473d39..f83bb8cae50 100644
--- a/src/metabase/api/search.clj
+++ b/src/metabase/api/search.clj
@@ -89,7 +89,9 @@
:database_id :integer
:table_schema :text
:table_name :text
- :table_description :text))
+ :table_description :text
+ ;; returned for Database and Table
+ :initial_sync_status :text))
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | Shared Query Logic |
@@ -317,7 +319,7 @@
{:select (:select base-query)
:from [[(merge
base-query
- {:select [:id :schema :db_id :name :description :display_name :updated_at
+ {:select [:id :schema :db_id :name :description :display_name :updated_at :initial_sync_status
[(hx/concat (hx/literal "/db/")
:db_id
(hx/literal "/schema/")
@@ -341,7 +343,7 @@
columns-to-search (->> all-search-columns
(filter (fn [[k v]] (= v :text)))
(map first)
- (remove #{:collection_authority_level :moderated_status}))
+ (remove #{:collection_authority_level :moderated_status :initial_sync_status}))
case-clauses (as-> columns-to-search <>
(map (fn [col] [:like (hsql/call :lower col) match]) <>)
(interleave <> (repeat 0))
@@ -438,7 +440,7 @@
models :- (s/maybe models-schema)
limit :- (s/maybe su/IntGreaterThanZero)
offset :- (s/maybe su/IntGreaterThanOrEqualToZero)]
- (cond-> {:search-string search-string
+ (cond-> {:search-string search-string
:archived? (Boolean/parseBoolean archived-string)
:current-user-perms @api/*current-user-permissions-set*}
(some? table-db-id) (assoc :table-db-id table-db-id)
diff --git a/src/metabase/api/setup.clj b/src/metabase/api/setup.clj
index ab3078a6798..35656edd759 100644
--- a/src/metabase/api/setup.clj
+++ b/src/metabase/api/setup.clj
@@ -58,14 +58,14 @@
(defn- setup-create-database!
"Create a new Database. Returns newly created Database."
- [{:keys [name driver details schedules database]}]
+ [{:keys [name driver details schedules database creator-id]}]
(when driver
(when-not (some-> (u/ignore-exceptions (driver/the-driver driver)) driver/available?)
(let [msg (tru "Cannot create Database: cannot find driver {0}." driver)]
(throw (ex-info msg {:errors {:database {:engine msg}}, :status-code 400}))))
(db/insert! Database
(merge
- {:name name, :engine driver, :details details}
+ {:name name, :engine driver, :details details, :creator_id creator-id}
(u/select-non-nil-keys database #{:is_on_demand :is_full_sync :auto_run_queries})
(when schedules
(sync.schedules/schedule-map->cron-strings schedules))))))
@@ -106,8 +106,12 @@
(db/transaction
(let [user-info (setup-create-user!
{:email email, :first-name first_name, :last-name last_name, :password password})
- db (setup-create-database!
- {:name name, :driver engine, :details details, :schedules schedules, :database database})]
+ db (setup-create-database! {:name name
+ :driver engine
+ :details details
+ :schedules schedules
+ :database database
+ :creator-id (:user-id user-info)})]
(setup-set-settings!
request
{:email email, :site-name site_name, :site-locale site_locale, :allow-tracking? allow_tracking})
diff --git a/src/metabase/models/database.clj b/src/metabase/models/database.clj
index d1a1d4c43ea..c84f859fc1b 100644
--- a/src/metabase/models/database.clj
+++ b/src/metabase/models/database.clj
@@ -176,7 +176,9 @@
:cache_field_values_schedule new-fieldvalues-schedule)))))))
(defn- pre-insert [database]
- (handle-secrets-changes database))
+ (-> database
+ handle-secrets-changes
+ (assoc :initial_sync_status "incomplete")))
(defn- perms-objects-set [database _]
#{(perms/data-perms-path (u/the-id database))})
diff --git a/src/metabase/models/table.clj b/src/metabase/models/table.clj
index 1bf0d418c4c..3b451dfd3a4 100644
--- a/src/metabase/models/table.clj
+++ b/src/metabase/models/table.clj
@@ -37,8 +37,9 @@
;;; --------------------------------------------------- Lifecycle ----------------------------------------------------
(defn- pre-insert [table]
- (let [defaults {:display_name (humanization/name->human-readable-name (:name table))
- :field_order (driver/default-field-order (-> table :db_id Database :engine))}]
+ (let [defaults {:display_name (humanization/name->human-readable-name (:name table))
+ :field_order (driver/default-field-order (-> table :db_id Database :engine))
+ :initial_sync_status "incomplete"}]
(merge defaults table)))
(defn- pre-delete [{:keys [db_id schema id]}]
diff --git a/src/metabase/public_settings.clj b/src/metabase/public_settings.clj
index 3010e8ea852..415fd8c53b5 100644
--- a/src/metabase/public_settings.clj
+++ b/src/metabase/public_settings.clj
@@ -453,3 +453,16 @@
:type :json
:setter :none
:getter fetch-cloud-gateway-ips-fn)
+
+(defsetting enable-database-syncing-modal
+ (str (deferred-tru "Whether an introductory modal should be shown after the next database connection is added.")
+ " "
+ (deferred-tru "Defaults to false if any non-default database connections already exist for this instance."))
+ :visibility :admin
+ :type :boolean
+ :getter (fn []
+ (let [v (setting/get-boolean :enable-database-syncing-modal)]
+ (if (nil? v)
+ (not (db/exists? 'Database :is_sample false))
+ ;; frontend should set this value to `true` after the modal has been shown once
+ v))))
diff --git a/src/metabase/search/config.clj b/src/metabase/search/config.clj
index d73e989885f..ea9f237690b 100644
--- a/src/metabase/search/config.clj
+++ b/src/metabase/search/config.clj
@@ -145,7 +145,7 @@
(defmethod columns-for-model "database"
[_]
- [:id :name :description :updated_at])
+ [:id :name :description :updated_at :initial_sync_status])
(defmethod columns-for-model "pulse"
[_]
@@ -171,6 +171,7 @@
:display_name
:description
:updated_at
+ :initial_sync_status
[:id :table_id]
[:db_id :database_id]
[:schema :table_schema]
diff --git a/src/metabase/sync/sync_metadata.clj b/src/metabase/sync/sync_metadata.clj
index f612cd49cc5..97e5114e20e 100644
--- a/src/metabase/sync/sync_metadata.clj
+++ b/src/metabase/sync/sync_metadata.clj
@@ -13,6 +13,7 @@
[metabase.sync.sync-metadata.sync-timezone :as sync-tz]
[metabase.sync.sync-metadata.tables :as sync-tables]
[metabase.sync.util :as sync-util]
+ [metabase.util :as u]
[metabase.util.i18n :refer [trs]]
[schema.core :as s]))
@@ -46,7 +47,10 @@
"Sync the metadata for a Metabase `database`. This makes sure child Table & Field objects are synchronized."
[database :- i/DatabaseInstance]
(sync-util/sync-operation :sync-metadata database (format "Sync metadata for %s" (sync-util/name-for-logging database))
- (sync-util/run-sync-operation "sync" database sync-steps)))
+ (u/prog1 (sync-util/run-sync-operation "sync" database sync-steps)
+ (if (some sync-util/abandon-sync? (map second (:steps <>)))
+ (sync-util/set-initial-database-sync-aborted! database)
+ (sync-util/set-initial-database-sync-complete! database)))))
(s/defn sync-table-metadata!
"Sync the metadata for an individual `table` -- make sure Fields and FKs are up-to-date."
diff --git a/src/metabase/sync/sync_metadata/fks.clj b/src/metabase/sync/sync_metadata/fks.clj
index bea6c74762d..fdb2a8ff020 100644
--- a/src/metabase/sync/sync_metadata/fks.clj
+++ b/src/metabase/sync/sync_metadata/fks.clj
@@ -77,6 +77,9 @@
[database :- i/DatabaseInstance]
(reduce (fn [update-info table]
(let [table-fk-info (sync-fks-for-table! database table)]
+ ;; Mark the table as done with its initial sync once this step is done even if it failed, because only
+ ;; sync-aborting errors should be surfaced to the UI (see [[sync-util/exception-classes-not-to-retry]]).
+ (sync-util/set-initial-table-sync-complete! table)
(if (instance? Exception table-fk-info)
(update update-info :total-failed inc)
(merge-with + update-info table-fk-info))))
diff --git a/src/metabase/sync/util.clj b/src/metabase/sync/util.clj
index b93a133be9e..f8d5406ffb8 100644
--- a/src/metabase/sync/util.clj
+++ b/src/metabase/sync/util.clj
@@ -10,6 +10,7 @@
[metabase.driver :as driver]
[metabase.driver.util :as driver.u]
[metabase.events :as events]
+ [metabase.models.database :refer [Database]]
[metabase.models.table :refer [Table]]
[metabase.models.task-history :refer [TaskHistory]]
[metabase.query-processor.interface :as qpi]
@@ -251,6 +252,32 @@
~emoji-progress-fn-binding (fn [] (emoji-progress-bar (swap! finished-count# inc) total-count# log-every-n#))]
~@body))
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | INITIAL SYNC STATUS |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+;; If this is the first sync of a database, we need to update the `initial_sync_status` field on individual tables
+;; when they have finished syncing, as well as the corresponding field on the database itself when the entire sync
+;; is complete (excluding analysis). This powers a UX that displays the progress of the initial sync to the admin who
+;; added the database, and enables individual tables when they become usable for queries.
+
+(defn set-initial-table-sync-complete!
+ "Marks initial sync as complete for this table so that it becomes usable in the UI, if not already set"
+ [table]
+ (when (not= (:initial_sync_status table) "complete")
+ (db/update! Table (u/the-id table) :initial_sync_status "complete")))
+
+(defn set-initial-database-sync-complete!
+ "Marks initial sync as complete for this database so that this is reflected in the UI, if not already set"
+ [database]
+ (when (not= (:initial_sync_status database) "complete")
+ (db/update! Database (u/the-id database) :initial_sync_status "complete")))
+
+(defn set-initial-database-sync-aborted!
+ "Marks initial sync as aborted for this database so that an error can be displayed on the UI"
+ [database]
+ (when (not= (:initial_sync_status database) "complete")
+ (db/update! Database (u/the-id database) :initial_sync_status "aborted")))
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | OTHER SYNC UTILITY FUNCTIONS |
@@ -349,7 +376,7 @@
(comp str log-summary-fn))}))
(s/defn run-step-with-metadata :- StepNameWithMetadata
- "Runs `step` on `database returning metadata from the run"
+ "Runs `step` on `database` returning metadata from the run"
[database :- i/DatabaseInstance
{:keys [step-name sync-fn log-summary-fn] :as step} :- StepDefinition]
(let [start-time (t/zoned-date-time)
@@ -441,6 +468,16 @@
(catch Throwable e
(log/warn e (trs "Error saving task history")))))
+(defn abandon-sync?
+ "Given the results of a sync step, returns true if a non-recoverable exception occurred"
+ [step-results]
+ (when (contains? step-results :throwable)
+ (let [caught-exception (:throwable step-results)
+ exception-classes (u/full-exception-chain caught-exception)]
+ (some true? (for [ex exception-classes
+ test-ex exception-classes-not-to-retry]
+ (= (.. ^Object ex getClass getName) (.. ^Class test-ex getName)))))))
+
(s/defn run-sync-operation
"Run `sync-steps` and log a summary message"
[operation :- s/Str
@@ -451,17 +488,9 @@
result []]
(let [[step-name r] (run-step-with-metadata database step-defn)
new-result (conj result [step-name r])]
- (if (contains? r :throwable)
- (let [caught-exception (:throwable r)
- exception-classes (u/full-exception-chain caught-exception)
- abandon? (some true? (for [ex exception-classes
- test-ex exception-classes-not-to-retry]
- (= (.. ^Object ex getClass getName) (.. ^Class test-ex getName))))]
- (cond abandon? new-result
- (not (seq rest-defns)) new-result
- :else (recur rest-defns new-result)))
- (cond (not (seq rest-defns)) new-result
- :else (recur rest-defns new-result)))))
+ (cond (abandon-sync? r) new-result
+ (not (seq rest-defns)) new-result
+ :else (recur rest-defns new-result))))
end-time (t/zoned-date-time)
sync-metadata {:start-time start-time
:end-time end-time
diff --git a/test/metabase/api/activity_test.clj b/test/metabase/api/activity_test.clj
index b7bb26f358b..f34b779aebc 100644
--- a/test/metabase/api/activity_test.clj
+++ b/test/metabase/api/activity_test.clj
@@ -122,10 +122,11 @@
(is (= [{:cnt 1,
:model "table",
:model_id (:id table1),
- :model_object {:db_id (:db_id table1),
- :id (:id table1),
- :name (:name table1)
- :display_name (:display_name table1)},
+ :model_object {:db_id (:db_id table1),
+ :id (:id table1),
+ :name (:name table1)
+ :display_name (:display_name table1)
+ :initial_sync_status "incomplete"},
:user_id (mt/user->id :crowberto)}
{:cnt 1
:user_id (mt/user->id :crowberto)
diff --git a/test/metabase/api/database_test.clj b/test/metabase/api/database_test.clj
index 41654584da3..513fc174fb7 100644
--- a/test/metabase/api/database_test.clj
+++ b/test/metabase/api/database_test.clj
@@ -45,19 +45,21 @@
(defn- db-details
"Return default column values for a database (either the test database, via `(mt/db)`, or optionally passed in)."
([]
- (db-details (mt/db)))
+ (-> (db-details (mt/db))
+ (assoc :initial_sync_status "complete")))
([{driver :engine, :as db}]
(merge
(mt/object-defaults Database)
(select-keys db [:created_at :id :details :updated_at :timezone :name])
- {:engine (u/qualified-name (:engine db))
- :features (map u/qualified-name (driver.u/features driver db))})))
+ {:engine (u/qualified-name (:engine db))
+ :features (map u/qualified-name (driver.u/features driver db))
+ :initial_sync_status "complete"})))
(defn- table-details [table]
(-> (merge (mt/obj->json->obj (mt/object-defaults Table))
(select-keys table [:active :created_at :db_id :description :display_name :entity_type
- :id :name :rows :schema :updated_at :visibility_type]))
+ :id :name :rows :schema :updated_at :visibility_type :initial_sync_status]))
(update :entity_type #(when % (str "entity/" (name %))))
(update :visibility_type #(when % (name %)))))
@@ -149,7 +151,8 @@
:details (s/eq {:db "my_db"})
:updated_at java.time.temporal.Temporal
:name su/NonBlankString
- :features (s/eq (driver.u/features ::test-driver (mt/db)))})
+ :features (s/eq (driver.u/features ::test-driver (mt/db)))
+ :creator_id (s/eq (mt/user->id :crowberto))})
(create-db-via-api!))))
(testing "can we set `is_full_sync` to `false` when we create the Database?"
@@ -236,41 +239,42 @@
(deftest fetch-database-metadata-test
(testing "GET /api/database/:id/metadata"
(is (= (merge (dissoc (mt/object-defaults Database) :details)
- (select-keys (mt/db) [:created_at :id :updated_at :timezone])
+ (select-keys (mt/db) [:created_at :id :updated_at :timezone :initial_sync_status])
{:engine "h2"
:name "test-data"
:features (map u/qualified-name (driver.u/features :h2 (mt/db)))
:tables [(merge
(mt/obj->json->obj (mt/object-defaults Table))
(db/select-one [Table :created_at :updated_at] :id (mt/id :categories))
- {:schema "PUBLIC"
- :name "CATEGORIES"
- :display_name "Categories"
- :entity_type "entity/GenericTable"
- :fields [(merge
- (field-details (Field (mt/id :categories :id)))
- {:table_id (mt/id :categories)
- :semantic_type "type/PK"
- :name "ID"
- :display_name "ID"
- :database_type "BIGINT"
- :base_type "type/BigInteger"
- :effective_type "type/BigInteger"
- :visibility_type "normal"
- :has_field_values "none"
- :database_position 0})
- (merge
- (field-details (Field (mt/id :categories :name)))
- {:table_id (mt/id :categories)
- :semantic_type "type/Name"
- :name "NAME"
- :display_name "Name"
- :database_type "VARCHAR"
- :base_type "type/Text"
- :effective_type "type/Text"
- :visibility_type "normal"
- :has_field_values "list"
- :database_position 1})]
+ {:schema "PUBLIC"
+ :name "CATEGORIES"
+ :display_name "Categories"
+ :entity_type "entity/GenericTable"
+ :initial_sync_status "complete"
+ :fields [(merge
+ (field-details (Field (mt/id :categories :id)))
+ {:table_id (mt/id :categories)
+ :semantic_type "type/PK"
+ :name "ID"
+ :display_name "ID"
+ :database_type "BIGINT"
+ :base_type "type/BigInteger"
+ :effective_type "type/BigInteger"
+ :visibility_type "normal"
+ :has_field_values "none"
+ :database_position 0})
+ (merge
+ (field-details (Field (mt/id :categories :name)))
+ {:table_id (mt/id :categories)
+ :semantic_type "type/Name"
+ :name "NAME"
+ :display_name "Name"
+ :database_type "VARCHAR"
+ :base_type "type/Text"
+ :effective_type "type/Text"
+ :visibility_type "normal"
+ :has_field_values "list"
+ :database_position 1})]
:segments []
:metrics []
:id (mt/id :categories)
diff --git a/test/metabase/api/field_test.clj b/test/metabase/api/field_test.clj
index 89795bebd1b..5040e35b316 100644
--- a/test/metabase/api/field_test.clj
+++ b/test/metabase/api/field_test.clj
@@ -20,8 +20,8 @@
(defn- db-details []
(merge
- (select-keys (mt/db) [:id :timezone])
- (dissoc (mt/object-defaults Database) :details)
+ (select-keys (mt/db) [:id :timezone :initial_sync_status])
+ (dissoc (mt/object-defaults Database) :details :initial_sync_status)
{:engine "h2"
:name "test-data"
:features (mapv u/qualified-name (driver.u/features :h2 (mt/db)))
@@ -36,7 +36,7 @@
{:table_id (mt/id :users)
:table (merge
(mt/obj->json->obj (mt/object-defaults Table))
- (db/select-one [Table :created_at :updated_at] :id (mt/id :users))
+ (db/select-one [Table :created_at :updated_at :initial_sync_status] :id (mt/id :users))
{:description nil
:entity_type "entity/UserTable"
:visibility_type nil
@@ -527,7 +527,7 @@
:fk_target_field_id false}
(mt/boolean-ids-and-timestamps (simple-field-details (Field field-id-2))))))
(mt/user-http-request :crowberto :put 200 (format "field/%d" field-id-2) {:semantic_type :type/FK
- :fk_target_field_id field-id-1})
+ :fk_target_field_id field-id-1})
(testing "after change"
(is (= {:name "Field Test 2"
:display_name "Field Test 2"
diff --git a/test/metabase/api/search_test.clj b/test/metabase/api/search_test.clj
index 8e817043b47..a2afced08f2 100644
--- a/test/metabase/api/search_test.clj
+++ b/test/metabase/api/search_test.clj
@@ -34,7 +34,8 @@
:table_schema nil
:table_name nil
:table_description nil
- :updated_at true})
+ :updated_at true
+ :initial_sync_status nil})
(defn- table-search-results
"Segments and Metrics come back with information about their Tables as of 0.33.0. The `model-defaults` for Segment and
@@ -470,12 +471,13 @@
(defn- default-table-search-row [table-name]
(merge
default-search-row
- {:name table-name
- :table_name table-name
- :table_id true
- :archived nil
- :model "table"
- :database_id true}))
+ {:name table-name
+ :table_name table-name
+ :table_id true
+ :archived nil
+ :model "table"
+ :database_id true
+ :initial_sync_status "incomplete"}))
(defmacro ^:private do-test-users {:style/indent 1} [[user-binding users] & body]
`(doseq [user# ~users
diff --git a/test/metabase/api/table_test.clj b/test/metabase/api/table_test.clj
index a348540ca24..c43870df487 100644
--- a/test/metabase/api/table_test.clj
+++ b/test/metabase/api/table_test.clj
@@ -32,7 +32,7 @@
(defn- db-details []
(merge
- (select-keys (mt/db) [:id :created_at :updated_at :timezone])
+ (select-keys (mt/db) [:id :created_at :updated_at :timezone :creator_id :initial_sync_status])
{:engine "h2"
:name "test-data"
:is_sample false
@@ -108,7 +108,7 @@
(testing "GET /api/table/:id"
(is (= (merge
(dissoc (table-defaults) :segments :field_values :metrics)
- (db/select-one [Table :created_at :updated_at] :id (mt/id :venues))
+ (db/select-one [Table :created_at :updated_at :initial_sync_status] :id (mt/id :venues))
{:schema "PUBLIC"
:name "VENUES"
:display_name "Venues"
@@ -140,7 +140,7 @@
(testing "Sensitive fields are included"
(is (= (merge
(query-metadata-defaults)
- (db/select-one [Table :created_at :updated_at] :id (mt/id :users))
+ (db/select-one [Table :created_at :updated_at :initial_sync_status] :id (mt/id :users))
{:schema "PUBLIC"
:name "USERS"
:display_name "Users"
@@ -203,7 +203,7 @@
(testing "Sensitive fields should not be included"
(is (= (merge
(query-metadata-defaults)
- (db/select-one [Table :created_at :updated_at] :id (mt/id :users))
+ (db/select-one [Table :created_at :updated_at :initial_sync_status] :id (mt/id :users))
{:schema "PUBLIC"
:name "USERS"
:display_name "Users"
@@ -276,7 +276,7 @@
(-> (table-defaults)
(dissoc :segments :field_values :metrics :updated_at)
(assoc-in [:db :details] (:details (mt/db))))
- (db/select-one [Table :id :schema :name :created_at] :id (u/the-id table))
+ (db/select-one [Table :id :schema :name :created_at :initial_sync_status] :id (u/the-id table))
{:description "What a nice table!"
:entity_type nil
:visibility_type "hidden"
@@ -362,7 +362,7 @@
:position 2
:table (merge
(dissoc (table-defaults) :segments :field_values :metrics)
- (db/select-one [Table :id :created_at :updated_at]
+ (db/select-one [Table :id :created_at :updated_at :initial_sync_status]
:id (mt/id :checkins))
{:schema "PUBLIC"
:name "CHECKINS"
@@ -379,7 +379,7 @@
:semantic_type "type/PK"
:table (merge
(dissoc (table-defaults) :db :segments :field_values :metrics)
- (db/select-one [Table :id :created_at :updated_at]
+ (db/select-one [Table :id :created_at :updated_at :initial_sync_status]
:id (mt/id :users))
{:schema "PUBLIC"
:name "USERS"
@@ -395,7 +395,7 @@
(testing "GET /api/table/:id/query_metadata"
(is (= (merge
(query-metadata-defaults)
- (db/select-one [Table :created_at :updated_at] :id (mt/id :categories))
+ (db/select-one [Table :created_at :updated_at :initial_sync_status] :id (mt/id :categories))
{:schema "PUBLIC"
:name "CATEGORIES"
:display_name "Categories"
diff --git a/test/metabase/sync/util_test.clj b/test/metabase/sync/util_test.clj
index d3ecf4fd8ad..2ec0442b6c6 100644
--- a/test/metabase/sync/util_test.clj
+++ b/test/metabase/sync/util_test.clj
@@ -5,9 +5,11 @@
[java-time :as t]
[metabase.driver :as driver]
[metabase.models.database :as mdb :refer [Database]]
+ [metabase.models.table :refer [Table]]
[metabase.models.task-history :refer [TaskHistory]]
[metabase.sync :as sync]
- [metabase.sync.util :as sync-util :refer :all]
+ [metabase.sync.sync-metadata :as sync-metadata]
+ [metabase.sync.util :as sync-util]
[metabase.test :as mt]
[metabase.test.util :as tu]
[toucan.db :as db]
@@ -107,11 +109,11 @@
(let [process-name (tu/random-name)
step-1-name (tu/random-name)
step-2-name (tu/random-name)
- sync-steps [(create-sync-step step-1-name (fn [_] (Thread/sleep 10) {:foo "bar"}))
- (create-sync-step step-2-name (fn [_] (Thread/sleep 10)))]
+ sync-steps [(sync-util/create-sync-step step-1-name (fn [_] (Thread/sleep 10) {:foo "bar"}))
+ (sync-util/create-sync-step step-2-name (fn [_] (Thread/sleep 10)))]
mock-db (mdb/map->DatabaseInstance {:name "test", :id 1, :engine :h2})
[results] (:operation-results
- (call-with-operation-info #(run-sync-operation process-name mock-db sync-steps)))]
+ (call-with-operation-info #(sync-util/run-sync-operation process-name mock-db sync-steps)))]
(testing "valid operation metadata?"
(is (= true
(validate-times results))))
@@ -245,3 +247,51 @@
(let [[step-name result] (second (:steps actual))]
(is (= "should-continue" step-name))
(is (= {:log-summary-fn nil} (dissoc result :start-time :end-time))))))))
+
+(deftest initial-sync-status-test
+ (mt/dataset sample-dataset
+ (testing "If `initial-sync-status` on a DB is `incomplete`, it is marked as `complete` when sync-metadata has finished"
+ (let [_ (db/update! Database (:id (mt/db)) :initial_sync_status "incomplete")
+ db (Database (:id (mt/db)))]
+ (sync/sync-database! db)
+ (is (= "complete" (db/select-one-field :initial_sync_status Database :id (:id db))))))
+
+ (testing "If `initial-sync-status` on a DB is `complete`, it remains `complete` when sync is run again"
+ (let [_ (db/update! Database (:id (mt/db)) :initial_sync_status "complete")
+ db (Database (:id (mt/db)))]
+ (sync/sync-database! db)
+ (is (= "complete" (db/select-one-field :initial_sync_status Database :id (:id db))))))
+
+ (testing "If `initial-sync-status` on a table is `incomplete`, it is marked as `complete` after the sync-fks step
+ has finished"
+ (let [table-id (db/select-one-field :id Table :db_id (:id (mt/db)))
+ _ (db/update! Table table-id :initial_sync_status "incomplete")
+ table (Table table-id)]
+ (sync/sync-database! (mt/db))
+ (is (= "complete" (db/select-one-field :initial_sync_status Table :id table-id)))))
+
+ (testing "Database and table syncs are marked as complete even if the initial scan is :schema only"
+ (let [_ (db/update! Database (:id (mt/db)) :initial_sync_status "incomplete")
+ db (Database (:id (mt/db)))
+ table-id (db/select-one-field :id Table :db_id (:id (mt/db)))
+ _ (db/update! Table table-id :initial_sync_status "incomplete")
+ table (Table table-id)]
+ (sync/sync-database! db {:scan :schema})
+ (is (= "complete" (db/select-one-field :initial_sync_status Database :id (:id db))))
+ (is (= "complete" (db/select-one-field :initial_sync_status Table :id table-id)))))
+
+ (testing "If a non-recoverable error occurs during sync, `initial-sync-status` on the database is set to `aborted`"
+ (let [_ (db/update! Database (:id (mt/db)) :initial_sync_status "incomplete")
+ db (Database (:id (mt/db)))]
+ (with-redefs [sync-metadata/sync-steps [(sync-util/create-sync-step
+ "fake-step"
+ (fn [_] (throw (java.net.ConnectException.))))]]
+ (sync/sync-database! db)
+ (is (= "aborted" (db/select-one-field :initial_sync_status Database :id (:id db)))))))
+
+ (testing "If `initial-sync-status` is `aborted` for a database, it is set to `complete` the next time sync finishes
+ without error"
+ (let [_ (db/update! Database (:id (mt/db)) :initial_sync_status "complete")
+ db (Database (:id (mt/db)))]
+ (sync/sync-database! db)
+ (is (= "complete" (db/select-one-field :initial_sync_status Database :id (:id db))))))))
diff --git a/test/metabase/sync_test.clj b/test/metabase/sync_test.clj
index dbedef3db56..efdab7f1ce4 100644
--- a/test/metabase/sync_test.clj
+++ b/test/metabase/sync_test.clj
@@ -190,17 +190,19 @@
(testing "`movie` Table"
(is (= (merge
(table-defaults)
- {:schema "default"
- :name "movie"
- :display_name "Movie"
- :fields [(field:movie-id) (field:movie-studio) (field:movie-title)]})
+ {:schema "default"
+ :name "movie"
+ :display_name "Movie"
+ :initial_sync_status "complete"
+ :fields [(field:movie-id) (field:movie-studio) (field:movie-title)]})
movie)))
(testing "`studio` Table"
(is (= (merge
(table-defaults)
- {:name "studio"
- :display_name "Studio"
- :fields [(field:studio-name) (field:studio-studio)]})
+ {:name "studio"
+ :display_name "Studio"
+ :initial_sync_status "complete"
+ :fields [(field:studio-name) (field:studio-studio)]})
studio)))))
(testing "Returns results from sync-database step"
(mt/with-temp Database [db {:engine ::sync-test}]
--
GitLab