Serdes v2: Several fixes that came out of the git <-> prod workflow (#25310)

- Limit the scanning of directories and files to those named after
  models; don't try to ingest `.git`, `README.md`, etc.
- `table_id` and `collection_id` are optional on Cards
- Deserialization was not resolving some deeply nested `:field`s inside MBQL
  queries.

enterprise/backend/src/metabase_enterprise/serialization/cmd.clj

@@ -74,6 +74,7 @@
   ; TODO This should be restored, but there's no manifest or other meta file written by v2 dumps.
   ;(when-not (load/compatible? path)
   ;  (log/warn (trs "Dump was produced using a different version of Metabase. Things may break!")))
+  (log/info (trs "Loading serialized Metabase files from {0}" path))
   (v2.load/load-metabase (v2.ingest/ingest-yaml path)))
 
 (defn load

enterprise/backend/src/metabase_enterprise/serialization/v2/ingest/yaml.clj

@@ -3,6 +3,7 @@
   to avoid confusion with filesystem paths."
   (:require [clojure.java.io :as io]
             [metabase-enterprise.serialization.v2.ingest :as ingest]
+            [metabase-enterprise.serialization.v2.models :as models]
             [metabase-enterprise.serialization.v2.utils.yaml :as u.yaml]
             [metabase.util.date-2 :as u.date]
             [yaml.core :as yaml]
@@ -29,7 +30,8 @@
 
 (defn- read-timestamps [entity]
   (->> (keys entity)
-       (filter #(.endsWith (name %) "_at"))
+       (filter #(or (#{:last_analyzed} %)
+                    (.endsWith (name %) "_at")))
        (reduce #(update %1 %2 u.date/parse) entity)))
 
 (defn- ingest-entity
@@ -50,9 +52,17 @@
 (deftype YamlIngestion [^File root-dir settings]
   ingest/Ingestable
   (ingest-list [_]
-    (eduction (comp (filter (fn [^File f] (.isFile f)))
-                    (mapcat (partial build-metas root-dir)))
-              (file-seq root-dir)))
+    (let [model-set (set models/exported-models)]
+      (eduction (comp (filter (fn [^File f] (.isFile f)))
+                      ;; The immediate parent directory should be a recognized model name.
+                      ;; If it's not, this may be in .git, or .github/actions/... or similar extra files.
+                      (filter (fn [^File f] (or (= (.getName f) "settings.yaml")
+                                                (-> f
+                                                    (.getParentFile)
+                                                    (.getName)
+                                                    model-set))))
+                      (mapcat (partial build-metas root-dir)))
+                (file-seq root-dir))))
 
   (ingest-one [_ abs-path]
     (let [{:keys [model id]} (first abs-path)]

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

@@ -236,8 +236,9 @@
                        ser))
           (is (not (contains? ser :id)))
 
-          (testing "cards depend on their Table and Collection, and any fields in their parameter_mappings"
-            (is (= #{[{:model "Database"   :id "My Database"}
+          (testing "cards depend on their Database, Table and Collection, and any fields in their parameter_mappings"
+            (is (= #{[{:model "Database"   :id "My Database"}]
+                     [{:model "Database"   :id "My Database"}
                       {:model "Schema"     :id "PUBLIC"}
                       {:model "Table"      :id "Schema'd Table"}]
                      [{:model "Collection" :id coll-eid}]
@@ -283,8 +284,9 @@
                        ser))
           (is (not (contains? ser :id)))
 
-          (testing "cards depend on their Table and Collection, and any fields in their visualization_settings"
-            (is (= #{[{:model "Database"   :id "My Database"}
+          (testing "cards depend on their Database, Table and Collection, and any fields in their visualization_settings"
+            (is (= #{[{:model "Database"   :id "My Database"}]
+                     [{:model "Database"   :id "My Database"}
                       {:model "Schema"     :id "PUBLIC"}
                       {:model "Table"      :id "Schema'd Table"}]
                      [{:model "Collection" :id coll-eid}]

src/metabase/models/card.clj

@@ -374,12 +374,13 @@
       (update :visualization_settings serdes.util/import-visualization-settings)))
 
 (defmethod serdes.base/serdes-dependencies "Card"
-  [{:keys [collection_id dataset_query parameter_mappings table_id visualization_settings]}]
-  ;; The Table implicitly depends on the Database.
+  [{:keys [collection_id database_id dataset_query parameter_mappings table_id visualization_settings]}]
   (->> (map serdes.util/mbql-deps parameter_mappings)
        (reduce set/union)
-       (set/union #{(serdes.util/table->path table_id)
-                    [{:model "Collection" :id collection_id}]})
+       (set/union #{[{:model "Database" :id database_id}]})
+       ; table_id and collection_id are nullable.
+       (set/union (when table_id #{(serdes.util/table->path table_id)}))
+       (set/union (when collection_id #{[{:model "Collection" :id collection_id}]}))
        (set/union (serdes.util/mbql-deps dataset_query))
        (set/union (serdes.util/visualization-settings-deps visualization_settings))
        vec))

src/metabase/models/serialization/util.clj

@@ -67,17 +67,21 @@
 ;; -------------------------------------------------- Tables ---------------------------------------------------------
 (defn export-table-fk
   "Given a numeric `table_id`, return a portable table reference.
+  If the `table_id` is `nil`, return `nil`. This is legal for a native question.
   That has the form `[db-name schema table-name]`, where the `schema` might be nil.
   [[import-table-fk]] is the inverse."
   [table-id]
-  (let [{:keys [db_id name schema]} (db/select-one 'Table :id table-id)
-        db-name                     (db/select-one-field :name 'Database :id db_id)]
-    [db-name schema name]))
+  (when table-id
+    (let [{:keys [db_id name schema]} (db/select-one 'Table :id table-id)
+          db-name                     (db/select-one-field :name 'Database :id db_id)]
+      [db-name schema name])))
 
 (defn import-table-fk
-  "Given a `table_id` as exported by [[export-table-fk]], resolve it back into a numeric `table_id`."
-  [[db-name schema table-name]]
-  (db/select-one-field :id 'Table :name table-name :schema schema :db_id (db/select-one-field :id 'Database :name db-name)))
+  "Given a `table_id` as exported by [[export-table-fk]], resolve it back into a numeric `table_id`.
+  The input might be nil, in which case so is the output. This is legal for a native question."
+  [[db-name schema table-name :as table-id]]
+  (when table-id
+    (db/select-one-field :id 'Table :name table-name :schema schema :db_id (db/select-one-field :id 'Database :name db-name))))
 
 (defn table->path
   "Given a `table_id` as exported by [[export-table-fk]], turn it into a `[{:model ...}]` path for the Table.
@@ -201,10 +205,10 @@
     ;; handle legacy `:field-id` forms encoded prior to 0.39.0
     ;; and also *current* expresion forms used in parameter mapping dimensions
     ;; example relevant clause - [:dimension [:fk-> [:field-id 1] [:field-id 2]]]
-    [:field-id (fully-qualified-name :guard string?)]
+    [(:or :field-id "field-id") (fully-qualified-name :guard string?)]
     (mbql-fully-qualified-names->ids* [:field fully-qualified-name nil])
 
-    [:field (fully-qualified-name :guard vector?) opts]
+    [(:or :field "field") (fully-qualified-name :guard vector?) opts]
     [:field (import-field-fk fully-qualified-name) (mbql-fully-qualified-names->ids* opts)]
 
     ;; source-field is also used within parameter mapping dimensions
@@ -217,15 +221,27 @@
         (assoc :database (db/select-one-id 'Database :name fully-qualified-name))
         mbql-fully-qualified-names->ids*) ; Process other keys
 
-    [:metric (fully-qualified-name :guard serdes.base/entity-id?)]
+    {:card-id (entity-id :guard (every-pred string? serdes.base/entity-id?))}
+    (-> &match
+        (assoc :card-id (import-fk entity-id 'Card))
+        mbql-fully-qualified-names->ids*) ; Process other keys
+
+    [(:or :metric "metric") (fully-qualified-name :guard serdes.base/entity-id?)]
     [:metric (import-fk fully-qualified-name 'Metric)]
 
-    [:segment (fully-qualified-name :guard serdes.base/entity-id?)]
+    [(:or :segment "segment") (fully-qualified-name :guard serdes.base/entity-id?)]
     [:segment (import-fk fully-qualified-name 'Segment)]
 
     (_ :guard (every-pred map? #(vector? (:source-table %))))
     (-> &match
         (assoc :source-table (import-table-fk (:source-table &match)))
+        mbql-fully-qualified-names->ids*)
+
+    (_ :guard (every-pred map?
+                          #(string? (:source-table %))
+                          #(serdes.base/entity-id? (:source-table %))))
+    (-> &match
+        (assoc :source-table (str "card__" (import-fk (:source-table &match) 'Card)))
         mbql-fully-qualified-names->ids*))) ;; process other keys
 
 (defn- mbql-fully-qualified-names->ids