Refactor field ordering logic in exports to avoid dropped fields (#18441)

frontend/test/metabase/scenarios/downloads/reproductions/18440-remapped-display-value-dropped.cy.spec.js

@@ -18,7 +18,7 @@ const testCases = [
   { type: "xlsx", sheetName: "Query result" },
 ];
 
-describe.skip("issue 18440", () => {
+describe("issue 18440", () => {
   beforeEach(() => {
     cy.intercept("POST", "/api/card").as("saveQuestion");
 

src/metabase/query_processor/streaming.clj

@@ -44,9 +44,14 @@
   The issue currently is that `table-columns` is passed down from the frontend for unsaved cards (in the viz settings)
   and has to be parsed from JSON, so some fields in metadata might be strings instead of keywords."
   [field-ref]
-  (if-let [join-alias (:join-alias (last field-ref))]
-    [(keyword (first field-ref)) (second field-ref) {:join-alias join-alias}]
-    [(keyword (first field-ref)) (second field-ref)]))
+  (let [field-ref-first-part (keyword (first field-ref))
+        key-first-part       (if (= :aggregation field-ref-first-part)
+                               :aggregation
+                               ;; Convert the deprecated :field-id to :field so that it matches the field refs in `cols` (#18382)
+                               :field)]
+    (if-let [join-alias (:join-alias (last field-ref))]
+      [key-first-part (second field-ref) {:join-alias join-alias}]
+      [key-first-part (second field-ref)])))
 
 (defn- export-column-order
   "For each entry in `table-columns` that is enabled, finds the index of the corresponding
@@ -64,14 +69,19 @@
                                     ::mb.viz/table-column-enabled true})))
         enabled-table-cols (filter ::mb.viz/table-column-enabled table-columns')
         cols-vector        (into [] cols)
+        ;; cols-index is a map from keys representing fields to their indices into `cols`
         cols-index         (reduce-kv (fn [m i col]
-                                        (if-let [field-ref (:field_ref col)]
-                                          ;; Use first two entries of field-ref, if available
-                                          (assoc m (field-ref->map-key field-ref) i)
-                                          ;; Otherwise construct a key using the name and/or id of the column
-                                          (let [m' (assoc m [:field (:name col)] i)]
+                                        ;; Always add [:field col-name] as a key, so that native queries,
+                                        ;; remapped fields, and old fields using :field-literal work correctly (#18382)
+                                        (let [m' (assoc m [:field (:name col)] i)]
+                                          (if-let [field-ref (:field_ref col)]
+                                            ;; Add a map key based on the column's field-ref, if available
+                                            (assoc m' (field-ref->map-key field-ref) i)
+
+                                            ;; Otherwise construct a key using the id of the column, if available
                                             (if (:id col)
-                                              (assoc m' [:field (:id col)] i) m'))))
+                                              (assoc m' [:field (:id col)] i)
+                                              m'))))
                                       {}
                                       cols-vector)]
     (->> (map

test/metabase/api/dataset_test.clj

@@ -227,6 +227,24 @@
             (is (= 101
                    (count (csv/read-csv result))))))))))
 
+(deftest export-with-remapped-fields
+  (testing "POST /api/dataset/:format"
+    (testing "Downloaded CSV/JSON/XLSX results should respect remapped fields (#18440)"
+      (let [query (json/generate-string {:database (mt/id)
+                                         :type     :query
+                                         :query    {:source-table (mt/id :venues)
+                                                    :limit 1}
+                                         :middleware
+                                         {:add-default-userland-constraints? true
+                                          :userland-query?                   true}})]
+        (mt/with-column-remappings [venues.category_id categories.name]
+          (let [result (mt/user-http-request :rasta :post 200 "dataset/csv"
+                                             :query query)]
+            (is (str/includes? result "Asian"))))
+        (mt/with-column-remappings [venues.category_id (values-of categories.name)]
+          (let [result (mt/user-http-request :rasta :post 200 "dataset/csv"
+                                             :query query)]
+            (is (str/includes? result "Asian"))))))))
 
 (deftest non--download--queries-should-still-get-the-default-constraints
   (testing (str "non-\"download\" queries should still get the default constraints "

test/metabase/query_processor/streaming_test.clj

@@ -317,6 +317,18 @@
             [{::mb.viz/table-column-field-ref ["expression" "Name1"], ::mb.viz/table-column-enabled true}
              {::mb.viz/table-column-field-ref ["expression" "Name0"], ::mb.viz/table-column-enabled true}]))))
 
+  (testing "correlation of fields using deprecated field dimensions"
+    (is (= [1 0]
+           (@#'qp.streaming/export-column-order
+            [{:id 0, :field_ref [:field_literal "Name0"]}, {:id 1, :field_ref [:field_literal "Name1"]}]
+            [{::mb.viz/table-column-field-ref ["field_literal" "Name1"], ::mb.viz/table-column-enabled true}
+             {::mb.viz/table-column-field-ref ["field_literal" "Name0"], ::mb.viz/table-column-enabled true}])))
+    (is (= [1 0]
+           (@#'qp.streaming/export-column-order
+            [{:id 0, :field_ref [:field_id 0]}, {:id 1, :field_ref [:field_id 1]}]
+            [{::mb.viz/table-column-field-ref ["field_id" 1], ::mb.viz/table-column-enabled true}
+             {::mb.viz/table-column-field-ref ["field_id" 0], ::mb.viz/table-column-enabled true}]))))
+
   (testing "disabled columns are excluded from ordering"
     (is (= [0]
            (@#'qp.streaming/export-column-order
@@ -332,7 +344,8 @@
   (testing "remapped columns use the index of the new column"
     (is (= [1]
            (@#'qp.streaming/export-column-order
-            [{:id 0, :name "Col1", :remapped_to "Col2"}, {:id 1, :name "Col2", :remapped_from "Col1"}]
+            [{:id 0, :name "Col1", :remapped_to "Col2", :field_ref ["field" 0 nil]},
+             {:id 1, :name "Col2", :remapped_from "Col1", :field_ref ["field" 1 nil]}]
             [{::mb.viz/table-column-field-ref ["field" 0 nil], ::mb.viz/table-column-enabled true}]))))
 
   (testing "entries in table-columns without corresponding entries in cols are ignored"