From 32010a5b93c2cce1ff7f6ae9a930c1afe4f074f8 Mon Sep 17 00:00:00 2001
From: Jerry Huang <34140255+qwef@users.noreply.github.com>
Date: Tue, 25 Jul 2023 13:11:08 -0700
Subject: [PATCH] Resolve pivot tables in serialization (#32458)
* initial commit
* remove print
* add fix
* remove load
* fix tests
---
.../serialization/test_util.clj | 1 +
.../serialization/load.clj | 26 +++++--
.../serialization/load_test.clj | 67 ++++++++++++-------
.../serialization/test_util.clj | 15 ++++-
4 files changed, 78 insertions(+), 31 deletions(-)
diff --git a/.clj-kondo/macros/metabase_enterprise/serialization/test_util.clj b/.clj-kondo/macros/metabase_enterprise/serialization/test_util.clj
index 2f9a80749ad..6060fc36923 100644
--- a/.clj-kondo/macros/metabase_enterprise/serialization/test_util.clj
+++ b/.clj-kondo/macros/metabase_enterprise/serialization/test_util.clj
@@ -37,6 +37,7 @@
~(macros.common/ignore-unused 'card-id-nested) nil
~(macros.common/ignore-unused 'card-id-nested-query) nil
~(macros.common/ignore-unused 'card-id-native-query) nil
+ ~(macros.common/ignore-unused 'card-id-pivot-table) nil
~(macros.common/ignore-unused 'dashcard-id) nil
~(macros.common/ignore-unused 'dashcard-top-level-click-id) nil
~(macros.common/ignore-unused 'dashcard-with-click-actions) nil
diff --git a/enterprise/backend/src/metabase_enterprise/serialization/load.clj b/enterprise/backend/src/metabase_enterprise/serialization/load.clj
index 45c94c06566..6ccc6094d3a 100644
--- a/enterprise/backend/src/metabase_enterprise/serialization/load.clj
+++ b/enterprise/backend/src/metabase_enterprise/serialization/load.clj
@@ -395,6 +395,25 @@
(pull-unresolved-names-up vs-norm [::mb.viz/column-settings] resolved-cs))
vs-norm))
+(defn- resolve-table-column-field-ref [[f-type f-str f-md]]
+ (if (names/fully-qualified-field-name? f-str)
+ [f-type ((comp :field fully-qualified-name->context) f-str) f-md]
+ [f-type f-str f-md]))
+
+(defn- resolve-pivot-table-settings
+ "Resolve the entries in a :pivot_table.column_split map (which is under a :visualization_settings map). These map entries
+ may contain fully qualified field names, or even other cards. In case of an unresolved name (i.e. a card that hasn't
+ yet been loaded), we will track it under ::unresolved-names and revisit on the next pass."
+ [vs-norm]
+ (if (:pivot_table.column_split vs-norm)
+ (letfn [(resolve-vec [pivot vec-type]
+ (update-in pivot [:pivot_table.column_split vec-type] (fn [tbl-vecs]
+ (mapv resolve-table-column-field-ref tbl-vecs))))]
+ (-> vs-norm
+ (resolve-vec :rows)
+ (resolve-vec :columns)))
+ vs-norm))
+
(defn- resolve-table-columns
"Resolve the :table.columns key from a :visualization_settings map, which may contain fully qualified field names.
Such fully qualified names will be converted to the numeric field ID before being filled into the loaded card. Only
@@ -402,11 +421,7 @@
to detect or track ::unresolved-names."
[vs-norm]
(if (::mb.viz/table-columns vs-norm)
- (letfn [(resolve-table-column-field-ref [[f-type f-str f-md]]
- (if (names/fully-qualified-field-name? f-str)
- [f-type ((comp :field fully-qualified-name->context) f-str) f-md]
- [f-type f-str f-md]))
- (resolve-field-id [tbl-col]
+ (letfn [(resolve-field-id [tbl-col]
(update tbl-col ::mb.viz/table-column-field-ref resolve-table-column-field-ref))]
(update vs-norm ::mb.viz/table-columns (fn [tbl-cols]
(mapv resolve-field-id tbl-cols))))
@@ -425,6 +440,7 @@
resolve-top-level-click-behavior
resolve-column-settings
resolve-table-columns
+ resolve-pivot-table-settings
mb.viz/norm->db)]
(pull-unresolved-names-up entity [:visualization_settings] resolved-vs))
entity))
diff --git a/enterprise/backend/test/metabase_enterprise/serialization/load_test.clj b/enterprise/backend/test/metabase_enterprise/serialization/load_test.clj
index df1830638d8..cf02b369695 100644
--- a/enterprise/backend/test/metabase_enterprise/serialization/load_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/serialization/load_test.clj
@@ -131,33 +131,47 @@
(fn [instance _fingerprint]
(mi/model instance)))
+(defn- test-loaded-card [card card-name]
+ (when (= "My Nested Card" card-name)
+ (testing "Visualization settings for a Card were persisted correctly"
+ (let [vs (:visualization_settings card)
+ col (-> (:column_settings vs)
+ first)
+ [col-key col-val] col
+ col-ref (mb.viz/parse-db-column-ref col-key)
+ {:keys [::mb.viz/field-id]} col-ref
+ [{col-name :name col-field-ref :fieldRef col-enabled :enabled :as _tbl-col} & _] (:table.columns vs)
+ [_ col-field-id _] col-field-ref]
+ (is (some? (:table.columns vs)))
+ (is (some? (:column_settings vs)))
+ (is (integer? field-id))
+ (is (= "latitude" (-> (t2/select-one-fn :name Field :id field-id)
+ u/lower-case-en)))
+ (is (= {:show_mini_bar true
+ :column_title "Parallel"} col-val))
+ (is (= "Venue Category" col-name))
+ (is (true? col-enabled))
+ (is (integer? col-field-id) "fieldRef within table.columns was properly serialized and loaded")
+ (is (= "category_id" (-> (t2/select-one-fn :name Field :id col-field-id)
+ u/lower-case-en)))))))
+
+(defn- test-pivot-card [card card-name]
+ (when (= "Pivot Table Card" card-name)
+ (testing "Visualization settings for a Card were persisted correctly"
+ (let [vs (:visualization_settings card)
+ pivot (:pivot_table.column_split vs)
+ vecs (concat (:columns pivot) (:rows pivot))]
+ (is (some? vecs))
+ (doseq [[_ field-id _] vecs]
+ (is (integer? field-id) "fieldRef within pivot table was properly serialized and loaded"))))))
+
(defmethod assert-loaded-entity Card
[{card-name :name :as card} {:keys [query-results collections]}]
(testing (format "Card: %s" card-name)
(query-res-match query-results card)
(collection-names-match collections card)
- (when (= "My Nested Card" card-name)
- (testing "Visualization settings for a Card were persisted correctly"
- (let [vs (:visualization_settings card)
- col (-> (:column_settings vs)
- first)
- [col-key col-val] col
- col-ref (mb.viz/parse-db-column-ref col-key)
- {:keys [::mb.viz/field-id]} col-ref
- [{col-name :name col-field-ref :fieldRef col-enabled :enabled :as _tbl-col} & _] (:table.columns vs)
- [_ col-field-id _] col-field-ref]
- (is (some? (:table.columns vs)))
- (is (some? (:column_settings vs)))
- (is (integer? field-id))
- (is (= "latitude" (-> (t2/select-one-fn :name Field :id field-id)
- u/lower-case-en)))
- (is (= {:show_mini_bar true
- :column_title "Parallel"} col-val))
- (is (= "Venue Category" col-name))
- (is (true? col-enabled))
- (is (integer? col-field-id) "fieldRef within table.columns was properly serialized and loaded")
- (is (= "category_id" (-> (t2/select-one-fn :name Field :id col-field-id)
- u/lower-case-en))))))
+ (test-loaded-card card card-name)
+ (test-pivot-card card card-name)
card))
(defn- collection-parent-name [collection]
@@ -332,7 +346,8 @@
card-id-temporal-unit
card-id-with-native-snippet
card-id-temporal-unit
- card-join-card-id])
+ card-join-card-id
+ card-id-pivot-table])
:collections (gather-collections [card-id
card-arch-id
card-id-root
@@ -346,7 +361,8 @@
card-id-temporal-unit
card-id-with-native-snippet
card-id-temporal-unit
- card-join-card-id])
+ card-join-card-id
+ card-id-pivot-table])
:entities [[Database (t2/select-one Database :id db-id)]
[Table (t2/select-one Table :id table-id)]
[Table (t2/select-one Table :id table-id-categories)]
@@ -392,7 +408,8 @@
[Collection (t2/select-one Collection :id snippet-nested-collection-id)]
[NativeQuerySnippet (t2/select-one NativeQuerySnippet :id nested-snippet-id)]
[Card (t2/select-one Card :id card-id-with-native-snippet)]
- [Card (t2/select-one Card :id card-join-card-id)]]})]
+ [Card (t2/select-one Card :id card-join-card-id)]
+ [Card (t2/select-one Card :id card-id-pivot-table)]]})]
(with-world-cleanup
(v1-load dump-dir {:on-error :continue :mode :skip})
(mt/with-db (t2/select-one Database :name ts/temp-db-name)
diff --git a/enterprise/backend/test/metabase_enterprise/serialization/test_util.clj b/enterprise/backend/test/metabase_enterprise/serialization/test_util.clj
index 819835ed3fc..210b1cc9fc0 100644
--- a/enterprise/backend/test/metabase_enterprise/serialization/test_util.clj
+++ b/enterprise/backend/test/metabase_enterprise/serialization/test_util.clj
@@ -422,7 +422,20 @@
nil]
[:field
~'venues-pk-field-id
- {:join-alias "v"}]]}]}}}]]
+ {:join-alias "v"}]]}]}}}]
+ Card [{~'card-id-pivot-table :id}
+ {:table_id ~'table-id
+ :name "Pivot Table Card"
+ :collection_id ~'collection-id
+ :dataset_query {:type :query
+ :database ~'db-id
+ :query {:source-table ~'table-id
+ :aggregation [:sum [:field ~'latitude-field-id nil]]
+ :breakout [[:field ~'category-field-id nil]]}}
+ :visualization_settings
+ {:pivot_table.column_split {:columns [["field" ~'latitude-field-id nil]]
+ :rows [["field" ~'latitude-field-id nil]]
+ :values [["aggregation" 0]]}}}]]
(qp.store/with-store ~@body)))
;; Don't memoize as IDs change in each `with-world` context
--
GitLab