From aa3b331bd3163894d09a9cfda1afc1668a8946be Mon Sep 17 00:00:00 2001
From: Braden Shepherdson <Braden.Shepherdson@gmail.com>
Date: Mon, 22 Aug 2022 09:25:44 -0400
Subject: [PATCH] Add serdes v2 for FieldValues, which were previously excluded
(#24855)
FieldValues can be rebuilt by the sync process, but they are portable
and including them aids in the plans for content moderation via git.
---
.../serialization/v2/models.clj | 1 +
.../serialization/v2/extract_test.clj | 45 ++++++-
.../serialization/v2/load_test.clj | 110 +++++++++++++++++-
src/metabase/models/field.clj | 10 +-
src/metabase/models/field_values.clj | 32 +++++
5 files changed, 186 insertions(+), 12 deletions(-)
diff --git a/enterprise/backend/src/metabase_enterprise/serialization/v2/models.clj b/enterprise/backend/src/metabase_enterprise/serialization/v2/models.clj
index 93ed0f165b4..b1da6cc9d58 100644
--- a/enterprise/backend/src/metabase_enterprise/serialization/v2/models.clj
+++ b/enterprise/backend/src/metabase_enterprise/serialization/v2/models.clj
@@ -9,6 +9,7 @@
"Database"
"Dimension"
"Field"
+ "FieldValues"
"Metric"
"NativeQuerySnippet"
"Pulse"
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 71819189ead..8046f0b7154 100644
--- a/enterprise/backend/test/metabase_enterprise/serialization/v2/extract_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/serialization/v2/extract_test.clj
@@ -3,7 +3,7 @@
[clojure.test :refer :all]
[metabase-enterprise.serialization.test-util :as ts]
[metabase-enterprise.serialization.v2.extract :as extract]
- [metabase.models :refer [Card Collection Dashboard DashboardCard Database Dimension Field Metric
+ [metabase.models :refer [Card Collection Dashboard DashboardCard Database Dimension Field FieldValues Metric
NativeQuerySnippet Pulse PulseCard Segment Table Timeline TimelineEvent User]]
[metabase.models.serialization.base :as serdes.base]
[schema.core :as s])
@@ -649,6 +649,49 @@
{:model "Field" :id "Some Field"}]}
(set (serdes.base/serdes-dependencies ser))))))))))
+(deftest field-values-test
+ (ts/with-empty-h2-app-db
+ (ts/with-temp-dpc [Database [{db-id :id} {:name "My Database"}]
+ Table [{no-schema-id :id} {:name "Schemaless Table" :db_id db-id}]
+ Field [{field-id :id} {:name "Some Field"
+ :table_id no-schema-id
+ :fingerprint {:global {:distinct-count 75 :nil% 0.0}
+ :type {:type/Text {:percent-json 0.0
+ :percent-url 0.0
+ :percent-email 0.0
+ :percent-state 0.0
+ :average-length 8.333333333333334}}}}]
+ FieldValues [{fv-id :id
+ values :values}
+ {:field_id field-id
+ :hash_key nil
+ :has_more_values false
+ :type :full
+ :human_readable_values []
+ :values ["Artisan" "Asian" "BBQ" "Bakery" "Bar" "Brewery" "Burger" "Coffee Shop"
+ "Diner" "Indian" "Italian" "Japanese" "Mexican" "Middle Eastern" "Pizza"
+ "Seafood" "Steakhouse" "Tea Room" "Winery"]}]]
+ (testing "field values"
+ (let [ser (serdes.base/extract-one "FieldValues" {} (select-one "FieldValues" [:= :id fv-id]))]
+ (is (schema= {:serdes/meta (s/eq [{:model "Database" :id "My Database"}
+ {:model "Table" :id "Schemaless Table"}
+ {:model "Field" :id "Some Field"}
+ {:model "FieldValues" :id "0"}]) ; Always 0.
+ :created_at LocalDateTime
+ (s/optional-key :updated_at) OffsetDateTime
+ :values (s/eq (json/generate-string values))
+ s/Keyword s/Any}
+ ser))
+ (is (not (contains? ser :id)))
+ (is (not (contains? ser :field_id))
+ ":field_id is dropped; its implied by the path")
+
+ (testing "depend on the parent Field"
+ (is (= #{[{:model "Database" :id "My Database"}
+ {:model "Table" :id "Schemaless Table"}
+ {:model "Field" :id "Some Field"}]}
+ (set (serdes.base/serdes-dependencies ser))))))))))
+
(deftest pulses-test
(ts/with-empty-h2-app-db
(ts/with-temp-dpc [User [{ann-id :id} {:first_name "Ann"
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 4f86606031e..bf635ac89b9 100644
--- a/enterprise/backend/test/metabase_enterprise/serialization/v2/load_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/serialization/v2/load_test.clj
@@ -4,8 +4,8 @@
[metabase-enterprise.serialization.v2.extract :as serdes.extract]
[metabase-enterprise.serialization.v2.ingest :as serdes.ingest]
[metabase-enterprise.serialization.v2.load :as serdes.load]
- [metabase.models :refer [Card Collection Dashboard DashboardCard Database Field Metric Pulse PulseChannel
- PulseChannelRecipient Segment Table User]]
+ [metabase.models :refer [Card Collection Dashboard DashboardCard Database Field FieldValues Metric Pulse
+ PulseChannel PulseChannelRecipient Segment Table User]]
[metabase.models.serialization.base :as serdes.base]
[metabase.models.serialization.hash :as serdes.hash]
[toucan.db :as db]))
@@ -153,7 +153,7 @@
t1s (atom nil)
t2s (atom nil)]
(ts/with-source-and-dest-dbs
- (testing "serializing the two collections"
+ (testing "serializing the two databases"
(ts/with-source-db
(reset! db1s (ts/create! Database :name "db1"))
(reset! t1s (ts/create! Table :name "posts" :db_id (:id @db1s)))
@@ -659,3 +659,107 @@
:card_id (:id @card1d)
:target [:dimension [:field (:id @field1d) {:source-field (:id @field2d)}]]}]
(:parameter_mappings @dashcard1d))))))))))
+
+(deftest field-values-test
+ ;; FieldValues are a bit special - they map 1-1 with Fields but are a separate table serialized separately.
+ ;; The main special thing to test here is that the custom load-find-local correctly finds an existing FieldValues.
+ ;; This test creates:
+ ;; - in src: a database, table, and two fields each with field values.
+ ;; - in dst: a different database, table and field, to fiddle the IDs; plus the same database, table, both fields, but
+ ;; only one FieldValues. The existing and new FieldValues should both work correctly.
+ ;; Another thing tested here is that the :field_id is properly reconstructed from the serdes path.
+ (testing "FieldValues are portable"
+ (let [serialized (atom nil)
+ db1s (atom nil)
+ table1s (atom nil)
+ field1s (atom nil)
+ field2s (atom nil)
+ fv1s (atom nil)
+ fv2s (atom nil)
+
+ db1d (atom nil)
+ table1d (atom nil)
+ field1d (atom nil)
+ field2d (atom nil)
+ fv1d (atom nil)
+ fv2d (atom nil)
+ db2d (atom nil)
+ table2d (atom nil)
+ field3d (atom nil)]
+
+ (testing "serializing the original database, table, field and fieldvalues"
+ (ts/with-empty-h2-app-db
+ (reset! db1s (ts/create! Database :name "my-db"))
+ (reset! table1s (ts/create! Table :name "CUSTOMERS" :db_id (:id @db1s)))
+ (reset! field1s (ts/create! Field :name "STATE" :table_id (:id @table1s)))
+ (reset! field2s (ts/create! Field :name "CATEGORY" :table_id (:id @table1s)))
+ (reset! fv1s (ts/create! FieldValues :field_id (:id @field1s) :values ["AZ" "CA" "NY" "TX"]))
+ (reset! fv2s (ts/create! FieldValues :field_id (:id @field2s)
+ :values ["CONSTRUCTION" "DAYLIGHTING" "DELIVERY" "HAULING"]))
+
+ (reset! serialized (into [] (serdes.extract/extract-metabase {})))
+
+ (testing "the expected fields are serialized"
+ (is (= 1
+ (->> @serialized
+ (filter #(= (:serdes/meta %)
+ [{:model "Database" :id "test-data"}
+ {:model "Schema" :id "PUBLIC"}
+ {:model "Table" :id "VENUES"}
+ {:model "Field" :id "NAME"}]))
+ count))))
+
+ (testing "FieldValues are serialized under their fields, with their own ID always 0"
+ (let [fvs (by-model @serialized "FieldValues")]
+ (is (= #{[{:model "Database" :id "my-db"}
+ {:model "Table" :id "CUSTOMERS"}
+ {:model "Field" :id "STATE"}
+ {:model "FieldValues" :id "0"}]
+ [{:model "Database" :id "my-db"}
+ {:model "Table" :id "CUSTOMERS"}
+ {:model "Field" :id "CATEGORY"}
+ {:model "FieldValues" :id "0"}]}
+ (->> fvs
+ (map serdes.base/serdes-path)
+ (filter #(-> % first :id (= "my-db")))
+ set)))))))
+
+ (testing "deserializing finds existing FieldValues properly"
+ (ts/with-empty-h2-app-db
+ ;; A different database and tables, so the IDs don't match.
+ (reset! db2d (ts/create! Database :name "other-db"))
+ (reset! table2d (ts/create! Table :name "ORDERS" :db_id (:id @db2d)))
+ (reset! field3d (ts/create! Field :name "SUBTOTAL" :table_id (:id @table2d)))
+ (ts/create! Field :name "DISCOUNT" :table_id (:id @table2d))
+ (ts/create! Field :name "UNITS" :table_id (:id @table2d))
+
+ ;; Now the database, table, fields and *one* of the FieldValues from the src side.
+ (reset! db1d (ts/create! Database :name "my-db"))
+ (reset! table1d (ts/create! Table :name "CUSTOMERS" :db_id (:id @db1d)))
+ (reset! field1d (ts/create! Field :name "STATE" :table_id (:id @table1d)))
+ (reset! field2d (ts/create! Field :name "CATEGORY" :table_id (:id @table1d)))
+ ;; The :values are different here; they should get overwritten by the update.
+ (reset! fv1d (ts/create! FieldValues :field_id (:id @field1d) :values ["WA" "NC" "NM" "WI"]))
+
+ ;; Load the serialized content.
+ (serdes.load/load-metabase (ingestion-in-memory @serialized))
+
+ ;; Fetch the relevant bits
+ (reset! fv1d (db/select-one FieldValues :field_id (:id @field1d)))
+ (reset! fv2d (db/select-one FieldValues :field_id (:id @field2d)))
+
+ (testing "the main Database, Table, and Field have different IDs now"
+ (is (not= (:id @db1s) (:id @db1d)))
+ (is (not= (:id @table1s) (:id @table1d)))
+ (is (not= (:id @field1s) (:id @field1d)))
+ (is (not= (:id @field2s) (:id @field2d))))
+
+ (testing "there are 2 FieldValues defined under fields of table1d"
+ (let [fields (db/select-ids Field :table_id (:id @table1d))]
+ (is (= 2 (db/count FieldValues :field_id [:in fields])))))
+
+ (testing "existing FieldValues are properly found and updated"
+ (is (= (:values @fv1s) (:values @fv1d))))
+ (testing "new FieldValues are properly added"
+ (is (= (dissoc @fv2s :id :field_id)
+ (dissoc @fv2d :id :field_id)))))))))
diff --git a/src/metabase/models/field.clj b/src/metabase/models/field.clj
index cf4cc156d0e..f42fe2b062d 100644
--- a/src/metabase/models/field.clj
+++ b/src/metabase/models/field.clj
@@ -418,11 +418,5 @@
(defmethod serdes.base/load-find-local "Field"
[path]
- (let [db-name (-> path first :id)
- schema-name (when (= 3 (count path))
- (-> path second :id))
- [{table-name :id}
- {field-name :id}] (take-last 2 path)
- db-id (db/select-one-field :id 'Database :name db-name)
- table-id (db/select-one-field :id 'Table :name table-name :db_id db-id :schema schema-name)]
- (db/select-one-field :id Field :name field-name :table_id table-id)))
+ (let [table-id (serdes.base/load-find-local (pop path))]
+ (db/select-one-field :id Field :name (-> path last :id) :table_id table-id)))
diff --git a/src/metabase/models/field_values.clj b/src/metabase/models/field_values.clj
index a9496f20051..e6dcb0d823c 100644
--- a/src/metabase/models/field_values.clj
+++ b/src/metabase/models/field_values.clj
@@ -18,7 +18,9 @@
But they will also be automatically deleted when the Full FieldValues of the same Field got updated."
(:require [clojure.tools.logging :as log]
[java-time :as t]
+ [metabase.models.serialization.base :as serdes.base]
[metabase.models.serialization.hash :as serdes.hash]
+ [metabase.models.serialization.util :as serdes.util]
[metabase.plugins.classloader :as classloader]
[metabase.public-settings.premium-features :refer [defenterprise]]
[metabase.util :as u]
@@ -413,3 +415,33 @@
(trs "Field {0} ''{1}'' should have FieldValues and belongs to a Database with On-Demand FieldValues updating."
(u/the-id field) (:name field)))
(create-or-update-full-field-values! field)))))
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | Serialization |
+;;; +----------------------------------------------------------------------------------------------------------------+
+(defmethod serdes.base/serdes-generate-path "FieldValues" [_ {:keys [field_id]}]
+ (let [field (db/select-one 'Field :id field_id)]
+ (conj (serdes.base/serdes-generate-path "Field" field)
+ {:model "FieldValues" :id "0"})))
+
+(defmethod serdes.base/serdes-dependencies "FieldValues" [fv]
+ ;; Take the path, but drop the FieldValues section at the end, to get the parent Field's path instead.
+ [(pop (serdes.base/serdes-path fv))])
+
+(defmethod serdes.base/extract-one "FieldValues" [_model-name _opts fv]
+ (-> (serdes.base/extract-one-basics "FieldValues" fv)
+ (dissoc :field_id)))
+
+(defmethod serdes.base/load-xform "FieldValues" [fv]
+ (let [[db schema table field :as field-ref] (map :id (pop (serdes.base/serdes-path fv)))
+ field-ref (if field
+ field-ref
+ ;; It's too short, so no schema. Shift them over and add a nil schema.
+ [db nil schema table])]
+ (-> (serdes.base/load-xform-basics fv)
+ (assoc :field_id (serdes.util/import-field-fk field-ref)))))
+
+(defmethod serdes.base/load-find-local "FieldValues" [path]
+ ;; Delegate to finding the parent Field, then look up its corresponding FieldValues.
+ (let [field-id (serdes.base/load-find-local (pop path))]
+ (db/select-one-id FieldValues :field_id field-id)))
--
GitLab