layerware / hugsql

A Clojure library for embracing SQL
https://hugsql.org
Apache License 2.0
690 stars 54 forks source link

Avoid calling apply in hugsql.adapter.clojure-java-jdbc #119

Closed arichiardi closed 3 years ago

arichiardi commented 3 years ago

This patch fixes a mismatch where the adapter was wrong splicing parameters instead of passing :command-options directly to jdbc/query.

arichiardi commented 3 years ago

This is the arity we are calling there:

https://github.com/clojure/java.jdbc/blob/master/src/main/clojure/clojure/java/jdbc.clj#L1161

csummers commented 3 years ago

Yep, the original arity of clojure.java.jdbc/query (and friends) used keyword arguments for options instead of a hashmap, though this arity was deprecated (and subsequently removed) quite a while ago!: https://github.com/clojure/java.jdbc/commit/a0e71397bccb1a9f28c8099864da73c05a8b805b

Thanks for the fix. I'll merge now and try to cut a release sometime in the next week.

arichiardi commented 3 years ago

Thank you! The release is greatly appreciated cause I am working around it at the moment.

csummers commented 3 years ago

@arichiardi Can we revisit this by you showing how you are calling your hugsql-defined functions? This change breaks the test, which includes a clojure.java.jdbc option in it.

(is (= [[:name] ["A"] ["B"]]
      (select-ordered db {:cols ["name"] :sort-by ["name"]} {} {:as-arrays? true})))

Notice that the call should pass the jdbc options as the 4th argument, and that the 3rd argument are hugsql options.

csummers commented 3 years ago

For hugsql-defined functions, the command-options is not a hashmap, but a variable number of args--which gives us some flexibility between adapters to provide additional arguments. This worked well for the deprecated var args style in clojure.java.jdbc/query, and continues to work with the newer map options, such that the first and only argument for clojure.java.jdbc command options should be hashmap.

From the docs:

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.
csummers commented 3 years ago

I've reverted this for now, but if there's a convincing reason to keep this change or change something else to work through whatever issue you're experiencing, please let me know. Thanks.

arichiardi commented 3 years ago

Ok will post an example when I get to it. I remember calling the last arity explicitely with apply in my code. Thank you!

arichiardi commented 3 years ago

Hi @csummers this is how I was calling it:

   ;; 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))

There is that apply there due to the fact that I thought options to db-fn* would be spliced. Is that right?