Skip to content

Commit

Permalink
[MLv2] fk-details drill should preserve := filters for other PKs
Browse files Browse the repository at this point in the history 
That is, **if** there is a table with multiple PKs, and another table with
multiple FKs for those PKs, **then** any `:=` filters for other PKs on
that table should be preserved.

This allows using 1 or more quick-filter drills to add `FOO_ID = 7` filters
for some of the foreign table's PKs, followed by a fk-details drill that
jumps to the filtered view of that table.

Progress towards #36253.
  • Loading branch information
@bshepherdson
bshepherdson committed Feb 2, 2024
1 parent 21712d6 commit 28e6e49
Show file tree
Hide file tree
Showing 2 changed files with 107 additions and 5 deletions.
28 changes: 23 additions & 5 deletions src/metabase/lib/drill_thru/fk_details.cljc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,26 @@
[query stage-number {:keys [column object-id]} & _]
;; generate a NEW query against the FK target table and column, e.g. if the original query was
;; ORDERS/ORDERS.USER_ID, the new query should by PEOPLE/PEOPLE.ID.
(let [fk-column-id (:fk-target-field-id column)
fk-column (some->> fk-column-id (lib.metadata/field query))
fk-column-table (some->> (:table-id fk-column) (lib.metadata/table query))]
(-> (lib.query/query query fk-column-table)
(lib.filter/filter stage-number (lib.filter/= fk-column object-id)))))
;; If there are filters on the original query which are:
;; - := filters, and
;; - Not for this same column, but
;; - Relevant to OTHER FKs which point to PKs on the target table;
;; then preserve those filters.
(let [fk-column-id (:fk-target-field-id column)
fk-column (some->> fk-column-id (lib.metadata/field query))
fk-column-table (some->> (:table-id fk-column) (lib.metadata/table query))
other-fk-filters (for [filter-clause (lib.filter/filters query stage-number)
:let [parts (lib.filter/filter-parts query stage-number filter-clause)]
:when (= (:short (:operator parts)) :=)
:let [other-fk-target (some->> parts
:column
:fk-target-field-id
(lib.metadata/field query))]
:when (and other-fk-target
(= (:table-id other-fk-target) (:id fk-column-table)) ; FK to this table
(not= (:id other-fk-target) fk-column-id))] ; But not this column
(lib.filter/= other-fk-target (first (:args parts))))]
(reduce #(lib.filter/filter %1 stage-number %2)
(lib.query/query query fk-column-table)
(concat other-fk-filters
[(lib.filter/= fk-column object-id)]))))
84 changes: 84 additions & 0 deletions test/metabase/lib/drill_thru/fk_details_test.cljc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
[metabase.lib.core :as lib]
[metabase.lib.drill-thru.test-util :as lib.drill-thru.tu]
[metabase.lib.drill-thru.test-util.canned :as canned]
[metabase.lib.metadata :as lib.metadata]
[metabase.lib.test-metadata :as meta]
[metabase.lib.test-util.metadata-providers.merged-mock :as merged-mock]
#?@(:cljs ([metabase.test-runner.assert-exprs.approximately-equal]))))

#?(:cljs (comment metabase.test-runner.assert-exprs.approximately-equal/keep-me))
Expand Down Expand Up @@ -140,3 +142,85 @@
:value nil}]}]
(is (not (m/find-first #(= (:type %) :drill-thru/fk-details)
(lib/available-drill-thrus query -1 context)))))))

(deftest ^:parallel preserve-filters-for-other-fks-forming-multi-column-pk-test
(testing "with multiple FKs forming a multi-column PK on another table"
(let [provider (merged-mock/merged-mock-metadata-provider
meta/metadata-provider
{:fields [;; Make Checkins.VENUE_ID + Checkins.USER_ID into a two-part primary key.
;; Turn Checkins.ID into a basic numeric field.
{:id (meta/id :checkins :id)
:semantic-type :type/Quantity}
{:id (meta/id :checkins :venue-id)
:semantic-type :type/PK
:fk-target-field-id nil}
{:id (meta/id :checkins :user-id)
:semantic-type :type/PK
:fk-target-field-id nil}

;; Then turn Orders.USER_ID and Orders.PRODUCT_ID into FKs pointing at Checkins.
{:id (meta/id :orders :user-id)
:semantic-type :type/FK
:fk-target-field-id (meta/id :checkins :user-id)}
{:id (meta/id :orders :product-id)
:semantic-type :type/FK
:fk-target-field-id (meta/id :checkins :venue-id)}]})
;; Then we can treat them as a two-part FK aimed at a two-part PK.
query (-> (lib/query provider (meta/table-metadata :orders))
;; This filter should get removed when filtering by the FK.
(lib/filter (lib/= (meta/field-metadata :orders :quantity) 1)))
venue-id (get-in lib.drill-thru.tu/test-queries ["ORDERS" :unaggregated :row "PRODUCT_ID"])
user-id (get-in lib.drill-thru.tu/test-queries ["ORDERS" :unaggregated :row "USER_ID"])
#_#_filtered-venue (-> basic
(lib/filter (lib/= (lib.metadata/field basic (meta/id :orders :product-id))
(get-in lib.drill-thru.tu/test-queries
["ORDERS" :unaggregated :row "PRODUCT_ID"]))))
]
(testing "work as normal with no related filter"
(lib.drill-thru.tu/test-drill-application
{:column-name "PRODUCT_ID"
:click-type :cell
:query-type :unaggregated
:custom-query query
:drill-type :drill-thru/fk-details
:expected {:type :drill-thru/fk-details
:column (m/find-first #(= (:name %) "PRODUCT_ID") (lib/returned-columns query))
:object-id venue-id
;; TODO: This field actually refers to the source table, not the target one. Is that right?
:many-pks? false}
:expected-query {:stages [{:filters [[:= {} [:field {} (meta/id :checkins :venue-id)] venue-id]]}]}}))

(testing "preserve any existing filter for another PK on the same table"
(testing "existing USER_ID, new \"VENUE_ID\" (really PRODUCT_ID)"
(let [filtered-user (lib/filter query (lib/= (lib.metadata/field query (meta/id :orders :user-id)) user-id))]
(lib.drill-thru.tu/test-drill-application
{:column-name "PRODUCT_ID"
:click-type :cell
:query-type :unaggregated
:custom-query filtered-user
:drill-type :drill-thru/fk-details
:expected {:type :drill-thru/fk-details
:column (m/find-first #(= (:name %) "PRODUCT_ID")
(lib/returned-columns filtered-user))
:object-id venue-id
:many-pks? false}
:expected-query
{:stages [{:filters [[:= {} [:field {} (meta/id :checkins :user-id)] user-id]
[:= {} [:field {} (meta/id :checkins :venue-id)] venue-id]]}]}})))
(testing "existing \"VENUE_ID\" (really PRODUCT_ID), new USER_ID"
(let [filtered-venue (lib/filter query (lib/= (lib.metadata/field query (meta/id :orders :product-id))
venue-id))]
(lib.drill-thru.tu/test-drill-application
{:column-name "USER_ID"
:click-type :cell
:query-type :unaggregated
:custom-query filtered-venue
:drill-type :drill-thru/fk-details
:expected {:type :drill-thru/fk-details
:column (m/find-first #(= (:name %) "USER_ID")
(lib/returned-columns filtered-venue))
:object-id user-id
:many-pks? false}
:expected-query
{:stages [{:filters [[:= {} [:field {} (meta/id :checkins :venue-id)] venue-id]
[:= {} [:field {} (meta/id :checkins :user-id)] user-id]]}]}})))))))

0 comments on commit 28e6e49

Please sign in to comment.