Skip to content
Snippets Groups Projects
Unverified Commit f4729305 authored by Cam Saul's avatar Cam Saul
Browse files

Fix MySQL timezones by using datetime literals [ci drivers]

parent 9c3be9ca
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/driver/mysql.clj
+ 78
− 12
View file @ f4729305
(ns metabase.driver.mysql
(:require [clojure
"MySQL driver. Builds off of the Generic SQL driver."
(:require [clj-time
[core :as t]
[format :as time]]
[clojure
[set :as set]
[string :as s]]
[honeysql.core :as hsql]
......@@ -8,12 +12,20 @@
[util :as u]]
[metabase.db.spec :as dbspec]
[metabase.driver.generic-sql :as sql]
[metabase.driver.generic-sql.query-processor :as sqlqp]
[metabase.util
[honeysql-extensions :as hx]
[ssh :as ssh]]))
[ssh :as ssh]])
(:import [java.util Date TimeZone]
org.joda.time.format.DateTimeFormatter))
(defrecord MySQLDriver []
clojure.lang.Named
(getName [_] "MySQL"))
;;; # IMPLEMENTATION
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | METHOD IMPLS |
;;; +----------------------------------------------------------------------------------------------------------------+
(defn- column->base-type [column-type]
({:BIGINT :type/BigInteger
......@@ -51,12 +63,16 @@
(def ^:private ^:const default-connection-args
"Map of args for the MySQL JDBC connection string.
Full list of is options is available here: http://dev.mysql.com/doc/connector-j/6.0/en/connector-j-reference-configuration-properties.html"
{:zeroDateTimeBehavior :convertToNull ; 0000-00-00 dates are valid in MySQL; convert these to `null` when they come back because they're illegal in Java
:useUnicode :true ; Force UTF-8 encoding of results
{;; 0000-00-00 dates are valid in MySQL; convert these to `null` when they come back because they're illegal in Java
:zeroDateTimeBehavior :convertToNull
;; Force UTF-8 encoding of results
:useUnicode :true
:characterEncoding :UTF8
:characterSetResults :UTF8
:useLegacyDatetimeCode :true ; Needs to be true to set useJDBCCompliantTimezoneShift to true
:useJDBCCompliantTimezoneShift :true}) ; This allows us to adjust the timezone of timestamps as we pull them from the resultset
;; Needs to be true to set useJDBCCompliantTimezoneShift to true
:useLegacyDatetimeCode :true
;; This allows us to adjust the timezone of timestamps as we pull them from the resultset
:useJDBCCompliantTimezoneShift :true})
(def ^:private ^:const ^String default-connection-args-string
(s/join \& (for [[k v] default-connection-args]
......@@ -86,6 +102,52 @@
(defn- date-format [format-str expr] (hsql/call :date_format expr (hx/literal format-str)))
(defn- str-to-date [format-str expr] (hsql/call :str_to_date expr (hx/literal format-str)))
(def ^:private ^DateTimeFormatter timezone-offset-formatter
"JodaTime formatter that returns just the raw timezone offset, e.g. `-08:00` or `+00:00`."
(time/formatter "ZZ"))
(defn- timezone-id->offset-str
"Get an appropriate timezone offset string for a timezone with `timezone-id`. MySQL only accepts these offsets as
strings like `-8:00`.
(timezone-id->offset-str \"US/Pacific\") ; -> \"-08:00\"
Returns `nil` if `timezone-id` is itself `nil`."
[^String timezone-id]
(when timezone-id
(time/unparse (.withZone timezone-offset-formatter (t/time-zone-for-id timezone-id)) (t/now))))
(def ^:private ^String system-timezone-offset-str
(timezone-id->offset-str (.getID (TimeZone/getDefault))))
;; 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 [report-timezone-offset-str (timezone-id->offset-str (driver/report-timezone))]
(if (and report-timezone-offset-str
(not= report-timezone-offset-str system-timezone-offset-str))
;; if we have a report timezone we want to generate SQL like convert_tz('2004-01-01T12:00:00','-8:00','-2:00')
;; to convert our timestamp from system timezone -> report timezone.
;; See https://dev.mysql.com/doc/refman/5.7/en/date-and-time-functions.html#function_convert-tz
;; (We're using raw offsets for the JVM timezone instead of the timezone ID because we can't be 100% sure that
;; MySQL will accept either of our timezone IDs as valid.)
;;
;; Note there's a small chance that report timezone will never be set on the MySQL connection, if attempting to
;; do so fails because the ID is valid; if the report timezone is different from the MySQL database's timezone,
;; this will result in the `convert_tz()` call below being incorrect. Unfortunately we don't currently have a
;; way to determine that setting a timezone has failed for the current query, since it actualy is attempted
;; after the query is compiled. Hopefully situtations where that happens are rare; at any rate it's probably
;; 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 (u/format-date :date-hour-minute-second-ms date))
(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)))
;; Since MySQL doesn't have date_trunc() we fake it by formatting a date to an appropriate string and then converting
;; back to a date. See http://dev.mysql.com/doc/refman/5.6/en/date-and-time-functions.html#function_date-format for an
;; explanation of format specifiers
......@@ -151,12 +213,16 @@
(defn- string-length-fn [field-key]
(hsql/call :char_length field-key))
(def ^:private mysql-date-formatter (driver/create-db-time-formatter "yyyy-MM-dd HH:mm:ss.SSSSSS zzz"))
(def ^:private mysql-db-time-query "select CONCAT(DATE_FORMAT(current_timestamp, '%Y-%m-%d %H:%i:%S.%f' ), ' ', @@system_time_zone)")
(def ^:private mysql-date-formatter
(driver/create-db-time-formatter "yyyy-MM-dd HH:mm:ss.SSSSSS zzz"))
(defrecord MySQLDriver []
clojure.lang.Named
(getName [_] "MySQL"))
(def ^:private mysql-db-time-query
"select CONCAT(DATE_FORMAT(current_timestamp, '%Y-%m-%d %H:%i:%S.%f' ), ' ', @@system_time_zone)")
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | IDRIVER & ISQLDRIVER METHOD MAPS |
;;; +----------------------------------------------------------------------------------------------------------------+
(u/strict-extend MySQLDriver
driver/IDriver
......
src/metabase/sync/fetch_metadata.clj
+ 7
− 12
View file @ f4729305
(ns metabase.sync.fetch-metadata
"Fetch metadata functions fetch 'snapshots' of the schema for a data warehouse database, including
information about tables, schemas, and fields, and their types.
For example, with SQL databases, these functions use the JDBC DatabaseMetaData to get this information."
"Fetch metadata functions fetch 'snapshots' of the schema for a data warehouse database, including information about
tables, schemas, and fields, and their types. For example, with SQL databases, these functions use the JDBC
DatabaseMetaData to get this information."
(:require [metabase.driver :as driver]
[metabase.sync.interface :as i]
[schema.core :as s])
(:import [org.joda.time DateTime]))
(:import org.joda.time.DateTime))
(s/defn db-metadata :- i/DatabaseMetadata
"Get basic Metadata about a DATABASE and its Tables. Doesn't include information about the Fields."
"Get basic Metadata about a `database` and its Tables. Doesn't include information about the Fields."
[database :- i/DatabaseInstance]
(driver/describe-database (driver/->driver database) database))
(s/defn table-metadata :- i/TableMetadata
"Get more detailed information about a TABLE belonging to DATABASE. Includes information about the Fields."
"Get more detailed information about a `table` belonging to `database`. Includes information about the Fields."
[database :- i/DatabaseInstance, table :- i/TableInstance]
(driver/describe-table (driver/->driver database) database table))
(s/defn fk-metadata :- i/FKMetadata
"Get information about the foreign keys belonging to TABLE."
"Get information about the foreign keys belonging to `table`."
[database :- i/DatabaseInstance, table :- i/TableInstance]
(let [driver (driver/->driver database)]
(when (driver/driver-supports? driver :foreign-keys)
(driver/describe-table-fks driver database table))))
(s/defn db-timezone :- i/TimeZoneId
[database :- i/DatabaseInstance]
(let [db-time ^DateTime (driver/current-db-time (driver/->driver database) database)]
(-> db-time .getChronology .getZone .getID)))
test/metabase/driver/mysql_test.clj
+ 37
− 5
View file @ f4729305
......@@ -3,7 +3,10 @@
[metabase
[sync :as sync]
[util :as u]]
[metabase.driver.generic-sql :as sql]
[metabase.driver
[generic-sql :as sql]
[mysql :as mysql]]
[metabase.driver.generic-sql.query-processor :as sqlqp]
[metabase.models.database :refer [Database]]
[metabase.test
[data :as data]
......@@ -11,11 +14,15 @@
[metabase.test.data
[datasets :refer [expect-with-engine]]
[interface :refer [def-database-definition]]]
[metabase.test.util :as tu]
[metabase.util :as u]
[honeysql.core :as hsql]
[toucan.db :as db]
[toucan.util.test :as tt])
(:import metabase.driver.mysql.MySQLDriver))
;; MySQL allows 0000-00-00 dates, but JDBC does not; make sure that MySQL is converting them to NULL when returning them like we asked
;; MySQL allows 0000-00-00 dates, but JDBC does not; make sure that MySQL is converting them to NULL when returning
;; them like we asked
(def-database-definition ^:private ^:const all-zero-dates
["exciting-moments-in-history"
[{:field-name "moment", :base-type :type/DateTime}]
......@@ -23,7 +30,8 @@
(expect-with-engine :mysql
[[1 nil]]
;; TODO - use the `rows` function from `metabse.query-processor-test`. Preferrably after it's moved to some sort of shared test util namespace
;; TODO - use the `rows` function from `metabse.query-processor-test`. Preferrably after it's moved to some sort of
;; shared test util namespace
(-> (data/dataset metabase.driver.mysql-test/all-zero-dates
(data/run-query exciting-moments-in-history))
:data :rows))
......@@ -40,8 +48,9 @@
:additional-options "tinyInt1isBit=false"})))
;; Test how TINYINT(1) columns are interpreted. By default, they should be interpreted as integers,
;; but with the correct additional options, we should be able to change that -- see https://github.com/metabase/metabase/issues/3506
;; Test how TINYINT(1) columns are interpreted. By default, they should be interpreted as integers, but with the
;; correct additional options, we should be able to change that -- see
;; https://github.com/metabase/metabase/issues/3506
(def-database-definition ^:private ^:const tiny-int-ones
["number-of-cans"
[{:field-name "thing", :base-type :type/Text}
......@@ -77,3 +86,26 @@
(expect-with-engine :mysql
"America/Los_Angeles"
(tu/db-timezone-id))
(expect (#'mysql/timezone-id->offset-str "US/Pacific") "-08:00")
(expect (#'mysql/timezone-id->offset-str "UTC") "+00:00")
(expect (#'mysql/timezone-id->offset-str "America/Los_Angeles") "-08:00")
;; make sure DateTime types generate appropriate SQL...
;; ...with no report-timezone set
(expect
["?" (u/->Timestamp "2018-01-03")]
(tu/with-temporary-setting-values [report-timezone nil]
(hsql/format (sqlqp/->honeysql (MySQLDriver.) (u/->Timestamp "2018-01-03")))))
;; ...with a report-timezone set
(expect
["convert_tz('2018-01-03T00:00:00.000', '+00:00', '-08:00')"]
(tu/with-temporary-setting-values [report-timezone "US/Pacific"]
(hsql/format (sqlqp/->honeysql (MySQLDriver.) (u/->Timestamp "2018-01-03")))))
;; ...with a report-timezone set to the same as the system timezone (shouldn't need to do TZ conversion)
(expect
["?" (u/->Timestamp "2018-01-03")]
(tu/with-temporary-setting-values [report-timezone "UTC"]
(hsql/format (sqlqp/->honeysql (MySQLDriver.) (u/->Timestamp "2018-01-03")))))
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment