Merge pull request #5548 from metabase/fix-5457

Let people use comma-separated numbers in SQL num param inputs

src/metabase/query_processor/middleware/parameters/sql.clj

@@ -50,6 +50,9 @@
 
 (defrecord ^:private DateRange [start end])
 
+;; List of numbers to faciliate things like using params in a SQL `IN` clause. See the discussion in `value->number` for more details.
+(s/defrecord ^:private CommaSeparatedNumbers [numbers :- [s/Num]])
+
 ;; convenience for representing an *optional* parameter present in a query but whose value is unspecified in the param values.
 (defrecord ^:private NoValue [])
 
@@ -75,6 +78,7 @@
 
 (def ^:private ParamValue
   (s/named (s/maybe (s/cond-pre NoValue
+                                CommaSeparatedNumbers
                                 Dimension
                                 Date
                                 s/Num
@@ -138,13 +142,36 @@
       (when required
         (throw (Exception. (format "'%s' is a required param." display_name))))))
 
-(s/defn ^:private ^:always-validate value->number :- s/Num
+(s/defn ^:private ^:always-validate parse-number :- s/Num
+  "Parse a string like `1` or `2.0` into a valid number. Done mostly to keep people from passing in
+   things that aren't numbers, like SQL identifiers."
+  [s :- s/Str]
+  (.parse (NumberFormat/getInstance) ^String s))
+
+(s/defn ^:private ^:always-validate value->number :- (s/cond-pre s/Num CommaSeparatedNumbers)
+  "Parse a 'numeric' param value. Normally this returns an integer or floating-point number,
+   but as a somewhat undocumented feature it also accepts comma-separated lists of numbers. This was a side-effect of the
+   old parameter code that unquestioningly substituted any parameter passed in as a number directly into the SQL. This has
+   long been changed for security purposes (avoiding SQL injection), but since users have come to expect comma-separated
+   numeric values to work we'll allow that (with validation) and return an instance of `CommaSeperatedNumbers`. (That
+   is converted to SQL as a simple comma-separated list.)"
   [value]
-  (if (string? value)
-    (.parse (NumberFormat/getInstance) ^String value)
-    value))
+  (cond
+    ;; if not a string it's already been parsed
+    (number? value) value
+    ;; same goes for an instance of CommanSepe
+    (instance? CommaSeparatedNumbers value) value
+    value
+    ;; if the value is a string, then split it by commas in the string. Usually there should be none.
+    ;; Parse each part as a number.
+    (let [parts (for [part (str/split value #",")]
+                  (parse-number part))]
+      (if (> (count parts) 1)
+        ;; If there's more than one number return an instance of `CommaSeparatedNumbers`
+        (strict-map->CommaSeparatedNumbers {:numbers parts})
+        ;; otherwise just return the single number
+        (first parts)))))
 
-;; TODO - this should probably be converting strings to numbers (issue #3816)
 (s/defn ^:private ^:always-validate parse-value-for-type :- ParamValue
   [param-type value]
   (cond
@@ -252,6 +279,10 @@
   SqlCall (->replacement-snippet-info [this] (honeysql->replacement-snippet-info this))
   NoValue (->replacement-snippet-info [_]    {:replacement-snippet ""})
 
+  CommaSeparatedNumbers
+  (->replacement-snippet-info [{:keys [numbers]}]
+    {:replacement-snippet (str/join ", " numbers)})
+
   Date
   (->replacement-snippet-info [{:keys [s]}]
     (honeysql->replacement-snippet-info (u/->Timestamp s)))

test/metabase/query_processor_test/parameters_test.clj

 (ns metabase.query-processor-test.parameters_test
   "Tests for query parameters."
-  (:require [metabase
+  (:require [expectations :refer [expect]]
+            [metabase
              [query-processor :as qp]
              [query-processor-test :refer :all]]
             [metabase.query-processor.middleware.expand :as ql]
@@ -24,3 +25,18 @@
           outer-query (data/wrap-inner-query inner-query)
           outer-query (assoc outer-query :parameters [{:name "price", :type "category", :target ["field-id" (data/id :venues :price)], :value 4}])]
       (rows (qp/process-query outer-query)))))
+
+;; Make sure using commas in numeric params treats them as separate IDs (#5457)
+(expect
+  "SELECT * FROM USERS where id IN (1, 2, 3)"
+  (-> (qp/process-query
+        {:database   (data/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))