From ff9b99d0b929f311624e3be122cb1f9d0d5d5d0f Mon Sep 17 00:00:00 2001
From: Pawit Pornkitprasan <pawit@metabase.com>
Date: Wed, 20 Oct 2021 10:22:37 +0700
Subject: [PATCH] fix wrong drill down query when using nested query (#17942)
* qp: fetch_source_query: store card-id for each query
we want to be able to determine further in the pipeline
whether a query came from a card (i.e. saved question)
or not
* qp: annotate: remove join-alias if source is a card
If the source is a card, the front-end should be able to treat it
similar to a database view so we should not expose the join
aliases outside.
If the card is on the right side of the join though, the alias
should still exists and refers to the current-level join alias.
---
.../binning/qb-explicit-joins.cy.spec.js | 38 ++--------------
.../scenarios/question/nested.cy.spec.js | 2 +-
.../query_processor/middleware/annotate.clj | 16 +++++--
.../middleware/fetch_source_query.clj | 19 ++++----
.../middleware/annotate_test.clj | 30 ++++++++++++-
.../middleware/fetch_source_query_test.clj | 44 ++++++++++++-------
6 files changed, 83 insertions(+), 66 deletions(-)
diff --git a/frontend/test/metabase/scenarios/binning/qb-explicit-joins.cy.spec.js b/frontend/test/metabase/scenarios/binning/qb-explicit-joins.cy.spec.js
index 26fdb7de671..22807ba4753 100644
--- a/frontend/test/metabase/scenarios/binning/qb-explicit-joins.cy.spec.js
+++ b/frontend/test/metabase/scenarios/binning/qb-explicit-joins.cy.spec.js
@@ -190,32 +190,11 @@ describe("scenarios > binning > from a saved QB question with explicit joins", (
cy.findByText("QB Binning").click();
});
- /**
- * Generated title seems to be incorrect.
- * Please see: https://github.com/metabase/metabase/issues/16693.
- *
- * 1. Todo: unskip the titles in this block once #16693 gets fixed.
- * 2. Unskip the repro for metabase#16693 which was conviniently created in this same file.
- *
- * Note: after #16693 gets fixed, it might even make sense to completly remove the related repro,
- * since all other tests within this `context` will already cover that implicitly and will guard against a regression.
- */
-
- it.skip("should render the correct title (metabase#16693)", () => {
- cy.findByText("People → Birth Date").click();
- cy.findByText("Distribution").click();
-
- cy.findByText("Count by People → Birth Date: Month");
- });
-
it("should work for time series", () => {
cy.findByText("People → Birth Date").click();
cy.findByText("Distribution").click();
- /**
- * Please see the comment no. 1 above.
- */
- // cy.findByText("Count by People → Birth Date: Month");
+ cy.findByText("Count by People → Birth Date: Month");
assertOnXYAxisLabels({ xLabel: "People → Birth Date", yLabel: "Count" });
@@ -230,10 +209,7 @@ describe("scenarios > binning > from a saved QB question with explicit joins", (
.click();
cy.findByText("Quarter").click();
- /**
- * Please see the comment no. 1 above.
- */
- // cy.findByText("Count by People → Birth Date: Quarter");
+ cy.findByText("Count by People → Birth Date: Quarter");
cy.findByText("Q1 - 1960");
cy.findByText("Q1 - 1965");
@@ -243,10 +219,7 @@ describe("scenarios > binning > from a saved QB question with explicit joins", (
cy.findByText("Products → Price").click();
cy.findByText("Distribution").click();
- /**
- * Please see the comment no. 1 above.
- */
- // cy.findByText("Count by Products → Price: Auto binned");
+ cy.findByText("Count by Products → Price: Auto binned");
assertOnXYAxisLabels({ xLabel: "Products → Price", yLabel: "Count" });
@@ -260,10 +233,7 @@ describe("scenarios > binning > from a saved QB question with explicit joins", (
cy.findByText("People → Longitude").click();
cy.findByText("Distribution").click();
- /**
- * Please see the comment no. 1 above.
- */
- // cy.findByText("Count by People → Longitude: Auto binned");
+ cy.findByText("Count by People → Longitude: Auto binned");
assertOnXYAxisLabels({
xLabel: "People → Longitude",
diff --git a/frontend/test/metabase/scenarios/question/nested.cy.spec.js b/frontend/test/metabase/scenarios/question/nested.cy.spec.js
index a06a8baa198..a49bbccf00f 100644
--- a/frontend/test/metabase/scenarios/question/nested.cy.spec.js
+++ b/frontend/test/metabase/scenarios/question/nested.cy.spec.js
@@ -383,7 +383,7 @@ describe("scenarios > question > nested", () => {
});
});
- it.skip("'distribution' should work on a joined table from a saved question (metabase#14787)", () => {
+ it("'distribution' should work on a joined table from a saved question (metabase#14787)", () => {
// Set the display really wide and really tall to avoid any scrolling
cy.viewport(1600, 1200);
diff --git a/src/metabase/query_processor/middleware/annotate.clj b/src/metabase/query_processor/middleware/annotate.clj
index 2dc84c860a6..fe9149069e7 100644
--- a/src/metabase/query_processor/middleware/annotate.clj
+++ b/src/metabase/query_processor/middleware/annotate.clj
@@ -173,9 +173,10 @@
:field_ref clause}))
(s/defn ^:private col-info-for-field-clause*
- [{:keys [source-metadata expressions], :as inner-query} [_ id-or-name opts :as clause] :- mbql.s/field]
- (let [join (when (:join-alias opts)
- (join-with-alias inner-query (:join-alias opts)))]
+ [{:keys [source-metadata source-card-id], :as inner-query} [_ id-or-name opts :as clause] :- mbql.s/field]
+ (let [join (when (:join-alias opts)
+ (join-with-alias inner-query (:join-alias opts)))
+ join-is-at-current-level? (some #(= (:alias %) (:join-alias opts)) (:joins inner-query))]
;; TODO -- I think we actually need two `:field_ref` columns -- one for referring to the Field at the SAME
;; level, and one for referring to the Field from the PARENT level.
(cond-> {:field_ref clause}
@@ -228,7 +229,14 @@
(or (:source-field opts)
(:fk-field-id join))
(assoc :fk_field_id (or (:source-field opts)
- (:fk-field-id join))))))
+ (:fk-field-id join)))
+
+ ;; If the source query is from a saved question, remove the join alias as the caller should not be aware of joins
+ ;; happening inside the saved question. The `not join-is-at-current-level?` check is to ensure that we are not
+ ;; removing `:join-alias` from fields from the right side of the join.
+ (and source-card-id
+ (not join-is-at-current-level?))
+ (update :field_ref mbql.u/update-field-options dissoc :join-alias))))
(s/defn ^:private col-info-for-field-clause :- {:field_ref mbql.s/Field, s/Keyword s/Any}
"Return results column metadata for a `:field` or `:expression` clause, in the format that gets returned by QP results"
diff --git a/src/metabase/query_processor/middleware/fetch_source_query.clj b/src/metabase/query_processor/middleware/fetch_source_query.clj
index 9c36250d9f8..fa495300410 100644
--- a/src/metabase/query_processor/middleware/fetch_source_query.clj
+++ b/src/metabase/query_processor/middleware/fetch_source_query.clj
@@ -9,6 +9,7 @@
{:source-query {...} ; Query for Card 1
:source-metadata [...] ; metadata about columns in Card 1
+ :source-card-id 1 ; Original Card ID
...}
This middleware resolves Card ID `:source-table`s at all levels of the query, but the top-level query often uses the
@@ -17,7 +18,7 @@
{:database <virtual-id>, :type :query, :query {:source-table \"card__1\"}}
->
- {:database 1, :type :query, :query {:source-query {...}, :source-metadata {...}}}
+ {:database 1, :type :query, :query {:source-query {...}, :source-metadata {...}, :source-card-id 1}}
TODO - consider renaming this namespace to `metabase.query-processor.middleware.resolve-card-id-source-tables`"
(:require [clojure.set :as set]
@@ -49,6 +50,7 @@
{:database mbql.s/DatabaseID
:source-metadata [mbql.s/SourceQueryMetadata]
:source-query mbql.s/SourceQuery
+ :source-card-id su/IntGreaterThanZero
s/Keyword s/Any}
(complement :source-table)
"`:source-table` should be removed"))
@@ -151,8 +153,8 @@
source-query-and-metadata (-> card-id card-id->source-query-and-metadata)]
(merge
(dissoc m :source-table)
- ;; record the `::card-id` we've resolved here. We'll include it in `:info` for permissions purposes later
- {::card-id card-id}
+ ;; record the `card-id` we've resolved here. We'll include it in `:info` for permissions purposes later
+ {:source-card-id card-id}
source-query-and-metadata)))
(defn- resolve-all*
@@ -225,14 +227,11 @@
(s/defn ^:private extract-resolved-card-id :- {:card-id (s/maybe su/IntGreaterThanZero)
:query su/Map}
- "If the ID of the Card we've resolved (`::card-id`) was added by a previous step, remove it from `query`, and add it
+ "If the ID of the Card we've resolved (`:source-card-id`) was added by a previous step, add it
to `:query` `:info` (so it can be included in the QueryExecution log), then return a map with the resolved
`:card-id` and updated `:query`."
[query :- su/Map]
- (let [card-id (get-in query [:query ::card-id])
- query (mbql.u/replace-in query [:query]
- (&match :guard (every-pred map? ::card-id))
- (recur (dissoc &match ::card-id)))]
+ (let [card-id (get-in query [:query :source-card-id])]
{:query (cond-> query
card-id (update-in [:info :card-id] #(or % card-id)))
:card-id card-id}))
@@ -242,9 +241,9 @@
"Recursively replace all Card ID source tables in `query` with resolved `:source-query` and `:source-metadata`. Since
the `:database` is only useful for top-level source queries, we'll remove it from all other levels."
[query :- su/Map]
- ;; if a `::card-id` is already in the query, remove it, so we don't pull user-supplied input up into `:info`
+ ;; if a `:source-card-id` is already in the query, remove it, so we don't pull user-supplied input up into `:info`
;; allowing someone to bypass permissions
- (-> (m/dissoc-in query [:query ::card-id])
+ (-> (m/dissoc-in query [:query :source-card-id])
check-for-circular-references
resolve-all*
copy-source-query-database-ids
diff --git a/test/metabase/query_processor/middleware/annotate_test.clj b/test/metabase/query_processor/middleware/annotate_test.clj
index b7b67d93a68..16849f81671 100644
--- a/test/metabase/query_processor/middleware/annotate_test.clj
+++ b/test/metabase/query_processor/middleware/annotate_test.clj
@@ -1,7 +1,7 @@
(ns metabase.query-processor.middleware.annotate-test
(:require [clojure.test :refer :all]
[metabase.driver :as driver]
- [metabase.models :refer [Field]]
+ [metabase.models :refer [Card Field]]
[metabase.query-processor :as qp]
[metabase.query-processor.middleware.annotate :as annotate]
[metabase.query-processor.store :as qp.store]
@@ -612,3 +612,31 @@
:id %ean
:field_ref &Products.ean})
(ean-metadata (add-column-info nested-query {}))))))))))))))
+
+;; metabase#14787
+(deftest col-info-for-fields-from-card-test
+ (mt/dataset sample-dataset
+ (let [card-1-query (mt/mbql-query orders
+ {:joins [{:fields :all
+ :source-table $$products
+ :condition [:= $product_id &Products.products.id]
+ :alias "Products"}]})]
+ (mt/with-temp* [Card [{card-1-id :id} {:dataset_query card-1-query}]
+ Card [{card-2-id :id} {:dataset_query (mt/mbql-query people)}]]
+ (testing "when a nested query is from a saved question, there should be no `:join-alias` on the left side"
+ (mt/$ids nil
+ (let [base-query (qp/query->preprocessed
+ (mt/mbql-query nil
+ {:source-table (str "card__" card-1-id)
+ :joins [{:fields :all
+ :source-table (str "card__" card-2-id)
+ :condition [:= $orders.user_id &Products.products.id]
+ :alias "Q"}]
+ :limit 1}))
+ fields #{%orders.discount %products.title %people.source}]
+ (is (= [{:display_name "Discount" :field_ref [:field %orders.discount nil]}
+ {:display_name "Products → Title" :field_ref [:field %products.title nil]}
+ {:display_name "Q → Source" :field_ref [:field %people.source {:join-alias "Q"}]}]
+ (->> (:cols (add-column-info base-query {}))
+ (filter #(fields (:id %)))
+ (map #(select-keys % [:display_name :field_ref]))))))))))))
diff --git a/test/metabase/query_processor/middleware/fetch_source_query_test.clj b/test/metabase/query_processor/middleware/fetch_source_query_test.clj
index c238eb5160a..dadb7cf5685 100644
--- a/test/metabase/query_processor/middleware/fetch_source_query_test.clj
+++ b/test/metabase/query_processor/middleware/fetch_source_query_test.clj
@@ -35,7 +35,8 @@
(testing "make sure that the `resolve-card-id-source-tables` middleware correctly resolves MBQL queries"
(mt/with-temp Card [card {:dataset_query (mt/mbql-query venues)}]
(is (= (assoc (default-result-with-inner-query
- {:source-query {:source-table (mt/id :venues)}}
+ {:source-query {:source-table (mt/id :venues)}
+ :source-card-id (u/the-id card)}
(qp/query->expected-cols (mt/mbql-query venues)))
:info {:card-id (u/the-id card)})
(resolve-card-id-source-tables
@@ -44,9 +45,10 @@
(testing "with aggregations/breakouts"
(is (= (assoc (default-result-with-inner-query
- {:aggregation [[:count]]
- :breakout [[:field "price" {:base-type :type/Integer}]]
- :source-query {:source-table (mt/id :venues)}}
+ {:aggregation [[:count]]
+ :breakout [[:field "price" {:base-type :type/Integer}]]
+ :source-query {:source-table (mt/id :venues)}
+ :source-card-id (u/the-id card)}
(qp/query->expected-cols (mt/mbql-query :venues)))
:info {:card-id (u/the-id card)})
(resolve-card-id-source-tables
@@ -58,8 +60,9 @@
(mt/with-temp Card [card {:dataset_query (mt/mbql-query checkins)}]
(testing "with filters"
(is (= (assoc (default-result-with-inner-query
- {:source-query {:source-table (mt/id :checkins)}
- :filter [:between [:field "date" {:base-type :type/Date}] "2015-01-01" "2015-02-01"]}
+ {:source-query {:source-table (mt/id :checkins)}
+ :source-card-id (u/the-id card)
+ :filter [:between [:field "date" {:base-type :type/Date}] "2015-01-01" "2015-02-01"]}
(qp/query->expected-cols (mt/mbql-query :checkins)))
:info {:card-id (u/the-id card)})
(resolve-card-id-source-tables
@@ -75,9 +78,10 @@
(mt/with-temp Card [card {:dataset_query (mt/native-query
{:query (format "SELECT * FROM %s" (mt/format-name "venues"))})}]
(is (= (assoc (default-result-with-inner-query
- {:aggregation [[:count]]
- :breakout [[:field "price" {:base-type :type/Integer}]]
- :source-query {:native (format "SELECT * FROM %s" (mt/format-name "venues"))}}
+ {:aggregation [[:count]]
+ :breakout [[:field "price" {:base-type :type/Integer}]]
+ :source-query {:native (format "SELECT * FROM %s" (mt/format-name "venues"))}
+ :source-card-id (u/the-id card)}
nil)
:info {:card-id (u/the-id card)})
(resolve-card-id-source-tables
@@ -94,11 +98,13 @@
:type :query
:query {:source-table (str "card__" (u/the-id card-1)), :limit 50}}}]]
(is (= (-> (default-result-with-inner-query
- {:limit 25
- :source-query {:limit 50
- :source-query {:source-table (mt/id :venues)
- :limit 100}
- :source-metadata nil}}
+ {:limit 25
+ :source-query {:limit 50
+ :source-query {:source-table (mt/id :venues)
+ :limit 100}
+ :source-card-id (u/the-id card-1)
+ :source-metadata nil}
+ :source-card-id (u/the-id card-2)}
(qp/query->expected-cols (mt/mbql-query :venues)))
(assoc-in [:query :source-query :source-metadata]
(mt/derecordize (qp/query->expected-cols (mt/mbql-query :venues))))
@@ -126,6 +132,7 @@
(testing "Are `card__id` source tables resolved in `:joins`?"
(is (= (mt/mbql-query venues
{:joins [{:source-query {:source-table $$categories, :limit 100}
+ :source-card-id card-id
:alias "c",
:condition [:= $category_id [:field %categories.id {:join-alias "c"}]]
:source-metadata metadata}]})
@@ -138,6 +145,7 @@
(testing "Are `card__id` source tables resolved in JOINs against a source query?"
(is (= (mt/mbql-query venues
{:joins [{:source-query {:source-query {:source-table $$categories, :limit 100}
+ :source-card-id card-id
:source-metadata metadata}
:alias "c",
:condition [:= $category_id [:field %categories.id {:join-alias "c"}]]}]})
@@ -152,6 +160,7 @@
{:source-query {:source-table $$venues
:joins [{:source-query {:source-table $$categories
:limit 100}
+ :source-card-id card-id
:alias "c"
:condition [:= $category_id [:field %categories.id {:join-alias "c"}]]
:source-metadata metadata}]}})
@@ -172,8 +181,10 @@
:condition [:= $category_id &c.$categories.id]
:source-query {:source-query {:source-table $$categories
:limit 100}
+ :source-card-id card-id
:source-metadata metadata
:limit 200}
+ :source-card-id card-2-id
;; TODO - FIXME
;;
;; The source metadata that comes back here looks like the result of running the Card
@@ -252,11 +263,11 @@
:condition [:= *ID/Number &c.venues.id]}]})))))))
(deftest ignore-user-supplied-internal-card-id-keys
- (testing (str "The middleware uses `::fetch-source-query/card-id` internally and adds it to `:info` at the end. "
+ (testing (str "The middleware uses `:source-card-id` internally and adds it to `:info` at the end. "
"Make sure we ignore ones supplied by a user.")
(is (= (mt/mbql-query venues)
(resolve-card-id-source-tables
- (assoc-in (mt/mbql-query venues) [:query ::fetch-source-query/card-id] 1))))))
+ (assoc-in (mt/mbql-query venues) [:query :source-card-id] 1))))))
(deftest dont-overwrite-existing-card-id-test
(testing "Don't overwrite existing values of `[:info :card-id]`"
@@ -264,6 +275,7 @@
(let [query (assoc (mt/mbql-query nil {:source-table (format "card__%d" card-id)})
:info {:card-id Integer/MAX_VALUE})]
(is (= (assoc (mt/mbql-query nil {:source-query {:source-table (mt/id :venues)}
+ :source-card-id card-id
:source-metadata (mt/derecordize (qp/query->expected-cols (mt/mbql-query :venues)))})
:info {:card-id Integer/MAX_VALUE})
(resolve-card-id-source-tables query)))))))
--
GitLab