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 26fdb7de671070912745fbcefce5d35d44035696..22807ba47530ed781037b645061d1589d3883b87 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 a06a8baa198700838b67e199eb8e07d7890a9406..a49bbccf00f7d6600e42f97fb878645922bc9b08 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 2dc84c860a6695958847fa03c61f16ae802a5b86..fe9149069e78113ef4474095d54c8839e8867867 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 9c36250d9f831c18538ab6450067dede675464ef..fa495300410171e762583e5cf111e33989749c0e 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 b7b67d93a6816b926500a982047390e0f322c4e2..16849f81671da96db2133d0f1cf5c52b159b2d99 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 c238eb5160a120fb8108dd5cefa59426095143e5..dadb7cf5685947c896e492e9524a02ebf37cd854 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)))))))