From 8b8669a26c0de31746f31fb4fb6c9267460d0927 Mon Sep 17 00:00:00 2001 From: Ngoc Khuat <qn.khuat@gmail.com> Date: Tue, 28 Mar 2023 01:29:23 +0700 Subject: [PATCH] Convert simple-* to toucan2 (#29427) * (db/simple-insert! Model opts) -> (t2/insert! (t2/table-name Model) opts) * (db/simple-delete! model opts) => (t2/delete! (t2/table-name) opts) * (db/simple-insert-many! Model opts) => (t2/insert! (t2/table-name Model) opts) --- .clj-kondo/config.edn | 4 +- .../serialization/cmd_test.clj | 7 +- src/metabase/api/metric.clj | 5 +- src/metabase/api/table.clj | 3 +- src/metabase/models/pulse_channel.clj | 2 +- src/metabase/models/setting.clj | 3 +- src/metabase/models/setting/cache.clj | 2 +- src/metabase/models/task_history.clj | 7 +- src/metabase/models/user.clj | 5 +- .../middleware/cache_backend/db.clj | 4 +- test/metabase/api/revision_test.clj | 3 +- test/metabase/db/schema_migrations_test.clj | 529 +++++++++--------- .../db/schema_migrations_test/impl.clj | 2 +- test/metabase/events/revision_test.clj | 3 +- test/metabase/models/setting/cache_test.clj | 2 +- test/metabase/models/setting_test.clj | 4 +- .../server/middleware/session_test.clj | 2 +- test/metabase/sync/field_values_test.clj | 5 +- test/metabase/test.clj | 2 +- 19 files changed, 293 insertions(+), 301 deletions(-) diff --git a/.clj-kondo/config.edn b/.clj-kondo/config.edn index 2d0df2257c5..8d087a329cd 100644 --- a/.clj-kondo/config.edn +++ b/.clj-kondo/config.edn @@ -143,6 +143,9 @@ toucan.db/select-field->field {:message "Use t2/select-fn->fn instead of toucan.db/select-field->field"} toucan.db/select-field->id {:message "Use t2/select-fn->pk instead of toucan.db/select-field->id"} toucan.db/select-id->field {:message "Use t2/select-pk->field instead of toucan.db/select-id->field"} + toucan.db/simple-insert! {:message "Use t2/insert-returning-pks! with table name instead of toucan.db/simple-insert!"} + toucan.db/simple-insert-many! {:message "Use t2/insert-returning-pks! with table name instead of toucan.db/simple-insert-many!"} + toucan.db/simple-delete! {:message "Use t2/delete! with table name instead of toucan.db/simple-delete!"} toucan.db/delete! {:message "Use t2/delete! instead of toucan.db/delete!"} toucan.db/insert! {:message "Use t2/insert-returning-instances! instead of toucan.db/insert!"} toucan.db/update! {:message "Use t2/update! instead of toucan.db/update!"} @@ -154,7 +157,6 @@ 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"}} - :discouraged-namespace {clojure.tools.logging {:message "Use metabase.util.log instead of clojure.tools.logging directly"} metabase.util.jvm {:message "All of metabase.util.jvm is re-exported from metabase.util; prefer that"}} diff --git a/enterprise/backend/test/metabase_enterprise/serialization/cmd_test.clj b/enterprise/backend/test/metabase_enterprise/serialization/cmd_test.clj index c5778e0b6a1..221e3c81b83 100644 --- a/enterprise/backend/test/metabase_enterprise/serialization/cmd_test.clj +++ b/enterprise/backend/test/metabase_enterprise/serialization/cmd_test.clj @@ -11,7 +11,6 @@ [metabase.util :as u] [metabase.util.log :as log] [metabase.util.yaml :as yaml] - [toucan.db :as db] [toucan2.core :as t2]) (:import (java.util UUID))) @@ -30,7 +29,7 @@ ;; already does what we need) (mt/with-empty-h2-app-db ;; create a single dummy User to own a Card and a Database for it to reference - (let [user (db/simple-insert! User + (let [user (t2/insert! (t2/table-name User) :email "nobody@nowhere.com" :first_name (mt/random-name) :last_name (mt/random-name) @@ -38,14 +37,14 @@ :date_joined :%now :is_active true :is_superuser true) - db (db/simple-insert! Database + db (t2/insert! (t2/table-name Database) :name "Test Database" :engine "h2" :details "{}" :created_at :%now :updated_at :%now)] ;; then the card itself - (db/simple-insert! Card + (t2/insert! (t2/table-name Card) :name "Single Card" :display "Single Card" :database_id (u/the-id db) diff --git a/src/metabase/api/metric.clj b/src/metabase/api/metric.clj index c06a5cc5e32..5d91c15a188 100644 --- a/src/metabase/api/metric.clj +++ b/src/metabase/api/metric.clj @@ -7,10 +7,9 @@ [metabase.api.query-description :as api.qd] [metabase.events :as events] [metabase.mbql.normalize :as mbql.normalize] + [metabase.models :refer [Metric MetricImportantField Table]] [metabase.models.interface :as mi] - [metabase.models.metric :as metric :refer [Metric]] [metabase.models.revision :as revision] - [metabase.models.table :refer [Table]] [metabase.related :as related] [metabase.util :as u] [metabase.util.i18n :refer [trs]] @@ -131,7 +130,7 @@ ;; delete old fields as needed (when (seq fields-to-remove) - (db/simple-delete! 'MetricImportantField {:metric_id id, :field_id [:in fields-to-remove]})) + (t2/delete! (t2/table-name MetricImportantField) {:metric_id id, :field_id [:in fields-to-remove]})) ;; add new fields as needed (db/insert-many! 'MetricImportantField (for [field-id fields-to-add] {:metric_id id, :field_id field-id})) diff --git a/src/metabase/api/table.clj b/src/metabase/api/table.clj index 736cefc43dc..ec2b197a537 100644 --- a/src/metabase/api/table.clj +++ b/src/metabase/api/table.clj @@ -24,7 +24,6 @@ [metabase.util.malli.schema :as ms] [metabase.util.schema :as su] [schema.core :as s] - [toucan.db :as db] [toucan.hydrate :refer [hydrate]] [toucan2.core :as t2])) @@ -481,7 +480,7 @@ {id ms/PositiveInt} (api/write-check (t2/select-one Table :id id)) (when-let [field-ids (t2/select-pks-set Field :table_id id)] - (db/simple-delete! FieldValues :field_id [:in field-ids])) + (t2/delete! (t2/table-name FieldValues) :field_id [:in field-ids])) {:status :success}) (api/defendpoint GET "/:id/related" diff --git a/src/metabase/models/pulse_channel.clj b/src/metabase/models/pulse_channel.clj index 6aa95907ff4..6a0712a82b4 100644 --- a/src/metabase/models/pulse_channel.clj +++ b/src/metabase/models/pulse_channel.clj @@ -281,7 +281,7 @@ (let [vs (map #(assoc {:pulse_channel_id id} :user_id %) recipients+)] (db/insert-many! PulseChannelRecipient vs))) (when (seq recipients-) - (db/simple-delete! PulseChannelRecipient + (t2/delete! (t2/table-name PulseChannelRecipient) :pulse_channel_id id :user_id [:in recipients-])))) diff --git a/src/metabase/models/setting.clj b/src/metabase/models/setting.clj index fd0c0845598..9c4d79eaee6 100644 --- a/src/metabase/models/setting.clj +++ b/src/metabase/models/setting.clj @@ -90,7 +90,6 @@ [metabase.util.i18n :refer [deferred-trs deferred-tru trs tru]] [metabase.util.log :as log] [schema.core :as s] - [toucan.db :as db] [toucan.models :as models] [toucan2.core :as t2]) (:import @@ -639,7 +638,7 @@ ;; write to DB (cond (nil? new-value) - (db/simple-delete! Setting :key setting-name) + (t2/delete! (t2/table-name Setting) :key setting-name) ;; if there's a value in the cache then the row already exists in the DB; update that (contains? (setting.cache/cache) setting-name) diff --git a/src/metabase/models/setting/cache.clj b/src/metabase/models/setting/cache.clj index 85416cf86ff..e3f96d453e8 100644 --- a/src/metabase/models/setting/cache.clj +++ b/src/metabase/models/setting/cache.clj @@ -84,7 +84,7 @@ ;; and ignore that Exception if one is thrown. (try ;; Use `simple-insert!` because we do *not* want to trigger pre-insert behavior, such as encrypting `:value` - (db/simple-insert! 'Setting :key settings-last-updated-key, :value current-timestamp-as-string-honeysql) + (t2/insert! (t2/table-name (t2/resolve-model 'Setting)) :key settings-last-updated-key, :value current-timestamp-as-string-honeysql) (catch java.sql.SQLException e ;; go ahead and log the Exception anyway on the off chance that it *wasn't* just a race condition issue (log/error (trs "Error updating Settings last updated value: {0}" diff --git a/src/metabase/models/task_history.clj b/src/metabase/models/task_history.clj index 39fb66e177f..3f23978a0dd 100644 --- a/src/metabase/models/task_history.clj +++ b/src/metabase/models/task_history.clj @@ -15,7 +15,6 @@ [metabase.util.log :as log] [metabase.util.schema :as su] [schema.core :as s] - [toucan.db :as db] [toucan.models :as models] [toucan2.core :as t2])) @@ -37,9 +36,9 @@ ;; ensures we'll have a good amount of history for debugging/troubleshooting, but not grow too large and fill the ;; disk. (when-let [clean-before-date (t2/select-one-fn :ended_at TaskHistory {:limit 1 - :offset num-rows-to-keep - :order-by [[:ended_at :desc]]})] - (db/simple-delete! TaskHistory :ended_at [:<= clean-before-date]))) + :offset num-rows-to-keep + :order-by [[:ended_at :desc]]})] + (t2/delete! (t2/table-name TaskHistory) :ended_at [:<= clean-before-date]))) ;;; Permissions to read or write Task. If `advanced-permissions` is enabled it requires superusers or non-admins with ;;; monitoring permissions, Otherwise it requires superusers. diff --git a/src/metabase/models/user.clj b/src/metabase/models/user.clj index d24924fffd0..46d2a2c10b7 100644 --- a/src/metabase/models/user.clj +++ b/src/metabase/models/user.clj @@ -22,7 +22,6 @@ [metabase.util.password :as u.password] [metabase.util.schema :as su] [schema.core :as schema] - [toucan.db :as db] [toucan.models :as models] [toucan2.core :as t2]) (:import @@ -105,7 +104,7 @@ ;; stack overflow of calls between the two. TODO - could we fix this issue by using a `post-delete` method? (and (not superuser?) in-admin-group?) - (db/simple-delete! PermissionsGroupMembership + (t2/delete! (t2/table-name PermissionsGroupMembership) :group_id (u/the-id (perms-group/admin)) :user_id id)))) ;; make sure email and locale are valid if set @@ -356,7 +355,7 @@ by [[pre-insert]] or [[pre-update]])" [user-id password] ;; when changing/resetting the password, kill any existing sessions - (db/simple-delete! Session :user_id user-id) + (t2/delete! (t2/table-name Session) :user_id user-id) ;; NOTE: any password change expires the password reset token (t2/update! User user-id {:password password diff --git a/src/metabase/query_processor/middleware/cache_backend/db.clj b/src/metabase/query_processor/middleware/cache_backend/db.clj index d8fe0bbc6a2..a005b7b0341 100644 --- a/src/metabase/query_processor/middleware/cache_backend/db.clj +++ b/src/metabase/query_processor/middleware/cache_backend/db.clj @@ -76,8 +76,8 @@ {:pre [(number? max-age-seconds)]} (log/tracef "Purging old cache entries.") (try - (db/simple-delete! QueryCache - :updated_at [:<= (seconds-ago max-age-seconds)]) + (t2/delete! (t2/table-name QueryCache) + :updated_at [:<= (seconds-ago max-age-seconds)]) (catch Throwable e (log/error e (trs "Error purging old cache entries")))) nil) diff --git a/test/metabase/api/revision_test.clj b/test/metabase/api/revision_test.clj index e8deff718ec..c5a9164d2b3 100644 --- a/test/metabase/api/revision_test.clj +++ b/test/metabase/api/revision_test.clj @@ -10,7 +10,6 @@ [metabase.test.data.users :as test.users] [metabase.test.fixtures :as fixtures] [metabase.util :as u] - [toucan.db :as db] [toucan.util.test :as tt] [toucan2.core :as t2])) @@ -128,7 +127,7 @@ :col 0))] (is (=? {:id id} (create-dashboard-revision! dash false :rasta))) - (is (true? (db/simple-delete! DashboardCard, :id (:id dashcard))))) + (is (pos? (t2/delete! (t2/table-name DashboardCard) :id (:id dashcard))))) (is (=? {:id id} (create-dashboard-revision! dash false :rasta))) (testing "Revert to the previous revision, allowed because rasta has permissions on parent collection" diff --git a/test/metabase/db/schema_migrations_test.clj b/test/metabase/db/schema_migrations_test.clj index ff28354e1be..7b42749c079 100644 --- a/test/metabase/db/schema_migrations_test.clj +++ b/test/metabase/db/schema_migrations_test.clj @@ -39,7 +39,6 @@ [metabase.test.fixtures :as fixtures] [metabase.test.util.random :as tu.random] [metabase.util :as u] - [toucan.db :as db] [toucan2.core :as t2] [toucan2.execute :as t2.execute]) (:import @@ -75,11 +74,11 @@ (testing "Migration 165: add `database_position` to Field" (impl/test-migrations 165 [migrate!] ;; create a Database with a Table with 2 Fields - (db/simple-insert! Database {:name "DB", :engine "h2", :created_at :%now, :updated_at :%now}) - (db/simple-insert! Table {:name "Table", :db_id 1, :created_at :%now, :updated_at :%now, :active true}) + (t2/insert! (t2/table-name Database) {:name "DB", :engine "h2", :created_at :%now, :updated_at :%now}) + (t2/insert! (t2/table-name Table) {:name "Table", :db_id 1, :created_at :%now, :updated_at :%now, :active true}) (let [mock-field {:table_id 1, :created_at :%now, :updated_at :%now, :base_type "type/Text", :database_type "VARCHAR"}] - (db/simple-insert! Field (assoc mock-field :name "Field 1")) - (db/simple-insert! Field (assoc mock-field :name "Field 2"))) + (t2/insert! (t2/table-name Field) (assoc mock-field :name "Field 1")) + (t2/insert! (t2/table-name Field) (assoc mock-field :name "Field 2"))) (testing "sanity check: Fields should not have a `:database_position` column yet" (is (not (contains? (t2/select-one Field :id 1) :database_position)))) ;; now run migration 165 @@ -93,14 +92,14 @@ (defn- create-raw-user! "create a user but skip pre and post insert steps" [email] - (db/simple-insert! User - :email email - :first_name (tu.random/random-name) - :last_name (tu.random/random-name) - :password (str (UUID/randomUUID)) - :date_joined :%now - :is_active true - :is_superuser false)) + (first (t2/insert-returning-instances! (t2/table-name User) + :email email + :first_name (tu.random/random-name) + :last_name (tu.random/random-name) + :password (str (UUID/randomUUID)) + :date_joined :%now + :is_active true + :is_superuser false))) (deftest email-lowercasing-test (testing "Migration 268-272: basic lowercasing `email` in `core_user`" @@ -120,11 +119,11 @@ (impl/test-migrations [283 296] [migrate!] ;; by name hoists results into a map by name so diffs are easier to read than sets. (let [by-name #(into {} (map (juxt :name identity)) %) - db-id (db/simple-insert! Database {:name "DB", :engine "h2", :created_at :%now, :updated_at :%now}) - table-id (db/simple-insert! Table {:name "Table", :db_id db-id, :created_at :%now, :updated_at :%now, :active true})] - ;; Using [[db/simple-insert-many!]] instead of [[db/insert-many!]] so we don't run into the field type validation + db-id (first (t2/insert-returning-pks! (t2/table-name Database) {:name "DB", :engine "h2", :created_at :%now, :updated_at :%now})) + table-id (first (t2/insert-returning-pks! (t2/table-name Table) {:name "Table", :db_id db-id, :created_at :%now, :updated_at :%now, :active true}))] + ;; Using insert with table-name, so we don't run into the field type validation ;; added in 38 - (db/simple-insert-many! Field + (t2/insert! (t2/table-name Field) (for [field [{:base_type :type/Text :semantic_type :type/Address :name "address" @@ -349,16 +348,16 @@ ;; we're using `simple-insert!` here instead of `with-temp` because Toucan `post-insert` for the DB will ;; normally try to add it to the magic Permissions Groups which don't exist yet. They're added in later ;; migrations - (let [db (db/simple-insert! Database {:name "Legacy BigQuery driver DB" - :engine "bigquery" - :details (json/generate-string {:service-account-json "{\"fake_key\": 14}"}) - :created_at :%now - :updated_at :%now})] + (let [db-id (first (t2/insert-returning-pks! (t2/table-name Database) {:name "Legacy BigQuery driver DB" + :engine "bigquery" + :details (json/generate-string {:service-account-json "{\"fake_key\": 14}"}) + :created_at :%now + :updated_at :%now}))] (migrate!) (is (= [{:engine "bigquery-cloud-sdk"}] - (mdb.query/query {:select [:engine], :from [:metabase_database], :where [:= :id (u/the-id db)]})))) + (mdb.query/query {:select [:engine], :from [:metabase_database], :where [:= :id db-id]})))) (finally - (db/simple-delete! Database :name "Legacy BigQuery driver DB")))))) + (t2/delete! (t2/table-name Database) :name "Legacy BigQuery driver DB")))))) (deftest create-root-permissions-entry-for-admin-test (testing "Migration v0.43.00-006: Add root permissions entry for 'Administrators' magic group" @@ -371,8 +370,8 @@ (is (integer? admin-group-id)) (when existing-entry? (t2/query-one {:insert-into :permissions - :values [{:object "/" - :group_id admin-group-id}]})) + :values [{:object "/" + :group_id admin-group-id}]})) (migrate!) (is (= [{:object "/"}] (mdb.query/query {:select [:object] @@ -385,15 +384,15 @@ (testing (format "With existing data migration? %s" (pr-str with-existing-data-migration?)) (impl/test-migrations "v43.00-007" [migrate!] (t2/query-one {:insert-into :metabase_database - :values [{:name "My DB" - :engine "h2" - :created_at :%now - :updated_at :%now - :details "{}"}]}) + :values [{:name "My DB" + :engine "h2" + :created_at :%now + :updated_at :%now + :details "{}"}]}) (when with-existing-data-migration? (t2/query-one {:insert-into :data_migrations - :values [{:id "add-databases-to-magic-permissions-groups" - :timestamp :%now}]})) + :values [{:id "add-databases-to-magic-permissions-groups" + :timestamp :%now}]})) (migrate!) (is (= (if with-existing-data-migration? [] @@ -407,8 +406,8 @@ (testing "Migration v43.00-008: migrate legacy `-site-url` Setting to `site-url`; remove trailing slashes (#4123, #4188, #20402)" (impl/test-migrations ["v43.00-008"] [migrate!] (t2/query-one {:insert-into :setting - :values [{:key "-site-url" - :value "http://localhost:3000/"}]}) + :values [{:key "-site-url" + :value "http://localhost:3000/"}]}) (migrate!) (is (= [{:key "site-url", :value "http://localhost:3000"}] (mdb.query/query {:select [:*], :from [:setting], :where [:= :key "site-url"]})))))) @@ -417,16 +416,16 @@ (testing "Migration v43.00-009: ensure `site-url` Setting starts with a protocol (#20403)" (impl/test-migrations ["v43.00-009"] [migrate!] (t2/query-one {:insert-into :setting - :values [{:key "site-url" - :value "localhost:3000"}]}) + :values [{:key "site-url" + :value "localhost:3000"}]}) (migrate!) (is (= [{:key "site-url", :value "http://localhost:3000"}] (mdb.query/query {:select [:*], :from [:setting], :where [:= :key "site-url"]})))))) (defn- add-legacy-data-migration-entry! [migration-name] (t2/query-one {:insert-into :data_migrations - :values [{:id migration-name - :timestamp :%now}]})) + :values [{:id migration-name + :timestamp :%now}]})) (defn- add-migrated-collections-data-migration-entry! [] (add-legacy-data-migration-entry! "add-migrated-collections")) @@ -435,54 +434,54 @@ (testing "Migrations v43.00-014 - v43.00-019" (letfn [(create-user! [] (t2/query-one {:insert-into :core_user - :values [{:first_name "Cam" - :last_name "Era" - :email "cam@era.com" - :password "abc123" - :date_joined :%now}]}))] + :values [{:first_name "Cam" + :last_name "Era" + :email "cam@era.com" + :password "abc123" + :date_joined :%now}]}))] (doseq [{:keys [model collection-name create-instance!]} [{:model Dashboard :collection-name "Migrated Dashboards" :create-instance! (fn [] (create-user!) (t2/query-one {:insert-into :report_dashboard - :values [{:name "My Dashboard" - :created_at :%now - :updated_at :%now - :creator_id 1 - :parameters "[]" - :collection_id nil}]}))} + :values [{:name "My Dashboard" + :created_at :%now + :updated_at :%now + :creator_id 1 + :parameters "[]" + :collection_id nil}]}))} {:model Pulse :collection-name "Migrated Pulses" :create-instance! (fn [] (create-user!) (t2/query-one {:insert-into :pulse - :values [{:name "My Pulse" - :created_at :%now - :updated_at :%now - :creator_id 1 - :parameters "[]" - :collection_id nil}]}))} + :values [{:name "My Pulse" + :created_at :%now + :updated_at :%now + :creator_id 1 + :parameters "[]" + :collection_id nil}]}))} {:model Card :collection-name "Migrated Questions" :create-instance! (fn [] (create-user!) (t2/query-one {:insert-into :metabase_database - :values [{:name "My DB" - :engine "h2" - :details "{}" - :created_at :%now - :updated_at :%now}]}) + :values [{:name "My DB" + :engine "h2" + :details "{}" + :created_at :%now + :updated_at :%now}]}) (t2/query-one {:insert-into :report_card - :values [{:name "My Saved Question" - :created_at :%now - :updated_at :%now - :creator_id 1 - :display "table" - :dataset_query "{}" - :visualization_settings "{}" - :database_id 1 - :collection_id nil}]}))}] + :values [{:name "My Saved Question" + :created_at :%now + :updated_at :%now + :creator_id 1 + :display "table" + :dataset_query "{}" + :visualization_settings "{}" + :database_id 1 + :collection_id nil}]}))}] :let [table-name-keyword (t2/table-name model)]] (testing (format "create %s Collection for %s in the Root Collection" @@ -522,7 +521,7 @@ (impl/test-migrations ["v43.00-014" "v43.00-019"] [migrate!] (create-instance!) (t2/query-one {:insert-into :collection - :values [{:name collection-name, :slug "existing_collection", :color "#abc123"}]}) + :values [{:name collection-name, :slug "existing_collection", :color "#abc123"}]}) (migrate!) (is (= [{:name collection-name, :slug "existing_collection"}] (collections))) @@ -559,8 +558,8 @@ (testing "entry already exists: don't create an entry" (impl/test-migrations ["v43.00-020" "v43.00-021"] [migrate!] (t2/query-one {:insert-into :permissions - :values [{:object "/collection/root/" - :group_id (all-users-group-id)}]}) + :values [{:object "/collection/root/" + :group_id (all-users-group-id)}]}) (migrate!) (is (= [{:object "/collection/root/"}] (all-user-perms)))))))) @@ -569,20 +568,20 @@ (testing "Migration v43.00-029: clear password and password_salt for LDAP users" (impl/test-migrations ["v43.00-029"] [migrate!] (t2/query-one {:insert-into :core_user - :values [{:first_name "Cam" - :last_name "Era" - :email "cam@era.com" - :date_joined :%now - :password "password" - :password_salt "and pepper" - :ldap_auth false} - {:first_name "LDAP Cam" - :last_name "Era" - :email "ldap_cam@era.com" - :date_joined :%now - :password "password" - :password_salt "and pepper" - :ldap_auth true}]}) + :values [{:first_name "Cam" + :last_name "Era" + :email "cam@era.com" + :date_joined :%now + :password "password" + :password_salt "and pepper" + :ldap_auth false} + {:first_name "LDAP Cam" + :last_name "Era" + :email "ldap_cam@era.com" + :date_joined :%now + :password "password" + :password_salt "and pepper" + :ldap_auth true}]}) (migrate!) (is (= [{:first_name "Cam", :password "password", :password_salt "and pepper", :ldap_auth false} {:first_name "LDAP Cam", :password nil, :password_salt nil, :ldap_auth true}] @@ -594,11 +593,11 @@ (testing "Migration v43.00-042: grant download permissions to All Users permissions group" (impl/test-migrations ["v43.00-042" "v43.00-043"] [migrate!] (t2/query-one {:insert-into :metabase_database - :values [{:name "My DB" - :engine "h2" - :created_at :%now - :updated_at :%now - :details "{}"}]}) + :values [{:name "My DB" + :engine "h2" + :created_at :%now + :updated_at :%now + :details "{}"}]}) (migrate!) (is (= [{:object "/collection/root/"} {:object "/download/db/1/"}] (mdb.query/query {:select [:p.object] @@ -631,21 +630,21 @@ (testing "Migration v44.00-022: Add parameters to report_card" (impl/test-migrations ["v44.00-022" "v44.00-024"] [migrate!] (let [user-id - (db/simple-insert! User {:first_name "Howard" - :last_name "Hughes" - :email "howard@aircraft.com" - :password "superstrong" - :date_joined :%now}) - database-id (db/simple-insert! Database {:name "DB", :engine "h2", :created_at :%now, :updated_at :%now}) - card-id (db/simple-insert! Card {:name "My Saved Question" - :created_at :%now - :updated_at :%now - :creator_id user-id - :display "table" - :dataset_query "{}" - :visualization_settings "{}" - :database_id database-id - :collection_id nil})] + (first (t2/insert-returning-pks! (t2/table-name User) {:first_name "Howard" + :last_name "Hughes" + :email "howard@aircraft.com" + :password "superstrong" + :date_joined :%now})) + database-id (first (t2/insert-returning-pks! (t2/table-name Database) {:name "DB", :engine "h2", :created_at :%now, :updated_at :%now})) + card-id (first (t2/insert-returning-pks! (t2/table-name Card) {:name "My Saved Question" + :created_at :%now + :updated_at :%now + :creator_id user-id + :display "table" + :dataset_query "{}" + :visualization_settings "{}" + :database_id database-id + :collection_id nil}))] (migrate!) (is (= nil (:parameters (first (t2/select (t2/table-name Card) {:where [:= :id card-id]}))))))))) @@ -654,23 +653,23 @@ (testing "Migration v44.00-024: Add parameter_mappings to cards" (impl/test-migrations ["v44.00-024" "v44.00-026"] [migrate!] (let [user-id - (db/simple-insert! User {:first_name "Howard" - :last_name "Hughes" - :email "howard@aircraft.com" - :password "superstrong" - :date_joined :%now}) + (first (t2/insert-returning-pks! (t2/table-name User) {:first_name "Howard" + :last_name "Hughes" + :email "howard@aircraft.com" + :password "superstrong" + :date_joined :%now})) database-id - (db/simple-insert! Database {:name "DB", :engine "h2", :created_at :%now, :updated_at :%now}) + (first (t2/insert-returning-pks! (t2/table-name Database) {:name "DB", :engine "h2", :created_at :%now, :updated_at :%now})) card-id - (db/simple-insert! Card {:name "My Saved Question" - :created_at :%now - :updated_at :%now - :creator_id user-id - :display "table" - :dataset_query "{}" - :visualization_settings "{}" - :database_id database-id - :collection_id nil})] + (first (t2/insert-returning-pks! (t2/table-name Card) {:name "My Saved Question" + :created_at :%now + :updated_at :%now + :creator_id user-id + :display "table" + :dataset_query "{}" + :visualization_settings "{}" + :database_id database-id + :collection_id nil}))] (migrate!) (is (= nil (:parameter_mappings (first (t2/select (t2/table-name Card) {:where [:= :id card-id]}))))))))) @@ -695,8 +694,8 @@ (testing "Should run for new EE instances" (impl/test-migrations ["v44.00-033" "v44.00-034"] [migrate!] - (db/simple-insert! Setting {:key "premium-embedding-token" - :value "fake-key"}) + (t2/insert! (t2/table-name Setting) {:key "premium-embedding-token" + :value "fake-key"}) (migrate!) (is (= ["All Users"] (get-perms))))) @@ -709,24 +708,24 @@ (testing "Should not run for existing EE instances" (impl/test-migrations ["v44.00-033" "v44.00-034"] [migrate!] (create-raw-user! "ngoc@metabase.com") - (db/simple-insert! Setting {:key "premium-embedding-token" - :value "fake-key"}) + (t2/insert! (t2/table-name Setting) {:key "premium-embedding-token" + :value "fake-key"}) (migrate!) (is (= [] (get-perms))))) (testing "Should not fail if permissions already exist" (impl/test-migrations ["v44.00-033" "v44.00-034"] [migrate!] (t2/query-one {:insert-into :permissions - :values [{:object (perms-path) - :group_id (all-users-group-id)}]}) + :values [{:object (perms-path) + :group_id (all-users-group-id)}]}) (migrate!) (is (= ["All Users"] (get-perms)))))))) (deftest make-database-details-not-null-test (testing "Migrations v45.00-042 and v45.00-043: set default value of '{}' for Database rows with NULL details" (impl/test-migrations ["v45.00-042" "v45.00-043"] [migrate!] - (let [database-id (db/simple-insert! Database (-> (dissoc (mt/with-temp-defaults Database) :details) - (assoc :engine "h2")))] + (let [database-id (first (t2/insert-returning-pks! (t2/table-name Database) (-> (dissoc (mt/with-temp-defaults Database) :details) + (assoc :engine "h2"))))] (is (partial= {:details nil} (t2/select-one Database :id database-id))) (migrate!) @@ -736,43 +735,43 @@ (deftest populate-collection-created-at-test (testing "Migrations v45.00-048 thru v45.00-050: add Collection.created_at and populate it" (impl/test-migrations ["v45.00-048" "v45.00-050"] [migrate!] - (let [database-id (db/simple-insert! Database {:details "{}" - :engine "h2" - :is_sample false - :name "populate-collection-created-at-test-db"}) - user-id (db/simple-insert! User {:first_name "Cam" - :last_name "Era" - :email "cam@example.com" - :password "123456" - :date_joined #t "2022-10-20T02:09Z"}) - personal-collection-id (db/simple-insert! Collection {:name "Cam Era's Collection" - :personal_owner_id user-id - :color "#ff0000" - :slug "personal_collection"}) - impersonal-collection-id (db/simple-insert! Collection {:name "Regular Collection" - :color "#ff0000" - :slug "regular_collection"}) - empty-collection-id (db/simple-insert! Collection {:name "Empty Collection" - :color "#ff0000" - :slug "empty_collection"}) - _ (db/simple-insert! Card {:collection_id impersonal-collection-id - :name "Card 1" - :display "table" - :dataset_query "{}" - :visualization_settings "{}" - :creator_id user-id - :database_id database-id - :created_at #t "2022-10-20T02:09Z" - :updated_at #t "2022-10-20T02:09Z"}) - _ (db/simple-insert! Card {:collection_id impersonal-collection-id - :name "Card 2" - :display "table" - :dataset_query "{}" - :visualization_settings "{}" - :creator_id user-id - :database_id database-id - :created_at #t "2021-10-20T02:09Z" - :updated_at #t "2022-10-20T02:09Z"})] + (let [database-id (first (t2/insert-returning-pks! (t2/table-name Database) {:details "{}" + :engine "h2" + :is_sample false + :name "populate-collection-created-at-test-db"})) + user-id (first (t2/insert-returning-pks! (t2/table-name User) {:first_name "Cam" + :last_name "Era" + :email "cam@example.com" + :password "123456" + :date_joined #t "2022-10-20T02:09Z"})) + personal-collection-id (first (t2/insert-returning-pks! (t2/table-name Collection) {:name "Cam Era's Collection" + :personal_owner_id user-id + :color "#ff0000" + :slug "personal_collection"})) + impersonal-collection-id (first (t2/insert-returning-pks! (t2/table-name Collection) {:name "Regular Collection" + :color "#ff0000" + :slug "regular_collection"})) + empty-collection-id (first (t2/insert-returning-pks! (t2/table-name Collection) {:name "Empty Collection" + :color "#ff0000" + :slug "empty_collection"})) + _ (t2/insert! (t2/table-name Card) {:collection_id impersonal-collection-id + :name "Card 1" + :display "table" + :dataset_query "{}" + :visualization_settings "{}" + :creator_id user-id + :database_id database-id + :created_at #t "2022-10-20T02:09Z" + :updated_at #t "2022-10-20T02:09Z"}) + _ (t2/insert! (t2/table-name Card) {:collection_id impersonal-collection-id + :name "Card 2" + :display "table" + :dataset_query "{}" + :visualization_settings "{}" + :creator_id user-id + :database_id database-id + :created_at #t "2021-10-20T02:09Z" + :updated_at #t "2022-10-20T02:09Z"})] (migrate!) (testing "A personal Collection should get created_at set by to the date_joined from its owner" (is (= (t/offset-date-time #t "2022-10-20T02:09Z") @@ -790,42 +789,42 @@ (deftest deduplicate-dimensions-test (testing "Migrations v46.00-029 thru v46.00-031: make Dimension field_id unique instead of field_id + name" (impl/test-migrations ["v46.00-029" "v46.00-031"] [migrate!] - (let [database-id (db/simple-insert! Database {:details "{}" - :engine "h2" - :is_sample false - :name "populate-collection-created-at-test-db"}) - table-id (db/simple-insert! Table {:db_id database-id - :name "Table" - :created_at :%now - :updated_at :%now - :active true}) - field-1-id (db/simple-insert! Field {:name "F1" - :table_id table-id - :base_type "type/Text" - :database_type "TEXT" - :created_at :%now - :updated_at :%now}) - field-2-id (db/simple-insert! Field {:name "F2" - :table_id table-id - :base_type "type/Text" - :database_type "TEXT" - :created_at :%now - :updated_at :%now}) - _ (db/simple-insert! Dimension {:field_id field-1-id - :name "F1 D1" - :type "internal" - :created_at #t "2022-12-07T18:30:30.000-08:00" - :updated_at #t "2022-12-07T18:30:30.000-08:00"}) - _ (db/simple-insert! Dimension {:field_id field-1-id - :name "F1 D2" - :type "internal" - :created_at #t "2022-12-07T18:45:30.000-08:00" - :updated_at #t "2022-12-07T18:45:30.000-08:00"}) - _ (db/simple-insert! Dimension {:field_id field-2-id - :name "F2 D1" - :type "internal" - :created_at #t "2022-12-07T18:45:30.000-08:00" - :updated_at #t "2022-12-07T18:45:30.000-08:00"})] + (let [database-id (first (t2/insert-returning-pks! (t2/table-name Database) {:details "{}" + :engine "h2" + :is_sample false + :name "populate-collection-created-at-test-db"})) + table-id (first (t2/insert-returning-pks! (t2/table-name Table) {:db_id database-id + :name "Table" + :created_at :%now + :updated_at :%now + :active true})) + field-1-id (first (t2/insert-returning-pks! (t2/table-name Field) {:name "F1" + :table_id table-id + :base_type "type/Text" + :database_type "TEXT" + :created_at :%now + :updated_at :%now})) + field-2-id (first (t2/insert-returning-pks! (t2/table-name Field) {:name "F2" + :table_id table-id + :base_type "type/Text" + :database_type "TEXT" + :created_at :%now + :updated_at :%now})) + _ (t2/insert! (t2/table-name Dimension) {:field_id field-1-id + :name "F1 D1" + :type "internal" + :created_at #t "2022-12-07T18:30:30.000-08:00" + :updated_at #t "2022-12-07T18:30:30.000-08:00"}) + _ (t2/insert! (t2/table-name Dimension) {:field_id field-1-id + :name "F1 D2" + :type "internal" + :created_at #t "2022-12-07T18:45:30.000-08:00" + :updated_at #t "2022-12-07T18:45:30.000-08:00"}) + _ (t2/insert! (t2/table-name Dimension) {:field_id field-2-id + :name "F2 D1" + :type "internal" + :created_at #t "2022-12-07T18:45:30.000-08:00" + :updated_at #t "2022-12-07T18:45:30.000-08:00"})] (is (= #{"F1 D1" "F1 D2" "F2 D1"} @@ -840,74 +839,74 @@ (testing "Migrations v46.00-064 to v46.00-067: rename `group_table_access_policy` table, add `permission_id` FK, and clean up orphaned rows" (impl/test-migrations ["v46.00-064" "v46.00-067"] [migrate!] - (let [db-id (db/simple-insert! Database {:name "DB" - :engine "h2" - :created_at :%now - :updated_at :%now - :details "{}"}) - table-id (db/simple-insert! Table {:db_id db-id - :name "Table" - :created_at :%now - :updated_at :%now - :active true}) + (let [db-id (first (t2/insert-returning-pks! (t2/table-name Database) {:name "DB" + :engine "h2" + :created_at :%now + :updated_at :%now + :details "{}"})) + table-id (first (t2/insert-returning-pks! (t2/table-name Table) {:db_id db-id + :name "Table" + :created_at :%now + :updated_at :%now + :active true})) _ (t2/query-one {:insert-into :group_table_access_policy - :values [{:group_id 1 - :table_id table-id - :attribute_remappings "{\"foo\", 1}"} - {:group_id 2 - :table_id table-id - :attribute_remappings "{\"foo\", 1}"}]}) - perm-id (db/simple-insert! Permissions {:group_id 1 - :object "/db/1/schema/PUBLIC/table/1/query/segmented/"})] - ;; Two rows are present in `group_table_access_policy` - (is (= [{:id 1, :group_id 1, :table_id table-id, :card_id nil, :attribute_remappings "{\"foo\", 1}"} - {:id 2, :group_id 2, :table_id table-id, :card_id nil, :attribute_remappings "{\"foo\", 1}"}] - (mdb.query/query {:select [:*] :from [:group_table_access_policy]}))) - (migrate!) - ;; Only the sandbox with a corresponding `Permissions` row is present, and the table is renamed to `sandboxes` - (is (= [{:id 1, :group_id 1, :table_id table-id, :card_id nil, :attribute_remappings "{\"foo\", 1}", :permission_id perm-id}] - (mdb.query/query {:select [:*] :from [:sandboxes]}))))))) + :values [{:group_id 1 + :table_id table-id + :attribute_remappings "{\"foo\", 1}"} + {:group_id 2 + :table_id table-id + :attribute_remappings "{\"foo\", 1}"}]}) + perm-id (first (t2/insert-returning-pks! (t2/table-name Permissions) {:group_id 1 + :object "/db/1/schema/PUBLIC/table/1/query/segmented/"}))] + ;; Two rows are present in `group_table_access_policy` + (is (= [{:id 1, :group_id 1, :table_id table-id, :card_id nil, :attribute_remappings "{\"foo\", 1}"} + {:id 2, :group_id 2, :table_id table-id, :card_id nil, :attribute_remappings "{\"foo\", 1}"}] + (mdb.query/query {:select [:*] :from [:group_table_access_policy]}))) + (migrate!) + ;; Only the sandbox with a corresponding `Permissions` row is present, and the table is renamed to `sandboxes` + (is (= [{:id 1, :group_id 1, :table_id table-id, :card_id nil, :attribute_remappings "{\"foo\", 1}", :permission_id perm-id}] + (mdb.query/query {:select [:*] :from [:sandboxes]}))))))) (deftest able-to-delete-db-with-actions-test (testing "Migrations v46.00-084 and v46.00-085 set delete CASCADE for action.model_id to fix the bug of unable to delete database with actions" (impl/test-migrations ["v46.00-084" "v46.00-085"] [migrate!] - (let [user-id (db/simple-insert! User {:first_name "Howard" - :last_name "Hughes" - :email "howard@aircraft.com" - :password "superstrong" - :date_joined :%now}) - db-id (db/simple-insert! Database {:name "db" - :engine "postgres" - :created_at :%now - :updated_at :%now - :settings "{\"database-enable-actions\":true}" - :details "{}"}) - table-id (db/simple-insert! Table {:db_id db-id - :name "Table" - :created_at :%now - :updated_at :%now - :active true}) - model-id (db/simple-insert! Card {:name "My Saved Question" - :created_at :%now - :updated_at :%now - :creator_id user-id - :table_id table-id - :display "table" - :dataset_query "{}" - :visualization_settings "{}" - :database_id db-id - :collection_id nil}) - _ (db/simple-insert! Action {:name "Update user name" - :type "implicit" - :model_id model-id - :archived false - :created_at :%now - :updated_at :%now})] - (is (thrown? clojure.lang.ExceptionInfo - (t2/delete! Database :id db-id))) - (migrate!) - (is (t2/delete! Database :id db-id)))))) + (let [user-id (first (t2/insert-returning-pks! (t2/table-name User) {:first_name "Howard" + :last_name "Hughes" + :email "howard@aircraft.com" + :password "superstrong" + :date_joined :%now})) + db-id (first (t2/insert-returning-pks! (t2/table-name Database) {:name "db" + :engine "postgres" + :created_at :%now + :updated_at :%now + :settings "{\"database-enable-actions\":true}" + :details "{}"})) + table-id (first (t2/insert-returning-pks! (t2/table-name Table) {:db_id db-id + :name "Table" + :created_at :%now + :updated_at :%now + :active true})) + model-id (first (t2/insert-returning-pks! (t2/table-name Card) {:name "My Saved Question" + :created_at :%now + :updated_at :%now + :creator_id user-id + :table_id table-id + :display "table" + :dataset_query "{}" + :visualization_settings "{}" + :database_id db-id + :collection_id nil})) + _ (t2/insert! (t2/table-name Action) {:name "Update user name" + :type "implicit" + :model_id model-id + :archived false + :created_at :%now + :updated_at :%now})] + (is (thrown? clojure.lang.ExceptionInfo + (t2/delete! Database :id db-id))) + (migrate!) + (is (t2/delete! Database :id db-id)))))) (deftest split-data-permission-test (testing "Migration v46.00-080: split existing v1 data permission paths into v2 data and query permission paths" diff --git a/test/metabase/db/schema_migrations_test/impl.clj b/test/metabase/db/schema_migrations_test/impl.clj index 432bc2193f3..cfb8ad85f79 100644 --- a/test/metabase/db/schema_migrations_test/impl.clj +++ b/test/metabase/db/schema_migrations_test/impl.clj @@ -223,7 +223,7 @@ this order: 1. Load data and check any preconditions before running migrations you're testing. - Prefer [[toucan.db/simple-insert!]] or plain SQL for loading data to avoid dependencies on the current state of + Prefer [[t2/insert!]] with a table name or plain SQL for loading data to avoid dependencies on the current state of the schema that may be present in Toucan `pre-insert` functions and the like. 2. Call `(migrate!)` to run migrations in range of `start-id` -> `end-id` (inclusive) diff --git a/test/metabase/events/revision_test.clj b/test/metabase/events/revision_test.clj index 965a615a1f7..2416c4b790d 100644 --- a/test/metabase/events/revision_test.clj +++ b/test/metabase/events/revision_test.clj @@ -6,7 +6,6 @@ :refer [Card Dashboard DashboardCard Database Metric Revision Segment Table]] [metabase.test :as mt] [metabase.util :as u] - [toucan.db :as db] [toucan2.core :as t2])) (defn- card-properties @@ -138,7 +137,7 @@ (mt/with-temp* [Dashboard [{dashboard-id :id, :as dashboard}] Card [{card-id :id} (card-properties)] DashboardCard [dashcard {:card_id card-id, :dashboard_id dashboard-id}]] - (db/simple-delete! DashboardCard, :id (:id dashcard)) + (t2/delete! (t2/table-name DashboardCard), :id (:id dashcard)) (revision/process-revision-event! {:topic :dashboard-remove-cards :item {:id dashboard-id :actor_id (mt/user->id :rasta) diff --git a/test/metabase/models/setting/cache_test.clj b/test/metabase/models/setting/cache_test.clj index 1d0436f1220..204ee76335b 100644 --- a/test/metabase/models/setting/cache_test.clj +++ b/test/metabase/models/setting/cache_test.clj @@ -38,7 +38,7 @@ (defn- simulate-another-instance-updating-setting! [setting-name new-value] (if new-value (db/update-where! Setting {:key (name setting-name)} :value new-value) - (db/simple-delete! Setting {:key (name setting-name)})) + (t2/delete! (t2/table-name Setting) {:key (name setting-name)})) (update-settings-last-updated-value-in-db!)) (defn reset-last-update-check! diff --git a/test/metabase/models/setting_test.clj b/test/metabase/models/setting_test.clj index 923356c2621..918617ebc90 100644 --- a/test/metabase/models/setting_test.clj +++ b/test/metabase/models/setting_test.clj @@ -514,7 +514,7 @@ (defn clear-settings-last-updated-value-in-db! "Deletes the timestamp for the last updated setting from the DB." [] - (db/simple-delete! Setting {:key setting.cache/settings-last-updated-key})) + (t2/delete! (t2/table-name Setting) :key setting.cache/settings-last-updated-key)) (defn settings-last-updated-value-in-db "Fetches the timestamp of the last updated setting." @@ -571,7 +571,7 @@ (deftest cache-sync-test (testing "make sure that if for some reason the cache gets out of sync it will reset so we can still set new settings values (#4178)" ;; clear out any existing values of `toucan-name` - (db/simple-delete! setting/Setting {:key "toucan-name"}) + (t2/delete! (t2/table-name setting/Setting) :key "toucan-name") ;; restore the cache (setting.cache/restore-cache-if-needed!) ;; now set a value for the `toucan-name` setting the wrong way diff --git a/test/metabase/server/middleware/session_test.clj b/test/metabase/server/middleware/session_test.clj index 437a5cf9690..5d43fc4648f 100644 --- a/test/metabase/server/middleware/session_test.clj +++ b/test/metabase/server/middleware/session_test.clj @@ -117,7 +117,7 @@ (testing (format "\n%s %s be expired." msg (if expected "SHOULD" "SHOULD NOT")) (mt/with-temp User [{user-id :id}] (let [session-id (str (UUID/randomUUID))] - (db/simple-insert! Session {:id session-id, :user_id user-id, :created_at created-at}) + (t2/insert! (t2/table-name Session) {:id session-id, :user_id user-id, :created_at created-at}) (let [session (#'mw.session/current-user-info-for-session session-id nil)] (if expected (is (= nil diff --git a/test/metabase/sync/field_values_test.clj b/test/metabase/sync/field_values_test.clj index 170a5a4dc82..20ec9427e28 100644 --- a/test/metabase/sync/field_values_test.clj +++ b/test/metabase/sync/field_values_test.clj @@ -12,7 +12,6 @@ [metabase.test :as mt] [metabase.test.data :as data] [metabase.test.data.one-off-dbs :as one-off-dbs] - [toucan.db :as db] [toucan2.core :as t2])) (defn- venues-price-field-values [] @@ -93,8 +92,8 @@ valid-sandbox-id valid-linked-filter-id old-full-id - new-full-id] (db/simple-insert-many! - FieldValues + new-full-id] (t2/insert-returning-pks! + (t2/table-name FieldValues) [;; expired sandbox fieldvalues {:field_id field-id :type "sandbox" diff --git a/test/metabase/test.clj b/test/metabase/test.clj index f3d6380e102..379c818d711 100644 --- a/test/metabase/test.clj +++ b/test/metabase/test.clj @@ -309,7 +309,7 @@ (defn do-with-single-admin-user [attributes thunk] (let [existing-admin-memberships (t2/select PermissionsGroupMembership :group_id (:id (perms-group/admin))) - _ (db/simple-delete! PermissionsGroupMembership :group_id (:id (perms-group/admin))) + _ (t2/delete! (t2/table-name PermissionsGroupMembership) :group_id (:id (perms-group/admin))) existing-admin-ids (t2/select-pks-set User :is_superuser true) _ (when (seq existing-admin-ids) (db/update-where! User {:id [:in existing-admin-ids]} :is_superuser false)) -- GitLab