From 09e16f6fc8fddd37a7c30d49f429c549dbcdb7d3 Mon Sep 17 00:00:00 2001
From: Cal Herries <39073188+calherries@users.noreply.github.com>
Date: Wed, 17 Apr 2024 18:36:05 +0300
Subject: [PATCH] Use `fk-metadata` instead of `table-fk-metadata` everywhere
(#41375)
---
src/metabase/driver.clj | 3 +-
src/metabase/sync/fetch_metadata.clj | 51 +++++++++++--------
src/metabase/sync/sync_metadata/fks.clj | 24 +++------
.../sync_metadata/sync_table_privileges.clj | 5 +-
src/metabase/sync/util.clj | 29 ++++++-----
5 files changed, 57 insertions(+), 55 deletions(-)
diff --git a/src/metabase/driver.clj b/src/metabase/driver.clj
index aac5b659b2b..1938ec16a7a 100644
--- a/src/metabase/driver.clj
+++ b/src/metabase/driver.clj
@@ -366,7 +366,8 @@
(defmulti describe-fks
"Returns a reducible collection of maps, each containing information about foreign keys.
- Takes keyword arguments to narrow down the results to a set of `schema-names` or `table-names`.
+ Takes optional keyword arguments to narrow down the results to a set of `schema-names`
+ and `table-names`.
Results match [[metabase.sync.interface/FKMetadataEntry]].
Results are optionally filtered by `schema-names` and `table-names` provided.
diff --git a/src/metabase/sync/fetch_metadata.clj b/src/metabase/sync/fetch_metadata.clj
index efd43e7bbe8..ed056750b08 100644
--- a/src/metabase/sync/fetch_metadata.clj
+++ b/src/metabase/sync/fetch_metadata.clj
@@ -7,6 +7,7 @@
[metabase.driver.sql-jdbc.sync :as sql-jdbc.sync]
[metabase.driver.util :as driver.u]
[metabase.sync.interface :as i]
+ [metabase.sync.util :as sync-util]
[metabase.util.log :as log]
[metabase.util.malli :as mu]
[metabase.util.malli.fn :as mu.fn]))
@@ -45,32 +46,40 @@
(set (fields-metadata database :table-names [(:name table)] :schema-names [(:schema table)]))
(:fields (driver/describe-table (driver.u/database->driver database) database table)))))
-(mu/defn fk-metadata
- "Effectively a wrapper for [[metabase.driver/describe-fks]] that also validates the output against the schema."
- [database :- i/DatabaseInstance & {:as args}]
- (log-if-error "fk-metadata"
- (cond->> (driver/describe-fks (driver.u/database->driver database) database args)
- ;; This is a workaround for the fact that [[mu/defn]] can't check reducible collections yet
- (mu.fn/instrument-ns? *ns*)
- (eduction (map #(mu.fn/validate-output {} i/FKMetadataEntry %))))))
-
-(mu/defn table-fk-metadata :- [:maybe [:sequential i/FKMetadataEntry]]
- "Get information about the foreign keys belonging to `table`."
- [database :- i/DatabaseInstance
- table :- i/TableInstance]
- (log-if-error "table-fk-metadata"
- (let [driver (driver.u/database->driver database)]
- (when (driver/database-supports? driver :foreign-keys database)
- (if (driver/database-supports? driver :describe-fks database)
- (vec (driver/describe-fks driver database :table-names [(:name table)] :schema-names [(:schema table)]))
- #_{:clj-kondo/ignore [:deprecated-var]}
- (vec (for [x (driver/describe-table-fks driver database table)]
+(defn backwards-compatible-describe-fks
+ "Replaces [[metabase.driver/describe-fks]] for drivers that haven't implemented it. Uses [[driver/describe-table-fks]]
+ which is deprecated."
+ [driver database & {:keys [schema-names table-names]}]
+ (let [tables (sync-util/db->reducible-sync-tables database :schema-names schema-names :table-names table-names)]
+ (eduction
+ (mapcat (fn [table]
+ #_{:clj-kondo/ignore [:deprecated-var]}
+ (for [x (driver/describe-table-fks driver database table)]
{:fk-table-name (:name table)
:fk-table-schema (:schema table)
:fk-column-name (:fk-column-name x)
:pk-table-name (:name (:dest-table x))
:pk-table-schema (:schema (:dest-table x))
- :pk-column-name (:dest-column-name x)})))))))
+ :pk-column-name (:dest-column-name x)})))
+ tables)))
+
+(mu/defn fk-metadata
+ "Effectively a wrapper for [[metabase.driver/describe-fks]] that also validates the output against the schema.
+ If the driver doesn't support [[metabase.driver/describe-fks]] it uses [[driver/describe-table-fks]] instead.
+ This will be deprecated in "
+ [database :- i/DatabaseInstance & {:as args}]
+ (log-if-error "fk-metadata"
+ (let [driver (driver.u/database->driver database)]
+ (when (driver/database-supports? driver :foreign-keys database)
+ (let [describe-fks-fn (if (driver/database-supports? driver :describe-fks database)
+ driver/describe-fks
+ ;; In version 52 we'll remove [[driver/describe-table-fks]]
+ ;; and we'll just use [[driver/describe-fks]] here
+ backwards-compatible-describe-fks)]
+ (cond->> (describe-fks-fn (driver.u/database->driver database) database args)
+ ;; This is a workaround for the fact that [[mu/defn]] can't check reducible collections yet
+ (mu.fn/instrument-ns? *ns*)
+ (eduction (map #(mu.fn/validate-output {} i/FKMetadataEntry %)))))))))
(mu/defn nfc-metadata :- [:maybe [:set i/TableMetadataField]]
"Get information about the nested field column fields within `table`."
diff --git a/src/metabase/sync/sync_metadata/fks.clj b/src/metabase/sync/sync_metadata/fks.clj
index bf5279eb235..df9546a3e69 100644
--- a/src/metabase/sync/sync_metadata/fks.clj
+++ b/src/metabase/sync/sync_metadata/fks.clj
@@ -12,8 +12,7 @@
[metabase.util :as u]
[metabase.util.log :as log]
[metabase.util.malli :as mu]
- [toucan2.core :as t2]
- [toucan2.realize :as t2.realize]))
+ [toucan2.core :as t2]))
(defn ^:private mark-fk-sql
"Returns [sql & params] for [[mark-fk!]] according to the application DB's dialect."
@@ -97,7 +96,9 @@
"Sync the foreign keys for a `database`."
[database :- i/DatabaseInstance]
(sync-util/with-error-handling (format "Error syncing FKs for %s" (sync-util/name-for-logging database))
- (let [schema-names (sync-util/db->sync-schemas database)
+ (let [driver (driver.u/database->driver database)
+ schema-names (when (driver/database-supports? driver :schemas database)
+ (sync-util/db->sync-schemas database))
fk-metadata (fetch-metadata/fk-metadata database :schema-names schema-names)]
(transduce (map (fn [x]
(let [[updated failed] (try [(mark-fk! database x) 0]
@@ -121,7 +122,9 @@
([database :- i/DatabaseInstance
table :- i/TableInstance]
(sync-util/with-error-handling (format "Error syncing FKs for %s" (sync-util/name-for-logging table))
- (let [fk-metadata (fetch-metadata/table-fk-metadata database table)]
+ (let [schema-names (when (driver/database-supports? (driver.u/database->driver database) :schemas database)
+ [(:schema table)])
+ fk-metadata (into [] (fetch-metadata/fk-metadata database :schema-names schema-names :table-names [(:name table)]))]
{:total-fks (count fk-metadata)
:updated-fks (sync-util/sum-numbers #(mark-fk! database %) fk-metadata)}))))
@@ -133,18 +136,7 @@
This function also sets all the tables that should be synced to have `initial-sync-status=complete` once the sync is done."
[database :- i/DatabaseInstance]
- (u/prog1 (if (driver/database-supports? (driver.u/database->driver database) :describe-fks database)
- (sync-fks-for-db! database)
- (reduce (fn [update-info table]
- (let [table (t2.realize/realize table)
- table-fk-info (sync-fks-for-table! database table)]
- (if (instance? Exception table-fk-info)
- (update update-info :total-failed inc)
- (merge-with + update-info table-fk-info))))
- {:total-fks 0
- :updated-fks 0
- :total-failed 0}
- (sync-util/db->reducible-sync-tables database)))
+ (u/prog1 (sync-fks-for-db! database)
;; 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
;; `:metabase.sync.util/exception-classes-not-to-retry`).
diff --git a/src/metabase/sync/sync_metadata/sync_table_privileges.clj b/src/metabase/sync/sync_metadata/sync_table_privileges.clj
index 8df930fda35..f3822a314d4 100644
--- a/src/metabase/sync/sync_metadata/sync_table_privileges.clj
+++ b/src/metabase/sync/sync_metadata/sync_table_privileges.clj
@@ -14,10 +14,7 @@
This is a cache of the data returned from `driver/table-privileges`, but it's stored in the database for performance."
[database :- (ms/InstanceOf :model/Database)]
(let [driver (driver.u/database->driver database)]
- (when (and (not= :redshift driver)
- ;; redshift does support table-privileges, but we don't want to sync it now because table privileges are
- ;; meant to enhance action features, but redshift does not support actions for now, so we skip it here.
- (driver/database-supports? driver :table-privileges database))
+ (when (driver/database-supports? driver :table-privileges database)
(let [rows (driver/current-user-table-privileges driver database)
schema+table->id (t2/select-fn->pk (fn [t] {:schema (:schema t), :table (:name t)}) :model/Table :db_id (:id database))
rows-with-table-id (keep (fn [row]
diff --git a/src/metabase/sync/util.clj b/src/metabase/sync/util.clj
index 57a45fcc418..f830f86709e 100644
--- a/src/metabase/sync/util.clj
+++ b/src/metabase/sync/util.clj
@@ -61,7 +61,6 @@
;; new function that will execute the original in whatever context or with whatever side effects appropriate for that
;; step.
-
;; This looks something like {:sync #{1 2}, :cache #{2 3}} when populated.
;; Key is a type of sync operation, e.g. `:sync` or `:cache`; vals are sets of DB IDs undergoing that operation.
;;
@@ -100,10 +99,10 @@
(keyword (or (namespace event-name-prefix) "event")
(str (name prefix) suffix)))]
(with-sync-events
- (event-keyword event-name-prefix "-begin")
- (event-keyword event-name-prefix "-end")
- database-or-id
- f)))
+ (event-keyword event-name-prefix "-begin")
+ (event-keyword event-name-prefix "-end")
+ database-or-id
+ f)))
([begin-event-name :- Topic
end-event-name :- Topic
@@ -129,8 +128,8 @@
_ (log-fn (u/format-color 'magenta "STARTING: %s" message))
result (f)]
(log-fn (u/format-color 'magenta "FINISHED: %s (%s)"
- message
- (u/format-nanoseconds (- (System/nanoTime) start-time))))
+ message
+ (u/format-nanoseconds (- (System/nanoTime) start-time))))
result))
(defn- with-start-and-finish-logging
@@ -226,7 +225,6 @@
[operation database message & body]
`(do-sync-operation ~operation ~database ~message (fn [] ~@body)))
-
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | EMOJI PROGRESS METER |
;;; +----------------------------------------------------------------------------------------------------------------+
@@ -341,11 +339,16 @@
(defn db->reducible-sync-tables
"Returns a reducible of all the Tables that should go through the sync processes for `database-or-id`."
- [database-or-id]
- (t2/reducible-select :model/Table, :db_id (u/the-id database-or-id), {:where sync-tables-clause}))
+ [database-or-id & {:keys [schema-names table-names]}]
+ (t2/reducible-select :model/Table
+ :db_id (u/the-id database-or-id)
+ {:where [:and sync-tables-clause
+ (when (seq schema-names) [:in :schema schema-names])
+ (when (seq table-names) [:in :name table-names])]}))
(defn db->sync-schemas
- "Returns all the Schemas that have their metadata sync'd for `database-or-id`."
+ "Returns all the Schemas that have their metadata sync'd for `database-or-id`.
+ Assumes the database supports schemas."
[database-or-id]
(vec (map :schema (t2/query {:select-distinct [:schema]
:from [:metabase_table]
@@ -362,7 +365,7 @@
mi/model)
(defmethod name-for-logging :model/Database
- [{database-name :name, id :id, engine :engine,}]
+ [{database-name :name, id :id, engine :engine}]
(format "%s Database %s ''%s''" (name engine) (str (or id "")) database-name))
(defn table-name-for-logging
@@ -495,7 +498,7 @@
"# %s\n"
"# %s\n"
(when log-summary-fn
- (format "# %s\n" (log-summary-fn step-info))))
+ (format "# %s\n" (log-summary-fn step-info))))
[(format "Completed step ''%s''" step-name)
(format "Start: %s" (u.date/format start-time))
(format "End: %s" (u.date/format end-time))
--
GitLab