Merge pull request #9220 from metabase/add-unique-constraints-for-tables-and-fields

Add unique constraints: Table db_id/schema/name, Field table_id/parent_id/name

resources/migrations/000_migrations.yaml

@@ -5135,3 +5135,81 @@ databaseChangeLog:
             tableName: query_cache
             columnName: results
             newDataType: longblob
+
+#
+# Add unique constraints for (Field name + table_id + parent_id) and for (Table name + schema + db_id) unless for one
+# reason or another those would-be constraints are already violated. This is to fix issue where sometimes the same Field
+# or Table is synced more than once (see #669, #8950, #9048)
+#
+# Note that the SQL standard says unique constraints should not apply to columns with NULL values. Consider the following:
+#
+# INSERT INTO metabase_table (db_id, schema, name) VALUES (1, 'PUBLIC', 'my_table');
+# INSERT INTO metabase_table (db_id, schema, name) VALUES (1, 'PUBLIC', 'my_table'); -- fails: violates UNIQUE constraint
+#
+# INSERT INTO metabase_table (db_id, schema, name) VALUES (1, NULL, 'my_table');
+# INSERT INTO metabase_table (db_id, schema, name) VALUES (1, NULL, 'my_table'); -- succeeds: because schema is NULL constraint does not apply
+#
+# Thus these constraints won't work if the data warehouse DB in question doesn't use schemas (e.g. MySQL or MongoDB). It
+# will work for other data warehouse types.
+#
+# Luckily Postgres (but not H2 or MySQL) supports constraints that only apply to columns matching conditions, so we can
+# add additional constraints to properly handle those cases.
+  - changeSet:
+      id: 98
+      author: camsaul
+      comment: 'Added 0.32.0'
+      preConditions:
+        - onFail: MARK_RAN
+        - onUpdateSQL: IGNORE
+        - or:
+            - and:
+                - dbms:
+                    type: mysql,mariadb
+                - sqlCheck:
+                    expectedResult: 0
+                    sql: SELECT count(*) FROM (SELECT count(*) FROM `metabase_table` GROUP BY `db_id`, `schema`, `name` HAVING count(*) > 1) t1
+            - and:
+                - dbms:
+                    type: h2,postgresql
+                - sqlCheck:
+                    expectedResult: 0
+                    sql: SELECT count(*) FROM (SELECT count(*) FROM METABASE_TABLE GROUP BY DB_ID, SCHEMA, NAME HAVING count(*) > 1) t1
+      changes:
+        - addUniqueConstraint:
+            tableName: metabase_table
+            columnNames: db_id, schema, name
+            constraintName: idx_uniq_table_db_id_schema_name
+        # For Postgres, add additional constraint to apply if schema is NULL
+        - sql:
+            dbms: postgresql
+            sql: CREATE UNIQUE INDEX idx_uniq_table_db_id_schema_name_2col ON "metabase_table" ("db_id", "name") WHERE "schema" IS NULL
+
+  - changeSet:
+      id: 99
+      author: camsaul
+      comment: 'Added 0.32.0'
+      preConditions:
+        - onFail: MARK_RAN
+        - onUpdateSQL: IGNORE
+        - or:
+            - and:
+                - dbms:
+                    type: mysql,mariadb
+                - sqlCheck:
+                    expectedResult: 0
+                    sql: SELECT count(*) FROM (SELECT count(*) FROM `metabase_field` GROUP BY `table_id`, `parent_id`, `name` HAVING count(*) > 1) t1
+            - and:
+                - dbms:
+                    type: h2,postgresql
+                - sqlCheck:
+                    expectedResult: 0
+                    sql: SELECT count(*) FROM (SELECT count(*) FROM METABASE_FIELD GROUP BY TABLE_ID, PARENT_ID, NAME HAVING count(*) > 1) t1
+      changes:
+        - addUniqueConstraint:
+            tableName: metabase_field
+            columnNames: table_id, parent_id, name
+            constraintName: idx_uniq_field_table_id_parent_id_name
+        # For Postgres, add additional constraint to apply if schema is NULL
+        - sql:
+            dbms: postgresql
+            sql: CREATE UNIQUE INDEX idx_uniq_field_table_id_parent_id_name_2col ON "metabase_field" ("table_id", "name") WHERE "parent_id" IS NULL

test/metabase/query_processor_test/aggregation_test.clj

@@ -332,11 +332,9 @@
 (expect
   (assoc (aggregate-col :sum (Field (data/id :venues :price)))
     :settings {:is_priceless false})
-  (tt/with-temp Field [copy-of-venues-price (-> (Field (data/id :venues :price))
-                                                (dissoc :id)
-                                                (assoc :settings {:is_priceless false}))]
+  (tu/with-temp-vals-in-db Field (data/id :venues :price) {:settings {:is_priceless false}}
     (let [results (data/run-mbql-query venues
-                    {:aggregation [[:sum [:field-id (u/get-id copy-of-venues-price)]]]})]
+                    {:aggregation [[:sum [:field-id $price]]]})]
       (or (-> results :data :cols first)
           results))))
 

test/metabase/sync/analyze/table_row_count_test.clj

 (ns metabase.sync.analyze.table-row-count-test
   "Tests for the sync logic that updates a Table's row count."
-  (:require [metabase
-             [query-processor-test :as qp-test]
-             [util :as u]]
-            [metabase.models.table :refer [Table]]
+  (:require [metabase.models.table :refer [Table]]
+            [metabase.query-processor-test :as qp-test]
             [metabase.sync.analyze.table-row-count :as table-row-count]
-            [metabase.test.data :as data]
+            [metabase.test
+             [data :as data]
+             [util :as tu]]
             [metabase.test.data.datasets :as datasets]
-            [toucan.db :as db]
-            [toucan.util.test :as tt]))
+            [toucan.db :as db]))
 
 ;; test that syncing table row counts works
 ;; TODO - write a Druid version of this test. Works slightly differently since Druid doesn't have a 'venues' table
 ;; TODO - not sure why this doesn't work on Oracle. Seems to be an issue with the test rather than with the Oracle driver
 (datasets/expect-with-drivers (disj qp-test/non-timeseries-drivers :oracle)
   100
-  (tt/with-temp Table [venues-copy (let [venues-table (Table (data/id :venues))]
-                                     (assoc (select-keys venues-table [:schema :name :db_id])
-                                       :rows 0))]
-    (table-row-count/update-row-count! venues-copy)
-    (db/select-one-field :rows Table :id (u/get-id venues-copy))))
+  (tu/with-temp-vals-in-db Table (data/id :venues) {:rows 0}
+      (table-row-count/update-row-count! (Table (data/id :venues)))
+      (db/select-one-field :rows Table :id (data/id :venues))))