Fix reading MySQL timediff() results when values are out of range for a LocalTime (#12447)

* Convert metabase.driver.mysql-test to modern test style

* Fix reading MySQL TIME values that have < 0 or > 23 for hour
[ci mysql] [ci mariadb]

src/metabase/driver/mysql.clj

@@ -338,15 +338,35 @@
 
 ;; MySQL TIMESTAMPS are actually TIMESTAMP WITH LOCAL TIME ZONE, i.e. they are stored normalized to UTC when stored.
 ;; However, MySQL returns them in the report time zone in an effort to make our lives horrible.
+(defmethod sql-jdbc.execute/read-column-thunk [:mysql Types/TIMESTAMP]
+  [_ ^ResultSet rs ^ResultSetMetaData rsmeta ^Integer i]
+  ;; Check and see if the column type is `TIMESTAMP` (as opposed to `DATETIME`, which is the equivalent of
+  ;; LocalDateTime), and normalize it to a UTC timestamp if so.
+  (if (= (.getColumnTypeName rsmeta i) "TIMESTAMP")
+    (fn read-timestamp-thunk []
+      (when-let [t (.getObject rs i LocalDateTime)]
+        (t/with-offset-same-instant (t/offset-date-time t (t/zone-id (qp.timezone/results-timezone-id))) (t/zone-offset 0))))
+    (fn read-datetime-thunk []
+      (.getObject rs i LocalDateTime))))
+
+;; Results of `timediff()` might come back as negative values, or might come back as values that aren't valid
+;; `LocalTime`s e.g. `-01:00:00` or `25:00:00`.
 ;;
-;; Check and see if the column type is `TIMESTAMP` (as opposed to `DATETIME`, which is the equivalent of
-;; LocalDateTime), and normalize it to a UTC timestamp if so.
-(defmethod sql-jdbc.execute/read-column [:mysql Types/TIMESTAMP]
-  [_ _ ^ResultSet rs ^ResultSetMetaData rsmeta ^Integer i]
-  (when-let [t (.getObject rs i LocalDateTime)]
-    (if (= (.getColumnTypeName rsmeta i) "TIMESTAMP")
-      (t/with-offset-same-instant (t/offset-date-time t (t/zone-id (qp.timezone/results-timezone-id))) (t/zone-offset 0))
-      t)))
+;; There is currently no way to tell whether the column is the result of a `timediff()` call (i.e., a duration) or a
+;; normal `LocalTime` -- JDBC doesn't have interval/duration type enums. `java.time.LocalTime`only accepts values of
+;; hour between 0 and 23 (inclusive). The MariaDB JDBC driver's implementations of `(.getObject rs i
+;; java.time.LocalTime)` will throw Exceptions theses cases.
+;;
+;; Thus we should attempt to fetch temporal results the normal way and fall back to string representations for cases
+;; where the values are unparseable.
+(defmethod sql-jdbc.execute/read-column-thunk [:mysql Types/TIME]
+  [driver ^ResultSet rs rsmeta ^Integer i]
+  (let [parent-thunk ((get-method sql-jdbc.execute/read-column-thunk [:sql-jdbc Types/TIME]) driver rs rsmeta i)]
+    (fn read-time-thunk []
+      (try
+        (parent-thunk)
+        (catch Throwable _
+          (.getString rs i))))))
 
 (defn- format-offset [t]
   (let [offset (t/format "ZZZZZ" (t/zone-offset t))]

test/metabase/driver/mysql_test.clj

@@ -3,53 +3,47 @@
              [string :as str]
              [test :refer :all]]
             [clojure.java.jdbc :as jdbc]
-            [expectations :refer [expect]]
             [metabase
              [driver :as driver]
              [query-processor :as qp]
-             [query-processor-test :as qp.test]
              [sync :as sync]
+             [test :as mt]
              [util :as u]]
             [metabase.driver.sql-jdbc.connection :as sql-jdbc.conn]
             [metabase.models
              [database :refer [Database]]
              [field :refer [Field]]]
-            [metabase.test
-             [data :as data]
-             [util :as tu]]
-            [metabase.test.data
-             [datasets :as datasets :refer [expect-with-driver]]
-             [interface :as tx]]
-            [metabase.test.util.timezone :as tu.tz]
+            [metabase.test.data.interface :as tx]
             [toucan.db :as db]
             [toucan.util.test :as tt]))
 
-;; 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
-(expect-with-driver :mysql
-  [[1 nil]]
-  (let [spec (sql-jdbc.conn/connection-details->spec :mysql (tx/dbdef->connection-details :mysql :server nil))]
-    (try
-      ;; Create the DB
-      (doseq [sql ["DROP DATABASE IF EXISTS all_zero_dates;"
-                   "CREATE DATABASE all_zero_dates;"]]
-        (jdbc/execute! spec [sql]))
-      ;; Create Table & add data
-      (let [details (tx/dbdef->connection-details :mysql :db {:database-name "all_zero_dates"})
-            spec    (-> (sql-jdbc.conn/connection-details->spec :mysql details)
-                        ;; allow inserting dates where value is '0000-00-00' -- this is disallowed by default on newer
-                        ;; versions of MySQL, but we still want to test that we can handle it correctly for older ones
-                        (assoc :sessionVariables "sql_mode='ALLOW_INVALID_DATES'"))]
-        (doseq [sql ["CREATE TABLE `exciting-moments-in-history` (`id` integer, `moment` timestamp);"
-                     "INSERT INTO `exciting-moments-in-history` (`id`, `moment`) VALUES (1, '0000-00-00');"]]
+(deftest all-zero-dates-test
+  (mt/test-driver :mysql
+    (testing (str "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"))
+    (let [spec (sql-jdbc.conn/connection-details->spec :mysql (tx/dbdef->connection-details :mysql :server nil))]
+      (try
+        ;; Create the DB
+        (doseq [sql ["DROP DATABASE IF EXISTS all_zero_dates;"
+                     "CREATE DATABASE all_zero_dates;"]]
           (jdbc/execute! spec [sql]))
-        ;; create & sync MB DB
-        (tt/with-temp Database [database {:engine "mysql", :details details}]
-          (sync/sync-database! database)
-          (data/with-db database
-            ;; run the query
-            (qp.test/rows
-             (data/run-mbql-query exciting-moments-in-history))))))))
+        ;; Create Table & add data
+        (let [details (tx/dbdef->connection-details :mysql :db {:database-name "all_zero_dates"})
+              spec    (-> (sql-jdbc.conn/connection-details->spec :mysql details)
+                          ;; allow inserting dates where value is '0000-00-00' -- this is disallowed by default on newer
+                          ;; versions of MySQL, but we still want to test that we can handle it correctly for older ones
+                          (assoc :sessionVariables "sql_mode='ALLOW_INVALID_DATES'"))]
+          (doseq [sql ["CREATE TABLE `exciting-moments-in-history` (`id` integer, `moment` timestamp);"
+                       "INSERT INTO `exciting-moments-in-history` (`id`, `moment`) VALUES (1, '0000-00-00');"]]
+            (jdbc/execute! spec [sql]))
+          ;; create & sync MB DB
+          (tt/with-temp Database [database {:engine "mysql", :details details}]
+            (sync/sync-database! database)
+            (mt/with-db database
+              ;; run the query
+              (is (= [[1 nil]]
+                     (mt/rows
+                       (mt/run-mbql-query exciting-moments-in-history)))))))))))
 
 
 ;; Test how TINYINT(1) columns are interpreted. By default, they should be interpreted as integers, but with the
@@ -67,30 +61,29 @@
   (let [table-ids (db/select-ids 'Table :db_id (u/get-id db))]
     (set (map (partial into {}) (db/select [Field :name :base_type :special_type] :table_id [:in table-ids])))))
 
-;; By default TINYINT(1) should be a boolean
-(expect-with-driver :mysql
-  #{{:name "number-of-cans", :base_type :type/Boolean, :special_type :type/Category}
-    {:name "id",             :base_type :type/Integer, :special_type :type/PK}
-    {:name "thing",          :base_type :type/Text,    :special_type :type/Category}}
-  (data/dataset tiny-int-ones
-    (db->fields (data/db))))
-
-;; if someone says specifies `tinyInt1isBit=false`, it should come back as a number instead
-(expect-with-driver :mysql
-  #{{:name "number-of-cans", :base_type :type/Integer, :special_type :type/Quantity}
-    {:name "id",             :base_type :type/Integer, :special_type :type/PK}
-    {:name "thing",          :base_type :type/Text,    :special_type :type/Category}}
-  (data/dataset tiny-int-ones
-    (tt/with-temp Database [db {:engine "mysql"
-                                :details (assoc (:details (data/db))
-                                           :additional-options "tinyInt1isBit=false")}]
-      (sync/sync-database! db)
-      (db->fields db))))
+(deftest tiny-int-1-test
+  (mt/test-driver :mysql
+    (mt/dataset tiny-int-ones
+      (testing "By default TINYINT(1) should be a boolean"
+        (is (= #{{:name "number-of-cans", :base_type :type/Boolean, :special_type :type/Category}
+                 {:name "id", :base_type :type/Integer, :special_type :type/PK}
+                 {:name "thing", :base_type :type/Text, :special_type :type/Category}}
+               (db->fields (mt/db)))))
+
+      (testing "if someone says specifies `tinyInt1isBit=false`, it should come back as a number instead"
+        (tt/with-temp Database [db {:engine  "mysql"
+                                    :details (assoc (:details (mt/db))
+                                                    :additional-options "tinyInt1isBit=false")}]
+          (sync/sync-database! db)
+          (is (= #{{:name "number-of-cans", :base_type :type/Integer, :special_type :type/Quantity}
+                   {:name "id", :base_type :type/Integer, :special_type :type/PK}
+                   {:name "thing", :base_type :type/Text, :special_type :type/Category}}
+                 (db->fields db))))))))
 
 (deftest db-timezone-id-test
-  (datasets/test-driver :mysql
+  (mt/test-driver :mysql
     (let [timezone (fn [result-row]
-                     (let [db (data/db)]
+                     (let [db (mt/db)]
                        (with-redefs [jdbc/query (let [orig jdbc/query]
                                                   (fn [spec sql-args & options]
                                                     (if (and (string? sql-args)
@@ -116,7 +109,7 @@
 
     (testing "real timezone query doesn't fail"
       (is (nil? (try
-                  (driver/db-default-timezone driver/*driver* (data/db))
+                  (driver/db-default-timezone driver/*driver* (mt/db))
                   nil
                   (catch Throwable e
                     e)))))))
@@ -125,51 +118,42 @@
 (def ^:private before-daylight-savings #t "2018-03-10T10:00:00Z")
 (def ^:private after-daylight-savings  #t "2018-03-12T10:00:00Z")
 
-;; Most of our tests either deal in UTC (offset 00:00) or America/Los_Angeles timezones (-07:00/-08:00). When dealing
-;; with dates, we will often truncate the timestamp to a date. When we only test with negative timezone offsets, in
-;; combination with this truncation, means we could have a bug and it's hidden by this negative-only offset. As an
-;; example, if we have a datetime like 2018-08-17 00:00:00-08:00, converting to UTC this becomes 2018-08-17
-;; 08:00:00+00:00, which when truncated is still 2018-08-17. That same scenario in Hong Kong is 2018-08-17
-;; 00:00:00+08:00, which then becomes 2018-08-16 16:00:00+00:00 when converted to UTC, which will truncate to
-;; 2018-08-16, instead of 2018-08-17
-;;
-;; This test ensures if our JVM timezone and reporting timezone are Asia/Hong_Kong, we get a correctly formatted date
-(expect-with-driver :mysql
-  ["2018-04-18T00:00:00+08:00"]
-  (tu.tz/with-system-timezone-id "Asia/Hong_Kong"
-    (tu/with-temporary-setting-values [report-timezone "Asia/Hong_Kong"]
-      (qp.test/first-row
-       (identity #_du/with-effective-timezone #_(data/db)
-         (qp/process-query
-           {:database   (data/id)
-            :type       :native
-            :settings   {:report-timezone "UTC"}
-            :native     {:query         "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"}]}))))))
-
-;; This tests a similar scenario, but one in which the JVM timezone is in Hong Kong, but the report timezone is in Los
-;; Angeles. The Joda Time date parsing functions for the most part default to UTC. Our tests all run with a UTC JVM
-;; timezone. This test catches a bug where we are incorrectly assuming a date is in UTC when the JVM timezone is
-;; different.
-;;
-;; The original bug can be found here: https://github.com/metabase/metabase/issues/8262. The MySQL driver code was
-;; parsing the date using JodateTime's date parser, which is in UTC. The MySQL driver code was assuming that date was
-;; in the system timezone rather than UTC which caused an incorrect conversion and with the trucation, let to it being
-;; off by a day
-(expect-with-driver :mysql
-  ["2018-04-18T00:00:00-07:00"]
-  (tu.tz/with-system-timezone-id "Asia/Hong_Kong"
-    (tu/with-temporary-setting-values [report-timezone "America/Los_Angeles"]
-      (qp.test/first-row
-       (identity #_du/with-effective-timezone #_(data/db)
-         (qp/process-query
-           {:database   (data/id)
-            :type       :native
-            :settings   {:report-timezone "UTC"}
-            :native     {:query         "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"}]}))))))
+(deftest timezone-date-formatting-test
+  (mt/test-driver :mysql
+    ;; Most of our tests either deal in UTC (offset 00:00) or America/Los_Angeles timezones (-07:00/-08:00). When dealing
+    ;; with dates, we will often truncate the timestamp to a date. When we only test with negative timezone offsets, in
+    ;; combination with this truncation, means we could have a bug and it's hidden by this negative-only offset. As an
+    ;; example, if we have a datetime like 2018-08-17 00:00:00-08:00, converting to UTC this becomes 2018-08-17
+    ;; 08:00:00+00:00, which when truncated is still 2018-08-17. That same scenario in Hong Kong is 2018-08-17
+    ;; 00:00:00+08:00, which then becomes 2018-08-16 16:00:00+00:00 when converted to UTC, which will truncate to
+    ;; 2018-08-16, instead of 2018-08-17
+    (mt/with-system-timezone-id "Asia/Hong_Kong"
+      (letfn [(run-query-with-report-timezone [report-timezone]
+                (mt/with-temporary-setting-values [report-timezone report-timezone]
+                  (mt/first-row
+                    (qp/process-query
+                     {:database   (mt/id)
+                      :type       :native
+                      :settings   {:report-timezone "UTC"}
+                      :native     {:query         "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"}]}))))]
+        (testing "date formatting when system-timezone == report-timezone"
+          (is (= ["2018-04-18T00:00:00+08:00"]
+                 (run-query-with-report-timezone "Asia/Hong_Kong"))))
+
+        ;; This tests a similar scenario, but one in which the JVM timezone is in Hong Kong, but the report timezone
+        ;; is in Los Angeles. The Joda Time date parsing functions for the most part default to UTC. Our tests all run
+        ;; with a UTC JVM timezone. This test catches a bug where we are incorrectly assuming a date is in UTC when
+        ;; the JVM timezone is different.
+        ;;
+        ;; The original bug can be found here: https://github.com/metabase/metabase/issues/8262. The MySQL driver code
+        ;; was parsing the date using JodateTime's date parser, which is in UTC. The MySQL driver code was assuming
+        ;; that date was in the system timezone rather than UTC which caused an incorrect conversion and with the
+        ;; trucation, let to it being off by a day
+        (testing "date formatting when system-timezone != report-timezone"
+          (is (= ["2018-04-18T00:00:00-07:00"]
+                 (run-query-with-report-timezone "America/Los_Angeles"))))))))
 
 (def ^:private sample-connection-details
   {:db "my_db", :host "localhost", :port "3306", :user "cam", :password "bad-password"})
@@ -186,18 +170,41 @@
    :useCompression       true
    :useUnicode           true})
 
-;; Do `:ssl` connection details give us the connection spec we'd expect?
-(expect
-  (assoc sample-jdbc-spec :useSSL true)
-  (sql-jdbc.conn/connection-details->spec :mysql (assoc sample-connection-details :ssl true)))
-
-;; what about non-SSL connections?
-(expect
-  (assoc sample-jdbc-spec :useSSL false)
-  (sql-jdbc.conn/connection-details->spec :mysql sample-connection-details))
-
-;; Connections that are `:ssl false` but with `useSSL` in the additional options should be treated as SSL (see #9629)
-(expect
-  (assoc sample-jdbc-spec :useSSL true, :subname "//localhost:3306/my_db?useSSL=true&trustServerCertificate=true")
-  (sql-jdbc.conn/connection-details->spec :mysql
-    (assoc sample-connection-details :ssl false, :additional-options "useSSL=true&trustServerCertificate=true")))
+(deftest connection-spec-test
+  (testing "Do `:ssl` connection details give us the connection spec we'd expect?"
+    (= (assoc sample-jdbc-spec :useSSL true)
+       (sql-jdbc.conn/connection-details->spec :mysql (assoc sample-connection-details :ssl true))))
+
+  (testing "what about non-SSL connections?"
+    (is (= (assoc sample-jdbc-spec :useSSL false)
+           (sql-jdbc.conn/connection-details->spec :mysql sample-connection-details))))
+
+  (testing "Connections that are `:ssl false` but with `useSSL` in the additional options should be treated as SSL (see #9629)"
+    (is (= (assoc sample-jdbc-spec :useSSL true, :subname "//localhost:3306/my_db?useSSL=true&trustServerCertificate=true")
+           (sql-jdbc.conn/connection-details->spec :mysql
+             (assoc sample-connection-details
+                    :ssl false
+                    :additional-options "useSSL=true&trustServerCertificate=true"))))))
+
+(deftest read-timediffs-test
+  (mt/test-driver :mysql
+    (testing "Make sure negative result of *diff() functions don't cause Exceptions (#10983)"
+      (doseq [{:keys [interval expected message]}
+              [{:interval "-1 HOUR"
+                :expected "-01:00:00"
+                :message  "Negative durations should come back as Strings"}
+               {:interval "25 HOUR"
+                :expected "25:00:00"
+                :message  "Durations outside the valid range of `LocalTime` should come back as Strings"}
+               {:interval "1 HOUR"
+                :expected #t "01:00:00"
+                :message  "A `timediff()` result within the valid range should still come back as a `LocalTime`"}]]
+        (testing (str "\n" interval "\n" message)
+          (is (= [expected]
+                 (mt/first-row
+                   (qp/process-query
+                    (assoc (mt/native-query
+                             {:query (format "SELECT timediff(current_timestamp + INTERVAL %s, current_timestamp)" interval)})
+                           ;; disable the middleware that normally converts `LocalTime` to `Strings` so we can verify
+                           ;; our driver is actually doing the right thing
+                           :middleware {:format-rows? false}))))))))))