Add SimpleDateFormat as a fallback to the JodaTime for db timezones [ci drivers]

Some timezone abbreviations, especially non-US timezone abbreviations
aren't parsed by JodaTime. The SimpleDateFormat parser is more
flexible with these. This commit adds a SimpleDateFormat fallback to
the JodaTime formatters so if the JodaTime one fails, it will try
SimpleDateFormat ones next, hopefully figuring out what timezone the
database is in.

Fixes #6354, fixes #6085

src/metabase/driver.clj

@@ -10,7 +10,10 @@
 
   This namespace also contains various other functions for fetching drivers, testing database connections, and the
   like."
-  (:require [clj-time.format :as tformat]
+  (:require [clj-time
+             [coerce :as tcoerce]
+             [core :as time]
+             [format :as tformat]]
             [clojure.tools.logging :as log]
             [medley.core :as m]
             [metabase.config :as config]
@@ -24,6 +27,7 @@
             [schema.core :as s]
             [toucan.db :as db])
   (:import clojure.lang.Keyword
+           java.text.SimpleDateFormat
            metabase.models.database.DatabaseInstance
            metabase.models.field.FieldInstance
            metabase.models.table.TableInstance
@@ -285,26 +289,47 @@
       (when-not (empty? report-tz)
         report-tz))))
 
-(defn create-db-time-formatter
-  "Creates a date formatter from `DATE-FORMAT-STR` that will preserve
-  the offset/timezone information. Results of this are threadsafe and
-  can safely be def'd"
+(defprotocol ^:private ParseDateTimeString
+  (^:private parse [this date-time-str] "Parse the `date-time-str` and return a `DateTime` instance"))
+
+(extend-protocol ParseDateTimeString
+  DateTimeFormatter
+  (parse [formatter date-time-str]
+    (tformat/parse formatter date-time-str)))
+
+;; Java's SimpleDateFormat is more flexible on what it accepts for a time zone identifier. As an example, CEST is not
+;; recognized by Joda's DateTimeFormatter but is recognized by Java's SimpleDateFormat. This defrecord is used to
+;; dispatch parsing for SimpleDateFormat instances. Dispatching off of the SimpleDateFormat directly wouldn't be good
+;; as it's not threadsafe. This will always create a new SimpleDateFormat instance and discard it after parsing the
+;; date
+(defrecord ^:private ThreadSafeSimpleDateFormat [format-str]
+  ParseDateTimeString
+  (parse [_ date-time-str]
+    (let [sdf         (SimpleDateFormat. format-str)
+          parsed-date (.parse sdf date-time-str)
+          joda-tz     (-> sdf .getTimeZone .getID time/time-zone-for-id)]
+      (time/to-time-zone (tcoerce/from-date parsed-date) joda-tz))))
+
+(defn create-db-time-formatters
+  "Creates date formatters from `DATE-FORMAT-STR` that will preserve the offset/timezone information. Will return a
+  JodaTime date formatter and a core Java SimpleDateFormat. Results of this are threadsafe and can safely be def'd."
   [date-format-str]
-  (.withOffsetParsed ^DateTimeFormatter (tformat/formatter date-format-str)))
+  [(.withOffsetParsed ^DateTimeFormatter (tformat/formatter date-format-str))
+   (ThreadSafeSimpleDateFormat. date-format-str)])
 
 (defn- first-successful-parse
   "Attempt to parse `time-str` with each of `date-formatters`, returning the first successful parse. If there are no
   successful parses throws the exception that the last formatter threw."
   [date-formatters time-str]
-  (or (some #(u/ignore-exceptions (tformat/parse % time-str)) date-formatters)
+  (or (some #(u/ignore-exceptions (parse % time-str)) date-formatters)
       (doseq [formatter (reverse date-formatters)]
-        (tformat/parse formatter time-str))))
+        (parse formatter time-str))))
 
 (defn make-current-db-time-fn
   "Takes a clj-time date formatter `DATE-FORMATTER` and a native query
   for the current time. Returns a function that executes the query and
   parses the date returned preserving it's timezone"
-  [native-query date-formatter & more-date-formatters]
+  [native-query date-formatters]
   (fn [driver database]
     (let [settings (when-let [report-tz (report-timezone-if-supported driver)]
                      {:settings {:report-timezone report-tz}})
@@ -319,7 +344,7 @@
                          (format "Error querying database '%s' for current time" (:name database)) e))))]
       (try
         (when time-str
-          (first-successful-parse (cons date-formatter more-date-formatters) time-str))
+          (first-successful-parse date-formatters time-str))
         (catch Exception e
           (throw
            (Exception.

src/metabase/driver/bigquery.clj

@@ -465,7 +465,7 @@
 
 
 ;; BigQuery doesn't return a timezone with it's time strings as it's always UTC, JodaTime parsing also defaults to UTC
-(def ^:private bigquery-date-formatter (driver/create-db-time-formatter "yyyy-MM-dd HH:mm:ss.SSSSSS"))
+(def ^:private bigquery-date-formatters (driver/create-db-time-formatters "yyyy-MM-dd HH:mm:ss.SSSSSS"))
 (def ^:private bigquery-db-time-query "select CAST(CURRENT_TIMESTAMP() AS STRING)")
 
 (def ^:private driver (BigQueryDriver.))
@@ -534,7 +534,7 @@
                                                              #{:foreign-keys})))
           :format-custom-field-name (u/drop-first-arg format-custom-field-name)
           :mbql->native             (u/drop-first-arg mbql->native)
-          :current-db-time          (driver/make-current-db-time-fn  bigquery-db-time-query bigquery-date-formatter)}))
+          :current-db-time          (driver/make-current-db-time-fn  bigquery-db-time-query bigquery-date-formatters)}))
 
 (defn -init-driver
   "Register the BigQuery driver"

src/metabase/driver/crate.clj

@@ -97,7 +97,7 @@
   clojure.lang.Named
   (getName [_] "Crate"))
 
-(def ^:private crate-date-formatter (driver/create-db-time-formatter "yyyy-MM-dd HH:mm:ss.SSSSSSZ"))
+(def ^:private crate-date-formatters (driver/create-db-time-formatters "yyyy-MM-dd HH:mm:ss.SSSSSSZ"))
 (def ^:private crate-db-time-query "select DATE_FORMAT(current_timestamp, '%Y-%m-%d %H:%i:%S.%fZ')")
 
 (u/strict-extend CrateDriver
@@ -110,7 +110,7 @@
                                          :display-name "Hosts"
                                          :default      "localhost:5432/"}])
           :features        (comp (u/rpartial disj :foreign-keys) sql/features)
-          :current-db-time (driver/make-current-db-time-fn crate-db-time-query crate-date-formatter)})
+          :current-db-time (driver/make-current-db-time-fn crate-db-time-query crate-date-formatters)})
   sql/ISQLDriver
   (merge (sql/ISQLDriverDefaultsMixin)
          {:connection-details->spec  (u/drop-first-arg connection-details->spec)

src/metabase/driver/h2.clj

@@ -205,7 +205,7 @@
   (hsql/call :length field-key))
 
 (def ^:private date-format-str   "yyyy-MM-dd HH:mm:ss.SSS zzz")
-(def ^:private h2-date-formatter (driver/create-db-time-formatter date-format-str))
+(def ^:private h2-date-formatters (driver/create-db-time-formatters date-format-str))
 (def ^:private h2-db-time-query  (format "select formatdatetime(current_timestamp(),'%s') AS VARCHAR" date-format-str))
 
 (defrecord H2Driver []
@@ -222,7 +222,7 @@
                                                            :required     true}])
           :humanize-connection-error-message (u/drop-first-arg humanize-connection-error-message)
           :process-query-in-context          (u/drop-first-arg process-query-in-context)
-          :current-db-time                   (driver/make-current-db-time-fn h2-db-time-query h2-date-formatter)})
+          :current-db-time                   (driver/make-current-db-time-fn h2-db-time-query h2-date-formatters)})
 
   sql/ISQLDriver
   (merge (sql/ISQLDriverDefaultsMixin)

src/metabase/driver/mysql.clj

@@ -213,13 +213,11 @@
 (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-offset-date-formatter
-  "In some timezones, MySQL doesn't return a timezone description but rather a truncated offset, such as '-02'. That
-  offset will fail to parse using a regular formatter"
-  (driver/create-db-time-formatter "yyyy-MM-dd HH:mm:ss.SSSSSS Z"))
+(def ^:private mysql-date-formatters
+  (concat (driver/create-db-time-formatters "yyyy-MM-dd HH:mm:ss.SSSSSS zzz")
+          ;;In some timezones, MySQL doesn't return a timezone description but rather a truncated offset, such as '-02'. That
+          ;;offset will fail to parse using a regular formatter
+          (driver/create-db-time-formatters "yyyy-MM-dd HH:mm:ss.SSSSSS Z")))
 
 (def ^:private mysql-db-time-query
   "select CONCAT(DATE_FORMAT(current_timestamp, '%Y-%m-%d %H:%i:%S.%f' ), ' ', @@system_time_zone)")
@@ -257,7 +255,7 @@
                                                              :display-name "Additional JDBC connection string options"
                                                              :placeholder  "tinyInt1isBit=false"}]))
           :humanize-connection-error-message (u/drop-first-arg humanize-connection-error-message)
-          :current-db-time                   (driver/make-current-db-time-fn mysql-db-time-query mysql-date-formatter mysql-offset-date-formatter)})
+          :current-db-time                   (driver/make-current-db-time-fn mysql-db-time-query mysql-date-formatters)})
 
   sql/ISQLDriver
   (merge (sql/ISQLDriverDefaultsMixin)

src/metabase/driver/oracle.clj

@@ -255,7 +255,7 @@
     "You must specify the SID and/or the Service Name."
     message))
 
-(def ^:private oracle-date-formatter (driver/create-db-time-formatter "yyyy-MM-dd HH:mm:ss.SSS zzz"))
+(def ^:private oracle-date-formatters (driver/create-db-time-formatters "yyyy-MM-dd HH:mm:ss.SSS zzz"))
 (def ^:private oracle-db-time-query "select to_char(current_timestamp, 'YYYY-MM-DD HH24:MI:SS.FF3 TZD') FROM DUAL")
 
 (u/strict-extend OracleDriver
@@ -287,7 +287,7 @@
                                                              :placeholder  "*******"}]))
           :execute-query                     (comp remove-rownum-column sqlqp/execute-query)
           :humanize-connection-error-message (u/drop-first-arg humanize-connection-error-message)
-          :current-db-time                   (driver/make-current-db-time-fn oracle-db-time-query oracle-date-formatter)})
+          :current-db-time                   (driver/make-current-db-time-fn oracle-db-time-query oracle-date-formatters)})
 
   sql/ISQLDriver
   (merge (sql/ISQLDriverDefaultsMixin)

src/metabase/driver/postgres.clj

@@ -224,7 +224,7 @@
   (sql/describe-table (assoc driver :enum-types (enum-types database)) database table))
 
 
-(def ^:private pg-date-formatter (driver/create-db-time-formatter "yyyy-MM-dd HH:mm:ss.SSS zzz"))
+(def ^:private pg-date-formatters (driver/create-db-time-formatters "yyyy-MM-dd HH:mm:ss.SSS zzz"))
 (def ^:private pg-db-time-query "select to_char(current_timestamp, 'YYYY-MM-DD HH24:MI:SS.MS TZ')")
 
 (def PostgresISQLDriverMixin
@@ -242,7 +242,7 @@
 (u/strict-extend PostgresDriver
   driver/IDriver
   (merge (sql/IDriverSQLDefaultsMixin)
-         {:current-db-time                   (driver/make-current-db-time-fn pg-db-time-query pg-date-formatter)
+         {:current-db-time                   (driver/make-current-db-time-fn pg-db-time-query pg-date-formatters)
           :date-interval                     (u/drop-first-arg date-interval)
           :describe-table                    describe-table
           :details-fields                    (constantly (ssh/with-tunnel-config

src/metabase/driver/presto.clj

@@ -261,7 +261,7 @@
 
 ;;; Driver implementation
 
-(def ^:private presto-date-formatter (driver/create-db-time-formatter "yyyy-MM-dd'T'HH:mm:ss.SSSZ"))
+(def ^:private presto-date-formatters (driver/create-db-time-formatters "yyyy-MM-dd'T'HH:mm:ss.SSSZ"))
 (def ^:private presto-db-time-query "select to_iso8601(current_timestamp)")
 
 (u/strict-extend PrestoDriver
@@ -308,7 +308,7 @@
                                                                       ;; during unit tests don't treat presto as having FK support
                                                                       #{:foreign-keys})))
           :humanize-connection-error-message (u/drop-first-arg humanize-connection-error-message)
-          :current-db-time                   (driver/make-current-db-time-fn presto-db-time-query presto-date-formatter)})
+          :current-db-time                   (driver/make-current-db-time-fn presto-db-time-query presto-date-formatters)})
 
   sql/ISQLDriver
   (merge (sql/ISQLDriverDefaultsMixin)

src/metabase/driver/redshift.clj

@@ -72,7 +72,7 @@
 
 ;; The docs say TZ should be allowed at the end of the format string, but it doesn't appear to work
 ;; Redshift is always in UTC and doesn't return it's timezone
-(def ^:private redshift-date-formatter (driver/create-db-time-formatter "yyyy-MM-dd HH:mm:ss.SSS zzz"))
+(def ^:private redshift-date-formatters (driver/create-db-time-formatters "yyyy-MM-dd HH:mm:ss.SSS zzz"))
 (def ^:private redshift-db-time-query "select to_char(current_timestamp, 'YYYY-MM-DD HH24:MI:SS.MS TZ')")
 
 (u/strict-extend RedshiftDriver
@@ -103,7 +103,7 @@
                                                     :placeholder  "*******"
                                                     :required     true}]))
           :format-custom-field-name (u/drop-first-arg str/lower-case)
-          :current-db-time          (driver/make-current-db-time-fn redshift-db-time-query redshift-date-formatter)})
+          :current-db-time          (driver/make-current-db-time-fn redshift-db-time-query redshift-date-formatters)})
 
   sql/ISQLDriver
   (merge postgres/PostgresISQLDriverMixin

src/metabase/driver/sqlite.clj

@@ -158,7 +158,7 @@
 
 
 ;; SQLite defaults everything to UTC
-(def ^:private sqlite-date-formatter (driver/create-db-time-formatter "yyyy-MM-dd HH:mm:ss"))
+(def ^:private sqlite-date-formatters (driver/create-db-time-formatters "yyyy-MM-dd HH:mm:ss"))
 (def ^:private sqlite-db-time-query "select cast(datetime('now') as text);")
 
 (u/strict-extend SQLiteDriver
@@ -178,7 +178,7 @@
                                              ;; the foreign key stuff in the tests.
                                              (when config/is-test?
                                                #{:foreign-keys})))
-          :current-db-time (driver/make-current-db-time-fn sqlite-db-time-query sqlite-date-formatter)})
+          :current-db-time (driver/make-current-db-time-fn sqlite-db-time-query sqlite-date-formatters)})
   sql/ISQLDriver
   (merge (sql/ISQLDriverDefaultsMixin)
          {:active-tables             sql/post-filtered-active-tables

src/metabase/driver/sqlserver.clj

@@ -159,7 +159,7 @@
   (hsql/call :len (hx/cast :VARCHAR field-key)))
 
 
-(def ^:private sqlserver-date-formatter (driver/create-db-time-formatter "yyyy-MM-dd'T'HH:mm:ss.SSSSSSSSZ"))
+(def ^:private sqlserver-date-formatters (driver/create-db-time-formatters "yyyy-MM-dd'T'HH:mm:ss.SSSSSSSSZ"))
 (def ^:private sqlserver-db-time-query "select CONVERT(nvarchar(30), SYSDATETIMEOFFSET(), 127)")
 
 (u/strict-extend SQLServerDriver
@@ -199,7 +199,7 @@
                                          {:name         "additional-options"
                                           :display-name "Additional JDBC connection string options"
                                           :placeholder  "trustServerCertificate=false"}]))
-          :current-db-time (driver/make-current-db-time-fn sqlserver-db-time-query sqlserver-date-formatter)})
+          :current-db-time (driver/make-current-db-time-fn sqlserver-db-time-query sqlserver-date-formatters)})
 
 
   sql/ISQLDriver

src/metabase/driver/vertica.clj

@@ -110,7 +110,7 @@
   clojure.lang.Named
   (getName [_] "Vertica"))
 
-(def ^:private vertica-date-formatter (driver/create-db-time-formatter "yyyy-MM-dd HH:mm:ss z"))
+(def ^:private vertica-date-formatters (driver/create-db-time-formatters "yyyy-MM-dd HH:mm:ss z"))
 (def ^:private vertica-db-time-query "select to_char(CURRENT_TIMESTAMP, 'YYYY-MM-DD HH24:MI:SS TZ')")
 
 (u/strict-extend VerticaDriver
@@ -141,7 +141,7 @@
                                             {:name         "additional-options"
                                              :display-name "Additional JDBC connection string options"
                                              :placeholder  "ConnectionLoadBalance=1"}]))
-          :current-db-time   (driver/make-current-db-time-fn vertica-db-time-query vertica-date-formatter)})
+          :current-db-time   (driver/make-current-db-time-fn vertica-db-time-query vertica-date-formatters)})
   sql/ISQLDriver
   (merge (sql/ISQLDriverDefaultsMixin)
          {:column->base-type         (u/drop-first-arg column->base-type)

test/metabase/driver/mysql_test.clj

@@ -92,6 +92,11 @@
   (with-redefs [metabase.driver/execute-query (constantly {:rows [["2018-01-09 18:39:08.000000 -02"]]})]
     (tu/db-timezone-id)))
 
+(expect-with-engine :mysql
+  "Europe/Paris"
+  (with-redefs [metabase.driver/execute-query (constantly {:rows [["2018-01-08 23:00:00.008 CET"]]})]
+    (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")