[chain filters] should not be using inactive fields for joins (#39635)

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

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)))))))))