Automatic transaction for the tests

[pupeno]

I think it would be nice if tests are automatically encased in a transaction and I did with the help of a dynamically bound connection:

(use-fixtures
  :once
  (fn [test-function]
    (db/connect!)
    (migrations/migrate ["migrate"])
    (test-function)))

(def ^:dynamic db-connection)

(use-fixtures
  :each
  (fn [test-function]
    (jdbc/with-db-transaction
      [db-connection-with-transaction @db/conn]
      (jdbc/db-set-rollback-only! db-connection-with-transaction)
      (binding [db-connection db-connection-with-transaction]
        (test-function)))))

(deftest test-users
  (is (submap? {:email          "sam.smith@example.com"
                :password       "pass"
                :name           "Sam Smith"
                :preferred_name "Sammy"}
               (db/run
                 db/create-user<!
                 {:email          "sam.smith@example.com"
                  :password       "pass"
                  :name           "Sam Smith"
                  :preferred_name "Sammy"}
                 db-connection)))
  (is (submap? {:email          "sam.smith@example.com"
                :password       "pass"
                :name           "Sam Smith"
                :preferred_name "Sammy"}
               (first (db/run db/get-user {:email "sam.smith@example.com"} db-connection)))))

Do you like it? Shall we include it in the template like that? While we are at it, maybe the boilerplate at the beginning should be in a function somewhere?

[yogthos]

Yeah that seems like a good idea, better than doing it for each test manually. :)

[pupeno]

I'm playing with having the actual main connection be dynamic: https://groups.google.com/forum/#!topic/clojure/fRi554wbPSk

That error proneness already caught me by surprise. What do you think?

[yogthos]

As James Reeves pointed out, using dynamic is considered bad practice in Clojure. I personally prefer the approach of writing explicit functions for handling stuff like transactions as well as seen in his examples. The dynamic approach can work obviously, but I don't think it would be appropriate for the template to use patterns that aren't considered idiomatic.

[pupeno]

I replied to James. I think that argument is a generic argument in favor of pure functional programming. Clojure itself uses dynamic vars and we don't stop using it because of it. But I do agree that dynamic vars can be problematic and hence I'm challenging it.

I think any time a function can optionally take a database connection or otherwise gets the global one, it's a problem because it's very error prone. Passing the wrong database connection fails very silently and caused corrupted data and you might miss it for months until you see your database has a lot of records it shouldn't have or vice-versa. Right now, this problem in Luminus is at a very low level in the chain of calls (essentially, right above yesql). Maybe having it way above in the chain of calls would be acceptable, but even then I'm doubtful. This issue already cost me half an hour of debugging in a 200 line project... it'll be insane when we get to 200k lines of code.

I haven't decided what pattern I'll use in my project, but either I use dynamic vars that way or the low level functions, like db/run, do not ever implicitly pick up the global connection. I explained much more in a reply to James in the mailing list.

[yogthos]

Yeah, I definitely agree with your reasoning here. I personally think the run function is a bit of a kludge in any case. Your point regarding the db access being inherently impure is also very valid. The more I think of it the more I want to experiment with the dynamic idea.

As Colin points out in the google groups thread, as long as the dynamic var is private and confined to the namespace so that it's only accessed by public functions externally then it's a reasonable approach.

The two things I'd like the solution to capture would be that the connection is managed internally in the database namespace. The dynamic var or the atom really shouldn't be exposed externally. The second part is the point you mention about transactions where you don't want to have silent errors when you forget to pass the right connection.

[pupeno]

I'm still very early on my experimentation with this model, but since most SQL libraries out there in Clojure leave connection management up to you, I think it would be great to have a library that does connection management. I think this pattern should be encapsulated into a library that makes it very easy and safe to use.

Think of your average programmer that creates a rails project and starts hacking away, writing business logic code, instead of thinking about database connections. I'd like to have the same level of productivity with Clojure.

[pupeno]

This seems to be working, in the db namespace:

(defonce ^:dynamic conn (atom nil))

(def pool-spec
  {:adapter    :postgresql
   :init-size  1
   :min-idle   1
   :max-idle   4
   :max-active 32})

(defn connect! []
  (try
    (reset!
      conn
      {:datasource
       (dbcp/make-datasource
         (assoc pool-spec
           :jdbc-url (to-jdbc-uri (env :database-url))))})
    (catch Exception e
      (timbre/error "Error occured while connecting to the database!" e))))

(defn disconnect! []
  (when-let [ds (:datasource @conn)]
    (when-not (.isClosed ds)
      (.close ds)
      (reset! conn nil))))

(defn run
  "executes a Yesql query using the given database connection and parameter map
  the parameter map defaults to an empty map and the database conection defaults
  to the conn atom"
  ([query-fn] (run query-fn {}))
  ([query-fn query-map] (run query-fn query-map @conn))
  ([query-fn query-map db]
   (try
     (query-fn query-map {:connection db})
     (catch BatchUpdateException e
       (throw (or (.getNextException e) e))))))

(defmacro with-db-transaction [& body]
  `(jdbc/with-db-transaction
     [db-connection-with-transaction# @conn]
     (binding [watu.db.core/conn (atom db-connection-with-transaction#)]
       ~@body)))

(defn db-set-rollback-only! []
  (jdbc/db-set-rollback-only! @conn))

(defn connection
  "Return the database connection in case you need to access directly. This breaks the abstraction, so, be careful."
  []
  @conn)

and then for example:

(use-fixtures
    :each
    (fn [test-function]
      (db/with-db-transaction
        (db/db-set-rollback-only!)
        (test-function))))

[pupeno]

Unfortunately, conn cannot be marked as private because the macro generates code that access it and that code is compiled in a different namespace.

[pupeno]

This is working beautifully for me:

(defonce ^:dynamic conn (atom nil))

(def pool-spec
  {:adapter    :postgresql
   :init-size  1
   :min-idle   1
   :max-idle   4
   :max-active 32})

(defn connect! []
  (try
    (reset!
      conn
      {:datasource
       (dbcp/make-datasource
         (assoc pool-spec
           :jdbc-url (to-jdbc-uri (env :database-url))))})
    (catch Exception e
      (timbre/error "Error occured while connecting to the database!" e))))

(defn disconnect! []
  (when-let [ds (:datasource @conn)]
    (when-not (.isClosed ds)
      (.close ds)
      (reset! conn nil))))

(defmacro with-db-transaction [& body]
  `(jdbc/with-db-transaction
     [db-connection-with-transaction# @conn]
     (binding [watu.db.core/conn (atom db-connection-with-transaction#)]
       ~@body)))

(defn db-set-rollback-only! []
  (jdbc/db-set-rollback-only! @conn))

(defn defqueries [filename]
  (let [base-namespace *ns*
        namespace-name (symbol (str (name (ns-name *ns*)) ".connectionless-queries"))]
    (create-ns namespace-name)
    (in-ns namespace-name)
    (require '[yesql.core :refer [defqueries]])
    (let [yesql-queries (yesql/defqueries filename)]
      (let [queries (doall (for [yesql-query yesql-queries]
                             (intern base-namespace (with-meta (:name (meta yesql-query)) (meta yesql-queries))
                                     (fn
                                       ([] (yesql-query {} {:connection @conn}))
                                       ([args] (yesql-query args {:connection @conn}))))))]
        (in-ns (ns-name base-namespace))
        queries))))

(defqueries "sql/queries.sql")

[yogthos]

I did a bit of cleanup, and I think this approach makes sense. I would propose changing with-db-transaction to allow the user to specify the transaction explicitly and drop the custom db-set-rollback-only! as it would work for any clojure.java.jdbc functions that accept a connection:

(defmacro with-transaction [t-conn & body]
  `(jdbc/with-db-transaction
     [~t-conn @conn]
     (binding [myapp.db.core/conn (atom ~t-conn)]
       ~@body)))

(with-transaction t
  (jdbc/db-set-rollback-only! t)
  (create-user! {:id "baz" :first_name "bar" :last_name "baz" :email nil :pass "pass"}))

(defonce ^:dynamic conn (atom nil))

(def pool-spec
  {:adapter    :postgresql
   :init-size  1
   :min-idle   1
   :max-idle   4
   :max-active 32})

(defn connect! [url]
  (try
    (reset!
      conn
      {:datasource
       (dbcp/make-datasource
         (assoc pool-spec
           :jdbc-url (to-jdbc-uri url)))})
    (catch Exception e
      (timbre/error "Error occured while connecting to the database!" e))))

(defn disconnect! []
  (when-let [ds (:datasource @conn)]
    (when-not (.isClosed ds)
      (.close ds)
      (reset! conn nil))))

(defn defqueries [filename]
  (let [base-namespace *ns*
        queries-ns (symbol (str (name (ns-name *ns*)) ".connectionless-queries"))]
    (create-ns queries-ns)
    (in-ns queries-ns)
    (require '[yesql.core :as yesql])
    (let [yesql-queries (yesql/defqueries filename)
          queries (doall (for [yesql-query yesql-queries]
                           (intern base-namespace
                                   (with-meta (:name (meta yesql-query)) (meta yesql-queries))
                                   (fn
                                     ([] (yesql-query {} {:connection @conn}))
                                     ([args] (yesql-query args {:connection @conn}))
                                     ([args conn] (yesql-query args conn))))))]
      (in-ns (ns-name base-namespace))
      queries)))

I think this is more or less ready to be wrapped up in a lib, I could do it today if like.

[yogthos]

ok I made a repo here, I figure it makes sense to keep this under Luminus. I'd like to start moving more libraries under the umbrella as that would make it easier to keep track of core functionality and maintain it in somewhat unified fashion. :)

[yogthos]

I'm just cleaning up the template to integrate the lib, should be working in a bit

[yogthos]

come to think of it the conn should really be definied within the project db namespace so that different namespaces could have their own separate connections

[pupeno]

Even if it's within luminus, I think it would be good to have a non-luminus name so others feel like adopting it as well. I was thinking of dbconman (database connection manager). I don't feel strongly about this.

[yogthos]

Yeah I don't have a strong opinion on that either, we can change it to dbconman :) Unfortunately, I rand into a bit of trouble there. Turns out that tracking the dynamic var outside the namespace doesn't work. So, the init! function just retains the original copy of the var. I'm going to have to play around with this a bit more.

Meanwhile, I pushed all the changes into the template, so that should work as expected. :)

[yogthos]

ok latest version is up on Clojars, and I've updated the docs to match, let me know if you find anything I missed :)