From 31d934c527e184b3d401a5775a9bdfbd5b1be723 Mon Sep 17 00:00:00 2001
From: Cam Saul <1455846+camsaul@users.noreply.github.com>
Date: Wed, 19 Jul 2023 15:31:15 -0700
Subject: [PATCH] `with-join-fields` should add join alias to field refs
(#32491)
---
src/metabase/lib/field.cljc | 4 +-
src/metabase/lib/join.cljc | 78 +++++++++++++----
src/metabase/lib/schema.cljc | 2 +
src/metabase/lib/schema/join.cljc | 11 +--
test/metabase/lib/join_test.cljc | 134 ++++++++++++++++++------------
test/metabase/lib/test_util.cljc | 12 +++
6 files changed, 168 insertions(+), 73 deletions(-)
diff --git a/src/metabase/lib/field.cljc b/src/metabase/lib/field.cljc
index eae94ea0ba9..72293bef577 100644
--- a/src/metabase/lib/field.cljc
+++ b/src/metabase/lib/field.cljc
@@ -98,8 +98,8 @@
(resolve-column-name-in-metadata column-name stage-columns))]
(cond-> column
previous-stage-number (-> (dissoc :id :table-id
- ::binning ::temporal-unit
- :metabase.lib.join/join-alias)
+ ::binning ::temporal-unit)
+ (lib.join/with-join-alias nil)
(assoc :name (or (:lib/desired-column-alias column) (:name column)))
(assoc :lib/source :source/previous-stage))))))
diff --git a/src/metabase/lib/join.cljc b/src/metabase/lib/join.cljc
index e3d71b058fd..21ed293a489 100644
--- a/src/metabase/lib/join.cljc
+++ b/src/metabase/lib/join.cljc
@@ -48,6 +48,58 @@
[:ref :mbql.clause/field]
PartialJoin])
+(mu/defn current-join-alias :- [:maybe ::lib.schema.common/non-blank-string]
+ "Get the current join alias associated with something, if it has one."
+ [field-or-join :- [:maybe FieldOrPartialJoin]]
+ (case (lib.dispatch/dispatch-value field-or-join)
+ :dispatch-type/nil nil
+ :field (:join-alias (lib.options/options field-or-join))
+ :metadata/column (::join-alias field-or-join)
+ :mbql/join (:alias field-or-join)))
+
+(declare with-join-alias)
+
+(defn- with-join-alias-update-join-fields
+ "Impl for [[with-join-alias]] for a join: recursively update the `:join-alias` for the `:field` refs inside `:fields`
+ as needed."
+ [join new-alias]
+ (cond-> join
+ (:fields join) (update :fields (fn [fields]
+ (if-not (sequential? fields)
+ fields
+ (mapv (fn [field-ref]
+ (with-join-alias field-ref new-alias))
+ fields))))))
+
+(defn- with-join-alias-update-join-conditions
+ "Impl for [[with-join-alias]] for a join: recursively update the `:join-alias` for inside the `:conditions` of the
+ join.
+
+ Only updates things that already had a join alias of `old-alias`; does not add alias to things that did not already
+ have one (such as the RHS of a 'typical' join condition). I went back and forth on this behavior, but eventually
+ decided NOT to assume that every join condition is the shape
+
+ [<operator> <lhs-column-from-source> <rhs-column-from-join>]
+
+ This is currently true of normal FE usage, but may not be true everywhere in the future; we don't want to make MLv2
+ impossible to use if you're doing something different like swapping the order of the columns or some sort of more
+ complex condition."
+ [join old-alias new-alias]
+ (if-not old-alias
+ join
+ (mbql.u.match/replace-in join [:conditions]
+ [:field {:join-alias old-alias} _id-or-name]
+ (with-join-alias &match new-alias))))
+
+(defn- with-join-alias-update-join
+ "Impl for [[with-join-alias]] for a join."
+ [join new-alias]
+ (let [old-alias (current-join-alias join)]
+ (-> join
+ (u/assoc-dissoc :alias new-alias)
+ (with-join-alias-update-join-fields new-alias)
+ (with-join-alias-update-join-conditions old-alias new-alias))))
+
(mu/defn with-join-alias :- FieldOrPartialJoin
"Add OR REMOVE a specific `join-alias` to `field-or-join`, which is either a `:field`/Field metadata, or a join map.
Does not recursively update other references (yet; we can add this in the future)."
@@ -62,16 +114,7 @@
(u/assoc-dissoc field-or-join ::join-alias join-alias)
:mbql/join
- (u/assoc-dissoc field-or-join :alias join-alias)))
-
-(mu/defn current-join-alias :- [:maybe ::lib.schema.common/non-blank-string]
- "Get the current join alias associated with something, if it has one."
- [field-or-join :- [:maybe FieldOrPartialJoin]]
- (case (lib.dispatch/dispatch-value field-or-join)
- :dispatch-type/nil nil
- :field (:join-alias (lib.options/options field-or-join))
- :metadata/column (::join-alias field-or-join)
- :mbql/join (:alias field-or-join)))
+ (with-join-alias-update-join field-or-join join-alias)))
(mu/defn resolve-join :- ::lib.schema.join/join
"Resolve a join with a specific `join-alias`."
@@ -256,10 +299,17 @@
references."
[joinable :- PartialJoin
fields :- [:maybe [:or [:enum :all :none] [:sequential some?]]]]
- (u/assoc-dissoc joinable :fields (cond
- (keyword? fields) fields
- (= fields []) :none
- :else (not-empty (mapv lib.ref/ref fields)))))
+ (let [fields (cond
+ (keyword? fields) fields
+ (= fields []) :none
+ :else (not-empty
+ (into []
+ (comp (map lib.ref/ref)
+ (if-let [current-alias (current-join-alias joinable)]
+ (map #(with-join-alias % current-alias))
+ identity))
+ fields)))]
+ (u/assoc-dissoc joinable :fields fields)))
(defn- select-home-column
[home-cols cond-fields]
diff --git a/src/metabase/lib/schema.cljc b/src/metabase/lib/schema.cljc
index 70743606c7f..d21bdc993cf 100644
--- a/src/metabase/lib/schema.cljc
+++ b/src/metabase/lib/schema.cljc
@@ -52,6 +52,8 @@
(mr/def ::breakouts
[:sequential {:min 1} [:ref ::ref/ref]])
+;;; TODO -- `:fields` is supposed to be distinct (ignoring UUID), e.g. you can't have `[:field {} 1]` in there
+;;; twice. (#32489)
(mr/def ::fields
[:sequential {:min 1} [:ref ::ref/ref]])
diff --git a/src/metabase/lib/schema/join.cljc b/src/metabase/lib/schema/join.cljc
index cb486458de3..20ef99d8301 100644
--- a/src/metabase/lib/schema/join.cljc
+++ b/src/metabase/lib/schema/join.cljc
@@ -3,7 +3,6 @@
(:require
[metabase.lib.schema.common :as common]
[metabase.lib.schema.expression :as expression]
- [metabase.lib.schema.ref :as ref]
[metabase.shared.util.i18n :as i18n]
[metabase.util.malli.registry :as mr]))
@@ -15,8 +14,9 @@
;;;
;;; * `:all`: will include all of the Fields from the joined table or query
;;;
-;;; * a sequence of Field clauses: include only the Fields specified. Valid clauses are the same as the top-level
-;;; `:fields` clause. This should be non-empty and all elements should be distinct. The normalizer will
+;;; * a sequence of Field clauses: include only the Fields specified. Only `:field` clauses are allowed here!
+;;; References to expressions or aggregations in the thing we're joining should use column literal (string column
+;;; name) `:field` references. This should be non-empty and all elements should be distinct. The normalizer will
;;; automatically remove duplicate fields for you, and replace empty clauses with `:none`.
;;;
;;; Driver implementations: you can ignore this clause. Relevant fields will be added to top-level `:fields` clause
@@ -24,8 +24,9 @@
(mr/def ::fields
[:or
[:enum :all :none]
- ;; TODO -- Pretty sure fields are supposed to be unique, even excluding `:lib/uuid`
- [:sequential {:min 1} [:ref ::ref/ref]]])
+ ;; TODO -- `:fields` is supposed to be distinct (ignoring UUID), e.g. you can't have `[:field {} 1]` in there
+ ;; twice. (#32489)
+ [:sequential {:min 1} [:ref :mbql.clause/field]]])
;;; The name used to alias the joined table or query. This is usually generated automatically and generally looks
;;; like `table__via__field`. You can specify this yourself if you need to reference a joined field with a
diff --git a/test/metabase/lib/join_test.cljc b/test/metabase/lib/join_test.cljc
index 549054a4d39..04ab89a1561 100644
--- a/test/metabase/lib/join_test.cljc
+++ b/test/metabase/lib/join_test.cljc
@@ -48,7 +48,7 @@
:join-alias "Categories"}
(meta/id :categories :id)]]]
:fields :all}]}]}
- (let [q (lib/query meta/metadata-provider (meta/table-metadata :venues))
+ (let [q lib.tu/venues-query
j (lib/query meta/metadata-provider (meta/table-metadata :categories))]
(lib/join q (lib/join-clause j [{:lib/type :lib/external-op
:operator :=
@@ -220,7 +220,7 @@
(lib.join/joined-thing query join)))))
(deftest ^:parallel joins-source-and-desired-aliases-test
- (let [query (-> (lib/query meta/metadata-provider (meta/table-metadata :venues))
+ (let [query (-> lib.tu/venues-query
(lib/join (-> (lib/join-clause
(meta/table-metadata :categories)
[(lib/=
@@ -386,56 +386,86 @@
(map (partial lib.metadata.calculation/display-info query)
(lib/available-join-strategies query))))))
-(deftest ^:parallel with-join-fields-test
- (doseq [[message {:keys [input expected]}] {:all
- {:input :all, :expected {:fields :all}}
-
- :none
- {:input :none, :expected {:fields :none}}
-
- "(with-join-fields ... []) should set :fields to :none"
- {:input [], :expected {:fields :none}}
+(deftest ^:parallel with-join-alias-update-fields-test
+ (testing "with-join-alias should update the alias of columns in :fields"
+ (let [query lib.tu/query-with-join-with-explicit-fields
+ [join] (lib/joins query)]
+ (is (=? {:alias "Cat"
+ :fields [[:field {:join-alias "Cat"} (meta/id :categories :name)]]}
+ join))
+ (let [join' (lib/with-join-alias join "New Alias")]
+ (is (=? {:alias "New Alias"
+ :fields [[:field {:join-alias "New Alias"} (meta/id :categories :name)]]}
+ join'))))))
+
+(deftest ^:parallel with-join-alias-update-condition-rhs-test
+ (testing "with-join-alias should update the alias of the RHS column(s) in the condition(s)"
+ (let [query lib.tu/query-with-join
+ [join] (lib/joins query)]
+ (is (=? {:conditions [[:= {}
+ [:field {} (meta/id :venues :category-id)]
+ [:field {:join-alias "Cat"} (meta/id :categories :id)]]]
+ :alias "Cat"}
+ join))
+ (let [join' (lib/with-join-alias join "New Alias")]
+ (is (=? {:conditions [[:= {}
+ [:field {} (meta/id :venues :category-id)]
+ [:field {:join-alias "New Alias"} (meta/id :categories :id)]]]
+ :alias "New Alias"}
+ join'))))))
+
+(defn- test-with-join-fields [input expected]
+ (testing (pr-str (list 'with-join-fields 'query input))
+ (let [query (-> lib.tu/venues-query
+ (lib/join (-> (lib/join-clause
+ (meta/table-metadata :categories)
+ [(lib/=
+ (meta/field-metadata :venues :category-id)
+ (lib/with-join-alias (meta/field-metadata :categories :id) "Cat"))])
+ (lib/with-join-alias "Cat")
+ (lib/with-join-fields input))))]
+ (is (=? {:stages [{:joins [(merge
+ {:alias "Cat"
+ :conditions [[:= {}
+ [:field {} (meta/id :venues :category-id)]
+ [:field {:join-alias "Cat"} (meta/id :categories :id)]]]}
+ expected)]}]}
+ query))
+ (let [[join] (lib/joins query)]
+ (is (some? join))
+ (is (= (:fields expected)
+ (lib/join-fields join)))))))
- nil
- {:input nil, :expected nil}
-
- "explicit :fields"
- (let [categories-id [:field {:lib/uuid (str (random-uuid))
- :join-alias "Cat"}
- (meta/id :categories :id)]]
- {:input [categories-id]
- :expected {:fields [categories-id]}})}]
- (testing message
- (let [query (-> (lib/query meta/metadata-provider (meta/table-metadata :venues))
- (lib/join (-> (lib/join-clause
- (meta/table-metadata :categories)
- [(lib/=
- (meta/field-metadata :venues :category-id)
- (lib/with-join-alias (meta/field-metadata :categories :id) "Cat"))])
- (lib/with-join-alias "Cat")
- (lib/with-join-fields input))))]
- (is (=? {:stages [{:joins [(merge
- {:alias "Cat"
- :conditions [[:= {}
- [:field {} (meta/id :venues :category-id)]
- [:field {:join-alias "Cat"} (meta/id :categories :id)]]]}
- expected)]}]}
- query))
- (let [[join] (lib/joins query)]
- (is (some? join))
- (is (= (:fields expected)
- (lib/join-fields join))))))))
-
-(def ^:private query-with-join-with-fields
- "A query against `VENUES` joining `CATEGORIES` with `:fields` set to return only `NAME`."
- (-> lib.tu/venues-query
- (lib/join (-> (lib/join-clause
- (meta/table-metadata :categories)
- [(lib/=
- (meta/field-metadata :venues :category-id)
- (lib/with-join-alias (meta/field-metadata :categories :id) "Cat"))])
- (lib/with-join-alias "Cat")
- (lib/with-join-fields [(lib/with-join-alias (meta/field-metadata :categories :name) "Cat")])))))
+(deftest ^:parallel with-join-fields-test
+ (doseq [{:keys [input expected]}
+ [{:input :all, :expected {:fields :all}}
+ {:input :none, :expected {:fields :none}}
+ ;; (with-join-fields ... []) should set :fields to :none
+ {:input [], :expected {:fields :none}}
+ {:input nil, :expected nil}]]
+ (test-with-join-fields input expected)))
+
+(deftest ^:parallel with-join-fields-explicit-fields-test
+ (let [categories-id [:field {:lib/uuid (str (random-uuid))
+ :join-alias "Cat"}
+ (meta/id :categories :id)]]
+ (test-with-join-fields
+ [categories-id]
+ {:fields [categories-id]})))
+
+(deftest ^:parallel with-join-fields-update-join-aliases-test
+ (testing "explicit :fields should change join alias for fields that have a different alias (#32437)"
+ (let [categories-id [:field {:lib/uuid (str (random-uuid))} (meta/id :categories :id)]]
+ (test-with-join-fields
+ [(lib/with-join-alias categories-id "Hat")]
+ {:fields [(lib/with-join-alias categories-id "Cat")]}))))
+
+(deftest ^:parallel with-join-fields-add-missing-aliases-test
+ (testing "explicit :fields should add join alias to fields missing it (#32437)"
+ (let [categories-id [:field {:lib/uuid (str (random-uuid))} (meta/id :categories :id)]]
+ (test-with-join-fields
+ [categories-id]
+ {:fields [(lib/with-join-alias categories-id "Cat")]}))))
(deftest ^:parallel join-condition-lhs-columns-test
(let [query lib.tu/venues-query]
@@ -452,7 +482,7 @@
(deftest ^:parallel join-condition-lhs-columns-with-previous-join-test
(testing "Include columns from previous join(s)"
- (let [query query-with-join-with-fields]
+ (let [query lib.tu/query-with-join-with-explicit-fields]
(doseq [rhs [nil (lib/with-join-alias (lib.metadata/field query (meta/id :users :id)) "User")]]
(testing (str "rhs = " (pr-str rhs))
(is (=? [{:lib/desired-column-alias "ID"}
diff --git a/test/metabase/lib/test_util.cljc b/test/metabase/lib/test_util.cljc
index 55a0268bff4..607387c6126 100644
--- a/test/metabase/lib/test_util.cljc
+++ b/test/metabase/lib/test_util.cljc
@@ -186,6 +186,18 @@
"A query against `VENUES` with an explicit join against `CATEGORIES`."
(add-joins venues-query "Cat"))
+(def query-with-join-with-explicit-fields
+ "A query against `VENUES` with an explicit join against `CATEGORIES`, that includes explicit `:fields` including just
+ `CATEGORIES.NAME`."
+ (-> venues-query
+ (lib/join (-> (lib/join-clause (meta/table-metadata :categories))
+ (lib/with-join-conditions [(lib/= (meta/field-metadata :venues :category-id)
+ (-> (meta/field-metadata :categories :id)
+ (lib/with-join-alias "Cat")))])
+ (lib/with-join-alias "Cat")
+ (lib/with-join-fields [(-> (meta/field-metadata :categories :name)
+ (lib/with-join-alias "Cat"))])))))
+
(def query-with-expression
"A query with an expression."
(-> venues-query
--
GitLab