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
750 stars 89 forks source link

`next.jdbc.sql/insert!` ignores `:options` when using `next.jdbc/with-options` and `next.jdbc/with-logging` together. #242

Closed lucassousaf closed 1 year ago

lucassousaf commented 1 year ago

Describe the bug next.jdbc.sql/insert! ignores the :options specified using the next.jdbc/with-options wrapper if next.jdbc/with-logging is also used. This only happens if you use the next.jdbc/with-logging wrapper after using next.jdbc/with-options. If you use the wrappers in reverse order, everything works as expected.

To Reproduce To reproduce run the following code:

(defn foo
  []
  (let [db-spec (-> (jdbc/get-datasource {:dbtype "postgresql"
                                          :dbname "postgres"
                                          :user "postgres"
                                          :host "postgres"
                                          :password "sa"
                                          :port 5432})
                    (jdbc/with-options jdbc/unqualified-snake-kebab-opts)
                    (jdbc/with-logging prn prn))]
    (jdbc.sql/insert! db-spec :foo {:id 1 :foo-type "foo"})))

The code throws an exception:

ERROR: syntax error at or near \"-\"\n  Position: 25

If you reverse the order of the wrappers, it works.

Expected behavior The order of the wrappers shouldn't affect the behavior of next.jdbc.sql/insert!.

project.clj/deps.edn

:dependencies [[org.clojure/clojure "1.11.0"]
               [com.github.seancorfield/next.jdbc "1.3.847"]
               [org.postgresql/postgresql "42.5.3"]]

Environment (please complete the following information):

Additional Context We think that the problem is in the call to next.jdbc.sql.builder/for-insert function in next.jdbc.sql/insert!. It doesn't take into account the nesting of :connectables.

seancorfield commented 1 year ago

The problem isn't the call to for-insert but a more fundamental issue that wrapped connectables just don't compose properly at all. When with-options is wrapped around with-logging, it works more or less by accident.

I can "solve" the problem by making with-logging aware of with-options (several places in the code already special case this, because the default options wrapper is "special" and is baked into a lot of the code) but anyone writing their own wrapped connectables is going to run into this same problem at some point.

Providing default options was a terrible idea -- I caved to various people complaining about having to pass options explicitly, because they didn't want to use the default builders. I wish I'd never budged on that... since it leads directly to problems like this 😒

seancorfield commented 1 year ago

OK, I've pushed a small change to with-logging -- can you test against the develop (or the latest 1.3.999-SNAPSHOT) to see if it solves this problem?

lucassousaf commented 1 year ago

Yep, I tested the 1.3.999-SNAPSHOT release and it works fine!