diff --git a/modules/drivers/bigquery/src/metabase/driver/bigquery/query_processor.clj b/modules/drivers/bigquery/src/metabase/driver/bigquery/query_processor.clj
index 78f0ca0730ec58a80609825d926f379a46c9fe4a..51440946dd9ed6e84c6ee3bff33ecfed48ec67c5 100644
--- a/modules/drivers/bigquery/src/metabase/driver/bigquery/query_processor.clj
+++ b/modules/drivers/bigquery/src/metabase/driver/bigquery/query_processor.clj
@@ -9,6 +9,7 @@
[driver :as driver]
[util :as u]]
[metabase.driver.sql :as sql]
+ [metabase.driver.sql.parameters.substitution :as sql.params.substitution]
[metabase.driver.sql.query-processor :as sql.qp]
[metabase.driver.sql.util.unprepare :as unprepare]
[metabase.mbql.util :as mbql.u]
@@ -26,6 +27,7 @@
[schema.core :as s]
[toucan.db :as db])
(:import [java.time LocalDate LocalDateTime LocalTime OffsetDateTime OffsetTime ZonedDateTime]
+ metabase.driver.common.parameters.FieldFilter
metabase.util.honeysql_extensions.Identifier))
(defn- valid-bigquery-identifier?
@@ -490,8 +492,19 @@
[_]
:mysql)
-;; TIMEZONE FIXME — Not working in all cases — see #11222
+;; convert LocalDate to an OffsetDateTime in UTC since BigQuery doesn't handle LocalDates as we'd like
(defmethod sql/->prepared-substitution [:bigquery LocalDate]
- [_ t]
- {:sql-string "?"
- :param-values [(t/offset-date-time t (t/local-time 0) (t/zone-offset 0))]})
+ [driver t]
+ (sql/->prepared-substitution driver (t/offset-date-time t (t/local-time 0) (t/zone-offset 0))))
+
+(defmethod sql.params.substitution/->replacement-snippet-info [:bigquery FieldFilter]
+ [driver {:keys [field], :as field-filter}]
+ (let [field-temporal-type (temporal-type field)
+ parent-method (get-method sql.params.substitution/->replacement-snippet-info [:sql FieldFilter])
+ result (parent-method driver field-filter)]
+ (cond-> result
+ field-temporal-type (update :prepared-statement-args (fn [args]
+ (for [arg args]
+ (if (instance? java.time.temporal.Temporal arg)
+ (->temporal-type field-temporal-type arg)
+ arg)))))))
diff --git a/modules/drivers/bigquery/test/metabase/driver/bigquery/query_processor_test.clj b/modules/drivers/bigquery/test/metabase/driver/bigquery/query_processor_test.clj
index caeb7bf28197dfae66e1a15a0822cef4a08ee0c8..0d4715288a75b847e9460cf8e82c90212ef370f9 100644
--- a/modules/drivers/bigquery/test/metabase/driver/bigquery/query_processor_test.clj
+++ b/modules/drivers/bigquery/test/metabase/driver/bigquery/query_processor_test.clj
@@ -7,12 +7,14 @@
[models :refer [Database Field]]
[query-processor :as qp]
[query-processor-test :as qp.test]
+ [sync :as sync]
[test :as mt]
[util :as u]]
[metabase.driver.bigquery :as bigquery]
[metabase.driver.bigquery.query-processor :as bigquery.qp]
[metabase.driver.sql.query-processor :as sql.qp]
[metabase.query-processor.store :as qp.store]
+ [metabase.test.data.bigquery :as bigquery.tx]
[metabase.test.util.timezone :as tu.tz]
[metabase.util.honeysql-extensions :as hx]
[toucan.util.test :as tt]))
@@ -362,3 +364,79 @@
[:field-literal "date" :type/Date]
(t/local-date-time "2019-11-11T12:00:00")
(t/local-date-time "2019-11-12T12:00:00")]))))))))))
+
+(deftest timezones-test
+ (mt/test-driver :bigquery
+ (testing "BigQuery does not support report-timezone, so setting it should not affect results"
+ (doseq [timezone ["UTC" "US/Pacific"]]
+ (mt/with-temporary-setting-values [report-timezone timezone]
+ (is (= [[37 "2015-11-19T00:00:00Z"]]
+ (mt/rows
+ (mt/run-mbql-query checkins
+ {:fields [$id $date]
+ :filter [:= $date "2015-11-19"]
+ :order-by [[:asc $id]]})))))))))
+
+(defn- do-with-datetime-timestamp-table [f]
+ (driver/with-driver :bigquery
+ (let [table-name (name (munge (gensym "table_")))]
+ (mt/with-temp-copy-of-db
+ (try
+ (bigquery.tx/execute!
+ (format "CREATE TABLE `test_data.%s` ( ts TIMESTAMP, dt DATETIME )" table-name))
+ (bigquery.tx/execute!
+ (format "INSERT INTO `test_data.%s` (ts, dt) VALUES (TIMESTAMP \"2020-01-01 00:00:00 UTC\", DATETIME \"2020-01-01 00:00:00\")"
+ table-name))
+ (sync/sync-database! (mt/db))
+ (f table-name)
+ (finally
+ (bigquery.tx/execute! "DROP TABLE IF EXISTS `test_data.%s`" table-name)))))))
+
+(deftest filter-by-datetime-timestamp-test
+ (mt/test-driver :bigquery
+ ;; there are more tests in the `bigquery.query-processor-test` namespace
+ (testing "Make sure we can filter against different types of BigQuery temporal columns (#11222)"
+ (do-with-datetime-timestamp-table
+ (fn [table-name]
+ (doseq [column [:ts :dt]]
+ (testing (format "Filtering against %s column" column)
+ (doseq [s ["2020-01-01" "2020-01-01T00:00:00"]
+ field [[:field-id (mt/id table-name column)]
+ [:datetime-field [:field-id (mt/id table-name column)] :default]
+ [:datetime-field [:field-id (mt/id table-name column)] :day]]
+ :let [filter-clause [:= field s]]]
+ (testing (format "\nMBQL filter clause = %s" (pr-str filter-clause))
+ (is (= [["2020-01-01T00:00:00Z" "2020-01-01T00:00:00Z"]]
+ (mt/rows
+ (mt/run-mbql-query nil
+ {:source-table (mt/id table-name)
+ :filter filter-clause})))))))))))))
+
+(deftest datetime-parameterized-sql-test
+ (testing "Make sure Field filters against temporal fields generates correctly-typed SQL (#11578)"
+ (mt/test-driver :bigquery
+ (mt/dataset attempted-murders
+ (doseq [field [:datetime
+ :date
+ :datetime_tz]
+ [value-type value] {:date/relative "past30days"
+ :date/range "2019-12-11~2020-01-09"
+ :date/single "2020-01-09"
+ :date/quarter-year "Q1-2020"
+ :date/month-year "2020-01"}]
+ (testing (format "\nField filter with %s Field" field)
+ (testing (format "\nfiltering against %s value '%s'" value-type value)
+ (is (= [[0]]
+ (mt/rows
+ (qp/process-query
+ {:database (mt/id)
+ :type :native
+ :native {:query "SELECT count(*) FROM `attempted_murders.attempts` WHERE {{d}}"
+ :template-tags {"d" {:name "d"
+ :display-name "Date"
+ :type :dimension
+ :dimension [:field-id (mt/id :attempts field)]}}}
+ :parameters [{:type value-type
+ :name "d"
+ :target [:dimension [:template-tag "d"]]
+ :value value}]})))))))))))
diff --git a/modules/drivers/bigquery/test/metabase/driver/bigquery_test.clj b/modules/drivers/bigquery/test/metabase/driver/bigquery_test.clj
index 6eb1fdc1d7d8e02ea62692d3197a2101a7d752d9..9ef3b6fd1f80ddc5acc7575a2e8ae44cd924fc04 100644
--- a/modules/drivers/bigquery/test/metabase/driver/bigquery_test.clj
+++ b/modules/drivers/bigquery/test/metabase/driver/bigquery_test.clj
@@ -80,50 +80,3 @@
:query {:source-table (data/id view-name)
:order-by [[:asc (data/id view-name :id)]]}})))
"We should be able to run queries against the view (#3414)"))))
-
-(deftest timezones-test
- (datasets/test-driver :bigquery
- (testing "BigQuery does not support report-timezone, so setting it should not affect results"
- (doseq [timezone ["UTC" "US/Pacific"]]
- (tu/with-temporary-setting-values [report-timezone timezone]
- (is (= [[37 "2015-11-19T00:00:00Z"]]
- (qp.test/rows
- (data/run-mbql-query checkins
- {:fields [$id $date]
- :filter [:= $date "2015-11-19"]
- :order-by [[:asc $id]]})))))))))
-
-(defn- do-with-datetime-timestamp-table [f]
- (driver/with-driver :bigquery
- (let [table-name (name (munge (gensym "table_")))]
- (data/with-temp-copy-of-db
- (try
- (bigquery.tx/execute!
- (format "CREATE TABLE `test_data.%s` ( ts TIMESTAMP, dt DATETIME )" table-name))
- (bigquery.tx/execute!
- (format "INSERT INTO `test_data.%s` (ts, dt) VALUES (TIMESTAMP \"2020-01-01 00:00:00 UTC\", DATETIME \"2020-01-01 00:00:00\")"
- table-name))
- (sync/sync-database! (data/db))
- (f table-name)
- (finally
- (bigquery.tx/execute! "DROP TABLE IF EXISTS `test_data.%s`" table-name)))))))
-
-(deftest filter-by-datetime-timestamp-test
- (datasets/test-driver :bigquery
- ;; there are more tests in the `bigquery.query-processor-test` namespace
- (testing "Make sure we can filter against different types of BigQuery temporal columns (#11222)"
- (do-with-datetime-timestamp-table
- (fn [table-name]
- (doseq [column [:ts :dt]]
- (testing (format "Filtering against %s column" column)
- (doseq [s ["2020-01-01" "2020-01-01T00:00:00"]
- field [[:field-id (data/id table-name column)]
- [:datetime-field [:field-id (data/id table-name column)] :default]
- [:datetime-field [:field-id (data/id table-name column)] :day]]
- :let [filter-clause [:= field s]]]
- (testing (format "\nMBQL filter clause = %s" (pr-str filter-clause))
- (is (= [["2020-01-01T00:00:00Z" "2020-01-01T00:00:00Z"]]
- (qp.test/rows
- (data/run-mbql-query nil
- {:source-table (data/id table-name)
- :filter filter-clause})))))))))))))
diff --git a/modules/drivers/bigquery/test/metabase/test/data/bigquery.clj b/modules/drivers/bigquery/test/metabase/test/data/bigquery.clj
index 34bc9b3ab04b0a035129a4c947a021119d4c98f3..4909ebc4d7238c4bc5ac7fa1cdf0755a1f8438ce 100644
--- a/modules/drivers/bigquery/test/metabase/test/data/bigquery.clj
+++ b/modules/drivers/bigquery/test/metabase/test/data/bigquery.clj
@@ -20,9 +20,8 @@
(:import com.google.api.client.util.DateTime
com.google.api.services.bigquery.Bigquery
[com.google.api.services.bigquery.model Dataset DatasetReference QueryRequest QueryResponse
- Table TableDataInsertAllRequest TableDataInsertAllRequest$Rows TableFieldSchema TableReference TableRow
- TableSchema]
- java.sql.Time))
+ Table TableDataInsertAllRequest TableDataInsertAllRequest$Rows TableDataInsertAllResponse TableFieldSchema
+ TableReference TableRow TableSchema]))
(sql.tx/add-test-extensions! :bigquery)
@@ -93,12 +92,16 @@
response))))
(def ^:private valid-field-types
- #{:BOOLEAN :FLOAT :INTEGER :RECORD :STRING :TIMESTAMP :TIME})
+ #{:BOOLEAN :DATE :DATETIME :FLOAT :INTEGER :NUMERIC :RECORD :STRING :TIME :TIMESTAMP})
+
+;; Fields must contain only letters, numbers, and underscores, start with a letter or underscore, and be at most 128
+;; characters long.
+(def ^:private ValidFieldName #"^[A-Za-z_]\w{0,127}$")
(s/defn ^:private create-table!
[dataset-id :- su/NonBlankString
table-id :- su/NonBlankString,
- field-name->type :- {su/KeywordOrString (apply s/enum valid-field-types)}]
+ field-name->type :- {ValidFieldName (apply s/enum valid-field-types)}]
(google/execute
(.insert
(.tables (bigquery))
@@ -124,59 +127,88 @@
(doto (QueryRequest.)
(.setQuery (format "SELECT COUNT(*) FROM [%s.%s]" dataset-id table-id)))))))))
-;; This is a dirty HACK
-(defn- ^DateTime timestamp-honeysql-form->GoogleDateTime
- "Convert the HoneySQL form we normally use to wrap a `Timestamp` to a Google `DateTime`."
- [{[{s :literal}] :args}]
- {:pre [(string? s) (seq s)]}
- (DateTime. (t/to-java-date (u.date/parse (str/replace s #"'" "")))))
+(defprotocol ^:private Insertable
+ (^:private ->insertable [this]
+ "Convert a value to an appropriate Google type when inserting a new row."))
+
+(extend-protocol Insertable
+ nil
+ (->insertable [_] nil)
+
+ Object
+ (->insertable [this] this)
+
+ java.time.temporal.Temporal
+ (->insertable [t] (str t))
+ java.time.ZonedDateTime
+ (->insertable [t] (->insertable (t/offset-date-time t)))
+
+ ;; normalize to UTC, since BigQuery doesn't support TIME WITH TIME ZONE
+ java.time.OffsetTime
+ (->insertable [t] (->insertable (t/local-time (t/with-offset-same-instant t (t/zone-offset 0)))))
+
+ ;; Convert the HoneySQL form we normally use to wrap a `Timestamp` to a plain literal string
+ honeysql.types.SqlCall
+ (->insertable [{[{s :literal}] :args}]
+ (->insertable (u.date/parse (str/replace s #"'" "")))))
(defn- insert-data! [^String dataset-id, ^String table-id, row-maps]
{:pre [(seq dataset-id) (seq table-id) (sequential? row-maps) (seq row-maps) (every? map? row-maps)]}
- (google/execute
- (.insertAll
- (.tabledata (bigquery)) (project-id) dataset-id table-id
- (doto (TableDataInsertAllRequest.)
- (.setRows (for [row-map row-maps]
- (let [data (TableRow.)]
- (doseq [[k v] row-map
- :let [v (cond
- (instance? honeysql.types.SqlCall v)
- (timestamp-honeysql-form->GoogleDateTime v)
- :else v)]]
- (.set data (name k) v))
- (doto (TableDataInsertAllRequest$Rows.)
- (.setJson data))))))))
- ;; Wait up to 30 seconds for all the rows to be loaded and become available by BigQuery
- (let [expected-row-count (count row-maps)]
- (loop [seconds-to-wait-for-load 30]
- (let [actual-row-count (table-row-count dataset-id table-id)]
- (cond
- (= expected-row-count actual-row-count)
- :ok
-
- (> seconds-to-wait-for-load 0)
- (do (Thread/sleep 1000)
- (recur (dec seconds-to-wait-for-load)))
-
- :else
- (throw (Exception. (format "Failed to load table data for %s.%s: expected %d rows, loaded %d"
- dataset-id table-id expected-row-count actual-row-count))))))))
-
-
-(def ^:private base-type->bigquery-type
- {:type/BigInteger :INTEGER
- :type/Boolean :BOOLEAN
- :type/Date :TIMESTAMP
- :type/DateTime :TIMESTAMP
- :type/DateTimeWithTZ :TIMESTAMP ; is this correct ???
- :type/Decimal :NUMERIC
- :type/Dictionary :RECORD
- :type/Float :FLOAT
- :type/Integer :INTEGER
- :type/Text :STRING
- :type/Time :TIME})
+ (let [rows
+ (for [row-map row-maps]
+ (let [data (TableRow.)]
+ (doseq [[k v] row-map
+ :let [v (->insertable v)]]
+ (.set data (name k) v))
+ (doto (TableDataInsertAllRequest$Rows.)
+ (.setJson data))))
+
+ request
+ (.insertAll
+ (.tabledata (bigquery)) (project-id) dataset-id table-id
+ (doto (TableDataInsertAllRequest.)
+ (.setRows rows)))
+
+ ^TableDataInsertAllResponse response (google/execute request)]
+ (when (seq (.getInsertErrors response))
+ (println "Error inserting rows:" (u/pprint-to-str (seq (.getInsertErrors response))))
+ (throw (ex-info "Error inserting rows"
+ {:errors (seq (.getInsertErrors response))
+ :metabase.util/no-auto-retry? true
+ :rows row-maps})))
+ ;; Wait up to 30 seconds for all the rows to be loaded and become available by BigQuery
+ (let [expected-row-count (count row-maps)]
+ (loop [seconds-to-wait-for-load 30]
+ (let [actual-row-count (table-row-count dataset-id table-id)]
+ (cond
+ (= expected-row-count actual-row-count)
+ :ok
+
+ (> seconds-to-wait-for-load 0)
+ (do (Thread/sleep 1000)
+ (recur (dec seconds-to-wait-for-load)))
+
+ :else
+ (let [error-message (format "Failed to load table data for %s.%s: expected %d rows, loaded %d"
+ dataset-id table-id expected-row-count actual-row-count)]
+ (println (u/format-color 'red error-message))
+ (throw (ex-info error-message {:metabase.util/no-auto-retry? true, :response response})))))))))
+
+(defn- base-type->bigquery-type [base-type]
+ (let [types {:type/BigInteger :INTEGER
+ :type/Boolean :BOOLEAN
+ :type/Date :DATE
+ :type/DateTime :DATETIME
+ :type/DateTimeWithTZ :TIMESTAMP
+ :type/Decimal :NUMERIC
+ :type/Dictionary :RECORD
+ :type/Float :FLOAT
+ :type/Integer :INTEGER
+ :type/Text :STRING
+ :type/Time :TIME}]
+ (or (get types base-type)
+ (some base-type->bigquery-type (parents base-type)))))
(defn- fielddefs->field-name->base-type
"Convert `field-definitions` to a format appropriate for passing to `create-table!`."
@@ -185,28 +217,17 @@
{"id" :INTEGER}
(for [{:keys [field-name base-type]} field-definitions]
{field-name (or (base-type->bigquery-type base-type)
- (println (u/format-color 'red "Don't know what BigQuery type to use for base type: %s" base-type))
- (throw (Exception. (format "Don't know what BigQuery type to use for base type: %s" base-type))))})))
+ (let [message (format "Don't know what BigQuery type to use for base type: %s" base-type)]
+ (println (u/format-color 'red message))
+ (throw (ex-info message {:metabase.util/no-auto-retry? true}))))})))
(defn- tabledef->prepared-rows
"Convert `table-definition` to a format approprate for passing to `insert-data!`."
[{:keys [field-definitions rows]}]
{:pre [(every? map? field-definitions) (sequential? rows) (seq rows)]}
(let [field-names (map :field-name field-definitions)]
- (for [[i row] (m/indexed rows)
- :let [vs (for [v row]
- (u/prog1 (cond
- (instance? Time v)
- (u.date/format-sql (t/local-time v))
-
- (instance? java.util.Date v)
- ;; convert to Google version of DateTime, otherwise it doesn't work (!)
- (DateTime. ^java.util.Date v)
-
- :else v)
- ;; make sure v is non-nil
- (assert (not (nil? <>)))))]]
- (assoc (zipmap field-names vs)
+ (for [[i row] (m/indexed rows)]
+ (assoc (zipmap field-names row)
:id (inc i)))))
(defn- load-tabledef! [dataset-name {:keys [table-name field-definitions], :as tabledef}]
@@ -238,7 +259,7 @@
(let [database-name (normalize-name database-name)]
(when-not (contains? @existing-datasets database-name)
(try
- (u/auto-retry 10
+ (u/auto-retry 2
;; if the dataset failed to load successfully last time around, destroy whatever was loaded so we start
;; again from a blank slate
(u/ignore-exceptions
@@ -253,8 +274,8 @@
;; if creating the dataset ultimately fails to complete, then delete it so it will hopefully work next time
;; around
(catch Throwable e
+ (println (u/format-color 'red "Failed to load BigQuery dataset '%s'." database-name))
(u/ignore-exceptions
- (println (u/format-color 'red "Failed to load BigQuery dataset '%s'." database-name))
(destroy-dataset! database-name))
(throw e))))))
diff --git a/modules/drivers/mongo/src/metabase/driver/mongo/parameters.clj b/modules/drivers/mongo/src/metabase/driver/mongo/parameters.clj
index a1e5e1a26aa7fb24bb03b5a7c69d297e74337d27..0461c98e78fa6bacd8b4d69bfdde7a51b9b1c9a1 100644
--- a/modules/drivers/mongo/src/metabase/driver/mongo/parameters.clj
+++ b/modules/drivers/mongo/src/metabase/driver/mongo/parameters.clj
@@ -34,7 +34,7 @@
(defn- field->name [field]
(pr-str (mongo.qp/field->name field ".")))
-(defn- substitute-one-field-filter-relative-date [{field :field, {param-type :type, value :value} :value}]
+(defn- substitute-one-field-filter-date-range [{field :field, {param-type :type, value :value} :value}]
(let [{:keys [start end]} (date-params/date-string->range value)
start-condition (when start
(format "{%s: {$gte: %s}}" (field->name field) (param-value->str (u.date/parse start))))
@@ -49,8 +49,8 @@
;; 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/relative-date-param-type? param-type)
- (substitute-one-field-filter-relative-date field-filter)
+ (if (date-params/date-range-type? param-type)
+ (substitute-one-field-filter-date-range field-filter)
(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/oracle/test/metabase/driver/oracle_test.clj b/modules/drivers/oracle/test/metabase/driver/oracle_test.clj
index 6acbea284b963687d05360cc468859e8acae8c4a..db4957ee865952edd3b24ffd0a32529ddc3818ce 100644
--- a/modules/drivers/oracle/test/metabase/driver/oracle_test.clj
+++ b/modules/drivers/oracle/test/metabase/driver/oracle_test.clj
@@ -129,14 +129,15 @@
(execute! "CREATE TABLE \"%s\".\"messages\" (\"id\" %s, \"message\" CLOB)" username pk-type)
(execute! "INSERT INTO \"%s\".\"messages\" (\"id\", \"message\") VALUES (1, 'Hello')" username)
(execute! "INSERT INTO \"%s\".\"messages\" (\"id\", \"message\") VALUES (2, NULL)" username)
- (tt/with-temp* [Table [table {:schema username, :name "messages", :db_id (data/id)}]
- Field [_ {:table_id (u/get-id table), :name "id", :base_type "type/Integer"}]
- Field [_ {:table_id (u/get-id table), :name "message", :base_type "type/Text"}]]
+ (tt/with-temp* [Table [table {:schema username, :name "messages", :db_id (data/id)}]
+ Field [id-field {:table_id (u/get-id table), :name "id", :base_type "type/Integer"}]
+ Field [_ {:table_id (u/get-id table), :name "message", :base_type "type/Text"}]]
(qp.test/rows
(qp/process-query
- {:database (data/id)
- :type :query
- :query {:source-table (u/get-id table)}}))))))
+ {:database (data/id)
+ :type :query
+ :query {:source-table (u/get-id table)
+ :order-by [[:asc [:field-id (u/get-id id-field)]]]}}))))))
;; let's make sure we're actually attempting to generate the correctl HoneySQL for joins and source queries so we
;; don't sit around scratching our heads wondering why the queries themselves aren't working
diff --git a/src/metabase/driver/common/parameters/dates.clj b/src/metabase/driver/common/parameters/dates.clj
index fbf3069a679e95524462b6299daed418fe893ef7..230c1101da108cad0b2869782701e5c1d83d9a80 100644
--- a/src/metabase/driver/common/parameters/dates.clj
+++ b/src/metabase/driver/common/parameters/dates.clj
@@ -4,8 +4,10 @@
[medley.core :as m]
[metabase.mbql.schema :as mbql.s]
[metabase.models.params :as params]
+ [metabase.query-processor.error-type :as error-type]
[metabase.util
[date-2 :as u.date]
+ [i18n :refer [tru]]
[schema :as su]]
[schema.core :as s]))
@@ -15,9 +17,11 @@
(or (= param-type :date)
(= "date" (namespace param-type))))
-(defn relative-date-param-type?
- "Is param type a relative `:date` param (e.g., not an absolute moment in time?)"
+(defn date-range-type?
+ "Does date `param-type` represent a range of dates, rather than a single absolute date? (The value may be relative,
+ such as `past30days`, or absolute, such as `2020-01`.)"
[param-type]
+ ;; TODO —
(#{:date/range :date/month-year :date/quarter-year :date/relative :date/all-options} param-type))
;; Both in MBQL and SQL parameter substitution a field value is compared to a date range, either relative or absolute.
@@ -218,7 +222,11 @@
;; 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 u.date/format)))))
+ (m/map-vals u.date/format))
+ ;; if both of the decoders above fail, then the date string is invalid
+ (throw (ex-info (tru "Don''t know how to parse date param ''{0}'' — invalid format" date-string)
+ {:param date-string
+ :type error-type/invalid-parameter})))))
;; 2-arg version is for legacy compatibility only; no longer needed
([date-string _]
diff --git a/src/metabase/driver/common/parameters/values.clj b/src/metabase/driver/common/parameters/values.clj
index 0fac0e52f19d6023fbb27e1deb7953f98e9ed9d0..e346aac594881887a4901207df18705c5aa23454 100644
--- a/src/metabase/driver/common/parameters/values.clj
+++ b/src/metabase/driver/common/parameters/values.clj
@@ -37,14 +37,16 @@
;; FieldFilter as well.
(def ^:private TagParam
"Schema for a tag parameter declaration, passed in as part of the `:template-tags` list."
- {(s/optional-key :id) su/NonBlankString ; this is used internally by the frontend
- :name su/NonBlankString
- :display-name su/NonBlankString
- :type ParamType
- (s/optional-key :dimension) [s/Any]
- (s/optional-key :widget-type) s/Keyword ; type of the [default] value if `:type` itself is `dimension`
- (s/optional-key :required) s/Bool
- (s/optional-key :default) s/Any})
+ (s/named
+ {(s/optional-key :id) su/NonBlankString ; this is used internally by the frontend
+ :name su/NonBlankString
+ :display-name su/NonBlankString
+ :type ParamType
+ (s/optional-key :dimension) [s/Any]
+ (s/optional-key :widget-type) s/Keyword ; type of the [default] value if `:type` itself is `dimension`
+ (s/optional-key :required) s/Bool
+ (s/optional-key :default) s/Any}
+ "valid template-tags tag"))
(def ^:private ParsedParamValue
"Schema for valid param value(s). Params can have one or more values."
@@ -93,10 +95,10 @@
(when-let [field-filter (:dimension tag)]
(i/map->FieldFilter
;; TODO - shouldn't this use the QP Store?
- {:field (or (db/select-one [Field :name :parent_id :table_id :base_type]
- :id (field-filter->field-id field-filter))
- (throw (Exception. (tru "Can''t find field with ID: {0}"
- (field-filter->field-id field-filter)))))
+ {:field (let [field-id (field-filter->field-id field-filter)]
+ (or (db/select-one [Field :name :parent_id :table_id :base_type] :id field-id)
+ (throw (ex-info (tru "Can''t find field with ID: {0}" field-id)
+ {:field-id field-id, :type qp.error-type/invalid-parameter}))))
:value (if-let [value-info-or-infos (or
;; look in the sequence of params we were passed to see if there's anything
;; that matches
@@ -222,9 +224,16 @@
->
{:checkin_date #t \"2019-09-19T23:30:42.233-07:00\"}"
[{tags :template-tags, params :parameters}]
- (into {} (for [[k tag] tags
- :let [v (value-for-tag tag params)]
- :when v]
- ;; TODO - if V is `nil` *on purpose* this still won't give us a query like `WHERE field = NULL`. That
- ;; kind of query shouldn't be possible from the frontend anyway
- {k v})))
+ (try
+ (into {} (for [[k tag] tags
+ :let [v (value-for-tag tag params)]
+ :when v]
+ ;; TODO - if V is `nil` *on purpose* this still won't give us a query like `WHERE field = NULL`. That
+ ;; kind of query shouldn't be possible from the frontend anyway
+ {k v}))
+ (catch Throwable e
+ (throw (ex-info (.getMessage e)
+ {:type (or (:type (ex-data e)) qp.error-type/invalid-parameter)
+ :tags tags
+ :params params}
+ e)))))
diff --git a/src/metabase/driver/sql/parameters/substitute.clj b/src/metabase/driver/sql/parameters/substitute.clj
index 1168608993a9f28f8b3c97323b4257263f5ec05a..72adfc70c6ba13407211c400319d72d5756305dd 100644
--- a/src/metabase/driver/sql/parameters/substitute.clj
+++ b/src/metabase/driver/sql/parameters/substitute.clj
@@ -1,5 +1,6 @@
(ns metabase.driver.sql.parameters.substitute
(:require [clojure.string :as str]
+ [metabase.driver :as driver]
[metabase.driver.common.parameters :as i]
[metabase.driver.sql.parameters.substitution :as substitution]
[metabase.query-processor.error-type :as error-type]
@@ -10,7 +11,7 @@
;; no-value field filters inside optional clauses are ignored, and eventually emitted entirely
[sql args (conj missing k)]
;; otherwise no values get replaced with `1 = 1` and other values get replaced normally
- (let [{:keys [replacement-snippet prepared-statement-args]} (substitution/->replacement-snippet-info v)]
+ (let [{:keys [replacement-snippet prepared-statement-args]} (substitution/->replacement-snippet-info driver/*driver* v)]
[(str sql replacement-snippet) (concat args prepared-statement-args) missing])))
(defn- substitute-param [param->value [sql args missing] in-optional? {:keys [k]}]
@@ -25,7 +26,7 @@
[sql args (conj missing k)]
:else
- (let [{:keys [replacement-snippet prepared-statement-args]} (substitution/->replacement-snippet-info v)]
+ (let [{:keys [replacement-snippet prepared-statement-args]} (substitution/->replacement-snippet-info driver/*driver* v)]
[(str sql replacement-snippet) (concat args prepared-statement-args) missing])))))
(declare substitute*)
@@ -62,8 +63,16 @@
{\"bird_type\" \"Steller's Jay\"})
;; -> [\"select * from foobars where bird_type = ?\" [\"Steller's Jay\"]]"
[parsed-query param->value]
- (let [[sql args missing] (substitute* param->value parsed-query false)]
+ (let [[sql args missing] (try
+ (substitute* param->value parsed-query false)
+ (catch Throwable e
+ (throw (ex-info (tru "Unable to substitute parameters")
+ {:type (or (:type (ex-data e)) error-type/qp)
+ :params param->value
+ :parsed-query parsed-query}
+ e))))]
(when (seq missing)
(throw (ex-info (tru "Cannot run the query: missing required parameters: {0}" (set missing))
- {:type error-type/invalid-query})))
+ {:type error-type/missing-required-parameter
+ :missing missing})))
[(str/trim sql) args]))
diff --git a/src/metabase/driver/sql/parameters/substitution.clj b/src/metabase/driver/sql/parameters/substitution.clj
index 0bd670d3dce74ea380f285b5f5383bb4dd866a0e..cdc59b5cc0317aa551d270989ad0de803a6ddb7a 100644
--- a/src/metabase/driver/sql/parameters/substitution.clj
+++ b/src/metabase/driver/sql/parameters/substitution.clj
@@ -12,7 +12,9 @@
[metabase.driver.common.parameters :as i]
[metabase.driver.common.parameters.dates :as date-params]
[metabase.driver.sql.query-processor :as sql.qp]
- [metabase.query-processor.timezone :as qp.timezone]
+ [metabase.query-processor
+ [error-type :as qp.error-type]
+ [timezone :as qp.timezone]]
[metabase.util
[date-2 :as u.date]
[i18n :refer [tru]]
@@ -24,7 +26,6 @@
java.util.UUID
[metabase.driver.common.parameters CommaSeparatedNumbers Date DateRange FieldFilter MultipleValues]))
-
;;; ------------------------------------ ->prepared-substitution & default impls -------------------------------------
(defmulti ->prepared-substitution
@@ -96,52 +97,53 @@
"Return information about how `value` should be converted to SQL, as a map with keys `:replacement-snippet` and
`:prepared-statement-args`.
- (->replacement-snippet-info \"ABC\") -> {:replacement-snippet \"?\", :prepared-statement-args \"ABC\"}"
- {:arglists '([value])}
- class)
+ (->replacement-snippet-info :h2 \"ABC\") -> {:replacement-snippet \"?\", :prepared-statement-args \"ABC\"}"
+ {:arglists '([driver value])}
+ (fn [driver v] [(driver/the-initialized-driver driver) (class v)])
+ :hierarchy #'driver/hierarchy)
(defn- create-replacement-snippet [nil-or-obj]
(let [{:keys [sql-string param-values]} (->prepared-substitution driver/*driver* nil-or-obj)]
{:replacement-snippet sql-string
:prepared-statement-args param-values}))
-(defmethod ->replacement-snippet-info nil
- [this]
+(defmethod ->replacement-snippet-info [:sql nil]
+ [_ this]
(create-replacement-snippet this))
-(defmethod ->replacement-snippet-info Object
- [this]
+(defmethod ->replacement-snippet-info [:sql Object]
+ [_ this]
(create-replacement-snippet (str this)))
-(defmethod ->replacement-snippet-info Number
- [this]
+(defmethod ->replacement-snippet-info [:sql Number]
+ [_ this]
(create-replacement-snippet this))
-(defmethod ->replacement-snippet-info Boolean
- [this]
+(defmethod ->replacement-snippet-info [:sql Boolean]
+ [_ this]
(create-replacement-snippet this))
-(defmethod ->replacement-snippet-info Keyword
- [this]
+(defmethod ->replacement-snippet-info [:sql Keyword]
+ [_ this]
(if (= this i/no-value)
{:replacement-snippet ""}
(create-replacement-snippet this)))
-(defmethod ->replacement-snippet-info SqlCall
- [this]
+(defmethod ->replacement-snippet-info [:sql SqlCall]
+ [_ this]
(create-replacement-snippet this))
-(defmethod ->replacement-snippet-info UUID
- [this]
+(defmethod ->replacement-snippet-info [:sql UUID]
+ [_ this]
{:replacement-snippet (format "CAST('%s' AS uuid)" (str this))})
-(defmethod ->replacement-snippet-info CommaSeparatedNumbers
- [{:keys [numbers]}]
+(defmethod ->replacement-snippet-info [:sql CommaSeparatedNumbers]
+ [_ {:keys [numbers]}]
{:replacement-snippet (str/join ", " numbers)})
-(defmethod ->replacement-snippet-info MultipleValues
- [{:keys [values]}]
- (let [values (map ->replacement-snippet-info values)]
+(defmethod ->replacement-snippet-info [:sql MultipleValues]
+ [driver {:keys [values]}]
+ (let [values (map (partial ->replacement-snippet-info driver) values)]
{:replacement-snippet (str/join ", " (map :replacement-snippet values))
:prepared-statement-args (apply concat (map :prepared-statement-args values))}))
@@ -149,34 +151,36 @@
(condp instance? x
String (u.date/parse x (qp.timezone/report-timezone-id-if-supported))
Temporal x
- (throw (Exception. (tru "Don''t know how to parse {0} {1}" (class x) x)))))
+ (throw (ex-info (tru "Don''t know how to parse {0} {1} as a temporal literal" (class x) (pr-str x))
+ {:type qp.error-type/invalid-parameter
+ :parameter x}))))
-(defmethod ->replacement-snippet-info Date
- [{:keys [s]}]
+(defmethod ->replacement-snippet-info [:sql Date]
+ [driver {:keys [s]}]
(create-replacement-snippet (maybe-parse-temporal-literal s)))
-(defn- prepared-ts-subs [operator date-str]
- (let [{:keys [sql-string param-values]} (->prepared-substitution driver/*driver* (maybe-parse-temporal-literal date-str))]
+(defn- prepared-ts-subs [driver operator date-str]
+ (let [{:keys [sql-string param-values]} (->prepared-substitution driver (maybe-parse-temporal-literal date-str))]
{:replacement-snippet (str operator " " sql-string)
:prepared-statement-args param-values}))
-(defmethod ->replacement-snippet-info DateRange
- [{:keys [start end]}]
+(defmethod ->replacement-snippet-info [:sql DateRange]
+ [driver {:keys [start end]}]
(cond
(= start end)
- (prepared-ts-subs \= start)
+ (prepared-ts-subs driver \= start)
(nil? start)
- (prepared-ts-subs \< end)
+ (prepared-ts-subs driver \< end)
(nil? end)
- (prepared-ts-subs \> start)
+ (prepared-ts-subs driver \> start)
:else
;; TIMEZONE FIXME - this is WRONG WRONG WRONG because date ranges should be inclusive for start and *exclusive*
;; for end
(let [[start end] (map (fn [s]
- (->prepared-substitution driver/*driver* (maybe-parse-temporal-literal s)))
+ (->prepared-substitution driver (maybe-parse-temporal-literal s)))
[start end])]
{:replacement-snippet (format "BETWEEN %s AND %s" (:sql-string start) (:sql-string end))
:prepared-statement-args (concat (:param-values start) (:param-values end))})))
@@ -191,40 +195,39 @@
:prepared-statement-args (reduce concat (map :prepared-statement-args replacement-snippet-maps))})
;; for relative dates convert the param to a `DateRange` record type and call `->replacement-snippet-info` on it
-(s/defn ^:private relative-date-field-filter->replacement-snippet-info :- ParamSnippetInfo
- [value]
- (-> (date-params/date-string->range value (qp.timezone/results-timezone-id))
- i/map->DateRange
- ->replacement-snippet-info))
+(s/defn ^:private date-range-field-filter->replacement-snippet-info :- ParamSnippetInfo
+ [driver value]
+ (->> (date-params/date-string->range value (qp.timezone/results-timezone-id))
+ i/map->DateRange
+ (->replacement-snippet-info driver)))
(s/defn ^:private field-filter->equals-clause-sql :- ParamSnippetInfo
- [value]
- (-> (->replacement-snippet-info value)
+ [driver value]
+ (-> (->replacement-snippet-info driver value)
(update :replacement-snippet (partial str "= "))))
(s/defn ^:private field-filter-multiple-values->in-clause-sql :- ParamSnippetInfo
- [values]
- (-> (i/map->MultipleValues {:values values})
- ->replacement-snippet-info
+ [driver values]
+ (-> (->replacement-snippet-info driver (i/map->MultipleValues {:values values}))
(update :replacement-snippet (partial format "IN (%s)"))))
(s/defn ^:private field-filter->replacement-snippet-info :- ParamSnippetInfo
"Return `[replacement-snippet & prepared-statement-args]` appropriate for a field filter parameter."
- [{param-type :type, value :value} :- i/ParamValue]
+ [driver {param-type :type, value :value} :- i/ParamValue]
(cond
- ;; convert relative dates to approprate date range representations
- (date-params/relative-date-param-type? param-type) (relative-date-field-filter->replacement-snippet-info value)
+ ;; convert date ranges to DateRange record types
+ (date-params/date-range-type? param-type) (date-range-field-filter->replacement-snippet-info driver value)
;; convert all other dates to `= <date>`
- (date-params/date-type? param-type) (field-filter->equals-clause-sql (i/map->Date {:s value}))
+ (date-params/date-type? param-type) (field-filter->equals-clause-sql driver (i/map->Date {:s value}))
;; for sequences of multiple values we want to generate an `IN (...)` clause
- (sequential? value) (field-filter-multiple-values->in-clause-sql value)
+ (sequential? value) (field-filter-multiple-values->in-clause-sql driver value)
;; convert everything else to `= <value>`
- :else (field-filter->equals-clause-sql value)))
+ :else (field-filter->equals-clause-sql driver value)))
(s/defn ^:private honeysql->replacement-snippet-info :- ParamSnippetInfo
- "Convert `x` to a replacement snippet info map by passing it to HoneySQL's `format` function."
- [x]
- (let [[snippet & args] (hsql/format x, :quoting (sql.qp/quote-style driver/*driver*), :allow-dashed-names? true)]
+ "Convert `hsql-form` to a replacement snippet info map by passing it to HoneySQL's `format` function."
+ [driver hsql-form]
+ (let [[snippet & args] (hsql/format hsql-form, :quoting (sql.qp/quote-style driver), :allow-dashed-names? true)]
{:replacement-snippet snippet
:prepared-statement-args args}))
@@ -232,16 +235,17 @@
"Return an approprate snippet to represent this `field` in SQL given its param type.
For non-date Fields, this is just a quoted identifier; for dates, the SQL includes appropriately bucketing based on
the `param-type`."
- [field param-type]
+ [driver field param-type]
(:replacement-snippet
(honeysql->replacement-snippet-info
- (let [identifier (sql.qp/->honeysql driver/*driver* (sql.qp/field->identifier driver/*driver* field))]
+ driver
+ (let [identifier (sql.qp/->honeysql driver (sql.qp/field->identifier driver field))]
(if (date-params/date-type? param-type)
- (sql.qp/date driver/*driver* :day identifier)
+ (sql.qp/date driver :day identifier)
identifier)))))
-(defmethod ->replacement-snippet-info FieldFilter
- [{:keys [field value], :as field-filter}]
+(defmethod ->replacement-snippet-info [:sql FieldFilter]
+ [driver {:keys [field value], :as field-filter}]
(cond
;; otherwise if the value isn't present just put in something that will always be true, such as `1` (e.g. `WHERE 1
;; = 1`). This is only used for field filters outside of optional clauses
@@ -251,9 +255,9 @@
;; FieldFilter, not in the sense that we have a single map with multiple values for `:value`.)
(vector? value)
(combine-replacement-snippet-maps (for [v value]
- (->replacement-snippet-info (assoc field-filter :value v))))
+ (->replacement-snippet-info driver (assoc field-filter :value v))))
;; otherwise convert single value to SQL.
;; Convert the value to a replacement snippet info map and then tack on the field identifier to the front
:else
- (update (field-filter->replacement-snippet-info value)
- :replacement-snippet (partial str (field->identifier field (:type value)) " "))))
+ (update (field-filter->replacement-snippet-info driver value)
+ :replacement-snippet (partial str (field->identifier driver field (:type value)) " "))))
diff --git a/src/metabase/driver/util.clj b/src/metabase/driver/util.clj
index 0a4665f0d5cae0c8fc44a76d2c9750804ef024a9..b312081741a89e7d61a8dbc4e834a7047149ea05 100644
--- a/src/metabase/driver/util.clj
+++ b/src/metabase/driver/util.clj
@@ -115,8 +115,12 @@
"Return info about all currently available drivers, including their connection properties fields and supported
features."
[]
- (into {} (for [driver (available-drivers)]
+ (into {} (for [driver (available-drivers)
+ :let [props (try
+ (driver/connection-properties driver)
+ (catch Throwable e
+ (log/error e (trs "Unable to determine connection properties for driver {0}" driver))))]
+ :when props]
;; TODO - maybe we should rename `details-fields` -> `connection-properties` on the FE as well?
- [driver {:details-fields (driver/connection-properties driver)
- :driver-name (driver/display-name driver)
- #_:features #_(features driver)}])))
+ [driver {:details-fields props
+ :driver-name (driver/display-name driver)}])))
diff --git a/src/metabase/query_processor/error_type.clj b/src/metabase/query_processor/error_type.clj
index a37e978ca310536092fd5cf2d01d97aa16c964d5..153c4363b4ca9eb065100ebc129bc0a6d255b194 100644
--- a/src/metabase/query_processor/error_type.clj
+++ b/src/metabase/query_processor/error_type.clj
@@ -1,6 +1,9 @@
(ns metabase.query-processor.error-type
"A hierarchy of all QP error types. Ideally all QP exceptions should be `ex-data` maps with an `:type` key whose value
- is one of the types here. If you see an Exception in QP code that doesn't return an `:type`, add it!")
+ is one of the types here. If you see an Exception in QP code that doesn't return an `:type`, add it!
+
+ (throw (ex-info (tru \"Don''t know how to parse {0} {1}\" (class x) x)
+ {:type qp.error-type/invalid-parameter}))")
(def ^:private hierarchy
(make-hierarchy))
@@ -56,6 +59,11 @@
:parent invalid-query
:show-in-embeds? true)
+(deferror invalid-parameter
+ "The query is parameterized, and a supplied parameter has an invalid value."
+ :parent invalid-query
+ :show-in-embeds? true)
+
;;;; ### Server-Side Errors
(deferror server
diff --git a/src/metabase/query_processor/middleware/catch_exceptions.clj b/src/metabase/query_processor/middleware/catch_exceptions.clj
index 35a6cd3c64e4b951e1b552a90d3afc62dadeccd4..90f75aa309c846b04e30da0d06f2da4c13b45730 100644
--- a/src/metabase/query_processor/middleware/catch_exceptions.clj
+++ b/src/metabase/query_processor/middleware/catch_exceptions.clj
@@ -81,7 +81,8 @@
(defmethod format-exception ExceptionInfo
[query e]
- (let [{error :error, error-type :type, :as data} (ex-data e)]
+ (let [{error :error, :as data} (ex-data e)
+ {error-type :type} (u/all-ex-data e)]
(merge
((get-method format-exception Throwable) query e)
(when-let [error-msg (and (= error-type :schema.core/error)
diff --git a/src/metabase/util.clj b/src/metabase/util.clj
index a5a84e3df46abbae3b11c0b05f54b46242e270c3..71c43f746e6c9a348d2c9352e21c1dba41ef0560 100644
--- a/src/metabase/util.clj
+++ b/src/metabase/util.clj
@@ -386,6 +386,24 @@
(^String [s max-length]
(str/join (take max-length (slugify s)))))
+(defn all-ex-data
+ "Like `ex-data`, but merges `ex-data` from causes. If duplicate keys exist, the keys from the highest level are
+ preferred.
+
+ (def e (ex-info \"A\" {:a true, :both \"a\"} (ex-info \"B\" {:b true, :both \"A\"})))
+
+ (ex-data e)
+ ;; -> {:a true, :both \"a\"}
+
+ (u/all-ex-data e)
+ ;; -> {:a true, :b true, :both \"a\"}"
+ [e]
+ (reduce
+ (fn [data e]
+ (merge (ex-data e) data))
+ nil
+ (take-while some? (iterate #(.getCause ^Throwable %) e))))
+
(defn do-with-auto-retries
"Execute `f`, a function that takes no arguments, and return the results.
If `f` fails with an exception, retry `f` up to `num-retries` times until it succeeds.
@@ -395,14 +413,20 @@
[num-retries f]
(if (<= num-retries 0)
(f)
- (try (f)
- (catch Throwable e
- (log/warn (format-color 'red "auto-retry %s: %s" f (.getMessage e)))
- (do-with-auto-retries (dec num-retries) f)))))
+ (try
+ (f)
+ (catch Throwable e
+ (when (::no-auto-retry? (all-ex-data e))
+ (throw e))
+ (log/warn (format-color 'red "auto-retry %s: %s" f (.getMessage e)))
+ (do-with-auto-retries (dec num-retries) f)))))
(defmacro auto-retry
- "Execute `body` and return the results.
- If `body` fails with an exception, retry execution up to `num-retries` times until it succeeds."
+ "Execute `body` and return the results. If `body` fails with an exception, retry execution up to `num-retries` times
+ until it succeeds.
+
+ You can disable auto-retries for a specific ExceptionInfo by including `{:metabase.util/no-auto-retry? true}` in its
+ data (or the data of one of its causes.)"
{:style/indent 1}
[num-retries & body]
`(do-with-auto-retries ~num-retries
diff --git a/test/metabase/api/public_test.clj b/test/metabase/api/public_test.clj
index 4c4bfd7d162bfddab871da31bd55d9ede29f929b..6bf500decdc5fcf1be26fffc3bdae54543c3823b 100644
--- a/test/metabase/api/public_test.clj
+++ b/test/metabase/api/public_test.clj
@@ -1,9 +1,11 @@
(ns metabase.api.public-test
"Tests for `api/public/` (public links) endpoints."
(:require [cheshire.core :as json]
- [clojure.string :as str]
+ [clojure
+ [string :as str]
+ [test :refer :all]]
[dk.ative.docjure.spreadsheet :as spreadsheet]
- [expectations :refer :all]
+ [expectations :refer [expect]]
[metabase
[http-client :as http]
[query-processor-test :as qp.test]
@@ -216,15 +218,15 @@
:target [:variable [:template-tag "price"]]
:value 1}]))))))
-;; if you're missing a required param, the error message should get passed thru, rather than the normal generic 'Query
-;; Failed' message that we show for most embedding errors
-(expect
- {:status "failed"
- :error "You'll need to pick a value for 'Price' before this query can run."
- :error_type "missing-required-parameter"}
- (do-with-required-param-card
- (fn [uuid]
- (http/client :get 200 (str "public/card/" uuid "/query")))))
+(deftest missing-required-param-error-message-test
+ (testing (str "If you're missing a required param, the error message should get passed thru, rather than the normal "
+ "generic 'Query Failed' message that we show for most embedding errors")
+ (is (= {:status "failed"
+ :error "You'll need to pick a value for 'Price' before this query can run."
+ :error_type "missing-required-parameter"}
+ (do-with-required-param-card
+ (fn [uuid]
+ (http/client :get 200 (str "public/card/" uuid "/query"))))))))
;; make sure CSV (etc.) downloads take editable params into account (#6407)
diff --git a/test/metabase/driver/sql/parameters/substitute_test.clj b/test/metabase/driver/sql/parameters/substitute_test.clj
index c0b4df0615ff4a9dd8bcf77868aa229856bb43c7..57e1bbae921c4dcdb9b7a7beb8645b9e611f973b 100644
--- a/test/metabase/driver/sql/parameters/substitute_test.clj
+++ b/test/metabase/driver/sql/parameters/substitute_test.clj
@@ -1,6 +1,5 @@
(ns metabase.driver.sql.parameters.substitute-test
(:require [clojure.test :refer :all]
- [expectations :refer [expect]]
[java-time :as t]
[metabase
[driver :as driver]
@@ -25,72 +24,57 @@
(driver/with-driver :h2
(substitute/substitute parsed param->value)))
-;; normal substitution
-(expect
- ["select * from foobars where bird_type = ?" ["Steller's Jay"]]
- (substitute
- ["select * from foobars where bird_type = " (param "bird_type")]
- {"bird_type" "Steller's Jay"}))
-
-;; make sure falsey values are substituted correctly
-;; `nil` should get substituted as `NULL`
-(expect
- ["select * from foobars where bird_type = NULL" []]
- (substitute
- ["select * from foobars where bird_type = " (param "bird_type")]
- {"bird_type" nil}))
-
-;; `false` should get substituted as `false`
-(expect
- ["select * from foobars where bird_type = FALSE" []]
- (substitute
- ["select * from foobars where bird_type = " (param "bird_type")]
- {"bird_type" false}))
-
-;; optional substitution -- param present
-(expect
- ;; should preserve whitespace inside optional params
- ["select * from foobars where bird_type = ?" ["Steller's Jay"]]
- (substitute
- ["select * from foobars " (optional " where bird_type = " (param "bird_type"))]
- {"bird_type" "Steller's Jay"}))
-
-;; optional substitution -- param not present
-(expect
- ["select * from foobars" nil]
- (substitute
- ["select * from foobars " (optional " where bird_type = " (param "bird_type"))]
- {}))
-
-;; optional -- multiple params -- all present
-(expect
- ["select * from foobars where bird_type = ? AND color = ?" ["Steller's Jay" "Blue"]]
- (substitute
- ["select * from foobars " (optional " where bird_type = " (param "bird_type") " AND color = " (param "bird_color"))]
- {"bird_type" "Steller's Jay", "bird_color" "Blue"}))
-
-;; optional -- multiple params -- some present
-(expect
- ["select * from foobars" nil]
- (substitute
- ["select * from foobars " (optional " where bird_type = " (param "bird_type") " AND color = " (param "bird_color"))]
- {"bird_type" "Steller's Jay"}))
-
-;; nested optionals -- all present
-(expect
- ["select * from foobars where bird_type = ? AND color = ?" ["Steller's Jay" "Blue"]]
- (substitute
- ["select * from foobars " (optional " where bird_type = " (param "bird_type")
- (optional " AND color = " (param "bird_color")))]
- {"bird_type" "Steller's Jay", "bird_color" "Blue"}))
-
-;; nested optionals -- some present
-(expect
- ["select * from foobars where bird_type = ?" ["Steller's Jay"]]
- (substitute
- ["select * from foobars " (optional " where bird_type = " (param "bird_type")
- (optional " AND color = " (param "bird_color")))]
- {"bird_type" "Steller's Jay"}))
+(deftest substitute-test
+ (testing "normal substitution"
+ (is (= ["select * from foobars where bird_type = ?" ["Steller's Jay"]]
+ (substitute
+ ["select * from foobars where bird_type = " (param "bird_type")]
+ {"bird_type" "Steller's Jay"}))))
+ (testing "make sure falsey values are substituted correctly"
+ (testing "`nil` should get substituted as `NULL`"
+ (is (= ["select * from foobars where bird_type = NULL" []]
+ (substitute
+ ["select * from foobars where bird_type = " (param "bird_type")]
+ {"bird_type" nil})))))
+ (testing "`false` should get substituted as `false`"
+ (is (= ["select * from foobars where bird_type = FALSE" []]
+ (substitute
+ ["select * from foobars where bird_type = " (param "bird_type")]
+ {"bird_type" false}))))
+ (testing "optional substitution -- param present"
+ (testing "should preserve whitespace inside optional params"
+ (is (= ["select * from foobars where bird_type = ?" ["Steller's Jay"]]
+ (substitute
+ ["select * from foobars " (optional " where bird_type = " (param "bird_type"))]
+ {"bird_type" "Steller's Jay"})))))
+ (testing "optional substitution -- param not present"
+ (is (= ["select * from foobars" nil]
+ (substitute
+ ["select * from foobars " (optional " where bird_type = " (param "bird_type"))]
+ {}))))
+ (testing "optional -- multiple params -- all present"
+ (is (= ["select * from foobars where bird_type = ? AND color = ?" ["Steller's Jay" "Blue"]]
+ (substitute
+ ["select * from foobars " (optional " where bird_type = " (param "bird_type") " AND color = " (param "bird_color"))]
+ {"bird_type" "Steller's Jay", "bird_color" "Blue"}))))
+ (testing "optional -- multiple params -- some present"
+ (is (= ["select * from foobars" nil]
+ (substitute
+ ["select * from foobars " (optional " where bird_type = " (param "bird_type") " AND color = " (param "bird_color"))]
+ {"bird_type" "Steller's Jay"}))))
+ (testing "nested optionals -- all present"
+ (is (= ["select * from foobars where bird_type = ? AND color = ?" ["Steller's Jay" "Blue"]]
+ (substitute
+ ["select * from foobars " (optional " where bird_type = " (param "bird_type")
+ (optional " AND color = " (param "bird_color")))]
+ {"bird_type" "Steller's Jay", "bird_color" "Blue"}))))
+ (testing "nested optionals -- some present"
+ (is (= ["select * from foobars where bird_type = ?" ["Steller's Jay"]]
+ (substitute
+ ["select * from foobars " (optional " where bird_type = " (param "bird_type")
+ (optional " AND color = " (param "bird_color")))]
+ {"bird_type" "Steller's Jay"})))))
+
;;; ------------------------------------------------- Field Filters --------------------------------------------------
@@ -103,35 +87,27 @@
:value {:type :date/single
:value (t/offset-date-time "2019-09-20T19:52:00.000-07:00")}}))
-;; field filter -- non-optional + present
-(expect
- ["select * from checkins where CAST(\"PUBLIC\".\"CHECKINS\".\"DATE\" AS date) = ?"
- [(t/offset-date-time "2019-09-20T19:52:00.000-07:00")]]
- (substitute
- ["select * from checkins where " (param "date")]
- {"date" (date-field-filter-value)}))
-
-;; field filter -- non-optional + missing -- should be replaced with 1 = 1
-(expect
- ["select * from checkins where 1 = 1" []]
- (substitute
- ["select * from checkins where " (param "date")]
- {"date" (assoc (date-field-filter-value) :value i/no-value)}))
-
-;; field filter -- optional + present
-(expect
- ["select * from checkins where CAST(\"PUBLIC\".\"CHECKINS\".\"DATE\" AS date) = ?"
- [(t/offset-date-time "2019-09-20T19:52:00.000-07:00")]]
- (substitute
- ["select * from checkins " (optional "where " (param "date"))]
- {"date" (date-field-filter-value)}))
-
-;; field filter -- optional + missing -- should be omitted entirely
-(expect
- ["select * from checkins" nil]
- (substitute
- ["select * from checkins " (optional "where " (param "date"))]
- {"date" (assoc (date-field-filter-value) :value i/no-value)}))
+(deftest substitute-field-filter-test
+ (testing "field-filters"
+ (testing "non-optional"
+ (let [query ["select * from checkins where " (param "date")]]
+ (testing "param is present"
+ (is (= ["select * from checkins where CAST(\"PUBLIC\".\"CHECKINS\".\"DATE\" AS date) = ?"
+ [(t/offset-date-time "2019-09-20T19:52:00.000-07:00")]]
+ (substitute query {"date" (date-field-filter-value)}))))
+ (testing "param is missing"
+ (is (= ["select * from checkins where 1 = 1" []]
+ (substitute query {"date" (assoc (date-field-filter-value) :value i/no-value)}))
+ "should be replaced with 1 = 1"))))
+ (testing "optional"
+ (let [query ["select * from checkins " (optional "where " (param "date"))]]
+ (testing "param is present"
+ (is (= ["select * from checkins where CAST(\"PUBLIC\".\"CHECKINS\".\"DATE\" AS date) = ?"
+ [#t "2019-09-20T19:52:00.000-07:00"]]
+ (substitute query {"date" (date-field-filter-value)}))))
+ (testing "param is missing — should be omitted entirely"
+ (is (= ["select * from checkins" nil]
+ (substitute query {"date" (assoc (date-field-filter-value) :value i/no-value)}))))))))
;;; ------------------------------------------ simple substitution — {{x}} ------------------------------------------
@@ -659,44 +635,42 @@
:target [:dimension [:template-tag "checkin_date"]]
:value "past5days"}]}))))))
-;; Make sure we can specify the type of a default value for a "Dimension" (Field Filter) by setting the
-;; `:widget-type` key. Check that it works correctly with relative dates...
-(expect
- {:query (str "SELECT count(*) AS \"count\", \"DATE\" "
- "FROM CHECKINS "
- "WHERE CAST(\"PUBLIC\".\"CHECKINS\".\"DATE\" AS date) BETWEEN ? AND ? "
- "GROUP BY \"DATE\"")
- :params [#t "2017-10-31"
- #t "2017-11-04"]}
- (t/with-clock (t/mock-clock #t "2017-11-05T12:00Z" (t/zone-id "UTC"))
- (expand* {:native {:query (str "SELECT count(*) AS \"count\", \"DATE\" "
- "FROM CHECKINS "
- "WHERE {{checkin_date}} "
- "GROUP BY \"DATE\"")
- :template-tags {"checkin_date" {:name "checkin_date"
- :display-name "Checkin Date"
- :type :dimension
- :dimension [:field-id (mt/id :checkins :date)]
- :default "past5days"
- :widget-type :date/all-options}}}})))
-
-;; Check that it works with absolute dates as well
-(expect
- {:query (str "SELECT count(*) AS \"count\", \"DATE\" "
- "FROM CHECKINS "
- "WHERE CAST(\"PUBLIC\".\"CHECKINS\".\"DATE\" AS date) = ? "
- "GROUP BY \"DATE\"")
- :params [#t "2017-11-14"]}
- (expand* {:native {:query (str "SELECT count(*) AS \"count\", \"DATE\" "
- "FROM CHECKINS "
- "WHERE {{checkin_date}} "
- "GROUP BY \"DATE\"")
- :template-tags {"checkin_date" {:name "checkin_date"
- :display-name "Checkin Date"
- :type :dimension
- :dimension [:field-id (mt/id :checkins :date)]
- :default "2017-11-14"
- :widget-type :date/all-options}}}}))
+(deftest field-filter-defaults-test
+ (testing (str "Make sure we can specify the type of a default value for a \"Dimension\" (Field Filter) by setting "
+ "the `:widget-type` key. Check that it works correctly with relative dates...")
+ (is (= {:query (str "SELECT count(*) AS \"count\", \"DATE\" "
+ "FROM CHECKINS "
+ "WHERE CAST(\"PUBLIC\".\"CHECKINS\".\"DATE\" AS date) BETWEEN ? AND ? "
+ "GROUP BY \"DATE\"")
+ :params [#t "2017-10-31"
+ #t "2017-11-04"]}
+ (t/with-clock (t/mock-clock #t "2017-11-05T12:00Z" (t/zone-id "UTC"))
+ (expand* {:native {:query (str "SELECT count(*) AS \"count\", \"DATE\" "
+ "FROM CHECKINS "
+ "WHERE {{checkin_date}} "
+ "GROUP BY \"DATE\"")
+ :template-tags {"checkin_date" {:name "checkin_date"
+ :display-name "Checkin Date"
+ :type :dimension
+ :dimension [:field-id (mt/id :checkins :date)]
+ :default "past5days"
+ :widget-type :date/all-options}}}})))))
+ (testing "Check that it works with absolute dates as well"
+ (is (= {:query (str "SELECT count(*) AS \"count\", \"DATE\" "
+ "FROM CHECKINS "
+ "WHERE CAST(\"PUBLIC\".\"CHECKINS\".\"DATE\" AS date) = ? "
+ "GROUP BY \"DATE\"")
+ :params [#t "2017-11-14"]}
+ (expand* {:native {:query (str "SELECT count(*) AS \"count\", \"DATE\" "
+ "FROM CHECKINS "
+ "WHERE {{checkin_date}} "
+ "GROUP BY \"DATE\"")
+ :template-tags {"checkin_date" {:name "checkin_date"
+ :display-name "Checkin Date"
+ :type :dimension
+ :dimension [:field-id (mt/id :checkins :date)]
+ :default "2017-11-14"
+ :widget-type :date/all-options}}}})))))
(deftest newlines-test
(testing "Make sure queries with newlines are parsed correctly (#11526)"
diff --git a/test/metabase/driver/sql/parameters/substitution_test.clj b/test/metabase/driver/sql/parameters/substitution_test.clj
index 5bb00c2cd83ef1ae132aa50777d92191d4613fc1..8daf043bf3d0ce5be3a9680513c45e47ce293ea6 100644
--- a/test/metabase/driver/sql/parameters/substitution_test.clj
+++ b/test/metabase/driver/sql/parameters/substitution_test.clj
@@ -1,11 +1,11 @@
(ns metabase.driver.sql.parameters.substitution-test
+ "Most of the code in `metabase.driver.sql.parameters.substitution` is actually tested by
+ `metabase.driver.sql.parameters.substitute-test`."
(:require [clojure.test :refer :all]
- [metabase.driver :as driver]
[metabase.driver.sql.parameters.substitution :as substitution]))
(deftest honeysql->replacement-snippet-info-test
- (driver/with-driver :h2
- (testing "make sure we handle quotes inside names correctly!"
- (is (= {:replacement-snippet "\"test-data\".\"PUBLIC\".\"checkins\".\"date\""
- :prepared-statement-args nil}
- (#'substitution/honeysql->replacement-snippet-info :test-data.PUBLIC.checkins.date))))))
+ (testing "make sure we handle quotes inside names correctly!"
+ (is (= {:replacement-snippet "\"test-data\".\"PUBLIC\".\"checkins\".\"date\""
+ :prepared-statement-args nil}
+ (#'substitution/honeysql->replacement-snippet-info :h2 :test-data.PUBLIC.checkins.date)))))
diff --git a/test/metabase/query_processor_test/timezones_test.clj b/test/metabase/query_processor_test/timezones_test.clj
index 0b44a8d4e1c2c40124b0fe1a689a566e8a596ab6..0c89cc1f476c90ff507982c80ff957577cb3a7f8 100644
--- a/test/metabase/query_processor_test/timezones_test.clj
+++ b/test/metabase/query_processor_test/timezones_test.clj
@@ -191,11 +191,11 @@
;; Make sure TIME values are handled consistently (#10366)
(defn- attempts []
(zipmap
- [:date :time :datetime :time-ltz :time-tz :datetime-ltz :datetime-tz :datetime-tz-id]
+ [:date :time :datetime :time_ltz :time_tz :datetime_ltz :datetime_tz :datetime_tz_id]
(qp.test/first-row
(qp/process-query
(data/query attempts
- {:query {:fields [$date $time $datetime $time-ltz $time-tz $datetime-ltz $datetime-tz $datetime-tz-id]
+ {:query {:fields [$date $time $datetime $time_ltz $time_tz $datetime_ltz $datetime_tz $datetime_tz_id]
:filter [:= $id 1]}
:middleware {:format-rows? false}})))))
@@ -216,15 +216,15 @@
{:date (t/local-date "2019-11-01")
:time (t/local-time "00:23:18.331")
:datetime (t/local-date-time "2019-11-01T00:23:18.331")
- :datetime-ltz (t/offset-date-time "2019-11-01T07:23:18.331Z")}
+ :datetime_ltz (t/offset-date-time "2019-11-01T07:23:18.331Z")}
(when (supports-time-with-time-zone?)
- {:time-ltz (t/offset-time "07:23:18.331Z")})
+ {:time_ltz (t/offset-time "07:23:18.331Z")})
(when (supports-time-with-offset?)
- {:time-tz (t/offset-time "00:23:18.331-07:00")})
+ {:time_tz (t/offset-time "00:23:18.331-07:00")})
(when (supports-datetime-with-offset?)
- {:datetime-tz (t/offset-date-time "2019-11-01T00:23:18.331-07:00")})
+ {:datetime_tz (t/offset-date-time "2019-11-01T00:23:18.331-07:00")})
(when (supports-datetime-with-zone-id?)
- {:datetime-tz-id (t/zoned-date-time "2019-11-01T00:23:18.331-07:00[America/Los_Angeles]")})))
+ {:datetime_tz_id (t/zoned-date-time "2019-11-01T00:23:18.331-07:00[America/Los_Angeles]")})))
(deftest time-timezone-handling-test
;; Actual value : "2019-11-01T00:23:18.331-07:00[America/Los_Angeles]"
diff --git a/test/metabase/test/data/dataset_definitions.clj b/test/metabase/test/data/dataset_definitions.clj
index 6a205c63a1a88250b9aa3b0e7f1654475fc64848..cc6a7e696ef41d574075e5df541699ed2954ac89 100644
--- a/test/metabase/test/data/dataset_definitions.clj
+++ b/test/metabase/test/data/dataset_definitions.clj
@@ -150,15 +150,15 @@
"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."
[["attempts"
- [{:field-name "num-crows", :base-type :type/Integer}
+ [{:field-name "num_crows", :base-type :type/Integer}
{:field-name "date", :base-type :type/Date}
{:field-name "time", :base-type :type/Time}
- {:field-name "time-ltz", :base-type :type/TimeWithLocalTZ}
- {:field-name "time-tz", :base-type :type/TimeWithZoneOffset}
+ {:field-name "time_ltz", :base-type :type/TimeWithLocalTZ}
+ {:field-name "time_tz", :base-type :type/TimeWithZoneOffset}
{:field-name "datetime", :base-type :type/DateTime}
- {:field-name "datetime-ltz", :base-type :type/DateTimeWithLocalTZ}
- {:field-name "datetime-tz", :base-type :type/DateTimeWithZoneOffset}
- {:field-name "datetime-tz-id", :base-type :type/DateTimeWithZoneID}]
+ {:field-name "datetime_ltz", :base-type :type/DateTimeWithLocalTZ}
+ {:field-name "datetime_tz", :base-type :type/DateTimeWithZoneOffset}
+ {:field-name "datetime_tz_id", :base-type :type/DateTimeWithZoneID}]
(for [[cnt t] [[6 #t "2019-11-01T00:23:18.331-07:00[America/Los_Angeles]"]
[8 #t "2019-11-02T00:14:14.246-07:00[America/Los_Angeles]"]
[6 #t "2019-11-03T23:35:17.906-08:00[America/Los_Angeles]"]