From 793fa3989e5cbd42d1c2f8a4576b6a0965625013 Mon Sep 17 00:00:00 2001
From: Cam Saul <cammsaul@gmail.com>
Date: Wed, 23 Aug 2017 12:28:21 -0700
Subject: [PATCH] Switch to official M$ SQL Server JDBC driver [ci drivers]
---
project.clj | 2 +-
src/metabase/config.clj | 11 ++++-
src/metabase/driver/generic_sql.clj | 19 ++++++--
src/metabase/driver/mongo/util.clj | 2 +-
src/metabase/driver/sqlserver.clj | 62 ++++++++++++++++---------
src/metabase/middleware.clj | 3 +-
src/metabase/util.clj | 5 --
test/metabase/driver/sqlserver_test.clj | 34 +++++++++++++-
8 files changed, 97 insertions(+), 41 deletions(-)
diff --git a/project.clj b/project.clj
index 0622a9174c4..17d57a85809 100644
--- a/project.clj
+++ b/project.clj
@@ -52,6 +52,7 @@
[com.h2database/h2 "1.4.194"] ; embedded SQL database
[com.mattbertolini/liquibase-slf4j "2.0.0"] ; Java Migrations lib
[com.mchange/c3p0 "0.9.5.2"] ; connection pooling library
+ [com.microsoft.sqlserver/mssql-jdbc "6.2.1.jre7"] ; SQLServer JDBC driver. TODO - Switch this to `.jre8` once we officially switch to Java 8
[com.novemberain/monger "3.1.0"] ; MongoDB Driver
[com.taoensso/nippy "2.13.0"] ; Fast serialization (i.e., GZIP) library for Clojure
[compojure "1.5.2"] ; HTTP Routing library built on Ring
@@ -73,7 +74,6 @@
[mysql/mysql-connector-java "5.1.39"] ; !!! Don't upgrade to 6.0+ yet -- that's Java 8 only !!!
[net.sf.cssbox/cssbox "4.12" ; HTML / CSS rendering
:exclusions [org.slf4j/slf4j-api]]
- [net.sourceforge.jtds/jtds "1.3.1"] ; Open Source SQL Server driver
[com.clearspring.analytics/stream "2.9.5" ; Various sketching algorithms
:exclusions [org.slf4j/slf4j-api
it.unimi.dsi/fastutil]]
diff --git a/src/metabase/config.clj b/src/metabase/config.clj
index 5a3d3b4c660..423e25325f8 100644
--- a/src/metabase/config.clj
+++ b/src/metabase/config.clj
@@ -82,7 +82,14 @@
(version-info-from-properties-file)
(version-info-from-shell-script)))
-(def ^:const mb-version-string
- "A formatted version string representing the currently running application."
+(def ^:const ^String mb-version-string
+ "A formatted version string representing the currently running application.
+ Looks something like `v0.25.0-snapshot (1de6f3f nested-queries-icon)`."
(let [{:keys [tag hash branch]} mb-version-info]
(format "%s (%s %s)" tag hash branch)))
+
+(def ^:const ^String mb-app-id-string
+ "A formatted version string including the word 'Metabase' appropriate for passing along
+ with database connections so admins can identify them as Metabase ones.
+ Looks something like `Metabase v0.25.0.RC1`."
+ (str "Metabase " (mb-version-info :tag)))
diff --git a/src/metabase/driver/generic_sql.clj b/src/metabase/driver/generic_sql.clj
index b0ec6470a25..d01d391bb1b 100644
--- a/src/metabase/driver/generic_sql.clj
+++ b/src/metabase/driver/generic_sql.clj
@@ -178,16 +178,25 @@
(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] [connection-spec details])}
+ (Some drivers like MySQL provide this details field to allow special behavior where needed).
+
+ Optionally specify SEPERATOR-STYLE, which defaults to `:url` (e.g. `?a=1&b=2`). You may instead set it to `:semicolon`,
+ which will separate different options with semicolons instead (e.g. `;a=1;b=2`). (While most drivers require the former
+ style, some require the latter.)"
+ {:arglists '([connection-spec] [connection-spec details & {:keys [seperator-style]}])}
;; single arity provided for cases when `connection-spec` is built by applying simple transformations to `details`
([connection-spec]
(handle-additional-options connection-spec connection-spec))
- ;; two-arity version provided for when `connection-spec` is being built up separately from `details` source
- ([{connection-string :subname, :as connection-spec} {additional-options :additional-options, :as details}]
+ ;; two-arity+options version provided for when `connection-spec` is being built up separately from `details` source
+ ([{connection-string :subname, :as connection-spec} {additional-options :additional-options, :as details} & {:keys [seperator-style]
+ :or {seperator-style :url}}]
(-> (dissoc connection-spec :additional-options)
(assoc :subname (str connection-string (when (seq additional-options)
- (str (if (str/includes? connection-string "?") "&" "?")
+ (str (case seperator-style
+ :semicolon ";"
+ :url (if (str/includes? connection-string "?")
+ "&"
+ "?"))
additional-options)))))))
diff --git a/src/metabase/driver/mongo/util.clj b/src/metabase/driver/mongo/util.clj
index c7a153a1a7c..e2162c1ba77 100644
--- a/src/metabase/driver/mongo/util.clj
+++ b/src/metabase/driver/mongo/util.clj
@@ -64,7 +64,7 @@
:or {ssl? false}}]
(-> (client-options-for-url-params additional-options)
client-options->builder
- (.description (str "Metabase " (config/mb-version-info :tag)))
+ (.description config/mb-app-id-string)
(.connectTimeout connection-timeout-ms)
(.serverSelectionTimeout connection-timeout-ms)
(.sslEnabled ssl?)
diff --git a/src/metabase/driver/sqlserver.clj b/src/metabase/driver/sqlserver.clj
index 4a99ca24a3e..2ec0c420f0e 100644
--- a/src/metabase/driver/sqlserver.clj
+++ b/src/metabase/driver/sqlserver.clj
@@ -1,14 +1,18 @@
(ns metabase.driver.sqlserver
+ "Driver for SQLServer databases. Uses the official Microsoft JDBC driver under the hood (pre-0.25.0, used jTDS)."
(:require [honeysql.core :as hsql]
[metabase
+ [config :as config]
[driver :as driver]
[util :as u]]
[metabase.driver.generic-sql :as sql]
- [metabase.util.honeysql-extensions :as hx]
- [metabase.util.ssh :as ssh]))
+ [metabase.util
+ [honeysql-extensions :as hx]
+ [ssh :as ssh]]))
(defn- column->base-type
- "See [this page](https://msdn.microsoft.com/en-us/library/ms187752.aspx) for details."
+ "Mappings for SQLServer types to Metabase types.
+ See the list here: https://docs.microsoft.com/en-us/sql/connect/jdbc/using-basic-data-types"
[column-type]
({:bigint :type/BigInteger
:binary :type/*
@@ -48,24 +52,31 @@
(keyword "int identity") :type/Integer} column-type)) ; auto-incrementing integer (ie pk) field
-(defn- connection-details->spec [{:keys [user password db host port instance domain ssl]
- :or {user "dbuser", password "dbpassword", db "", host "localhost", port 1433}
- :as details}]
- {:classname "net.sourceforge.jtds.jdbc.Driver"
- :subprotocol "jtds:sqlserver"
- :loginTimeout 5 ; Wait up to 10 seconds for connection success. If we get no response by then, consider the connection failed
- :subname (str "//" host ":" port "/" db)
- ;; everything else gets passed as `java.util.Properties` to the JDBC connection. See full list of properties here: `http://jtds.sourceforge.net/faq.html#urlFormat`
- ;; (passing these as Properties instead of part of the `:subname` is preferable because they support things like passwords with special characters)
- :user user
- :password password
- :instance instance
- :domain domain
- :useNTLMv2 (boolean domain) ; if domain is specified, send LMv2/NTLMv2 responses when using Windows authentication
- ;; for whatever reason `ssl=request` doesn't work with RDS (it hangs indefinitely), so just set ssl=off (disabled) if SSL isn't being used
- :ssl (if ssl
- "require"
- "off")})
+(defn- connection-details->spec
+ "Build the connection spec for a SQL Server database from the DETAILS set in the admin panel.
+ Check out the full list of options here: `https://technet.microsoft.com/en-us/library/ms378988(v=sql.105).aspx`"
+ [{:keys [user password db host port instance domain ssl]
+ :or {user "dbuser", password "dbpassword", db "", host "localhost", port 1433}
+ :as details}]
+ (-> {:applicationName config/mb-app-id-string
+ :classname "com.microsoft.sqlserver.jdbc.SQLServerDriver"
+ :subprotocol "sqlserver"
+ ;; it looks like the only thing that actually needs to be passed as the `subname` is the host; everything else can be passed as part of the Properties
+ :subname (str "//" host)
+ ;; everything else gets passed as `java.util.Properties` to the JDBC connection.
+ ;; (passing these as Properties instead of part of the `:subname` is preferable because they support things like passwords with special characters)
+ :database db
+ :port port
+ :password password
+ ;; Wait up to 10 seconds for connection success. If we get no response by then, consider the connection failed
+ :loginTimeout 10
+ ;; apparently specifying `domain` with the official SQLServer driver is done like `user:domain\user` as opposed to specifying them seperately as with jTDS
+ ;; see also: https://social.technet.microsoft.com/Forums/sqlserver/en-US/bc1373f5-cb40-479d-9770-da1221a0bc95/connecting-to-sql-server-in-a-different-domain-using-jdbc-driver?forum=sqldataaccess
+ :user (str (when domain (str domain "\\"))
+ user)
+ :instanceName instance
+ :encrypt (boolean ssl)}
+ (sql/handle-additional-options details, :seperator-style :semicolon)))
(defn- date-part [unit expr]
@@ -75,7 +86,8 @@
(apply hsql/call :dateadd (hsql/raw (name unit)) exprs))
(defn- date
- "See also the [jTDS SQL <-> Java types table](http://jtds.sourceforge.net/typemap.html)"
+ "Wrap a HoneySQL datetime EXPRession in appropriate forms to cast/bucket it as UNIT.
+ See [this page](https://msdn.microsoft.com/en-us/library/ms187752.aspx) for details on the functions we're using."
[unit expr]
(case unit
:default expr
@@ -85,6 +97,7 @@
:hour-of-day (date-part :hour expr)
;; jTDS is retarded; I sense an ongoing theme here. It returns DATEs as strings instead of as java.sql.Dates
;; like every other SQL DB we support. Work around that by casting to DATE for truncation then back to DATETIME so we get the type we want
+ ;; TODO - I'm not sure we still need to do this now that we're using the official Microsoft JDBC driver. Maybe we can simplify this now?
:day (hx/->datetime (hx/->date expr))
:day-of-week (date-part :weekday expr)
:day-of-month (date-part :day expr)
@@ -175,7 +188,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 "trustServerCertificate=false"}]))})
sql/ISQLDriver
(merge (sql/ISQLDriverDefaultsMixin)
diff --git a/src/metabase/middleware.clj b/src/metabase/middleware.clj
index 5511be6662c..b4ea7aaa0ba 100644
--- a/src/metabase/middleware.clj
+++ b/src/metabase/middleware.clj
@@ -278,7 +278,7 @@
;; ## Custom JSON encoders
;; Always fall back to `.toString` instead of barfing.
-;; In some cases we should be able to improve upon this behavior; `.toString` may just return the Class and address, e.g. `net.sourceforge.jtds.jdbc.ClobImpl@72a8b25e`
+;; In some cases we should be able to improve upon this behavior; `.toString` may just return the Class and address, e.g. `some.Class@72a8b25e`
;; The following are known few classes where `.toString` is the optimal behavior:
;; * `org.postgresql.jdbc4.Jdbc4Array` (Postgres arrays)
;; * `org.bson.types.ObjectId` (Mongo BSON IDs)
@@ -290,7 +290,6 @@
;; stringify JDBC clobs
(add-encoder org.h2.jdbc.JdbcClob encode-jdbc-clob) ; H2
-(add-encoder net.sourceforge.jtds.jdbc.ClobImpl encode-jdbc-clob) ; SQLServer
(add-encoder org.postgresql.util.PGobject encode-jdbc-clob) ; Postgres
;; Encode BSON undefined like `nil`
diff --git a/src/metabase/util.clj b/src/metabase/util.clj
index a8d3dbb0888..b94e8fcbbca 100644
--- a/src/metabase/util.clj
+++ b/src/metabase/util.clj
@@ -332,11 +332,6 @@
;; H2 -- See also http://h2database.com/javadoc/org/h2/jdbc/JdbcClob.html
org.h2.jdbc.JdbcClob
- (jdbc-clob->str [this]
- (jdbc-clob->str (.getCharacterStream this)))
-
- ;; SQL Server -- See also http://jtds.sourceforge.net/doc/net/sourceforge/jtds/jdbc/ClobImpl.html
- net.sourceforge.jtds.jdbc.ClobImpl
(jdbc-clob->str [this]
(jdbc-clob->str (.getCharacterStream this))))
diff --git a/test/metabase/driver/sqlserver_test.clj b/test/metabase/driver/sqlserver_test.clj
index fd492d874c2..bf92b699674 100644
--- a/test/metabase/driver/sqlserver_test.clj
+++ b/test/metabase/driver/sqlserver_test.clj
@@ -1,5 +1,10 @@
(ns metabase.driver.sqlserver-test
- (:require [metabase.test
+ (:require [clojure.string :as str]
+ [expectations :refer [expect]]
+ [metabase.driver
+ [generic-sql :as sql]
+ [sqlserver :as sqlserver]]
+ [metabase.test
[data :as data]
[util :refer [obj->json->obj]]]
[metabase.test.data
@@ -7,7 +12,7 @@
[interface :refer [def-database-definition]]]))
;;; ------------------------------------------------------------ VARCHAR(MAX) ------------------------------------------------------------
-;; VARCHAR(MAX) comes back from jTDS as a "ClobImpl" so make sure it gets encoded like a normal string by Cheshire
+;; Make sure something long doesn't come back as some weird type like `ClobImpl`
(def ^:private ^:const a-gene
"Really long string representing a gene like \"GGAGCACCTCCACAAGTGCAGGCTATCCTGTCGAGTAAGGCCT...\""
(apply str (repeatedly 1000 (partial rand-nth [\A \G \C \T]))))
@@ -22,3 +27,28 @@
(-> (data/dataset metabase.driver.sqlserver-test/genetic-data
(data/run-query genetic-data))
:data :rows obj->json->obj)) ; convert to JSON + back so the Clob gets stringified
+
+
+;;; Test that additional connection string options work (#5296)
+(expect
+ {:classname "com.microsoft.sqlserver.jdbc.SQLServerDriver"
+ :subprotocol "sqlserver"
+ :applicationName "Metabase <version>"
+ :subname "//localhost;trustServerCertificate=false"
+ :database "birddb"
+ :port 1433
+ :instanceName nil
+ :user "cam"
+ :password "toucans"
+ :encrypt false
+ :loginTimeout 10}
+ (-> (sql/connection-details->spec
+ (sqlserver/->SQLServerDriver)
+ {:user "cam"
+ :password "toucans"
+ :db "birddb"
+ :host "localhost"
+ :port 1433
+ :additional-options "trustServerCertificate=false"})
+ ;; the MB version Is subject to change between test runs, so replace the part like `v.0.25.0` with `<version>`
+ (update :applicationName #(str/replace % #"\s.*$" " <version>"))))
--
GitLab