From 5ad50f50d73e2351bd01a76b92fe6a774b80c142 Mon Sep 17 00:00:00 2001
From: Braden Shepherdson <Braden.Shepherdson@gmail.com>
Date: Mon, 11 Jul 2022 12:30:29 -0400
Subject: [PATCH] Serdes v2 for Cards (#23803)
---
.../serialization/v2/models.clj | 3 +-
.../serialization/v2/extract_test.clj | 92 +++++++++++++++----
.../serialization/v2/yaml_test.clj | 27 +++++-
src/metabase/models/card.clj | 55 +++++++++++
src/metabase/models/serialization/base.clj | 20 ++--
test/metabase/test/generate.clj | 6 +-
6 files changed, 173 insertions(+), 30 deletions(-)
diff --git a/enterprise/backend/src/metabase_enterprise/serialization/v2/models.clj b/enterprise/backend/src/metabase_enterprise/serialization/v2/models.clj
index 11ebe6e4808..6a79f894f91 100644
--- a/enterprise/backend/src/metabase_enterprise/serialization/v2/models.clj
+++ b/enterprise/backend/src/metabase_enterprise/serialization/v2/models.clj
@@ -2,7 +2,8 @@
(def exported-models
"The list of models which are exported by serialization. Used for production code and by tests."
- ["Collection"
+ ["Card"
+ "Collection"
"Database"
"Field"
"Setting"
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 f5edd4fca58..9b500dd9e9c 100644
--- a/enterprise/backend/test/metabase_enterprise/serialization/v2/extract_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/serialization/v2/extract_test.clj
@@ -2,12 +2,20 @@
(:require [clojure.test :refer :all]
[metabase-enterprise.serialization.test-util :as ts]
[metabase-enterprise.serialization.v2.extract :as extract]
- [metabase.models :refer [Collection User]]
+ [metabase.models :refer [Card Collection Database Table User]]
[metabase.models.serialization.base :as serdes.base]))
(defn- select-one [model-name where]
(first (into [] (serdes.base/raw-reducible-query model-name {:where where}))))
+(defn- by-model [model-name extraction]
+ (->> extraction
+ (into [])
+ (map (comp last :serdes/meta))
+ (filter #(= model-name (:model %)))
+ (map :id)
+ set))
+
(deftest fundamentals-test
(ts/with-empty-h2-app-db
(ts/with-temp-dpc [Collection [{coll-id :id
@@ -52,20 +60,72 @@
(is (= "mark@direstrai.ts" (:personal_owner_id ser)))))
(testing "overall extraction returns the expected set"
- (letfn [(collections [extraction] (->> extraction
- (into [])
- (map (comp last :serdes/meta))
- (filter #(= "Collection" (:model %)))
- (map :id)
- set))]
- (testing "no user specified"
- (is (= #{coll-eid child-eid}
- (collections (extract/extract-metabase nil)))))
+ (testing "no user specified"
+ (is (= #{coll-eid child-eid}
+ (by-model "Collection" (extract/extract-metabase nil)))))
+
+ (testing "valid user specified"
+ (is (= #{coll-eid child-eid pc-eid}
+ (by-model "Collection" (extract/extract-metabase {:user mark-id})))))
+
+ (testing "invalid user specified"
+ (is (= #{coll-eid child-eid}
+ (by-model "Collection" (extract/extract-metabase {:user 218921})))))))))
- (testing "valid user specified"
- (is (= #{coll-eid child-eid pc-eid}
- (collections (extract/extract-metabase {:user mark-id})))))
+(deftest dashboard-and-cards-test
+ (ts/with-empty-h2-app-db
+ (ts/with-temp-dpc [Collection [{coll-id :id
+ coll-eid :entity_id
+ coll-slug :slug} {:name "Some Collection"}]
+ User [{mark-id :id} {:first_name "Mark"
+ :last_name "Knopfler"
+ :email "mark@direstrai.ts"}]
+ Database [{db-id :id} {:name "My Database"}]
+ Table [{no-schema-id :id} {:name "Schemaless Table" :db_id db-id}]
+ Table [{schema-id :id} {:name "Schema'd Table"
+ :db_id db-id
+ :schema "PUBLIC"}]
+ Card [{c1-id :id
+ c1-eid :entity_id} {:name "Some Question"
+ :database_id db-id
+ :table_id no-schema-id
+ :collection_id coll-id
+ :creator_id mark-id
+ :dataset_query "{\"json\": \"string values\"}"}]
+ Card [{c2-id :id
+ c2-eid :entity_id} {:name "Second Question"
+ :database_id db-id
+ :table_id schema-id
+ :collection_id coll-id
+ :creator_id mark-id}]]
+ (testing "table and database are extracted as [db schema table] triples"
+ (let [ser (serdes.base/extract-one "Card" {} (select-one "Card" [:= :id c1-id]))]
+ (is (= {:serdes/meta [{:model "Card" :id c1-eid}]
+ :table ["My Database" nil "Schemaless Table"]
+ :creator_id "mark@direstrai.ts"
+ :collection_id coll-eid
+ :dataset_query "{\"json\": \"string values\"}"} ; Undecoded, still a string.
+ (select-keys ser [:serdes/meta :table :creator_id :collection_id :dataset_query])))
+ (is (not (contains? ser :id)))
+
+ (testing "cards depend on their Table and Collection"
+ (is (= #{[{:model "Database" :id "My Database"}
+ {:model "Table" :id "Schemaless Table"}]
+ [{:model "Collection" :id coll-eid}]}
+ (set (serdes.base/serdes-dependencies ser))))))
+
+ (let [ser (serdes.base/extract-one "Card" {} (select-one "Card" [:= :id c2-id]))]
+ (is (= {:serdes/meta [{:model "Card" :id c2-eid}]
+ :table ["My Database" "PUBLIC" "Schema'd Table"]
+ :creator_id "mark@direstrai.ts"
+ :collection_id coll-eid
+ :dataset_query "{}"} ; Undecoded, still a string.
+ (select-keys ser [:serdes/meta :table :creator_id :collection_id :dataset_query])))
+ (is (not (contains? ser :id)))
- (testing "invalid user specified"
- (is (= #{coll-eid child-eid}
- (collections (extract/extract-metabase {:user 218921}))))))))))
+ (testing "cards depend on their Table and Collection"
+ (is (= #{[{:model "Database" :id "My Database"}
+ {:model "Schema" :id "PUBLIC"}
+ {:model "Table" :id "Schema'd Table"}]
+ [{:model "Collection" :id coll-eid}]}
+ (set (serdes.base/serdes-dependencies ser))))))))))
diff --git a/enterprise/backend/test/metabase_enterprise/serialization/v2/yaml_test.clj b/enterprise/backend/test/metabase_enterprise/serialization/v2/yaml_test.clj
index 031bd911677..5079237a6bd 100644
--- a/enterprise/backend/test/metabase_enterprise/serialization/v2/yaml_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/serialization/v2/yaml_test.clj
@@ -12,6 +12,7 @@
[metabase.test.generate :as test-gen]
[metabase.util.date-2 :as u.date]
[reifyhealth.specmonstah.core :as rs]
+ [toucan.db :as db]
[yaml.core :as yaml]))
(defn- dir->file-set [dir]
@@ -95,6 +96,9 @@
(assoc :serdes/meta (mapv #(dissoc % :label) abs-path)))
(ingest/ingest-one ingestable abs-path))))))))
+(defn- random-key [prefix n]
+ (keyword (str prefix (rand-int n))))
+
(deftest e2e-storage-ingestion-test
(ts/with-random-dump-dir [dump-dir "serdesv2-"]
(ts/with-empty-h2-app-db
@@ -104,7 +108,14 @@
[10 {:refs {:db_id db}}]))
:field (into [] (for [n (range 100)
:let [table (keyword (str "t" n))]]
- [10 {:refs {:table_id table}}]))})
+ [10 {:refs {:table_id table}}]))
+ :core-user [[10]]
+ :card [[100 {:refs (let [db (rand-int 10)
+ t (rand-int 10)]
+ {:database_id (keyword (str "db" db))
+ :table_id (keyword (str "t" (+ t (* 10 db))))
+ :collection_id (random-key "coll" 100)
+ :creator_id (random-key "u" 10)})}]]})
(let [extraction (into [] (extract/extract-metabase {}))
entities (reduce (fn [m entity]
(update m (-> entity :serdes/meta last :model)
@@ -162,6 +173,20 @@
(update :updated_at u.date/format))
(yaml/from-file (io/file dump-dir "Database" db "Table" table "Field" (str name ".yaml")))))))
+ (testing "for cards"
+ (is (= 100 (count (dir->file-set (io/file dump-dir "Card")))))
+ (doseq [{[db-name schema table] :table
+ :keys [collection_id creator_id entity_id]
+ :as card} (get entities "Card")
+ :let [filename (str entity_id ".yaml")
+ db (db/select-one 'Database :name db-name)
+ table (db/select-one 'Table :db_id (:id db) :name table :schema schema)]]
+ (is (= (-> card
+ (dissoc :serdes/meta)
+ (update :created_at u.date/format)
+ (update :updated_at u.date/format))
+ (yaml/from-file (io/file dump-dir "Card" filename))))))
+
(testing "for settings"
(is (= (into {} (for [{:keys [key value]} (get entities "Setting")]
[key value]))
diff --git a/src/metabase/models/card.clj b/src/metabase/models/card.clj
index 045c1a2fbf4..59c6cf207ff 100644
--- a/src/metabase/models/card.clj
+++ b/src/metabase/models/card.clj
@@ -13,6 +13,7 @@
[metabase.models.permissions :as perms]
[metabase.models.query :as query]
[metabase.models.revision :as revision]
+ [metabase.models.serialization.base :as serdes.base]
[metabase.models.serialization.hash :as serdes.hash]
[metabase.moderation :as moderation]
[metabase.plugins.classloader :as classloader]
@@ -325,3 +326,57 @@
serdes.hash/IdentityHashable
{:identity-hash-fields (constantly [:name (serdes.hash/hydrated-hash :collection)])})
+
+;;; ------------------------------------------------- Serialization --------------------------------------------------
+(defmethod serdes.base/extract-query "Card" [_ {:keys [user]}]
+ (serdes.base/raw-reducible-query
+ "Card"
+ {:select [:card.*]
+ :from [[:report_card :card]]
+ :left-join [[:collection :coll] [:= :coll.id :card.collection_id]]
+ :where (if user
+ [:or [:= :coll.personal_owner_id user] [:is :coll.personal_owner_id nil]]
+ [:is :coll.personal_owner_id nil])}))
+
+(defmethod serdes.base/extract-one "Card"
+ [_ _ {:keys [table_id database_id collection_id creator_id] :as card}]
+ ;; Cards have :table_id, :database_id, :collection_id, :creator_id that need conversion.
+ ;; :table_id and :database_id are extracted as just :table [database_name schema table_name].
+ ;; :collection_id is extracted as its entity_id or identity-hash.
+ ;; :creator_id as the user's email
+ (let [db-name (db/select-one-field :name 'Database :id database_id)
+ {:keys [schema name]} (db/select-one 'Table :id table_id)
+ coll (db/select-one 'Collection :id collection_id)
+ {collection-eid :id} (serdes.base/infer-self-path "Collection" coll)
+ email (db/select-one-field :email 'User :id creator_id)]
+ (-> (serdes.base/extract-one-basics "Card" card)
+ (dissoc :table_id :database_id)
+ (assoc :table [db-name schema name]
+ :collection_id collection-eid
+ :creator_id email))))
+
+(defmethod serdes.base/load-xform "Card"
+ [{[db-name schema table-name] :table
+ email :creator_id
+ collection-eid :collection_id
+ :as card}]
+ (let [db-id (db/select-one-id 'Database :name db-name)
+ table-id (db/select-one-id 'Table :database_id db-id :schema schema :name table-name)
+ coll-id (serdes.base/lookup-by-id 'Collection collection-eid)
+ user-id (db/select-one-id 'User :email email)]
+ (-> card
+ serdes.base/load-xform-basics
+ (dissoc :table)
+ (assoc :database_id db-id
+ :table_id table-id
+ :collection_id coll-id
+ :creator_id user-id))))
+
+(defmethod serdes.base/serdes-dependencies "Card"
+ [{[db-name schema table-name] :table
+ :keys [collection_id]}]
+ ;; The Table implicitly depends on the Database.
+ [(filterv some? [{:model "Database" :id db-name}
+ (when schema {:model "Schema" :id schema})
+ {:model "Table" :id table-name}])
+ [{:model "Collection" :id collection_id}]])
diff --git a/src/metabase/models/serialization/base.clj b/src/metabase/models/serialization/base.clj
index 99912438296..ce9b7618448 100644
--- a/src/metabase/models/serialization/base.clj
+++ b/src/metabase/models/serialization/base.clj
@@ -194,6 +194,14 @@
(eduction (map (partial extract-one model opts))
(extract-query model opts)))
+(defn- model-name->table
+ "The model name is not necessarily the table name. This pulls the table name from the Toucan model."
+ [model-name]
+ (-> model-name
+ symbol
+ db/resolve-model
+ :table))
+
(defn raw-reducible-query
"Helper for calling Toucan's raw [[db/reducible-query]]. With just the model name, fetches everything. You can filter
with a HoneySQL map like `{:where [:= :archived true]}`.
@@ -202,19 +210,11 @@
([model-name]
(raw-reducible-query model-name nil))
([model-name honeysql-form]
- (db/reducible-query (merge {:select [:*] :from [(symbol model-name)]}
+ (db/reducible-query (merge {:select [:*] :from [(model-name->table model-name)]}
honeysql-form))))
-(defn- model-name->table
- "The model name is not necessarily the table name. This pulls the table name from the Toucan model."
- [model-name]
- (-> model-name
- symbol
- db/resolve-model
- :table))
-
(defmethod extract-query :default [model-name _]
- (raw-reducible-query (model-name->table model-name)))
+ (raw-reducible-query model-name))
(defn extract-one-basics
"A helper for writing [[extract-one]] implementations. It takes care of the basics:
diff --git a/test/metabase/test/generate.clj b/test/metabase/test/generate.clj
index 8f6d4f6a0c5..9e2b88c256d 100644
--- a/test/metabase/test/generate.clj
+++ b/test/metabase/test/generate.clj
@@ -187,8 +187,10 @@
:card {:prefix :c
:spec ::card
:insert! {:model Card}
- :relations {:creator_id [:core-user :id]
- :database_id [:database :id]}}
+ :relations {:creator_id [:core-user :id]
+ :database_id [:database :id]
+ :table_id [:table :id]
+ :collection_id [:collection :id]}}
:dashboard {:prefix :d
:spec ::dashboard
:insert! {:model Dashboard}
--
GitLab