Add missing models to `copy` command (#34412)

enterprise/backend/test/metabase_enterprise/models/entity_id_test.clj

@@ -44,7 +44,6 @@
     :model/CardBookmark
     :model/CollectionBookmark
     :model/DashboardBookmark
-    :metabase.models.collection.root/RootCollection
     :model/CollectionPermissionGraphRevision
     :model/DashboardCardSeries
     :model/LoginHistory
@@ -66,7 +65,6 @@
     :model/QueryCache
     :model/QueryExecution
     :model/Revision
-    :model/FakedCard
     :model/Secret
     :model/Session
     :model/TablePrivileges
@@ -79,6 +77,8 @@
 
 (deftest ^:parallel comprehensive-entity-id-test
   (doseq [model (->> (v2.seed-entity-ids/toucan-models)
+                     (remove (fn [model]
+                               (not= (namespace model) "model")))
                      (remove entities-not-exported)
                      (remove entities-external-name))]
     (testing (format (str "Model %s should either: have the ::mi/entity-id property, or be explicitly listed as having "

src/metabase/cmd/copy.clj

@@ -4,58 +4,17 @@
   data from an application database to any empty application database for all combinations of supported application
   database types."
   (:require
+   [clojure.java.classpath :as classpath]
    [clojure.java.jdbc :as jdbc]
+   [clojure.string :as str]
+   [clojure.tools.namespace.find :as ns.find]
    [honey.sql :as sql]
+   [metabase.config :as config]
    [metabase.db.connection :as mdb.connection]
    #_{:clj-kondo/ignore [:deprecated-namespace]}
    [metabase.db.data-migrations :refer [DataMigrations]]
    [metabase.db.setup :as mdb.setup]
-   [metabase.models
-    :refer [Action
-            Activity
-            ApplicationPermissionsRevision
-            BookmarkOrdering
-            Card
-            CardBookmark
-            Collection
-            CollectionBookmark
-            CollectionPermissionGraphRevision
-            Dashboard
-            DashboardBookmark
-            DashboardCard
-            DashboardCardSeries
-            Database
-            Dimension
-            Field
-            FieldValues
-            HTTPAction
-            ImplicitAction
-            LoginHistory
-            Metric
-            MetricImportantField
-            ModerationReview
-            NativeQuerySnippet
-            ParameterCard
-            Permissions
-            PermissionsGroup
-            PermissionsGroupMembership
-            PermissionsRevision
-            PersistedInfo
-            Pulse
-            PulseCard
-            PulseChannel
-            PulseChannelRecipient
-            QueryAction
-            Revision
-            Secret
-            Segment
-            Session
-            Setting
-            Table
-            Timeline
-            TimelineEvent
-            User
-            ViewLog]]
+   [metabase.plugins.classloader :as classloader]
    [metabase.util :as u]
    [metabase.util.i18n :refer [trs]]
    [metabase.util.log :as log]
@@ -89,54 +48,63 @@
 (def entities
   "Entities in the order they should be serialized/deserialized. This is done so we make sure that we load load
   instances of entities before others that might depend on them, e.g. `Databases` before `Tables` before `Fields`."
-  [Database
-   User
-   Setting
-   Table
-   Field
-   FieldValues
-   Segment
-   Metric
-   MetricImportantField
-   ModerationReview
-   Revision
-   ViewLog
-   Session
-   Collection
-   CollectionPermissionGraphRevision
-   Dashboard
-   Card
-   CardBookmark
-   DashboardBookmark
-   CollectionBookmark
-   BookmarkOrdering
-   DashboardCard
-   DashboardCardSeries
-   Activity
-   Pulse
-   PulseCard
-   PulseChannel
-   PulseChannelRecipient
-   PermissionsGroup
-   PermissionsGroupMembership
-   Permissions
-   PermissionsRevision
-   PersistedInfo
-   ApplicationPermissionsRevision
-   Dimension
-   NativeQuerySnippet
-   LoginHistory
-   Timeline
-   TimelineEvent
-   Secret
-   ParameterCard
-   Action
-   ImplicitAction
-   HTTPAction
-   QueryAction
+  (concat
+   [:model/Database
+    :model/User
+    :model/Setting
+    :model/Table
+    :model/Field
+    :model/FieldValues
+    :model/Segment
+    :model/Metric
+    :model/MetricImportantField
+    :model/ModerationReview
+    :model/Revision
+    :model/ViewLog
+    :model/Session
+    :model/Collection
+    :model/CollectionPermissionGraphRevision
+    :model/Dashboard
+    :model/Card
+    :model/CardBookmark
+    :model/DashboardBookmark
+    :model/CollectionBookmark
+    :model/BookmarkOrdering
+    :model/DashboardCard
+    :model/DashboardCardSeries
+    :model/Activity
+    :model/Pulse
+    :model/PulseCard
+    :model/PulseChannel
+    :model/PulseChannelRecipient
+    :model/PermissionsGroup
+    :model/PermissionsGroupMembership
+    :model/Permissions
+    :model/PermissionsRevision
+    :model/PersistedInfo
+    :model/ApplicationPermissionsRevision
+    :model/Dimension
+    :model/NativeQuerySnippet
+    :model/LoginHistory
+    :model/Timeline
+    :model/TimelineEvent
+    :model/Secret
+    :model/ParameterCard
+    :model/Action
+    :model/ImplicitAction
+    :model/HTTPAction
+    :model/QueryAction
+    :model/DashboardTab
+    :model/ModelIndex
+    :model/ModelIndexValue
+    ;; 48+
+    :model/TablePrivileges]
+   (when config/ee-available?
+     [:model/GroupTableAccessPolicy
+      :model/ConnectionImpersonation])
    ;; migrate the list of finished DataMigrations as the very last thing (all models to copy over should be listed
    ;; above this line)
-   DataMigrations])
+   [DataMigrations]))
 
 (defn- objects->colums+values
   "Given a sequence of objects/rows fetched from the H2 DB, return a the `columns` that should be used in the `INSERT`
@@ -346,7 +314,27 @@
 
 (def ^:private entities-without-autoinc-ids
   "Entities that do NOT use an auto incrementing ID column."
-  #{Setting Session DataMigrations ImplicitAction HTTPAction QueryAction})
+  #{:model/Setting
+    :model/Session
+    :model/DataMigrations
+    :model/ImplicitAction
+    :model/HTTPAction
+    :model/QueryAction
+    :model/ModelIndexValue
+    :model/TablePrivileges})
+
+(defmulti ^:private postgres-id-sequence-name
+  {:arglists '([model])}
+  keyword)
+
+(defmethod postgres-id-sequence-name :default
+  [model]
+  (str (name (t2/table-name model)) "_id_seq"))
+
+;;; we changed the table name to `sandboxes` but never updated the underlying ID sequences or constraint names.
+(defmethod postgres-id-sequence-name :model/GroupTableAccessPolicy
+  [_model]
+  "group_table_access_policy_id_seq")
 
 (defmulti ^:private update-sequence-values!
   {:arglists '([db-type data-source])}
@@ -357,21 +345,26 @@
 
 ;; Update the sequence nextvals.
 (defmethod update-sequence-values! :postgres
-  [_ data-source]
+  [_db-type data-source]
   #_{:clj-kondo/ignore [:discouraged-var]}
   (jdbc/with-db-transaction [target-db-conn {:datasource data-source}]
     (step (trs "Setting Postgres sequence ids to proper values...")
-      (doseq [e     entities
-              :when (not (contains? entities-without-autoinc-ids e))
-              :let  [table-name (name (t2/table-name e))
-                     seq-name   (str table-name "_id_seq")
+      (doseq [model entities
+              :when (not (contains? entities-without-autoinc-ids model))
+              :let  [table-name (name (t2/table-name model))
+                     seq-name   (postgres-id-sequence-name model)
                      sql        (format "SELECT setval('%s', COALESCE((SELECT MAX(id) FROM %s), 1), true) as val"
                                         seq-name (name table-name))]]
-        (jdbc/db-query-with-resultset target-db-conn [sql] :val)))))
+        (try
+          (jdbc/db-query-with-resultset target-db-conn [sql] :val)
+          (catch Throwable e
+            (throw (ex-info (format "Error updating sequence values for %s: %s" model (ex-message e))
+                            {:model model}
+                            e))))))))
 
 
 (defmethod update-sequence-values! :h2
-  [_ data-source]
+  [_db-type data-source]
   #_{:clj-kondo/ignore [:discouraged-var]}
   (jdbc/with-db-transaction [target-db-conn {:datasource data-source}]
     (step (trs "Setting H2 sequence ids to proper values...")
@@ -389,6 +382,11 @@
    source-data-source :- javax.sql.DataSource
    target-db-type     :- (s/enum :h2 :postgres :mysql)
    target-data-source :- javax.sql.DataSource]
+  ;; make sure the entire system is loaded before running this test, to make sure we account for all the models.
+  (doseq [ns-symb (ns.find/find-namespaces (classpath/system-classpath))
+          :when   (and (str/starts-with? ns-symb "metabase")
+                       (not (str/includes? ns-symb "test")))]
+    (classloader/require ns-symb))
   ;; make sure the source database is up-do-date
   (step (trs "Set up {0} source database and run migrations..." (name source-db-type))
     (mdb.setup/setup-db! source-db-type source-data-source true))

src/metabase/util/jvm.clj

@@ -272,8 +272,8 @@
                         initialization."}
  metabase-namespace-symbols
   (vec (sort (for [ns-symb (ns.find/find-namespaces (classpath/system-classpath))
-                   :when   (and (.startsWith (name ns-symb) "metabase.")
-                                (not (.contains (name ns-symb) "test")))]
+                   :when   (and (str/starts-with? ns-symb "metabase.")
+                                (not (str/includes? ns-symb "test")))]
                ns-symb))))
 
 (defn deref-with-timeout

test/metabase/cmd/copy/h2_test.clj

@@ -4,7 +4,7 @@
    [metabase.cmd.copy.h2 :as copy.h2]
    [metabase.db.data-source :as mdb.data-source]))
 
-(deftest h2-data-source-test
+(deftest ^:parallel h2-data-source-test
   (testing "works without file: schema"
     (is (= (mdb.data-source/raw-connection-string->DataSource "jdbc:h2:file:/path/to/metabase.db")
            (copy.h2/h2-data-source "/path/to/metabase.db"))))

test/metabase/cmd/copy_test.clj

 (ns metabase.cmd.copy-test
   (:require
+   [clojure.java.classpath :as classpath]
+   [clojure.string :as str]
    [clojure.test :refer :all]
+   [clojure.tools.namespace.find :as ns.find]
    [metabase.cmd.copy :as copy]
-   [metabase.db.util :as mdb.u]
-   [metabase.plugins.classloader :as classloader]
-   [metabase.util :as u]))
+   [metabase.plugins.classloader :as classloader]))
 
 (deftest ^:parallel sql-for-selecting-instances-from-source-db-test
   (is (= "SELECT * FROM metabase_field ORDER BY id ASC"
@@ -22,19 +23,28 @@
                 [{:id 1, :engine "h2", :details "{:db \"metabase.db\"}"}
                  {:id 2, :engine "postgres", :details "{:db \"metabase\"}"}])))))))
 
+(def ^:private models-to-exclude
+  "Models that should *not* be migrated in `load-from-h2`."
+  #{:model/TaskHistory
+    :model/Query
+    :model/QueryCache
+    :model/QueryExecution
+    :model/CardFavorite
+    :model/DashboardFavorite})
+
+(defn- all-model-names []
+  (into (sorted-set)
+        (comp (filter #(= (namespace %) "model"))
+              (remove models-to-exclude))
+        (descendants :metabase/model)))
+
 (deftest ^:paralell all-models-accounted-for-test
-  ;; This fetches the `metabase.cmd.load-from-h2/entities` and compares it all existing entities
-  (let [migrated-model-names (set (map :name copy/entities))
-        ;; Models that should *not* be migrated in `load-from-h2`.
-        models-to-exclude    #{"TaskHistory" "Query" "QueryCache" "QueryExecution" "CardFavorite" "DashboardFavorite"}
-        all-model-names      (set (for [ns       u/metabase-namespace-symbols
-                                        :when    (or (re-find #"^metabase\.models\." (name ns))
-                                                     (= (name ns) "metabase.db.data-migrations"))
-                                        :when    (not (re-find #"test" (name ns)))
-                                        [_ varr] (do (classloader/require ns)
-                                                     (ns-interns ns))
-                                        :let     [{model-name :name, :as model} (var-get varr)]
-                                        :when    (and (mdb.u/toucan-model? model)
-                                                      (not (contains? models-to-exclude model-name)))]
-                                    model-name))]
-    (is (= all-model-names migrated-model-names))))
+  ;; make sure the entire system is loaded before running this test, to make sure we account for all the models.
+  (doseq [ns-symb (ns.find/find-namespaces (classpath/system-classpath))
+          :when   (and (str/starts-with? ns-symb "metabase")
+                       (not (str/includes? ns-symb "test")))]
+    (classloader/require ns-symb))
+  (doseq [model (all-model-names)
+          :let  [copy-models (set copy/entities)]]
+    (is (contains? copy-models model)
+        (format "%s should be added to %s, or to %s" model `copy/entities `models-to-exclude))))

test/metabase/cmd/load_and_dump_test.clj

-(ns metabase.cmd.load-and-dump-test
+(ns ^:mb/once metabase.cmd.load-and-dump-test
   (:require
    [clojure.java.io :as io]
    [clojure.test :refer :all]

test/metabase/models/revision_test.clj

-(ns metabase.models.revision-test
+(ns ^:mb/once metabase.models.revision-test
   (:require
    [clojure.test :refer :all]
    [metabase.models.card :refer [Card]]
@@ -15,8 +15,8 @@
 (def ^:private reverted-to
   (atom nil))
 
-(methodical/defmethod t2/table-name :model/FakedCard [_model] :report_card)
-(derive :model/FakedCard :metabase/model)
+(methodical/defmethod t2/table-name ::FakedCard [_model] :report_card)
+(derive ::FakedCard :metabase/model)
 
 (defn- do-with-model-i18n-strs! [thunk]
   (with-redefs [revision.diff/model-str->i18n-str (fn [model-str]
@@ -29,26 +29,26 @@
                                                       "FakeCard"      "FakeCard"))]
     (thunk)))
 
-(defmethod revision/serialize-instance :model/FakedCard
+(defmethod revision/serialize-instance ::FakedCard
   [_model _id obj]
   (into {} (assoc obj :serialized true)))
 
-(defmethod revision/revert-to-revision! :model/FakedCard
+(defmethod revision/revert-to-revision! ::FakedCard
   [_model _id _user-id serialized-instance]
   (reset! reverted-to (dissoc serialized-instance :serialized)))
 
-(defmethod revision/diff-map :model/FakedCard
+(defmethod revision/diff-map ::FakedCard
   [_model o1 o2]
   {:o1 (when o1 (into {} o1)), :o2 (when o2 (into {} o2))})
 
-(defmethod revision/diff-strings :model/FakedCard
+(defmethod revision/diff-strings ::FakedCard
   [_model o1 o2]
   (when o1
     [(str "BEFORE=" (into {} o1) ",AFTER=" (into {} o2))]))
 
 (defn- push-fake-revision! [card-id & {:keys [message] :as object}]
   (revision/push-revision!
-    :entity   :model/FakedCard
+    :entity   ::FakedCard
     :id       card-id
     :user-id  (mt/user->id :rasta)
     :object   (dissoc object :message)
@@ -104,7 +104,7 @@
   (testing "Test that a newly created Card doesn't have any revisions"
     (t2.with-temp/with-temp [Card {card-id :id}]
       (is (= []
-             (revision/revisions :model/FakedCard card-id))))))
+             (revision/revisions ::FakedCard card-id))))))
 
 (deftest add-revision-test
   (testing "Test that we can add a revision"
@@ -114,11 +114,11 @@
                Revision
                {:model        "FakedCard"
                 :user_id      (mt/user->id :rasta)
-                :object       (mi/instance :model/FakedCard {:name "Tips Created by Day", :serialized true})
+                :object       (mi/instance 'FakedCard {:name "Tips Created by Day", :serialized true})
                 :is_reversion false
                 :is_creation  false
                 :message      "yay!"})]
-             (for [revision (revision/revisions :model/FakedCard card-id)]
+             (for [revision (revision/revisions ::FakedCard card-id)]
                (dissoc revision :timestamp :id :model_id)))))))
 
 (deftest sorting-test
@@ -126,24 +126,25 @@
     (t2.with-temp/with-temp [Card {card-id :id}]
       (push-fake-revision! card-id, :name "Tips Created by Day")
       (push-fake-revision! card-id, :name "Spots Created by Day")
-      (is (= [(mi/instance
-               Revision
-               {:model        "FakedCard"
-                :user_id      (mt/user->id :rasta)
-                :object       (mi/instance :model/FakedCard {:name "Spots Created by Day", :serialized true})
-                :is_reversion false
-                :is_creation  false
-                :message      nil})
-              (mi/instance
-               Revision
-               {:model        "FakedCard"
-                :user_id      (mt/user->id :rasta)
-                :object       (mi/instance :model/FakedCard {:name "Tips Created by Day", :serialized true})
-                :is_reversion false
-                :is_creation  false
-                :message      nil})]
-             (->> (revision/revisions :model/FakedCard card-id)
-                  (map #(dissoc % :timestamp :id :model_id))))))))
+      (testing `revision/revisions
+        (is (= [(mi/instance
+                 Revision
+                 {:model        "FakedCard"
+                  :user_id      (mt/user->id :rasta)
+                  :object       (mi/instance 'FakedCard {:name "Spots Created by Day", :serialized true})
+                  :is_reversion false
+                  :is_creation  false
+                  :message      nil})
+                (mi/instance
+                 Revision
+                 {:model        "FakedCard"
+                  :user_id      (mt/user->id :rasta)
+                  :object       (mi/instance 'FakedCard {:name "Tips Created by Day", :serialized true})
+                  :is_reversion false
+                  :is_creation  false
+                  :message      nil})]
+               (->> (revision/revisions ::FakedCard card-id)
+                    (map #(dissoc % :timestamp :id :model_id)))))))))
 
 (deftest delete-old-revisions-test
   (testing "Check that old revisions get deleted"
@@ -152,7 +153,7 @@
       (dorun (doseq [i (range (inc revision/max-revisions))]
                (push-fake-revision! card-id, :name (format "Tips Created by Day %d" i))))
       (is (= revision/max-revisions
-             (count (revision/revisions :model/FakedCard card-id)))))))
+             (count (revision/revisions ::FakedCard card-id)))))))
 
 (deftest do-not-record-if-object-is-not-changed-test
   (testing "Check that we don't record a revision if the object hasn't changed"
@@ -161,15 +162,15 @@
                            (push-fake-revision! card-id, :name (format "Tips Created by Day %s" x)))]
         (testing "first revision should be recorded"
           (new-revision 1)
-          (is (= 1 (count (revision/revisions :model/FakedCard card-id)))))
+          (is (= 1 (count (revision/revisions ::FakedCard card-id)))))
 
         (testing "repeatedly push reivisions with the same object shouldn't create new revision"
           (dorun (repeatedly 5 #(new-revision 1)))
-          (is (= 1 (count (revision/revisions :model/FakedCard card-id)))))
+          (is (= 1 (count (revision/revisions ::FakedCard card-id)))))
 
         (testing "push a revision with different object should create new revision"
           (new-revision 2)
-          (is (= 2 (count (revision/revisions :model/FakedCard card-id))))))))
+          (is (= 2 (count (revision/revisions ::FakedCard card-id))))))))
 
   (testing "Check that we don't record revision on dashboard if it has a filter"
     (t2.with-temp/with-temp
@@ -214,19 +215,19 @@
                                      :o2 {:name "Modified Name", :serialized true}}
               :has_multiple_changes false
               :description          "BEFORE={:name \"Initial Name\", :serialized true},AFTER={:name \"Modified Name\", :serialized true}."}
-             (let [revisions (revision/revisions :model/FakedCard card-id)]
+             (let [revisions (revision/revisions ::FakedCard card-id)]
                (assert (= 2 (count revisions)))
-               (-> (revision/add-revision-details :model/FakedCard (first revisions) (last revisions))
+               (-> (revision/add-revision-details ::FakedCard (first revisions) (last revisions))
                    (dissoc :timestamp :id :model_id)
                    mt/derecordize))))))
 
   (testing "test that we return a description even when there is no change between revision"
     (is (= "created a revision with no change."
-           (str (:description (revision/add-revision-details :model/FakedCard {:name "Apple"} {:name "Apple"}))))))
+           (str (:description (revision/add-revision-details ::FakedCard {:name "Apple"} {:name "Apple"}))))))
 
   (testing "that we return a descrtiopn when there is no previous revision"
     (is (= "modified this."
-           (str (:description (revision/add-revision-details :model/FakedCard {:name "Apple"} nil)))))))
+           (str (:description (revision/add-revision-details ::FakedCard {:name "Apple"} nil)))))))
 
 (deftest revisions+details-test
   (testing "Check that revisions+details pulls in user info and adds description"
@@ -242,7 +243,7 @@
                                        :o2 {:name "Tips Created by Day", :serialized true}}
                 :has_multiple_changes false
                 :description          "modified this."})]
-             (->> (revision/revisions+details :model/FakedCard card-id)
+             (->> (revision/revisions+details ::FakedCard card-id)
                   (map #(dissoc % :timestamp :id :model_id))
                   (map #(update % :description str))))))))
 
@@ -272,7 +273,7 @@
                                        :o2 {:name "Tips Created by Day", :serialized true}}
                 :has_multiple_changes false
                 :description          "modified this."})]
-             (->> (revision/revisions+details :model/FakedCard card-id)
+             (->> (revision/revisions+details ::FakedCard card-id)
                   (map #(dissoc % :timestamp :id :model_id))
                   (map #(update % :description str))))))))
 
@@ -282,8 +283,8 @@
   (testing "Check that revert defers to revert-to-revision!"
     (t2.with-temp/with-temp [Card {card-id :id}]
       (push-fake-revision! card-id, :name "Tips Created by Day")
-      (let [[{revision-id :id}] (revision/revisions :model/FakedCard card-id)]
-        (revision/revert! :entity :model/FakedCard, :id card-id, :user-id (mt/user->id :rasta), :revision-id revision-id)
+      (let [[{revision-id :id}] (revision/revisions ::FakedCard card-id)]
+        (revision/revert! :entity ::FakedCard, :id card-id, :user-id (mt/user->id :rasta), :revision-id revision-id)
         (is (= {:name "Tips Created by Day"}
                @reverted-to))))))
 
@@ -304,8 +305,8 @@
     (t2.with-temp/with-temp [Card {card-id :id}]
       (push-fake-revision! card-id, :name "Tips Created by Day")
       (push-fake-revision! card-id, :name "Spots Created by Day")
-      (let [[_ {old-revision-id :id}] (revision/revisions :model/FakedCard card-id)]
-        (revision/revert! :entity :model/FakedCard, :id card-id, :user-id (mt/user->id :rasta), :revision-id old-revision-id)
+      (let [[_ {old-revision-id :id}] (revision/revisions ::FakedCard card-id)]
+        (revision/revert! :entity ::FakedCard, :id card-id, :user-id (mt/user->id :rasta), :revision-id old-revision-id)
         (is (partial=
              [(mi/instance
                Revision
@@ -331,7 +332,7 @@
                 :is_reversion false
                 :is_creation  false
                 :message      nil})]
-             (->> (revision/revisions :model/FakedCard card-id)
+             (->> (revision/revisions ::FakedCard card-id)
                   (map #(dissoc % :timestamp :id :model_id)))))))))
 
 (deftest generic-models-revision-title+description-test