Joined fields: use FK name (implicit) or join alias (explict) (#10564)

src/metabase/query_processor/middleware/annotate.clj

@@ -98,13 +98,17 @@
 
 ;;; --------------------------------------------------- Field Info ---------------------------------------------------
 
-(s/defn ^:private display-name-for-joined-field
-  "Return an appropriate display name for a joined field that includes the table it came from if applicable."
-  [field-display-name {:keys [source-table], join-alias :alias}]
-  (let [join-display-name (if (integer? source-table)
-                            (some (qp.store/table source-table) [:display_name :name])
-                            join-alias)]
-    (format "%s → %s" join-display-name field-display-name)))
+(defn- display-name-for-joined-field
+  "Return an appropriate display name for a joined field. For *explicitly* joined Fields, the qualifier is the join
+  alias; for implicitly joined fields, it is the display name of the foreign key used to create the join."
+  [field-display-name {:keys [fk-field-id], join-alias :alias}]
+  (let [qualifier (if fk-field-id
+                    ;; strip off trailing ` id` from FK display name
+                    (str/replace (:display_name (qp.store/field fk-field-id))
+                                 #"(?i)\sid$"
+                                 "")
+                    join-alias)]
+    (format "%s → %s" qualifier field-display-name)))
 
 (defn- infer-expression-type
   [expression]

test/metabase/query_processor/middleware/annotate_test.clj

@@ -4,16 +4,12 @@
             [metabase
              [driver :as driver]
              [util :as u]]
-            [metabase.models
-             [field :refer [Field]]
-             [table :refer [Table]]]
+            [metabase.models.field :refer [Field]]
             [metabase.query-processor
              [store :as qp.store]
              [test-util :as qp.test-util]]
             [metabase.query-processor.middleware.annotate :as annotate]
-            [metabase.test
-             [data :as data]
-             [util :as tu]]
+            [metabase.test.data :as data]
             [toucan.db :as db]
             [toucan.util.test :as tt]))
 
@@ -91,11 +87,11 @@
       {:columns [:name]}))))
 
 ;; we should get `:fk_field_id` and information where possible when using `:joined-field` clauses; display_name should
-;; include the joined table (for IMPLICIT JOINS)
+;; include the display name of the FK field  (for IMPLICIT JOINS)
 (expect
   [(data/$ids venues
      (assoc (info-for-field :categories :name)
-       :display_name "Venues → Name"
+       :display_name "Category → Name"
        :source       :fields
        :field_ref    $category_id->categories.name
        :fk_field_id  %category_id))]
@@ -117,65 +113,21 @@
 (expect
   [(data/$ids venues
      (assoc (info-for-field :categories :name)
-       :display_name "Venues → Name"
+       :display_name "Categories → Name"
        :source       :fields
-       :field_ref    &CATEGORIES__via__CATEGORY_ID.categories.name))]
+       :field_ref    &Categories.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"
+         :query {:fields [&Categories.categories.name]
+                 :joins  [{:alias        "Categories"
                            :source-table $$venues
-                           :condition    [:= $category_id &CATEGORIES__via__CATEGORY_ID.categories.id]
+                           :condition    [:= $category_id &Categories.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
-       :source       :fields
-       :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"}
-        (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
-                             :fk-field-id  %category_id}]}}
-          {:columns [:name]}))))))
-
-;; when using `:joined-field` clauses for a join a source query (instead of a source table), `display_name` should
-;; include the join alias
-(expect
-  [(data/$ids venues
-     (assoc (info-for-field :categories :name)
-       :display_name "cats → Name"
-       :source       :fields
-       :field_ref    $category_id->categories.name
-       :fk_field_id  %category_id))]
-  (qp.test-util/with-everything-store
-    (data/$ids venues
-      (doall
-       (annotate/column-info
-        {:type  :query
-         :query {:fields [&cats.categories.name]
-                 :joins  [{:alias        "cats"
-                           :source-query {:source-table $$venues}
-                           :condition    [:= $category_id &cats.categories.id]
-                           :strategy     :left-join
-                           :fk-field-id  %category_id}]}}
-        {:columns [:name]})))))
-
 ;; when a `:datetime-field` form is used, we should add in info about the `:unit`
 (expect
   [(data/$ids venues

test/metabase/query_processor_test.clj

@@ -2,16 +2,16 @@
   "Helper functions for various query processor tests. The tests themselves can be found in various
   `metabase.query-processor-test.*` namespaces; there are so many that it is no longer feasible to keep them all in
   this one. Event-based DBs such as Druid are tested in `metabase.driver.event-query-processor-test`."
-  (:require [clojure.set :as set]
+  (:require [clojure
+             [set :as set]
+             [string :as str]]
             [medley.core :as m]
             [metabase
              [driver :as driver]
              [query-processor :as qp]
              [util :as u]]
             [metabase.driver.util :as driver.u]
-            [metabase.models
-             [field :refer [Field]]
-             [table :refer [Table]]]
+            [metabase.models.field :refer [Field]]
             [metabase.test.data :as data]
             [metabase.test.data
              [datasets :as datasets]
@@ -168,12 +168,10 @@
 (defn fk-col
   "Return expected `:cols` info for a Field that came in via an implicit join (i.e, via an `fk->` clause)."
   [source-table-kw source-field-kw, dest-table-kw dest-field-kw]
-  (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))]
+  (let [source-col (col source-table-kw source-field-kw)
+        dest-col   (col dest-table-kw dest-field-kw)]
     (-> dest-col
-        (update :display_name (partial format "%s → %s" (or dest-table-display-name dest-table-name)))
+        (update :display_name (partial format "%s → %s" (str/replace (:display_name source-col) #"(?i)\sid$" "")))
         (assoc :field_ref   [:fk-> [:field-id (:id source-col)] [:field-id (:id dest-col)]]
                :fk_field_id (:id source-col)))))