Fix multiple filters against same SQL field

src/metabase/query_processor/sql_parameters.clj

@@ -20,15 +20,19 @@
 (defprotocol ^:private ISQLParamSubstituion
   (^:private ->sql ^String [this]))
 
-(defrecord ^:private Dimension [^FieldInstance field, param])
+(defrecord ^:private Dimension [^FieldInstance field, param]) ;; param is either a single param or a vector of params
 
 (defrecord ^:private DateRange [start end])
 
 (defrecord ^:private NumberValue [value])
 
-(defn- dimension-value->sql [dimension-type value]
+(defn- dimension-value->sql
+  "Return an appropriate operator and rhs of a SQL `WHERE` clause, e.g. \"= 100\"."
+  ^String [dimension-type value]
   (if (contains? #{"date/range" "date/month-year" "date/quarter-year" "date/relative"} dimension-type)
+    ;; for relative dates convert the param to a `DateRange` record type and call `->sql` on it
     (->sql (map->DateRange ((resolve 'metabase.query-processor.parameters/date->range) value *timezone*))) ; TODO - get timezone from query dict
+    ;; for everything else just call `->sql` on it directly
     (str "= " (->sql value))))
 
 (defn- honeysql->sql ^String [x]
@@ -45,7 +49,7 @@
 (extend-protocol ISQLParamSubstituion
   nil         (->sql [_]    "NULL")
   Object      (->sql [this] (str this))
-  Boolean     (->sql [this] (if this "TRUE" "FALSE"))
+  Boolean     (->sql [this] (honeysql->sql this))
   NumberValue (->sql [this] (:value this))
   String      (->sql [this] (str \' (s/replace this #"'" "\\\\'") \')) ; quote single quotes inside the string
   Keyword     (->sql [this] (honeysql->sql this))
@@ -74,10 +78,16 @@
         (format "BETWEEN '%s' AND '%s'" start end))))
 
   Dimension
-  (->sql [{:keys [field param]}]
-    (if-not param
+  (->sql [{:keys [field param], :as dimension}]
+    (cond
       ;; if the param is `nil` just put in something that will always be true, such as `1` (e.g. `WHERE 1 = 1`)
-      "1 = 1"
+      (nil? param) "1 = 1"
+      ;; if we have a vector of multiple params recursively convert them to SQL and combine into an `AND` clause
+      (vector? param)
+      (format "(%s)" (s/join " AND " (for [p param]
+                                       (->sql (assoc dimension :param p)))))
+      ;; otherwise convert single param to SQL
+      :else
       (let [param-type (:type param)]
         (format "%s %s" (->sql (assoc field :type param-type)) (dimension-value->sql param-type (:value param)))))))
 
@@ -89,12 +99,17 @@
         v (->sql (k params))]
     (s/replace-first s match v)))
 
-(defn- handle-simple [s params]  (loop [s s, [[match param] & more] (re-seq #"\{\{\s*(\w+)\s*\}\}" s)]
+(defn- handle-simple
+  "Replace a 'simple' SQL parameter (curly brackets only, i.e. `{{...}}`)."
+  [s params]
+  (loop [s s, [[match param] & more] (re-seq #"\{\{\s*(\w+)\s*\}\}" s)]
     (if-not match
       s
       (recur (replace-param s params match param) more))))
 
-(defn- handle-optional [s params]
+(defn- handle-optional
+  "Replace an 'optional' parameter."
+  [s params]
   (try (handle-simple s params)
        (catch Throwable _
          "")))
@@ -117,11 +132,16 @@
 
 ;;; ------------------------------------------------------------ Param Resolution ------------------------------------------------------------
 
-(defn- param-with-target [params target]
-  (some (fn [param]
-          (when (= (:target param) target)
-            param))
-        params))
+(defn- param-with-target
+  "Return the param in PARAMS with a matching TARGET."
+  [params target]
+  (when-let [matching-params (seq (for [param params
+                                        :when (= (:target param) target)]
+                                    param))]
+    ;; if there's only one matching param no need to nest it inside a vector. Otherwise return vector of params
+    ((if (= (count matching-params) 1)
+       first
+       vec) matching-params)))
 
 (defn- param-value-for-tag [tag params]
   (when (not= (:type tag) "dimension")
@@ -158,7 +178,16 @@
                                         (dimension-value-for-tag tag params)
                                         (default-value-for-tag tag))))
 
-(defn- query->params-map [{{tags :template_tags} :native, params :parameters}]
+(defn- query->params-map
+  "Extract parameters info from QUERY. Return a map of parameter name -> value.
+
+     (query->params-map some-query)
+      ->
+      {:checkin_date {:field {:name \"date\", :parent_id nil, :table_id 1375}
+                      :param {:type   \"date/range\"
+                              :target [\"dimension\" [\"template-tag\" \"checkin_date\"]]
+                              :value  \"2015-01-01~2016-09-01\"}}}"
+  [{{tags :template_tags} :native, params :parameters}]
   (into {} (for [[k tag] tags
                  :let    [v (value-for-tag tag params)]
                  :when   v]

test/metabase/query_processor/sql_parameters_test.clj

@@ -17,7 +17,9 @@
 
 ;;; ------------------------------------------------------------ simple substitution -- {{x}} ------------------------------------------------------------
 
-(tu/resolve-private-vars metabase.query-processor.sql-parameters substitute)
+(defn- substitute {:style/indent 1} [sql params]
+  (binding [metabase.query-processor.sql-parameters/*driver* (driver/engine->driver :h2)] ; apparently you can still bind private dynamic vars
+    ((resolve 'metabase.query-processor.sql-parameters/substitute) sql params)))
 
 (expect "SELECT * FROM bird_facts WHERE toucans_are_cool = TRUE"
   (substitute "SELECT * FROM bird_facts WHERE toucans_are_cool = {{toucans_are_cool}}"
@@ -180,7 +182,23 @@
            :parent_id nil
            :table_id  (data/id :checkins)}
    :param nil}
-  (into {} (value-for-tag {:name "checkin_date", :display_name "Checkin Date", :type "dimension", :dimension ["field-id" (data/id :checkins :date)]} nil)))
+  (into {} (value-for-tag {:name "checkin_date", :display_name "Checkin Date", :type "dimension", :dimension ["field-id" (data/id :checkins :date)]}
+                          nil)))
+
+;; multiple values for the same tag should return a vector with multiple params instead of a single param
+(expect
+  {:field {:name      "DATE"
+           :parent_id nil
+           :table_id  (data/id :checkins)}
+   :param [{:type   "date/range"
+            :target ["dimension" ["template-tag" "checkin_date"]]
+            :value  "2015-01-01~2016-09-01"}
+           {:type   "date/single"
+            :target ["dimension" ["template-tag" "checkin_date"]]
+            :value  "2015-07-01"}]}
+  (into {} (value-for-tag {:name "checkin_date", :display_name "Checkin Date", :type "dimension", :dimension ["field-id" (data/id :checkins :date)]}
+                          [{:type "date/range",  :target ["dimension" ["template-tag" "checkin_date"]], :value "2015-01-01~2016-09-01"}
+                           {:type "date/single", :target ["dimension" ["template-tag" "checkin_date"]], :value "2015-07-01"}])))
 
 
 ;;; ------------------------------------------------------------ expansion tests: variables ------------------------------------------------------------
@@ -342,28 +360,28 @@
 (def ^:private ^:const sql-parameters-engines
   (set/difference (engines-that-support :native-parameters) #{:redshift :crate}))
 
+(defn- process-native {:style/indent 0} [& kvs]
+  (qp/process-query
+    (apply assoc {:database (data/id), :type :native} kvs)))
+
 (datasets/expect-with-engines sql-parameters-engines
   [29]
   (first-row
     (format-rows-by [int]
-      (qp/process-query
-        {:database   (data/id)
-         :type       :native
-         :native     {:query (format "SELECT COUNT(*) FROM %s WHERE {{checkin_date}}" (checkins-identifier))
-                      :template_tags {:checkin_date {:name "checkin_date", :display_name "Checkin Date", :type "dimension", :dimension ["field-id" (data/id :checkins :date)]}}}
-         :parameters [{:type "date/range", :target ["dimension" ["template-tag" "checkin_date"]], :value "2015-04-01~2015-05-01"}]}))))
+      (process-native
+        :native     {:query         (format "SELECT COUNT(*) FROM %s WHERE {{checkin_date}}" (checkins-identifier))
+                     :template_tags {:checkin_date {:name "checkin_date", :display_name "Checkin Date", :type "dimension", :dimension ["field-id" (data/id :checkins :date)]}}}
+        :parameters [{:type "date/range", :target ["dimension" ["template-tag" "checkin_date"]], :value "2015-04-01~2015-05-01"}]))))
 
 ;; no parameter -- should give us a query with "WHERE 1 = 1"
 (datasets/expect-with-engines sql-parameters-engines
   [1000]
   (first-row
     (format-rows-by [int]
-      (qp/process-query
-        {:database   (data/id)
-         :type       :native
-         :native     {:query (format "SELECT COUNT(*) FROM %s WHERE {{checkin_date}}" (checkins-identifier))
-                      :template_tags {:checkin_date {:name "checkin_date", :display_name "Checkin Date", :type "dimension", :dimension ["field-id" (data/id :checkins :date)]}}}
-         :parameters []}))))
+      (process-native
+        :native     {:query         (format "SELECT COUNT(*) FROM %s WHERE {{checkin_date}}" (checkins-identifier))
+                     :template_tags {:checkin_date {:name "checkin_date", :display_name "Checkin Date", :type "dimension", :dimension ["field-id" (data/id :checkins :date)]}}}
+        :parameters []))))
 
 ;; test that relative dates work correctly. It should be enough to try just one type of relative date here,
 ;; since handling them gets delegated to the functions in `metabase.query-processor.parameters`, which is fully-tested :D
@@ -371,9 +389,19 @@
   [0]
   (first-row
     (format-rows-by [int]
-      (qp/process-query
-        {:database   (data/id)
-         :type       :native
-         :native     {:query (format "SELECT COUNT(*) FROM %s WHERE {{checkin_date}}" (checkins-identifier))
-                      :template_tags {:checkin_date {:name "checkin_date", :display_name "Checkin Date", :type "dimension", :dimension ["field-id" (data/id :checkins :date)]}}}
-         :parameters [{:type "date/relative", :target ["dimension" ["template-tag" "checkin_date"]], :value "thismonth"}]}))))
+      (process-native
+        :native     {:query         (format "SELECT COUNT(*) FROM %s WHERE {{checkin_date}}" (checkins-identifier))
+                     :template_tags {:checkin_date {:name "checkin_date", :display_name "Checkin Date", :type "dimension", :dimension ["field-id" (data/id :checkins :date)]}}}
+        :parameters [{:type "date/relative", :target ["dimension" ["template-tag" "checkin_date"]], :value "thismonth"}]))))
+
+
+;; test that multiple filters applied to the same variable combine into `AND` clauses (#3539)
+(datasets/expect-with-engines sql-parameters-engines
+  [4]
+  (first-row
+    (format-rows-by [int]
+      (process-native
+       :native     {:query         (format "SELECT COUNT(*) FROM %s WHERE {{checkin_date}}" (checkins-identifier))
+                    :template_tags {:checkin_date {:name "checkin_date", :display_name "Checkin Date", :type "dimension", :dimension ["field-id" (data/id :checkins :date)]}}}
+       :parameters [{:type "date/range",  :target ["dimension" ["template-tag" "checkin_date"]], :value "2015-01-01~2016-09-01"}
+                    {:type "date/single", :target ["dimension" ["template-tag" "checkin_date"]], :value "2015-07-01"}]))))