Improvements to MongoDB params [ci mongo] (#11485)

modules/drivers/mongo/src/metabase/driver/mongo/parameters.clj

@@ -15,6 +15,15 @@
   (:import java.time.temporal.Temporal
            metabase.driver.common.parameters.Date))
 
+(defn- param-value->str [x]
+  (condp instance? x
+    ;; Date = the Parameters Date type, not an actual Temporal type
+    Date     (param-value->str (u.date/parse (:s x)))
+    ;; convert temporal types to ISODate("2019-12-09T...") (etc.)
+    Temporal (format "ISODate(\"%s\")" (u.date/format x))
+    ;; for everything else, splice it in as its string representation
+    (pr-str x)))
+
 (defn- substitute-param [param->value [acc missing] in-optional? {:keys [k]}]
   (if-not (contains? param->value k)
     [acc (conj missing k)]
@@ -24,7 +33,7 @@
                  {:type error-type/invalid-query})))
       (if (= params/no-value v)
         [acc (conj missing k)]
-        [(conj acc v) missing]))))
+        [(conj acc (param-value->str v)) missing]))))
 
 (declare substitute*)
 
@@ -55,26 +64,20 @@
    [[] nil]
    xs))
 
-(defn- ->str [x]
-  (condp instance? x
-    Temporal (u.date/format x)
-    Date     (->str (u.date/parse (:s x)))
-    x))
-
 (defn- substitute [param->value xs]
   (let [[replaced missing] (substitute* param->value xs false)]
     (when (seq missing)
       (throw (ex-info (tru "Cannot run query: missing required parameters: {0}" (set missing))
                {:type error-type/invalid-query})))
     (when (seq replaced)
-      (str/join (map ->str replaced)))))
+      (str/join replaced))))
 
 (defn- parse-and-substitute [param->value x]
   (if-not (string? x)
     x
     (u/prog1 (substitute param->value (parse/parse x))
       (when-not (= x <>)
-        (log/debug (tru "Substituted {0} -> {1}" (pr-str x) (pr-str <> )))))))
+        (log/debug (tru "Substituted {0} -> {1}" (pr-str x) (pr-str <>)))))))
 
 (defn substitute-native-parameters
   "Implementation of `driver/substitue-native-parameters` for MongoDB."

modules/drivers/mongo/src/metabase/driver/mongo/query_processor.clj

@@ -14,6 +14,7 @@
              [util :as mbql.u]]
             [metabase.models.field :refer [Field]]
             [metabase.query-processor
+             [error-type :as error-type]
              [interface :as i]
              [store :as qp.store]]
             [metabase.query-processor.middleware.annotate :as annotate]
@@ -659,8 +660,14 @@
 (defn parse-query-string
   "Parse a serialized native query. Like a normal JSON parse, but handles BSON/MongoDB extended JSON forms."
   [^String s]
-  (for [^org.bson.BsonValue v (org.bson.BsonArray/parse s)]
-    (com.mongodb.BasicDBObject. (.asDocument v))))
+  (try
+    (for [^org.bson.BsonValue v (org.bson.BsonArray/parse s)]
+      (com.mongodb.BasicDBObject. (.asDocument v)))
+    (catch Throwable e
+      (throw (ex-info (tru "Unable to parse query. Query should be an array of aggregation pipeline stages.")
+               {:type  error-type/invalid-query
+                :query s}
+               e)))))
 
 (defn execute-query
   "Process and run a native MongoDB query."

modules/drivers/mongo/test/metabase/driver/mongo/parameters_test.clj

@@ -27,12 +27,12 @@
                (substitute {:x 100} ["2" (param :x)]))
             "\"2{{x}}\" with x = 100 should be replaced with string \"2100\""))
       (testing "temporal params"
-        (is (= "2019-12-06T17:01:00-08:00"
+        (is (= "ISODate(\"2019-12-06T17:01:00-08:00\")"
                (substitute {:written-at #t "2019-12-06T17:01:00-08:00[America/Los_Angeles]"} [(param :written-at)]))))
       (testing "multiple params in one string"
-        (is (= "2019-12-06"
-               (substitute {:year 2019, :month 12, :day "06"} [(param :year) "-" (param :month) "-" (param :day)]))
-            "\"{{year}}-{{month}}-{{day}}\" should be replaced with \"2019-12-06\"")
+        (is (= "2019-12-10"
+               (substitute {:year 2019, :month 12, :day 10} [(param :year) "-" (param :month) "-" (param :day)]))
+            "\"{{year}}-{{month}}-{{day}}\" should be replaced with \"2019-12-09\"")
         (testing "some params missing"
           (is (thrown-with-msg? clojure.lang.ExceptionInfo
                                 #"missing required parameters: #\{:day\}"
@@ -45,8 +45,8 @@
       (testing "{{year}}[[-{{month}}[[-{{day}}]]]]"
         (let [params [(param :year) (optional "-" (param :month) (optional "-" (param :day)))]]
           (testing "with all params present"
-            (is (= "2019-12-06"
-                   (substitute {:year 2019, :month 12, :day "06"} params))))
+            (is (= "2019-12-10"
+                   (substitute {:year 2019, :month 12, :day 10} params))))
           (testing "with :year & :month present but not :day"
             (is (= "2019-12"
                    (substitute {:year 2019, :month 12} params))))

test/metabase/query_processor_test/parameters_test.clj

 (ns metabase.query-processor-test.parameters-test
   "Tests for support for parameterized queries in drivers that support it. (There are other tests for parameter support
   in various places; these are mainly for high-level verification that parameters are working.)"
-  (:require [cheshire.core :as json]
+  (:require [cheshire
+             [core :as json]
+             [generate :as json.generate]]
             [clojure
              [string :as str]
              [test :refer :all]]
@@ -11,7 +13,8 @@
              [models :refer [Field]]
              [query-processor :as qp]
              [test :as mt]
-             [util :as u]]))
+             [util :as u]])
+  (:import com.fasterxml.jackson.core.JsonGenerator))
 
 (defmulti native-count-query
   "Generate a native query for the count of rows in `table` matching a set of conditions defined by `field->type+value`,
@@ -49,18 +52,26 @@
                   query)
       {:query query})))
 
+(defn- json-raw
+  "Wrap a string so it will be spliced directly into resulting JSON as-is. Analogous to HoneySQL `raw`."
+  [^String s]
+  (reify json.generate/JSONable
+    (to-json [_ generator]
+      (.writeRawValue ^JsonGenerator generator s))))
+
+(deftest json-raw-test
+  (testing "Make sure the `json-raw` util fn actually works the way we expect it to"
+    (is (= "{\"x\":{{param}}}"
+           (json/generate-string {:x (json-raw "{{param}}")})))))
+
 (defmethod native-count-query :mongo
   [driver table field->type+value]
   (driver/with-driver driver
     {:projections [:count]
      :query       (json/generate-string
                    [{:$match (into {} (for [[field [param-type]] field->type+value
-                                            :let                 [base-type (:base_type (Field (mt/id table field)))
-                                                                  xform     (case param-type
-                                                                              :number      (fn [k] {:$numberInt k})
-                                                                              :text        identity
-                                                                              :date/single (fn [k] {:$date k}))]]
-                                        [(name field) (xform (format "{{%s}}" (name field)))]))}
+                                            :let                 [base-type (:base_type (Field (mt/id table field)))]]
+                                        [(name field) (json-raw (format "{{%s}}" (name field)))]))}
                     {:$group {"_id" nil, "count" {:$sum 1}}}
                     {:$sort {"_id" 1}}
                     {:$project {"_id" false, "count" true}}])
@@ -108,5 +119,7 @@
         (count= 1
                 :users {:last_login [:date/single "2014-08-02T09:30Z"]})))))
 
+;; TODO - tests for optional params
+
 ;; TODO - field-filter params, only for drivers that support `:native-parameters-field-filters`. Currently, this is
 ;; only SQL, and this feature is tested elsewhere. But we should add some tests here too.