diff --git a/frontend/test/metabase/scenarios/downloads/reproductions/19889-native-query-export-column-order.cy.spec.js b/frontend/test/metabase/scenarios/downloads/reproductions/19889-native-query-export-column-order.cy.spec.js new file mode 100644 index 0000000000000000000000000000000000000000..35214d20ff3030d53a84ea8f751786f2f7ec01ff --- /dev/null +++ b/frontend/test/metabase/scenarios/downloads/reproductions/19889-native-query-export-column-order.cy.spec.js @@ -0,0 +1,83 @@ +import { restore, downloadAndAssert } from "__support__/e2e/cypress"; + +const questionDetails = { + name: "19889", + native: { + query: 'select 1 "column a", 2 "column b", 3 "column c"', + }, +}; + +const testCases = ["csv", "xlsx"]; + +describe("issue 19889", () => { + beforeEach(() => { + cy.intercept("POST", "/api/dataset").as("dataset"); + + restore(); + cy.signInAsAdmin(); + + cy.createNativeQuestion(questionDetails).then(({ body: { id } }) => { + cy.wrap(id).as("questionId"); + + cy.intercept("POST", `/api/card/${id}/query`).as("cardQuery"); + cy.visit(`/question/${id}`); + + cy.wait("@cardQuery"); + + // Reorder columns a and b + cy.findByText("column a") + .trigger("mousedown", 0, 0, { force: true }) + .trigger("mousemove", 5, 5, { force: true }) + .trigger("mousemove", 100, 0, { force: true }) + .trigger("mouseup", 100, 0, { force: true }); + cy.findByText("Started from").click(); // Give DOM some time to update + }); + }); + + testCases.forEach(fileType => { + it(`should order columns correctly in unsaved native query exports`, () => { + downloadAndAssert({ fileType, raw: true }, sheet => { + expect(sheet["A1"].v).to.equal("column b"); + expect(sheet["B1"].v).to.equal("column a"); + expect(sheet["C1"].v).to.equal("column c"); + }); + }); + + it(`should order columns correctly in saved native query exports`, () => { + saveAndOverwrite(); + + cy.get("@questionId").then(questionId => { + downloadAndAssert({ fileType, questionId, raw: true }, sheet => { + expect(sheet["A1"].v).to.equal("column b"); + expect(sheet["B1"].v).to.equal("column a"); + expect(sheet["C1"].v).to.equal("column c"); + }); + }); + }); + + it(`should order columns correctly in saved native query exports when the query was modified but not re-run before save (#19889)`, () => { + cy.contains(/open editor/i).click(); + cy.get(".ace_editor").type( + '{selectall}select 1 "column x", 2 "column y", 3 "column c"', + ); + + saveAndOverwrite(); + + cy.get("@questionId").then(questionId => { + cy.visit(`/question/${questionId}`); + cy.wait("@cardQuery"); + + downloadAndAssert({ fileType, questionId, raw: true }, sheet => { + expect(sheet["A1"].v).to.equal("column x"); + expect(sheet["B1"].v).to.equal("column y"); + expect(sheet["C1"].v).to.equal("column c"); + }); + }); + }); + }); +}); + +function saveAndOverwrite() { + cy.findByText("Save").click(); + cy.button("Save").click(); +} diff --git a/shared/src/metabase/shared/models/visualization_settings.cljc b/shared/src/metabase/shared/models/visualization_settings.cljc index df3afe50624e486b8c8bc2d014864bbc5be726a6..a1e64ea63373aa75fee1fc6051a975c91135816a 100644 --- a/shared/src/metabase/shared/models/visualization_settings.cljc +++ b/shared/src/metabase/shared/models/visualization_settings.cljc @@ -420,11 +420,12 @@ (set/map-invert db->norm-param-ref-keys)) (def ^:private db->norm-table-columns-keys - {:name ::table-column-name + {:name ::table-column-name ; for now, do not translate the value of this key (the field vector) - :fieldref ::table-column-field-ref - :fieldRef ::table-column-field-ref - :enabled ::table-column-enabled}) + :fieldref ::table-column-field-ref + :field_ref ::table-column-field-ref + :fieldRef ::table-column-field-ref + :enabled ::table-column-enabled}) (def ^:private norm->db-table-columns-keys (set/map-invert db->norm-table-columns-keys)) diff --git a/src/metabase/query_processor/streaming.clj b/src/metabase/query_processor/streaming.clj index 8ed184b00b9e5fbdb18d70234bc535d2475752a6..e8cfe536c44f9858d31e43d454daf9c08ec68128 100644 --- a/src/metabase/query_processor/streaming.clj +++ b/src/metabase/query_processor/streaming.clj @@ -35,14 +35,15 @@ (mbql.u/uniquify-names (map :name cols)))) (defn- validate-table-columms - "Validate that all of the field refs in `table-columns` correspond to actual columns in `cols`, if `cols` contains - field refs. Returns `nil` if any do not, so that we fall back to using `cols` directly for the export (#19465). + "Validate that all of the columns in `table-columns` correspond to actual columns in `cols`, correlating them by + field ref or name. Returns `nil` if any do not, so that we fall back to using `cols` directly for the export (#19465). Otherwise returns `table-columns`." [table-columns cols] - (let [col-field-refs (set (remove nil? (map :field_ref cols)))] - ;; If there are no field refs in `cols` (e.g. for native queries), we should use `table-columns` as-is - (when (or (empty? col-field-refs) - (every? (fn [table-col] (col-field-refs (::mb.viz/table-column-field-ref table-col))) table-columns)) + (let [col-field-refs (set (remove nil? (map :field_ref cols))) + col-names (set (remove nil? (map :name cols)))] + (when (every? (fn [table-col] (or (col-field-refs (::mb.viz/table-column-field-ref table-col)) + (col-names (::mb.viz/table-column-name table-col)))) + table-columns) table-columns))) (defn- export-column-order diff --git a/test/metabase/query_processor/streaming_test.clj b/test/metabase/query_processor/streaming_test.clj index 2d6ed8677c274a393d2248817c3bd8744bb94652..1f8e6b3a654b8044e4dd2b2b5d7b070db6b23e62 100644 --- a/test/metabase/query_processor/streaming_test.clj +++ b/test/metabase/query_processor/streaming_test.clj @@ -476,7 +476,12 @@ (@#'qp.streaming/export-column-order [{:id 0, :name "Col1" :field_ref [:field 0 nil]}] [{::mb.viz/table-column-field-ref [:field 1 nil], ::mb.viz/table-column-enabled true} - {::mb.viz/table-column-field-ref [:field 2 nil], ::mb.viz/table-column-enabled true}])))) + {::mb.viz/table-column-field-ref [:field 2 nil], ::mb.viz/table-column-enabled true}]))) + (is (= [0] + (@#'qp.streaming/export-column-order + [{:id 0, :name "Col1" :field_ref [:field 0 nil]}] + [{::mb.viz/table-column-name "Col1" , ::mb.viz/table-column-enabled true} + {::mb.viz/table-column-name "Col2" , ::mb.viz/table-column-enabled true}])))) (testing "if table-columns is nil, original order of cols is used" (is (= [0 1]