Make sure MBQL param tests use driver-specific identifiers [ci drivers]

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

@@ -13,7 +13,6 @@
 ;; See https://github.com/metabase/metabase/pull/4607#issuecomment-290884313 how we could support
 ;; hour/minute granularity in field parameter queries.
 
-
 (defn- day-range
   [^DateTime start, ^DateTime end]
   {:end   end
@@ -63,9 +62,9 @@
   (tf/parse (tf/formatters :date-opt-time) date))
 
 
-;;; +-------------------------------------------------------------------------------------------------------+
-;;; |                                    DATE STRING DECODERS                                               |
-;;; +-------------------------------------------------------------------------------------------------------+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; |                                              DATE STRING DECODERS                                              |
+;;; +----------------------------------------------------------------------------------------------------------------+
 
 ;; For parsing date strings and producing either a date range (for raw SQL parameter substitution) or a MBQL clause
 
@@ -80,9 +79,10 @@
 
 
 (defn- regex->parser
-  "Takes a regex and labels matching the regex capturing groups. Returns a parser which
-  takes a parameter value, validates the value against regex and gives a map of labels
-  and group values. Respects the following special label names:
+  "Takes a regex and labels matching the regex capturing groups. Returns a parser which takes a parameter value,
+  validates the value against regex and gives a map of labels and group values. Respects the following special label
+  names:
+
       :unit – finds a matching date unit and merges date unit operations to the result
       :int-value – converts the group value to integer
       :date, :date1, date2 – converts the group value to absolute date"
@@ -190,8 +190,8 @@
   (concat relative-date-string-decoders absolute-date-string-decoders))
 
 (defn- execute-decoders
-  "Returns the first successfully decoded value, run through both
-   parser and a range/filter decoder depending on `decoder-type`."
+  "Returns the first successfully decoded value, run through both parser and a range/filter decoder depending on
+  `decoder-type`."
   [decoders decoder-type decoder-param date-string]
   (some (fn [{parser :parser, parser-result-decoder decoder-type}]
           (when-let [parser-result (parser date-string)]
@@ -199,19 +199,19 @@
         decoders))
 
 (defn date-string->range
-  "Takes a string description of a date range such as 'lastmonth' or '2016-07-15~2016-08-6' and
-   return a MAP with `:start` and `:end` as iso8601 string formatted dates, respecting the given timezone."
+  "Takes a string description of a date range such as 'lastmonth' or '2016-07-15~2016-08-6' and return a MAP with
+  `:start` and `:end` as iso8601 string formatted dates, respecting the given timezone."
   [date-string report-timezone]
   (let [tz                 (t/time-zone-for-id report-timezone)
         formatter-local-tz (tf/formatter "yyyy-MM-dd" tz)
         formatter-no-tz    (tf/formatter "yyyy-MM-dd")
         today              (.withTimeAtStartOfDay (t/to-time-zone (t/now) tz))]
-    ;; Relative dates respect the given time zone because a notion like "last 7 days" might mean a different range of days
-    ;; depending on the user timezone
+    ;; Relative dates respect the given time zone because a notion like "last 7 days" might mean a different range of
+    ;; days depending on the user timezone
     (or (->> (execute-decoders relative-date-string-decoders :range today date-string)
              (m/map-vals (partial tf/unparse formatter-local-tz)))
-        ;; Absolute date ranges don't need the time zone conversion because in SQL the date ranges are compared against
-        ;; the db field value that is casted granularity level of a day in the db time zone
+        ;; Absolute date ranges don't need the time zone conversion because in SQL the date ranges are compared
+        ;; against the db field value that is casted granularity level of a day in the db time zone
         (->> (execute-decoders absolute-date-string-decoders :range nil date-string)
              (m/map-vals (partial tf/unparse formatter-no-tz))))))
 

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

@@ -45,9 +45,16 @@
   values in the queries themselves)."
   [query-dict [{:keys [target value], :as param} & rest]]
   (cond
-    (not param)      query-dict
+    (not param)
+    query-dict
+
     (or (not target)
-        (not value)) (recur query-dict rest)
-    :else            (let [filter-subclause (build-filter-clause param)
-                           query            (assoc-in query-dict [:query :filter] (merge-filter-clauses (get-in query-dict [:query :filter]) filter-subclause))]
-                       (recur query rest))))
+        (not value))
+    (recur query-dict rest)
+
+    :else
+    (let [filter-subclause (build-filter-clause param)
+          query            (assoc-in query-dict [:query :filter] (merge-filter-clauses
+                                                                  (get-in query-dict [:query :filter])
+                                                                  filter-subclause))]
+      (recur query rest))))

test/metabase/query_processor/middleware/parameters/mbql_test.clj

@@ -3,12 +3,16 @@
   (:require [expectations :refer :all]
             [metabase
              [query-processor :as qp]
-             [query-processor-test :refer [first-row rows format-rows-by non-timeseries-engines]]]
+             [query-processor-test :refer [first-row format-rows-by non-timeseries-engines rows]]
+             [util :as u]]
+            [metabase.models
+             [field :refer [Field]]
+             [table :refer [Table]]]
             [metabase.query-processor.middleware.expand :as ql]
             [metabase.query-processor.middleware.parameters.mbql :refer :all]
             [metabase.test.data :as data]
             [metabase.test.data.datasets :as datasets]
-            [metabase.util :as u]))
+            [metabase.util.honeysql-extensions :as hx]))
 
 (defn- expand-parameters [query]
   (expand (dissoc query :parameters) (:parameters query)))
@@ -203,9 +207,29 @@
       (rows (qp/process-query outer-query)))))
 
 ;; now let's make sure the correct query is actually being generated for the same thing above...
+;;
+;; TODO - the util function below is similar to a few other ones that are used elsewhere in the tests. It would be
+;; nice to unifiy those at some point or possibly move them into a shared utility namespace. Something to consider!
+(defn- mbql-param-quoted-and-qualified-name
+  "Generate a quoted and qualified identifier for a Table or Field for the current driver that will be used for MBQL
+  param subsitution below. e.g.
+
+    ;; with SQLServer
+    (mbql-param-quoted-and-qualified-name :venues) ;-> :dbo.venues
+    (mbql-param-quoted-and-qualified-name :venues :price) ;-> :dbo.venues.price"
+  ([table-kw]
+   (let [table (Table (data/id table-kw))]
+     (hx/qualify-and-escape-dots (:schema table) (:name table))))
+  ([table-kw field-kw]
+   (let [table (Table (data/id table-kw))
+         field (Field (data/id table-kw field-kw))]
+     (hx/qualify-and-escape-dots (:schema table) (:name table) (:name field)))))
+
 (datasets/expect-with-engines params-test-engines
-  {:query  (str "SELECT count(*) AS \"count\" FROM \"PUBLIC\".\"VENUES\" "
-                "WHERE (\"PUBLIC\".\"VENUES\".\"PRICE\" = 3 OR \"PUBLIC\".\"VENUES\".\"PRICE\" = 4)")
+  {:query  (format "SELECT count(*) AS \"count\" FROM %s WHERE (%s = 3 OR %s = 4)"
+                   (mbql-param-quoted-and-qualified-name :venues)
+                   (mbql-param-quoted-and-qualified-name :venues :price)
+                   (mbql-param-quoted-and-qualified-name :venues :price))
    :params nil}
   (let [inner-query (data/query venues
                       (ql/aggregation (ql/count)))
@@ -220,9 +244,12 @@
 ;; try it with date params as well. Even though there's no way to do this in the frontend AFAIK there's no reason we
 ;; can't handle it on the backend
 (datasets/expect-with-engines params-test-engines
-  {:query  (str "SELECT count(*) AS \"count\" FROM \"PUBLIC\".\"CHECKINS\" "
-                "WHERE (CAST(\"PUBLIC\".\"CHECKINS\".\"DATE\" AS date) BETWEEN CAST(? AS date) AND CAST(? AS date) "
-                "OR CAST(\"PUBLIC\".\"CHECKINS\".\"DATE\" AS date) BETWEEN CAST(? AS date) AND CAST(? AS date))")
+  {:query  (format (str "SELECT count(*) AS \"count\" FROM %s "
+                        "WHERE (CAST(%s AS date) BETWEEN CAST(? AS date) AND CAST(? AS date) "
+                        "OR CAST(%s date) BETWEEN CAST(? AS date) AND CAST(? AS date))")
+                   (mbql-param-quoted-and-qualified-name :checkins)
+                   (mbql-param-quoted-and-qualified-name :checkins :date)
+                   (mbql-param-quoted-and-qualified-name :checkins :date))
    :params [(u/->Timestamp #inst "2014-06-01")
             (u/->Timestamp #inst "2014-06-30")
             (u/->Timestamp #inst "2015-06-01")

test/metabase/test/data/datasets.clj

@@ -88,6 +88,7 @@
   "Bind `*engine*` and `*driver*` as appropriate for ENGINE and execute F, a function that takes no args."
   {:style/indent 1}
   [engine f]
+  {:pre [(keyword? engine)]}
   (binding [*engine* engine
             *driver* (engine->driver engine)]
     (f)))

test/metabase/test/data/generic_sql.clj

@@ -142,6 +142,7 @@
                (name (hx/qualify-and-escape-dots (quote-name driver n))))))
 
 (defn- default-qualify+quote-name
+  ;; TODO - what about schemas?
   ([driver db-name]
    (quote+combine-names driver (qualified-name-components driver db-name)))
   ([driver db-name table-name]