seancorfield / next-jdbc

A modern low-level Clojure wrapper for JDBC-based access to databases.
https://cljdoc.org/d/com.github.seancorfield/next.jdbc/
Eclipse Public License 1.0
755 stars 90 forks source link

Code that uses both raw connections and options-wrapped connections can be hard to write #185

Closed seancorfield closed 2 years ago

seancorfield commented 2 years ago

See this example from Slack:

(cond 
    (instance? java.sql.Connection ds)
    (rs/datafiable-result-set (tables (.getMetaData ds)))

    (next.jdbc.default-options/wrapped-connection? ds)
    (rs/datafiable-result-set (tables (.getMetaData (:connectable ds))))

    :else
    (with-open [conn (jdbc/get-connection ds)]
      (rs/datafiable-result-set (tables (.getMetaData conn)))))

Obviously, we don't want folks to be writing conditional code based on types like this -- execute-batch! already does something horrible like this:

   (if (or (instance? java.sql.Connection connectable)
           (opts/wrapped-connection? connectable))
     (with-open [ps (prepare connectable [sql] opts)]
       (execute-batch! ps param-groups opts))
     (with-open [con (get-connection connectable)]
       (execute-batch! con sql param-groups opts)))

We can fix both of these "bad" things by adding something like a with-connection macro that uses an existing Connection (or options-wrapped connection) or creates a new connection for the body to execute in.

seancorfield commented 2 years ago

Having played with a few "solutions" to this, solving execute-batch!'s problem turns out to be not the same as solving the OP's problem (and using a solution to the OP's problem in execute-batch! actually makes it worse).

In addition, trying to solve the OP's problem leads to code that is less predictable about what happens with options so I'm still a bit stuck on this.

seancorfield commented 2 years ago

This was my first cut but it's really not very nice:

(defmacro on-connection
  "Given a connectable object, binds a `Connection` to `sym`,
  then executes the `body` in that context. Only to be used when you
  specifically need a raw `Connection` object rather than for calling
  `next.jdbc` functions on a connectable!

  Like `with-open`, if `on-connection` creates a new `Connection` object,
  it will automatically close it for you.

  If the connectable is a `Connection`, it is used as-is.

  If the connectable is an options-wrapped `Connection`, it is unwrapped
  (be aware that you get the 'raw' `Connection` and lose the effect of
  the options from the wrapper).

  Otherwise, `get-connection` is called on the connectable (and that
  newly-obtained `Connection` is closed after the `body` is done)."
  [[sym connectable] & body]
  (let [con (vary-meta sym assoc :tag 'java.sql.Connection)]
    `(let [con# ~connectable]
       (cond (instance? java.sql.Connection con#)
             ((^{:once true} fn* [~con] ~@body) con#)
             (opts/wrapped-connection? con#)
             ((^{:once true} fn* [~con] ~@body) (:connectable con#))
             :else
             (with-open [new# (get-connection con#)]
               ((^{:once true} fn* [~con] ~@body) new#))))))
seancorfield commented 2 years ago

I think this is a bit of a dead-end in terms of usability and maintenance so I'm closing this out for now.