From b6877747b776f19197d83320c6bd0f6b43bd3219 Mon Sep 17 00:00:00 2001 From: Cam Saul <1455846+camsaul@users.noreply.github.com> Date: Fri, 10 Jan 2020 17:40:27 -0800 Subject: [PATCH] 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 | 1 + modules/drivers/bigquery/project.clj | 6 +- .../bigquery/resources/metabase-plugin.yaml | 2 +- modules/drivers/google/project.clj | 4 +- .../google/resources/metabase-plugin.yaml | 2 +- .../google/src/metabase/driver/google.clj | 2 +- .../src/metabase/driver/mongo/parameters.clj | 13 +- .../metabase/driver/mongo/parameters_test.clj | 4 + .../oracle/src/metabase/driver/oracle.clj | 9 +- modules/drivers/sparksql/project.clj | 2 +- .../src/metabase/driver/sqlserver.clj | 9 +- .../test/metabase/test/data/sqlserver.clj | 6 +- project.clj | 1 + .../query_processor_test/parameters_test.clj | 225 +++++++++++------- test/metabase/test.clj | 1 + .../test/data/dataset_definitions.clj | 6 +- 16 files changed, 194 insertions(+), 99 deletions(-) diff --git a/.gitignore b/.gitignore index 07b7b504af6..3a9d6edb55d 100644 --- a/.gitignore +++ b/.gitignore @@ -36,6 +36,7 @@ /local/src /local/src /locales/metabase-*.pot +/modules/drivers/*/resources/namespaces.edn /modules/drivers/*/target /node_modules/ /osx-artifacts diff --git a/modules/drivers/bigquery/project.clj b/modules/drivers/bigquery/project.clj index 354c8bf2c7e..235557fcf04 100644 --- a/modules/drivers/bigquery/project.clj +++ b/modules/drivers/bigquery/project.clj @@ -1,15 +1,15 @@ -(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 diff --git a/modules/drivers/bigquery/resources/metabase-plugin.yaml b/modules/drivers/bigquery/resources/metabase-plugin.yaml index e00e51c468d..88753131257 100644 --- a/modules/drivers/bigquery/resources/metabase-plugin.yaml +++ b/modules/drivers/bigquery/resources/metabase-plugin.yaml @@ -1,6 +1,6 @@ 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 diff --git a/modules/drivers/google/project.clj b/modules/drivers/google/project.clj index c5f91e23a47..cf94b367210 100644 --- a/modules/drivers/google/project.clj +++ b/modules/drivers/google/project.clj @@ -1,11 +1,11 @@ -(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 diff --git a/modules/drivers/google/resources/metabase-plugin.yaml b/modules/drivers/google/resources/metabase-plugin.yaml index 85b769c91eb..055357d80d0 100644 --- a/modules/drivers/google/resources/metabase-plugin.yaml +++ b/modules/drivers/google/resources/metabase-plugin.yaml @@ -1,6 +1,6 @@ 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 diff --git a/modules/drivers/google/src/metabase/driver/google.clj b/modules/drivers/google/src/metabase/driver/google.clj index e108b2677f0..176cc78f2e4 100644 --- a/modules/drivers/google/src/metabase/driver/google.clj +++ b/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." diff --git a/modules/drivers/mongo/src/metabase/driver/mongo/parameters.clj b/modules/drivers/mongo/src/metabase/driver/mongo/parameters.clj index 0461c98e78f..b3ca54a772e 100644 --- a/modules/drivers/mongo/src/metabase/driver/mongo/parameters.clj +++ b/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}] diff --git a/modules/drivers/mongo/test/metabase/driver/mongo/parameters_test.clj b/modules/drivers/mongo/test/metabase/driver/mongo/parameters_test.clj index 60a834964e9..f3b0530af4f 100644 --- a/modules/drivers/mongo/test/metabase/driver/mongo/parameters_test.clj +++ b/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) "}]"]))))) diff --git a/modules/drivers/oracle/src/metabase/driver/oracle.clj b/modules/drivers/oracle/src/metabase/driver/oracle.clj index 1d52d362187..80468b41e44 100644 --- a/modules/drivers/oracle/src/metabase/driver/oracle.clj +++ b/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))) diff --git a/modules/drivers/sparksql/project.clj b/modules/drivers/sparksql/project.clj index 6b5c3b52c40..7b302959dd8 100644 --- a/modules/drivers/sparksql/project.clj +++ b/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 diff --git a/modules/drivers/sqlserver/src/metabase/driver/sqlserver.clj b/modules/drivers/sqlserver/src/metabase/driver/sqlserver.clj index 27553336d10..cdac0809fba 100644 --- a/modules/drivers/sqlserver/src/metabase/driver/sqlserver.clj +++ b/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))) diff --git a/modules/drivers/sqlserver/test/metabase/test/data/sqlserver.clj b/modules/drivers/sqlserver/test/metabase/test/data/sqlserver.clj index a4534ba0ffa..bc9a7086d7a 100644 --- a/modules/drivers/sqlserver/test/metabase/test/data/sqlserver.clj +++ b/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)}) diff --git a/project.clj b/project.clj index 238cdd983c4..8b638a2b15e 100644 --- a/project.clj +++ b/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 diff --git a/test/metabase/query_processor_test/parameters_test.clj b/test/metabase/query_processor_test/parameters_test.clj index 6c3343f92fc..1388ef5ac4a 100644 --- a/test/metabase/query_processor_test/parameters_test.clj +++ b/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))))))) diff --git a/test/metabase/test.clj b/test/metabase/test.clj index 75b7d2a05b1..c93b561b9fe 100644 --- a/test/metabase/test.clj +++ b/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? diff --git a/test/metabase/test/data/dataset_definitions.clj b/test/metabase/test/data/dataset_definitions.clj index cc6a7e696ef..f7948dc2a10 100644 --- a/test/metabase/test/data/dataset_definitions.clj +++ b/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} -- GitLab