From d9301a17673ed13477a643426a68314cc6d176db Mon Sep 17 00:00:00 2001
From: Cam Saul <1455846+camsaul@users.noreply.github.com>
Date: Fri, 6 Oct 2023 18:26:56 -0700
Subject: [PATCH] Add missing models to `copy` command (#34412)
---
.../models/entity_id_test.clj | 4 +-
src/metabase/cmd/copy.clj | 198 +++++++++---------
src/metabase/util/jvm.clj | 4 +-
test/metabase/cmd/copy/h2_test.clj | 2 +-
test/metabase/cmd/copy_test.clj | 46 ++--
test/metabase/cmd/load_and_dump_test.clj | 2 +-
test/metabase/models/revision_test.clj | 89 ++++----
7 files changed, 177 insertions(+), 168 deletions(-)
diff --git a/enterprise/backend/test/metabase_enterprise/models/entity_id_test.clj b/enterprise/backend/test/metabase_enterprise/models/entity_id_test.clj
index 84d78345586..c64aa5983eb 100644
--- a/enterprise/backend/test/metabase_enterprise/models/entity_id_test.clj
+++ b/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 "
diff --git a/src/metabase/cmd/copy.clj b/src/metabase/cmd/copy.clj
index 7bd8152db7f..c32b6e36a55 100644
--- a/src/metabase/cmd/copy.clj
+++ b/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))
diff --git a/src/metabase/util/jvm.clj b/src/metabase/util/jvm.clj
index e225a4155c9..db0a0f3fdbd 100644
--- a/src/metabase/util/jvm.clj
+++ b/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
diff --git a/test/metabase/cmd/copy/h2_test.clj b/test/metabase/cmd/copy/h2_test.clj
index 8c3d7248762..ecff5f02709 100644
--- a/test/metabase/cmd/copy/h2_test.clj
+++ b/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"))))
diff --git a/test/metabase/cmd/copy_test.clj b/test/metabase/cmd/copy_test.clj
index 28473d445a8..9d7df9dd5cb 100644
--- a/test/metabase/cmd/copy_test.clj
+++ b/test/metabase/cmd/copy_test.clj
@@ -1,10 +1,11 @@
(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))))
diff --git a/test/metabase/cmd/load_and_dump_test.clj b/test/metabase/cmd/load_and_dump_test.clj
index af2fee19d1f..6463b20b176 100644
--- a/test/metabase/cmd/load_and_dump_test.clj
+++ b/test/metabase/cmd/load_and_dump_test.clj
@@ -1,4 +1,4 @@
-(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]
diff --git a/test/metabase/models/revision_test.clj b/test/metabase/models/revision_test.clj
index 7acd672a8f0..a33dca66cf0 100644
--- a/test/metabase/models/revision_test.clj
+++ b/test/metabase/models/revision_test.clj
@@ -1,4 +1,4 @@
-(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
--
GitLab