[MLv2] Remove and replace clause for order-by only (#29657)

* [MLv2] Remove and replace clause for order-by only

* Remove replace filter test

* Fix tests

* Fix kondo

src/metabase/lib/core.cljc

@@ -114,7 +114,6 @@
    add-filter
    current-filter
    current-filters
-   replace-filter
    and
    or
    not
@@ -159,6 +158,8 @@
   [lib.query
    native-query
    query
+   remove-clause
+   replace-clause
    saved-question-query]
   [lib.stage
    append-stage

src/metabase/lib/filter.cljc

@@ -252,20 +252,3 @@
    (let [stage-number (clojure.core/or stage-number -1)
          new-filter (lib.common/->op-arg query stage-number boolean-expression)]
      (lib.util/update-query-stage query stage-number update :filter conjoin new-filter))))
-
-(mu/defn replace-filter :- :metabase.lib.schema/query
-  "Replaces the expression with `target-uuid` with `boolean-expression` the filter of `query`."
-  ([query :- :metabase.lib.schema/query
-    target-uuid :- :string
-    boolean-expression]
-   (metabase.lib.filter/replace-filter query nil target-uuid boolean-expression))
-
-  ([query :- :metabase.lib.schema/query
-    stage-number :- [:maybe :int]
-    target-uuid :- :string
-    boolean-expression]
-   (let [stage-number (clojure.core/or stage-number -1)
-         new-filter   (lib.common/->op-arg query stage-number boolean-expression)]
-     (lib.util/update-query-stage query stage-number
-                                  update :filter
-                                  lib.util/replace-clause target-uuid new-filter))))

src/metabase/lib/query.cljc

 (ns metabase.lib.query
+  (:refer-clojure :exclude [remove])
   (:require
+   [metabase.lib.common :as lib.common]
    [metabase.lib.convert :as lib.convert]
    [metabase.lib.dispatch :as lib.dispatch]
    [metabase.lib.metadata :as lib.metadata]
@@ -11,6 +13,45 @@
    [metabase.lib.util :as lib.util]
    [metabase.util.malli :as mu]))
 
+(mu/defn replace-clause :- :metabase.lib.schema/query
+  "Replaces the `target-clause` with `new-clase` in the `query` stage."
+  ([query :- :metabase.lib.schema/query
+    target-clause
+    new-clause]
+   (replace-clause query -1 target-clause new-clause))
+  ([query :- :metabase.lib.schema/query
+    stage-number :- :int
+    target-clause
+    new-clause]
+   (let [replacement (lib.common/->op-arg query stage-number new-clause)]
+     ;; Right now, this works for clauses that cannot have dependents.
+     ;; This will change and have different logic depending on `location`
+     ;; `location` should probably be "found" first before iterating.
+     (reduce
+       (fn [query location]
+         (lib.util/update-query-stage query stage-number
+                                      lib.util/replace-clause location target-clause replacement))
+       query
+       [:order-by]))))
+
+(mu/defn remove-clause :- :metabase.lib.schema/query
+  "Removes the `target-clause` in the filter of the `query`."
+  ([query :- :metabase.lib.schema/query
+    target-clause]
+   (remove-clause query -1 target-clause))
+  ([query :- :metabase.lib.schema/query
+    stage-number :- :int
+    target-clause]
+   ;; Right now, this works for clauses that cannot have dependents.
+   ;; This will change and have different logic depending on `location`
+   ;; `location` should probably be "found" first before iterating.
+   (reduce
+     (fn [query location]
+       (lib.util/update-query-stage query stage-number
+                                    lib.util/remove-clause location target-clause))
+     query
+     [:order-by])))
+
 (defmethod lib.normalize/normalize :mbql/query
   [query]
   (lib.normalize/normalize-map

src/metabase/lib/util.cljc

 (ns metabase.lib.util
   (:refer-clojure :exclude [format])
   (:require
+   #?@(:clj
+       ([potemkin :as p]))
+   #?@(:cljs
+       ([goog.string :as gstring]
+        [goog.string.format :as gstring.format]))
    [clojure.set :as set]
    [clojure.string :as str]
    [metabase.lib.options :as lib.options]
    [metabase.lib.schema :as lib.schema]
    [metabase.lib.schema.common :as lib.schema.common]
    [metabase.shared.util.i18n :as i18n]
-   [metabase.util.malli :as mu]
-   #?@(:clj
-       ([potemkin :as p]))
-   #?@(:cljs
-       ([goog.string :as gstring]
-        [goog.string.format :as gstring.format]))))
+   [metabase.util.malli :as mu]))
 
 ;; The formatting functionality is only loaded if you depend on goog.string.format.
 #?(:cljs (comment gstring.format/keep-me))
@@ -28,30 +28,45 @@
 (defn- clause? [clause]
   (and (vector? clause)
        (> (count clause) 1)
-       (keyword? (first clause))))
+       (keyword? (first clause))
+       (map? (second clause))
+       (contains? (second clause) :lib/uuid)))
 
 (defn- clause-uuid [clause]
   (when (clause? clause)
     (get-in clause [1 :lib/uuid])))
 
 (defn replace-clause
-  "Replace the clause with `target-uuid` in `clause` with `new-clause`.
-  If `clause` itself has :lib/uuid equal to `target-uuid`, `new-clause` is returned.
-  If `clause` contains no clause with `target-uuid` no replacement happens.
-  Since UUIDs are assumed to be unique, at most one replacement happens."
-  [clause target-uuid new-clause]
-  (if (clause? clause)
-    (if (= (clause-uuid clause) target-uuid)
-      new-clause
-      (reduce (fn [clause i]
-                (let [arg (clause i)
-                      arg' (replace-clause arg target-uuid new-clause)]
-                  (if (not= arg' arg)
-                    (reduced (assoc clause i arg'))
-                    clause)))
-              clause
-              (range 2 (count clause))))
-    clause))
+  "Replace the `target-clause` in `stage` `location` with `new-clause`.
+   If a clause has :lib/uuid equal to the `target-clause` it is swapped with `new-clause`.
+   If `location` contains no clause with `target-clause` no replacement happens."
+  [stage location target-clause new-clause]
+  {:pre [(clause? target-clause)]}
+  (update
+    stage
+    location
+    #(->> (for [clause %]
+            (if (= (clause-uuid clause) (clause-uuid target-clause))
+              new-clause
+              clause))
+          vec)))
+
+(defn remove-clause
+  "Replace the `target-clause` in `stage` `location`.
+   If a clause has :lib/uuid equal to the `target-clause` it is removed.
+   If `location` contains no clause with `target-clause` no removal happens.
+   If the the location is empty, dissoc it from stage."
+  [stage location target-clause]
+  {:pre [(clause? target-clause)]}
+  (let [target-uuid (clause-uuid target-clause)
+        target (get stage location)
+        result (->> target
+                    (remove (comp #{target-uuid} clause-uuid))
+                    vec
+                    not-empty)]
+    (if result
+      (assoc stage location result)
+      (dissoc stage location))))
 
 ;;; TODO -- all of this `->pipeline` stuff should probably be merged into [[metabase.lib.convert]] at some point in
 ;;; the near future.

test/metabase/lib/filter_test.cljc

@@ -221,40 +221,3 @@
       (is (=? extended-and-query third-add))
       (is (=? [first-result-filter second-result-filter third-result-filter]
               (lib/current-filters third-add))))))
-
-(deftest ^:parallel replace-filter-test
-  (let [q1                          (lib/query-for-table-name meta/metadata-provider "CATEGORIES")
-        venues-name-metadata        (lib.metadata/field q1 nil "VENUES" "NAME")
-        venues-category-id-metadata (lib.metadata/field q1 nil "VENUES" "CATEGORY_ID")
-        simple-filtered-query
-        (-> q1
-            (lib/filter (lib/between venues-category-id-metadata 42 100)))
-        between-uuid (-> (lib/current-filter simple-filtered-query)
-                         :options
-                         :lib/uuid)
-        result-query
-        (assoc-in simple-filtered-query
-                  [:stages 0 :filter]
-                  [:starts-with
-                   {:lib/uuid string?}
-                   [:field {:base-type :type/Text, :lib/uuid string?} (meta/id :venues :name)]
-                   "part"])]
-    (testing "sanity"
-      (is (string? between-uuid)))
-    (testing "replacing a simple filter"
-      (is (=? result-query
-              (lib/replace-filter simple-filtered-query
-                                  between-uuid
-                                  (lib/starts-with venues-name-metadata "part")))))
-    (testing "setting a simple filter thunk"
-      (is (=? result-query
-              (lib/replace-filter simple-filtered-query
-                                  between-uuid
-                                  {:operator "starts-with"
-                                   :args [(lib.field/field q1 venues-name-metadata) "part"]}))))
-    (testing "setting a simple filter expression"
-      (is (=? result-query
-              (lib/replace-filter simple-filtered-query
-                                  between-uuid
-                                  {:operator :starts-with
-                                   :args [(lib.field/field q1 venues-name-metadata) "part"]}))))))

test/metabase/lib/query_test.cljc

 (ns metabase.lib.query-test
   (:require
-   [clojure.test :refer [deftest is]]
+   #?@(:cljs ([metabase.test-runner.assert-exprs.approximately-equal]))
+   [clojure.test :refer [deftest is testing]]
    [metabase.lib.core :as lib]
    [metabase.lib.metadata.calculation :as lib.metadata.calculation]
-   [metabase.lib.test-metadata :as meta]
-   #?@(:cljs ([metabase.test-runner.assert-exprs.approximately-equal]))))
+   [metabase.lib.test-metadata :as meta]))
 
 #?(:cljs (comment metabase.test-runner.assert-exprs.approximately-equal/keep-me))
 
@@ -71,3 +71,36 @@
           (lib/query meta/metadata-provider {:database (meta/id)
                                              :type     :query
                                              :query    {:source-query {:source-query {:source-table (meta/id :venues)}}}}))))
+
+(deftest ^:parallel remove-clause-test
+  (let [query (-> (lib/query-for-table-name meta/metadata-provider "VENUES")
+                  (lib/filter (lib/= "myvenue" (lib/field (meta/id :venues :name))))
+                  (lib/order-by (lib/field (meta/id :venues :name)))
+                  (lib/order-by (lib/field (meta/id :venues :name))))
+        order-bys (lib/order-bys query)]
+    (testing "order-bys"
+      (is (= 2 (count order-bys)))
+      (is (= 1 (-> query
+                   (lib/remove-clause (first order-bys))
+                   (lib/order-bys)
+                   count)))
+      (is (= 0 (-> query
+                   (lib/remove-clause (first order-bys))
+                   (lib/remove-clause (second order-bys))
+                   (lib/order-bys)
+                   count))))))
+
+(deftest ^:parallel replace-clause-test
+  (let [query (-> (lib/query-for-table-name meta/metadata-provider "VENUES")
+                  (lib/filter (lib/= "myvenue" (lib/field (meta/id :venues :name))))
+                  (lib/order-by (lib/field (meta/id :venues :name)))
+                  (lib/order-by (lib/field (meta/id :venues :name))))
+        order-bys (lib/order-bys query)]
+    (testing "order-bys"
+      (is (= 2 (count order-bys)))
+      (let [replaced (-> query
+                         (lib/replace-clause (first order-bys) (lib/order-by-clause (lib/field (meta/id :venues :id)))))
+            replaced-order-bys (lib/order-bys replaced)]
+        (is (not= order-bys replaced-order-bys))
+        (is (= 2 (count replaced-order-bys)))
+        (is (= (second order-bys) (second replaced-order-bys)))))))

test/metabase/lib/util_test.cljc

 (ns metabase.lib.util-test
   (:require
    [clojure.test :refer [are deftest is testing]]
-   [clojure.test.check.generators :as gen]
-   [com.gfredericks.test.chuck.clojure-test :refer [checking]]
    [metabase.lib.test-metadata :as meta]
    [metabase.lib.util :as lib.util]
    #?@(:cljs ([metabase.test-runner.assert-exprs.approximately-equal]))))
@@ -218,28 +216,3 @@
     ["a" "b"]         "a and b"
     ["a" "b" "c"]     "a, b, and c"
     ["a" "b" "c" "d"] "a, b, c, and d"))
-
-(deftest ^:parallel replace-clause-test
-  (checking "can be called with anything"
-    [x gen/any-equatable]
-    (is (identical? x (lib.util/replace-clause x
-                                               (str (random-uuid))
-                                               (random-uuid)))))
-  (let [target-uuid (random-uuid)
-        replacement (random-uuid)]
-    (testing "full replacement"
-      (is (identical? replacement
-                      (lib.util/replace-clause [:= {:lib/uuid target-uuid}]
-                                               target-uuid
-                                               replacement))))
-    (let [clause [:= {:lib/uuid (str (random-uuid))}
-                  3
-                  [:field {:lib/uuid (str (random-uuid))}]
-                  [:+ 2 [:expression {:lib/uuid target-uuid} "a"] 7]]]
-      (testing "nested replacement"
-        (is (= (assoc-in clause [4 2] replacement)
-               (lib.util/replace-clause clause target-uuid replacement))))
-      (testing "only one occurrence is replaced"
-        (let [clause (assoc clause 5 [:< [:field {:lib/uuid target-uuid} 2]])]
-          (is (= (assoc-in clause [4 2] replacement)
-                 (lib.util/replace-clause clause target-uuid replacement))))))))