diff --git a/enterprise/backend/test/metabase_enterprise/serialization/v2/extract_test.clj b/enterprise/backend/test/metabase_enterprise/serialization/v2/extract_test.clj
index 6c5d6dd1308bf38fbe3718c966c3aef082a496b2..7a76f64d7b0307feaf4c8af8d64e3909a6f2947d 100644
--- a/enterprise/backend/test/metabase_enterprise/serialization/v2/extract_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/serialization/v2/extract_test.clj
@@ -1410,7 +1410,13 @@
(testing "Fields that reference parents are properly exported as Field references"
(is (= ["My Database" "PUBLIC" "Schema'd Table" "Other Field"]
- (:parent_id (ts/extract-one "Field" nested-id))))))))
+ (:parent_id (ts/extract-one "Field" nested-id))))
+ (is (= [{:model "Database", :id "My Database"}
+ {:model "Schema", :id "PUBLIC"}
+ {:model "Table", :id "Schema'd Table"}
+ {:model "Field", :id "Other Field"}
+ {:model "Field", :id "Nested Field"}]
+ (:serdes/meta (ts/extract-one "Field" nested-id))))))))
(deftest escape-report-test
(mt/with-empty-h2-app-db
diff --git a/enterprise/backend/test/metabase_enterprise/serialization/v2/load_test.clj b/enterprise/backend/test/metabase_enterprise/serialization/v2/load_test.clj
index 53e52cf74585e5269280a7f82b02648bc3835b25..1ca78c9cb56fef646874b1fc348db39fe697e91e 100644
--- a/enterprise/backend/test/metabase_enterprise/serialization/v2/load_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/serialization/v2/load_test.clj
@@ -1336,3 +1336,37 @@
(testing (format "%s has identity hash in the db" model)
(is (= (serdes/identity-hash e)
(serdes/entity-id (name model) e))))))))))))))
+
+(deftest identically-named-fields-test
+ (mt/with-empty-h2-app-db
+ (let [db (ts/create! Database :name "mydb")
+ t (ts/create! Table :name "table" :db_id (:id db))
+ f1 (ts/create! Field :name "field" :table_id (:id t))
+ ;; name is the same, but parent_id is different
+ f2 (ts/create! Field :name "field" :table_id (:id t) :parent_id (:id f1))
+ f3 (ts/create! Field :name "field" :table_id (:id t) :parent_id (:id f2)
+ :description "desc")
+
+ ser (into [] (serdes.extract/extract {}))]
+
+ (is (=? {:parent_id ["mydb" nil "table" "field"]
+ :serdes/meta [{:model "Database" :id "mydb"}
+ {:model "Table" :id "table"}
+ {:model "Field" :id "field"}
+ {:model "Field" :id "field"}]}
+ (ts/extract-one "Field" (:id f2))))
+
+ (is (=? {:parent_id ["mydb" nil "table" "field" "field"],
+ :serdes/meta [{:model "Database" :id "mydb"}
+ {:model "Table" :id "table"}
+ {:model "Field" :id "field"}
+ {:model "Field" :id "field"}
+ {:model "Field" :id "field"}]}
+ (ts/extract-one "Field" (:id f3))))
+
+ (t2/update! :model/Field (:id f3) {:description "some new one"})
+
+ (is (serdes.load/load-metabase! (ingestion-in-memory ser)))
+
+ (is (= "desc"
+ (t2/select-one-fn :description :model/Field (:id f3)))))))
diff --git a/enterprise/backend/test/metabase_enterprise/serialization/v2/storage_test.clj b/enterprise/backend/test/metabase_enterprise/serialization/v2/storage_test.clj
index d468cb3e7a348e0ce18759af65682f9995686c49..a03a94e5905da22e349d20ea17b625dd743836b0 100644
--- a/enterprise/backend/test/metabase_enterprise/serialization/v2/storage_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/serialization/v2/storage_test.clj
@@ -10,6 +10,7 @@
[metabase-enterprise.serialization.v2.storage :as storage]
[metabase.models :refer [Card Collection Dashboard DashboardCard Database Field FieldValues NativeQuerySnippet
Table]]
+ [metabase.models.serialization :as serdes]
[metabase.test :as mt]
[metabase.util.yaml :as yaml]
[toucan2.core :as t2]))
@@ -221,3 +222,18 @@
(is (thrown-with-msg? Exception #"Destination path is not writeable: "
(storage/store! [{:serdes/meta [{:model "A" :id "B"}]}]
dump-dir)))))))))
+
+(deftest nested-fields-test
+ (ts/with-random-dump-dir [dump-dir "serdesv2-"]
+ (mt/with-empty-h2-app-db
+ (let [db (ts/create! Database :name "mydb")
+ t (ts/create! Table :name "table" :db_id (:id db))
+ f1 (ts/create! Field :name "parent" :table_id (:id t))
+ _f2 (ts/create! Field :name "child" :table_id (:id t) :parent_id (:id f1))]
+ (serdes/with-cache
+ (-> (extract/extract {:no-settings true})
+ (storage/store! dump-dir)))
+ (testing "we get correct names for nested fields"
+ (is (= #{["parent.yaml"]
+ ["parent.child.yaml"]}
+ (file-set (io/file dump-dir "databases" "mydb" "tables" "table" "fields")))))))))
diff --git a/src/metabase/models/field.clj b/src/metabase/models/field.clj
index c91b87559f05cadb09cd3a988b3665a12e134eb4..649f001f604c877cb40accde03615d34f52abf75 100644
--- a/src/metabase/models/field.clj
+++ b/src/metabase/models/field.clj
@@ -353,41 +353,38 @@
;; In order to retrieve the dependencies for a field its table_id needs to be serialized as [database schema table],
;; a trio of strings with schema maybe nil.
-(defmethod serdes/generate-path "Field" [_ {table_id :table_id field :name}]
- (let [table (when (number? table_id)
- (t2/select-one 'Table :id table_id))
- db (when table
- (t2/select-one-fn :name 'Database :id (:db_id table)))
- [db schema table] (if (number? table_id)
- [db (:schema table) (:name table)]
- ;; If table_id is not a number, it's already been exported as a [db schema table] triple.
- table_id)]
- (filterv some? [{:model "Database" :id db}
- (when schema {:model "Schema" :id schema})
- {:model "Table" :id table}
- {:model "Field" :id field}])))
+(defmethod serdes/generate-path "Field" [_ field]
+ (let [[db schema table & fields] (serdes/*export-field-fk* (:id field))]
+ (->> (into (serdes/table->path [db schema table])
+ (map (fn [n] {:model "Field" :id n}) fields))
+ (filterv some?))))
(defmethod serdes/entity-id "Field" [_ {:keys [name]}]
name)
(defmethod serdes/load-find-local "Field"
[path]
- (let [table (serdes/load-find-local (pop path))]
- (t2/select-one Field :name (-> path last :id) :table_id (:id table))))
+ (let [[table-path fields] (split-with #(not= "Field" (:model %)) path)
+ table (serdes/load-find-local table-path)
+ field-q (serdes/recursively-find-field-q (:id table) (map :id (reverse fields)))]
+ (t2/select-one :model/Field field-q)))
(defmethod serdes/dependencies "Field" [field]
;; Fields depend on their parent Table, plus any foreign Fields referenced by their Dimensions.
;; Take the path, but drop the Field section to get the parent Table's path instead.
(let [this (serdes/path field)
- table (pop this)
+ table (remove #(= "Field" (:model %)) this)
fks (some->> field :fk_target_field_id serdes/field->path)
human (->> (:dimensions field)
(keep :human_readable_field_id)
(map serdes/field->path)
set)]
- (cond-> (set/union #{table} human)
- fks (set/union #{fks})
- true (disj this))))
+ (-> (set/union
+ #{table}
+ human
+ (when fks #{fks})
+ (when (:parent_id field) #{(butlast this)}))
+ (disj this))))
(defmethod serdes/make-spec "Field" [_model-name opts]
{:copy [:active :base_type :caveats :coercion_strategy :custom_position :database_indexed
@@ -403,7 +400,6 @@
:dimensions (serdes/nested :model/Dimension :field_id opts)}})
(defmethod serdes/storage-path "Field" [field _]
- (-> (serdes/path field)
- drop-last
- serdes/storage-table-path-prefix
- (concat ["fields" (:name field)])))
+ (let [[path fields] (split-with #(not= "Field" (:model %)) (serdes/path field))]
+ (concat (serdes/storage-path-prefixes path)
+ ["fields" (str/join "." (map :id fields))])))
diff --git a/src/metabase/models/field_values.clj b/src/metabase/models/field_values.clj
index 1fc54b81504e408823e31a922c40a0655f741cb7..ed218c6caf8e9f7c2fe8e5445b179b924030fc64 100644
--- a/src/metabase/models/field_values.clj
+++ b/src/metabase/models/field_values.clj
@@ -615,7 +615,5 @@
;; [path to table "fields" "field-name___fieldvalues"] since there's zero or one FieldValues per Field, and Fields
;; don't have their own directories.
(let [hierarchy (serdes/path fv)
- field (last (drop-last hierarchy))
- table-prefix (serdes/storage-table-path-prefix (drop-last 2 hierarchy))]
- (concat table-prefix
- ["fields" (str (:id field) field-values-slug)])))
+ field-path (serdes/storage-path-prefixes (drop-last hierarchy))]
+ (update field-path (dec (count field-path)) str field-values-slug)))
diff --git a/src/metabase/models/segment.clj b/src/metabase/models/segment.clj
index cc0cf4bd7c71eb9f279f29d703d2fbc44380b959..ab3355d9c59eb3659607437e404267c353c140a1 100644
--- a/src/metabase/models/segment.clj
+++ b/src/metabase/models/segment.clj
@@ -164,7 +164,7 @@
(-> segment
:table_id
serdes/table->path
- serdes/storage-table-path-prefix
+ serdes/storage-path-prefixes
(concat ["segments" (serdes/storage-leaf-file-name id label)]))))
(defmethod serdes/make-spec "Segment" [_model-name _opts]
diff --git a/src/metabase/models/serialization.clj b/src/metabase/models/serialization.clj
index 45d6db76f846b2a157b9c65009b08a10c1434949..0b16d5a90a98583d3ccb684d2c6ad5724a9bd8fd 100644
--- a/src/metabase/models/serialization.clj
+++ b/src/metabase/models/serialization.clj
@@ -14,6 +14,8 @@
- Add it to the appropriate list in [[metabase-enterprise.serialization.v2.models]]
- If it is in the `excluded-models` list, then your job is done
+ - If it's not, you probably want `entity_id` field autopopulated (that's a most common way to make model
+ transportable between instances), add `(derive :model/Model :hook/entity-id)` to your model
- Define serialization multi-methods on a model (see `Card` and `Collection` for more complex examples or `Action`
and `Segment` for less involved stuff)
- `serdes/make-spec` - this is the main entry point. Should list every field in the model (this is checked in
@@ -941,7 +943,12 @@
(when schema {:model "Schema" :id schema})
{:model "Table" :id table-name}]))
-(defn storage-table-path-prefix
+(def ^:private STORAGE-DIRS {"Database" "databases"
+ "Schema" "schemas"
+ "Table" "tables"
+ "Field" "fields"})
+
+(defn storage-path-prefixes
"The [[serdes/storage-path]] for Table is a bit tricky, and shared with Fields and FieldValues, so it's
factored out here.
Takes the :serdes/meta value for a `Table`!
@@ -950,32 +957,57 @@
With a schema: `[\"databases\" \"db_name\" \"schemas\" \"public\" \"tables\" \"customers\"]`
No schema: `[\"databases\" \"db_name\" \"tables\" \"customers\"]`"
[path]
- (let [db-name (-> path first :id)
- schema (when (= (count path) 3)
- (-> path second :id))
- table-name (-> path last :id)]
- (concat ["databases" db-name]
- (when schema ["schemas" schema])
- ["tables" table-name])))
+ (into [] cat
+ (for [entry path]
+ [(or (get STORAGE-DIRS (:model entry))
+ (throw (ex-info "Could not find dir name" {:entry entry})))
+ (:id entry)])))
;;; ## Fields
+(defn- field-hierarchy [id]
+ (reverse
+ (t2/select :model/Field
+ {:with-recursive [[[:parents {:columns [:id :name :parent_id :table_id]}]
+ {:union-all [{:from [[:metabase_field :mf]]
+ :select [:mf.id :mf.name :mf.parent_id :mf.table_id]
+ :where [:= :id id]}
+ {:from [[:metabase_field :pf]]
+ :select [:pf.id :pf.name :pf.parent_id :pf.table_id]
+ :join [[:parents :p] [:= :p.parent_id :pf.id]]}]}]]
+ :from [:parents]
+ :select [:name :table_id]})))
+
+(defn recursively-find-field-q
+ "Build a query to find a field among parents (should start with bottom-most field first), i.e.:
+
+ `(recursively-find-field-q 1 [\"inner\" \"outer\"])`"
+ [table-id [field & rest]]
+ (when field
+ {:from [:metabase_field]
+ :select [:id]
+ :where [:and
+ [:= :table_id table-id]
+ [:= :name field]
+ [:= :parent_id (recursively-find-field-q table-id rest)]]}))
+
(defn ^:dynamic ^::cache *export-field-fk*
"Given a numeric `field_id`, return a portable field reference.
That has the form `[db-name schema table-name field-name]`, where the `schema` might be nil.
- [[import-field-fk]] is the inverse."
+ [[*import-field-fk*]] is the inverse."
[field-id]
(when field-id
- (let [{:keys [name table_id]} (t2/select-one 'Field :id field-id)
- [db-name schema field-name] (*export-table-fk* table_id)]
- [db-name schema field-name name])))
+ (let [fields (field-hierarchy field-id)
+ [db-name schema field-name] (*export-table-fk* (:table_id (first fields)))]
+ (into [db-name schema field-name] (map :name fields)))))
(defn ^:dynamic ^::cache *import-field-fk*
- "Given a `field_id` as exported by [[export-field-fk]], resolve it back into a numeric `field_id`."
- [[db-name schema table-name field-name :as field-id]]
+ "Given a `field_id` as exported by [[*export-field-fk*]], resolve it back into a numeric `field_id`."
+ [[db-name schema table-name & fields :as field-id]]
(when field-id
- (let [table_id (*import-table-fk* [db-name schema table-name])]
- (t2/select-one-pk 'Field :table_id table_id :name field-name))))
+ (let [table-id (*import-table-fk* [db-name schema table-name])
+ field-q (recursively-find-field-q table-id (reverse fields))]
+ (t2/select-one-pk :model/Field field-q))))
(defn field->path
"Given a `field_id` as exported by [[export-field-fk]], turn it into a `[{:model ...}]` path for the Field.
diff --git a/src/metabase/models/table.clj b/src/metabase/models/table.clj
index b0822a79e0d29b31c30ce73fff7c40ac241fb3ff..dabffabcd9df246668aa3101289cce1e5f5a3b8f 100644
--- a/src/metabase/models/table.clj
+++ b/src/metabase/models/table.clj
@@ -288,7 +288,7 @@
:db_id (serdes/fk :model/Database :name)}})
(defmethod serdes/storage-path "Table" [table _ctx]
- (concat (serdes/storage-table-path-prefix (serdes/path table))
+ (concat (serdes/storage-path-prefixes (serdes/path table))
[(:name table)]))
;;; -------------------------------------------------- Audit Log Table -------------------------------------------------