Merge pull request #3765 from metabase/workaround-to-allow-mysql-jdbc-connection-string-args

Allow custom MySQL JDBC connection string params

src/metabase/driver/generic_sql.clj

 (ns metabase.driver.generic-sql
   (:require [clojure.java.jdbc :as jdbc]
             [clojure.math.numeric-tower :as math]
-            [clojure.set :as set]
+            (clojure [set :as set]
+                     [string :as str])
             [clojure.tools.logging :as log]
             (honeysql [core :as hsql]
                       [format :as hformat])
@@ -151,6 +152,16 @@
   [{:keys [engine details], :as database}]
   (db->pooled-connection-spec database))
 
+(defn handle-additional-options
+  "If DETAILS contains an `:addtional-options` key, append those options to the connection string in CONNECTION-SPEC.
+   (Some drivers like MySQL provide this details field to allow special behavior where needed)."
+  {:arglists '([connection-spec details])}
+  [{connection-string :subname, :as connection-spec} {additional-options :additional-options, :as details}]
+  (-> (dissoc connection-spec :additional-options)
+      (assoc :subname (str connection-string (when (seq additional-options)
+                                               (str (if (str/includes? connection-string "?") "&" "?")
+                                                    additional-options))))))
+
 
 (defn escape-field-name
   "Escape dots in a field name so HoneySQL doesn't get confused and separate them. Returns a keyword."

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,25 @@
    :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
+  "Append `default-connection-args-string` to the connection string in CONNECTION-DETAILS, and an additional option to explicitly disable SSL if appropriate.
+   (Newer versions of MySQL will complain if you don't explicitly disable SSL.)"
+  {:argslist '([connection-spec details])}
+  [{connection-string :subname, :as connection-spec} {ssl? :ssl}]
+  (assoc connection-spec
+    :subname (str connection-string "?" default-connection-args-string (when-not ssl?
+                                                                         "&useSSL=false"))))
 
 (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)
+      (sql/handle-additional-options details)))
 
 (defn- unix-timestamp->timestamp [expr seconds-or-milliseconds]
   (hsql/call :from_unixtime (case seconds-or-milliseconds
@@ -161,7 +170,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

src/metabase/driver/postgres.clj

@@ -15,67 +15,66 @@
   (:import java.util.UUID
            org.postgresql.ssl.NonValidatingFactory))
 
-(defn- column->base-type
+(def ^:private ^:const column->base-type
   "Map of Postgres column types -> Field base types.
    Add more mappings here as you come across them."
-  [column-type]
-  ({:bigint        :type/BigInteger
-    :bigserial     :type/BigInteger
-    :bit           :type/*
-    :bool          :type/Boolean
-    :boolean       :type/Boolean
-    :box           :type/*
-    :bpchar        :type/Text       ; "blank-padded char" is the internal name of "character"
-    :bytea         :type/*          ; byte array
-    :cidr          :type/Text       ; IPv4/IPv6 network address
-    :circle        :type/*
-    :date          :type/Date
-    :decimal       :type/Decimal
-    :float4        :type/Float
-    :float8        :type/Float
-    :geometry      :type/*
-    :inet          :type/IPAddress
-    :int           :type/Integer
-    :int2          :type/Integer
-    :int4          :type/Integer
-    :int8          :type/BigInteger
-    :interval      :type/*          ; time span
-    :json          :type/Text
-    :jsonb         :type/Text
-    :line          :type/*
-    :lseg          :type/*
-    :macaddr       :type/Text
-    :money         :type/Decimal
-    :numeric       :type/Decimal
-    :path          :type/*
-    :pg_lsn        :type/Integer    ; PG Log Sequence #
-    :point         :type/*
-    :real          :type/Float
-    :serial        :type/Integer
-    :serial2       :type/Integer
-    :serial4       :type/Integer
-    :serial8       :type/BigInteger
-    :smallint      :type/Integer
-    :smallserial   :type/Integer
-    :text          :type/Text
-    :time          :type/Time
-    :timetz        :type/Time
-    :timestamp     :type/DateTime
-    :timestamptz   :type/DateTime
-    :tsquery       :type/*
-    :tsvector      :type/*
-    :txid_snapshot :type/*
-    :uuid          :type/UUID
-    :varbit        :type/*
-    :varchar       :type/Text
-    :xml           :type/Text
-    (keyword "bit varying")                :type/*
-    (keyword "character varying")          :type/Text
-    (keyword "double precision")           :type/Float
-    (keyword "time with time zone")        :type/Time
-    (keyword "time without time zone")     :type/Time
-    (keyword "timestamp with timezone")    :type/DateTime
-    (keyword "timestamp without timezone") :type/DateTime} column-type))
+  {:bigint        :type/BigInteger
+   :bigserial     :type/BigInteger
+   :bit           :type/*
+   :bool          :type/Boolean
+   :boolean       :type/Boolean
+   :box           :type/*
+   :bpchar        :type/Text ; "blank-padded char" is the internal name of "character"
+   :bytea         :type/*    ; byte array
+   :cidr          :type/Text ; IPv4/IPv6 network address
+   :circle        :type/*
+   :date          :type/Date
+   :decimal       :type/Decimal
+   :float4        :type/Float
+   :float8        :type/Float
+   :geometry      :type/*
+   :inet          :type/IPAddress
+   :int           :type/Integer
+   :int2          :type/Integer
+   :int4          :type/Integer
+   :int8          :type/BigInteger
+   :interval      :type/*               ; time span
+   :json          :type/Text
+   :jsonb         :type/Text
+   :line          :type/*
+   :lseg          :type/*
+   :macaddr       :type/Text
+   :money         :type/Decimal
+   :numeric       :type/Decimal
+   :path          :type/*
+   :pg_lsn        :type/Integer         ; PG Log Sequence #
+   :point         :type/*
+   :real          :type/Float
+   :serial        :type/Integer
+   :serial2       :type/Integer
+   :serial4       :type/Integer
+   :serial8       :type/BigInteger
+   :smallint      :type/Integer
+   :smallserial   :type/Integer
+   :text          :type/Text
+   :time          :type/Time
+   :timetz        :type/Time
+   :timestamp     :type/DateTime
+   :timestamptz   :type/DateTime
+   :tsquery       :type/*
+   :tsvector      :type/*
+   :txid_snapshot :type/*
+   :uuid          :type/UUID
+   :varbit        :type/*
+   :varchar       :type/Text
+   :xml           :type/Text
+   (keyword "bit varying")                :type/*
+   (keyword "character varying")          :type/Text
+   (keyword "double precision")           :type/Float
+   (keyword "time with time zone")        :type/Time
+   (keyword "time without time zone")     :type/Time
+   (keyword "timestamp with timezone")    :type/DateTime
+   (keyword "timestamp without timezone") :type/DateTime})
 
 (defn- column->special-type
   "Attempt to determine the special-type of a Field given its name and Postgres column type."
@@ -96,17 +95,21 @@
   "Params to include in the JDBC connection spec to disable SSL."
   {:sslmode "disable"})
 
-(defn- connection-details->spec [{:keys [ssl] :as details-map}]
+(defn- connection-details->spec [{ssl? :ssl, :as details-map}]
   (-> details-map
       (update :port (fn [port]
-                      (if (string? port) (Integer/parseInt port)
-                          port)))
-      (dissoc :ssl)               ; remove :ssl in case it's false; DB will still try (& fail) to connect if the key is there
-      (merge (if ssl
+                      (if (string? port)
+                        (Integer/parseInt port)
+                        port)))
+      ;; remove :ssl in case it's false; DB will still try (& fail) to connect if the key is there
+      (dissoc :ssl)
+      (merge (if ssl?
                ssl-params
                disable-ssl-params))
       (rename-keys {:dbname :db})
-      dbspec/postgres))
+      dbspec/postgres
+      (sql/handle-additional-options details-map)))
+
 
 (defn- unix-timestamp->timestamp [expr seconds-or-milliseconds]
   (case seconds-or-milliseconds
@@ -241,7 +244,10 @@
                                                           {:name         "ssl"
                                                            :display-name "Use a secure connection (SSL)?"
                                                            :type         :boolean
-                                                           :default      false}])
+                                                           :default      false}
+                                                          {:name         "additional-options"
+                                                           :display-name "Additional JDBC connection string options"
+                                                           :placeholder  "prepareThreshold=0"}])
           :humanize-connection-error-message (u/drop-first-arg humanize-connection-error-message)})
 
   sql/ISQLDriver PostgresISQLDriverMixin)

test/metabase/driver/mysql_test.clj

 (ns metabase.driver.mysql-test
   (:require [expectations :refer :all]
+            [metabase.db :as db]
+            (metabase.driver [generic-sql :as 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 (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))))

test/metabase/driver/postgres_test.clj

@@ -16,7 +16,7 @@
             [metabase.util :as u])
   (:import metabase.driver.postgres.PostgresDriver))
 
-(def ^:private pg-driver (PostgresDriver.))
+(def ^:private ^PostgresDriver pg-driver (PostgresDriver.))
 
 ;; # Check that SSL params get added the connection details in the way we'd like
 ;; ## no SSL -- this should *not* include the key :ssl (regardless of its value) since that will cause the PG driver to use SSL anyway
@@ -213,3 +213,12 @@
 (expect-with-engine :postgres
   (get-timezone-with-report-timezone nil)
   (get-timezone-with-report-timezone "Crunk Burger"))
+
+
+;; make sure connection details w/ extra params work as expected
+(expect
+  "//localhost:5432/cool?prepareThreshold=0"
+  (:subname (sql/connection-details->spec pg-driver {:host               "localhost"
+                                                     :port               "5432"
+                                                     :dbname             "cool"
+                                                     :additional-options "prepareThreshold=0"})))