[MLv2] Only allow implicit join on a column if its real ID is known (#37536)

This is how legacy worked but MLv2 changed this behavior. Even if the
metadata for a native column includes `:semantic-type :type/FK` and
`:fk-target-field-id`, it can't be used for a JOIN unless it's a real
field in the database.

This undoes some hacks from #37079 that were misguided. Fixes #37067.

e2e/test/scenarios/models/models-metadata.cy.spec.js

@@ -6,6 +6,7 @@ import {
   visitDashboard,
   popover,
   openQuestionActions,
+  queryBuilderHeader,
   questionInfoButton,
   addOrUpdateDashboardCard,
   openColumnOptions,
@@ -290,7 +291,7 @@ describe("scenarios > models metadata", () => {
           name: "Native Model",
           dataset: true,
           native: {
-            query: "select * from orders limit 10",
+            query: "select * from orders limit 100",
           },
         },
         { wrapId: true, idAlias: "modelId" },
@@ -350,6 +351,40 @@ describe("scenarios > models metadata", () => {
       });
     });
 
+    it("should show implicit joins on FK columns with real DB columns (#37067)", () => {
+      cy.get("@modelId").then(modelId => {
+        cy.visit(`/model/${modelId}`);
+        cy.wait("@dataset");
+
+        // Drill to People table
+        // FK column is mapped to real DB column
+        queryBuilderHeader().button("Filter").click();
+
+        modal().within(() => {
+          cy.findByRole("tablist").within(() => {
+            cy.get("button").should("have.length", 2); // Just the two we're expecting and not the other fake FK.
+            cy.findByText("Native Model").should("exist");
+
+            const userTab = cy.findByText("User");
+            userTab.should("exist");
+            userTab.click();
+          });
+
+          cy.findByTestId("filter-column-Source").findByText("Twitter").click();
+          cy.findByTestId("apply-filters").click();
+        });
+
+        cy.wait("@dataset");
+        cy.findByTestId("question-row-count")
+          .invoke("text")
+          .should("match", /Showing \d+ rows/);
+        cy.findByTestId("question-row-count").should(
+          "not.contain",
+          "Showing 100 rows",
+        );
+      });
+    });
+
     it("should allow drills on FK columns from dashboards", () => {
       cy.get("@modelId").then(modelId => {
         cy.createDashboard().then(response => {

src/metabase/lib/card.cljc

@@ -77,29 +77,34 @@
                  ;; ignore `:field-ref`, it's very likely a legacy field ref, and it's probably wrong either way. We
                  ;; can always calculate a new one.
                  (dissoc :field-ref))]
-     (merge
-      {:base-type :type/*, :lib/type :metadata/column}
-      (when-let [field-id (:id col)]
-        (try
-          (lib.metadata/field metadata-providerable field-id)
-          (catch #?(:clj Throwable :cljs :default) _
-            nil)))
-      col
-      {:lib/type                :metadata/column
-       :lib/source              :source/card
-       :lib/source-column-alias ((some-fn :lib/source-column-alias :name) col)}
-      (when card-or-id
-        {:lib/card-id (u/the-id card-or-id)})
-      (when (and *force-broken-card-refs*
-                 ;; never force broken refs for datasets, because datasets can have give columns with completely
-                 ;; different names the Field ID of a different column, somehow. See #22715
-                 (or
-                  ;; we can only do this check if `card-or-id` is passed in.
-                  (not card-or-id)
-                  (not (:dataset (lib.metadata/card metadata-providerable (u/the-id card-or-id))))))
-        {::force-broken-id-refs true}
-        #_(when-let [legacy-join-alias (:source-alias col)]
-            {:lib/desired-column-alias (lib.util/format "%s__%s" legacy-join-alias (:name col))}))))))
+     (cond-> (merge
+               {:base-type :type/*, :lib/type :metadata/column}
+               (when-let [field-id (:id col)]
+                 (try
+                   (lib.metadata/field metadata-providerable field-id)
+                   (catch #?(:clj Throwable :cljs :default) _
+                     nil)))
+               col
+               {:lib/type                :metadata/column
+                :lib/source              :source/card
+                :lib/source-column-alias ((some-fn :lib/source-column-alias :name) col)})
+       card-or-id
+       (assoc :lib/card-id (u/the-id card-or-id))
+
+       (and *force-broken-card-refs*
+            ;; never force broken refs for datasets, because datasets can have give columns with completely
+            ;; different names the Field ID of a different column, somehow. See #22715
+            (or
+              ;; we can only do this check if `card-or-id` is passed in.
+              (not card-or-id)
+              (not (:dataset (lib.metadata/card metadata-providerable (u/the-id card-or-id))))))
+       (assoc ::force-broken-id-refs true)
+
+       ;; If the incoming col doesn't have `:semantic-type :type/FK`, drop `:fk-target-field-id`.
+       ;; This comes up with metadata on SQL cards, which might be linked to their original DB field but should not be
+       ;; treated as FKs unless the metadata is configured accordingly.
+       (not= (:semantic-type col) :type/FK)
+       (assoc :fk-target-field-id nil)))))
 
 (def ^:private CardColumnMetadata
   [:merge

src/metabase/lib/column_group.cljc

 (ns metabase.lib.column-group
   (:require
    [metabase.lib.card :as lib.card]
-   [metabase.lib.equality :as lib.equality]
    [metabase.lib.join :as lib.join]
    [metabase.lib.join.util :as lib.join.util]
    [metabase.lib.metadata :as lib.metadata]
    [metabase.lib.metadata.calculation :as lib.metadata.calculation]
-   [metabase.lib.options :as lib.options]
    [metabase.lib.schema.common :as lib.schema.common]
    [metabase.lib.schema.id :as lib.schema.id]
    [metabase.lib.util :as lib.util]
@@ -47,8 +45,7 @@
          (>= (count (keys (select-keys m [:join-alias :table-id :card-id]))) 1))]]]
     [:group-type/join.implicit
      [:map
-      ;; TODO: Strings are allowed here to work around a QP bug; see #37067.
-      [:fk-field-id [:or [:ref ::lib.schema.id/field] ::lib.schema.common/non-blank-string]]]]]])
+      [:fk-field-id [:ref ::lib.schema.id/field]]]]]])
 
 (defmethod lib.metadata.calculation/metadata-method :metadata/column-group
   [_query _stage-number column-group]
@@ -97,18 +94,7 @@
 (defmethod display-info-for-group-method :group-type/join.implicit
   [query stage-number {:keys [fk-field-id], :as _column-group}]
   (merge
-   (when-let [field (if (string? fk-field-id)
-                      ;; TODO: This is probably working around a QP bug. See #37067 - the `data.cols` from the API has
-                      ;; fk_field_id: "customer_id" but I think that's wrong and it should not have dropped the real
-                      ;; field ID somewhere in the QP.
-                      (some->> (lib.util/query-stage query stage-number)
-                               (lib.metadata.calculation/visible-columns query stage-number)
-                               (lib.equality/find-matching-column
-                                 query stage-number
-                                 (lib.options/ensure-uuid [:field {:base-type :type/*} fk-field-id]))
-                               :id
-                               (lib.metadata/field query))
-                      (lib.metadata/field query fk-field-id))]
+   (when-let [field (lib.metadata/field query fk-field-id)]
      (let [field-info (lib.metadata.calculation/display-info query stage-number field)]
        ;; Implicitly joined column pickers don't use the target table's name, they use the FK field's name with
        ;; "ID" dropped instead.

src/metabase/lib/metadata/calculation.cljc

@@ -551,9 +551,9 @@
 
 (defn implicitly-joinable-columns
   "Columns that are implicitly joinable from some other columns in `column-metadatas`. To be joinable, the column has to
-  have appropriate FK metadata, i.e. have an `:fk-target-field-id` pointing to another Field. (I think we only include
-  this information for Databases that support FKs and joins, so I don't think we need to do an additional DB feature
-  check here.)
+  have (1) appropriate FK metadata, i.e. have an `:fk-target-field-id` pointing to another Field, and (2) have a numeric
+  `:id`, i.e. be a real database column that can be used in a JOIN condition. (I think we only include this information
+  for Databases that support FKs and joins, so I don't think we need to do an additional DB feature check here.)
 
   Does not include columns from any Tables that are already explicitly joined.
 
@@ -562,6 +562,8 @@
   (let [existing-table-ids (into #{} (map :table-id) column-metadatas)]
     (into []
           (comp (filter :fk-target-field-id)
+                (filter :id)
+                (filter (comp number? :id))
                 (map (fn [{source-field-id :id, :keys [fk-target-field-id] :as source}]
                        (-> (lib.metadata/field query fk-target-field-id)
                            (assoc ::source-field-id source-field-id

test/metabase/lib/column_group_test.cljc

@@ -412,25 +412,3 @@
                {:lib/desired-column-alias "PEOPLE__via__USER_ID__LATITUDE_2", :fk-join-alias "Mock orders card"}
                {:lib/desired-column-alias "PEOPLE__via__USER_ID__CREATED_AT_2", :fk-join-alias "Mock orders card"}]
               (::lib.column-group/columns product-2))))))
-
-(deftest ^:parallel string-fk-id-test
-  (testing "models sometimes result in a string FK ID - those should be correctly looked up (#37067)"
-    (let [query   (lib/query meta/metadata-provider (meta/table-metadata :orders))
-          columns (filter #(= (:table-id %) (meta/id :products)) (lib/visible-columns query))
-          group   {:lib/type                     :metadata/column-group
-                   ::lib.column-group/group-type :group-type/join.implicit
-                   ::lib.column-group/columns    columns
-                   :fk-field-id                  "PRODUCT_ID"}]
-      (is (=? {:display-name           "Product ID"
-               :name                   "PRODUCT_ID"
-               :long-display-name      "Product ID"
-               :fk-reference-name      "Product"
-               :is-from-join           false
-               :is-implicitly-joinable true
-               :semantic-type          :type/FK
-               :effective-type         :type/Integer
-               :table                  {:display-name      "Orders"
-                                        :long-display-name "Orders"
-                                        :is-source-table   true
-                                        :name              "ORDERS"}}
-              (lib/display-info query 0 group))))))

test/metabase/lib/field_test.cljc

@@ -625,6 +625,7 @@
                :base-type                :type/*
                :id                       4
                :name                     "Field 4"
+               :fk-target-field-id       nil
                :lib/source               :source/card
                :lib/card-id              3
                :lib/source-column-alias  "Field 4"
@@ -635,6 +636,7 @@
               :effective-type          :type/Text
               :id                      4
               :name                    "Field 4"
+              :fk-target-field-id      nil
               :display-name            "Field 4"
               :lib/card-id             3
               :lib/source              :source/card

test/metabase/lib/metadata/calculation_test.cljc

@@ -2,6 +2,7 @@
   (:require
    #?@(:cljs ([metabase.test-runner.assert-exprs.approximately-equal]))
    [clojure.test :refer [deftest is testing]]
+   [medley.core :as m]
    [metabase.lib.core :as lib]
    [metabase.lib.metadata :as lib.metadata]
    [metabase.lib.metadata.calculation :as lib.metadata.calculation]
@@ -200,3 +201,76 @@
                                (for [field (meta/fields :products)]
                                  (meta/field-metadata :products field))))
               (lib/visible-columns query -1 (lib.util/query-stage query -1)))))))
+
+(deftest ^:parallel implicitly-joinable-requires-numeric-id-test
+  (testing "implicit join requires real field IDs, so SQL models need to provide that metadata (#37067)"
+    (let [model (assoc (lib.tu/mock-cards :orders/native) :dataset true)
+          mp    (lib.tu/metadata-provider-with-mock-card model)
+          query (lib/query mp model)]
+      (testing "without FK metadata, only the own columns are returned"
+        (is (= 9 (count (lib/visible-columns query))))
+        (is (= []
+               (->> (lib/visible-columns query)
+                    (remove (comp #{:source/card} :lib/source)))))))
+
+    (testing "metadata for the FK target field is not sufficient"
+      (let [base    (lib.tu/mock-cards :orders/native)
+            with-fk (for [col (:result-metadata base)]
+                      (if (= (:name col) "USER_ID")
+                        (assoc col :fk-target-field-id (meta/id :people :id))
+                        col))
+            model   (assoc base
+                           :dataset         true
+                           :result-metadata with-fk)
+            mp      (lib.tu/metadata-provider-with-mock-card model)
+            query   (lib/query mp model)]
+        (is (= 9 (count (lib/visible-columns query))))
+        (is (= []
+               (->> (lib/visible-columns query)
+                    (remove (comp #{:source/card} :lib/source)))))))
+
+    (testing "an ID for the FK field itself is not sufficient"
+      (let [base    (lib.tu/mock-cards :orders/native)
+            with-id (for [col (:result-metadata base)]
+                      (merge col
+                             (when (= (:name col) "USER_ID")
+                               {:id            (meta/id :orders :user-id)
+                                :semantic-type nil})))
+            model   (assoc base
+                           :dataset         true
+                           :result-metadata with-id)
+            mp      (lib.tu/metadata-provider-with-mock-card model)
+            query   (lib/query mp model)]
+        (is (= 9 (count (lib/visible-columns query))))
+        (is (= []
+               (->> (lib/visible-columns query)
+                    (remove (comp #{:source/card} :lib/source)))))))
+    (testing "the ID and :semantic-type :type/FK are sufficient for an implicit join"
+      (let [base          (lib.tu/mock-cards :orders/native)
+            with-fk       (for [col (:result-metadata base)]
+                            (merge col
+                                   (when (= (:name col) "USER_ID")
+                                     {:id            (meta/id :orders :user-id)
+                                      :semantic-type :type/FK})))
+            model         (assoc base
+                                 :dataset         true
+                                 :result-metadata with-fk)
+            mp            (lib.tu/metadata-provider-with-mock-card model)
+            query         (lib/query mp model)
+            fields-of     (fn [table-kw order-fn]
+                            (->> (meta/fields table-kw)
+                                 (map #(meta/field-metadata table-kw %))
+                                 (sort-by order-fn)))
+            orders-fields (into {} (for [[index field] (m/indexed ["ID" "SUBTOTAL" "TOTAL" "TAX" "DISCOUNT" "QUANTITY"
+                                                                   "CREATED_AT" "PRODUCT_ID" "USER_ID"])]
+                                     [field index]))
+            orders-cols   (fields-of :orders (comp orders-fields :name))
+            people-cols   (fields-of :people :position)]
+        (is (= 22 (count (lib/visible-columns query))))
+        (is (=? (concat (for [col orders-cols]
+                          {:name       (:name col)
+                           :lib/source :source/card})
+                        (for [col people-cols]
+                          {:name       (:name col)
+                           :lib/source :source/implicitly-joinable}))
+                (lib/visible-columns query)))))))

test/metabase/lib/test_util.cljc

@@ -220,7 +220,7 @@
                                   (->> (meta/fields table)
                                        (map (partial meta/field-metadata table))
                                        (sort-by :id)
-                                       (mapv #(if native? (dissoc % :table-id :id) %)))}))])))
+                                       (mapv #(if native? (dissoc % :table-id :id :fk-target-field-id) %)))}))])))
         (meta/tables)))
 
 (defn metadata-provider-with-mock-card [card]