From 71a58264b76987dd4428efe4c6446109e7091fee Mon Sep 17 00:00:00 2001
From: Cam Saul <1455846+camsaul@users.noreply.github.com>
Date: Tue, 17 Dec 2019 22:53:27 +0000
Subject: [PATCH] Make sure query templates containing newlines are parsed
properly (#11532)
Make sure query templates containing newlines are parsed properly [ci drivers]
---
.../driver/common/parameters/parse.clj | 2 +-
.../driver/common/parameters/parse_test.clj | 4 +
.../driver/sql/parameters/substitute_test.clj | 33 ++++++--
.../query_processor_test/parameters_test.clj | 82 +++++++++++--------
4 files changed, 76 insertions(+), 45 deletions(-)
diff --git a/src/metabase/driver/common/parameters/parse.clj b/src/metabase/driver/common/parameters/parse.clj
index df3f8e24f06..efc16d4f72b 100644
--- a/src/metabase/driver/common/parameters/parse.clj
+++ b/src/metabase/driver/common/parameters/parse.clj
@@ -61,7 +61,7 @@
["]]" :optional-end]
;; param-begin should only match the last two opening brackets in a sequence of > 2, e.g.
;; [{$match: {{{x}}, field: 1}}] should parse to ["[$match: {" (param "x") ", field: 1}}]"]
- [#"(.*?)(\{\{(?!\{))(.*)" :param-begin]
+ [#"(?s)(.*?)(\{\{(?!\{))(.*)" :param-begin]
["}}" :param-end]]))
(defn- param [& [k & more]]
diff --git a/test/metabase/driver/common/parameters/parse_test.clj b/test/metabase/driver/common/parameters/parse_test.clj
index ab3bfb5f4b8..27af2e33566 100644
--- a/test/metabase/driver/common/parameters/parse_test.clj
+++ b/test/metabase/driver/common/parameters/parse_test.clj
@@ -70,6 +70,10 @@
(let [query "SELECT array_dims(1 || '[0:1]={2,3}'::int[])"]
{query [query]})
+ "Queries with newlines (#11526)"
+ {"SELECT count(*)\nFROM products\nWHERE category = {{category}}"
+ ["SELECT count(*)\nFROM products\nWHERE category = " (param "category")]}
+
"JSON queries that contain non-param fragments like '}}'"
{"{x: {y: \"{{param}}\"}}" ["{x: {y: \"" (param "param") "\"}}"]
"{$match: {{{date}}, field: 1}}}" ["{$match: {" (param "date") ", field: 1}}}"]}}]
diff --git a/test/metabase/driver/sql/parameters/substitute_test.clj b/test/metabase/driver/sql/parameters/substitute_test.clj
index 45a0e0e4c04..c0b4df0615f 100644
--- a/test/metabase/driver/sql/parameters/substitute_test.clj
+++ b/test/metabase/driver/sql/parameters/substitute_test.clj
@@ -698,6 +698,21 @@
:default "2017-11-14"
:widget-type :date/all-options}}}}))
+(deftest newlines-test
+ (testing "Make sure queries with newlines are parsed correctly (#11526)"
+ (is (= [[1]]
+ (mt/rows
+ (qp/process-query
+ {:database (mt/id)
+ :type "native"
+ :native {:query "SELECT count(*)\nFROM venues\n WHERE name = {{name}}"
+ :template-tags {:name {:name "name"
+ :display_name "Name"
+ :type "text"
+ :required true
+ :default "Fred 62"}}}
+ :parameters []}))))))
+
;;; ------------------------------- Multiple Value Support (comma-separated or array) --------------------------------
@@ -705,15 +720,15 @@
(testing "Make sure using commas in numeric params treats them as separate IDs (#5457)"
(is (= "SELECT * FROM USERS where id IN (1, 2, 3)"
(-> (qp/process-query
- {:database (mt/id)
- :type "native"
- :native {:query "SELECT * FROM USERS [[where id IN ({{ids_list}})]]"
- :template-tags {"ids_list" {:name "ids_list"
- :display-name "Ids list"
- :type :number}}}
- :parameters [{:type "category"
- :target [:variable [:template-tag "ids_list"]]
- :value "1,2,3"}]})
+ {:database (mt/id)
+ :type "native"
+ :native {:query "SELECT * FROM USERS [[where id IN ({{ids_list}})]]"
+ :template-tags {"ids_list" {:name "ids_list"
+ :display-name "Ids list"
+ :type :number}}}
+ :parameters [{:type "category"
+ :target [:variable [:template-tag "ids_list"]]
+ :value "1,2,3"}]})
:data :native_form :query))))
(testing "make sure you can now also pass multiple values in by passing an array of values"
(is (= {:query "SELECT * FROM CATEGORIES where name IN (?, ?, ?)"
diff --git a/test/metabase/query_processor_test/parameters_test.clj b/test/metabase/query_processor_test/parameters_test.clj
index c8e6c8c842e..6c3343f92fc 100644
--- a/test/metabase/query_processor_test/parameters_test.clj
+++ b/test/metabase/query_processor_test/parameters_test.clj
@@ -78,44 +78,56 @@
{:$project {"_id" false, "count" true}}])
:collection (name table)}))
-(defn- count-query [table field->type+value]
+(defn- count-query [table field->type+value {:keys [defaults?]}]
{:database (mt/id)
:type :native
:native (assoc (native-count-query driver/*driver* table field->type+value)
- :template-tags (into {} (for [[field [param-type]] field->type+value]
- [field {:name (name field)
- :display-name (name field)
- :type (or (namespace param-type)
- (name param-type))}])))
- :parameters (for [[field [param-type v]] field->type+value]
- {:type param-type
- :target [:variable [:template-tag (name field)]]
- :value v})})
-
-(defn- count= [expected table field->type+value]
- (let [query (count-query table field->type+value)]
- (testing (str "\nquery =\n" (u/pprint-to-str query))
- (is (= expected
- (ffirst
- (mt/formatted-rows [int]
- (qp/process-query query))))
- (format "count with of %s with %s should be %d"
- (name table)
- (str/join " and " (for [[field [_ v]] field->type+value]
- (format "%s = %s" (name field) v)))
- expected)))))
+ :template-tags (into {} (for [[field [param-type v]] field->type+value]
+ [field (cond-> {:name (name field)
+ :display-name (name field)
+ :type (or (namespace param-type)
+ (name param-type))}
+ defaults? (assoc :default v))])))
+ :parameters (when-not defaults?
+ (for [[field [param-type v]] field->type+value]
+ {:type param-type
+ :target [:variable [:template-tag (name field)]]
+ :value v}))})
(deftest param-test
(mt/test-drivers (mt/normal-drivers-with-feature :native-parameters)
- (testing "text params"
- (count= 1
- :venues {:name [:text "In-N-Out Burger"]}))
- (testing "number params"
- (count= 22
- :venues {:price [:number "1"]}))
- ;; FIXME — This is not currently working on SQLite, probably because SQLite's implementation of temporal types is
- ;; wacko.
- (when-not (= driver/*driver* :sqlite)
- (testing "date params"
- (count= 1
- :users {:last_login [:date/single "2014-08-02T09:30Z"]})))))
+ (doseq [[message {:keys [expected-count table param-name param-type value exclude-drivers]}]
+ {"text params" {:expected-count 1
+ :table :venues
+ :param-name :name
+ :param-type :text
+ :value "In-N-Out Burger"}
+ "number params" {:expected-count 22
+ :table :venues
+ :param-name :price
+ :param-type :number
+ :value "1"}
+ "date params" {:expected-count 1
+ ;; FIXME — This is not currently working on SQLite, probably because SQLite's
+ ;; implementation of temporal types is wacko.
+ :exclude-drivers #{:sqlite}
+ :table :users
+ :param-name :last_login
+ :param-type :date/single
+ :value "2014-08-02T09:30Z"}}
+ :when (not (contains? exclude-drivers driver/*driver*))]
+ (testing (str "\n" message)
+ (doseq [[message options] {"Query with all supplied parameters" nil
+ "Query using default values" {:defaults? true}}]
+ (testing (str "\n" message)
+ (let [query (count-query table {param-name [param-type value]} options)]
+ (testing (str "\nquery =\n" (u/pprint-to-str query))
+ (is (= expected-count
+ (ffirst
+ (mt/formatted-rows [int]
+ (qp/process-query query))))
+ (format "count with of %s with %s = %s should be %d"
+ (name table)
+ (name param-name)
+ value
+ expected-count))))))))))
--
GitLab