Fix model metadata editing for SQL models with manually sorted columns (#44574)

e2e/test/scenarios/models/reproductions.cy.spec.js

@@ -51,6 +51,7 @@ import {
   focusNativeEditor,
   tableHeaderClick,
   onlyOnOSS,
+  visitModel,
 } from "e2e/support/helpers";
 import {
   createMockActionParameter,
@@ -1806,3 +1807,57 @@ describe("issue 40252", () => {
     cy.findAllByTestId("header-cell").contains("Model B - A1 → B1Upd");
   });
 });
+
+describe("issue 42355", () => {
+  beforeEach(() => {
+    restore();
+    cy.signInAsNormalUser();
+  });
+
+  it("should allow overriding database fields for models with manually ordered columns (metabase#42355)", () => {
+    createNativeQuestion({
+      type: "model",
+      native: { query: "SELECT ID, PRODUCT_ID FROM ORDERS" },
+      visualization_settings: {
+        "table.columns": [
+          {
+            name: "PRODUCT_ID",
+            key: '["name","PRODUCT_ID"]',
+            enabled: true,
+            fieldRef: ["field", "PRODUCT_ID", { "base-type": "type/Integer" }],
+          },
+          {
+            name: "ID",
+            key: '["name","ID"]',
+            enabled: true,
+            fieldRef: ["field", "ID", { "base-type": "type/BigInteger" }],
+          },
+        ],
+        "table.cell_column": "ID",
+      },
+    }).then(({ body: card }) => visitModel(card.id));
+
+    cy.log("update metadata");
+    openQuestionActions();
+    popover().findByText("Edit metadata").click();
+    rightSidebar()
+      .findByText("Database column this maps to")
+      .next()
+      .findByText("None")
+      .click();
+    popover().within(() => {
+      cy.findByText("Orders").click();
+      cy.findByText("ID").click();
+    });
+    cy.button("Save changes").click();
+
+    cy.log("check metadata changes are visible");
+    openQuestionActions();
+    popover().findByText("Edit metadata").click();
+    rightSidebar()
+      .findByText("Database column this maps to")
+      .next()
+      .findByText("Orders → ID")
+      .should("be.visible");
+  });
+});

frontend/src/metabase-lib/v1/metadata/utils/models.ts

@@ -5,7 +5,7 @@ import { getQuestionVirtualTableId } from "metabase-lib/v1/metadata/utils/saved-
 import type NativeQuery from "metabase-lib/v1/queries/NativeQuery";
 import { isSameField } from "metabase-lib/v1/queries/utils/field-ref";
 import type {
-  DatasetColumn,
+  Field,
   FieldId,
   FieldReference,
   ModelCacheRefreshStatus,
@@ -148,20 +148,16 @@ export function getModelCacheSchemaName(databaseId: number, siteUUID: string) {
   return `metabase_cache_${firstLetters}_${databaseId}`;
 }
 
-type QueryField = FieldReference & { field_ref: FieldReference };
-
-function getFieldFromColumnVizSetting(
-  columnVizSetting: TableColumnOrderSetting,
-  columns: DatasetColumn[],
-  columnMetadata: QueryField[],
+function getFieldIndexFromColumnVizSetting(
+  column: Field,
+  columnSettings: TableColumnOrderSetting[],
 ) {
-  // We have some corrupted visualization settings where both names are mixed
-  // We should settle on `fieldRef`, make it required and remove `field_ref`
-  const fieldRef = columnVizSetting.fieldRef || columnVizSetting.field_ref;
-  return (
-    columns.find(column => isSameField(column.field_ref, fieldRef)) ||
-    columnMetadata.find(column => isSameField(column.field_ref, fieldRef))
-  );
+  return columnSettings.findIndex(columnSetting => {
+    // We have some corrupted visualization settings where both names are mixed
+    // We should settle on `fieldRef`, make it required and remove `field_ref`
+    const fieldRef = columnSetting.fieldRef || columnSetting.field_ref;
+    return isSameField(column.field_ref, fieldRef);
+  });
 }
 
 // Columns in resultsMetadata contain all the necessary metadata
@@ -171,29 +167,23 @@ function getFieldFromColumnVizSetting(
 // This ensures metadata rich columns are sorted correctly not to break the "Tab" key navigation behavior.
 export function getSortedModelFields(
   model: Question,
-  columnMetadata?: QueryField[],
+  columnMetadata: Field[] | undefined | null,
 ) {
   if (!Array.isArray(columnMetadata)) {
     return [];
   }
 
-  const orderedColumns = model.setting("table.columns");
-
-  if (!Array.isArray(orderedColumns)) {
+  const columnSettings = model.setting("table.columns");
+  if (!Array.isArray(columnSettings)) {
     return columnMetadata;
   }
 
-  const table = model.legacyQueryTable();
-  const tableFields = table?.fields ?? [];
-  const tableColumns = tableFields.map(field => field.column());
-
-  return orderedColumns
-    .map(columnVizSetting =>
-      getFieldFromColumnVizSetting(
-        columnVizSetting,
-        tableColumns,
-        columnMetadata,
-      ),
-    )
-    .filter(Boolean);
+  // always return metadata columns even if the corresponding viz settings don't exist
+  return columnMetadata
+    .map(column => ({
+      column,
+      index: getFieldIndexFromColumnVizSetting(column, columnSettings),
+    }))
+    .sort((a, b) => a.index - b.index)
+    .map(({ column }) => column);
 }