Merge pull request #1426 from metabase/fix-timezones-on-mysql

Fix setting timezone for native MySQL queries

src/metabase/driver.clj

@@ -405,31 +405,32 @@
   [query {:keys [executed_by]
           :as options}]
   {:pre [(integer? executed_by)]}
-  (let [query-execution {:uuid            (.toString (java.util.UUID/randomUUID))
-                         :executor_id     executed_by
-                         :json_query      query
-                         :query_id        nil
-                         :version         0
-                         :status          :starting
-                         :error           ""
-                         :started_at      (u/new-sql-timestamp)
-                         :finished_at     (u/new-sql-timestamp)
-                         :running_time    0
-                         :result_rows     0
-                         :result_file     ""
-                         :result_data     "{}"
-                         :raw_query       ""
-                         :additional_info ""}]
-    (let [query-execution (assoc query-execution :start_time_millis (System/currentTimeMillis))]
-      (try
-        (let [query-result (process-query query)]
-          (when-not (contains? query-result :status)
-            (throw (Exception. "invalid response from database driver. no :status provided")))
-          (when (= :failed (:status query-result))
-            (throw (Exception. ^String (get query-result :error "general error"))))
-          (query-complete query-execution query-result))
-        (catch Exception e
-          (query-fail query-execution (.getMessage e)))))))
+  (let [query-execution {:uuid              (.toString (java.util.UUID/randomUUID))
+                         :executor_id       executed_by
+                         :json_query        query
+                         :query_id          nil
+                         :version           0
+                         :status            :starting
+                         :error             ""
+                         :started_at        (u/new-sql-timestamp)
+                         :finished_at       (u/new-sql-timestamp)
+                         :running_time      0
+                         :result_rows       0
+                         :result_file       ""
+                         :result_data       "{}"
+                         :raw_query         ""
+                         :additional_info   ""
+                         :start_time_millis (System/currentTimeMillis)}]
+    (try
+      (let [query-result (process-query query)]
+        (when-not (contains? query-result :status)
+          (throw (Exception. "invalid response from database driver. no :status provided")))
+        (when (= :failed (:status query-result))
+          (throw (Exception. ^String (get query-result :error "general error"))))
+        (query-complete query-execution query-result))
+      (catch Exception e
+        (log/error (u/format-color 'red "Query failure: %s" (.getMessage e)))
+        (query-fail query-execution (.getMessage e))))))
 
 (defn query-fail
   "Save QueryExecution state and construct a failed query response"

src/metabase/driver/generic_sql.clj

@@ -173,10 +173,11 @@
       Return a korma form appropriate for converting a Unix timestamp integer field or value to an proper SQL `Timestamp`.
       SECONDS-OR-MILLISECONDS refers to the resolution of the int in question and with be either `:seconds` or `:milliseconds`.
 
-   *  `(timezone->set-timezone-sql [timezone])` *(OPTIONAL)*
+   *  `set-timezone-sql` *(OPTIONAL)*
 
-      Return a string that represents the SQL statement that should be used to set the timezone
-      for the current transaction.
+      This should be a prepared JDBC SQL statement string to be used to set the timezone for the current transaction.
+
+          \"SET @@session.timezone = ?;\"
 
    *  `(date [this ^Keyword unit field-or-value])`
 

src/metabase/driver/generic_sql/native.clj

@@ -7,7 +7,8 @@
             [metabase.db :refer [sel]]
             [metabase.driver :as driver]
             [metabase.driver.generic-sql.util :refer :all]
-            [metabase.models.database :refer [Database]]))
+            [metabase.models.database :refer [Database]]
+            [metabase.util :as u]))
 
 (defn- value->base-type
   "Attempt to match a value we get back from the DB with the corresponding base-type`."
@@ -16,37 +17,37 @@
 
 (defn process-and-run
   "Process and run a native (raw SQL) QUERY."
-  {:arglists '([query])}
   [{{sql :query} :native, database-id :database, :as query}]
-  {:pre [(string? sql)
-         (integer? database-id)]}
-  (log/debug "QUERY: \n"
-             (with-out-str (clojure.pprint/pprint (update query :driver class))))
-  (try (let [database (sel :one [Database :engine :details] :id database-id)
-             db (-> database
-                    db->korma-db
-                    korma.db/get-connection)
-             [columns & [first-row :as rows]] (jdbc/with-db-transaction [conn db :read-only? true]
-                                                ;; If timezone is specified in the Query and the driver supports setting the timezone
-                                                ;; then execute SQL to set it
-                                                (when-let [timezone (or (-> query :native :timezone)
-                                                                        (driver/report-timezone))]
-                                                  (when (seq timezone)
-                                                    (let [{:keys [features timezone->set-timezone-sql]} (driver/engine->driver (:engine database))]
-                                                      (when (contains? features :set-timezone)
-                                                        (log/debug "Setting timezone to:" timezone)
-                                                        (jdbc/db-do-prepared conn (timezone->set-timezone-sql timezone))))))
-                                                (jdbc/query conn sql :as-arrays? true))]
-         ;; TODO - Why don't we just use annotate?
-         {:rows    rows
-          :columns columns
-          :cols    (map (fn [column first-value]
-                          {:name      column
-                           :base_type (value->base-type first-value)})
-                        columns first-row)})
+  (try (let [database                            (sel :one :fields [Database :engine :details] :id database-id)
+             db-conn                             (-> database
+                                                     db->korma-db
+                                                     korma.db/get-connection)
+             {:keys [features set-timezone-sql]} (driver/engine->driver (:engine database))]
+
+         (jdbc/with-db-transaction [t-conn db-conn]
+
+           ;; Set the timezone if applicable. We do this *before* making the transaction read-only because some DBs
+           ;; won't let you set the timezone on a read-only connection
+           (when-let [timezone (driver/report-timezone)]
+             (when (and (seq timezone)
+                        (contains? features :set-timezone))
+               (log/debug (u/format-color 'green "%s" set-timezone-sql))
+               (try (jdbc/db-do-prepared t-conn set-timezone-sql [timezone])
+                    (catch Throwable e
+                      (log/error (u/format-color 'red "Failed to set timezone: %s" (.getMessage e)))))))
+
+           ;; Now make the transaction read-only and run the query itself
+           (.setReadOnly ^com.mchange.v2.c3p0.impl.NewProxyConnection (:connection t-conn) true)
+           (log/debug (u/format-color 'green "%s" sql))
+           (let [[columns & [first-row :as rows]] (jdbc/query t-conn sql, :as-arrays? true)]
+             {:rows    rows
+              :columns columns
+              :cols    (for [[column first-value] (zipmap columns first-row)]
+                         {:name      column
+                          :base_type (value->base-type first-value)})})))
        (catch java.sql.SQLException e
-         (let [^String message (or (->> (.getMessage e)     ; error message comes back like 'Column "ZID" not found; SQL statement: ... [error-code]' sometimes
+         (let [^String message (or (->> (.getMessage e) ; error message comes back like 'Column "ZID" not found; SQL statement: ... [error-code]' sometimes
                                         (re-find #"^(.*);") ; the user already knows the SQL, and error code is meaningless
-                                        second)             ; so just return the part of the exception that is relevant
+                                        second) ; so just return the part of the exception that is relevant
                                    (.getMessage e))]
            (throw (Exception. message))))))

src/metabase/driver/generic_sql/query_processor.clj

@@ -236,7 +236,9 @@
           (if (and (seq timezone)
                    (contains? (:features driver) :set-timezone))
             (kdb/transaction
-             (k/exec-raw ((:timezone->set-timezone-sql driver) timezone))
+             (try (k/exec-raw [(:set-timezone-sql driver) [timezone]])
+                  (catch Throwable e
+                    (log/error (u/format-color 'red "Failed to set timezone: %s" (.getMessage e)))))
              (k/exec korma-query))
             (k/exec korma-query))))
 

src/metabase/driver/mysql.clj

@@ -69,12 +69,6 @@
                 :milliseconds "FROM_UNIXTIME(%s / 1000)")
                 [field-or-value]))
 
-(defn- timezone->set-timezone-sql [timezone]
-  ;; If this fails you need to load the timezone definitions from your system into MySQL;
-  ;; run the command `mysql_tzinfo_to_sql /usr/share/zoneinfo | mysql -u root mysql`
-  ;; See https://dev.mysql.com/doc/refman/5.7/en/time-zone-support.html for details
-  (format "SET @@session.time_zone = '%s';" timezone))
-
 ;; 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
 (defn- trunc-with-format [format-str]
@@ -170,7 +164,10 @@
        :unix-timestamp->timestamp         unix-timestamp->timestamp
        :date                              date
        :date-interval                     date-interval
-       :timezone->set-timezone-sql        timezone->set-timezone-sql
+       ;; If this fails you need to load the timezone definitions from your system into MySQL;
+       ;; run the command `mysql_tzinfo_to_sql /usr/share/zoneinfo | mysql -u root mysql`
+       ;; See https://dev.mysql.com/doc/refman/5.7/en/time-zone-support.html for details
+       :set-timezone-sql                  "SET @@session.time_zone = ?;"
        :humanize-connection-error-message humanize-connection-error-message}
       sql-driver
       (update :features conj :set-timezone)))

src/metabase/driver/postgres.clj

@@ -100,10 +100,6 @@
                 :milliseconds "TO_TIMESTAMP(%s / 1000)")
               [field-or-value]))
 
-(defn- timezone->set-timezone-sql [timezone]
-  (format "SET LOCAL timezone TO '%s';" timezone))
-
-
 (defn- driver-specific-sync-field! [{:keys [table], :as field}]
   (with-jdbc-metadata [^java.sql.DatabaseMetaData md @(:db @table)]
     (let [[{:keys [type_name]}] (->> (.getColumns md nil nil (:name @table) (:name field))
@@ -200,7 +196,7 @@
        :unix-timestamp->timestamp         unix-timestamp->timestamp
        :date                              date
        :date-interval                     date-interval
-       :timezone->set-timezone-sql        timezone->set-timezone-sql
+       :set-timezone-sql                  "SET LOCAL timezone TO ?;"
        :driver-specific-sync-field!       driver-specific-sync-field!
        :humanize-connection-error-message humanize-connection-error-message}
       sql-driver