Put together a report and database timezone test suite [ci drivers]

The goal is to make it easier to write tests with different data and
report timezone scenarios easier. The tests already have found issues
with our current code and their handling of these timezone
conversions. These tests exposed a bug in MySQL date support which is
also fixed in this commit.

Fixes #6554, fixes #6994, fixes #7393

src/metabase/driver/generic_sql.clj

@@ -19,11 +19,13 @@
             metabase.query-processor.interface
             [metabase.util
              [honeysql-extensions :as hx]
-             [ssh :as ssh]])
+             [ssh :as ssh]]
+            [schema.core :as s])
   (:import [clojure.lang Keyword PersistentVector]
            com.mchange.v2.c3p0.ComboPooledDataSource
+           honeysql.types.SqlCall
            [java.sql DatabaseMetaData ResultSet]
-           java.util.Map
+           [java.util Date Map]
            metabase.models.field.FieldInstance
            [metabase.query_processor.interface Field Value]))
 
@@ -90,20 +92,6 @@
 
      Return `nil` to prevent FIELD from being aliased.")
 
-  (prepare-sql-param [this obj]
-    "*OPTIONAL*. Do any neccesary type conversions, etc. to an object being passed as a prepared statment argument in
-     a parameterized raw SQL query. For example, a raw SQL query with a date param, `x`, e.g. `WHERE date > {{x}}`, is
-     converted to SQL like `WHERE date > ?`, and the value of `x` is passed as a `java.sql.Timestamp`. Some databases,
-     notably SQLite, don't work with `Timestamps`, and dates must be passed as string literals instead; the SQLite
-     driver overrides this method to convert dates as needed.
-
-  The default implementation is `identity`.
-
-  NOTE - This method is only used for parameters in raw SQL queries. It's not needed for MBQL queries because
-  the multimethod `metabase.driver.generic-sql.query-processor/->honeysql` provides an opportunity for drivers to do
-  type conversions as needed. In the future we may simplify a bit and combine them into a single method used in both
-  places.")
-
   (quote-style ^clojure.lang.Keyword [this]
     "*OPTIONAL*. Return the quoting style that should be used by [HoneySQL](https://github.com/jkk/honeysql) when
      building a SQL statement. Defaults to `:ansi`, but other valid options are `:mysql`, `:sqlserver`, `:oracle`, and
@@ -382,6 +370,62 @@
                                :schema (:pktable_schem result)}
             :dest-column-name (:pkcolumn_name result)}))))
 
+;;; ## Native SQL parameter functions
+
+(def PreparedStatementSubstitution
+  "Represents the SQL string replace value (usually ?) and the typed parameter value"
+  {:sql-string   s/Str
+   :param-values [s/Any]})
+
+(s/defn make-stmt-subs :- PreparedStatementSubstitution
+  "Create a `PreparedStatementSubstitution` map for `sql-string` and the `param-seq`"
+  [sql-string param-seq]
+  {:sql-string   sql-string
+   :param-values param-seq})
+
+(defmulti ^{:doc          (str "Returns a `PreparedStatementSubstitution` for `x` and the given driver. "
+                               "This allows driver specific parameters and SQL replacement text (usually just ?). "
+                               "The param value is already prepared and ready for inlcusion in the query, such as "
+                               "what's needed for SQLite and timestamps.")
+            :arglists     '([driver x])
+            :style/indent 1}
+  ->prepared-substitution
+  (fn [driver x]
+    [(class driver) (class x)]))
+
+(s/defn ^:private honeysql->prepared-stmt-subs
+  "Convert X to a replacement snippet info map by passing it to HoneySQL's `format` function."
+  [driver x]
+  (let [[snippet & args] (hsql/format x, :quoting (quote-style driver))]
+    (make-stmt-subs snippet args)))
+
+(s/defmethod ->prepared-substitution [Object nil] :- PreparedStatementSubstitution
+  [driver _]
+  (honeysql->prepared-stmt-subs driver nil))
+
+(s/defmethod ->prepared-substitution [Object Object] :- PreparedStatementSubstitution
+  [driver obj]
+  (honeysql->prepared-stmt-subs driver (str obj)))
+
+(s/defmethod ->prepared-substitution [Object Number] :- PreparedStatementSubstitution
+  [driver num]
+  (honeysql->prepared-stmt-subs driver num))
+
+(s/defmethod ->prepared-substitution [Object Boolean] :- PreparedStatementSubstitution
+  [driver b]
+  (honeysql->prepared-stmt-subs driver b))
+
+(s/defmethod ->prepared-substitution [Object Keyword] :- PreparedStatementSubstitution
+  [driver kwd]
+  (honeysql->prepared-stmt-subs driver kwd))
+
+(s/defmethod ->prepared-substitution [Object SqlCall] :- PreparedStatementSubstitution
+  [driver sql-call]
+  (honeysql->prepared-stmt-subs driver sql-call))
+
+(s/defmethod ->prepared-substitution [Object Date] :- PreparedStatementSubstitution
+  [driver date]
+  (make-stmt-subs "?" [date]))
 
 (defn ISQLDriverDefaultsMixin
   "Default implementations for methods in `ISQLDriver`."
@@ -404,7 +448,6 @@
    :excluded-schemas     (constantly nil)
    :field->identifier    (u/drop-first-arg (comp (partial apply hsql/qualify) field/qualified-name-components))
    :field->alias         (u/drop-first-arg name)
-   :prepare-sql-param    (u/drop-first-arg identity)
    :quote-style          (constantly :ansi)
    :set-timezone-sql     (constantly nil)
    :stddev-fn            (constantly :STDDEV)})

src/metabase/driver/mysql.clj

@@ -6,7 +6,7 @@
              [format :as time]]
             [clojure
              [set :as set]
-             [string :as s]]
+             [string :as str]]
             [honeysql.core :as hsql]
             [metabase
              [driver :as driver]
@@ -17,9 +17,11 @@
             [metabase.util
              [date :as du]
              [honeysql-extensions :as hx]
-             [ssh :as ssh]])
+             [ssh :as ssh]]
+            [schema.core :as s])
   (:import java.sql.Time
            [java.util Date TimeZone]
+           metabase.util.honeysql_extensions.Literal
            org.joda.time.format.DateTimeFormatter))
 
 (defrecord MySQLDriver []
@@ -62,7 +64,7 @@
     :TINYTEXT   :type/Text
     :VARBINARY  :type/*
     :VARCHAR    :type/Text
-    :YEAR       :type/Integer} (keyword (s/replace (name column-type) #"\sUNSIGNED$" "")))) ; strip off " UNSIGNED" from end if present
+    :YEAR       :type/Integer} (keyword (str/replace (name column-type) #"\sUNSIGNED$" "")))) ; strip off " UNSIGNED" from end if present
 
 (def ^:private ^:const default-connection-args
   "Map of args for the MySQL JDBC connection string.
@@ -79,8 +81,8 @@
    :useJDBCCompliantTimezoneShift :true})
 
 (def ^:private ^:const ^String default-connection-args-string
-  (s/join \& (for [[k v] default-connection-args]
-               (str (name k) \= (name v)))))
+  (str/join \& (for [[k v] default-connection-args]
+                 (str (name k) \= (name v)))))
 
 (defn- append-connection-args
   "Append `default-connection-args-string` to the connection string in CONNECTION-DETAILS, and an additional option to
@@ -128,11 +130,14 @@
   [date-time]
   (timezone-id->offset-str (.getID (TimeZone/getDefault)) date-time))
 
-;; MySQL doesn't seem to correctly want to handle timestamps no matter how nicely we ask. SAD! Thus we will just
-;; convert them to appropriate timestamp literals and include functions to convert timezones as needed
-(defmethod sqlqp/->honeysql [MySQLDriver Date]
-  [_ date]
-  (let [date-as-dt                 (tcoerce/from-date date)
+(s/defn ^:private create-hsql-for-date
+  "Returns an HoneySQL structure representing the date for MySQL. If there's a report timezone, we need to ensure the
+  timezone conversion is wrapped around the `date-literal-or-string`. It supports both an `hx/literal` and a plain
+  string depending on whether or not the date value should be emedded in the statement or separated as a prepared
+  statement parameter. Use a string for prepared statement values, a literal if you want it embedded in the statement"
+  [date-obj :- java.util.Date
+   date-literal-or-string :- (s/either s/Str Literal)]
+  (let [date-as-dt                 (tcoerce/from-date date-obj)
         report-timezone-offset-str (timezone-id->offset-str (driver/report-timezone) date-as-dt)
         system-timezone-offset-str (system-timezone->offset-str date-as-dt)]
     (if (and report-timezone-offset-str
@@ -151,12 +156,29 @@
       ;; preferable to have timezones slightly wrong in these rare theoretical situations, instead of all the time, as
       ;; was the previous behavior.
       (hsql/call :convert_tz
-        (hx/literal (du/format-date :date-hour-minute-second-ms date))
+        date-literal-or-string
         (hx/literal system-timezone-offset-str)
         (hx/literal report-timezone-offset-str))
       ;; otherwise if we don't have a report timezone we can continue to pass the object as-is, e.g. as a prepared
       ;; statement param
-      date)))
+      date-obj)))
+
+;; MySQL doesn't seem to correctly want to handle timestamps no matter how nicely we ask. SAD! Thus we will just
+;; convert them to appropriate timestamp literals and include functions to convert timezones as needed
+(defmethod sqlqp/->honeysql [MySQLDriver Date]
+  [_ date]
+  (create-hsql-for-date date (hx/literal (du/format-date :date-hour-minute-second-ms date))))
+
+;; The sqlqp/->honeysql entrypoint is used by MBQL, but native queries with field filters have the same issue. Below
+;; will return a map that will be used in the prepared statement to correctly convert and parameterize the date
+(s/defmethod sql/->prepared-substitution [MySQLDriver Date] :- sql/PreparedStatementSubstitution
+  [_ date]
+  (let [date-str (du/format-date :date-hour-minute-second-ms date)]
+    (sql/make-stmt-subs (-> (create-hsql-for-date date date-str)
+                            hx/->date
+                            (hsql/format :quoting (sql/quote-style (MySQLDriver.)))
+                            first)
+                        [(du/format-date :date-hour-minute-second-ms date)])))
 
 (defmethod sqlqp/->honeysql [MySQLDriver Time]
   [_ time-value]

src/metabase/driver/sqlite.clj

@@ -14,7 +14,8 @@
             [metabase.driver.generic-sql.query-processor :as sqlqp]
             [metabase.util
              [date :as du]
-             [honeysql-extensions :as hx]])
+             [honeysql-extensions :as hx]]
+            [schema.core :as s])
   (:import [java.sql Time Timestamp]))
 
 (defrecord SQLiteDriver []
@@ -139,20 +140,17 @@
     :seconds      (->datetime expr (hx/literal "unixepoch"))
     :milliseconds (recur (hx// expr 1000) :seconds)))
 
-
 ;; SQLite doesn't like things like Timestamps getting passed in as prepared statement args, so we need to convert them
 ;; to date literal strings instead to get things to work
 ;;
 ;; TODO - not sure why this doesn't need to be done in `->honeysql` as well? I think it's because the MBQL date values
 ;; are funneled through the `date` family of functions above
-(defn- prepare-sql-param [obj]
-  (if (instance? java.util.Date obj)
-    ;; for anything that's a Date (usually a java.sql.Timestamp) convert it to a yyyy-MM-dd formatted date literal
-    ;; string For whatever reason the SQL generated from parameters ends up looking like `WHERE date(some_field) = ?`
-    ;; sometimes so we need to use just the date rather than a full ISO-8601 string
-    (du/format-date "yyyy-MM-dd" obj)
-    ;; every other prepared statement arg can be returned as-is
-    obj))
+(s/defmethod sql/->prepared-substitution [SQLiteDriver java.util.Date] :- sql/PreparedStatementSubstitution
+  [_ date]
+  ;; for anything that's a Date (usually a java.sql.Timestamp) convert it to a yyyy-MM-dd formatted date literal
+  ;; string For whatever reason the SQL generated from parameters ends up looking like `WHERE date(some_field) = ?`
+  ;; sometimes so we need to use just the date rather than a full ISO-8601 string
+  (sql/make-stmt-subs "?" [(du/format-date "yyyy-MM-dd" date)]))
 
 ;; SQLite doesn't support `TRUE`/`FALSE`; it uses `1`/`0`, respectively; convert these booleans to numbers.
 (defmethod sqlqp/->honeysql [SQLiteDriver Boolean]
@@ -206,7 +204,6 @@
     :connection-details->spec  (u/drop-first-arg connection-details->spec)
     :current-datetime-fn       (constantly (hsql/call :datetime (hx/literal :now)))
     :date                      (u/drop-first-arg date)
-    :prepare-sql-param         (u/drop-first-arg prepare-sql-param)
     :string-length-fn          (u/drop-first-arg string-length-fn)
     :unix-timestamp->timestamp (u/drop-first-arg unix-timestamp->timestamp)}))
 

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

@@ -8,6 +8,7 @@
             [medley.core :as m]
             [metabase.driver :as driver]
             [metabase.models.field :as field :refer [Field]]
+            [metabase.driver.generic-sql :as sql]
             [metabase.query-processor.middleware.expand :as ql]
             [metabase.query-processor.middleware.parameters.dates :as date-params]
             [metabase.util
@@ -328,7 +329,7 @@
 (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 ((resolve 'metabase.driver.generic-sql/quote-style) *driver*))]
+  (let [[snippet & args] (hsql/format x, :quoting (sql/quote-style *driver*))]
     {:replacement-snippet     snippet
      :prepared-statement-args args}))
 
@@ -337,9 +338,9 @@
    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]
-  (-> (honeysql->replacement-snippet-info (let [identifier ((resolve 'metabase.driver.generic-sql/field->identifier) *driver* field)]
+  (-> (honeysql->replacement-snippet-info (let [identifier (sql/field->identifier *driver* field)]
                                             (if (date-param-type? param-type)
-                                              ((resolve 'metabase.driver.generic-sql/date) *driver* :day identifier)
+                                              (sql/date *driver* :day identifier)
                                               identifier)))
       :replacement-snippet))
 
@@ -349,13 +350,23 @@
   {:replacement-snippet     (str \( (str/join " AND " (map :replacement-snippet replacement-snippet-maps)) \))
    :prepared-statement-args (reduce concat (map :prepared-statement-args replacement-snippet-maps))})
 
+(defn- create-replacement-snippet [nil-or-obj]
+  (let [{:keys [sql-string param-values]} (sql/->prepared-substitution *driver* nil-or-obj)]
+    {:replacement-snippet sql-string
+     :prepared-statement-args param-values}))
+
+(defn- prepared-ts-subs [operator date-str]
+  (let [{:keys [sql-string param-values]} (sql/->prepared-substitution *driver* (du/->Timestamp date-str))]
+    {:replacement-snippet (str operator " " sql-string)
+     :prepared-statement-args param-values}))
+
 (extend-protocol ISQLParamSubstituion
-  nil     (->replacement-snippet-info [this] (honeysql->replacement-snippet-info this))
-  Object  (->replacement-snippet-info [this] (honeysql->replacement-snippet-info (str this)))
-  Number  (->replacement-snippet-info [this] (honeysql->replacement-snippet-info this))
-  Boolean (->replacement-snippet-info [this] (honeysql->replacement-snippet-info this))
-  Keyword (->replacement-snippet-info [this] (honeysql->replacement-snippet-info this))
-  SqlCall (->replacement-snippet-info [this] (honeysql->replacement-snippet-info this))
+  nil     (->replacement-snippet-info [this] (create-replacement-snippet this))
+  Object  (->replacement-snippet-info [this] (create-replacement-snippet (str this)))
+  Number  (->replacement-snippet-info [this] (create-replacement-snippet this))
+  Boolean (->replacement-snippet-info [this] (create-replacement-snippet this))
+  Keyword (->replacement-snippet-info [this] (create-replacement-snippet this))
+  SqlCall (->replacement-snippet-info [this] (create-replacement-snippet this))
   NoValue (->replacement-snippet-info [_]    {:replacement-snippet ""})
 
   CommaSeparatedNumbers
@@ -370,15 +381,24 @@
 
   Date
   (->replacement-snippet-info [{:keys [s]}]
-    (honeysql->replacement-snippet-info (du/->Timestamp s)))
+    (create-replacement-snippet (du/->Timestamp s)))
 
   DateRange
   (->replacement-snippet-info [{:keys [start end]}]
     (cond
-      (= start end) {:replacement-snippet "= ?",             :prepared-statement-args [(du/->Timestamp start)]}
-      (nil? start)  {:replacement-snippet "< ?",             :prepared-statement-args [(du/->Timestamp end)]}
-      (nil? end)    {:replacement-snippet "> ?",             :prepared-statement-args [(du/->Timestamp start)]}
-      :else         {:replacement-snippet "BETWEEN ? AND ?", :prepared-statement-args [(du/->Timestamp start) (du/->Timestamp end)]}))
+      (= start end)
+      (prepared-ts-subs \= start)
+
+      (nil? start)
+      (prepared-ts-subs \< end)
+
+      (nil? end)
+      (prepared-ts-subs \> start)
+
+      :else
+      (let [params (map (comp #(sql/->prepared-substitution *driver* %) du/->Timestamp) [start end])]
+        {:replacement-snippet     (apply format "BETWEEN %s AND %s" (map :sql-string params)),
+         :prepared-statement-args (vec (mapcat :param-values params))})))
 
   ;; TODO - clean this up if possible!
   Dimension
@@ -506,14 +526,9 @@
 ;;; |                                            PUTTING IT ALL TOGETHER                                             |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
-(defn- prepare-sql-param-for-driver [param]
-  ((resolve 'metabase.driver.generic-sql/prepare-sql-param) *driver* param))
-
 (s/defn ^:private expand-query-params
   [{sql :query, :as native}, param-key->value :- ParamValues]
-  (merge native
-         ;; `prepare-sql-param-for-driver` can't be lazy as it needs `*driver*` to be bound
-         (update (parse-template sql param-key->value) :params #(mapv prepare-sql-param-for-driver %))))
+  (merge native (parse-template sql param-key->value)))
 
 (defn- ensure-driver
   "Depending on where the query came from (the user, permissions check etc) there might not be an driver associated to

src/metabase/types.clj

@@ -82,6 +82,7 @@
 
 (derive :type/Time :type/DateTime)
 (derive :type/Date :type/DateTime)
+(derive :type/DateTimeWithTZ :type/DateTime)
 
 (derive :type/UNIXTimestamp :type/DateTime)
 (derive :type/UNIXTimestamp :type/Integer)

src/metabase/util/date.clj

@@ -197,7 +197,8 @@
   (->Timestamp (System/currentTimeMillis)))
 
 (defn format-date
-  "Format DATE using a given DATE-FORMAT.
+  "Format DATE using a given DATE-FORMAT. NOTE: This will create a date string in the JVM's timezone, not the report
+  timezone.
 
    DATE is anything that can coerced to a `Timestamp` via `->Timestamp`, such as a `Date`, `Timestamp`,
    `Long` (ms since the epoch), or an ISO-8601 `String`. DATE defaults to the current moment in time.

test/metabase/driver/bigquery_test.clj

@@ -44,7 +44,6 @@
        (sort-by first)
        (take 5)))
 
-
 ;; make sure that BigQuery native queries maintain the column ordering specified in the SQL -- post-processing
 ;; ordering shouldn't apply (Issue #2821)
 (expect-with-engine :bigquery

test/metabase/driver/oracle_test.clj

@@ -5,6 +5,7 @@
             [metabase.driver
              [generic-sql :as sql]
              [oracle :as oracle]]
+            [metabase.models.setting :as setting]
             [metabase.test.data.datasets :refer [expect-with-engine]]
             [metabase.test.util :as tu])
   (:import metabase.driver.oracle.OracleDriver))
@@ -67,4 +68,6 @@
 
 (expect-with-engine :oracle
   "UTC"
-  (tu/db-timezone-id))
+  (do
+    (setting/set! :report-timezone "")
+    (tu/db-timezone-id)))

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

@@ -592,13 +592,25 @@
 
 ;; Test that native dates are parsed with the report timezone (when supported)
 (datasets/expect-with-engines (disj sql-parameters-engines :sqlite)
-  [(if (qpt/supports-report-timezone? datasets/*engine*)
+  [(cond
+     (= :presto datasets/*engine*)
+     "2018-04-18"
+
+     (qpt/supports-report-timezone? datasets/*engine*)
      "2018-04-18T00:00:00.000-07:00"
+
+     :else
      "2018-04-18T00:00:00.000Z")]
   (tu/with-temporary-setting-values [report-timezone "America/Los_Angeles"]
     (first-row
       (process-native
-        :native     {:query         "SELECT cast({{date}} as date)"
+        :native     {:query         (cond
+                                      (= :bigquery datasets/*engine*)
+                                      "SELECT {{date}} as date"
+                                      (= :oracle datasets/*engine*)
+                                      "SELECT cast({{date}} as date) from dual"
+                                      :else
+                                      "SELECT cast({{date}} as date)")
                      :template_tags {:date {:name "date" :display_name "Date" :type "date" }}}
         :parameters [{:type "date/single" :target ["variable" ["template-tag" "date"]] :value "2018-04-18"}]))))
 

test/metabase/query_processor_test/timezones_test.clj

+(ns metabase.query-processor-test.timezones-test
+  (:require [clojure.java.jdbc :as jdbc]
+            [expectations :refer :all]
+            [metabase
+             [query-processor :as qp]
+             [query-processor-test :as qptest]]
+            [metabase.query-processor.middleware.expand :as ql]
+            [metabase.query-processor-test :as qpt]
+            [metabase.test
+             [data :as data]
+             [util :as tu]]
+            [metabase.test.data.interface :as i]
+            [metabase.test.data.mysql :as mysql-data]
+            [metabase.driver.generic-sql :as sql]
+            [metabase.test.data.generic-sql :as generic-sql]
+            [metabase.test.data.datasets :refer [expect-with-engine expect-with-engines *engine* *driver*]]
+            [metabase.test.data.dataset-definitions :as defs]
+            [toucan.db :as db])
+  (:import metabase.driver.mysql.MySQLDriver))
+
+(def ^:private mysql-driver (MySQLDriver.))
+
+(defn- fix-mysql-timestamps?
+  "Returns true if the database at `db-spec` needs to have it's timestamps fixed. See the `update-mysql-timestamps
+  comment for more information on why these are being fixed"
+  [db-spec]
+  (empty? (jdbc/query db-spec "select 1 from users where id=1 and last_login='2014-04-01 01:30:00'")))
+
+(defn- update-mysql-timestamps
+  "Unfortunately the timestamps we insert in this dataset are in UTC, but MySQL is inserting them as if they were in
+  pacific time. This means that they are rolling them forward 7 (or 8) hours. Instead of inserting 08:30 it's
+  inserting 15:30. This is wrong, rather than hack something together that weaves through all of the data loading
+  code, this function just fixes up the timestamps after the data is loaded using MySQL's `convert_tz` function"
+  []
+  (when (= :mysql *engine*)
+    (let [details (i/database->connection-details mysql-driver :db {:database-name "test-data-with-timezones"})
+          db-spec (sql/connection-details->spec mysql-driver details)]
+      (when (fix-mysql-timestamps? db-spec)
+        (jdbc/execute! db-spec "update users set last_login=convert_tz(last_login,'UTC','America/Los_Angeles')")))))
+
+(defn- call-with-timezones-db [f]
+  ;; Does the database exist?
+  (when-not (i/metabase-instance defs/test-data-with-timezones *engine*)
+    ;; The database doesn't exist, so we need to create it
+    (data/get-or-create-database! defs/test-data-with-timezones)
+    ;; The db has been created but the timestamps are wrong on MySQL, fix them up
+    (update-mysql-timestamps))
+  ;; The database can now be used in tests
+  (data/with-db (data/get-or-create-database! defs/test-data-with-timezones)
+    (f)))
+
+(defmacro ^:private with-tz-db
+  "Calls `with-db` on the `test-data-with-timezones` dataset and ensures the timestamps are fixed up on MySQL"
+  [& body]
+  `(call-with-timezones-db (fn [] ~@body)))
+
+(def ^:private default-utc-results
+  #{[6 "Shad Ferdynand" "2014-08-02T12:30:00.000Z"]
+    [7 "Conchúr Tihomir" "2014-08-02T09:30:00.000Z"]})
+
+(def ^:private default-pacific-results
+  #{[6 "Shad Ferdynand" "2014-08-02T05:30:00.000-07:00"]
+    [7 "Conchúr Tihomir" "2014-08-02T02:30:00.000-07:00"]})
+
+;; Test querying a database that does NOT support report timezones
+;;
+;; The report-timezone of Europe/Brussels is UTC+2, our tests use a JVM timezone of UTC. If the timestamps below are
+;; interpretted incorrectly as Europe/Brussels, it would adjust that back 2 hours to UTC
+;; (i.e. 2014-07-01T22:00:00.000Z). We then cast that time to a date, which truncates it to 2014-07-01, which is then
+;; querying the day before. This reproduces the bug found in https://github.com/metabase/metabase/issues/7584
+(expect-with-engine :bigquery
+  #{[10 "Frans Hevel" "2014-07-03T19:30:00.000Z"]
+    [12 "Kfir Caj" "2014-07-03T01:30:00.000Z"]}
+  (with-tz-db
+    (tu/with-temporary-setting-values [report-timezone "Europe/Brussels"]
+      (-> (data/run-query users
+            (ql/filter (ql/between $last_login
+                                   "2014-07-02"
+                                   "2014-07-03")))
+          qptest/rows
+          set))))
+
+;; Query PG using a report-timezone set to pacific time. Should adjust the query parameter using that report timezone
+;; and should return the timestamp in pacific time as well
+(expect-with-engines [:postgres :mysql]
+  default-pacific-results
+  (with-tz-db
+    (tu/with-temporary-setting-values [report-timezone "America/Los_Angeles"]
+      (-> (data/run-query users
+            (ql/filter (ql/between $last_login
+                                   "2014-08-02T03:00:00.000000"
+                                   "2014-08-02T06:00:00.000000")))
+          qptest/rows
+          set))))
+
+(defn- quote-name [identifier]
+  (generic-sql/quote-name *driver* identifier))
+
+(defn- users-table-identifier []
+  ;; HACK ! I don't have all day to write protocol methods to make this work the "right" way so for BigQuery and
+  (if (= *engine* :bigquery)
+    "[test_data_with_timezones.users]"
+    (let [{table-name :name, schema :schema} (db/select-one ['Table :name :schema], :id (data/id :users))]
+      (str (when (seq schema)
+             (str (quote-name schema) \.))
+           (quote-name table-name)))))
+
+(defn- field-identifier [& kwds]
+  (let [field (db/select-one ['Field :name :table_id] :id (apply data/id kwds))
+        {table-name :name, schema :schema} (db/select-one ['Table :name :schema] :id (:table_id field))]
+    (str (when (seq schema)
+           (str (quote-name schema) \.))
+         (quote-name table-name) \. (quote-name (:name field)))))
+
+(def ^:private process-query' (comp set qpt/rows qp/process-query))
+
+;; Test that native dates are parsed with the report timezone (when supported)
+(expect-with-engines [:postgres :mysql]
+  default-pacific-results
+  (with-tz-db
+    (tu/with-temporary-setting-values [report-timezone "America/Los_Angeles"]
+      (process-query'
+       {:database (data/id)
+        :type :native
+        :native     {:query         (format "select %s, %s, %s from %s where cast(last_login as date) between {{date1}} and {{date2}}"
+                                            (field-identifier :users :id)
+                                            (field-identifier :users :name)
+                                            (field-identifier :users :last_login)
+                                            (users-table-identifier))
+                     :template_tags {:date1 {:name "date1" :display_name "Date1" :type "date" }
+                                     :date2 {:name "date2" :display_name "Date2" :type "date" }}}
+        :parameters [{:type "date/single" :target ["variable" ["template-tag" "date1"]] :value "2014-08-02T02:00:00.000000"}
+                     {:type "date/single" :target ["variable" ["template-tag" "date2"]] :value "2014-08-02T06:00:00.000000"}]}))))
+
+;; This does not currently work for MySQL
+(expect-with-engines [:postgres :mysql]
+  default-pacific-results
+  (with-tz-db
+    (tu/with-temporary-setting-values [report-timezone "America/Los_Angeles"]
+      (process-query'
+       {:database (data/id)
+        :type :native
+        :native     {:query         (format "select %s, %s, %s from %s where {{ts_range}}"
+                                            (field-identifier :users :id)
+                                            (field-identifier :users :name)
+                                            (field-identifier :users :last_login)
+                                            (users-table-identifier))
+                     :template_tags {:ts_range {:name "ts_range", :display_name "Timestamp Range", :type "dimension",
+                                                :dimension ["field-id" (data/id :users :last_login)]}}}
+        :parameters [{:type "date/range", :target ["dimension" ["template-tag" "ts_range"]], :value "2014-08-02~2014-08-03"}]}))))
+
+;; Querying using a single date
+(expect-with-engines [:postgres :mysql]
+  default-pacific-results
+  (with-tz-db
+    (tu/with-temporary-setting-values [report-timezone "America/Los_Angeles"]
+      (process-query'
+       {:database (data/id)
+        :type :native
+        :native     {:query         (format "select %s, %s, %s from %s where {{just_a_date}}"
+                                            (field-identifier :users :id)
+                                            (field-identifier :users :name)
+                                            (field-identifier :users :last_login)
+                                            (users-table-identifier))
+                     :template_tags {:just_a_date {:name "just_a_date", :display_name "Just A Date", :type "dimension",
+                                                   :dimension ["field-id" (data/id :users :last_login)]}}}
+        :parameters [{:type "date/single", :target ["dimension" ["template-tag" "just_a_date"]], :value "2014-08-02"}]}))))
+
+;; This is the same answer as above but uses timestamp with the timezone included. The report timezone is still
+;; pacific though, so it should return as pacific regardless of how the filter was specified
+(expect-with-engines [:postgres :mysql]
+  default-pacific-results
+  (with-tz-db
+    (tu/with-temporary-setting-values [report-timezone "America/Los_Angeles"]
+      (-> (data/run-query users
+            (ql/filter (ql/between $last_login
+                                   "2014-08-02T10:00:00.000000Z"
+                                   "2014-08-02T13:00:00.000000Z")))
+          qptest/rows
+          set))))
+
+;; Checking UTC report timezone filtering and responses
+(expect-with-engines [:postgres :bigquery :mysql]
+  default-utc-results
+  (with-tz-db
+    (tu/with-temporary-setting-values [report-timezone "UTC"]
+      (-> (data/run-query users
+            (ql/filter (ql/between $last_login
+                                   "2014-08-02T10:00:00.000000"
+                                   "2014-08-02T13:00:00.000000")))
+          qptest/rows
+          set))))
+
+;; With no report timezone, the JVM timezone is used. For our tests this is UTC so this should be the same as
+;; specifying UTC for a report timezone
+(expect-with-engines [:postgres :bigquery :mysql]
+  default-utc-results
+  (with-tz-db
+    (-> (data/run-query users
+          (ql/filter (ql/between $last_login
+                                 "2014-08-02T10:00:00.000000"
+                                 "2014-08-02T13:00:00.000000")))
+        qptest/rows
+        set)))

test/metabase/test/data/bigquery.clj

@@ -130,16 +130,17 @@
 
 
 (def ^:private ^:const base-type->bigquery-type
-  {:type/BigInteger :INTEGER
-   :type/Boolean    :BOOLEAN
-   :type/Date       :TIMESTAMP
-   :type/DateTime   :TIMESTAMP
-   :type/Decimal    :FLOAT
-   :type/Dictionary :RECORD
-   :type/Float      :FLOAT
-   :type/Integer    :INTEGER
-   :type/Text       :STRING
-   :type/Time       :TIME})
+  {:type/BigInteger     :INTEGER
+   :type/Boolean        :BOOLEAN
+   :type/Date           :TIMESTAMP
+   :type/DateTime       :TIMESTAMP
+   :type/DateTimeWithTZ :TIMESTAMP
+   :type/Decimal        :FLOAT
+   :type/Dictionary     :RECORD
+   :type/Float          :FLOAT
+   :type/Integer        :INTEGER
+   :type/Text           :STRING
+   :type/Time           :TIME})
 
 (defn- fielddefs->field-name->base-type
   "Convert FIELD-DEFINITIONS to a format appropriate for passing to `create-table!`."
@@ -200,7 +201,7 @@
 
 (defn- create-db! [{:keys [database-name table-definitions]}]
   {:pre [(seq database-name) (sequential? table-definitions)]}
-  ;; fetch existing datasets if we haven't done so yet
+    ;; fetch existing datasets if we haven't done so yet
   (when-not (seq @existing-datasets)
     (reset! existing-datasets (set (existing-dataset-names)))
     (println "These BigQuery datasets have already been loaded:\n" (u/pprint-to-str (sort @existing-datasets))))

test/metabase/test/data/dataset_definitions.clj

 (ns metabase.test.data.dataset-definitions
   "Definitions of various datasets for use in tests with `with-temp-db`."
   (:require [clojure.tools.reader.edn :as edn]
-            [metabase.test.data.interface :as di])
+            [metabase.test.data.interface :as di]
+            [metabase.util.date :as du])
   (:import java.sql.Time
            java.util.Calendar))
 
@@ -77,6 +78,15 @@
                          (mapv #(conj % nil) rows))
                        (di/slurp-edn-table-def "test-data")))
 
+(di/def-database-definition test-data-with-timezones
+  (di/update-table-def "users"
+                       (fn [table-def]
+                         [(first table-def)
+                          {:field-name "last_login", :base-type :type/DateTimeWithTZ}
+                          (peek table-def)])
+                       identity
+                       (di/slurp-edn-table-def "test-data")))
+
 (def test-data-map
   "Converts data from `test-data` to a map of maps like the following:
 

test/metabase/test/data/mysql.clj

 (ns metabase.test.data.mysql
   "Code for creating / destroying a MySQL database from a `DatabaseDefinition`."
-  (:require [metabase.driver.mysql :as mysql]
+  (:require [clojure.java.jdbc :as jdbc]
+            [metabase.driver.generic-sql :as gsql]
+            [metabase.driver.mysql :as mysql]
             [metabase.test.data
              [generic-sql :as generic]
              [interface :as i]]
             [metabase.util :as u]))
 
 (def ^:private ^:const field-base-type->sql-type
-  {:type/BigInteger "BIGINT"
-   :type/Boolean    "BOOLEAN" ; Synonym of TINYINT(1)
-   :type/Date       "DATE"
-   :type/DateTime   "TIMESTAMP"
-   :type/Decimal    "DECIMAL"
-   :type/Float      "DOUBLE"
-   :type/Integer    "INTEGER"
-   :type/Text       "TEXT"
-   :type/Time       "TIME"})
+  {:type/BigInteger     "BIGINT"
+   :type/Boolean        "BOOLEAN" ; Synonym of TINYINT(1)
+   :type/Date           "DATE"
+   :type/DateTime       "TIMESTAMP"
+   :type/DateTimeWithTZ "TIMESTAMP"
+   :type/Decimal        "DECIMAL"
+   :type/Float          "DOUBLE"
+   :type/Integer        "INTEGER"
+   :type/Text           "TEXT"
+   :type/Time           "TIME"})
 
 (defn- database->connection-details [context {:keys [database-name]}]
   (merge {:host         (i/db-test-env-var-or-throw :mysql :host "localhost")
           :port         (i/db-test-env-var-or-throw :mysql :port 3306)
           :user         (i/db-test-env-var :mysql :user "root")
-          :timezone     :America/Los_Angeles}
+          :timezone     :America/Los_Angeles
+;;          :serverTimezone "UTC"
+          }
          (when-let [password (i/db-test-env-var :mysql :password)]
            {:password password})
          (when (= context :db)
@@ -34,6 +39,7 @@
 (defn- quote-name [nm]
   (str \` nm \`))
 
+
 (u/strict-extend (class (mysql/->MySQLDriver))
   generic/IGenericSQLTestExtensions
   (merge generic/DefaultsMixin

test/metabase/test/data/postgres.clj

@@ -7,17 +7,18 @@
   (:import metabase.driver.postgres.PostgresDriver))
 
 (def ^:private ^:const field-base-type->sql-type
-  {:type/BigInteger "BIGINT"
-   :type/Boolean    "BOOL"
-   :type/Date       "DATE"
-   :type/DateTime   "TIMESTAMP"
-   :type/Decimal    "DECIMAL"
-   :type/Float      "FLOAT"
-   :type/Integer    "INTEGER"
-   :type/IPAddress  "INET"
-   :type/Text       "TEXT"
-   :type/Time       "TIME"
-   :type/UUID       "UUID"})
+  {:type/BigInteger     "BIGINT"
+   :type/Boolean        "BOOL"
+   :type/Date           "DATE"
+   :type/DateTime       "TIMESTAMP"
+   :type/DateTimeWithTZ "TIMESTAMP WITH TIME ZONE"
+   :type/Decimal        "DECIMAL"
+   :type/Float          "FLOAT"
+   :type/Integer        "INTEGER"
+   :type/IPAddress      "INET"
+   :type/Text           "TEXT"
+   :type/Time           "TIME"
+   :type/UUID           "UUID"})
 
 (defn- database->connection-details [context {:keys [database-name]}]
   (merge {:host     (i/db-test-env-var-or-throw :postgresql :host "localhost")