From 77b649dbcbe295b497b3265b50d97928fe9b6da6 Mon Sep 17 00:00:00 2001
From: Alexander Solovyov <alexander@solovyov.net>
Date: Fri, 8 Mar 2024 15:39:26 +0300
Subject: [PATCH] [chain filters] should not be using inactive fields for joins
(#39635)
---
src/metabase/models/params/chain_filter.clj | 24 ++++++----
.../models/params/chain_filter_test.clj | 44 +++++++++++++++++++
2 files changed, 60 insertions(+), 8 deletions(-)
diff --git a/src/metabase/models/params/chain_filter.clj b/src/metabase/models/params/chain_filter.clj
index d65028bdaaa..98bd4d495c6 100644
--- a/src/metabase/models/params/chain_filter.clj
+++ b/src/metabase/models/params/chain_filter.clj
@@ -195,12 +195,17 @@
[:pk-field.id :f2]
[:pk-field.table_id :t2]]
:from [[:metabase_field :fk-field]]
- :left-join [[:metabase_table :fk-table] [:= :fk-field.table_id :fk-table.id]
+ :left-join [[:metabase_table :fk-table] [:and [:= :fk-field.table_id :fk-table.id]
+ :fk-table.active]
[:metabase_database :database] [:= :fk-table.db_id :database.id]
- [:metabase_field :pk-field] [:= :fk-field.fk_target_field_id :pk-field.id]]
+ [:metabase_field :pk-field] [:and [:= :fk-field.fk_target_field_id :pk-field.id]
+ :pk-field.active]]
:where [:and
[:= :database.id database-id]
- [:not= :fk-field.fk_target_field_id nil]]})]
+ [:not= :fk-field.fk_target_field_id nil]
+ :fk-field.active]
+ :order-by [[:fk-field.id :desc]
+ [:pk-field.id :desc]]})]
(reduce
(partial merge-with merge)
{}
@@ -298,11 +303,14 @@
enable-reverse-joins?])}
find-joins*
:ttl/threshold find-joins-cache-duration-ms)]
- (fn
- ([database-id source-table-id other-table-id]
- (f database-id source-table-id other-table-id *enable-reverse-joins*))
- ([database-id source-table-id other-table-id enable-reverse-joins?]
- (f database-id source-table-id other-table-id enable-reverse-joins?)))))
+ ;; expose memoize metadata
+ (with-meta
+ (fn
+ ([database-id source-table-id other-table-id]
+ (f database-id source-table-id other-table-id *enable-reverse-joins*))
+ ([database-id source-table-id other-table-id enable-reverse-joins?]
+ (f database-id source-table-id other-table-id enable-reverse-joins?)))
+ (meta f))))
(def ^:private ^{:arglists '([source-table other-table-ids enable-reverse-joins?])} find-all-joins*
(memoize/ttl
diff --git a/test/metabase/models/params/chain_filter_test.clj b/test/metabase/models/params/chain_filter_test.clj
index be9c456e8a6..45f628ddf6b 100644
--- a/test/metabase/models/params/chain_filter_test.clj
+++ b/test/metabase/models/params/chain_filter_test.clj
@@ -670,3 +670,47 @@
(testing "`true` if the limit is less than the number of values the field has"
(is (= true
(:has_more_values (chain-filter venues.latitude {venues.price 4} :limit 1))))))))))
+
+;; TODO: make this test parallel, but clj-kondo complains about t2/update! being destructive and no amount of
+;; :clj-kondo/ignore convinces it.
+(deftest chain-filter-inactive-test
+ (testing "Inactive fields are not used to generate joins"
+ ;; custom dataset so that destructive operations (especially marking PK inactive) won't have any effect on other
+ ;; tests
+ (mt/with-temp-test-data [["users"
+ [{:field-name "name"
+ :base-type :type/Text}]
+ []]
+ ["messages"
+ [{:field-name "receiver_id"
+ :base-type :type/Integer
+ :fk :users}
+ {:field-name "sender_id"
+ :base-type :type/Integer
+ :fk :users}]
+ []]]
+ (mt/$ids nil
+ (mt/with-dynamic-redefs [chain-filter/database-fk-relationships @#'chain-filter/database-fk-relationships*
+ chain-filter/find-joins (fn
+ ([a b c]
+ (#'chain-filter/find-joins* a b c false))
+ ([a b c d]
+ (#'chain-filter/find-joins* a b c d)))]
+ (testing "receiver_id is active and should be used for the join"
+ (is (= [{:lhs {:table $$messages, :field %messages.receiver_id}
+ :rhs {:table $$users, :field %users.id}}]
+ (#'chain-filter/find-joins (mt/id) $$messages $$users))))
+
+ (try
+ (t2/update! :model/Field {:id %messages.receiver_id} {:active false})
+ (testing "check that it switches to sender once receiver is inactive"
+ (is (= [{:lhs {:table $$messages, :field %messages.sender_id}
+ :rhs {:table $$users, :field %users.id}}]
+ (#'chain-filter/find-joins (mt/id) $$messages $$users))))
+ (finally
+ (t2/update! :model/Field {:id %messages.receiver_id} {:active true})))
+
+ ;; mark field
+ (t2/update! :model/Field {:id %users.id} {:active false})
+ (testing "there are no connections when PK is inactive"
+ (is (nil? (#'chain-filter/find-joins (mt/id) $$messages $$users)))))))))
--
GitLab