Update field positions on sync if the fields should be ordered by "Database" (#30446)

* Update the field position on sync if the table's fields should be ordered by "database"

* Fix test

* Select position in the current field metadata

* Fix test

src/metabase/sync/interface.clj

@@ -27,6 +27,7 @@
    :database-type                               (s/maybe su/NonBlankString) ; blank if the Field is all NULL & untyped, i.e. in Mongo
    :base-type                                   su/FieldType
    :database-position                           su/IntGreaterThanOrEqualToZero
+   (s/optional-key :position)                   su/IntGreaterThanOrEqualToZero
    (s/optional-key :semantic-type)              (s/maybe su/FieldSemanticOrRelationType)
    (s/optional-key :effective-type)             (s/maybe su/FieldType)
    (s/optional-key :coercion-strategy)          (s/maybe su/CoercionStrategy)

src/metabase/sync/sync_metadata/fields/fetch_metadata.clj

@@ -35,6 +35,7 @@
           :field-comment             (:description field)
           :json-unfolding            (:json_unfolding field)
           :database-is-auto-increment (:database_is_auto_increment field)
+          :position                  (:position field)
           :database-position         (:database_position field)
           :database-required         (:database_required field)})
        ;; make a map of parent-id -> set of child Fields
@@ -71,7 +72,7 @@
   [table :- i/TableInstance]
  (t2/select [Field :name :database_type :base_type :effective_type :coercion_strategy :semantic_type
              :parent_id :id :description :database_position :nfc_path :database_is_auto_increment :database_required
-             :json_unfolding]
+              :json_unfolding :position]
      :table_id  (u/the-id table)
      :active    true
      {:order-by table/field-order-rule}))

src/metabase/sync/sync_metadata/fields/sync_metadata.clj

@@ -24,6 +24,7 @@
          old-field-comment              :field-comment
          old-semantic-type              :semantic-type
          old-database-position          :database-position
+         old-position                   :position
          old-database-name              :name
          old-database-is-auto-increment :database-is-auto-increment
          old-db-required                :database-required} metabase-field
@@ -95,6 +96,13 @@
                           old-database-position
                           new-database-position))
            {:database_position new-database-position})
+         (when (and (= (:field_order table) :database)
+                    (not= old-position new-database-position))
+           (log/info (trs "Position of {0} has changed from ''{1}'' to ''{2}''."
+                          (common/field-metadata-name-for-logging table metabase-field)
+                          old-position
+                          new-database-position))
+           {:position new-database-position})
          (when new-name?
            (log/info (trs "Name of {0} has changed from ''{1}'' to ''{2}''."
                           (common/field-metadata-name-for-logging table metabase-field)

test/metabase/sync/sync_metadata/fields/fetch_metadata_test.clj

@@ -105,6 +105,6 @@
                                                                    %
                                                                    ;; database-position isn't stable since they are
                                                                    ;; defined in sets. changing keys will change the
-                                                                   ;; order in the set implementation
-                                                                   (m/filter-vals some? (dissoc % :id :database-position))))]
+                                                                   ;; order in the set implementation. (and position depends on database-position)
+                                                                   (m/filter-vals some? (dissoc % :id :database-position :position))))]
              (remove-ids-and-nil-vals (#'fetch-metadata/our-metadata (t2/select-one Table :id transactions-table-id))))))))

test/metabase/sync/sync_metadata/fields/sync_metadata_test.clj

@@ -6,17 +6,21 @@
    [toucan.util.test :as tt]
    [toucan2.core :as t2]))
 
-(defn- updates-that-will-be-performed [new-metadata-from-sync metadata-in-application-db]
-  (tt/with-temp Table [table]
-    (let [update-operations (atom [])]
-      (with-redefs [t2/update! (fn [model id updates]
-                                 (swap! update-operations conj [(name model) id updates])
-                                 (count updates))]
-        (#'sync-metadata/update-field-metadata-if-needed!
-         table
-         new-metadata-from-sync
-         metadata-in-application-db)
-        @update-operations))))
+(defn- updates-that-will-be-performed
+  ([new-metadata-from-sync metadata-in-application-db]
+   ;; use alphabetical field_order by default because the default, database, will update the position
+   (updates-that-will-be-performed new-metadata-from-sync metadata-in-application-db {:field_order :alphabetical}))
+  ([new-metadata-from-sync metadata-in-application-db table]
+   (tt/with-temp Table [table table]
+     (let [update-operations (atom [])]
+       (with-redefs [t2/update! (fn [model id updates]
+                                  (swap! update-operations conj [(name model) id updates])
+                                  (count updates))]
+         (#'sync-metadata/update-field-metadata-if-needed!
+          table
+          new-metadata-from-sync
+          metadata-in-application-db)
+         @update-operations)))))
 
 (deftest database-type-changed-test
   (testing "test that if database-type changes we will update it in the DB"
@@ -36,6 +40,33 @@
              :database-required          false
              :database-is-auto-increment false})))))
 
+(def ^:private default-metadata
+  {:name                       "My Field"
+   :database-type              "Integer"
+   :base-type                  :type/Integer
+   :database-position          0
+   :database-required          false
+   :database-is-auto-increment false})
+
+(deftest database-position-changed-test
+  (testing "test that if database-position changes and table.field_order=database we will update the position too"
+    (is (= [["Field" 1 {:database_position 1
+                        :position          1}]]
+           (updates-that-will-be-performed
+            (merge default-metadata {:database-position 1})
+            (merge default-metadata {:database-position 0
+                                     :position          0
+                                     :id                1})
+            {:field_order :database})))
+    (testing "but not if the table's fields should not be sorted according to the database"
+      (is (= [["Field" 1 {:database_position 1}]]
+             (updates-that-will-be-performed
+              (merge default-metadata {:database-position 1})
+              (merge default-metadata {:database-position 0
+                                       :position          0
+                                       :id                1})
+              {:field_order :alphabetical}))))))
+
 (deftest database-required-changed-test
   (testing "test that if database-required changes we will update it in the DB"
     (is (= [["Field" 1 {:database_required false}]]
@@ -50,6 +81,7 @@
              :database-type              "Integer"
              :base-type                  :type/Integer
              :id                         1
+             :position                   0
              :database-position          0
              :database-required          true
              :database-is-auto-increment false})))))