Fix QP remapping middleware not recursing into joins or source queries of source queries (#20154)

* Fix #9236 [WIP]

* Revert unneeded changes

* Only merge namespaced :options into result info

* Revert unneeded changes

* Everything is working 

* Revert unneeded change

* Fix #15578 part 2

* Fix #15578 (second attempt)

* Test fixes

* Sort namespaces

* Clean namespaces

* Add `:test` profile to `namespace-checker` to suppress log messages.

* PR feedback

* Fix namespaces

* Fix docstring

frontend/test/metabase/scenarios/joins/joins.cy.spec.js

@@ -14,7 +14,7 @@ describe("scenarios > question > joined questions", () => {
     cy.signInAsAdmin();
   });
 
-  it.skip("joining on a question with remapped values should work (metabase#15578)", () => {
+  it("joining on a question with remapped values should work (metabase#15578)", () => {
     cy.intercept("POST", "/api/dataset").as("dataset");
     // Remap display value
     cy.request("POST", `/api/field/${ORDERS.PRODUCT_ID}/dimension`, {

src/metabase/models/params/chain_filter.clj

@@ -391,7 +391,8 @@
                              ;; but sort by [remapped-value]
                              :order-by [[:asc [:field field-id nil]]]}))
                    (add-joins source-table-id joins)
-                   (add-filters source-table-id joined-table-ids constraints)))})
+                   (add-filters source-table-id joined-table-ids constraints)))
+   :middleware {:disable-remaps? true}})
 
 
 ;;; ------------------------ Chain filter (powers GET /api/dashboard/:id/params/:key/values) -------------------------

src/metabase/query_processor/middleware/add_dimension_projections.clj

@@ -25,6 +25,8 @@
   `name` is `:remapped_from` `:category_id`."
   (:require [clojure.data :as data]
             [clojure.tools.logging :as log]
+            [clojure.walk :as walk]
+            [medley.core :as m]
             [metabase.mbql.schema :as mbql.s]
             [metabase.mbql.util :as mbql.u]
             [metabase.models.dimension :refer [Dimension]]
@@ -166,31 +168,18 @@
   {:remaps (s/maybe (su/distinct [ExternalRemappingDimension]))
    :query  mbql.s/Query})
 
-(declare add-fk-remaps)
-
-(s/defn ^:private source-query-remaps :- QueryAndRemaps
-  [{{:keys [source-query]} :query, :as query}]
-  (if (and source-query (not (:native source-query)))
-    ;; Only do lifting if source is MBQL query
-    (let [{source-query-remaps :remaps, source-query :query} (add-fk-remaps (assoc query :query source-query))]
-      {:remaps source-query-remaps
-       :query  (assoc-in query [:query :source-query] (:query source-query))})
-    {:remaps nil, :query query}))
-
-(s/defn ^:private add-fk-remaps :- QueryAndRemaps
-  "Add any Fields needed for `:external` remappings to the `:fields` clause of the query, and update `:order-by` and
-  `breakout` clauses as needed. Returns a map with `:query` (the updated query) and `:remaps` (a sequence
-  of [[ExternalRemappingDimension]] information maps)."
-  [{{:keys [fields order-by breakout]} :query, :as query} :- mbql.s/Query]
-  (let [{source-query-remaps :remaps, query :query} (source-query-remaps query)]
+(defn- add-fk-remaps-one-level
+  [{:keys [fields order-by breakout], {source-query-remaps ::remaps} :source-query, :as query}]
+  (let [query (m/dissoc-in query [:source-query ::remaps])]
     ;; fetch remapping column pairs if any exist...
     (if-let [infos (not-empty (remap-column-infos (concat fields breakout)))]
       ;; if they do, update `:fields`, `:order-by` and `:breakout` clauses accordingly and add to the query
       (let [ ;; make a map of field-id-clause -> fk-clause from the tuples
             original->remapped             (into {} (map (juxt :original-field-clause :new-field-clause)) infos)
             existing-fields                (add-fk-remaps-rewrite-existing-fields infos fields)
-            ;; don't add any new entries for fields that already exist. Use [[mbql.u/remove-namespaced-options]] here so we don't
-            ;; add new entries even if the existing Field has some extra info e.g. extra unknown namespaced keys.
+            ;; don't add any new entries for fields that already exist. Use [[mbql.u/remove-namespaced-options]] here so
+            ;; we don't add new entries even if the existing Field has some extra info e.g. extra unknown namespaced
+            ;; keys.
             existing-normalized-fields-set (into #{} (map mbql.u/remove-namespaced-options) existing-fields)
             new-fields                     (into
                                             existing-fields
@@ -198,15 +187,32 @@
                                                   (remove (comp existing-normalized-fields-set mbql.u/remove-namespaced-options)))
                                             infos)
             new-breakout                   (add-fk-remaps-rewrite-breakout original->remapped breakout)
-            new-order-by                   (add-fk-remaps-rewrite-order-by original->remapped order-by)]
+            new-order-by                   (add-fk-remaps-rewrite-order-by original->remapped order-by)
+            remaps                         (into [] (comp cat (distinct)) [source-query-remaps (map :dimension infos)])]
         ;; return the Dimensions we are using and the query
-        {:remaps (into [] (comp cat (distinct)) [source-query-remaps (map :dimension infos)])
-         :query  (cond-> query
-                   (seq fields)   (assoc-in [:query :fields] new-fields)
-                   (seq order-by) (assoc-in [:query :order-by] new-order-by)
-                   (seq breakout) (assoc-in [:query :breakout] new-breakout))})
+        (cond-> query
+          (seq fields)   (assoc :fields new-fields)
+          (seq order-by) (assoc :order-by new-order-by)
+          (seq breakout) (assoc :breakout new-breakout)
+          (seq remaps)   (assoc ::remaps remaps)))
       ;; otherwise return query as-is
-      {:remaps source-query-remaps, :query query})))
+      (cond-> query
+        (seq source-query-remaps) (assoc ::remaps source-query-remaps)))))
+
+(s/defn ^:private add-fk-remaps :- QueryAndRemaps
+  "Add any Fields needed for `:external` remappings to the `:fields` clause of the query, and update `:order-by` and
+  `breakout` clauses as needed. Returns a map with `:query` (the updated query) and `:remaps` (a sequence
+  of [[ExternalRemappingDimension]] information maps)."
+  [query]
+  (let [query (walk/postwalk
+               (fn [form]
+                 (if (and (map? form)
+                          ((some-fn :source-table :source-query) form)
+                          (not (:condition form)))
+                   (add-fk-remaps-one-level form)
+                   form))
+               query)]
+    {:query (m/dissoc-in query [:query ::remaps]), :remaps (get-in query [:query ::remaps])}))
 
 (defn add-remapped-columns
   "Pre-processing middleware. For columns that have remappings to other columns (FK remaps), rewrite the query to

test/metabase/query_processor/middleware/add_dimension_projections_test.clj

@@ -444,3 +444,36 @@
                                       {:display_name "Sender ID [external remap]", :name "NAME", :remapped_from "SENDER_ID"}
                                       {:display_name "Receiver ID [external remap]", :name "NAME_2", :remapped_from "RECEIVER_ID"}]}
                               (#'add-dim-projections/add-remapped-to-and-from-metadata metadata remap-info nil)))))))))))
+
+(deftest add-remappings-inside-joins-test
+  (testing "Remappings should work inside joins (#15578)"
+    (mt/dataset sample-dataset
+      (mt/with-column-remappings [orders.product_id products.title]
+        (is (partial (mt/mbql-query products
+                       {:joins  [{:source-query {:source-table $$orders}
+                                  :alias        "Q1"
+                                  :fields       [&Q1.orders.id
+                                                 &Q1.orders.product_id
+                                                 &PRODUCTS__via__PRODUCT_ID.orders.product_id->title]
+                                  :condition    [:= $id &Q1.orders.product_id]
+                                  :strategy     :left-join}
+                                 {:source-table $$products
+                                  :alias        "PRODUCTS__via__PRODUCT_ID"
+                                  :condition    [:= $orders.product_id &PRODUCTS__via__PRODUCT_ID.products.id]
+                                  :strategy     :left-join
+                                  :fk-field-id  %orders.product_id}]
+                        :fields [&Q1.orders.id
+                                 &Q1.orders.product_id
+                                 &PRODUCTS__via__PRODUCT_ID.orders.product_id->products.title]
+                        :limit  2})
+                     (:query
+                      (#'add-dim-projections/add-fk-remaps
+                       (mt/mbql-query products
+                         {:joins  [{:strategy     :left-join
+                                    :source-query {:source-table $$orders}
+                                    :alias        "Q1"
+                                    :condition    [:= $id &Q1.orders.product_id]
+                                    :fields       [&Q1.orders.id
+                                                   &Q1.orders.product_id]}]
+                          :fields [&Q1.orders.id &Q1.orders.product_id]
+                          :limit  2})))))))))

test/metabase/query_processor_test/remapping_test.clj

 (ns metabase.query-processor-test.remapping-test
   "Tests for the remapping results"
   (:require [clojure.test :refer :all]
+            [metabase.driver :as driver]
+            [metabase.models.card :refer [Card]]
             [metabase.models.dimension :refer [Dimension]]
             [metabase.models.field :refer [Field]]
             [metabase.query-processor :as qp]
@@ -257,3 +259,64 @@
                       [2 8 3 "Bip bip bip bip" "Annie Albatross" "Peter Pelican"]
                       [3 3 2 "Coo"             "Peter Pelican"   "Lucky Pigeon"]]
                      (mt/rows results))))))))))
+
+(deftest remapped-columns-in-joined-source-queries-test
+  (mt/test-drivers (mt/normal-drivers-with-feature :nested-queries :left-join)
+    (testing "Remapped columns in joined source queries should work (#15578)"
+      (mt/dataset sample-dataset
+        (mt/with-bigquery-fks #{:bigquery :bigquery-cloud-sdk}
+          (mt/with-column-remappings [orders.product_id products.title]
+            (let [query (mt/mbql-query products
+                          {:joins    [{:source-query {:source-table $$orders
+                                                      :breakout     [$orders.product_id]
+                                                      :aggregation  [[:sum $orders.quantity]]}
+                                       :alias        "Orders"
+                                       :condition    [:= $id &Orders.orders.product_id]
+                                       :fields       [&Orders.title
+                                                      &Orders.*sum/Integer]}]
+                           :fields   [$title $category]
+                           :order-by [[:asc $id]]
+                           :limit    3})]
+              (mt/with-native-query-testing-context query
+                (let [results (qp/process-query query)]
+                  (when (= driver/*driver* :h2)
+                    (testing "Metadata"
+                      (is (= [["TITLE"    "Title"]
+                              ["CATEGORY" "Category"]
+                              ["TITLE_2"  "Orders → Title"]
+                              ["sum"      "Orders → Sum"]]
+                             (map (juxt :name :display_name) (mt/cols results))))))
+                  (is (= [["Rustic Paper Wallet"       "Gizmo"     "Rustic Paper Wallet"       347]
+                          ["Small Marble Shoes"        "Doohickey" "Small Marble Shoes"        352]
+                          ["Synergistic Granite Chair" "Doohickey" "Synergistic Granite Chair" 286]]
+                         (mt/formatted-rows [str str str int]
+                           results))))))))))))
+
+(deftest inception-style-nested-query-with-joins-test
+  (testing "source query > source query > query with join (with remappings) should work (#14724)"
+    ;; this error only seems to be triggered when actually using Cards as sources (and include the source metadata)
+    (mt/dataset sample-dataset
+      (mt/with-column-remappings [orders.product_id products.title]
+        ;; this is only triggered when using the results metadata from the Card itself --  see #19895
+        (let [q1 (mt/mbql-query orders
+                   {:fields   [$id $product_id]
+                    :joins    [{:source-table $$products
+                                :alias        "Products"
+                                :condition    [:= $product_id &Products.products.id]
+                                :fields       [$id
+                                               &Products.products.title]}]
+                    :order-by [[:asc $id]]
+                    :limit    3})]
+          (mt/with-temp Card [{card-1-id :id} {:dataset_query   q1
+                                               :result_metadata (get-in (qp/process-query q1)
+                                                                        [:data :results_metadata :columns])}]
+            (let [q2 (mt/mbql-query nil {:source-table (format "card__%d" card-1-id)})]
+              (mt/with-temp Card [{card-2-id :id} {:dataset_query   q2
+                                                   :result_metadata (get-in (qp/process-query q2)
+                                                                            [:data :results_metadata :columns])}]
+                (let [q3 (mt/mbql-query nil {:source-table (format "card__%d" card-2-id)})]
+                  (mt/with-native-query-testing-context q3
+                    (is (= [[1  14 "Awesome Concrete Shoes" "Awesome Concrete Shoes"]
+                            [2 123 "Mediocre Wooden Bench"  "Mediocre Wooden Bench"]
+                            [3 105 "Fantastic Wool Shirt"   "Fantastic Wool Shirt"]]
+                           (mt/rows (qp/process-query q3))))))))))))))