diff --git a/enterprise/backend/test/metabase_enterprise/serialization/v2/e2e_test.clj b/enterprise/backend/test/metabase_enterprise/serialization/v2/e2e_test.clj
index 8a9b45f12209b9bd5198b4a4b3b53f42e774980e..daf70db389ff8ddcc40e78b9e690b37aa390313e 100644
--- a/enterprise/backend/test/metabase_enterprise/serialization/v2/e2e_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/serialization/v2/e2e_test.clj
@@ -422,9 +422,9 @@
Collection coll1s {:name "My Collection"}
Table table1s {:name "CUSTOMERS"
:db_id (:id db1s)}
- Field field1s {:name "NAME"
+ Field field1s {:name (mt/random-name)
:table_id (:id table1s)}
- Card card1s {:name "Source card"}
+ Card card1s {:name (mt/random-name)}
Card card2s {:name "Card with parameter"
:database_id (:id db1s)
:table_id (:id table1s)
@@ -436,7 +436,7 @@
;; card_id is in a different collection with dashboard's collection
:values_source_config {:card_id (:id card1s)
:value_field [:field (:id field1s) nil]}}]}
- Dashboard dash1s {:name "A dashboard"
+ Dashboard dash1s {:name (mt/random-name)
:collection_id (:id coll1s)
:parameters [{:id "abc"
:type "category"
@@ -457,7 +457,7 @@
:type :category,
:values_source_config {:card_id (:entity_id card1s),
:value_field [:field
- ["my-db" nil "CUSTOMERS" "NAME"]
+ ["my-db" nil "CUSTOMERS" (:name field1s)]
nil]},
:values_source_type "card"}]
(:parameters (first (by-model extraction "Dashboard")))))
@@ -469,7 +469,7 @@
:type :category,
:values_source_config {:card_id (:entity_id card1s),
:value_field [:field
- ["my-db" nil "CUSTOMERS" "NAME"]
+ ["my-db" nil "CUSTOMERS" (:name field1s)]
nil]},
:values_source_type "card"}]}
(set (map :parameters (by-model extraction "Card")))))
diff --git a/resources/migrations/001_update_migrations.yaml b/resources/migrations/001_update_migrations.yaml
index fd17c9a93fe651fed1f08a15ac33a3d7c0e3929e..828294b6cbf79b008579908ac359d34c639a8891 100644
--- a/resources/migrations/001_update_migrations.yaml
+++ b/resources/migrations/001_update_migrations.yaml
@@ -5747,17 +5747,6 @@ databaseChangeLog:
defaultValue: true
remarks: Whether or not to apply formatting to the rows of the export
- - changeSet:
- id: v49.2024-03-26T20:27:58
- author: noahmoss
- comment: Added 0.46.0 - Delete the truncate audit log task (renamed to truncate audit tables)
- # Apparently this migration can hang when connection pool is small (#44528). It's also not needed
- # as of v50 due to #42383 so let's skip it.
- ignore: true
- changes:
- - customChange:
- class: "metabase.db.custom_migrations.DeleteTruncateAuditLogTask"
-
- changeSet:
id: v49.2024-04-09T10:00:00
author: qnkhuat
@@ -6494,6 +6483,211 @@ databaseChangeLog:
- dbms:
type: mysql,mariadb
+ # The changesets from v49.2024-06-27T00:00:00 to v49.2024-06-27T00:00:08 prevent duplicate fields from being
+ # created on MySQL and H2. See #44866 for details. Below is an index of the changesets:
+ # - v49.2024-06-27T00:00:00: Make metabase_field.name use case-sensitive collation for MySQL
+ # - v49.2024-06-27T00:00:01: Add metabase_field.is_defective_duplicate
+ # - v49.2024-06-27T00:00:02: Populate metabase_field.is_defective_duplicate
+ # - v49.2024-06-27T00:00:03: Drop fk_field_parent_ref_field_id constraint with ON DELETE CASCADE
+ # - v49.2024-06-27T00:00:04: Add fk_field_parent_ref_field_id constraint with ON DELETE RESTRICT
+ # - v49.2024-06-27T00:00:05: Remove idx_uniq_field_table_id_parent_id_name because it is redundant with idx_unique_field
+ # - v49.2024-06-27T00:00:06: Remove the idx_uniq_field_table_id_parent_id_name_2col unique index because it blocks load-from-h2
+ # - v49.2024-06-27T00:00:07: Add unique_field_helper column to metabase_field
+ # - v49.2024-06-27T00:00:08: Add unique constraint on metabase_field's (name, table_id, unique_field_helper)
+
+ # This is necessary to make unique indexes using metabase_field.name case-sensitive for MySQL.
+ - changeSet:
+ id: v49.2024-06-27T00:00:00
+ author: calherries
+ comment: Make metabase_field.name use case-sensitive collation for MySQL
+ changes:
+ - sql:
+ dbms: mysql,mariadb
+ sql: >-
+ ALTER TABLE metabase_field MODIFY
+ name VARCHAR(254)
+ CHARACTER SET utf8mb4
+ COLLATE utf8mb4_bin;
+ rollback:
+ - sql:
+ dbms: mysql,mariadb
+ sql: >-
+ ALTER TABLE metabase_field MODIFY
+ name VARCHAR(254)
+ CHARACTER SET utf8mb4
+ COLLATE utf8mb4_unicode_ci;
+
+ # metabase_field.is_defective_duplicate is a new column that indicates whether a field is a defective duplicate field
+ # that should never have been created under Postgres' `idx_uniq_field_table_id_parent_id_name_2col` constraint, but
+ # was allowed to be created in MySQL or H2.
+ - changeSet:
+ id: v49.2024-06-27T00:00:01
+ author: calherries
+ comment: Add metabase_field.is_defective_duplicate
+ changes:
+ - addColumn:
+ tableName: metabase_field
+ columns:
+ - column:
+ name: is_defective_duplicate
+ type: ${boolean.type}
+ remarks: "Indicates whether column is a defective duplicate field that should never have been created."
+ constraints:
+ nullable: false
+ defaultValue: false
+
+ - changeSet:
+ id: v49.2024-06-27T00:00:02
+ author: calherries
+ comment: Populate metabase_field.is_defective_duplicate
+ changes:
+ - sql:
+ sql: >-
+ UPDATE metabase_field mf
+ SET is_defective_duplicate = TRUE
+ WHERE mf.id IN (
+ SELECT id
+ FROM (
+ SELECT
+ mf.id,
+ ROW_NUMBER() OVER (
+ PARTITION BY mf.table_id, mf.name, mf.parent_id
+ ORDER BY
+ CASE WHEN mf.active THEN 0 ELSE 1 END,
+ CASE WHEN mf.nfc_path IS NULL THEN 0 ELSE 1 END,
+ mf.created_at
+ ) AS rn
+ FROM metabase_field mf
+ ) x
+ WHERE x.rn > 1
+ );
+ rollback: # No rollback needed since is_defective_duplicate is introduced in 49
+
+ # The next two changesets replace ON DELETE CASCADE with ON DELETE RESTRICT on fk_field_parent_ref_field_id.
+ # We need to do this for MySQL because it doesn't support ON DELETE CASCADE in a stored generated column, and we
+ # want to use parent_id in the unique_field_helper generated column. Cascading deletes are handled in the
+ # application instead.
+ - changeSet:
+ id: v49.2024-06-27T00:00:03
+ author: calherries
+ comment: Drop fk_field_parent_ref_field_id constraint with ON DELETE CASCADE
+ changes:
+ - dropForeignKeyConstraint:
+ baseTableName: metabase_field
+ constraintName: fk_field_parent_ref_field_id
+ rollback:
+ - addForeignKeyConstraint:
+ baseTableName: metabase_field
+ baseColumnNames: parent_id
+ referencedTableName: metabase_field
+ referencedColumnNames: id
+ constraintName: fk_field_parent_ref_field_id
+ onDelete: CASCADE
+
+ - changeSet:
+ id: v49.2024-06-27T00:00:04
+ author: calherries
+ comment: Add fk_field_parent_ref_field_id constraint with ON DELETE RESTRICT
+ changes:
+ - addForeignKeyConstraint:
+ baseTableName: metabase_field
+ baseColumnNames: parent_id
+ referencedTableName: metabase_field
+ referencedColumnNames: id
+ constraintName: fk_field_parent_ref_field_id
+ onDelete: RESTRICT
+ rollback:
+ - dropForeignKeyConstraint:
+ baseTableName: metabase_field
+ constraintName: fk_field_parent_ref_field_id
+
+ - changeSet:
+ id: v49.2024-06-27T00:00:05
+ author: calherries
+ comment: Remove idx_uniq_field_table_id_parent_id_name because it is redundant with idx_unique_field
+ changes:
+ - dropUniqueConstraint:
+ tableName: metabase_field
+ constraintName: idx_uniq_field_table_id_parent_id_name
+ rollback:
+ - addUniqueConstraint:
+ tableName: metabase_field
+ columnNames: table_id, parent_id, name
+ constraintName: idx_uniq_field_table_id_parent_id_name
+
+ - changeSet:
+ id: v49.2024-06-27T00:00:06
+ author: calherries
+ comment: Remove the idx_uniq_field_table_id_parent_id_name_2col unique index because it blocks load-from-h2
+ changes:
+ - sql:
+ dbms: postgresql
+ sql: DROP INDEX IF EXISTS idx_uniq_field_table_id_parent_id_name_2col;
+ rollback: # We deliberately don't restore this constraint because otherwise it can block downgrading to 48 after load-from-h2
+
+ # Previously we had two unique constraints on metabase_field's name, table_id, and parent_id columns.
+ #
+ # 1. `idx_uniq_field_table_id_parent_id_name` is a unique constraint on (name, table_id, parent_id)
+ # Since NULL <> NULL, it only applies to fields where parent_id IS NOT NULL.
+ # Worse, the constraint was not case-sensitive for MySQL.
+ #
+ # 2. `idx_uniq_field_table_id_parent_id_name_2col` is a Postgres-only unique constraint on
+ # `(name, table_id) WHERE parent_id IS NULL`. There was no equivalent constraint for H2 or MySQL because only
+ # Postgres supports partial indexes. The following changesets introduce an equivalent constraint for H2 and
+ # MySQL by adding a constraint that uses a generated column to handle NULL parent_id values.
+ #
+ # At the same time, we exclude fields where is_defective_duplicate=TRUE from the above constraints.
+ #
+ # The following two changesets add a new column `unique_field_helper` to `metabase_field` and a unique constraint
+ # `idx_unique_field`.
+ # The combination of `unique_field_helper` and `idx_unique_field` achieves two things:
+ # 1. It enforces the conditional constraints:
+ # - if parent_id IS NULL, then (table_id, name) is unique. (like idx_uniq_field_table_id_parent_id_name_col2)
+ # - if parent_id IS NOT NULL, then (table_id, name, parent_id) is unique. (like idx_uniq_field_table_id_parent_id_name)
+ # 2. It only enforces the constraints when is_defective_duplicate = FALSE
+
+ - changeSet:
+ id: v49.2024-06-27T00:00:07
+ author: calherries
+ comment: Add unique_field_helper column to metabase_field
+ changes:
+ - sql:
+ dbms: postgresql
+ sql: >-
+ ALTER TABLE metabase_field ADD COLUMN unique_field_helper INTEGER GENERATED ALWAYS AS (
+ CASE WHEN is_defective_duplicate = TRUE THEN NULL ELSE (CASE WHEN parent_id IS NULL THEN 0 ELSE parent_id END) END
+ ) STORED;
+ - sql:
+ dbms: h2
+ sql: >-
+ ALTER TABLE metabase_field ADD COLUMN unique_field_helper INTEGER GENERATED ALWAYS AS (
+ CASE WHEN is_defective_duplicate = TRUE THEN NULL ELSE (CASE WHEN parent_id IS NULL THEN 0 ELSE parent_id END) END
+ );
+ - sql:
+ dbms: mysql,mariadb
+ sql: >-
+ ALTER TABLE metabase_field ADD COLUMN unique_field_helper INTEGER GENERATED ALWAYS AS (
+ CASE WHEN is_defective_duplicate = TRUE THEN NULL ELSE (CASE WHEN parent_id IS NULL THEN 0 ELSE parent_id END) END
+ ) STORED;
+ rollback:
+ - sql:
+ dbms: postgresql,h2,mysql,mariadb
+ sql: ALTER TABLE metabase_field DROP COLUMN unique_field_helper;
+
+ - changeSet:
+ id: v49.2024-06-27T00:00:08
+ author: calherries
+ comment: Add unique constraint on metabase_field's (name, table_id, unique_field_helper)
+ changes:
+ - addUniqueConstraint:
+ tableName: metabase_field
+ constraintName: idx_unique_field
+ columnNames: name, table_id, unique_field_helper
+ rollback:
+ - dropUniqueConstraint:
+ tableName: metabase_field
+ constraintName: idx_unique_field
+
- changeSet:
id: v50.2024-01-04T13:52:51
author: noahmoss
diff --git a/src/metabase/cmd/copy.clj b/src/metabase/cmd/copy.clj
index 6ffc0010dce948aec811d1c3a276860e9b56d7c1..663fe5f19b91edc99633c1bc677f021b545e4b3a 100644
--- a/src/metabase/cmd/copy.clj
+++ b/src/metabase/cmd/copy.clj
@@ -166,6 +166,9 @@
;; Never create dumps with read-only-mode turned on.
;; It will be confusing to restore from and prevent key rotation.
(remove (fn [{k :key}] (= k "read-only-mode")))
+ :model/Field
+ ;; unique_field_helper is a computed/generated column
+ (map #(dissoc % :unique_field_helper))
;; else
identity))
diff --git a/src/metabase/models/database.clj b/src/metabase/models/database.clj
index b91e4f051419ad2c2f4f3d781bea099fff68f760..e3f77547bd38a0ab1ed961fcac089d116acc8c15 100644
--- a/src/metabase/models/database.clj
+++ b/src/metabase/models/database.clj
@@ -208,6 +208,10 @@
[{id :id, driver :engine, :as database}]
(unschedule-tasks! database)
(delete-orphaned-secrets! database)
+ ;; We need to use toucan to delete the fields instead of cascading deletes because MySQL doesn't support columns with cascade delete
+ ;; foreign key constraints in generated columns. #44866
+ (when-some [table-ids (not-empty (t2/select-pks-vec :model/Table :db_id id))]
+ (t2/delete! :model/Field :table_id [:in table-ids]))
(try
(driver/notify-database-updated driver database)
(catch Throwable e
diff --git a/src/metabase/models/field.clj b/src/metabase/models/field.clj
index d970ce804129bbc84b77cff76424766b34c726e1..a866f6d8890b7ea4eb47818fa9e3dd14286eb099 100644
--- a/src/metabase/models/field.clj
+++ b/src/metabase/models/field.clj
@@ -127,6 +127,10 @@
(derive :metabase/model)
(derive :hook/timestamped?))
+(t2/define-after-select :model/Field
+ [field]
+ (dissoc field :is_defective_duplicate :unique_field_helper))
+
(t2/define-before-insert :model/Field
[field]
(let [defaults {:display_name (humanization/name->human-readable-name (:name field))}]
@@ -139,6 +143,13 @@
(t2/update! :model/Field {:fk_target_field_id (:id field)} {:semantic_type nil
:fk_target_field_id nil}))))
+(t2/define-before-delete :model/Field
+ [field]
+ ; Cascading deletes through parent_id cannot be done with foreign key constraints in the database
+ ; because parent_id constributes to a generated column, and MySQL doesn't support columns with cascade delete
+ ;; foreign key constraints in generated columns. #44866
+ (t2/delete! :model/Field :parent_id (:id field)))
+
(defn- field->db-id
[{table-id :table_id, {db-id :db_id} :table}]
(or db-id (database/table-id->database-id table-id)))
diff --git a/src/metabase/models/table.clj b/src/metabase/models/table.clj
index 8bc0429365d23fd959d93b246686c74be0be3de8..288a853a19aa77157d52950242f864778de2d240 100644
--- a/src/metabase/models/table.clj
+++ b/src/metabase/models/table.clj
@@ -61,6 +61,12 @@
:field_order (driver/default-field-order (t2/select-one-fn :engine :model/Database :id (:db_id table)))}]
(merge defaults table)))
+(t2/define-before-delete :model/Table
+ [table]
+ ;; We need to use toucan to delete the fields instead of cascading deletes because MySQL doesn't support columns with cascade delete
+ ;; foreign key constraints in generated columns. #44866
+ (t2/delete! :model/Field :table_id (:id table)))
+
(defn- set-new-table-permissions!
[table]
(t2/with-transaction [_conn]
diff --git a/test/metabase/cmd/load_from_h2_test.clj b/test/metabase/cmd/load_from_h2_test.clj
index 34419da61fa7a698df7ef3b7cadea1914ba23004..562bee61b183669ae5ef704412b23a435be5f544 100644
--- a/test/metabase/cmd/load_from_h2_test.clj
+++ b/test/metabase/cmd/load_from_h2_test.clj
@@ -58,12 +58,12 @@
(is (=? {:db string?}
(t2/select-one-fn :details :model/Database :engine :h2))))))))
-(defn- get-data-source [db-type db-def]
+(defn get-data-source [db-type db-def]
(let [connection-details (tx/dbdef->connection-details db-type :db db-def)
db-spec (sql-jdbc.conn/connection-details->spec db-type connection-details)]
(mdb.test-util/->ClojureJDBCSpecDataSource db-spec)))
-(defn- create-current-database
+(defn create-current-database
[db-type db-def data-source]
(tx/create-db! db-type db-def)
(mdb.setup/setup-db! db-type data-source true false))
diff --git a/test/metabase/db/schema_migrations_test.clj b/test/metabase/db/schema_migrations_test.clj
index 61382f1b7b819c99869e78e00f7602dfa7cfb31c..138cc43022b297807e7ef939f6b6299d7fc65550 100644
--- a/test/metabase/db/schema_migrations_test.clj
+++ b/test/metabase/db/schema_migrations_test.clj
@@ -11,15 +11,21 @@
(:require
[cheshire.core :as json]
[clojure.java.jdbc :as jdbc]
+ [clojure.set :as set]
[clojure.test :refer :all]
[java-time.api :as t]
[medley.core :as m]
+ [metabase.cmd.dump-to-h2 :as dump-to-h2]
+ [metabase.cmd.load-from-h2 :as load-from-h2]
+ [metabase.cmd.load-from-h2-test :as load-from-h2-test]
[metabase.config :as config]
[metabase.db :as mdb]
+ [metabase.db.connection :as mdb.connection]
[metabase.db.custom-migrations-test :as custom-migrations-test]
[metabase.db.liquibase :as liquibase]
[metabase.db.query :as mdb.query]
[metabase.db.schema-migrations-test.impl :as impl]
+ [metabase.driver :as driver]
[metabase.models
:refer [Action
Card
@@ -33,6 +39,7 @@
User]]
[metabase.models.collection :as collection]
[metabase.test :as mt]
+ [metabase.test.data.env :as tx.env]
[metabase.test.fixtures :as fixtures]
[metabase.util.encryption :as encryption]
[metabase.util.encryption-test :as encryption-test]
@@ -2205,3 +2212,220 @@
#{i} ;; => I is the last archived subtree. It's a grandchild of G, but H isn't archived.
#{b h}} ;; => not archived at all, `archive_operation_id` is nil
(set (vals archive-operation-id->collection-ids))))))))))
+
+(deftest populate-is-defective-duplicate-test
+ (testing "Migration v49.2024-06-27T00:00:02 populates is_defective_duplicate correctly"
+ (mt/test-drivers #{:postgres :h2 :mysql}
+ (impl/test-migrations ["v49.2024-06-27T00:00:00" "v49.2024-06-27T00:00:08"] [migrate!]
+ (when (= driver/*driver* :postgres)
+ ;; This is to test what happens when Postgres is rolled back to 48 from 49, and
+ ;; then rolled back to 49 again. The rollback to 48 will cause the
+ ;; idx_uniq_field_table_id_parent_id_name_2col index to be dropped
+ (t2/query "DROP INDEX IF EXISTS idx_uniq_field_table_id_parent_id_name_2col;"))
+ (let [db-id (t2/insert-returning-pk! (t2/table-name Database)
+ {:details "{}"
+ :created_at :%now
+ :updated_at :%now
+ :engine "h2"
+ :is_sample false
+ :name "populate-is-defective-duplicate-test-db"})
+ table! (fn []
+ (t2/insert-returning-instance! (t2/table-name Table)
+ {:db_id db-id
+ :name (mt/random-name)
+ :created_at :%now
+ :updated_at :%now
+ :active true}))
+ field! (fn [table values]
+ (t2/insert-returning-instance! (t2/table-name Field)
+ (merge {:table_id (:id table)
+ :parent_id nil
+ :base_type "type/Text"
+ :database_type "TEXT"
+ :created_at :%now
+ :updated_at :%now}
+ values)))
+ earlier #t "2023-01-01T00:00:00"
+ later #t "2024-01-01T00:00:00"
+ ; 1.
+ table-1 (table!)
+ cases-1 {; field ; is_defective_duplicate
+ (field! table-1 {:name "F1", :active true, :nfc_path "NOT NULL", :created_at later}) false
+ (field! table-1 {:name "F1", :active false, :nfc_path nil, :created_at earlier}) true}
+ ; 2.
+ table-2 (table!)
+ cases-2 {(field! table-2 {:name "F2", :active true, :nfc_path nil, :created_at later}) false
+ (field! table-2 {:name "F2", :active true, :nfc_path "NOT NULL", :created_at earlier}) true}
+ ; 3.
+ table-3 (table!)
+ cases-3 {(field! table-3 {:name "F3", :active true, :nfc_path nil, :created_at earlier}) false
+ (field! table-3 {:name "F3", :active true, :nfc_path nil, :created_at later}) true}
+ ; 4.
+ table-4 (table!)
+ cases-4 {(field! table-4 {:name "F4", :active true, :nfc_path nil, :created_at earlier}) false
+ (field! table-4 {:name "F4", :active false, :nfc_path nil, :created_at later}) true
+ (field! table-4 {:name "F4", :active false, :nfc_path "NOT NULL", :created_at earlier}) true
+ (field! table-4 {:name "F4", :active false, :nfc_path "NOT NULL", :created_at later}) true}
+ ; 5.
+ table-5 (table!)
+ field-no-parent-1 (field! table-5 {:name "F5", :active true, :parent_id nil})
+ field-no-parent-2 (field! table-5 {:name "F5", :active false, :parent_id nil})
+ field-with-parent-1 (field! table-5 {:name "F5", :active true, :parent_id (:id field-no-parent-1)})
+ field-with-parent-2 (field! table-5 {:name "F5", :active true, :parent_id (:id field-no-parent-2)})
+ cases-5 {field-no-parent-1 false
+ field-no-parent-2 true
+ field-with-parent-1 false
+ field-with-parent-2 false}
+ assert-defective-cases (fn [field->defective?]
+ (doseq [[field-before defective?] field->defective?]
+ (let [field-after (t2/select-one (t2/table-name Field) :id (:id field-before))]
+ (is (= defective? (:is_defective_duplicate field-after))))))]
+ (migrate!)
+ (testing "1. Active is 1st preference"
+ (assert-defective-cases cases-1))
+ (testing "2. NULL nfc_path is 2nd preference"
+ (assert-defective-cases cases-2))
+ (testing "3. Earlier created_at is 3rd preference"
+ (assert-defective-cases cases-3))
+ (testing "4. More than two fields can be defective"
+ (assert-defective-cases cases-4))
+ (testing "5. Fields with different parent_id's are not defective duplicates"
+ (assert-defective-cases cases-5))
+ (when (not= driver/*driver* :mysql) ; skipping MySQL because of rollback flakes (metabase#37434)
+ (testing "Migrate down succeeds"
+ (migrate! :down 48))))))))
+
+(deftest is-defective-duplicate-constraint-test
+ (testing "Migrations for H2 and MySQL to prevent duplicate fields"
+ (impl/test-migrations ["v49.2024-06-27T00:00:00" "v49.2024-06-27T00:00:08"] [migrate!]
+ (let [db-id (t2/insert-returning-pk! (t2/table-name Database)
+ {:details "{}"
+ :created_at :%now
+ :updated_at :%now
+ :engine "h2"
+ :is_sample false
+ :name "populate-is-defective-duplicate-test-db"})
+ table (t2/insert-returning-instance! (t2/table-name Table)
+ {:db_id db-id
+ :name (mt/random-name)
+ :created_at :%now
+ :updated_at :%now
+ :active true})
+ field! (fn [values]
+ (t2/insert-returning-instance! (t2/table-name Field)
+ (merge {:table_id (:id table)
+ :parent_id nil
+ :base_type "type/Text"
+ :database_type "TEXT"
+ :created_at :%now
+ :updated_at :%now}
+ values)))
+ field-no-parent-1 (field! {:name "F1", :active true, :parent_id nil})
+ field-no-parent-2 (field! {:name "F2", :active true, :parent_id nil})
+ defective+field-thunk [; A field is defective if they have non-unique (table, name) but parent_id is NULL
+ [true #(field! {:name "F1", :active true, :parent_id nil, :nfc_path "NOT NULL"})]
+ ; A field is not defective if they have non-unique (table, name) but different parent_id
+ [false #(field! {:name "F1", :active true, :parent_id (:id field-no-parent-1)})]
+ [false #(field! {:name "F1", :active true, :parent_id (:id field-no-parent-2)})]]
+ fields-to-clean-up (atom [])
+ clean-up-fields! (fn []
+ (t2/delete! (t2/table-name Field) :id [:in (map :id @fields-to-clean-up)])
+ (reset! fields-to-clean-up []))]
+ (if (= driver/*driver* :postgres)
+ (testing "Before the migrations, Postgres does not allow fields to have the same table, name, but different parent_id"
+ (doseq [[defective? field-thunk] defective+field-thunk]
+ (if defective?
+ (is (thrown? Exception (field-thunk)))
+ (let [field (field-thunk)]
+ (is (some? field))
+ (swap! fields-to-clean-up conj field)))))
+ (testing "Before the migrations, all fields are allowed"
+ (doseq [[_ field-thunk] defective+field-thunk]
+ (let [field (field-thunk)]
+ (is (some? field))
+ (swap! fields-to-clean-up conj field)))))
+ (migrate!)
+ (clean-up-fields!)
+ (testing "After the migrations, only allow fields that have the same table, name, but different parent_id"
+ (doseq [[defective? field-thunk] defective+field-thunk]
+ (if defective?
+ (is (thrown? Exception (field-thunk)))
+ (let [field (field-thunk)]
+ (is (some? field))
+ (swap! fields-to-clean-up conj field)))))
+ (when (not= driver/*driver* :mysql) ; skipping MySQL because of rollback flakes (metabase#37434)
+ (testing "Migrate down succeeds"
+ (migrate! :down 48))
+ (clean-up-fields!)
+ (testing "After rolling back the migrations, all fields are allowed"
+ ;; Postgres' unique index is removed on rollback, so we can add defective fields
+ ;; This is needed to allow load-from-h2 to Postgres and then downgrading to work
+ (testing "After migrating down, all fields are allowed"
+ (doseq [[_ field-thunk] defective+field-thunk]
+ (is (some? (field-thunk))))))
+ (testing "Migrate up again succeeds"
+ (migrate!)))))))
+
+(deftest is-defective-duplicate-constraint-load-from-h2
+ (testing "Test that you can use load-from-h2 with fields that meet the conditions for is_defective_duplicate=TRUE"
+ ;; In this test:
+ ;; 1. starting from an H2 app DB, create a field that meets the conditions for is_defective_duplicate=TRUE
+ ;; 2. migrate, adding constraints around is_defective_duplicate to prevent duplicates
+ ;; 3. test load-from-h2 works successfully by migrating to MySQL or Postgres
+ ;; 4. test you can downgrade and upgrade again after that
+ (when-let [test-drivers (set/intersection (tx.env/test-drivers) #{:mysql :postgres})]
+ (mt/with-driver :h2
+ (impl/test-migrations-for-driver!
+ :h2
+ ["v49.2024-06-27T00:00:00" "v49.2024-06-27T00:00:08"]
+ (fn [migrate!]
+ (let [db-id (t2/insert-returning-pk! (t2/table-name Database)
+ {:details "{}"
+ :created_at :%now
+ :updated_at :%now
+ :engine "h2"
+ :is_sample false
+ :name ""})
+ table (t2/insert-returning-instance! (t2/table-name Table)
+ {:db_id db-id
+ :name (mt/random-name)
+ :created_at :%now
+ :updated_at :%now
+ :active true})
+ field! (fn [values]
+ (t2/insert-returning-instance! (t2/table-name Field)
+ (merge {:table_id (:id table)
+ :active true
+ :parent_id nil
+ :base_type "type/Text"
+ :database_type "TEXT"
+ :created_at :%now
+ :updated_at :%now}
+ values)))
+ _normal-field (field! {:name "F1", :parent_id nil})
+ create-defective-field! #(field! {:name "F1", :parent_id nil})
+ defective-field-id (:id (create-defective-field!))]
+ (testing "Before the migration, creating a defective duplicate field is allowed"
+ (is (some? defective-field-id)))
+ (migrate!)
+ (testing "After the migration, defective duplicate fields are not allowed"
+ (is (thrown-with-msg? clojure.lang.ExceptionInfo #"Unique index" (field! {:name "F1", :parent_id nil}))))
+ (mt/with-temp-dir [dir nil]
+ (let [h2-filename (str dir "/dump")]
+ (dump-to-h2/dump-to-h2! h2-filename) ; this migrates the DB back to the newest and creates a dump
+ (mt/test-drivers test-drivers
+ (let [db-def {:database-name "field-test-db"}
+ data-source (load-from-h2-test/get-data-source driver/*driver* db-def)]
+ (load-from-h2-test/create-current-database driver/*driver* db-def data-source)
+ (binding [mdb.connection/*application-db* (mdb.connection/application-db driver/*driver* data-source)]
+ (load-from-h2/load-from-h2! h2-filename)
+ (testing "The defective field should still exist after loading from H2"
+ (is (= #{defective-field-id}
+ (t2/select-pks-set (t2/table-name :model/Field) :is_defective_duplicate true)))))
+ (when (not= driver/*driver* :mysql) ; skipping MySQL because of rollback flakes (metabase#37434)
+ (testing "Migrating down to 48 should still work"
+ (migrate! :down 48))
+ (testing "The defective field should still exist after loading from H2 and downgrading"
+ (is (t2/exists? (t2/table-name :model/Field) :id defective-field-id)))
+ (testing "Migrating up again should still work"
+ (migrate!))))))))))))))
diff --git a/test/metabase/db/schema_migrations_test/impl.clj b/test/metabase/db/schema_migrations_test/impl.clj
index b6855a3d83390b3aeae657a38af9b625c9890051..0270b3853bdedf573430d7da0eb73d82d9a0e3a1 100644
--- a/test/metabase/db/schema_migrations_test/impl.clj
+++ b/test/metabase/db/schema_migrations_test/impl.clj
@@ -212,7 +212,7 @@
(.generateDeploymentId change-log-service)
(liquibase/update-with-change-log liquibase {:change-set-filters change-set-filters})))))
-(defn- test-migrations-for-driver! [driver [start-id end-id] f]
+(defn test-migrations-for-driver! [driver [start-id end-id] f]
(log/debug (u/format-color 'yellow "Testing migrations for driver %s..." driver))
(with-temp-empty-app-db [conn driver]
;; sanity check: make sure the DB is actually empty
diff --git a/test/metabase/native_query_analyzer_test.clj b/test/metabase/native_query_analyzer_test.clj
index 620f8f7dbecfc1878be5ca929ece69d3a8435e67..945cba410345cddcf72d1504e5c5211af4f4e356 100644
--- a/test/metabase/native_query_analyzer_test.clj
+++ b/test/metabase/native_query_analyzer_test.clj
@@ -1,7 +1,6 @@
(ns metabase.native-query-analyzer-test
(:require
[clojure.test :refer :all]
- [metabase.db.connection :as mdb.connection]
[metabase.driver :as driver]
[metabase.native-query-analyzer :as query-analyzer]
[metabase.public-settings :as public-settings]
@@ -39,13 +38,8 @@
(is (= {:explicit #{(mt/id :venues :id)} :implicit nil}
(q "select id from venues"))))
(testing "quotes stop case matching"
- ;; MySQL does case-insensitive string comparisons by default; quoting does not make it consider case
- ;; in field names either, so it's consistent behavior
- (if (= (mdb.connection/db-type) :mysql)
- (is (= {:explicit #{(mt/id :venues :id)} :implicit nil}
- (q "select \"id\" from venues")))
- (is (= {:explicit nil :implicit nil}
- (q "select \"id\" from venues"))))
+ (is (= {:explicit nil :implicit nil}
+ (q "select \"id\" from venues")))
(is (= {:explicit #{(mt/id :venues :id)} :implicit nil}
(q "select \"ID\" from venues"))))
(testing "you can mix quoted and unquoted names"