Merge pull request #5 from metabase/fix-proxy-data-source-getConnection

Support next-jdbc; bug fixes

README.md

@@ -8,7 +8,10 @@
 
 ### Creating a Connection Pool
 
-You can create a C3P0 connection pool with any `clojure.java.jdbc` connection spec map. (Currently, only maps with `:subname` and `:subprotocol` are supported.) `connection-pool-spec` will return a `clojure.java.jdbc` connection spec you can use directly with JDBC:
+#### From a `clojure.java.jdbc` spec
+
+You can create a C3P0 connection pool with any `clojure.java.jdbc` connection spec map with `:subname` and
+`:subprotocol` keys. `connection-pool-spec` will return a `clojure.java.jdbc` connection spec you can use directly:
 
 ```clj
 (require '[clojure.java.jdbc :as jdbc]
@@ -22,6 +25,18 @@ You can create a C3P0 connection pool with any `clojure.java.jdbc` connection sp
 
 (You will almost certainly want to store your pool somewhere, such as in an atom).
 
+#### From a JDBC URL String:
+
+You can create a pooled `DataSource` (e.g., for use with [`next-jdbc`](https://github.com/seancorfield/next-jdbc)) by calling `pooled-data-source-from-url`:
+
+```clj
+(require '[next.jdbc :as jdbc]
+         '[metabase.connection-pool :as connection-pool])
+
+(with-open [connection (jdbc/get-connection (connection-pool/pooled-data-source-from-url "jdbc:postgres:cam@localhost:3000/my_db"))]
+  (reduce my-fn init-value (jdbc/plan connection ["SELECT *"])))
+```
+
 ### Configuring the connection pool
 
 You can set connection pool options such as size in a `c3p0.properties` file, or by passing them as a map to `connection-pool-spec`:

project.clj

@@ -1,4 +1,4 @@
-(defproject metabase/connection-pool "1.0.3"
+(defproject metabase/connection-pool "1.1.0"
   :description "Connection pools for JDBC databases. Simple wrapper around C3P0."
   :url "https://github.com/metabase/connection-pool"
   :min-lein-version "2.5.0"
@@ -24,6 +24,8 @@
      [com.h2database/h2 "1.4.197"]
      [pjstadig/humane-test-output "0.9.0"]]
 
+    :global-vars {*warn-on-reflection* true}
+
     :jvm-opts
     ["-Xverify:none"]}
 

src/metabase/connection_pool.clj

@@ -22,17 +22,19 @@
    (reify DataSource
      (getConnection [_]
        (.connect driver jdbc-url properties))
+
      (getConnection [_ username password]
-       (doseq [[k v] {"user" username, "password" password}]
-         (if (some? k)
-           (.setProperty properties k (name v))
-           (.remove properties k)))
-       (.connect driver jdbc-url properties)))))
+       (let [properties (or properties (Properties.))]
+         (doseq [[k v] {"user" username, "password" password}]
+           (if (some? v)
+             (.setProperty properties k (name v))
+             (.remove properties k)))
+         (.connect driver jdbc-url properties))))))
 
 
 ;;; ------------------------------------------- Creating Connection Pools --------------------------------------------
 
-(defn- map->properties
+(defn map->properties
   "Create a `Properties` object from a JDBC connection spec map. Properties objects are maps of String -> String, so all
   keys and values are converted to Strings appropriately."
   ^Properties [m]
@@ -46,16 +48,23 @@
 (defn- spec->properties ^Properties [spec]
   (map->properties (dissoc spec :classname :subprotocol :subname)))
 
-(defn ^:private unpooled-data-source
-  ^DataSource [{:keys [subname subprotocol], :as spec}]
-  (proxy-data-source (format "jdbc:%s:%s" subprotocol subname) (spec->properties spec)))
+(defn- unpooled-data-source
+  (^DataSource [{:keys [subname subprotocol], :as spec}]
+   (proxy-data-source (format "jdbc:%s:%s" subprotocol subname) (spec->properties spec)))
 
-(defn- pooled-data-source ^DataSource
-  ([spec]
+  (^DataSource [driver {:keys [subname subprotocol], :as spec}]
+   (proxy-data-source driver (format "jdbc:%s:%s" subprotocol subname) (spec->properties spec))))
+
+(defn pooled-data-source
+  "Create a new pooled DataSource from a `clojure.java.jdbc` spec."
+  (^DataSource [spec]
    (DataSources/pooledDataSource (unpooled-data-source spec)))
 
-  ([spec pool-properties-map]
-   (DataSources/pooledDataSource (unpooled-data-source spec) (map->properties pool-properties-map))))
+  (^DataSource [spec pool-properties-map]
+   (DataSources/pooledDataSource (unpooled-data-source spec) (map->properties pool-properties-map)))
+
+  (^DataSource [driver spec pool-properties-map]
+   (DataSources/pooledDataSource (unpooled-data-source driver spec) (map->properties pool-properties-map))))
 
 (defn connection-pool-spec
   "Create a new connection pool for a JDBC `spec` and return a spec for it. Optionally pass a map of connection pool
@@ -65,8 +74,21 @@
    {:datasource (pooled-data-source spec)})
 
   ([spec pool-properties-map]
-   {:datasource (pooled-data-source spec pool-properties-map)}))
+   {:datasource (pooled-data-source spec pool-properties-map)})
+
+  ([driver spec pool-properties-map]
+   {:datasource (pooled-data-source driver spec pool-properties-map)}))
+
+(defn pooled-data-source-from-url
+  "Create a new pooled DataSource from a JDBC URL string."
+  (^DataSource [url]
+   (DataSources/pooledDataSource (proxy-data-source url nil)))
+
+  (^DataSource [url pool-properties-map]
+   (DataSources/pooledDataSource (proxy-data-source url nil) (map->properties pool-properties-map)))
 
+  (^DataSource [driver url pool-properties-map]
+   (DataSources/pooledDataSource (proxy-data-source driver url nil) (map->properties pool-properties-map))))
 
 (defn destroy-connection-pool!
   "Immediately release all resources held by a connection pool."

test/metabase/connection_pool_test.clj

@@ -1,19 +1,78 @@
 (ns metabase.connection-pool-test
-  (:require [clojure.test :as t]
+  (:require [clojure.test :refer :all]
             [metabase.connection-pool :as connection-pool]))
 
 (def ^:private spec
   {:classname "org.h2.Driver", :subprotocol "h2", :subname "mem:db"})
 
-(t/deftest properties-test
-  (t/testing "Options passed in to `connection-pool-spec` should get parsed correctly"
+(deftest properties-test
+  (testing "Options passed in to `connection-pool-spec` should get parsed correctly"
     (let [description (-> (connection-pool/connection-pool-spec spec {"acquireIncrement"        1
                                                                       "testConnectionOnCheckin" true})
                           :datasource
                           str)]
-      (t/is (= "acquireIncrement -> 1"
-               (re-find #"acquireIncrement -> \d" description))
-            "numeric options should get converted correctly")
-      (t/is (= "testConnectionOnCheckin -> true"
-               (re-find #"testConnectionOnCheckin -> \w+" description))
-            "boolean options should get converted correctly"))))
+      (is (= "acquireIncrement -> 1"
+             (re-find #"acquireIncrement -> \d" description))
+          "numeric options should get converted correctly")
+      (is (= "testConnectionOnCheckin -> true"
+             (re-find #"testConnectionOnCheckin -> \w+" description))
+          "boolean options should get converted correctly"))))
+
+(deftest map->properties-test
+  (testing "Properties should be converted to strings"
+    ;; Properties are equality-comparable to maps
+    (is (= {"A" "true", "B" "false", "C" "100"}
+           (connection-pool/map->properties {:A "true", "B" false, "C" 100})))))
+
+(defrecord ^:private FakeConnection [url props]
+  java.sql.Connection)
+
+(defrecord ^:private FakeDriver []
+  java.sql.Driver
+  (connect [_ url props]
+    (FakeConnection. url props)))
+
+(defn- proxy-data-source ^javax.sql.DataSource [& args]
+ (apply #'connection-pool/proxy-data-source args))
+
+(deftest proxy-data-source-test
+  (testing "Make sure we can create a data source with an explicit driver instance"
+    (is (= (FakeConnection. "jdbc:my-fake-db:localhost" nil)
+           (.getConnection (proxy-data-source (FakeDriver.) "jdbc:my-fake-db:localhost" nil)))))
+
+  (testing "Make sure username/password are set when using the 3-arg getConnection method"
+    (doseq [props [(java.util.Properties.) nil (doto (java.util.Properties.)
+                                                 (.setProperty "password" "abc")
+                                                 (.setProperty "user" "cam-2"))]]
+      (testing (format "with initial properties = %s" (pr-str props))
+        (is (= (FakeConnection. "jdbc:my-fake-db:localhost" {"password" "passw0rd", "user" "cam"})
+               (.getConnection (proxy-data-source (FakeDriver.) "jdbc:my-fake-db:localhost" props)
+                               "cam"
+                               "passw0rd"))))))
+
+  (testing "passing nil username/password to 3-arg getConnection method should existing props"
+    (let [props (doto (java.util.Properties.)
+                  (.setProperty "password" "abc")
+                  (.setProperty "user" "cam-2"))]
+      (is (= (FakeConnection. "jdbc:my-fake-db:localhost" {})
+             (.getConnection (proxy-data-source (FakeDriver.) "jdbc:my-fake-db:localhost" props)
+                             nil
+                             nil))))))
+
+(deftest connection-pool-spec-test
+  (let [{:keys [^javax.sql.DataSource datasource]} (connection-pool/connection-pool-spec
+                                                    {:subprotocol "h2", :subname "mem:in-memory"})]
+    (with-open [conn (.getConnection datasource)
+                stmt (.prepareStatement conn "SELECT 1 AS one;")
+                rset (.executeQuery stmt)]
+      (.next rset)
+      (is (= 1
+             (.getObject rset 1))))))
+
+(deftest pooled-data-source-from-url-test
+  (with-open [conn (.getConnection (connection-pool/pooled-data-source-from-url "jdbc:h2:mem:in-memory"))
+              stmt (.prepareStatement conn "SELECT 1 AS one;")
+              rset (.executeQuery stmt)]
+    (.next rset)
+    (is (= 1
+           (.getObject rset 1)))))