[serdes] use spec instead of a pipeline for Card (#45209)

enterprise/backend/test/metabase_enterprise/serialization/v2/models_test.clj

@@ -4,9 +4,13 @@
    [metabase-enterprise.serialization.v2.backfill-ids :as serdes.backfill]
    [metabase-enterprise.serialization.v2.entity-ids :as v2.entity-ids]
    [metabase-enterprise.serialization.v2.models :as serdes.models]
-   [metabase.models.serialization :as serdes]))
+   [metabase.db :as mdb]
+   [metabase.models.serialization :as serdes]
+   [metabase.util :as u]
+   [metabase.util.connection :as u.conn]
+   [toucan2.core :as t2]))
 
-(deftest every-model-is-supported-test
+(deftest ^:parallel every-model-is-supported-test
   (testing "Serialization support\n"
     (testing "We know about every model"
       (is (= (set (concat serdes.models/exported-models
@@ -35,3 +39,37 @@
               ;; TODO: strip serialization stuff off Pulse*
               (when-not (#{"Pulse" "PulseChannel" "PulseCard" "User" "PermissionsGroup"} (name model))
                 (is (not random-entity-id?))))))))))
+
+(deftest ^:parallel serialization-complete-spec-test
+  ;; When serialization spec is defined, it describes every column
+  (doseq [m     serdes.models/exported-models
+          :let  [spec (serdes/make-spec m)]
+          :when spec]
+    (let [t      (t2/table-name (keyword "model" m))
+          fields (u.conn/app-db-column-types (mdb/app-db) t)
+          spec'  (merge (zipmap (:copy spec) (repeat :copy))
+                        (zipmap (:skip spec) (repeat :skip))
+                        (:transform spec))]
+      (testing (format "%s should declare every column in serialization spec" m)
+        (is (= (->> (keys fields)
+                    (map u/lower-case-en)
+                    set)
+               (->> (keys spec')
+                    (map name)
+                    set))))
+      (testing "Foreign keys should be declared as such\n"
+        (doseq [[fk _] (filter #(:fk (second %)) fields)
+                :let   [fk (u/lower-case-en fk)
+                        action (get spec' (keyword fk))]]
+          (testing (format "%s.%s is foreign key which is handled correctly" m fk)
+            ;; FIXME: serialization can guess where FK points by itself, but `collection_id` and `database_id` are
+            ;; specifying that themselves right now
+            (when-not (#{"collection_id" "database_id"} fk)
+              (is (#{:skip
+                     serdes/*export-fk*
+                     serdes/*export-fk-keyed*
+                     serdes/*export-table-fk*
+                     serdes/*export-user*}
+                   (if (vector? action)
+                     (first action) ;; tuple of [ser des]
+                     action))))))))))

src/metabase/models/card.clj

@@ -876,55 +876,55 @@
                                     [(when (:table_id m) #{(serdes/table->path (:table_id m))})
                                      (when (:id m)       #{(serdes/field->path (:id m))})])))))
 
-(defmethod serdes/extract-one "Card"
-  [_model-name _opts card]
-  ;; Cards have :table_id, :database_id, :collection_id, :creator_id that need conversion.
-  ;; :table_id and :database_id are extracted as just :table_id [database_name schema table_name].
-  ;; :collection_id is extracted as its entity_id or identity-hash.
-  ;; :creator_id as the user's email.
-  (-> (serdes/extract-one-basics "Card" card)
-      (update :database_id            serdes/*export-fk-keyed* 'Database :name)
-      (update :table_id               serdes/*export-table-fk*)
-      (update :collection_id          serdes/*export-fk* 'Collection)
-      (update :creator_id             serdes/*export-user*)
-      (update :made_public_by_id      serdes/*export-user*)
-      (update :dataset_query          serdes/export-mbql)
-      (update :parameters             serdes/export-parameters)
-      (update :parameter_mappings     serdes/export-parameter-mappings)
-      (update :visualization_settings serdes/export-visualization-settings)
-      (update :result_metadata        export-result-metadata)
-      (dissoc :cache_invalidated_at :view_count :last_used_at :initially_published_at
-              :dataset_query_metrics_v2_migration_backup)))
-
-(defmethod serdes/load-xform "Card"
-  [card]
-  (-> card
-      serdes/load-xform-basics
-      (update :database_id            serdes/*import-fk-keyed* 'Database :name)
-      (update :table_id               serdes/*import-table-fk*)
-      (update :creator_id             serdes/*import-user*)
-      (update :made_public_by_id      serdes/*import-user*)
-      (update :collection_id          serdes/*import-fk* 'Collection)
-      (update :dataset_query          serdes/import-mbql)
-      (update :parameters             serdes/import-parameters)
-      (update :parameter_mappings     serdes/import-parameter-mappings)
-      (update :visualization_settings serdes/import-visualization-settings)
-      (update :result_metadata        import-result-metadata)))
+(defmethod serdes/make-spec "Card"
+  [_model-name]
+  {:copy [:archived :archived_directly :collection_position :collection_preview :created_at :description :display
+          :embedding_params :enable_embedding :entity_id :metabase_version :public_uuid :query_type :type :name]
+   :skip [ ;; always instance-specific
+          :id :updated_at
+          ;; cache invalidation is instance-specific
+          :cache_invalidated_at
+          ;; those are instance-specific analytic columns
+          :view_count :last_used_at :initially_published_at
+          ;; this is data migration column
+          :dataset_query_metrics_v2_migration_backup
+          ;; this column is not used anymore
+          :cache_ttl]
+   :transform
+   {:database_id            [#(serdes/*export-fk-keyed* % 'Database :name)
+                             #(serdes/*import-fk-keyed* % 'Database :name)]
+    :table_id               [serdes/*export-table-fk*
+                             serdes/*import-table-fk*]
+    :collection_id          [#(serdes/*export-fk* % 'Collection)
+                             #(serdes/*import-fk* % 'Collection)]
+    :creator_id             [serdes/*export-user*
+                             serdes/*import-user*]
+    :made_public_by_id      [serdes/*export-user*
+                             serdes/*import-user*]
+    :dataset_query          [serdes/export-mbql
+                             serdes/import-mbql]
+    :parameters             [serdes/export-parameters
+                             serdes/import-parameters]
+    :parameter_mappings     [serdes/export-parameter-mappings
+                             serdes/import-parameter-mappings]
+    :visualization_settings [serdes/export-visualization-settings
+                             serdes/import-visualization-settings]
+    :result_metadata        [export-result-metadata
+                             import-result-metadata]}})
 
 (defmethod serdes/dependencies "Card"
   [{:keys [collection_id database_id dataset_query parameters parameter_mappings
            result_metadata table_id visualization_settings]}]
-  (->> (map serdes/mbql-deps parameter_mappings)
-       (reduce set/union #{})
-       (set/union (serdes/parameters-deps parameters))
-       (set/union #{[{:model "Database" :id database_id}]})
-       ; table_id and collection_id are nullable.
-       (set/union (when table_id #{(serdes/table->path table_id)}))
-       (set/union (when collection_id #{[{:model "Collection" :id collection_id}]}))
-       (set/union (result-metadata-deps result_metadata))
-       (set/union (serdes/mbql-deps dataset_query))
-       (set/union (serdes/visualization-settings-deps visualization_settings))
-       vec))
+  (set
+   (concat
+    (mapcat serdes/mbql-deps parameter_mappings)
+    (serdes/parameters-deps parameters)
+    [[{:model "Database" :id database_id}]]
+    (when table_id #{(serdes/table->path table_id)})
+    (when collection_id #{[{:model "Collection" :id collection_id}]})
+    (result-metadata-deps result_metadata)
+    (serdes/mbql-deps dataset_query)
+    (serdes/visualization-settings-deps visualization_settings))))
 
 (defmethod serdes/descendants "Card" [_model-name id]
   (let [card               (t2/select-one Card :id id)
@@ -932,19 +932,17 @@
         template-tags      (some->> card :dataset_query :native :template-tags vals (keep :card-id))
         parameters-card-id (some->> card :parameters (keep (comp :card_id :values_source_config)))
         snippets           (some->> card :dataset_query :native :template-tags vals (keep :snippet-id))]
-    (set/union
+    (set
+     (concat
       (when (and (string? source-table)
                  (str/starts-with? source-table "card__"))
-        #{["Card" (Integer/parseInt (.substring ^String source-table 6))]})
-      (when (seq template-tags)
-        (set (for [card-id template-tags]
-               ["Card" card-id])))
-      (when (seq parameters-card-id)
-        (set (for [card-id parameters-card-id]
-               ["Card" card-id])))
-      (when (seq snippets)
-        (set (for [snippet-id snippets]
-               ["NativeQuerySnippet" snippet-id]))))))
+        [["Card" (parse-long (subs source-table 6))]])
+      (for [card-id template-tags]
+        ["Card" card-id])
+      (for [card-id parameters-card-id]
+        ["Card" card-id])
+      (for [snippet-id snippets]
+        ["NativeQuerySnippet" snippet-id])))))
 
 
 ;;; ------------------------------------------------ Audit Log --------------------------------------------------------

src/metabase/models/serialization.clj

@@ -288,6 +288,26 @@
 ;;;
 ;;; *Note:* "descendants" and "dependencies" are quite different things!
 
+(defmulti make-spec
+  "Return specification for serialization. This should be a map of three keys: `:copy`, `:skip`, `:transform`.
+
+  `:copy` and `:skip` are vectors of field names. `:skip` is only used in tests to check that all fields were
+  mentioned.
+
+  `:transform` is a map from field name to a `{:ser (fn [v] ...) :des (fn [v] ...)}` map with functions to
+  serialize/deserialize data.
+
+  For behavior, see `extract-by-spec` and `load-by-spec`."
+  (fn [model-name] model-name))
+
+(defmethod make-spec :default [_] nil)
+
+(defn- extract-by-spec [model-name _opts instance]
+  (when-let [spec (make-spec model-name)]
+    (into (select-keys instance (:copy spec))
+          (for [[k [ser _des]] (:transform spec)]
+            [k (ser (get instance k))]))))
+
 (defmulti extract-all
   "Entry point for extracting all entities of a particular model:
   `(extract-all \"ModelName\" {opts...})`
@@ -387,8 +407,11 @@
         (assoc :serdes/meta (generate-path model-name entity))
         (dissoc pk :updated_at))))
 
-(defmethod extract-one :default [model-name _opts entity]
-  (extract-one-basics model-name entity))
+(defmethod extract-one :default [model-name opts entity]
+  ;; `extract-by-spec` is called here since most of tests use `extract-one` right now
+  (or (some-> (extract-by-spec model-name opts entity)
+              (assoc :serdes/meta (generate-path model-name entity)))
+      (extract-one-basics model-name entity)))
 
 (defmulti descendants
   "Returns set of `[model-name database-id]` pairs for all entities contained or used by this entity. e.g. the Dashboard
@@ -540,7 +563,7 @@
 (defn- ->table-name
   "Returns the table name that a particular ingested entity should finally be inserted into."
   [ingested]
-  (->> ingested ingested-model (keyword "model") t2/table-name name))
+  (->> ingested ingested-model (keyword "model") t2/table-name))
 
 (defmulti ingested-model-columns
   "Called by `drop-excess-keys` (which is in turn called by `load-xform-basics`) to determine the full set of keys that
@@ -639,11 +662,20 @@
   (fn [ingested _]
     (ingested-model ingested)))
 
+(defn- load-by-spec [ingested]
+  (let [model-name (ingested-model ingested)
+        spec       (make-spec model-name)]
+    (when spec
+      (into (select-keys ingested (:copy spec))
+            (for [[k [_ser des]] (:transform spec)]
+              [k (des (get ingested k))])))))
+
 (defn default-load-one!
   "Default implementation of `load-one!`"
   [ingested maybe-local]
   (let [model    (ingested-model ingested)
-        adjusted (load-xform ingested)]
+        adjusted (or (load-by-spec ingested)
+                     (load-xform ingested))]
     (binding [mi/*deserializing?* true]
       (if (nil? maybe-local)
         (load-insert! model adjusted)
@@ -752,7 +784,8 @@
     (let [model-name (name model)
           model      (t2.model/resolve-model (symbol model-name))
           entity     (t2/select-one model (first (t2/primary-keys model)) id)
-          path       (when entity (mapv :id (generate-path model-name entity)))]
+          path       (when entity
+                       (mapv :id (generate-path model-name entity)))]
       (cond
         (nil? entity)      (throw (ex-info "FK target not found" {:model model
                                                                   :id    id

src/metabase/util/connection.clj

@@ -2,20 +2,38 @@
   (:require [metabase.util :as u]
             [toucan2.core :as t2])
   (:import
-   (java.sql Connection)))
+   (java.sql Connection ResultSet ResultSetMetaData)))
 
 (set! *warn-on-reflection* true)
 
+(defn- consume-rset [^ResultSet rset cb]
+  (into {} (iteration
+            (fn [_]
+              (when (.next rset)
+                (cb rset))))))
+
 (defn app-db-column-types
   "Returns a map of all column names to their respective type names for the given `table-name` in the provided
   application-db."
   [app-db table-name']
-  (let [table-name (cond-> table-name'
+  (let [table-name (cond-> (name table-name')
                      (= (:db-type app-db) :h2) u/upper-case-en)]
     (t2/with-connection [^Connection conn]
-      (with-open [rset (.getColumns (.getMetaData conn) nil nil table-name nil)]
-        (into {}
-              (iteration
-               (fn [_]
-                 (when (.next rset)
-                   [(.getString rset "COLUMN_NAME") (.getString rset "TYPE_NAME")]))))))))
+      (let [md (.getMetaData conn)]
+        (with-open [cols (.getColumns md nil nil table-name nil)
+                    pks  (.getPrimaryKeys md nil nil table-name)
+                    fks  (.getImportedKeys md nil nil table-name)]
+          (let [pks (consume-rset pks (fn [^ResultSet pks]
+                                        [(.getString pks "COLUMN_NAME") true]))
+                fks (consume-rset fks (fn [^ResultSet fks]
+                                        [(.getString fks "FKCOLUMN_NAME")
+                                         (str (.getString fks "PKTABLE_NAME")
+                                              "."
+                                              (.getString fks "PKCOLUMN_NAME"))]))]
+            (consume-rset cols (fn [^ResultSet cols]
+                                 (let [col (.getString cols "COLUMN_NAME")]
+                                   [col {:type    (.getString cols "TYPE_NAME")
+                                         :notnull (= (.getInt cols "NULLABLE")
+                                                     ResultSetMetaData/columnNoNulls)
+                                         :pk      (get pks col)
+                                         :fk      (get fks col)}])))))))))