Fix SQL Server field filters against boolean Fields (#11691)

* Fix BigQuery driver from Leiningen [ci bigquery] [ci sparksql] (#11650)

* Fix SQL Server field filters against boolean Fields.
[ci drivers]

* Test fixes for drivers with no TIME data type
[ci oracle] [ci presto] [ci redshift] [ci snowflake] [ci sparksql]

* Fix boolean field filters for Oracle [ci oracle] [ci sparksql]

.gitignore

@@ -36,6 +36,7 @@
 /local/src
 /local/src
 /locales/metabase-*.pot
+/modules/drivers/*/resources/namespaces.edn
 /modules/drivers/*/target
 /node_modules/
 /osx-artifacts

modules/drivers/bigquery/project.clj

-(defproject metabase/bigquery-driver "1.0.0-SNAPSHOT-1.27.0"
+(defproject metabase/bigquery-driver "1.0.0-SNAPSHOT-1.30.3"
   :min-lein-version "2.5.0"
 
   :dependencies
-  [[com.google.apis/google-api-services-bigquery "v2-rev20181202-1.27.0"]]
+  [[com.google.apis/google-api-services-bigquery "v2-rev20190917-1.30.3"]]
 
   :profiles
   {:provided
    {:dependencies
     [[org.clojure/clojure "1.10.1"]
      [metabase-core "1.0.0-SNAPSHOT"]
-     [metabase/google-driver "1.0.0-SNAPSHOT-1.27.0"]]}
+     [metabase/google-driver "1.0.0-SNAPSHOT-1.30.7"]]}
 
    :uberjar
    {:auto-clean    true

modules/drivers/bigquery/resources/metabase-plugin.yaml

 info:
   name: Metabase BigQuery Driver
-  version: 1.0.0-SNAPSHOT-1.27.0
+  version: 1.0.0-SNAPSHOT-1.30.3
   description: Allows Metabase to connect to Google BigQuery databases.
 dependencies:
   - plugin: Metabase Google Drivers Shared Dependencies

modules/drivers/google/project.clj

-(defproject metabase/google-driver "1.0.0-SNAPSHOT-1.27.0"
+(defproject metabase/google-driver "1.0.0-SNAPSHOT-1.30.7"
   :min-lein-version "2.5.0"
 
   :aliases
   {"install-for-building-drivers" ["with-profile" "+install-for-building-drivers" "install"]}
 
   :dependencies
-  [[com.google.api-client/google-api-client "1.27.0"]]
+  [[com.google.api-client/google-api-client "1.30.7"]]
 
   :profiles
   {:provided

modules/drivers/google/resources/metabase-plugin.yaml

 info:
   name: Metabase Google Drivers Shared Dependencies
-  version: 1.0.0-SNAPSHOT-1.27.0
+  version: 1.0.0-SNAPSHOT-1.30.7
   description: Shared dependencies for BigQuery, Google Analytics, and other Google drivers.
 driver:
   name: google

modules/drivers/google/src/metabase/driver/google.clj

@@ -41,7 +41,7 @@
                            (into {} error)))))))
 
 (defn execute
-  "`execute` REQUEST, and catch any `GoogleJsonResponseException` is throws, converting them to `ExceptionInfo` and
+  "Execute `request`, and catch any `GoogleJsonResponseException` is throws, converting them to `ExceptionInfo` and
   rethrowing them.
 
   This automatically retries any failed requests up to 2 times."

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

@@ -49,8 +49,19 @@
 ;; sequence of those maps.
 (defn- substitute-one-field-filter [{field :field, {param-type :type, value :value} :value, :as field-filter}]
   ;; convert relative dates to approprate date range representations
-  (if (date-params/date-range-type? param-type)
+  (cond
+    (date-params/date-range-type? param-type)
     (substitute-one-field-filter-date-range field-filter)
+
+    ;; a `date/single` like `2020-01-10`
+    (and (date-params/date-type? param-type)
+         (string? value))
+    (let [t (u.date/parse value)]
+      (format "{$and: [%s, %s]}"
+              (format "{%s: {$gte: %s}}" (field->name field) (param-value->str t))
+              (format "{%s: {$lt: %s}}"  (field->name field) (param-value->str (u.date/add t :day 1)))))
+
+    :else
     (format "{%s: %s}" (field->name field) (param-value->str value))))
 
 (defn- substitute-field-filter [{field :field, {:keys [value]} :value, :as field-filter}]

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

@@ -91,6 +91,10 @@
         (is (= "[{$match: {\"id\": {$in: [1, 2, 3]}}}]"
                (substitute {:id (field-filter "id" :number v)}
                            ["[{$match: " (param :id) "}]"]))))))
+  (testing "single date"
+    (is (= "[{$match: {$and: [{\"date\": {$gte: ISODate(\"2019-12-08\")}}, {\"date\": {$lt: ISODate(\"2019-12-09\")}}]}}]"
+           (substitute {:date (field-filter "date" :date/single "2019-12-08")}
+                       ["[{$match: " (param :date) "}]"]))))
   (testing "parameter not supplied"
     (is (= "[{$match: {}}]"
            (substitute {:date (common.params/->FieldFilter {:name "date"} common.params/no-value)} ["[{$match: " (param :date) "}]"])))))

modules/drivers/oracle/src/metabase/driver/oracle.clj

@@ -4,7 +4,9 @@
             [honeysql.core :as hsql]
             [java-time :as t]
             [metabase.driver :as driver]
-            [metabase.driver.common :as driver.common]
+            [metabase.driver
+             [common :as driver.common]
+             [sql :as sql]]
             [metabase.driver.sql
              [query-processor :as sql.qp]
              [util :as sql.u]]
@@ -313,3 +315,8 @@
 (defmethod unprepare/unprepare-value [:oracle Instant]
   [driver t]
   (unprepare/unprepare-value driver (t/zoned-date-time t (t/zone-id "UTC"))))
+
+;; Oracle doesn't really support boolean types so use bits instead (See #11592, similar issue for SQL Server)
+(defmethod sql/->prepared-substitution [:oracle Boolean]
+  [driver bool]
+  (sql/->prepared-substitution driver (if bool 1 0)))

modules/drivers/sparksql/project.clj

@@ -7,7 +7,7 @@
    ;; implementations of things like log4j <-> slf4j, or are part of both hadoop-common and hive-jdbc;
    [org.apache.hadoop/hadoop-common "3.1.1"
     :exclusions [com.fasterxml.jackson.core/jackson-core
-                 #_com.google.guava/guava
+                 com.google.guava/guava
                  commons-logging
                  org.apache.httpcomponents/httpcore
                  org.codehaus.jackson/jackson-core-asl

modules/drivers/sqlserver/src/metabase/driver/sqlserver.clj

@@ -5,7 +5,9 @@
             [metabase
              [config :as config]
              [driver :as driver]]
-            [metabase.driver.common :as driver.common]
+            [metabase.driver
+             [common :as driver.common]
+             [sql :as sql]]
             [metabase.driver.sql-jdbc
              [common :as sql-jdbc.common]
              [connection :as sql-jdbc.conn]
@@ -303,3 +305,8 @@
 (defmethod sql-jdbc.execute/read-column [:sqlserver microsoft.sql.Types/DATETIMEOFFSET]
   [_ _^ResultSet rs _ ^Integer i]
   (.getObject rs i OffsetDateTime))
+
+;; SQL Server doesn't really support boolean types so use bits instead (#11592)
+(defmethod sql/->prepared-substitution [:sqlserver Boolean]
+  [driver bool]
+  (sql/->prepared-substitution driver (if bool 1 0)))

modules/drivers/sqlserver/test/metabase/test/data/sqlserver.clj

@@ -26,10 +26,10 @@
 
 (defmethod tx/dbdef->connection-details :sqlserver
   [_ context {:keys [database-name]}]
-  {:host     (tx/db-test-env-var-or-throw :sqlserver :host)
+  {:host     (tx/db-test-env-var-or-throw :sqlserver :host "localhost")
    :port     (Integer/parseInt (tx/db-test-env-var-or-throw :sqlserver :port "1433"))
-   :user     (tx/db-test-env-var-or-throw :sqlserver :user)
-   :password (tx/db-test-env-var-or-throw :sqlserver :password)
+   :user     (tx/db-test-env-var-or-throw :sqlserver :user "SA")
+   :password (tx/db-test-env-var-or-throw :sqlserver :password "P@ssw0rd")
    :db       (when (= context :db)
                database-name)})
 

project.clj

@@ -77,6 +77,7 @@
                  it.unimi.dsi/fastutil]]
    [com.draines/postal "2.0.3"]                                       ; SMTP library
    [com.jcraft/jsch "0.1.55"]                                         ; SSH client for tunnels
+   [com.google.guava/guava "28.2-jre"]                                ; dep for BigQuery, Spark, and GA. Require here rather than letting different dep versions stomp on each other — see comments on #9697
    [com.h2database/h2 "1.4.197"]                                      ; embedded SQL database
    [com.mattbertolini/liquibase-slf4j "2.0.0"]                        ; Java Migrations lib logging. We don't actually use this AFAIK (?)
    [com.taoensso/nippy "2.14.0"]                                      ; Fast serialization (i.e., GZIP) library for Clojure

test/metabase/query_processor_test/parameters_test.clj

@@ -7,7 +7,6 @@
             [clojure
              [string :as str]
              [test :refer :all]]
-            [clojure.tools.logging :as log]
             [metabase
              [driver :as driver]
              [models :refer [Field]]
@@ -16,41 +15,37 @@
              [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`,
-  which looks like
+(defn- run-count-query [query]
+  (or (ffirst
+       (mt/formatted-rows [int]
+         (qp/process-query query)))
+      ;; HACK (!) Mongo returns `nil` count instead of 0 — (#5419) — workaround until this is fixed
+      0))
 
-    {field-name [param-type param-value]}
+(defn- query-with-default-parameter-value [query param-name param-value]
+  (assoc-in query [:native :template-tags (name param-name) :default] param-value))
 
-  e.g.
 
-    (native-count-query :mongo :venues {:price [:number 2]})"
-  ^{:arglists '([driver table field->type+value])}
-  driver/dispatch-on-initialized-driver
-  :hierarchy #'driver/hierarchy)
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; |                                              Template Tag Params                                               |
+;;; +----------------------------------------------------------------------------------------------------------------+
 
-;; TODO - these should go in test extensions namespaces, not here
+(defmulti ^:private count-with-template-tag-query
+  "Generate a native query for the count of rows in `table` matching a set of conditions where `field-name` is equal to
+  a param `value`."
+  ^{:arglists '([driver table-name field-name param-type])}
+  mt/dispatch-on-driver-with-test-extensions
+  :hierarchy #'driver/hierarchy)
 
-(defmethod native-count-query :sql
-  [driver table field->type+value]
+(defmethod count-with-template-tag-query :sql
+  [driver table field param-type]
   (driver/with-driver driver
     (let [mbql-query      (mt/mbql-query nil
                             {:source-table (mt/id table)
                              :aggregation  [[:count]]
-                             :filter       (into [:and]
-                                                 (for [[i [field]] (map-indexed vector field->type+value)]
-                                                   [:= [:field-id (mt/id table field)] i]))})
+                             :filter       [:= [:field-id (mt/id table field)] 1]})
           {:keys [query]} (qp/query->native mbql-query)
-          query           (reduce
-                           (fn [query [field]]
-                             ;; TODO — currently only supports one field
-                             (str/replace query (re-pattern #"= .*") (format "= {{%s}}" (name field))))
-                           query
-                           field->type+value)]
-      (log/tracef "%s\n->\n%s\n->\n%s"
-                  (pr-str (list 'native-count-query driver table field->type+value))
-                  (pr-str mbql-query)
-                  query)
+          query           (str/replace query (re-pattern #"= .*") (format "= {{%s}}" (name field)))]
       {:query query})))
 
 (defn- json-raw
@@ -65,69 +60,135 @@
     (is (= "{\"x\":{{param}}}"
            (json/generate-string {:x (json-raw "{{param}}")})))))
 
-(defmethod native-count-query :mongo
-  [driver table field->type+value]
+(defmethod count-with-template-tag-query :mongo
+  [driver table-name field-name param-type]
+  (let [{base-type :base_type} (Field (driver/with-driver driver (mt/id table-name field-name)))]
+    {:projections [:count]
+     :query       (json/generate-string
+                   [{:$match {(name field-name) (json-raw (format "{{%s}}" (name field-name)))}}
+                    {:$group {"_id" nil, "count" {:$sum 1}}}
+                    {:$sort {"_id" 1}}
+                    {:$project {"_id" false, "count" true}}])
+     :collection  (name table-name)}))
+
+(defn- template-tag-count-query [table field param-type param-value {:keys [defaults?]}]
+  (let [query {:database (mt/id)
+               :type     :native
+               :native   (assoc (count-with-template-tag-query driver/*driver* table field param-type)
+                                :template-tags {(name field) {:name         (name field)
+                                                              :display-name (name field)
+                                                              :type         (or (namespace param-type)
+                                                                                (name param-type))}})}]
+    (if defaults?
+      (query-with-default-parameter-value query field param-value)
+      (assoc query :parameters [{:type   param-type
+                                 :target [:variable [:template-tag (name field)]]
+                                 :value  param-value}]))))
+
+(deftest template-tag-param-test
+  (mt/test-drivers (mt/normal-drivers-with-feature :native-parameters)
+    (letfn [(count-with-params [table param-name param-type value & [options]]
+              (run-count-query
+               (template-tag-count-query table param-name param-type value options)))]
+      (doseq [[message options] {"Query with all supplied parameters" nil
+                                 "Query using default values"         {:defaults? true}}]
+        (testing message
+          (testing "text params"
+            (is (= 1
+                   (count-with-params :venues :name :text "In-N-Out Burger" options))))
+          (testing "number params"
+            (is (= 22
+                   (count-with-params :venues :price :number "1" options))))
+          ;; 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"
+              (is (= 1
+                     (count-with-params :users :last_login :date/single "2014-08-02T09:30Z" options))))))))))
+
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; |                                              Field Filter Params                                               |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+(defmulti ^:private count-with-field-filter-query
+  ^{:arglists '([driver table-name field-name])}
+  mt/dispatch-on-driver-with-test-extensions
+  :hierarchy #'driver/hierarchy)
+
+(defmethod count-with-field-filter-query :sql
+  [driver table field]
   (driver/with-driver driver
+    (let [mbql-query      (mt/mbql-query nil
+                            {:source-table (mt/id table)
+                             :aggregation  [[:count]]
+                             :filter       [:= [:field-id (mt/id table field)] 1]})
+          {:keys [query]} (qp/query->native mbql-query)
+          query           (str/replace query (re-pattern #"WHERE .* = .*") (format "WHERE {{%s}}" (name field)))]
+      {:query query})))
+
+(defmethod count-with-field-filter-query :mongo
+  [driver table-name field-name]
+  (let [{base-type :base_type} (Field (driver/with-driver driver (mt/id table-name field-name)))]
     {: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)))]]
-                                        [(name field) (json-raw (format "{{%s}}" (name field)))]))}
+                   [{:$match (json-raw (format "{{%s}}" (name field-name)))}
                     {:$group {"_id" nil, "count" {:$sum 1}}}
                     {:$sort {"_id" 1}}
                     {:$project {"_id" false, "count" true}}])
-     :collection  (name table)}))
+     :collection  (name table-name)}))
 
-(defn- count-query [table field->type+value {:keys [defaults?]}]
+(defn- field-filter-count-query [table field value-type value]
   {:database   (mt/id)
    :type       :native
-   :native     (assoc (native-count-query driver/*driver* table field->type+value)
-                      :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)
-    (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))
+   :native     (assoc (count-with-field-filter-query driver/*driver* table field)
+                      :template-tags {(name field) {:name         (name field)
+                                                    :display-name (name field)
+                                                    :type         :dimension
+                                                    :dimension    [:field-id (mt/id table field)]}})
+   :parameters [{:type   value-type
+                 :name   (name field)
+                 :target [:dimension [:template-tag (name field)]]
+                 :value  value}]})
+
+;; this isn't a complete test for all possible field filter types, but it covers mostly everything
+(deftest field-filter-param-test
+  (letfn [(is-count-= [expected-count table field value-type value]
+            (let [query (field-filter-count-query table field value-type value)]
+              (testing (format "\nquery = \n%s" (u/pprint-to-str 'cyan 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))))))))))
+                       (run-count-query query))))))]
+    (mt/test-drivers (mt/normal-drivers-with-feature :native-parameters)
+      (testing "temporal field filters"
+        ;; TIMEZONE FIXME — The excluded drivers below don't have TIME types, so the `attempted-murders` dataset doesn't
+        ;; currently work. We should use the closest equivalent types (e.g. `DATETIME` or `TIMESTAMP` so we can still
+        ;; load the dataset and run tests using this dataset such as these, which doesn't even use the TIME type.
+        (when-not (#{:oracle :presto :redshift :sparksql :snowflake} driver/*driver*)
+          (mt/dataset attempted-murders
+            (doseq [field
+                    [:datetime
+                     :date
+                     :datetime_tz]
+
+                    [value-type value expected-count]
+                    [[:date/relative     "past30days" 0]
+                     [:date/range        "2019-11-01~2020-01-09" 20]
+                     [:date/single       "2019-11-12" 1]
+                     [:date/quarter-year "Q4-2019" 20]
+                     [:date/month-year   "2019-11" 20]]]
+              (testing (format "\nField filter with %s Field" field)
+                (testing (format "\nfiltering against %s value '%s'" value-type value)
+                  (is-count-= expected-count
+                              :attempts field value-type value)))))))
+      ;; FIXME — Field Filters don't seem to be working correctly for SparkSQL
+      (when-not (= driver/*driver* :sparksql)
+        (testing "text params"
+          (is-count-= 1
+                      :venues :name :text "In-N-Out Burger"))
+        (testing "number params"
+          (is-count-= 22
+                      :venues :price :number "1"))
+        (testing "boolean params"
+          (mt/dataset places-cam-likes
+            (is-count-= 2
+                        :places :liked :boolean true)))))))

test/metabase/test.clj

@@ -141,6 +141,7 @@
   db-test-env-var
   db-test-env-var-or-throw
   dbdef->connection-details
+  dispatch-on-driver-with-test-extensions
   get-dataset-definition
   has-questionable-timezone-support?
   has-test-extensions?

test/metabase/test/data/dataset_definitions.clj

@@ -145,10 +145,12 @@
           [username last-login password-text (if (zero? idx)
                                                1
                                                idx)])))))
-
 (tx/defdataset ^:private attempted-murders
   "A dataset for testing temporal values with and without timezones. Records of number of crow counts spoted and the
-  date/time when they spotting occured in several different column types."
+  date/time when they spotting occured in several different column types.
+
+  No Database we support supports all of these different types, so the expectation is that we'll use the closest
+  equivalent for each column."
   [["attempts"
     [{:field-name "num_crows",      :base-type :type/Integer}
      {:field-name "date",           :base-type :type/Date}