Fix field_refs returned for implicit join fields (#10380)

src/metabase/query_processor/middleware/annotate.clj

@@ -114,41 +114,54 @@
     {:base_type    :type/Float
      :special_type :type/Number}))
 
-(s/defn ^:private col-info-for-field-clause :- su/Map
+(s/defn ^:private col-info-for-field-clause :- {:field_ref mbql.s/Field, s/Keyword s/Any}
   [{:keys [source-metadata expressions], :as inner-query} :- su/Map, clause :- mbql.s/Field]
   ;; for various things that can wrap Field clauses recurse on the wrapped Field but include a little bit of info
   ;; about the clause doing the wrapping
   (mbql.u/match-one clause
     [:binning-strategy field strategy _ resolved-options]
-    (assoc (col-info-for-field-clause inner-query field)
-      :binning_info (assoc (u/snake-keys resolved-options)
-                      :binning_strategy strategy))
+    (let [recursive-info (col-info-for-field-clause inner-query field)]
+      (assoc recursive-info
+        :binning_info (assoc (u/snake-keys resolved-options)
+                        :binning_strategy strategy)
+        :field_ref    (assoc (vec &match) 1 (:field_ref recursive-info))))
 
     [:datetime-field field unit]
-    (assoc (col-info-for-field-clause inner-query field) :unit unit)
-
-    [:joined-field alias field]
-    (let [{:keys [fk-field-id], :as join} (join-with-alias inner-query alias)]
-      (-> (col-info-for-field-clause inner-query field)
-          (assoc :fk_field_id fk-field-id)
-          (update :display_name display-name-for-joined-field join)))
+    (let [recursive-info (col-info-for-field-clause inner-query field)]
+      (assoc recursive-info
+        :unit      unit
+        :field_ref (assoc (vec &match) 1 (:field_ref recursive-info))))
+
+    [:joined-field join-alias field]
+    (let [{:keys [fk-field-id], :as join} (join-with-alias inner-query join-alias)]
+      (let [recursive-info (col-info-for-field-clause inner-query field)]
+        (-> recursive-info
+            (merge (when fk-field-id {:fk_field_id fk-field-id}))
+            (assoc :field_ref (if fk-field-id
+                                [:fk-> [:field-id fk-field-id] field]
+                                (assoc (vec &match) 2 (:field_ref recursive-info))))
+            (update :display_name display-name-for-joined-field join))))
 
     ;; TODO - should be able to remove this now
     [:fk-> [:field-id source-field-id] field]
-    (assoc (col-info-for-field-clause inner-query field) :fk_field_id source-field-id)
+    (assoc (col-info-for-field-clause inner-query field)
+      :field_ref  &match
+      :fk_field_id source-field-id)
 
     ;; TODO - should be able to remove this now
     ;; for FKs where source is a :field-literal don't include `:fk_field_id`
     [:fk-> _ field]
-    (recur field)
+    (assoc (col-info-for-field-clause inner-query field)
+      :field_ref &match)
 
     ;; for field literals, look for matching `source-metadata`, and use that if we can find it; otherwise generate
     ;; basic info based on the content of the field literal
     [:field-literal field-name field-type]
-    (or (some #(when (= (:name %) field-name) %) source-metadata)
-        {:name         field-name
-         :base_type    field-type
-         :display_name (humanization/name->human-readable-name field-name)})
+    (assoc (or (some #(when (= (:name %) field-name) %) source-metadata)
+               {:name         field-name
+                :base_type    field-type
+                :display_name (humanization/name->human-readable-name field-name)})
+      :field_ref &match)
 
     [:expression expression-name]
     (merge
@@ -158,14 +171,16 @@
      {:name            expression-name
       :display_name    expression-name
       ;; provided so the FE can add easily add sorts and the like when someone clicks a column header
-      :expression_name expression-name})
+      :expression_name expression-name
+      :field_ref       &match})
 
     [:field-id id]
     (let [{parent-id :parent_id, :as field} (dissoc (qp.store/field id) :database_type)]
-      (if-not parent-id
-        field
-        (let [parent (col-info-for-field-clause inner-query [:field-id parent-id])]
-          (update field :name #(str (:name parent) \. %)))))
+      (assoc (if-not parent-id
+               field
+               (let [parent (col-info-for-field-clause inner-query [:field-id parent-id])]
+                 (update field :name #(str (:name parent) \. %))))
+        :field_ref &match))
 
     ;; we should never reach this if our patterns are written right so this is more to catch code mistakes than
     ;; something the user should expect to see
@@ -358,16 +373,14 @@
   [{:keys [fields], :as inner-query} :- su/Map]
   (for [field fields]
     (assoc (col-info-for-field-clause inner-query field)
-      :source    :fields
-      :field_ref field)))
+      :source :fields)))
 
 (s/defn ^:private cols-for-ags-and-breakouts
   [{aggregations :aggregation, breakouts :breakout, :as inner-query} :- su/Map]
   (concat
    (for [breakout breakouts]
      (assoc (col-info-for-field-clause inner-query breakout)
-       :source    :breakout
-       :field_ref breakout))
+       :source :breakout))
    (for [[i aggregation] (m/indexed aggregations)]
      (assoc (col-info-for-aggregation-clause inner-query aggregation)
        :source    :aggregation

test/metabase/query_processor/middleware/annotate_test.clj

@@ -91,14 +91,14 @@
       {:columns [:name]}))))
 
 ;; we should get `:fk_field_id` and information where possible when using `:joined-field` clauses; display_name should
-;; include the joined table
+;; include the joined table (for IMPLICIT JOINS)
 (expect
   [(data/$ids venues
      (assoc (info-for-field :categories :name)
        :display_name "Venues → Name"
-       :fk_field_id  %category_id
        :source       :fields
-       :field_ref    &CATEGORIES__via__CATEGORY_ID.categories.name))]
+       :field_ref    $category_id->categories.name
+       :fk_field_id  %category_id))]
   (qp.test-util/with-everything-store
     (data/$ids venues
       (doall
@@ -112,14 +112,34 @@
                            :fk-field-id  %category_id}]}}
         {:columns [:name]})))))
 
+;; for EXPLICIT JOINS (which do not include an `:fk-field-id` in the Join info) the returned `:field_ref` should be a
+;; `joined-field` clause instead of an `fk->` clause
+(expect
+  [(data/$ids venues
+     (assoc (info-for-field :categories :name)
+       :display_name "Venues → Name"
+       :source       :fields
+       :field_ref    &CATEGORIES__via__CATEGORY_ID.categories.name))]
+  (qp.test-util/with-everything-store
+    (data/$ids venues
+      (doall
+       (annotate/column-info
+        {:type  :query
+         :query {:fields [&CATEGORIES__via__CATEGORY_ID.categories.name]
+                 :joins  [{:alias        "CATEGORIES__via__CATEGORY_ID"
+                           :source-table $$venues
+                           :condition    [:= $category_id &CATEGORIES__via__CATEGORY_ID.categories.id]
+                           :strategy     :left-join}]}}
+        {:columns [:name]})))))
+
 ;; we shuld use the `display_name` of a Table instead of its `name` in joined display names
 (expect
   [(data/$ids venues
      (assoc (info-for-field :categories :name)
        :display_name "Geographical locations to share Tips about → Name" ; RIP GeoTips
-       :fk_field_id  %category_id
        :source       :fields
-       :field_ref    &CATEGORIES__via__CATEGORY_ID.categories.name))]
+       :field_ref    $category_id->categories.name
+       :fk_field_id  %category_id))]
   (qp.test-util/with-everything-store
     (data/$ids venues
       (tu/with-temp-vals-in-db Table $$venues {:display_name "Geographical locations to share Tips about"}
@@ -140,9 +160,9 @@
   [(data/$ids venues
      (assoc (info-for-field :categories :name)
        :display_name "cats → Name"
-       :fk_field_id  %category_id
        :source       :fields
-       :field_ref    &cats.categories.name))]
+       :field_ref    $category_id->categories.name
+       :fk_field_id  %category_id))]
   (qp.test-util/with-everything-store
     (data/$ids venues
       (doall
@@ -220,7 +240,7 @@
 
 ;; For fields with parents we should return them with a combined name including parent's name
 (tt/expect-with-temp [Field [parent {:name "parent", :table_id (data/id :venues)}]
-                      Field [child  {:name "child",  :table_id (data/id :venues), :parent_id (u/get-id parent)}]]
+                      Field [child  {:name "child", :table_id (data/id :venues), :parent_id (u/get-id parent)}]]
   {:description     nil
    :table_id        (data/id :venues)
    :special_type    nil
@@ -231,14 +251,15 @@
    :visibility_type :normal
    :display_name    "Child"
    :fingerprint     nil
+   :field_ref       [:field-id (u/get-id child)]
    :base_type       :type/Text}
   (qp.test-util/with-everything-store
     (#'annotate/col-info-for-field-clause {} [:field-id (u/get-id child)])))
 
 ;; nested-nested fields should include grandparent name (etc)
 (tt/expect-with-temp [Field [grandparent {:name "grandparent", :table_id (data/id :venues)}]
-                      Field [parent      {:name "parent",      :table_id (data/id :venues), :parent_id (u/get-id grandparent)}]
-                      Field [child       {:name "child",       :table_id (data/id :venues), :parent_id (u/get-id parent)}]]
+                      Field [parent      {:name "parent", :table_id (data/id :venues), :parent_id (u/get-id grandparent)}]
+                      Field [child       {:name "child", :table_id (data/id :venues), :parent_id (u/get-id parent)}]]
   {:description     nil
    :table_id        (data/id :venues)
    :special_type    nil
@@ -249,6 +270,7 @@
    :visibility_type :normal
    :display_name    "Child"
    :fingerprint     nil
+   :field_ref       [:field-id (u/get-id child)]
    :base_type       :type/Text}
   (qp.test-util/with-everything-store
     (#'annotate/col-info-for-field-clause {} [:field-id (u/get-id child)])))
@@ -258,10 +280,11 @@
   {:name         "sum"
    :display_name "sum of User ID"
    :base_type    :type/Integer
+   :field_ref    [:field-literal "sum" :type/Integer]
    :special_type :type/FK}
   (qp.test-util/with-everything-store
     (#'annotate/col-info-for-field-clause
-     {:source-metadata [{:name "abc", :display_name "another Field",  :base_type :type/Integer, :special_type :type/FK}
+     {:source-metadata [{:name "abc", :display_name "another Field", :base_type :type/Integer, :special_type :type/FK}
                         {:name "sum", :display_name "sum of User ID", :base_type :type/Integer, :special_type :type/FK}]}
      [:field-literal "sum" :type/Integer])))
 

test/metabase/query_processor_test.clj

@@ -12,7 +12,6 @@
             [metabase.models
              [field :refer [Field]]
              [table :refer [Table]]]
-            [metabase.query-processor.middleware.add-implicit-joins :as add-implicit-joins]
             [metabase.test.data :as data]
             [metabase.test.data
              [datasets :as datasets]
@@ -172,11 +171,10 @@
   (let [source-col              (col source-table-kw source-field-kw)
         dest-col                (col dest-table-kw dest-field-kw)
         dest-table-display-name (db/select-one-field :display_name Table :id (data/id dest-table-kw))
-        dest-table-name         (db/select-one-field :name Table :id (data/id dest-table-kw))
-        join-alias              (#'add-implicit-joins/join-alias dest-table-name (:name source-col))]
+        dest-table-name         (db/select-one-field :name Table :id (data/id dest-table-kw))]
     (-> dest-col
         (update :display_name (partial format "%s → %s" (or dest-table-display-name dest-table-name)))
-        (assoc :field_ref   [:joined-field join-alias [:field-id (:id dest-col)]]
+        (assoc :field_ref   [:fk-> [:field-id (:id source-col)] [:field-id (:id dest-col)]]
                :fk_field_id (:id source-col)))))
 
 (declare cols)

test/metabase/query_processor_test/implicit_joins_test.clj

 (ns metabase.query-processor-test.implicit-joins-test
   "Test for JOIN behavior."
-  (:require [metabase
+  (:require [expectations :refer [expect]]
+            [metabase
              [driver :as driver]
              [query-processor-test :as qp.test]]
             [metabase.test.data :as data]
@@ -108,6 +109,17 @@
         :limit    10}))
    [:status :error]))
 
+;; Implicit joins should come back with `:fk->` field refs
+(expect
+  (data/$ids venues $category_id->categories.name)
+  (-> (qp.test/cols
+        (data/run-mbql-query :venues
+          {:fields   [$category_id->categories.name]
+           :order-by [[:asc $id]]
+           :limit    1}))
+      first
+      :field_ref))
+
 
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                                 MULTIPLE JOINS                                                 |

test/metabase/query_processor_test/remapping_test.clj

@@ -7,7 +7,6 @@
              [dimension :refer [Dimension]]
              [field :refer [Field]]]
             [metabase.query-processor.middleware.add-dimension-projections :as add-dimension-projections]
-            [metabase.query-processor.test-util :as qp.test-util]
             [metabase.test.data :as data]
             [metabase.test.data.datasets :as datasets]
             [toucan.db :as db]))
@@ -59,9 +58,7 @@
               :display_name  "Foo"
               :name          (data/format-name "name_2")
               :remapped_from (data/format-name "category_id")
-              :field_ref     [:joined-field
-                              (qp.test-util/fk-table-alias-name $$categories %category_id)
-                              $categories.name]))]}
+              :field_ref     $category_id->categories.name))]}
   (data/with-temp-objects
     (data/create-venue-category-fk-remapping "Foo")
     (select-columns (set (map data/format-name ["name" "price" "name_2"]))
@@ -84,9 +81,7 @@
                      :display_name  "Foo"
                      :name          (data/format-name "name_2")
                      :remapped_from (data/format-name "category_id")
-                     :field_ref     [:joined-field
-                                     (qp.test-util/fk-table-alias-name $$categories %category_id)
-                                     $categories.name]))]}
+                     :field_ref     $category_id->categories.name))]}
   (data/with-temp-objects
     (data/create-venue-category-fk-remapping "Foo")
     (select-columns (set (map data/format-name ["name" "price" "name_2"]))