From ffbb526e5deabbb668cc62cf363af2f4ecaac720 Mon Sep 17 00:00:00 2001
From: Cal Herries <39073188+calherries@users.noreply.github.com>
Date: Wed, 21 Jun 2023 16:03:03 +0300
Subject: [PATCH] BE: Set initial_sync_status="incomplete" during sync for any
previously inactive tables (#31607)
---
.../components/TableBrowser/TableBrowser.jsx | 12 ++---
.../TableBrowser/TableBrowser.unit.spec.js | 47 ++++++++++---------
.../containers/TableBrowser/TableBrowser.jsx | 8 +---
src/metabase/models/table.clj | 5 +-
src/metabase/sync/sync_metadata/tables.clj | 30 +++++++-----
test/metabase/api/search_test.clj | 2 +-
test/metabase/sync/util_test.clj | 33 +++++++++++++
7 files changed, 83 insertions(+), 54 deletions(-)
diff --git a/frontend/src/metabase/browse/components/TableBrowser/TableBrowser.jsx b/frontend/src/metabase/browse/components/TableBrowser/TableBrowser.jsx
index c27e6a4f268..3462916f83f 100644
--- a/frontend/src/metabase/browse/components/TableBrowser/TableBrowser.jsx
+++ b/frontend/src/metabase/browse/components/TableBrowser/TableBrowser.jsx
@@ -55,12 +55,10 @@ const TableBrowser = ({
<Grid>
{tables.map(table => (
<TableGridItem key={table.id}>
- <TableCard hoverable={!isTableLoading(table, database)}>
+ <TableCard hoverable={!isSyncInProgress(table)}>
<TableLink
to={
- !isTableLoading(table, database)
- ? getTableUrl(table, metadata)
- : ""
+ !isSyncInProgress(table) ? getTableUrl(table, metadata) : ""
}
data-metabase-event={`${ANALYTICS_CONTEXT};Table Click`}
>
@@ -90,7 +88,7 @@ const itemPropTypes = {
const TableBrowserItem = ({ database, table, dbId, xraysEnabled }) => {
const isVirtual = isVirtualCardId(table.id);
- const isLoading = isTableLoading(table, database);
+ const isLoading = isSyncInProgress(table);
return (
<EntityItem
@@ -143,10 +141,6 @@ const TableBrowserItemButtons = ({ tableId, dbId, xraysEnabled }) => {
TableBrowserItemButtons.propTypes = itemButtonsPropTypes;
-const isTableLoading = (table, database) => {
- return database && isSyncInProgress(database) && isSyncInProgress(table);
-};
-
const getDatabaseCrumbs = dbId => {
if (dbId === SAVED_QUESTIONS_VIRTUAL_DB_ID) {
return {
diff --git a/frontend/src/metabase/browse/components/TableBrowser/TableBrowser.unit.spec.js b/frontend/src/metabase/browse/components/TableBrowser/TableBrowser.unit.spec.js
index 0e353b3719b..3c1ab6e347c 100644
--- a/frontend/src/metabase/browse/components/TableBrowser/TableBrowser.unit.spec.js
+++ b/frontend/src/metabase/browse/components/TableBrowser/TableBrowser.unit.spec.js
@@ -24,28 +24,31 @@ describe("TableBrowser", () => {
expect(screen.queryByTestId("loading-spinner")).not.toBeInTheDocument();
});
- it("should render syncing tables", () => {
- const database = getDatabase({
- initial_sync_status: "incomplete",
- });
-
- const tables = [
- getTable({ id: 1, name: "Orders", initial_sync_status: "incomplete" }),
- ];
-
- render(
- <TableBrowser
- database={database}
- tables={tables}
- getTableUrl={getTableUrl}
- xraysEnabled={true}
- />,
- );
-
- expect(screen.getByText("Orders")).toBeInTheDocument();
- expect(screen.queryByLabelText("bolt_filled icon")).not.toBeInTheDocument();
- expect(screen.getByTestId("loading-spinner")).toBeInTheDocument();
- });
+ it.each(["incomplete", "complete"])(
+ "should render syncing tables, regardless of the database's initial_sync_status",
+ initial_sync_status => {
+ const database = getDatabase({ initial_sync_status });
+
+ const tables = [
+ getTable({ id: 1, name: "Orders", initial_sync_status: "incomplete" }),
+ ];
+
+ render(
+ <TableBrowser
+ database={database}
+ tables={tables}
+ getTableUrl={getTableUrl}
+ xraysEnabled={true}
+ />,
+ );
+
+ expect(screen.getByText("Orders")).toBeInTheDocument();
+ expect(
+ screen.queryByLabelText("bolt_filled icon"),
+ ).not.toBeInTheDocument();
+ expect(screen.getByTestId("loading-spinner")).toBeInTheDocument();
+ },
+ );
it("should render tables with a sync error", () => {
const database = getDatabase({
diff --git a/frontend/src/metabase/browse/containers/TableBrowser/TableBrowser.jsx b/frontend/src/metabase/browse/containers/TableBrowser/TableBrowser.jsx
index 814dd1f17be..7593dadcc35 100644
--- a/frontend/src/metabase/browse/containers/TableBrowser/TableBrowser.jsx
+++ b/frontend/src/metabase/browse/containers/TableBrowser/TableBrowser.jsx
@@ -31,12 +31,8 @@ const getSchemaName = props => {
return props.schemaName || props.params.schemaName;
};
-const getReloadInterval = (state, { database }, tables = []) => {
- if (
- database &&
- isSyncInProgress(database) &&
- tables.some(t => isSyncInProgress(t))
- ) {
+const getReloadInterval = (state, _props, tables = []) => {
+ if (tables.some(t => isSyncInProgress(t))) {
return RELOAD_INTERVAL;
} else {
return 0;
diff --git a/src/metabase/models/table.clj b/src/metabase/models/table.clj
index 07923249fcb..af3df00cbf5 100644
--- a/src/metabase/models/table.clj
+++ b/src/metabase/models/table.clj
@@ -57,9 +57,8 @@
(t2/define-before-insert :model/Table
[table]
- (let [defaults {:display_name (humanization/name->human-readable-name (:name table))
- :field_order (driver/default-field-order (t2/select-one-fn :engine Database :id (:db_id table)))
- :initial_sync_status "incomplete"}]
+ (let [defaults {:display_name (humanization/name->human-readable-name (:name table))
+ :field_order (driver/default-field-order (t2/select-one-fn :engine Database :id (:db_id table)))}]
(merge defaults table)))
(t2/define-before-delete :model/Table
diff --git a/src/metabase/sync/sync_metadata/tables.clj b/src/metabase/sync/sync_metadata/tables.clj
index 3a9e5efe7d3..d447e3d202c 100644
--- a/src/metabase/sync/sync_metadata/tables.clj
+++ b/src/metabase/sync/sync_metadata/tables.clj
@@ -96,25 +96,29 @@
(defn create-or-reactivate-table!
"Create a single new table in the database, or mark it as active if it already exists."
[database {schema :schema, table-name :name, :as table}]
- (if-let [existing-id (t2/select-one-pk Table
- :db_id (u/the-id database)
- :schema schema
- :name table-name
- :active false)]
- ;; if the table already exists but is marked *inactive*, mark it as *active*
- (t2/update! Table existing-id
- {:active true})
- ;; otherwise create a new Table
- (let [is-crufty? (is-crufty-table? table)]
+ (let [;; if this is a crufty table, mark initial sync as complete since we'll skip the subsequent sync steps
+ is-crufty? (is-crufty-table? table)
+ initial-sync-status (if is-crufty? "complete" "incomplete")
+ visibility-type (when is-crufty? :cruft)]
+ (if-let [existing-id (t2/select-one-pk Table
+ :db_id (u/the-id database)
+ :schema schema
+ :name table-name
+ :active false)]
+ ;; if the table already exists but is marked *inactive*, mark it as *active*
+ (t2/update! Table existing-id
+ {:active true
+ :visibility_type visibility-type
+ :initial_sync_status initial-sync-status})
+ ;; otherwise create a new Table
(first (t2/insert-returning-instances! Table
:db_id (u/the-id database)
:schema schema
:name table-name
:display_name (humanization/name->human-readable-name table-name)
:active true
- :visibility_type (when is-crufty? :cruft)
- ;; if this is a crufty table, mark initial sync as complete since we'll skip the subsequent sync steps
- :initial_sync_status (if is-crufty? "complete" "incomplete"))))))
+ :visibility_type visibility-type
+ :initial_sync_status initial-sync-status)))))
;; TODO - should we make this logic case-insensitive like it is for fields?
diff --git a/test/metabase/api/search_test.clj b/test/metabase/api/search_test.clj
index f67369123c2..969c4e63ad7 100644
--- a/test/metabase/api/search_test.clj
+++ b/test/metabase/api/search_test.clj
@@ -592,7 +592,7 @@
:model "table"
:database_id true
:pk_ref nil
- :initial_sync_status "incomplete"}))
+ :initial_sync_status "complete"}))
(defmacro ^:private do-test-users {:style/indent 1} [[user-binding users] & body]
`(doseq [user# ~users
diff --git a/test/metabase/sync/util_test.clj b/test/metabase/sync/util_test.clj
index 69690ccc90d..ab202178f53 100644
--- a/test/metabase/sync/util_test.clj
+++ b/test/metabase/sync/util_test.clj
@@ -1,6 +1,7 @@
(ns ^:mb/once metabase.sync.util-test
"Tests for the utility functions shared by all parts of sync, such as the duplicate ops guard."
(:require
+ [clojure.core.async :as a]
[clojure.string :as str]
[clojure.test :refer :all]
[java-time :as t]
@@ -11,6 +12,7 @@
[metabase.models.task-history :refer [TaskHistory]]
[metabase.sync :as sync]
[metabase.sync.sync-metadata :as sync-metadata]
+ [metabase.sync.sync-metadata.fields :as sync-fields]
[metabase.sync.util :as sync-util]
[metabase.test :as mt]
[metabase.test.util :as tu]
@@ -299,3 +301,34 @@
db (t2/select-one Database :id (:id (mt/db)))]
(sync/sync-database! db)
(is (= "complete" (t2/select-one-fn :initial_sync_status Database :id (:id db))))))))
+
+(deftest initial-sync-status-table-only-test
+ ;; Test that if a database is already completed sync'ing, then the sync is started again, it should initially be marked as
+ ;; incomplete, but then marked as complete after the sync is finished.
+ (mt/dataset sample-dataset
+ (testing "If `initial-sync-status` on a DB is already `complete`"
+ (let [[active-table inactive-table] (t2/select Table :db_id (mt/id))
+ get-active-table #(t2/select-one Table :id (:id active-table))
+ get-inactive-table #(t2/select-one Table :id (:id inactive-table))]
+ (t2/update! Table (:id active-table) {:initial_sync_status "complete" :active true})
+ (t2/update! Table (:id inactive-table) {:initial_sync_status "complete" :active false})
+ (let [syncing-chan (a/chan)
+ completed-chan (a/chan)]
+ (let [sync-fields! sync-fields/sync-fields!]
+ (with-redefs [sync-fields/sync-fields! (fn [database]
+ (a/>!! syncing-chan ::syncing)
+ (sync-fields! database))]
+ (future
+ (sync/sync-database! (mt/db))
+ (a/>!! completed-chan ::sync-completed))
+ (a/<!! syncing-chan)
+ (testing "for existing tables initial_sync_status is complete while sync is running"
+ (is (= "complete" (:initial_sync_status (get-active-table)))))
+ (testing "for new or previously inactive tables, initial_sync_status is incomplete while sync is running"
+ (is (= "incomplete" (:initial_sync_status (get-inactive-table)))))
+ (a/<!! completed-chan)
+ (testing "initial_sync_status is complete after the sync is finished"
+ (is (= "complete" (:initial_sync_status (get-active-table))))
+ (is (= "complete" (:initial_sync_status (get-inactive-table)))))))
+ (a/close! syncing-chan)
+ (a/close! completed-chan))))))
--
GitLab