From 7d4f727df0d4261762d938c906fa992f82afe497 Mon Sep 17 00:00:00 2001
From: Ngoc Khuat <qn.khuat@gmail.com>
Date: Tue, 28 Mar 2023 17:02:42 +0700
Subject: [PATCH] `resolve-model, primary-key` handles toucan 2 model (#29559)
* use resolve-model with keyword check
* do the same for primary-key and fix a check in serialization
---
.clj-kondo/config.edn | 4 +++-
.../serialization/v2/backfill_ids.clj | 9 +++----
.../serialization/v2/seed_entity_ids.clj | 3 ++-
.../serialization/upsert_test.clj | 4 ++--
src/metabase/db/util.clj | 21 +++++++++++++++-
src/metabase/models/interface.clj | 3 ++-
src/metabase/models/revision.clj | 4 ++--
src/metabase/models/serialization.clj | 24 +++++++++----------
test/metabase/test.clj | 6 ++---
test/metabase/test/util.clj | 8 +++----
10 files changed, 55 insertions(+), 31 deletions(-)
diff --git a/.clj-kondo/config.edn b/.clj-kondo/config.edn
index dc2016d70d9..12266d75f68 100644
--- a/.clj-kondo/config.edn
+++ b/.clj-kondo/config.edn
@@ -156,7 +156,9 @@
toucan.db/transaction {:message "Use t2/with-transaction instead of toucan.db/transaction"}
toucan.db/with-call-counting {:message "Use t2/with-call-count instead of toucan.db/with-call-counting"}
toucan.db/execute! {:message "Use t2/query-one instead of toucan.db/execute!"}
- toucan.db/reducible-query {:message "Use mdb.query/reducible-query instead of toucan.db/reducible-query"}}
+ toucan.db/reducible-query {:message "Use mdb.query/reducible-query instead of toucan.db/reducible-query"}
+ toucan.db/resolve-model {:message "Use metabase.db.util/resolve-model instead of toucan.db/resolve-model"}
+ toucan.models/primary-key {:message "Use metabase.db.util/primary-key instead of toucan.models/primary-key"}}
:discouraged-namespace
{clojure.tools.logging {:message "Use metabase.util.log instead of clojure.tools.logging directly"}
diff --git a/enterprise/backend/src/metabase_enterprise/serialization/v2/backfill_ids.clj b/enterprise/backend/src/metabase_enterprise/serialization/v2/backfill_ids.clj
index 125f57dd1f7..a075ece19b3 100644
--- a/enterprise/backend/src/metabase_enterprise/serialization/v2/backfill_ids.clj
+++ b/enterprise/backend/src/metabase_enterprise/serialization/v2/backfill_ids.clj
@@ -6,11 +6,12 @@
so this should produce identical IDs on all platforms and JVM implementations."
(:require
[metabase-enterprise.serialization.v2.models :as serdes.models]
+ [metabase.db.util :as mdb.u]
+ [metabase.models.interface :as mi]
[metabase.models.serialization :as serdes]
[metabase.util :as u]
[metabase.util.i18n :refer [trs]]
[metabase.util.log :as log]
- [toucan.db :as db]
[toucan.models :as models]
[toucan2.core :as t2]))
@@ -18,7 +19,7 @@
"Updates all rows of a particular model to have `:entity_id` set, based on the [[serdes/identity-hash]]."
[model]
(let [missing (t2/select model :entity_id nil)
- pk (models/primary-key model)]
+ pk (mdb.u/primary-key model)]
(when (seq missing)
(log/info (trs "Backfilling entity_id for {0} rows of {1}" (pr-str (count missing)) (name model)))
(doseq [entity missing
@@ -27,7 +28,7 @@
(t2/update! model (get entity pk) {:entity_id eid})))))
(defn- has-entity-id? [model]
- (:entity_id (models/properties model)))
+ (::mi/entity-id (models/properties model)))
(defn backfill-ids
"Updates all rows of all models that are (a) serialized and (b) have `entity_id` columns to have the
@@ -35,6 +36,6 @@
row."
[]
(doseq [model-name (concat serdes.models/exported-models serdes.models/inlined-models)
- :let [model (db/resolve-model (symbol model-name))]
+ :let [model (mdb.u/resolve-model (symbol model-name))]
:when (has-entity-id? model)]
(backfill-ids-for model)))
diff --git a/enterprise/backend/src/metabase_enterprise/serialization/v2/seed_entity_ids.clj b/enterprise/backend/src/metabase_enterprise/serialization/v2/seed_entity_ids.clj
index e8ee10932ab..10f19d954f9 100644
--- a/enterprise/backend/src/metabase_enterprise/serialization/v2/seed_entity_ids.clj
+++ b/enterprise/backend/src/metabase_enterprise/serialization/v2/seed_entity_ids.clj
@@ -3,6 +3,7 @@
[clojure.string :as str]
[metabase.db :as mdb]
[metabase.db.connection :as mdb.connection]
+ [metabase.db.util :as mdb.u]
[metabase.models]
[metabase.models.serialization :as serdes]
[metabase.util :as u]
@@ -63,7 +64,7 @@
(defn- seed-entity-id-for-instance! [model instance]
(try
- (let [primary-key (models/primary-key model)
+ (let [primary-key (mdb.u/primary-key model)
pk-value (get instance primary-key)]
(when-not (some? pk-value)
(throw (ex-info (format "Missing value for primary key column %s" (pr-str primary-key))
diff --git a/enterprise/backend/test/metabase_enterprise/serialization/upsert_test.clj b/enterprise/backend/test/metabase_enterprise/serialization/upsert_test.clj
index 531002bfa26..5ca7a2a45a6 100644
--- a/enterprise/backend/test/metabase_enterprise/serialization/upsert_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/serialization/upsert_test.clj
@@ -3,12 +3,12 @@
[clojure.data :as data]
[clojure.test :refer :all]
[metabase-enterprise.serialization.upsert :as upsert]
+ [metabase.db.util :as mdb.u]
[metabase.models :refer [Card Collection Dashboard DashboardCard Database Field Metric NativeQuerySnippet
Pulse Segment Table User]]
[metabase.models.interface :as mi]
[metabase.test :as mt]
[metabase.util :as u]
- [toucan.models :as models]
[toucan2.core :as t2]))
(def ^:private same? (comp nil? second data/diff))
@@ -94,7 +94,7 @@
;; create an additional entity so we're sure whe get the right one
model [_ (dummy-entity dashboard model e1 1)]
model [{id :id} (dummy-entity dashboard model e2 2)]]
- (let [e (t2/select-one model (models/primary-key model) id)]
+ (let [e (t2/select-one model (mdb.u/primary-key model) id)]
;; make sure that all columns in identity-condition actually exist in the model
(is (= (set id-cond) (-> e
(select-keys id-cond)
diff --git a/src/metabase/db/util.clj b/src/metabase/db/util.clj
index 8c9044e62a7..f49911d68d4 100644
--- a/src/metabase/db/util.clj
+++ b/src/metabase/db/util.clj
@@ -5,8 +5,27 @@
[metabase.util.schema :as su]
[schema.core :as s]
[toucan.db :as db]
+ [toucan.models :as models]
[toucan2.core :as t2]))
+(defn primary-key
+ "Replacement of [[mdb.u/primary-key]], this is used to make the transition to toucan 2 easier.
+ In toucan2, every keyword can be a model so if `model` is a keyword, returns as is, otherwise calls [[mdb.u/primary-key]]."
+ [model]
+ (if (keyword? model)
+ (first (t2/primary-keys :m/card))
+ #_{:clj-kondo/ignore [:discouraged-var]}
+ (models/primary-key model)))
+
+(defn resolve-model
+ "Replacement of [[mb.models/resolve-model]], this is used to make the transition to toucan 2 easier.
+ In toucan2, every keyword can be a model so if `model` is a keyword, returns as is, otherwise calls [[toucan1.db/resolve-model]]."
+ [model]
+ (if (keyword? model)
+ model
+ #_{:clj-kondo/ignore [:discouraged-var]}
+ (db/resolve-model model)))
+
(defn join
"Convenience for generating a HoneySQL `JOIN` clause.
@@ -14,7 +33,7 @@
(mdb/join [FieldValues :field_id] [Field :id])
:active true)"
[[source-entity fk] [dest-entity pk]]
- {:left-join [(t2/table-name (db/resolve-model dest-entity))
+ {:left-join [(t2/table-name (resolve-model dest-entity))
[:= (db/qualify source-entity fk) (db/qualify dest-entity pk)]]})
(def ^:private NamespacedKeyword
diff --git a/src/metabase/models/interface.clj b/src/metabase/models/interface.clj
index d41977158ed..39c85623ff1 100644
--- a/src/metabase/models/interface.clj
+++ b/src/metabase/models/interface.clj
@@ -8,6 +8,7 @@
[clojure.spec.alpha :as s]
[clojure.walk :as walk]
[metabase.db.connection :as mdb.connection]
+ [metabase.db.util :as mdb.u]
[metabase.mbql.normalize :as mbql.normalize]
[metabase.mbql.schema :as mbql.s]
[metabase.models.dispatch :as models.dispatch]
@@ -478,7 +479,7 @@
(defn- check-perms-with-fn
([fn-symb read-or-write a-model object-id]
(or (current-user-has-root-permissions?)
- (check-perms-with-fn fn-symb read-or-write (t2/select-one a-model (models/primary-key a-model) object-id))))
+ (check-perms-with-fn fn-symb read-or-write (t2/select-one a-model (mdb.u/primary-key a-model) object-id))))
([fn-symb read-or-write object]
(and object
diff --git a/src/metabase/models/revision.clj b/src/metabase/models/revision.clj
index 7cc4bc25803..28219454641 100644
--- a/src/metabase/models/revision.clj
+++ b/src/metabase/models/revision.clj
@@ -1,12 +1,12 @@
(ns metabase.models.revision
(:require
[clojure.data :as data]
+ [metabase.db.util :as mdb.u]
[metabase.models.interface :as mi]
[metabase.models.revision.diff :refer [diff-string]]
[metabase.models.user :refer [User]]
[metabase.util :as u]
[metabase.util.i18n :refer [tru]]
- [toucan.db :as db]
[toucan.hydrate :refer [hydrate]]
[toucan.models :as models]
[toucan2.core :as t2]))
@@ -68,7 +68,7 @@
[{:keys [model], :as revision}]
;; in some cases (such as tests) we have 'fake' models that cannot be resolved normally; don't fail entirely in
;; those cases
- (let [model (u/ignore-exceptions (db/resolve-model (symbol model)))]
+ (let [model (u/ignore-exceptions (mdb.u/resolve-model (symbol model)))]
(cond-> revision
model (update :object (partial models/do-post-select model)))))
diff --git a/src/metabase/models/serialization.clj b/src/metabase/models/serialization.clj
index bb35e8d3d8e..638e62c3b6a 100644
--- a/src/metabase/models/serialization.clj
+++ b/src/metabase/models/serialization.clj
@@ -15,6 +15,7 @@
[clojure.set :as set]
[clojure.string :as str]
[medley.core :as m]
+ [metabase.db.util :as mdb.u]
[metabase.mbql.normalize :as mbql.normalize]
[metabase.mbql.schema :as mbql.s]
[metabase.mbql.util :as mbql.u]
@@ -25,7 +26,6 @@
[metabase.util.log :as log]
[toucan.db :as db]
[toucan.hydrate :refer [hydrate]]
- [toucan.models :as models]
[toucan2.core :as t2])
(:refer-clojure :exclude [descendants]))
@@ -92,8 +92,8 @@
(defn infer-self-path
"Returns `{:model \"ModelName\" :id \"id-string\"}`"
[model-name entity]
- (let [model (db/resolve-model (symbol model-name))
- pk (models/primary-key model)]
+ (let [model (mdb.u/resolve-model (symbol model-name))
+ pk (mdb.u/primary-key model)]
{:model model-name
:id (or (entity-id model-name entity)
(some-> (get entity pk) model identity-hash))}))
@@ -197,8 +197,8 @@
Returns the Clojure map."
[model-name entity]
- (let [model (db/resolve-model (symbol model-name))
- pk (models/primary-key model)]
+ (let [model (mdb.u/resolve-model (symbol model-name))
+ pk (mdb.u/primary-key model)]
(-> (into {} entity)
(assoc :serdes/meta (generate-path model-name entity))
(dissoc pk :updated_at))))
@@ -246,7 +246,7 @@
(defmethod load-find-local :default [path]
(let [{id :id model-name :model} (last path)
- model (db/resolve-model (symbol model-name))]
+ model (mdb.u/resolve-model (symbol model-name))]
(when model
(lookup-by-id model id))))
@@ -300,8 +300,8 @@
(fn [model _ _] model))
(defmethod load-update! :default [model-name ingested local]
- (let [model (db/resolve-model (symbol model-name))
- pk (models/primary-key model)
+ (let [model (mdb.u/resolve-model (symbol model-name))
+ pk (mdb.u/primary-key model)
id (get local pk)]
(log/tracef "Upserting %s %d: old %s new %s" model-name id (pr-str local) (pr-str ingested))
(t2/update! model id ingested)
@@ -445,8 +445,8 @@
[id model]
(when id
(let [model-name (name model)
- model (db/resolve-model (symbol model-name))
- entity (t2/select-one model (models/primary-key model) id)
+ model (mdb.u/resolve-model (symbol model-name))
+ entity (t2/select-one model (mdb.u/primary-key model) id)
path (mapv :id (generate-path model-name entity))]
(if (= (count path) 1)
(first path)
@@ -466,13 +466,13 @@
[eid model]
(when eid
(let [model-name (name model)
- model (db/resolve-model (symbol model-name))
+ model (mdb.u/resolve-model (symbol model-name))
eid (if (vector? eid)
(last eid)
eid)
entity (lookup-by-id model eid)]
(if entity
- (get entity (models/primary-key model))
+ (get entity (mdb.u/primary-key model))
(throw (ex-info "Could not find foreign key target - bad serdes-dependencies or other serialization error"
{:entity_id eid :model (name model)}))))))
diff --git a/test/metabase/test.clj b/test/metabase/test.clj
index b681934beab..7d2fb15406e 100644
--- a/test/metabase/test.clj
+++ b/test/metabase/test.clj
@@ -15,6 +15,7 @@
[medley.core :as m]
[metabase.actions.test-util :as actions.test-util]
[metabase.config :as config]
+ [metabase.db.util :as mdb.u]
[metabase.driver :as driver]
[metabase.driver.sql-jdbc.test-util :as sql-jdbc.tu]
[metabase.driver.sql.query-processor-test-util :as sql.qp-test-util]
@@ -48,7 +49,6 @@
[pjstadig.humane-test-output :as humane-test-output]
[potemkin :as p]
[toucan.db :as db]
- [toucan.models :as models]
[toucan.util.test :as tt]
[toucan2.core :as t2]))
@@ -316,7 +316,7 @@
temp-admin (first (t2/insert-returning-instances! User (merge (with-temp-defaults User)
attributes
{:is_superuser true})))
- primary-key (models/primary-key User)]
+ primary-key (mdb.u/primary-key User)]
(try
(thunk temp-admin)
(finally
@@ -420,7 +420,7 @@
(fn [toucan-model]
(hawk.init/assert-tests-are-not-initializing (list 'object-defaults (symbol (name toucan-model))))
(initialize/initialize-if-needed! :db)
- (db/resolve-model toucan-model))))
+ (mdb.u/resolve-model toucan-model))))
(defmacro disable-flaky-test-when-running-driver-tests-in-ci
"Only run `body` when we're not running driver tests in CI (i.e., `DRIVERS` and `CI` are both not set). Perfect for
diff --git a/test/metabase/test/util.clj b/test/metabase/test/util.clj
index f79dc664973..fd0523ffdf9 100644
--- a/test/metabase/test/util.clj
+++ b/test/metabase/test/util.clj
@@ -13,6 +13,7 @@
[hawk.parallel]
[java-time :as t]
[metabase.db.query :as mdb.query]
+ [metabase.db.util :as mdb.u]
[metabase.models
:refer [Card
Collection
@@ -59,7 +60,6 @@
[metabase.util :as u]
[metabase.util.files :as u.files]
[methodical.core :as methodical]
- [toucan.db :as db]
[toucan.models :as models]
[toucan.util.test :as tt]
[toucan2.core :as t2]
@@ -474,7 +474,7 @@
(hawk.parallel/assert-test-is-not-parallel "with-temp-vals-in-db")
;; use low-level `query` and `execute` functions here, because Toucan `select` and `update` functions tend to do
;; things like add columns like `common_name` that don't actually exist, causing subsequent update to fail
- (let [model (db/resolve-model model)
+ (let [model (mdb.u/resolve-model model)
[original-column->value] (mdb.query/query {:select (keys column->temp-value)
:from [(t2/table-name model)]
:where [:= :id (u/the-id object-or-id)]})]
@@ -621,7 +621,7 @@
(hawk.parallel/assert-test-is-not-parallel "with-model-cleanup")
(initialize/initialize-if-needed! :db)
(let [model->old-max-id (into {} (for [model models]
- [model (:max-id (t2/select-one [model [(keyword (str "%max." (name (models/primary-key model))))
+ [model (:max-id (t2/select-one [model [(keyword (str "%max." (name (mdb.u/primary-key model))))
:max-id]]))]))]
(try
(testing (str "\n" (pr-str (cons 'with-model-cleanup (map name models))) "\n")
@@ -631,7 +631,7 @@
;; might not have an old max ID if this is the first time the macro is used in this test run.
:let [old-max-id (or (get model->old-max-id model)
0)
- max-id-condition [:> (models/primary-key model) old-max-id]
+ max-id-condition [:> (mdb.u/primary-key model) old-max-id]
additional-conditions (with-model-cleanup-additional-conditions model)]]
(t2/query-one
{:delete-from (t2/table-name model)
--
GitLab