[ParseSQL] Support queries with {[variable}}s and {{field filter}}s in them (#41193)

* Support parsing {{variable}}s and such in queries

[Fixes #39954]

[Fixes #39953]

.clj-kondo/config.edn

@@ -306,6 +306,7 @@
     metabase.models.timeline                                      timeline
     metabase.models.timeline-event                                timeline-event
     metabase.native-query-analyzer                                query-analyzer
+    metabase.native-query-analyzer.parameter-substitution         nqa.sub
     metabase.plugins.initialize                                   plugins.init
     metabase.public-settings                                      public-settings
     metabase.public-settings.premium-features                     premium-features

src/metabase/native_query_analyzer.clj

@@ -9,11 +9,10 @@
    [clojure.string :as str]
    [macaw.core :as mac]
    [metabase.config :as config]
+   [metabase.native-query-analyzer.parameter-substitution :as nqa.sub]
    [metabase.util :as u]
    [metabase.util.log :as log]
-   [toucan2.core :as t2])
-  (:import
-   [net.sf.jsqlparser JSQLParserException]))
+   [toucan2.core :as t2]))
 
 (def ^:dynamic *parse-queries-in-test?*
   "Normally, a native card's query is parsed on every create/update. For most tests, this is an unnecessary
@@ -80,19 +79,21 @@
       (seq table-wildcards)            (active-fields-from-tables table-wildcards))))
 
 (defn- field-ids-for-card
-  "Returns a `{:direct #{...} :indirect #{...}}` map with field IDs that (may) be referenced in the given cards's query. Errs on the side of optimism:
-  i.e., it may return fields that are *not* in the query, and is unlikely to fail to return fields that are in the
-  query.
+  "Returns a `{:direct #{...} :indirect #{...}}` map with field IDs that (may) be referenced in the given cards's
+  query. Errs on the side of optimism: i.e., it may return fields that are *not* in the query, and is unlikely to fail
+  to return fields that are in the query.
 
-  Direct references are columns that are named in the query; indirect ones are from wildcards. If a field could be both direct and indirect, it will *only* show up in the `:direct` set."
+  Direct references are columns that are named in the query; indirect ones are from wildcards. If a field could be
+  both direct and indirect, it will *only* show up in the `:direct` set."
   [card]
-  (let [{native-query :native
-         db-id        :database} (:dataset_query card)
-        parsed-query             (mac/query->components (mac/parsed-query (:query native-query)))
-        direct-ids               (direct-field-ids-for-query parsed-query db-id)
-        indirect-ids             (set/difference
-                                  (indirect-field-ids-for-query parsed-query db-id)
-                                  direct-ids)]
+  (let [query        (:dataset_query card)
+        db-id        (:database query)
+        sql-string   (:query (nqa.sub/replace-tags query))
+        parsed-query (mac/query->components (mac/parsed-query sql-string))
+        direct-ids   (direct-field-ids-for-query parsed-query db-id)
+        indirect-ids (set/difference
+                      (indirect-field-ids-for-query parsed-query db-id)
+                      direct-ids)]
     {:direct   direct-ids
      :indirect indirect-ids}))
 
@@ -122,5 +123,5 @@
         (t2/with-transaction [_conn]
           (t2/delete! :model/QueryField :card_id card-id)
           (t2/insert! :model/QueryField query-field-records)))
- (catch JSQLParserException e
-      (log/error e "Error parsing native query")))))
+      (catch Exception e
+        (log/error e "Error parsing native query")))))

src/metabase/native_query_analyzer/parameter_substitution.clj

+(ns metabase.native-query-analyzer.parameter-substitution
+  "Replace {{variable}}s and {{field filters}} in SQL queries with parse-able equivalents."
+  (:require
+   [metabase.query-processor.compile :as qp.compile]))
+
+(def default-values
+  "Map of default values for each type"
+  {;; Normal variables
+   :date                    "2024-01-09"
+   :number                  "1"
+   :text                    "sample text"
+   ;; Potentially deprecated
+   :boolean                 "true"
+   :id                      "1"
+   :category                "sample category"
+   :date/single             "2024-01-09"
+   ;; Card ref: doesn't matter
+   :card                    nil
+   ;; Field filters
+   :date/range              "2023-01-09~2024-01-09"
+   :date/month-year         "2024-01"
+   :date/quarter-year       "Q1-2024"
+   :date/relative           "past1years"
+   :date/all-options        "2024-01"
+   :number/!=               ["1"]
+   :number/<=               ["1"]
+   :number/=                ["1"]
+   :number/>=               ["1"]
+   :number/between          ["1" "2"]
+   :string/!=               ["sample text"]
+   :string/=                ["sample text"]
+   :string/contains         ["sample text"]
+   :string/does-not-contain ["sample text"]
+   :string/ends-with        ["sample text"]
+   :string/starts-with      ["sample text"]
+   ;; Potentially deprecated
+   :location/city           ["Moon Twp"]
+   :location/state          ["PA"]
+   :location/zip_code       ["15108"]
+   :location/country        ["USA"]})
+
+(defn- type->value
+  [t]
+  (default-values t))
+
+(defn- tag-default
+  [{:keys [default type widget-type] :as tag}]
+  (assoc tag :default
+         (or default
+             (if (= type :dimension)
+               (type->value widget-type)
+               (type->value type)))))
+
+(defn replace-tags
+  "Given a native dataset_query, return a `{:query \"<SQL string>\"}` where the SQL no longer has Metabase-specific
+  template tags."
+  [query]
+  (if-let [name->tag (seq (get-in query [:native :template-tags]))]
+    (qp.compile/compile (assoc-in query [:native :template-tags] (update-vals name->tag tag-default)))
+    (:native query)))

test/metabase/models/query_field_test.clj

@@ -44,7 +44,9 @@
   ([card-id]
    (trigger-parse! card-id "SELECT TAX, TOTAL FROM orders"))
   ([card-id query]
-   (t2/update! :model/Card card-id {:dataset_query (mt/native-query {:query query})})))
+   (if (string? query)
+     (t2/update! :model/Card card-id {:dataset_query (mt/native-query {:query query})})
+     (t2/update! :model/Card card-id {:dataset_query query}))))
 
 ;;;;
 ;;;; Actual tests
@@ -68,9 +70,22 @@
         (is (= #{tax-qf total-qf}
                (query-fields-for-card card-id))))
 
-      (testing "Removing columns from the query removes the Queryfields"
+      (testing "Removing columns from the query removes the QueryFields"
         (trigger-parse! card-id "SELECT tax, not_total FROM orders")
         (is (= #{tax-qf}
+               (query-fields-for-card card-id))))
+
+      (testing "Columns referenced via field filters are still found"
+        (trigger-parse! card-id
+                        (mt/native-query {:query "SELECT tax FROM orders WHERE {{adequate_total}}"
+                                          :template-tags {"adequate_total"
+                                                          {:type         :dimension
+                                                           :name         "adequate_total"
+                                                           :display-name "Total is big enough"
+                                                           :dimension    [:field (mt/id :orders :total)
+                                                                          {:base-type :type/Number}]
+                                                           :widget-type  :number/>=}}}))
+        (is (= #{tax-qf total-qf}
                (query-fields-for-card card-id)))))))
 
 (deftest bogus-queries-test

test/metabase/native_query_analyzer/parameter_substitution_test.clj

+(ns ^:parallel metabase.native-query-analyzer.parameter-substitution-test
+  (:require
+   [clojure.set :as set]
+   [clojure.test :refer :all]
+   [metabase.lib.native :as lib-native]
+   [metabase.lib.schema.parameter :as lib.schema.parameter]
+   [metabase.native-query-analyzer.parameter-substitution :as nqa.sub]
+   [metabase.test :as mt]
+   [toucan2.tools.with-temp :as t2.with-temp]))
+
+(def ^:private ->sql (comp :query nqa.sub/replace-tags))
+
+(def ^:private tag-definitions
+  (delay
+    { ;; Normal variables
+     "source"        {:type         :text
+                      :display-name "Source"
+                      :name         "source"}
+     "favorite_nbr"  {:type         :number
+                      :display-name "Fav"
+                      :name         "favorite_nbr"}
+     "created"       {:type         :date
+                      :display-name "Genesis"
+                      :name         "created"}
+     ;; Field Filters: Dates
+     "created_range" {:type         :dimension,
+                      :name         "created_range"
+                      :display-name "Created At :)"
+                      :dimension    [:field (mt/id :people :created_at) {:base-type :type/Date}]
+                      :widget-type  :date/range}
+     "created_my"    {:type         :dimension,
+                      :name         "created_my"
+                      :display-name "Created At :)"
+                      :dimension    [:field (mt/id :people :created_at) {:base-type :type/Date}]
+                      :widget-type  :date/month-year}
+     "created_qy"    {:type         :dimension,
+                      :name         "created_qy"
+                      :display-name "Created At :)"
+                      :dimension    [:field (mt/id :people :created_at) {:base-type :type/Date}]
+                      :widget-type  :date/quarter-year}
+     "created_rel"   {:type         :dimension,
+                      :name         "created_rel"
+                      :display-name "Created At :)"
+                      :dimension    [:field (mt/id :people :created_at) {:base-type :type/Date}]
+                      :widget-type  :date/relative}
+     "date_ao"       {:type         :dimension,
+                      :name         "date_ao"
+                      :display-name "Date: All Options"
+                      :dimension    [:field (mt/id :people :created_at) {:base-type :type/Date}]
+                      :widget-type  :date/all-options}
+     ;; Field Filters: Numbers
+     "num_not_eq"    {:type         :dimension,
+                      :name         "num_not_eq"
+                      :display-name "!="
+                      :dimension    [:field (mt/id :orders :total) {:base-type :type/Number}]
+                      :widget-type  :number/!=}
+     "num_lte"       {:type         :dimension,
+                      :name         "num_lte"
+                      :display-name "<="
+                      :dimension    [:field (mt/id :orders :total) {:base-type :type/Number}]
+                      :widget-type  :number/<=}
+     "num_eq"        {:type         :dimension,
+                      :name         "num_eq"
+                      :display-name "="
+                      :dimension    [:field (mt/id :orders :total) {:base-type :type/Number}]
+                      :widget-type  :number/=}
+     "num_gte"       {:type         :dimension,
+                      :name         "num_gte"
+                      :display-name ">="
+                      :dimension    [:field (mt/id :orders :total) {:base-type :type/Number}]
+                      :widget-type  :number/>=}
+     "num_between"   {:type         :dimension,
+                      :name         "num_between"
+                      :display-name "Betwixt"
+                      :dimension    [:field (mt/id :orders :total) {:base-type :type/Number}]
+                      :widget-type  :number/between}
+     ;; Field Filters: Strings
+     "str_not_eq"    {:type         :dimension,
+                      :name         "str_not_eq"
+                      :display-name "String !="
+                      :dimension    [:field (mt/id :people :name) {:base-type :type/Text}]
+                      :widget-type  :string/!=}
+     "str_eq"        {:type         :dimension,
+                      :name         "str_eq"
+                      :display-name "String ="
+                      :dimension    [:field (mt/id :people :name) {:base-type :type/Text}]
+                      :widget-type  :string/=}
+     "str_contains"  {:type         :dimension,
+                      :name         "str_contains"
+                      :display-name "String Contains"
+                      :dimension    [:field (mt/id :people :name) {:base-type :type/Text}]
+                      :widget-type  :string/contains}
+     "str_dnc"       {:type         :dimension,
+                      :name         "str_dnc"
+                      :display-name "String Does Not Contain"
+                      :dimension    [:field (mt/id :people :name) {:base-type :type/Text}]
+                      :widget-type  :string/does-not-contain}
+     "str_ends"      {:type         :dimension,
+                      :name         "str_ends"
+                      :display-name "String Ends With"
+                      :dimension    [:field (mt/id :people :name) {:base-type :type/Text}]
+                      :widget-type  :string/ends-with}
+     "str_starts"    {:type         :dimension,
+                      :name         "str_starts"
+                      :display-name "String Starts With"
+                      :dimension    [:field (mt/id :people :name) {:base-type :type/Text}]
+                      :widget-type  :string/starts-with}}))
+
+(defn- tags
+  [& ts]
+  (select-keys @tag-definitions ts))
+
+(deftest basic-variables-test
+  (testing "text variable"
+    (is (= "SELECT * FROM people WHERE source = ?"
+           (->sql (mt/native-query {:template-tags (tags "source")
+                                    :query "SELECT * FROM people WHERE source = {{source}}"})))))
+  (testing "number variable"
+    (is (= "SELECT * FROM people WHERE favorite_number = 1" ; Numbers are put right in, no `?`
+           (->sql (mt/native-query {:template-tags (tags "favorite_nbr")
+                                    :query "SELECT * FROM people WHERE favorite_number = {{favorite_nbr}}"})))))
+  (testing "date variable"
+    (is (= "SELECT * FROM people WHERE created_at = ?"
+           (->sql (mt/native-query {:template-tags (tags "created")
+                                    :query "SELECT * FROM people WHERE created_at = {{created}}"}))))))
+
+(deftest many-variables-test
+  (is (= "SELECT * FROM people WHERE source = ? AND favorite_number = 1 AND created_at = ?"
+         (->sql (mt/native-query {:template-tags (tags "source" "favorite_nbr" "created")
+                                  :query         (str "SELECT * FROM people WHERE source = {{source}} AND "
+                                                      "favorite_number = {{favorite_nbr}} AND created_at = "
+                                                      "{{created}}")})))))
+
+(deftest optional-test
+  (is (= "SELECT * FROM people WHERE  source = ? AND favorite_number = 1  AND created_at = ?"
+         (->sql (mt/native-query {:template-tags (tags "source" "favorite_nbr" "created")
+                                  :query         (str "SELECT * FROM people WHERE [[ source = {{source}} AND ]]"
+                                                      "favorite_number = {{favorite_nbr}} [[ AND created_at = "
+                                                      "{{created}} ]]")})))))
+
+(deftest field-filter-date-test
+  (doseq [date-filter ["created_range" "created_my" "created_qy" "created_rel" "date_ao"]]
+    (is (= "SELECT * FROM people WHERE CAST(\"PUBLIC\".\"PEOPLE\".\"CREATED_AT\" AS date) BETWEEN ? AND ?"
+           (->sql (mt/native-query {:template-tags (tags date-filter)
+                                    :query         (format "SELECT * FROM people WHERE {{%s}}" date-filter)}))))))
+
+(deftest field-filter-number-test
+  ;; :number/!=
+  (is (= "SELECT * FROM orders WHERE ((\"PUBLIC\".\"ORDERS\".\"TOTAL\" <> 1) OR (\"PUBLIC\".\"ORDERS\".\"TOTAL\" IS NULL))"
+         (->sql (mt/native-query {:template-tags (tags "num_not_eq")
+                                  :query         "SELECT * FROM orders WHERE {{num_not_eq}}"}))))
+  ;; :number/<=
+  (is (= "SELECT * FROM orders WHERE (\"PUBLIC\".\"ORDERS\".\"TOTAL\" <= 1)"
+         (->sql (mt/native-query {:template-tags (tags "num_lte")
+                                  :query         "SELECT * FROM orders WHERE {{num_lte}}"}))))
+  ;; :number/=
+  (is (= "SELECT * FROM orders WHERE (\"PUBLIC\".\"ORDERS\".\"TOTAL\" = 1)"
+         (->sql (mt/native-query {:template-tags (tags "num_eq")
+                                  :query         "SELECT * FROM orders WHERE {{num_eq}}"}))))
+  ;; :number/>=
+  (is (= "SELECT * FROM orders WHERE (\"PUBLIC\".\"ORDERS\".\"TOTAL\" >= 1)"
+         (->sql (mt/native-query {:template-tags (tags "num_gte")
+                                  :query         "SELECT * FROM orders WHERE {{num_gte}}"}))))
+  ;; :number/between
+  (is (= "SELECT * FROM orders WHERE \"PUBLIC\".\"ORDERS\".\"TOTAL\" BETWEEN 1 AND 2"
+         (->sql (mt/native-query {:template-tags (tags "num_between")
+                                  :query         "SELECT * FROM orders WHERE {{num_between}}"})))))
+
+(deftest field-filter-string-test
+  (is (= "SELECT * FROM people WHERE ((\"PUBLIC\".\"PEOPLE\".\"NAME\" <> ?) OR (\"PUBLIC\".\"PEOPLE\".\"NAME\" IS NULL))"
+         (->sql (mt/native-query {:template-tags (tags "str_not_eq")
+                                  :query         "SELECT * FROM people WHERE {{str_not_eq}}"}))))
+  (is (= "SELECT * FROM people WHERE (\"PUBLIC\".\"PEOPLE\".\"NAME\" = ?)"
+         (->sql (mt/native-query {:template-tags (tags "str_eq")
+                                  :query         "SELECT * FROM people WHERE {{str_eq}}"}))))
+  (is (= "SELECT * FROM people WHERE (\"PUBLIC\".\"PEOPLE\".\"NAME\" LIKE ?)"
+         (->sql (mt/native-query {:template-tags (tags "str_contains")
+                                  :query         "SELECT * FROM people WHERE {{str_contains}}"}))))
+  (is (= "SELECT * FROM people WHERE (NOT (\"PUBLIC\".\"PEOPLE\".\"NAME\" LIKE ?) OR (\"PUBLIC\".\"PEOPLE\".\"NAME\" IS NULL))"
+         (->sql (mt/native-query {:template-tags (tags "str_dnc")
+                                  :query         "SELECT * FROM people WHERE {{str_dnc}}"}))))
+  (is (= "SELECT * FROM people WHERE (\"PUBLIC\".\"PEOPLE\".\"NAME\" LIKE ?)"
+         (->sql (mt/native-query {:template-tags (tags "str_ends")
+                                  :query         "SELECT * FROM people WHERE {{str_ends}}"}))))
+  (is (= "SELECT * FROM people WHERE (\"PUBLIC\".\"PEOPLE\".\"NAME\" LIKE ?)"
+         (->sql (mt/native-query {:template-tags (tags "str_starts")
+                                  :query         "SELECT * FROM people WHERE {{str_starts}}"})))))
+
+(deftest card-ref-test
+  (t2.with-temp/with-temp
+    [:model/Card {card-id :id} {:type          :model
+                                :dataset_query (mt/native-query {:query "select * from checkins"})}]
+    (let [q  (format "SELECT * FROM {{#%s}} LIMIT 3" card-id)
+          tt (lib-native/extract-template-tags q)]
+      (is (= "SELECT * FROM (select * from checkins) LIMIT 3"
+             (->sql (mt/native-query {:template-tags tt
+                                      :query         q})))))))
+
+(deftest default-values
+  (let [blocklist #{;; there's a type check on :template-tags blocking these
+                    :date/single
+                    :id
+                    :category
+                    :boolean
+                    ;; no longer in use
+                    :location/city
+                    :location/state
+                    :location/zip_code
+                    :location/country}
+        expected  (set/difference (into #{} (keys lib.schema.parameter/types))
+                                  blocklist)
+        tag->type (fn [{:keys [type widget-type]}] (if (= type :dimension) widget-type type))]
+    (is (set/subset? expected (into #{} (keys nqa.sub/default-values)))
+        "Ensure that you have a default value for all types defined in lib.schema.parameter/types")
+    (is (set/subset? expected (into #{} (map tag->type (vals @tag-definitions))))
+        "Ensure that you have a test for each type defined in lib.schema.parameter/types")))