[ParseSQL] Find-and-replace with optional tags (#43013)

* Support find-and-replace with optional tags [Fixes #42583]
metabase · May 23, 2024 · 4f7f558 · 4f7f558
1 parent 925454f
commit 4f7f558
Show file tree

Hide file tree

Showing 2 changed files with 43 additions and 9 deletions.
diff --git a/src/metabase/native_query_analyzer/replacement.clj b/src/metabase/native_query_analyzer/replacement.clj
@@ -36,6 +36,16 @@
                                     sentinel snippet-contents sentinel))]
     (first-unique raw-query #(delimited-snippet (gensym "mb_") content))))
 
+(defn- gen-option-sentinels
+  [raw-query]
+  (let [unique-sentinels?       (fn [[open close]] (not (or (str/includes? raw-query open)
+                                                            (str/includes? raw-query close))))
+        gen-sentinel-candidates (fn [] (let [postfix  (gensym "mb_")
+                                             template "/* opt_%s_%s */"]
+                                         [(format template "open"  postfix)
+                                          (format template "close" postfix)]))]
+    (first (filter unique-sentinels? (repeatedly gen-sentinel-candidates)))))
+
 (defn- braceify
   [s]
   (format "{{%s}}" s))
@@ -75,10 +85,24 @@
                    (str query-so-far sub)
                    (add-tag substitutions sub token)))
 
+          (params/Optional? token)
+          (let [[open-sentinel close-sentinel] (gen-option-sentinels raw-query)
+                {inner-query :query
+                 inner-subs  :substitutions} (parse-tree->clean-query raw-query (:args token) param->value)]
+            (recur rest
+                   (str query-so-far
+                        open-sentinel
+                        inner-query
+                        close-sentinel)
+                   (merge inner-subs
+                          substitutions
+                          {open-sentinel  "[["
+                           close-sentinel "]]"})))
+
           ;; Plain variable
-          (and (params/Param? token)
-               (not card-ref?)
-               (not snippet?))
+          ;; Note that the order of the clauses matters: `card-ref?` or `snippet?` could be true when is a `Param?`,
+          ;; so we need to handle those cases specially first and leave this as the the token fall-through
+          (params/Param? token)
           (let [sub (gen-variable-sentinel raw-query)]
             (recur rest
                    (str query-so-far sub)
@@ -91,7 +115,7 @@
                    (add-tag substitutions sub token)))
 
           :else
-          ;; will be addressed by #42582, etc.
+          ;; "this should never happen" but if it does we certainly want to know about it.
           (throw (ex-info "Unsupported token in native query" {:token token})))))))
 
 (defn- replace-all

diff --git a/test/metabase/native_query_analyzer/replacement_test.clj b/test/metabase/native_query_analyzer/replacement_test.clj
@@ -1,4 +1,4 @@
-(ns metabase.native-query-analyzer.replacement-test
+(ns ^:parallel metabase.native-query-analyzer.replacement-test
   (:require
    [clojure.test :refer :all]
    [metabase.lib.native :as lib-native]
@@ -13,7 +13,7 @@
     (mt/native-query {:query         query
                       :template-tags (lib-native/extract-template-tags query)})))
 
-(deftest ^:parallel replace-names-simple-test
+(deftest replace-names-simple-test
   (testing "columns can be renamed"
     (is (= "select cost from orders"
            (replace-names (q "select amount from orders") {:columns {"amount" "cost"}})) ))
@@ -26,7 +26,7 @@
                                                                           "fee"    "tax"}
                                                                 :tables {"orders" "purchases"}})) )))
 
-(deftest ^:parallel replace-names-whitespace-test
+(deftest replace-names-whitespace-test
   (testing "comments, whitespace, etc. are preserved"
     (is (= (str "select cost, tax -- from orders\n"
                 "from purchases")
@@ -36,7 +36,7 @@
                                      "fee"    "tax"}
                            :tables {"orders" "purchases"}})) )))
 
-(deftest ^:parallel variables-test
+(deftest variables-test
   (testing "with variables (template tags)"
     (is (= (str "\n\nSELECT *\nFROM folk\nWHERE\n  referral = {{source}}\n  OR \n  id = {{id}}\n  OR \n  birthday = "
                 "{{birthday}}\n  OR\n  zip = {{zipcode}}")
@@ -46,7 +46,7 @@
                                      "birth_date" "birthday"}
                            :tables  {"people" "folk"}})))))
 
-(deftest ^:parallel field-filter-test
+(deftest field-filter-test
   (testing "with variables *and* field filters"
     (is (= (str "SELECT *\nFROM folk\nWHERE\n  referral = {{source}}\n   OR \n  id = {{id}} \n  OR \n"
                 "  {{birthday}}\n  OR\n  {{zipcode}}\n  OR\n  {{city}} ")
@@ -78,3 +78,13 @@
                              snippet-id)
                             {:columns {"total" "amount"}
                              :tables  {"orders" "purchases"}}))))))
+
+(deftest optional-test
+  (testing "With optional tags"
+    (is (= (str "SELECT cost FROM purchases WHERE id IS NOT NULL [[ AND pretax > {{subtotal}} ]] "
+                "[[ AND {{amazing_filter}} ]]")
+           (replace-names (q "SELECT total FROM orders WHERE id IS NOT NULL [[ AND subtotal > {{subtotal}} ]] "
+                             "[[ AND {{amazing_filter}} ]]")
+                          {:columns {"total"    "cost"
+                                     "subtotal" "pretax"}
+                           :tables {"orders" "purchases"}})))))