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

`IPersistentCollection` `cons` implementation for `mapify-result-set` seems wrong #222

Closed camsaul closed 1 year ago

camsaul commented 1 year ago

As discussed in the Clojurians Slack https://clojurians.slack.com/archives/C1Q164V29/p1662513332746319?thread_ts=1662494291.800529&cid=C1Q164V29

I was running into some weird issues where

(merge <next.jdbc.result-set.mapify-result-set$reify__1234> some-map)

was giving me something like

([:a 1] [:b 2])

instead of

{:a 1, :b 2}

I'm not really an expert on map internals but it seems like calling .cons on a map type is supposed to give you another map... the implementation for APersistentMap does this for example:

https://github.com/clojure/clojure/blob/b1b88dd25373a86e41310a525a21b497799dbbf2/src/jvm/clojure/lang/APersistentMap.java#L24-L46

The implementation in Potemkin does something similar:

https://github.com/clj-commons/potemkin/blob/1538d4060339461609f8a513cf4933a4de4fa8df/src/potemkin/collections.clj#L68-L79

Compare to the version here in next.jdbc:

https://github.com/seancorfield/next-jdbc/blob/af57829fcbd22e980417f18924dac2693be71163/src/next/jdbc/result_set.clj#L529-L531

I have a custom version of mapify-result-set locally, and when I changed the cons impl to one similar to the Potemkin one it fixed my weird merge issues. So it seems like this might be a bug.

seancorfield commented 1 year ago

Now that I've added some tests, it looks like the correct code here would be:

  (conj (row-builder @builder) obj)

That works for both map and array builders -- and hopefully will work for any custom builders folks have out there.