Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JdbcTemplate should store generated keys only if database driver supports it #31463

Closed
wants to merge 2 commits into from

Conversation

maciejmiklas
Copy link
Contributor

@maciejmiklas maciejmiklas commented Oct 20, 2023

Not every database supports PreparedStatement#getGeneratedKeys(), for example, SQLLitle does not, and calling this method throws an exception, it has been discussed here: xerial/sqlite-jdbc#996

JdbcTemplate calls PreparedStatement#getGeneratedKeys() whether it's supported or not, and in the case of SQLLitle, it causes an exception.

This PR changes the behavior of JdbcTemplate that it will call PreparedStatement#getGeneratedKeys() only if the underlying driver supports it by querying: DatabaseMetaData#supportsGetGeneratedKeys().

@gotson
Copy link

gotson commented Oct 20, 2023

It might be worth reading similar questions asked on jooq's github some years ago, as apparently supportsGetGeneratedKeys may not be reliable in some drivers.

jOOQ/jOOQ#8996 (comment)

There's also jOOQ/jOOQ#8993 (comment) where apparently returning null from getGeneratedKeys, instead an empty resultset should be returned.

@jhoeller
Copy link
Contributor

So your code is calling a JdbcTemplate operation with a KeyHolder argument there but would be happy for that key holder not to be populated? Note that there are non-KeyHolder overloads of those methods which never trigger a getGeneratedKeys call to begin with, in case you do not need those keys anyway.

If we consider key retrieval as sort-of-optional even for our KeyHolder-based operations, we could simply catch SQLFeatureNotSupportedException on actual access, I suppose, just like we check for null there. That would avoid any potential inconsistency with DatabaseMetaData support checks.

However, I'm still concerned by the mismatch with the javadoc for our KeyHolder-based operations which explicitly state that "generated keys will be put into the given KeyHolder". If the JDBC driver does not support key retrieval, arguably those JdbcTemplate operations are meant to fail...

@sbrannen sbrannen added the in: data Issues in data modules (jdbc, orm, oxm, tx) label Oct 23, 2023
@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Oct 23, 2023
@maciejmiklas
Copy link
Contributor Author

maciejmiklas commented Oct 24, 2023

@jhoeller you are correct - we can use update without keys

@snicoll
Copy link
Member

snicoll commented Oct 24, 2023

Thanks for the follow-up. I've created #31486 to review the Javadoc.

@snicoll snicoll added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged or decided on labels Oct 24, 2023
@prrvchr
Copy link

prrvchr commented Feb 9, 2024

Hi all,
Here is a JDBC 4.1 compatible SQLite version with getGeneratedKeys() support.
sqlite-jdbc-3.45.1.5-SNAPSHOT.jar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: data Issues in data modules (jdbc, orm, oxm, tx) status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants