the point I gave up

This reverts commit a3bca401d6.

resources/migrations/001_update_migrations.yaml

@@ -6542,6 +6542,9 @@ databaseChangeLog:
       comment: Populate metabase_field.is_defective_duplicate
       changes:
         - sql:
+            # idx_uniq_field_table_id_parent_id_name_2col would prevent defective duplicates with Postgres but it
+            # can be removed by upgrading from 49 and downgrading again. We need to populate is_defective_duplicate
+            # in that case.
             sql: >-
               UPDATE metabase_field mf
               SET is_defective_duplicate = TRUE
@@ -6688,6 +6691,26 @@ databaseChangeLog:
             tableName: metabase_field
             constraintName: idx_unique_field
 
+  - changeSet:
+      id: v49.2024-06-27T00:00:09
+      author: calherries
+      comment: Set is_defective_duplicate=TRUE fields that shouldn't exist to have active=FALSE
+      changes:
+        - sql:
+            sql: >-
+              UPDATE metabase_field
+              SET active = false
+              WHERE is_defective_duplicate AND (nfc_path = concat('["', name, '"]') OR nfc_path IS NULL);
+      rollback: # No rollback needed since this is backward compatible
+
+  - changeSet:
+      id: v49.2024-06-27T00:00:10
+      author: calherries
+      comment: Replace inactive fields with is_defective_duplicate=TRUE with their non-defective counterparts
+      changes:
+        - customChange:
+            class: "metabase.db.custom_migrations.ReplaceDefectiveDuplicateFields"
+
   - changeSet:
       id: v50.2024-01-04T13:52:51
       author: noahmoss

src/metabase/db/custom_migrations.clj

@@ -19,10 +19,13 @@
    [clojurewerkz.quartzite.jobs :as jobs]
    [clojurewerkz.quartzite.scheduler :as qs]
    [clojurewerkz.quartzite.triggers :as triggers]
+   [malli.core :as mc]
    [medley.core :as m]
    [metabase.config :as config]
    [metabase.db.connection :as mdb.connection]
    [metabase.db.custom-migrations.metrics-v2 :as metrics-v2]
+   [metabase.legacy-mbql.schema :as mbql.s]
+   [metabase.models.interface :as mi]
    [metabase.plugins.classloader :as classloader]
    [metabase.util.date-2 :as u.date]
    [metabase.util.encryption :as encryption]
@@ -1465,3 +1468,114 @@
     (run! decrypt! ["query-caching-ttl-ratio"
                     "query-caching-min-ttl"
                     "enable-query-caching"])))
+
+(defn- replace-field [s from to]
+  (-> s
+      (str/replace (re-pattern (str "\"source-field\":" from "(,|})"))
+                   (str "\"source-field\":" to "$1"))
+      (str/replace (re-pattern (str "\\[\"field\"," from ","))
+                   (str "[\"field\"," to ","))))
+
+(defn- references-field? [s field-id]
+  (or (re-find (re-pattern (str "\"source-field\":" field-id "(,|})")) s)
+      (re-find (re-pattern (str "\\[\"field\"," field-id ",")) s)))
+
+(def from 72)
+(def from 37) ;; normal references
+(def to 73)
+(def from 40) ;; source-field ref
+(def to 43)   ;; source-field ref
+
+(def from 43) ;; source-field ref
+(def to 40)   ;; source-field ref
+(:dataset_query (t2/select-one :model/Card 6))
+
+(defn- migrate []
+  (when-some [from+to (not-empty
+                       (t2/query
+                        {:select [[:fr.id :from] [:to.id :to]]
+                         :from   [[:metabase_field :fr]]
+                         :join   [[:metabase_field :to]
+                                  [:and
+                                   [:= :fr.name :to.name]
+                                   [:= :fr.table_id :to.table_id]
+                                   [:= :to.active true]]]
+                         :where  [:and
+                                  [:= :fr.is_defective_duplicate true]
+                                  [:raw "(fr.nfc_path = concat('[\"', fr.name, '\"]') OR fr.nfc_path IS NULL)"]]}))]
+   (doseq [{:keys [from to]} from+to]
+     ;; TODO make this reducible
+     (->> (t2/query {:select [:id :dataset_query :result_metadata :visualization_settings]
+                     :from   [:report_card]
+                     :where  [:and
+                              [:= :query_type [:inline "query"]]
+                              [:raw (case (mdb.connection/db-type)
+                                      :postgres
+                                      (str "(dataset_query ~ concat('\"source-field\":', " from ") OR"
+                                           " dataset_query ~ concat('\\[\"field\",', " from "))")
+                                      :mysql
+                                      (str "(dataset_query REGEXP CONCAT('\"source-field\":', " from ") OR"
+                                           " dataset_query REGEXP CONCAT('\\\\[\"field\",', " from "))")
+                                      :h2
+                                      (str "(dataset_query REGEXP concat('\"source-field\":', " from ") OR"
+                                           " dataset_query REGEXP concat('\\[\"field\",', " from "))"))]]})
+          (run! (fn [{:keys [id dataset_query result_metadata visualization_settings]}]
+                  ;; if there are any references to `to` field, we don't want to replace `from`, because it could create
+                  ;; duplicate references to the same field in the same clause, which is invalid.
+                  (when (not (references-field? dataset_query to))
+                    (let [dataset_query          (replace-field dataset_query from to)
+                          ;; check that the card passes query validation
+                          parsed-dataset-query   ((:out mi/transform-metabase-query) dataset_query)
+                          _ (sc.api/spy)
+                          _ (cal/spy (mc/validate mbql.s/MBQLQuery parsed-dataset-query))
+                          valid-dataset-query?   (boolean (try (mc/validate mbql.s/MBQLQuery parsed-dataset-query)
+                                                               (catch Exception _e
+                                                                 ;; TODO: remove this before merging
+                                                                 (prn "Failed to validate query" parsed-dataset-query)
+                                                                 false)))
+                             ;; TODO: visualization_settings & result_metadata
+                          ]
+                      (when valid-dataset-query?
+                        (t2/query-one {:update :report_card
+                                       :set    {:dataset_query          dataset_query
+                                                :result_metadata        result_metadata
+                                                :visualization_settings visualization_settings}
+                                       :where  [:= :id id]}))))))))))
+
+(define-migration ReplaceDefectiveDuplicateFields
+  (when-let [defective-fields (t2/query {:select [:id :name :table_id]
+                                         :from   [:metabase_field]
+                                         :where  [:and
+                                                  [:= :is_defective_duplicate true]
+                                                  [:raw "(nfc_path = concat('[\"', name, '\"]') OR nfc_path IS NULL)"]]})]
+    (doseq [from defective-fields]
+      (when-let [to ]
+        (->> (t2/reducible-query {:select [:id :dataset_query :result_metadata :visualization_settings]
+                                  :from   [:report_card]
+                                  :where  [:and
+                                           [:= :query_type "query"]
+                                           [:raw (case (mdb.connection/db-type)
+                                                   :postgres
+                                                   (str "(dataset_query ~ concat('\"source-field\":', " (:name from) ") OR"
+                                                        " dataset_query ~ concat('\\[\"field\",' " (:name from) "))")
+                                                   :mysql
+                                                   (str "(dataset_query REGEXP CONCAT('\"source-field\":', " (:name from) ") OR"
+                                                        " dataset_query REGEXP CONCAT('\\\\[\"field\",' " (:name from) "))")
+                                                   :h2
+                                                   (str "(dataset_query REGEXP concat('\"source-field\":', " (:name from) ") OR"
+                                                        " dataset_query REGEXP concat('\\[\"field\",' " (:name from) "))"))]]})
+             (run! (fn [{:keys [id dataset_query result_metadata visualization_settings]}]
+                     (let [;; TODO: check there is no reference to the 'to' field in the dataset_query
+                           dataset_query          (replace-field dataset_query (:id from) (:id to))
+                             ;; check that the card passes query validation
+                           validate-dataset-query (:out mi/transform-metabase-query)
+                           valid-dataset-query?   (boolean (try (mc/validate dataset_query)
+                                                                (catch Exception _e false)))
+                             ;; TODO: visualization_settings & result_metadata
+                           ]
+                       (when valid-dataset-query?
+                         (t2/query-one {:update :report_card
+                                        :set    {:dataset_query          dataset_query
+                                                 :result_metadata        result_metadata
+                                                 :visualization_settings visualization_settings}
+                                        :where  [:= :id id]}))))))))))

test/metabase/db/custom_migrations_test.clj

@@ -2017,3 +2017,65 @@
       (is (= "true" (t2/select-one-fn :value :setting :key "enable-query-caching")))
       (is (= "100" (t2/select-one-fn :value :setting :key "query-caching-ttl-ratio")))
       (is (= "123" (t2/select-one-fn :value :setting :key "query-caching-min-ttl"))))))
+
+(def table (t2/select-one :model/Table))
+(t2/delete! :model/Field 77)
+(t2/insert-returning-instance! (t2/table-name :model/Field)
+                               {:table_id               (:id table)
+                                :name                   "F1"
+                                :is_defective_duplicate true
+                                :parent_id              nil
+                                :nfc_path               "[\"F1\"]"
+                                :base_type              "type/Text"
+                                :database_type          "TEXT"
+                                :created_at             :%now
+                                :updated_at             :%now
+                                :active                 true})
+
+;; ReplaceDefectiveDuplicateFields
+(let [(def db-id (t2/insert-returning-pk! :metabase_database
+                                          {:details    "{}"
+                                           :created_at :%now
+                                           :updated_at :%now
+                                           :engine     "h2"
+                                           :is_sample  false
+                                           :name       "some_db"}))
+      (def table (t2/insert-returning-instance! :metabase_table
+                                                {:db_id      db-id
+                                                 :name       "some_table"
+                                                 :created_at :%now
+                                                 :updated_at :%now
+                                                 :active     true}))
+      (def field (t2/insert-returning-instance! :metabase_field
+                                                {:table_id               (:id table)
+                                                 :name                   "F1"
+                                                 :is_defective_duplicate true
+                                                 :parent_id              nil
+                                                 :nfc_path               "[\"F1\"]"
+                                                 :base_type              "type/Text"
+                                                 :database_type          "TEXT"
+                                                 :created_at             :%now
+                                                 :updated_at             :%now
+                                                 :active                 true}))
+      (def field-ref [:field (:id field) {:base-type "type/Integer"}])
+      (def card (t2/insert-returning-instance!
+                 :model/Card
+                 {:name                   "some_card"
+                  :type                   "question"
+                  :creator_id             1
+                  :database_id            db-id
+                  :created_at             :%now
+                  :updated_at             :%now
+                  :collection_id          nil
+                  :description            nil
+                  :display                "table"
+                  :visualization_settings "{}"
+                  :dataset_query (json/generate-string {:database db-id,
+                                                        :type     :query,
+                                                        :query    {:source-table (:id table),
+                                                                   :aggregation  [[:count]],
+                                                                   :expressions  {"ID concat" [:concat field-ref field-ref]},
+                                                                   :breakout     [field-ref
+                                                                                  [:expression "ID concat" {:base-type "type/Text"}]],
+                                                                   :filter       [:= field-ref 1]}})}))]
+  )

test/metabase/db/schema_migrations_test.clj

@@ -2222,7 +2222,7 @@
           ;; 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)
+        (let [db-id (t2/insert-returning-pk! :metabase_database
                                              {:details    "{}"
                                               :created_at :%now
                                               :updated_at :%now
@@ -2230,14 +2230,14 @@
                                               :is_sample  false
                                               :name       "populate-is-defective-duplicate-test-db"})
               table! (fn []
-                       (t2/insert-returning-instance! (t2/table-name Table)
+                       (t2/insert-returning-instance! :metabase_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)
+                       (t2/insert-returning-instance! :metabase_field
                                                       (merge {:table_id      (:id table)
                                                               :parent_id     nil
                                                               :base_type     "type/Text"
@@ -2278,7 +2278,7 @@
                        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))]
+                                         (let [field-after (t2/select-one :metabase_field :id (:id field-before))]
                                            (is (= defective? (:is_defective_duplicate field-after))))))]
           (migrate!)
           (testing "1. Active is 1st preference"
@@ -2298,21 +2298,21 @@
 (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)
+      (let [db-id (t2/insert-returning-pk! :metabase_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)
+            table (t2/insert-returning-instance! :metabase_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)
+                     (t2/insert-returning-instance! :metabase_field
                                                     (merge {:table_id      (:id table)
                                                             :parent_id     nil
                                                             :base_type     "type/Text"
@@ -2329,7 +2329,7 @@
                                    [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)])
+                                    (t2/delete! :metabase_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"
@@ -2379,21 +2379,21 @@
          :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)
+           (let [db-id (t2/insert-returning-pk! :metabase_database
                                                 {:details    "{}"
                                                  :created_at :%now
                                                  :updated_at :%now
                                                  :engine     "h2"
                                                  :is_sample  false
                                                  :name       ""})
-                 table (t2/insert-returning-instance! (t2/table-name Table)
+                 table (t2/insert-returning-instance! :metabase_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)
+                          (t2/insert-returning-instance! :metabase_field
                                                          (merge {:table_id      (:id table)
                                                                  :active        true
                                                                  :parent_id     nil
@@ -2429,3 +2429,37 @@
                          (is (t2/exists? (t2/table-name :model/Field) :id defective-field-id)))
                        (testing "Migrating up again should still work"
                          (migrate!))))))))))))))
+
+(deftest deactivate-defective-duplicates-test
+  (testing "Migration v49.2024-06-27T00:00:09"
+    (impl/test-migrations ["v49.2024-06-27T00:00:09"] [migrate!]
+      (let [db-id         (t2/insert-returning-pk! :metabase_database
+                                                   {:details    "{}"
+                                                    :created_at :%now
+                                                    :updated_at :%now
+                                                    :engine     "h2"
+                                                    :is_sample  false
+                                                    :name       "some_db"})
+            table         (t2/insert-returning-instance! :metabase_table
+                                                         {:db_id      db-id
+                                                          :name       "some_table"
+                                                          :created_at :%now
+                                                          :updated_at :%now
+                                                          :active     true})
+            field!        (fn [values]
+                            (t2/insert-returning-instance! :metabase_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)))
+            active+field [[true  (field! {:name "x",  :is_defective_duplicate true, :nfc_path "[\"x\",\"y\""})]
+                          [false (field! {:name "x", :is_defective_duplicate true, :nfc_path nil})]
+                          [false (field! {:name "x", :is_defective_duplicate true, :nfc_path "[\"x\"]"})]]]
+        (migrate!)
+        (testing "After the migration, fields are deactivated correctly"
+          (doseq [[active? field] active+field]
+            (is (= active? (t2/select-one-fn :active :metabase_field (:id field))))))))))