Avoid calling apply in 'hugsql.adapter.clojure-java-jdbc'

[arichiardi]

Re-posting here for visibility:

The doc mentions:

The functions defined by def-db-fns have the following arities:

      [db]
      [db params]
      [db params options & command-options]
where:

db is a db-spec, a connection, a connection pool, or a transaction object
params is a hashmap of parameter data where the keys match parameter placeholder names in your SQL
options is a hashmap of HugSQL-specific options (e.g., :quoting and :adapter)
& command-options is a variable number of options to be passed down into the underlying adapter and database library functions.

However, the following call fails instead.

;; For the internals see:
;; https://github.com/layerware/hugsql/blob/18f1fa72883cd64a22dd2252a12bbee331ca0ac5/hugsql-core/src/hugsql/core.clj#L438-L461
;;
;; Please enable the below implementation once this bug fix is in:
;; https://github.com/layerware/hugsql/pull/119
;;
;; (let [hugsql-command-opts [:row-fn tx]
;;       db-fn* (-> queries q :fn)]
;;   (apply db-fn* db (db/clj->sql params) {} hugsql-command-opts))

Note that I am splicing the last vector by using apply for adhering with the var-arg format.

For completeness, PR #119 was reverted #120 so that we can investigate this more.

[csummers]

I think the confusion here is that the clojure.java.jdbc library now no longer supports var-args for it's options. But HugSQL supports the ability for the underlying library/adapter to support any number of additional arguments. For the purposes of clojure.java.jdbc, though, your code should include the options in a single map and not call apply:

(let [command-opts {:row-fn tx}
       db-fn* (-> queries q :fn)]
   (db-fn* db (db/clj->sql params) {} command-opts))

[arichiardi]

Ok I can do that but I wonder whether it is a good idea, from a library that abstracts over adapters perspective, to behave like that.

It feels more natural to drop the apply with hugsql and either deprecate or special case for the old version of clojure.java.jdbc.

I personally saw the signature and went with apply cause I expected hugsql to handle that. Does all this make sense?

[csummers]

I'm not quite sure I understand from your code, as you have to use a hashmap if you use apply or not because that's what newer versions of clojure.java.jdbc expect:

Without apply w/ a hashmap:

(let [command-opts {:row-fn tx}
       db-fn* (-> queries q :fn)]
   (db-fn* db (db/clj->sql params) {} command-opts))

With apply you still need the hashmap, just wrapped in a list/vec:

(let [command-opts [{:row-fn tx}]
       db-fn* (-> queries q :fn)]
   (apply db-fn* db (db/clj->sql params) {} command-opts))

From a library perspective, you're asking to create a backwards-incompatible version by removing apply from the adapter. This breaks a contract. The current adapter supports both cases: the older calling convention and the newer one.

The fact that the first argument of command-opts must be a hashmap for the newer calling convention was not a HugSQL decision: it was a breaking change in the clojure.java.jdbc library. This is even mentioned in the docs:

Please note that as of clojure.java.jdbc 0.5.8 and HugSQL 0.4.7, the above additional options are now required to be a hashmap instead of keyword arguments as in previous versions. In clojure.java.jdbc 0.5.8 the deprecated usage will emit a warning. In clojure.java.jdbc 0.6.0 the usage is deprecated and not allowed. See the clojure.java.jdbc changelog for details.

(I understand why clojure.java.jdbc moved away from keyword args because they are, quite frankly, more trouble than they are worth, but I'm not sure it was a great library move to remove the signature altogether.)

[arichiardi]

Thanks @csummers for the detailed response.

In the eye of a library user, passing a map instead of a series of keyword value keyword value is still a breaking change. My apply with a vector above was trying to comply with the & command-opts part of the signature and expecting hugsql to deal with passing things to the adapter. Apologies for reiterating but I am just trying to explain my point of view.

From a library perspective, you're asking to create a backwards-incompatible version by removing apply from the adapter.

Agree now that that is not the right approach. I am leaning more towards detecting somehow if that clojure.java.jdbc function accepts multi-arity or plain map (probably inspecting metadata). Anyways, it seems the direction here is to warn in the README (that I missed thank you!). In my inherited code base probably someone stumbled across the same problem and decided not to pass any options and do everything in a wrapper - tech debt was born :smile:

Closing.

[csummers]

@arichiardi Thanks for understanding. I realize it's not ideal. I think if the original signature for clojure.java.jdbc/query had not used keyword arguments, then it's likely that command-options would not have been var args in the first place.

The boundary is tricky since I want HugSQL-defined functions to not differ too much from the underlying library's calling expectations. So, the variable arguments of command-options does provide some flexibility for the HugSQL-defined functions to match the underlying library's expectations either way: imagine an underlying library that is not using keyword arguments, but still needs multiple positional arguments.