Use `fk-metadata` instead of `table-fk-metadata` everywhere (#41375)

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.

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`."

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`).

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]

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))