From a7663762ce7a0b770173138b4b2e7bebcd7412d3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cam=20Sa=C3=BCl?= <cammsaul@gmail.com>
Date: Tue, 15 Nov 2016 16:39:30 -0800
Subject: [PATCH] Allow custom MySQL JDBC connection string params
---
src/metabase/db/spec.clj | 2 +-
src/metabase/driver/mysql.clj | 29 +++++++++++----
test/metabase/driver/mysql_test.clj | 55 ++++++++++++++++++++++++++++-
3 files changed, 77 insertions(+), 9 deletions(-)
diff --git a/src/metabase/db/spec.clj b/src/metabase/db/spec.clj
index 969dfbd97f9..1f55e85c2a9 100644
--- a/src/metabase/db/spec.clj
+++ b/src/metabase/db/spec.clj
@@ -35,7 +35,7 @@
:subprotocol "mysql"
:subname (str "//" host ":" port "/" db)
:delimiters "`"}
- (dissoc opts :host :port :db)))
+ (dissoc opts :host :port :db :additional-options)))
;; TODO - These other ones can acutally be moved directly into their respective drivers themselves since they're not supported as backing DBs
diff --git a/src/metabase/driver/mysql.clj b/src/metabase/driver/mysql.clj
index 9b7745cd575..66e51fab8ec 100644
--- a/src/metabase/driver/mysql.clj
+++ b/src/metabase/driver/mysql.clj
@@ -43,7 +43,7 @@
:VARCHAR :type/Text
:YEAR :type/Integer} (keyword (s/replace (name column-type) #"\sUNSIGNED$" "")))) ; strip off " UNSIGNED" from end if present
-(def ^:private ^:const connection-args
+(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
@@ -51,16 +51,28 @@
:characterEncoding :UTF8
:characterSetResults :UTF8})
-(def ^:private ^:const connection-args-string
- (apply str "?" (interpose \& (for [[k v] connection-args]
- (str (name k) \= (name v))))))
+(def ^:private ^:const ^String default-connection-args-string
+ (s/join \& (for [[k v] default-connection-args]
+ (str (name k) \= (name v)))))
+
+(defn- append-connection-args
+ {:argslist '([connection-spec details])}
+ [{connection-string :subname, :as connection-spec} {ssl? :ssl, additional-options :additional-options, :as details}]
+ (assoc connection-spec
+ :subname (str connection-string
+ "?"
+ default-connection-args-string
+ ;; newer versions of MySQL will complain if you don't explicitly disable SSL
+ (when-not ssl?
+ "&useSSL=false")
+ (when (seq additional-options)
+ (str "&" additional-options)))))
(defn- connection-details->spec [details]
(-> details
(set/rename-keys {:dbname :db})
dbspec/mysql
- (update :subname #(str % connection-args-string (when-not (:ssl details)
- "&useSSL=false"))))) ; newer versions of MySQL will complain if you don't explicitly disable SSL
+ (append-connection-args details)))
(defn- unix-timestamp->timestamp [expr seconds-or-milliseconds]
(hsql/call :from_unixtime (case seconds-or-milliseconds
@@ -161,7 +173,10 @@
{:name "password"
:display-name "Database password"
:type :password
- :placeholder "*******"}])
+ :placeholder "*******"}
+ {:name "additional-options"
+ :display-name "Additional JDBC connection string options"
+ :placeholder "tinyInt1isBit=false"}])
:humanize-connection-error-message (u/drop-first-arg humanize-connection-error-message)})
sql/ISQLDriver
diff --git a/test/metabase/driver/mysql_test.clj b/test/metabase/driver/mysql_test.clj
index a4376b99c85..9c046a629e1 100644
--- a/test/metabase/driver/mysql_test.clj
+++ b/test/metabase/driver/mysql_test.clj
@@ -1,8 +1,16 @@
(ns metabase.driver.mysql-test
(:require [expectations :refer :all]
+ [metabase.db :as db]
+ (metabase.driver [generic-sql :as generic-sql]
+ mysql)
+ [metabase.models.database :refer [Database]]
+ [metabase.sync-database :as sync-db]
[metabase.test.data :as data]
(metabase.test.data [datasets :refer [expect-with-engine]]
- [interface :refer [def-database-definition]])))
+ [interface :refer [def-database-definition]])
+ [metabase.test.util :as tu]
+ [metabase.util :as u])
+ (: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
(def-database-definition ^:private ^:const all-zero-dates
@@ -12,6 +20,51 @@
(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
(-> (data/dataset metabase.driver.mysql-test/all-zero-dates
(data/run-query exciting-moments-in-history))
:data :rows))
+
+
+;; make sure connection details w/ extra params work as expected
+(expect
+ "//localhost:3306/cool?zeroDateTimeBehavior=convertToNull&useUnicode=true&characterEncoding=UTF8&characterSetResults=UTF8&useSSL=false&tinyInt1isBit=false"
+ (:subname (generic-sql/connection-details->spec (MySQLDriver.) {:host "localhost"
+ :port "3306"
+ :dbname "cool"
+ :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
+(def-database-definition ^:private ^:const tiny-int-ones
+ ["number-of-cans"
+ [{:field-name "thing", :base-type :type/Text}
+ {:field-name "number-of-cans", :base-type {:native "tinyint(1)"}}]
+ [["Six Pack" 6]
+ ["Toucan" 2]
+ ["Empty Vending Machine" 0]]])
+
+(defn- db->fields [db]
+ (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-engine :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/with-temp-db [db tiny-int-ones]
+ (db->fields db)))
+
+;; if someone says specifies `tinyInt1isBit=false`, it should come back as a number instead
+(expect-with-engine :mysql
+ #{{:name "number-of-cans", :base_type :type/Integer, :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/with-temp-db [db tiny-int-ones]
+ (tu/with-temp Database [db {:engine "mysql"
+ :details (assoc (:details db)
+ :additional-options "tinyInt1isBit=false")}]
+ (sync-db/sync-database! db)
+ (db->fields db))))
--
GitLab