`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

@@ -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"}

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)))

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))

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)

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

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

src/metabase/models/revision.clj

 (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)))))
 

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)}))))))
 

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

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)