search

funcool / clojure.jdbc

JDBC library for Clojure
http://funcool.github.io/clojure.jdbc/latest/
Apache License 2.0
105 stars 26 forks source link

.getGeneratedKeys support #30

Closed csummers closed 8 years ago

csummers commented 8 years ago

I'm the author of HugSQL, which provides an adapter which uses clojure.jdbc, and I'd like to see support for JDBC's .getGeneratedKeys such that there is parity between clojure.java.jdbc and clojure.jdbc for inserts returning auto-generated IDs.

I noticed that .getGeneratedKeys is currently commented out with a TODO, so I'm assuming there is an intention to support this.

Are there any concerns you have in supporting this?

Thanks!

niwinz commented 8 years ago

I haven't implemented it because it is looks inconsistent. The execute api is designed just for side effectfull operations that does not return nothing, and fetch is designed to be used for query like stuff (anything that return a result). The getGeneratedKeys is very convenient but it is inconsistent in terms of operations in execute... and can be properly done using RETURNING * with fetch operation.

The main difference with clojure.java.jdbc, is that clojure.jdbc tries to have concise consistent api. I really like clear path for api usage and I'm not very convinced about the .getGeneratedKeys utility in balance with the api consistency. (e.g I have never used that in my apps).

PS: nice library, thanks for creating it, I have plans to create an adapter for suricatta. ;)

csummers commented 8 years ago

I definitely understand the consistency issue (see the HugSQL tests for examples). Support is really quite the mess! I made distinctions in HugSQL between "returning execute" and "insert w/ .getGeneratedKeys" in part to encourage the usage of RETURNING * if the user's database supports it. And, I personally use Postgresql and the RETURNING clause in my own apps.

However, database support for the RETURNING * clause is widely unsupported. I think support for it is only available in Postgresql and Oracle.

When a user actually needs support for getting an auto-generated ID, lack of support for .getGeneratedKeys leaves him/her with no option in clojure.jdbc except having to wrap their insert queries with a secondary query to fetch the last inserted id. Maybe that's a hurdle users are willing to jump over to use clojure.jdbc, but I suspect the inconvenience might hinder some adoption of the library.

Either way, I respect the choice to keep a consistent API and to make users deal with those trade-offs.

niwinz commented 8 years ago

Hmm, i will consider adding this as completely undocumented stuff, because I understand the utility of that, but I don't like promote the usage of that.

I'll implement it today as I have planed release a new version with already merged some bugfixes. Thanks!

csummers commented 8 years ago

Thanks for considering it. No pressure here.

(I didn't add support for .getGeneratedKeys in HugSQL until version 0.4.0 because of my own uncertainty about how to implement it well. Ultimately, I decided to make it available but have good tests and documentation on all of the inconsistencies--leaving the user to choose what was appropriate for his/her database.)

niwinz commented 8 years ago

@csummers This is something that you will expecting? https://github.com/funcool/clojure.jdbc/commit/77afb73218abef3e9d128d6d55ef1e3fddc16768#diff-33815d1d7b25e739b789ed1020d3856dR102

niwinz commented 8 years ago

It behaves differently from clojure.java.jdbc just because it returns a vector of results instead of one unique result.

csummers commented 8 years ago

That looks reasonable to me.

niwinz commented 8 years ago

Released 0.8.0 with that change and other bug fixes.

csummers commented 8 years ago

Great! I'll incorporate support for this into the next release of HugSQL.

Thanks!