[MLv2] Use the provided `column-ref` for drills (#38047)

There are cases where the ref as provided is not the same as what a
fresh `(lib.ref/ref column)` would give. This seems mostly to affect
models, where the metadata leaks some of the underlying details like
joins.

Fixes #38034.

src/metabase/lib/drill_thru/column_filter.cljc

@@ -30,10 +30,10 @@
    [metabase.lib.equality :as lib.equality]
    [metabase.lib.filter :as lib.filter]
    [metabase.lib.filter.operator :as lib.filter.operator]
-   [metabase.lib.ref :as lib.ref]
    [metabase.lib.schema :as lib.schema]
    [metabase.lib.schema.drill-thru :as lib.schema.drill-thru]
    [metabase.lib.schema.metadata :as lib.schema.metadata]
+   [metabase.lib.schema.ref :as lib.schema.ref]
    [metabase.lib.stage :as lib.stage]
    [metabase.lib.types.isa :as lib.types.isa]
    [metabase.lib.util :as lib.util]
@@ -55,7 +55,8 @@
   or certain tricky cases of quick filter."
   [query        :- ::lib.schema/query
    stage-number :- :int
-   column       :- ::lib.schema.metadata/column]
+   column       :- ::lib.schema.metadata/column
+   column-ref   :- ::lib.schema.ref/ref]
   (let [next-stage    (->> (lib.util/canonical-stage-index query stage-number)
                            (lib.util/next-stage-number query))
         base          (cond
@@ -72,7 +73,7 @@
                                     :stage-number -1})
         columns       (lib.filter/filterable-columns (:query base) (:stage-number base))
         filter-column (or (lib.equality/find-matching-column
-                            (:query base) (:stage-number base) (lib.ref/ref column) columns)
+                            (:query base) (:stage-number base) column-ref columns)
                           (and (:lib/source-uuid column)
                                (m/find-first #(= (:lib/source-uuid %) (:lib/source-uuid column))
                                              columns)))]
@@ -84,9 +85,9 @@
 
   Note that if the clicked column is an aggregation, filtering by it will require a new stage. Therefore this drill
   returns a possibly-updated `:query` and `:stage-number` along with a `:column` referencing that later stage."
-  [query                  :- ::lib.schema/query
-   stage-number           :- :int
-   {:keys [column value]} :- ::lib.schema.drill-thru/context]
+  [query                             :- ::lib.schema/query
+   stage-number                      :- :int
+   {:keys [column column-ref value]} :- ::lib.schema.drill-thru/context]
   ;; Note: original code uses an addition `clicked.column.field_ref != null` condition.
   (when (and (lib.drill-thru.common/mbql-stage? query stage-number)
              column
@@ -102,7 +103,7 @@
          :type       :drill-thru/column-filter
          :initial-op initial-op}
         ;; When the column we would be filtering on is an aggregation, it can't be filtered without adding a stage.
-        (filter-drill-adjusted-query query stage-number column)))))
+        (filter-drill-adjusted-query query stage-number column column-ref)))))
 
 (defmethod lib.drill-thru.common/drill-thru-info-method :drill-thru/column-filter
   [_query _stage-number {:keys [initial-op]}]

src/metabase/lib/drill_thru/fk_filter.cljc

@@ -26,7 +26,6 @@
    [metabase.lib.metadata :as lib.metadata]
    [metabase.lib.metadata.calculation :as lib.metadata.calculation]
    [metabase.lib.options :as lib.options]
-   [metabase.lib.ref :as lib.ref]
    [metabase.lib.schema :as lib.schema]
    [metabase.lib.schema.drill-thru :as lib.schema.drill-thru]
    [metabase.lib.types.isa :as lib.types.isa]
@@ -40,9 +39,9 @@
 
   Contrast [[metabase.lib.drill-thru.object-details/object-detail-drill]], which shows the details of the foreign
   object."
-  [query                                :- ::lib.schema/query
-   stage-number                        :- :int
-   {:keys [column value], :as _context} :- ::lib.schema.drill-thru/context]
+  [query                                           :- ::lib.schema/query
+   stage-number                                    :- :int
+   {:keys [column column-ref value], :as _context} :- ::lib.schema.drill-thru/context]
   (when (and column
              (some? value)
              (not= value :null)         ; If the FK is null, don't show this option.
@@ -53,7 +52,7 @@
                      (some->> query lib.util/source-card-id (lib.metadata/card query)))]
       {:lib/type :metabase.lib.drill-thru/drill-thru
        :type     :drill-thru/fk-filter
-       :filter   (lib.options/ensure-uuid [:= {} (lib.ref/ref column) value])
+       :filter   (lib.options/ensure-uuid [:= {} column-ref value])
        :column-name (lib.metadata.calculation/display-name query stage-number column :long)
        :table-name (lib.metadata.calculation/display-name query 0 source)})))
 

src/metabase/lib/drill_thru/quick_filter.cljc

@@ -100,9 +100,9 @@
 
   Note that this returns a single `::drill-thru` value with 1 or more `:operators`; these are rendered as a set of small
   buttons in a single row of the drop-down."
-  [query                                :- ::lib.schema/query
-   stage-number                         :- :int
-   {:keys [column value], :as _context} :- ::lib.schema.drill-thru/context]
+  [query                                           :- ::lib.schema/query
+   stage-number                                    :- :int
+   {:keys [column column-ref value], :as _context} :- ::lib.schema.drill-thru/context]
   (when (and (lib.drill-thru.common/mbql-stage? query stage-number)
              column
              (some? value) ; Deliberately allows value :null, only a missing value should fail this test.
@@ -111,7 +111,7 @@
              (not (lib.types.isa/foreign-key? column)))
     ;; For aggregate columns, we want to introduce a new stage when applying the drill-thru.
     ;; [[lib.drill-thru.column-filter/filter-drill-adjusted-query]] handles this. (#34346)
-    (let [adjusted (lib.drill-thru.column-filter/filter-drill-adjusted-query query stage-number column)]
+    (let [adjusted (lib.drill-thru.column-filter/filter-drill-adjusted-query query stage-number column column-ref)]
       (merge {:lib/type   :metabase.lib.drill-thru/drill-thru
               :type       :drill-thru/quick-filter
               :operators  (operators-for (:column adjusted) value)

src/metabase/lib/drill_thru/sort.cljc

@@ -23,7 +23,6 @@
    [metabase.lib.drill-thru.common :as lib.drill-thru.common]
    [metabase.lib.equality :as lib.equality]
    [metabase.lib.order-by :as lib.order-by]
-   [metabase.lib.ref :as lib.ref]
    [metabase.lib.schema :as lib.schema]
    [metabase.lib.schema.drill-thru :as lib.schema.drill-thru]
    [metabase.lib.schema.order-by :as lib.schema.order-by]
@@ -31,11 +30,11 @@
    [metabase.util.malli :as mu]))
 
 (defn- orderable-column?
-  "Is `column` orderable? (Does it appear in [[lib.order-by/orderable-columns]]?)"
-  [query stage-number column]
+  "Is `column-ref` orderable? (Does it appear in [[lib.order-by/orderable-columns]]?)"
+  [query stage-number column-ref]
   (lib.equality/find-matching-column query
                                      stage-number
-                                     (lib.ref/ref column)
+                                     column-ref
                                      (lib.order-by/orderable-columns query stage-number)))
 
 (mu/defn ^:private existing-order-by-clause :- [:maybe ::lib.schema.order-by/order-by]
@@ -51,16 +50,16 @@
 
 (mu/defn sort-drill :- [:maybe ::lib.schema.drill-thru/drill-thru.sort]
   "Sorting on a clicked column."
-  [query                                :- ::lib.schema/query
-   stage-number                         :- :int
-   {:keys [column value], :as _context} :- ::lib.schema.drill-thru/context]
+  [query                                           :- ::lib.schema/query
+   stage-number                                    :- :int
+   {:keys [column column-ref value], :as _context} :- ::lib.schema.drill-thru/context]
   ;; if we have a context with a `:column`, but no `:value`...
   (when (and (lib.drill-thru.common/mbql-stage? query stage-number)
              column
              (nil? value)
              (not (lib.types.isa/structured? column)))
     ;; ...and the column is orderable, we can return a sort drill-thru.
-    (when (orderable-column? query stage-number column)
+    (when (orderable-column? query stage-number column-ref)
       ;; check and see if there is already a sort on this column. If there is, we should only suggest flipping the
       ;; direction to the opposite of what it is now. If there is no existing sort, then return both directions as
       ;; options.

test/metabase/lib/drill_thru/column_filter_test.cljc

@@ -261,3 +261,28 @@
       (is (= "Products" (:source-alias category)))
       (is (= "Products" (-> context :column-ref second :join-alias)))
       (is (some? (:column colfilter))))))
+
+;; TODO: Bring back this test. It doesn't work in CLJ due to the inconsistencies noted in #38558.
+#_(deftest ^:parallel leaky-model-ref-test
+  (testing "input `:column-ref` must be used for the drill, in case a model leaks metadata like `:join-alias` (#38034)"
+    (let [query      (lib/query lib.tu/metadata-provider-with-mock-cards (lib.tu/mock-cards :model/products-and-reviews))
+          retcols    (lib/returned-columns query)
+          by-id      (m/index-by :id retcols)
+          reviews-id (by-id (meta/id :reviews :id))
+          _ (is (some? reviews-id))
+          context    {:column reviews-id
+                      :value  nil
+                      :column-ref (-> reviews-id
+                                      lib/ref
+                                      ((fn [r] (prn r) r))
+                                      (lib.options/update-options select-keys [:lib/uuid :base-type]))}
+          drills     (lib/available-drill-thrus query -1 context)]
+      (lib.drill-thru.tu/test-returns-drill
+        {:drill-type   :drill-thru/column-filter
+         :click-type   :header
+         :query-type   :unaggregated
+         :column-name  "ID_2"
+         :custom-query query
+         :expected     {:type       :drill-thru/column-filter
+                        :initial-op {:short :=}
+                        :column     {:lib/type :metadata/column}}}))))

test/metabase/lib/test_util.cljc

@@ -229,13 +229,42 @@
                                         (mapv #(if native? (dissoc % :table-id :id :fk-target-field-id) %)))}))])))
         table-key-and-ids))
 
+(defn- make-mock-cards-special-cases
+  [metadata-provider]
+  (let [{products "PRODUCTS"
+         reviews  "REVIEWS"} (m/index-by :name (lib.metadata/tables metadata-provider))
+        {pk "ID"}            (m/index-by :name (lib.metadata/fields metadata-provider (:id products)))
+        {fk "PRODUCT_ID"}    (m/index-by :name (lib.metadata/fields metadata-provider (:id reviews)))]
+    {:model/products-and-reviews
+     {:lib/type      :metadata/card
+      :id            1000
+      :database-id   (:id (lib.metadata/database metadata-provider))
+      :name          "Mock model - Products and Reviews"
+      :dataset       true
+      :dataset-query
+      {:database (:id (lib.metadata/database metadata-provider))
+       :type     :query
+       :query    {:source-table (:id products)
+                  :joins        [{:fields       :all
+                                  :alias        "Reviews"
+                                  :source-table (:id reviews)
+                                  :condition    [:=
+                                                 [:field (:id pk) {:base-type :type/BigInteger}]
+                                                 [:field (:id fk)
+                                                  {:base-type :type/Integer
+                                                   :join-alias "Reviews"}]]}]}}}}))
+
 (def mock-cards
   "Map of mock MBQL query Card against the test tables. There are three versions of the Card for each table:
 
   * `:venues`, a Card WITH `:result-metadata`
   * `:venues/no-metadata`, a Card WITHOUT `:result-metadata`
-  * `:venues/native`, a Card with `:result-metadata` and a NATIVE query."
-  (make-mock-cards meta/metadata-provider (map (juxt identity (comp :id meta/table-metadata)) (meta/tables))))
+  * `:venues/native`, a Card with `:result-metadata` and a NATIVE query.
+
+  There are also some specialized mock cards used for corner cases:
+  * `:model/products-and-reviews`, a model joining products to reviews"
+  (merge (make-mock-cards meta/metadata-provider (map (juxt identity (comp :id meta/table-metadata)) (meta/tables)))
+         (make-mock-cards-special-cases meta/metadata-provider)))
 
 (defn metadata-provider-with-mock-card [card]
   (lib/composed-metadata-provider