Skip to content

Commit

Permalink
[MLv2] Look up FK columns in visible-columns, not metadata (#37771)
Browse files Browse the repository at this point in the history
Models can override the `:id` and `:fk-target-field-id` of a column, and
can even lie and treat a quantity as a FK. Therefore we have to use the
query's view of the column and not the global field from the metadata.

This is a speculative fix for #36400, which I can't reproduce locally.
  • Loading branch information
bshepherdson committed Jan 18, 2024
1 parent 7634fad commit 162349c
Showing 1 changed file with 10 additions and 3 deletions.
13 changes: 10 additions & 3 deletions src/metabase/lib/column_group.cljc
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
(ns metabase.lib.column-group
(:require
[medley.core :as m]
[metabase.lib.card :as lib.card]
[metabase.lib.join :as lib.join]
[metabase.lib.join.util :as lib.join.util]
Expand Down Expand Up @@ -94,14 +95,20 @@
(defmethod display-info-for-group-method :group-type/join.implicit
[query stage-number {:keys [fk-field-id], :as _column-group}]
(merge
(when-let [field (lib.metadata/field query fk-field-id)]
(let [field-info (lib.metadata.calculation/display-info query stage-number field)]
(when-let [;; TODO: This is clumsy and expensive; there is likely a neater way to find the full FK column.
;; Note that using `lib.metadata/field` is out - we need to respect metadata overrides etc. in models, and
;; `lib.metadata/field` uses the field's original status.
fk-column (->> (lib.util/query-stage query stage-number)
(lib.metadata.calculation/visible-columns query stage-number)
(m/find-first #(and (= (:id %) fk-field-id)
(:fk-target-field-id %))))]
(let [fk-info (lib.metadata.calculation/display-info query stage-number fk-column)]
;; Implicitly joined column pickers don't use the target table's name, they use the FK field's name with
;; "ID" dropped instead.
;; This is very intentional: one table might have several FKs to one foreign table, each with different
;; meaning (eg. ORDERS.customer_id vs. ORDERS.supplier_id both linking to a PEOPLE table).
;; See #30109 for more details.
(update field-info :display-name lib.util/strip-id)))
(update fk-info :display-name lib.util/strip-id)))
{:is-from-join false
:is-implicitly-joinable true}))

Expand Down

0 comments on commit 162349c

Please sign in to comment.