[MLv2] Removing a final JOIN condition removes the whole join (#37229)

This still throws an exception on trying to directly remove the final
JOIN condition. But if the condition is being removed because a field it
depends on is being removed (eg. a custom column, a breakout, a
previous stage aggregation) then the entire JOIN is also removed, in the
usual cascading fashion.

Fixes #36690.

dev/src/dev/fe_helpers.cljs

 (ns dev.fe-helpers)
 
-(defn redux-state []
+(defn redux-state
+  "Returns the root Redux state, the JS object holding the complete state of the app.
+
+  This is hacky - it reaches deep into the internals of Redux, and may break in the future. That seems acceptable for a
+  dev time helper."
+  []
   (let [root  (js/document.querySelector "#root")
         store (.. root -_reactRootContainer -_internalRoot -current -child -memoizedProps -store)]
     (.getState store)))
 
-(defn current-card []
+(defn current-card
+  "Retrieves the current query's card from the Redux state.
+
+  Undefined behavior if there is not currently a single question loaded in the UI."
+  []
   (.. (redux-state) -qb -card))
 
-(defn current-legacy-query-js []
+(defn current-legacy-query-js
+  "Gets the legacy query for the currently loaded question."
+  []
   (.-dataset_query (current-card)))
 
-(defn current-query []
+(defn current-query
+  "Gets the MLv2 query for the currently loaded question.
+
+  Hack: This relies on a dev-mode-only global property that's set whenever a Question object is converted to MLv2."
+  []
   (.-__MLv2_query js/window))

src/metabase/lib/remove_replace.cljc

@@ -35,6 +35,7 @@
 
 (declare remove-local-references)
 (declare remove-stage-references)
+(declare remove-join)
 (declare normalize-fields-clauses)
 
 (defn- find-matching-order-by-index
@@ -121,12 +122,24 @@
                                        (_ :guard #(or (empty? target-opts)
                                                       (set/subset? (set target-opts) (set %))))
                                        target-ref-id] [location clause]))))))
-                   (stage-paths query stage-number))]
-    (reduce
-     (fn [query [location target-clause]]
-       (remove-replace-location query stage-number unmodified-query-for-stage location target-clause #(lib.util/remove-clause %1 %2 %3 stage-number)))
-     query
-     to-remove)))
+                   (stage-paths query stage-number))
+        dead-joins (volatile! (transient []))]
+    (as-> query q
+      (reduce
+        (fn [query [location target-clause]]
+          (remove-replace-location
+            query stage-number unmodified-query-for-stage location target-clause
+            #(try (lib.util/remove-clause %1 %2 %3 stage-number)
+                  (catch #?(:clj Exception :cljs js/Error) e
+                    (let [{:keys [error join]} (ex-data e)]
+                      (if (= error :metabase.lib.util/cannot-remove-final-join-condition)
+                        ;; Return the stage unchanged, but keep track of the dead joins.
+                        (do (vswap! dead-joins conj! join)
+                            %1)
+                        (throw e)))))))
+        q
+        to-remove)
+      (reduce #(remove-join %1 stage-number %2) q (persistent! @dead-joins)))))
 
 (defn- remove-stage-references
   [query previous-stage-number unmodified-query-for-stage target-uuid]
@@ -185,8 +198,6 @@
             normalize-fields-clauses)
         query))))
 
-(declare remove-join)
-
 (mu/defn remove-clause :- :metabase.lib.schema/query
   "Removes the `target-clause` from the stage specified by `stage-number` of `query`.
   If `stage-number` is not specified, the last stage is used."

test/metabase/lib/remove_replace_test.cljc

@@ -44,24 +44,50 @@
                     (lib/filters)))))))
 
 (deftest ^:parallel remove-clause-join-conditions-test
-  (let [query (-> lib.tu/venues-query
-                  (lib/join (lib/join-clause (lib/query meta/metadata-provider (meta/table-metadata :categories))
-                                             [(lib/= (meta/field-metadata :venues :price) 4)
-                                              (lib/= (meta/field-metadata :venues :name) "x")])))
-        conditions (lib/join-conditions (first (lib/joins query)))]
-    (is (= 2 (count conditions)))
-    (is (= [(second conditions)]
-           (-> query
-               (lib/remove-clause (first conditions))
-               lib/joins
-               first
-               lib/join-conditions)))
-    (is (thrown-with-msg?
-          #?(:clj Exception :cljs js/Error)
-          #"Cannot remove the final join condition"
-          (-> query
-              (lib/remove-clause (first conditions))
-              (lib/remove-clause (second conditions)))))))
+  (testing "directly removing the final join condition throws an exception"
+    (let [query (-> lib.tu/venues-query
+                    (lib/join (lib/join-clause (lib/query meta/metadata-provider (meta/table-metadata :categories))
+                                               [(lib/= (meta/field-metadata :venues :price) 4)
+                                                (lib/= (meta/field-metadata :venues :name) "x")])))
+          conditions (lib/join-conditions (first (lib/joins query)))]
+      (is (= 2 (count conditions)))
+      (is (= [(second conditions)]
+             (-> query
+                 (lib/remove-clause (first conditions))
+                 lib/joins
+                 first
+                 lib/join-conditions)))
+      (is (thrown-with-msg?
+            #?(:clj Exception :cljs js/Error)
+            #"Cannot remove the final join condition"
+            (-> query
+                (lib/remove-clause (first conditions))
+                (lib/remove-clause (second conditions)))))))
+
+  (testing "a cascading delete that removes the final join condition should remove the whole join (#36690)"
+    (let [base   (-> (lib/query meta/metadata-provider (meta/table-metadata :orders))
+                     (lib/join (lib/join-clause (meta/table-metadata :products)
+                                                [(lib/= (meta/field-metadata :orders :product-id)
+                                                        (meta/field-metadata :products :id))]))
+                     (lib/aggregate (lib/count))
+                     (lib/breakout (meta/field-metadata :products :id))
+                     lib/append-stage)
+          cols   (lib/returned-columns base)
+          id     (first cols)
+          query  (lib/join base (lib/join-clause (meta/table-metadata :reviews)
+                                                 [(lib/= id (meta/field-metadata :reviews :product-id))]))]
+      (is (=? {:stages [{:joins       [{:conditions [[:= {} vector? vector?]]}]
+                         :aggregation [[:count {}]]
+                         :breakout    [[:field {} int?]]}
+                        {:joins       [{:conditions [[:= {} vector? vector?]]}]
+                         :aggregation (symbol "nil #_\"key is not present.\"")
+                         :breakout    (symbol "nil #_\"key is not present.\"")}]}
+              query))
+      (is (=? {:stages [{:joins       [{:conditions [[:= {} vector? vector?]]}]
+                         :aggregation [[:count {}]]
+                         :breakout    (symbol "nil #_\"key is not present.\"")}
+                        {:joins (symbol "nil #_\"key is not present.\"")}]}
+              (lib/remove-clause query 0 (first (lib/breakouts query 0))))))))
 
 (deftest ^:parallel remove-clause-breakout-test
   (let [query (-> lib.tu/venues-query