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

Add JSR-310 Support #386

Merged
merged 1 commit into from Oct 30, 2016
Merged

Add JSR-310 Support #386

merged 1 commit into from Oct 30, 2016

Conversation

marschall
Copy link
Contributor

Add support for Java 8 Date and Time API AKA JSR-310. On the face of it
the existing JDBC types java.sql.Date, java.sql.Time and
java.sql.Timestamp offer conversion methods to and from JSR-310 data
types. Unfortunately java.sql.Timestamp does silent data truncation
when the timestamp falls into a DST transition on the JVM time zone.
Worse still for java.time.OffsetDateTime there is no corresponding JDBC
type. Therefore these conversions have to be implemented. All of this
has to be programmed using reflection so the code compiles and can be
executed on Java 7.

@svladykin
Copy link
Member

I see lots of code duplications. Can you please move all the code you've added to a separate class LocalDateTimeUtils and use constants and static methods from this new class where needed? All the reflection has to be encapsulated in this utility class.

@marschall
Copy link
Contributor Author

I can do this.

  • Do you want the reflection that is only used in tests also moved there?
  • Do you have any preference on how to best handle the checked exceptions from reflection that "shouldn't happen"?

@marschall marschall force-pushed the jsr-310 branch 3 times, most recently from 182877d to 81bbe9d Compare October 29, 2016 08:36
Copy link
Member

@svladykin svladykin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good now! Need few minor fixes.

@@ -0,0 +1,481 @@
package org.h2.util;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add file header.

try {
return Class.forName(className);
} catch (ClassNotFoundException e) {
throw new IllegalStateException("Java 8 or later but class " + className + " is missing");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add the original ClassNotFoundException as cause.

return clazz.getMethod(methodName, parameterTypes);
} catch (NoSuchMethodException e) {
throw new IllegalStateException("Java 8 or later but method "
+ clazz.getName() + "#" + methodName + "(" + Arrays.toString(parameterTypes) + ") is missing");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add the original exception as cause.

try {
return clazz.getField(fieldName).get(null);
} catch (NoSuchFieldException | IllegalAccessException e) {
throw new IllegalStateException("Java 8 or later but field " + clazz.getName()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add the original exception as cause.

} catch (IllegalAccessException e) {
throw DbException.convert(e);
} catch (InvocationTargetException e) {
throw DbException.convertInvocation(e, "timestampe conversion failed");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

timestamp

@svladykin
Copy link
Member

Looks very good now!

  • I see only parse reflective calls in tests, I think it is ok to add methods like parseLocalDate to LocalDateTimeUtils.
  • Converting them to unchecked exception must be enough. As I see you did just that, but lets do not miss the original exceptions.

Add support for Java 8 Date and Time API AKA JSR-310. On the face of it
the existing JDBC types java.sql.Date, java.sql.Time and
java.sql.Timestamp offer conversion methods to and from JSR-310 data
types. Unfortunately java.sql.Timestamp does silent data truncation
when the timestamp falls into a DST transition on the JVM time zone.
Worse still for java.time.OffsetDateTime there is no corresponding JDBC
type. Therefore these conversions have to be implemented. All of this
has to be programmed using reflection so the code compiles and can be
executed on Java 7.
@svladykin
Copy link
Member

Thanks a lot for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants