hibernate-orm
hibernate-orm copied to clipboard
Simplify handling of Instant and OffsetDateTime and remove incorrect/dubious comments about JDBC 4.2
In this PR I simplify the code based on the guarantees of the JDBC 4.2 standard.
There is a lot of confusion (EDIT: in the general discussions on the internet, not in the hibernate project) around JDBC 4.2 support for Instant and OffsetDateTime. I think it stems from subtle differences in precise words between "Support in JDBC" vs "Support in the JDBC driver" vs "Support in getObject/SetObject". see https://stackoverflow.com/a/76984102
TLDR:
- jdbc 4.2 does guarantee that getObject/setObject work for OffsetDateTime, there is absolutely no doubt about it.
- jdbc 4.2 can rightfully claim "safe" Instant support, albeit through weird APIs (not getObject/setObject, not based on driver support): jdbc 4.2 adds toInstant() and from(Instant) to java.sql.Timestamp which is part of JDBC and builds upon the existing getTimestamp/setTimestamp methods with a Calendar argument. There is absolutely no doubt that the JDBC 4.2. standard does not support Instant in getObject/setObject (but some drivers may).
EDIT: For context, this PR currently tries to remove as much code as possible as a proposal to see the actual effects in tests, but it's not meant to be merged as is necessarily.
NOTE: For OffsetDateTime, another strategy might be to keep support for non compliant JDBC 4.2 drivers, but do they really exist ? Are they worth the confusion ? I will let you decide. NOTE2: For Instant, unless the jdbc standard adds support for Instant in getObject/setObject in a next version, I don't see any value in maintaining duplicated code (additionally the standard code is not that bad and required anyway) and it imposes a performance penalty (exception try catch) for compliant drivers. NOTE3: the current code in the main branch for TimestampUtcAsOffsetDateTimeJdbcType already doesn't support non compliant drivers (no try catch), so this PR makes everything more homogeneous (unless the difference is on purpose because TimestampUtcAsOffsetDateTimeJdbcType is known to be used only with compliant drivers ? in which case maybe it's better to add a comment saying why TimestampWithTimeZoneJdbcType needs non compliant driver support but TimestampUtcAsOffsetDateTimeJdbcType doesn't ?)
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion. For more information on licensing, please check here.