diff --git a/frontend/src/metabase-lib/join.ts b/frontend/src/metabase-lib/join.ts
index 0b2f179502bd69e85bfef067b052b1db54653997..8f1feb8efe3cf325148edc11c5dc687d789552c5 100644
--- a/frontend/src/metabase-lib/join.ts
+++ b/frontend/src/metabase-lib/join.ts
@@ -196,12 +196,12 @@ export function joinConditionOperators(
return ML.join_condition_operators(query, stageIndex, lhsColumn, rhsColumn);
}
-export function suggestedJoinCondition(
+export function suggestedJoinConditions(
query: Query,
stageIndex: number,
joinable: Joinable,
-): JoinCondition | null {
- return ML.suggested_join_condition(query, stageIndex, joinable);
+): JoinCondition[] {
+ return ML.suggested_join_conditions(query, stageIndex, joinable);
}
export type JoinFields = ColumnMetadata[] | "all" | "none";
diff --git a/frontend/src/metabase/query_builder/components/notebook/steps/JoinStep/use-join.ts b/frontend/src/metabase/query_builder/components/notebook/steps/JoinStep/use-join.ts
index e579f8e345545dc3ffc0edd68abd5748259e66e1..35e80c6d2399d481abb3d03a19702f2049460dde 100644
--- a/frontend/src/metabase/query_builder/components/notebook/steps/JoinStep/use-join.ts
+++ b/frontend/src/metabase/query_builder/components/notebook/steps/JoinStep/use-join.ts
@@ -94,17 +94,16 @@ export function useJoin(query: Lib.Query, stageIndex: number, join?: Lib.Join) {
const setTable = useCallback(
(nextTable: Lib.Joinable) => {
- const suggestedCondition = Lib.suggestedJoinCondition(
+ const suggestedConditions = Lib.suggestedJoinConditions(
query,
stageIndex,
nextTable,
);
- if (suggestedCondition) {
- const nextConditions = [suggestedCondition];
- _setConditions(nextConditions);
+ if (suggestedConditions.length > 0) {
+ _setConditions(suggestedConditions);
- let nextJoin = Lib.joinClause(nextTable, nextConditions);
+ let nextJoin = Lib.joinClause(nextTable, suggestedConditions);
nextJoin = Lib.withJoinFields(nextJoin, "all");
nextJoin = Lib.withJoinStrategy(nextJoin, strategy);
diff --git a/src/metabase/lib/core.cljc b/src/metabase/lib/core.cljc
index 0d552e5d21b268ec7dfdc7615627be7af6c3d19f..c7b14fd4af1b143093618269a1ec9cf223e080d7 100644
--- a/src/metabase/lib/core.cljc
+++ b/src/metabase/lib/core.cljc
@@ -209,7 +209,7 @@
joinable-columns
joins
raw-join-strategy
- suggested-join-condition
+ suggested-join-conditions
with-join-alias
with-join-fields
with-join-strategy
diff --git a/src/metabase/lib/join.cljc b/src/metabase/lib/join.cljc
index a78193b86304d09a81211716cdbfb243d68b103e..ec3f4fa2857c2195e2b7ccdbf5627f82ed9bda9a 100644
--- a/src/metabase/lib/join.cljc
+++ b/src/metabase/lib/join.cljc
@@ -22,6 +22,7 @@
[metabase.lib.schema.expression :as lib.schema.expression]
[metabase.lib.schema.filter :as lib.schema.filter]
[metabase.lib.schema.join :as lib.schema.join]
+ [metabase.lib.schema.metadata :as lib.schema.metadata]
[metabase.lib.schema.temporal-bucketing :as lib.schema.temporal-bucketing]
[metabase.lib.temporal-bucket :as lib.temporal-bucket]
[metabase.lib.types.isa :as lib.types.isa]
@@ -200,7 +201,7 @@
display name."
[query :- ::lib.schema/query
stage-number :- :int
- column-metadata :- lib.metadata/ColumnMetadata
+ column-metadata :- ::lib.schema.metadata/column
join-alias :- ::lib.schema.common/non-blank-string]
(let [column-metadata (assoc column-metadata :source-alias join-alias)
col (-> (assoc column-metadata
@@ -499,7 +500,7 @@
(declare join-conditions
joined-thing
- suggested-join-condition)
+ suggested-join-conditions)
(mu/defn join :- ::lib.schema/query
"Add a join clause to a `query`."
@@ -510,10 +511,10 @@
stage-number :- :int
a-join :- [:or lib.join.util/PartialJoin Joinable]]
(let [a-join (join-clause a-join)
- suggested-condition (when (empty? (join-conditions a-join))
- (suggested-join-condition query stage-number (joined-thing query a-join)))
+ suggested-conditions (when (empty? (join-conditions a-join))
+ (suggested-join-conditions query stage-number (joined-thing query a-join)))
a-join (cond-> a-join
- suggested-condition (with-join-conditions [suggested-condition]))
+ (seq suggested-conditions) (with-join-conditions suggested-conditions))
a-join (add-default-alias query stage-number a-join)]
(lib.util/update-query-stage query stage-number update :joins (fn [existing-joins]
(conj (vec existing-joins) a-join))))))
@@ -619,11 +620,11 @@
;;; options for each respective part. At the time of this writing, selecting one does not filter out incompatible
;;; options for the other parts, but hopefully we can implement this in the future -- see #31174
-(mu/defn ^:private sort-join-condition-columns :- [:sequential lib.metadata/ColumnMetadata]
+(mu/defn ^:private sort-join-condition-columns :- [:sequential ::lib.schema.metadata/column]
"Sort potential join condition columns as returned by [[join-condition-lhs-columns]]
or [[join-condition-rhs-columns]]. PK columns are returned first, followed by FK columns, followed by other columns.
Otherwise original order is maintained."
- [columns :- [:sequential lib.metadata/ColumnMetadata]]
+ [columns :- [:sequential ::lib.schema.metadata/column]]
(let [{:keys [pk fk other]} (group-by (fn [column]
(cond
(lib.types.isa/primary-key? column) :pk
@@ -643,7 +644,7 @@
column))
(lib.equality/mark-selected-columns query stage-number columns [existing-column-or-nil]))))
-(mu/defn join-condition-lhs-columns :- [:sequential lib.metadata/ColumnMetadata]
+(mu/defn join-condition-lhs-columns :- [:sequential ::lib.schema.metadata/column]
"Get a sequence of columns that can be used as the left-hand-side (source column) in a join condition. This column
is the one that comes from the source Table/Card/previous stage of the query or a previous join.
@@ -698,7 +699,7 @@
(mark-selected-column query stage-number lhs-column-or-nil)
sort-join-condition-columns))))
-(mu/defn join-condition-rhs-columns :- [:sequential lib.metadata/ColumnMetadata]
+(mu/defn join-condition-rhs-columns :- [:sequential ::lib.schema.metadata/column]
"Get a sequence of columns that can be used as the right-hand-side (target column) in a join condition. This column
is the one that belongs to the thing being joined, `join-or-joinable`, which can be something like a
Table ([[metabase.lib.metadata/TableMetadata]]), Saved Question/Model ([[metabase.lib.metadata/CardMetadata]]),
@@ -751,67 +752,80 @@
([_query :- ::lib.schema/query
_stage-number :- :int
;; not yet used, hopefully we will use in the future when present for filtering incompatible options out.
- _lhs-column-or-nil :- [:maybe lib.metadata/ColumnMetadata]
- _rhs-column-or-nil :- [:maybe lib.metadata/ColumnMetadata]]
+ _lhs-column-or-nil :- [:maybe ::lib.schema.metadata/column]
+ _rhs-column-or-nil :- [:maybe ::lib.schema.metadata/column]]
;; currently hardcoded to these six operators regardless of LHS and RHS.
lib.filter.operator/join-operators))
-(mu/defn ^:private pk-column :- [:maybe lib.metadata/ColumnMetadata]
- "Given something `x` (e.g. a Table metadata) find the PK column."
+(mu/defn ^:private fk-columns-to :- [:maybe [:sequential
+ {:min 1}
+ [:and
+ ::lib.schema.metadata/column
+ [:map
+ [::target ::lib.schema.metadata/column]]]]]
+ "Find FK columns in `source` pointing at a column in `target`. Includes the target column under the `::target` key."
[query :- ::lib.schema/query
stage-number :- :int
- x
- options]
- (m/find-first lib.types.isa/primary-key?
- (lib.metadata.calculation/visible-columns query stage-number x options)))
-
-(mu/defn ^:private fk-column-for-pk-in :- [:maybe lib.metadata/ColumnMetadata]
- [pk-col :- [:maybe lib.metadata/ColumnMetadata]
- visible-columns :- [:maybe [:sequential lib.metadata/ColumnMetadata]]]
- (when-let [pk-id (:id pk-col)]
- (let [candidates (filter (fn [{:keys [fk-target-field-id], :as col}]
- (and (lib.types.isa/foreign-key? col)
- (= fk-target-field-id pk-id)))
- visible-columns)
- primary (first candidates)]
- (if (some #(= (:id %) (:id primary)) (rest candidates))
- (when (not-any? #(= (:lib/desired-column-alias %) (:lib/desired-column-alias primary)) (rest candidates))
- ;; there are multiple candidates but :lib/desired-column-alias disambiguates them,
- ;; so reference by name
- (dissoc primary :id))
- primary))))
-
-(mu/defn ^:private fk-column-for :- [:maybe lib.metadata/ColumnMetadata]
- "Given a query stage find an FK column that points to the PK `pk-col`."
- [query :- ::lib.schema/query
- stage-number :- :int
- pk-col :- [:maybe lib.metadata/ColumnMetadata]]
- (when pk-col
- (let [visible-columns (lib.metadata.calculation/visible-columns query stage-number (lib.util/query-stage query stage-number))]
- (fk-column-for-pk-in pk-col visible-columns))))
-
-(mu/defn suggested-join-condition :- [:maybe ::lib.schema.expression/boolean] ; i.e., a filter clause
- "Return a suggested default join condition when constructing a join against `joinable`, e.g. a Table, Saved
- Question, or another query. A suggested condition will be returned if the query stage has a foreign key to the
+ source
+ target]
+ (let [target-columns (delay
+ (lib.metadata.calculation/visible-columns
+ query stage-number target
+ {:include-implicitly-joinable? false
+ :include-implicitly-joinable-for-source-card? false}))]
+ (not-empty
+ (into []
+ (keep (fn [{:keys [fk-target-field-id], :as col}]
+ (when (and (lib.types.isa/foreign-key? col)
+ fk-target-field-id)
+ (when-let [target-column (m/find-first (fn [target-column]
+ (= fk-target-field-id
+ (:id target-column)))
+ @target-columns)]
+ (assoc col ::target target-column)))))
+ (lib.metadata.calculation/visible-columns query stage-number source)))))
+
+(mu/defn suggested-join-conditions :- [:maybe [:sequential {:min 1} ::lib.schema.expression/boolean]] ; i.e., a filter clause
+ "Return suggested default join conditions when constructing a join against `joinable`, e.g. a Table, Saved
+ Question, or another query. Suggested conditions will be returned if the source Table has a foreign key to the
primary key of the thing we're joining (see #31175 for more info); otherwise this will return `nil` if no default
- condition is suggested."
+ conditions are suggested."
([query joinable]
- (suggested-join-condition query -1 joinable))
+ (suggested-join-conditions query -1 joinable))
([query :- ::lib.schema/query
stage-number :- :int
joinable]
- (let [joinable-col-options {:include-implicitly-joinable? false
- :include-implicitly-joinable-for-source-card? false}]
- (letfn [(filter-clause [x y]
- (lib.filter/filter-clause (lib.filter.operator/operator-def :=) x y))]
- (or (when-let [pk-col (pk-column query stage-number joinable joinable-col-options)]
- (when-let [fk-col (fk-column-for query stage-number pk-col)]
- (filter-clause fk-col pk-col)))
- (when-let [pk-col (pk-column query stage-number (lib.util/query-stage query stage-number) nil)]
- (when-let [fk-col (fk-column-for-pk-in pk-col (lib.metadata.calculation/visible-columns
- query stage-number joinable joinable-col-options))]
- (filter-clause pk-col fk-col))))))))
+ (let [stage (lib.util/query-stage query stage-number)]
+ (letfn [ ;; only keep one FK to each target column e.g. for
+ ;;
+ ;; messages (sender_id REFERENCES user(id), recipient_id REFERENCES user(id))
+ ;;
+ ;; we only want join on one or the other, not both, because that makes no sense. However with a composite
+ ;; FK -> composite PK suggest multiple conditions. See #34184
+ (fks [source target]
+ (->> (fk-columns-to query stage-number source target)
+ (m/distinct-by #(-> % ::target :id))
+ not-empty))
+ (filter-clause [x y]
+ ;; DO NOT force broken refs for fields that come from Cards (broken refs in this case means use Field
+ ;; ID refs instead of nominal field literal refs), that will break things if a Card returns the same
+ ;; Field more than once (there would be no way to disambiguate). See #34227 for more info
+ (let [x (dissoc x ::lib.card/force-broken-id-refs)
+ y (dissoc y ::lib.card/force-broken-id-refs)]
+ (lib.filter/filter-clause (lib.filter.operator/operator-def :=) x y)))]
+ (or
+ ;; find cases where we have FK(s) pointing to joinable. Our column goes on the LHS.
+ (when-let [fks (fks stage joinable)]
+ (mapv (fn [fk]
+ (filter-clause fk (::target fk)))
+ fks))
+ ;; find cases where the `joinable` has FK(s) pointing to us. Note our column is the target this time around --
+ ;; keep in on the LHS.
+ (when-let [fks (fks joinable stage)]
+ (mapv (fn [fk]
+ (filter-clause (::target fk) fk))
+ fks)))))))
(defn- add-join-alias-to-joinable-columns [cols a-join]
(let [join-alias (lib.join.util/current-join-alias a-join)
@@ -833,7 +847,7 @@
cols)
(lib.equality/mark-selected-columns cols j-fields))))
-(mu/defn joinable-columns :- [:sequential lib.metadata/ColumnMetadata]
+(mu/defn joinable-columns :- [:sequential ::lib.schema.metadata/column]
"Return information about the fields that you can pass to [[with-join-fields]] when constructing a join against
something [[Joinable]] (i.e., a Table or Card) or manipulating an existing join. When passing in a join, currently
selected columns (those in the join's `:fields`) will include `:selected true` information."
@@ -931,7 +945,7 @@
([query :- ::lib.schema/query
stage-number :- :int
join-or-joinable :- [:maybe JoinOrJoinable]
- condition-lhs-column-or-nil :- [:maybe [:or lib.metadata/ColumnMetadata :mbql.clause/field]]]
+ condition-lhs-column-or-nil :- [:maybe [:or ::lib.schema.metadata/column :mbql.clause/field]]]
(or
(join-lhs-display-name-from-condition-lhs query stage-number join-or-joinable condition-lhs-column-or-nil)
(join-lhs-display-name-for-first-join-in-first-stage query stage-number join-or-joinable)
diff --git a/src/metabase/lib/js.cljs b/src/metabase/lib/js.cljs
index bdae5be61b7652f1afffdec227efe7e4f40b7653..d70db5d2597b9f995099372ad991f7994d976403 100644
--- a/src/metabase/lib/js.cljs
+++ b/src/metabase/lib/js.cljs
@@ -698,13 +698,13 @@
([a-query stage-number expression-position]
(to-array (lib.core/expressionable-columns a-query stage-number expression-position))))
-(defn ^:export suggested-join-condition
- "Return a suggested default join condition when constructing a join against `joinable`, e.g. a Table, Saved
- Question, or another query. A suggested condition will be returned if the source Table has a foreign key to the
+(defn ^:export suggested-join-conditions
+ "Return suggested default join conditions when constructing a join against `joinable`, e.g. a Table, Saved
+ Question, or another query. Suggested conditions will be returned if the source Table has a foreign key to the
primary key of the thing we're joining (see #31175 for more info); otherwise this will return `nil` if no default
- condition is suggested."
+ conditions are suggested."
[a-query stage-number joinable]
- (lib.core/suggested-join-condition a-query stage-number joinable))
+ (to-array (lib.core/suggested-join-conditions a-query stage-number joinable)))
(defn ^:export join-fields
"Get the `:fields` associated with a join."
diff --git a/test/metabase/lib/field_test.cljc b/test/metabase/lib/field_test.cljc
index 2e4385eda212f4a7b162022e500fbce5c51ac099..53383223a399ffa79e5ce5ed2b303d1d74a11e86 100644
--- a/test/metabase/lib/field_test.cljc
+++ b/test/metabase/lib/field_test.cljc
@@ -746,8 +746,7 @@
(let [query (as-> (meta/table-metadata :orders) <>
(lib/query meta/metadata-provider <>)
(lib/join <> -1 (->> (meta/table-metadata :people)
- (lib/suggested-join-condition <> -1)
- vector
+ (lib/suggested-join-conditions <> -1)
(lib/join-clause (meta/table-metadata :people)))))
fields [[:field {} (meta/id :orders :id)]
[:field {} (meta/id :orders :subtotal)]
@@ -835,8 +834,7 @@
(let [query (as-> (meta/table-metadata :orders) <>
(lib/query meta/metadata-provider <>)
(lib/join <> -1 (->> (meta/table-metadata :people)
- (lib/suggested-join-condition <> -1)
- vector
+ (lib/suggested-join-conditions <> -1)
(lib/join-clause (meta/table-metadata :people)))))
all-columns (lib/returned-columns query)
table-columns (lib/fieldable-columns query -1)
@@ -992,8 +990,7 @@
(let [query (as-> (meta/table-metadata :orders) <>
(lib/query meta/metadata-provider <>)
(lib/join <> -1 (->> (meta/table-metadata :people)
- (lib/suggested-join-condition <> -1)
- vector
+ (lib/suggested-join-conditions <> -1)
(lib/join-clause (meta/table-metadata :people)))))
all-columns (lib/returned-columns query)
table-columns (lib/fieldable-columns query -1)
diff --git a/test/metabase/lib/join_test.cljc b/test/metabase/lib/join_test.cljc
index f804b190a31eea6b165ca2cf8064e5ca893aac4c..462e8578a9bce3a2f6a1e0fa3e3a41cfe87d254c 100644
--- a/test/metabase/lib/join_test.cljc
+++ b/test/metabase/lib/join_test.cljc
@@ -837,13 +837,13 @@
(meta/field-metadata :checkins :user-id))])))
:stages first :joins)))))
-(deftest ^:parallel suggested-join-condition-test
+(deftest ^:parallel suggested-join-conditions-test
(testing "DO suggest a join condition for an FK -> PK relationship"
- (are [query] (=? [:=
- {}
- [:field {} (meta/id :venues :category-id)]
- [:field {} (meta/id :categories :id)]]
- (lib/suggested-join-condition
+ (are [query] (=? [[:=
+ {}
+ [:field {} (meta/id :venues :category-id)]
+ [:field {} (meta/id :categories :id)]]]
+ (lib/suggested-join-conditions
query
(meta/table-metadata :categories)))
;; plain query
@@ -853,26 +853,26 @@
(-> lib.tu/venues-query
(lib/aggregate (lib/count))))))
-(deftest ^:parallel suggested-join-condition-pk->fk-test
+(deftest ^:parallel suggested-join-conditions-pk->fk-test
;; this is to preserve the existing behavior from MLv1, it doesn't necessarily make sense, but we don't want to have
;; to update a million tests, right? Once v1-compatible joins lands then maybe we can go in and make this work,
;; since it seems like it SHOULD work.
(testing "DO suggest join conditions for a PK -> FK relationship"
- (is (=? [:=
- {}
- [:field {} (meta/id :categories :id)]
- [:field {} (meta/id :venues :category-id)]]
- (lib/suggested-join-condition
+ (is (=? [[:=
+ {}
+ [:field {} (meta/id :categories :id)]
+ [:field {} (meta/id :venues :category-id)]]]
+ (lib/suggested-join-conditions
(lib/query meta/metadata-provider (meta/table-metadata :categories))
(meta/table-metadata :venues))))))
-(deftest ^:parallel suggested-join-condition-fk-from-join-test
+(deftest ^:parallel suggested-join-conditions-fk-from-join-test
(testing "DO suggest join conditions for a FK -> PK relationship if the FK comes from a join"
- (is (=? [:=
- {}
- [:field {:join-alias "Venues"} (meta/id :venues :category-id)]
- [:field {} (meta/id :categories :id)]]
- (lib/suggested-join-condition
+ (is (=? [[:=
+ {}
+ [:field {:join-alias "Venues"} (meta/id :venues :category-id)]
+ [:field {} (meta/id :categories :id)]]]
+ (lib/suggested-join-conditions
(-> (lib/query meta/metadata-provider (meta/table-metadata :checkins))
(lib/join (-> (lib/join-clause
(meta/table-metadata :venues)
@@ -882,7 +882,7 @@
(lib/with-join-alias "Venues"))))
(meta/table-metadata :categories))))))
-(deftest ^:parallel suggested-join-condition-fk-from-implicitly-joinable-test
+(deftest ^:parallel suggested-join-conditions-fk-from-implicitly-joinable-test
(testing "DON'T suggest join conditions for a FK -> implicitly joinable PK relationship (#34526)"
(let [orders (meta/table-metadata :orders)
card-query (as-> (lib/query meta/metadata-provider orders) q
@@ -898,7 +898,153 @@
:fields (lib/returned-columns card-query)}]})
card (lib.metadata/card metadata-provider 1)
query (lib/query metadata-provider orders)]
- (is (nil? (lib/suggested-join-condition query card))))))
+ (is (nil? (lib/suggested-join-conditions query card))))))
+
+(deftest ^:parallel suggested-join-condition-for-fks-pointing-to-non-pk-columns
+ (testing (str "We should be able to suggest a join condition if any column in the current table is an FK pointing "
+ "to the target, regardless of whether that column is marked as a PK or not")
+ (let [id-user 1
+ id-order 2
+ id-user-id 10
+ id-order-user-id 20
+ metadata-provider (lib.tu/mock-metadata-provider
+ {:database meta/database
+ :tables [{:id id-user
+ :name "user"}
+ {:id id-order
+ :name "order"}]
+ :fields [{:id id-user-id
+ :table-id id-user
+ :name "id"
+ :base-type :type/Integer}
+ {:id id-order-user-id
+ :table-id id-order
+ :name "user_id"
+ :base-type :type/Integer
+ :semantic-type :type/FK
+ :fk-target-field-id id-user-id}]})
+ order (lib.metadata/table metadata-provider id-order)
+ user (lib.metadata/table metadata-provider id-user)]
+ (testing "ORDER joining USER (we have an FK to the joined thing)"
+ (let [query (lib/query metadata-provider order)]
+ (is (=? [[:= {}
+ [:field {} id-order-user-id]
+ [:field {} id-user-id]]]
+ (lib/suggested-join-conditions query user)))))
+ (testing "USER joining ORDER (joined thing has an FK to us)"
+ (let [query (lib/query metadata-provider user)]
+ (is (=? [[:= {}
+ [:field {} id-user-id]
+ [:field {} id-order-user-id]]]
+ (lib/suggested-join-conditions query order))))))))
+
+(deftest ^:parallel suggested-join-conditions-multiple-fks-to-same-column-test
+ (testing "when there are multiple FKs to a table, but they point to the same column, only suggest one or the other"
+ (let [id-user 1
+ id-message 2
+ id-user-id 10
+ id-message-sender-id 20
+ id-message-recipient-id 21
+ metadata-provider (lib.tu/mock-metadata-provider
+ {:database meta/database
+ :tables [{:id id-user
+ :name "user"}
+ {:id id-message
+ :name "message"}]
+ :fields [{:id id-user-id
+ :table-id id-user
+ :name "id"
+ :base-type :type/Integer
+ :semantic-type :type/PK}
+ {:id id-message-sender-id
+ :table-id id-message
+ :name "sender_id"
+ :base-type :type/Integer
+ :semantic-type :type/FK
+ :fk-target-field-id id-user-id}
+ {:id id-message-recipient-id
+ :table-id id-message
+ :name "recipient_id"
+ :base-type :type/Integer
+ :semantic-type :type/FK
+ :fk-target-field-id id-user-id}]})
+ message (lib.metadata/table metadata-provider id-message)
+ user (lib.metadata/table metadata-provider id-user)]
+ (testing "MESSAGE joining USER (we have 2 FKs to the joined thing)"
+ (let [query (lib/query metadata-provider message)]
+ (is (=? [[:= {}
+ ;; doesn't particularly matter which one gets picked, and order is not necessarily determinate
+ [:field {} #(contains? #{id-message-sender-id id-message-recipient-id} %)]
+ [:field {} id-user-id]]]
+ (lib/suggested-join-conditions query user)))))
+ (testing "USER joining MESSAGE (joined thing has 2 FKs to us)"
+ (let [query (lib/query metadata-provider user)]
+ (is (=? [[:= {}
+ [:field {} id-user-id]
+ [:field {} #(contains? #{id-message-sender-id id-message-recipient-id} %)]]]
+ (lib/suggested-join-conditions query message))))))))
+
+(deftest ^:parallel suggested-join-conditions-multiple-fks-to-different-columns-test
+ (testing "when there are multiple FKs to a table, and they point to different columns, suggest multiple join conditions (#34184)"
+ ;; let's pretend we live in crazy land and the PK for USER is ID + EMAIL (a composite PK) and ORDER has USER_ID
+ ;; and USER_EMAIL
+ (let [id-user 1
+ id-order 2
+ id-user-id 10
+ id-user-email 11
+ id-order-user-id 20
+ id-order-user-email 21
+ metadata-provider (lib.tu/mock-metadata-provider
+ {:database meta/database
+ :tables [{:id id-user
+ :name "user"}
+ {:id id-order
+ :name "order"}]
+ :fields [{:id id-user-id
+ :table-id id-user
+ :name "id"
+ :base-type :type/Integer}
+ {:id id-user-email
+ :table-id id-user
+ :name "email"
+ :base-type :type/Text}
+ {:id id-order-user-id
+ :table-id id-order
+ :name "user_id"
+ :base-type :type/Integer
+ :semantic-type :type/FK
+ :fk-target-field-id id-user-id}
+ {:id id-order-user-email
+ :table-id id-order
+ :name "user_email"
+ :base-type :type/Text
+ :semantic-type :type/FK
+ :fk-target-field-id id-user-email}]})
+ order (lib.metadata/table metadata-provider id-order)
+ user (lib.metadata/table metadata-provider id-user)
+ ;; the order the conditions get returned in is indeterminate, so for convenience let's just sort them by
+ ;; Field IDs so we get consistent results in the test assertions.
+ sort-conditions #(sort-by (fn [[_= _opts [_field _opts lhs-id, :as _lhs] [_field _opts rhs-id, :as _rhs]]]
+ [lhs-id rhs-id])
+ %)]
+ (testing "ORDER joining USER (we have a composite FK to the joined thing)"
+ (let [query (lib/query metadata-provider order)]
+ (is (=? [[:= {}
+ [:field {} id-order-user-id]
+ [:field {} id-user-id]]
+ [:= {}
+ [:field {} id-order-user-email]
+ [:field {} id-user-email]]]
+ (sort-conditions (lib/suggested-join-conditions query user))))))
+ (testing "USER joining ORDER (joined thing has a composite FK to us)"
+ (let [query (lib/query metadata-provider user)]
+ (is (=? [[:= {}
+ [:field {} id-user-id]
+ [:field {} id-order-user-id]]
+ [:= {}
+ [:field {} id-user-email]
+ [:field {} id-order-user-email]]]
+ (sort-conditions (lib/suggested-join-conditions query order)))))))))
(deftest ^:parallel join-conditions-test
(let [joins (lib/joins lib.tu/query-with-join)]
diff --git a/test/metabase/lib/test_util/mocks_31769.cljc b/test/metabase/lib/test_util/mocks_31769.cljc
index 770e8259cba97a6f91caaee713d7a7f473736bc7..faf1c3d202c76b8269cc96b0d016ee7e14a59ff0 100644
--- a/test/metabase/lib/test_util/mocks_31769.cljc
+++ b/test/metabase/lib/test_util/mocks_31769.cljc
@@ -22,10 +22,10 @@
(as-> (lib/query metadata-provider orders) q
(lib/join q (let [products (lib.metadata/table metadata-provider (id-fn :products))]
(-> (lib/join-clause products)
- (lib/with-join-conditions [(lib/suggested-join-condition q products)]))))
+ (lib/with-join-conditions (lib/suggested-join-conditions q products)))))
(lib/join q (let [people (lib.metadata/table metadata-provider (id-fn :people))]
(-> (lib/join-clause people)
- (lib/with-join-conditions [(lib/suggested-join-condition q people)]))))
+ (lib/with-join-conditions (lib/suggested-join-conditions q people)))))
(lib/breakout q (let [breakout (m/find-first #(and (= (:id %) (id-fn :products :category))
(not= (:lib/source %) :source/implicitly-joinable))
(lib/breakoutable-columns q))]
diff --git a/test/metabase/query_processor_test/explicit_joins_test.clj b/test/metabase/query_processor_test/explicit_joins_test.clj
index 7421ecbb528ef89cbfd3818e31f3608773e2de44..3e59b40630dd4a6aaf10ff8f7b0ef56dcc2054bf 100644
--- a/test/metabase/query_processor_test/explicit_joins_test.clj
+++ b/test/metabase/query_processor_test/explicit_joins_test.clj
@@ -378,8 +378,8 @@
(testing "suggested join condition references the FK by name"
(let [query (lib/query metadata-provider (lib.metadata/table metadata-provider (mt/id :people)))
card-meta (lib.metadata/card metadata-provider 3)]
- (is (=? [:= {} [:field {} (mt/id :people :id)] [:field {} cuser-id]]
- (lib/suggested-join-condition query card-meta))))))
+ (is (=? [[:= {} [:field {} (mt/id :people :id)] [:field {} cuser-id]]]
+ (lib/suggested-join-conditions query card-meta))))))
(testing "the query runs and returns correct data"
(is (= {:columns [cid cid2 cuser-id ccount cuser-id2 ccount2]