From 34954c8d6478cb4523c46fd7e48dca05c205083d Mon Sep 17 00:00:00 2001
From: Tim Macdonald <tim@metabase.com>
Date: Wed, 10 Apr 2024 14:46:29 +0100
Subject: [PATCH] [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 | 1 +
src/metabase/native_query_analyzer.clj | 33 +--
.../parameter_substitution.clj | 60 +++++
test/metabase/models/query_field_test.clj | 19 +-
.../parameter_substitution_test.clj | 216 ++++++++++++++++++
5 files changed, 311 insertions(+), 18 deletions(-)
create mode 100644 src/metabase/native_query_analyzer/parameter_substitution.clj
create mode 100644 test/metabase/native_query_analyzer/parameter_substitution_test.clj
diff --git a/.clj-kondo/config.edn b/.clj-kondo/config.edn
index 4b5317e5a57..df54cb85fdb 100644
--- a/.clj-kondo/config.edn
+++ b/.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
diff --git a/src/metabase/native_query_analyzer.clj b/src/metabase/native_query_analyzer.clj
index 7d5b2380eb8..94bd457965a 100644
--- a/src/metabase/native_query_analyzer.clj
+++ b/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")))))
diff --git a/src/metabase/native_query_analyzer/parameter_substitution.clj b/src/metabase/native_query_analyzer/parameter_substitution.clj
new file mode 100644
index 00000000000..9fba4cb9a6f
--- /dev/null
+++ b/src/metabase/native_query_analyzer/parameter_substitution.clj
@@ -0,0 +1,60 @@
+(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)))
diff --git a/test/metabase/models/query_field_test.clj b/test/metabase/models/query_field_test.clj
index b44f7348ab1..af97442b554 100644
--- a/test/metabase/models/query_field_test.clj
+++ b/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
diff --git a/test/metabase/native_query_analyzer/parameter_substitution_test.clj b/test/metabase/native_query_analyzer/parameter_substitution_test.clj
new file mode 100644
index 00000000000..3bc1d365fe8
--- /dev/null
+++ b/test/metabase/native_query_analyzer/parameter_substitution_test.clj
@@ -0,0 +1,216 @@
+(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")))
--
GitLab