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

JDBC 4.2 for LocalTime, LocalDate & LocalDateTime #1478

Merged
merged 3 commits into from Apr 3, 2019

Conversation

Projects
None yet
3 participants
@kdubb
Copy link
Contributor

commented Feb 7, 2019

The follows other recent commits for OffsetDateTime & ZonedDateTime to defer to the driver’s JDBC 4.2 support for JSR 310 objects.

JDBC 4.2 for LocalTime, LocalDate & LocalDateTime
The follows other recent commits for OffsetDateTime & ZonedDateTime to defer to the driver’s JDBC 4.2 support for JSR 310 objects.
@harawata

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

Thank you for the PR, @kdubb !

Could you explain the motive behind this change?
These new type handlers won't work with old drivers and there are some major DBs that have no JDBC 4.2 compliant drivers yet (Db2, SAP ASE), so users may expect some clear advantages.

@kdubb

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2019

MySQL and two PostgreSQL drivers support 4.2 types. Full disclosure, I'm the lead on pgjdbc-ng.

Drivers, like ours, have made the switch completely and there is no translation to java.sql.Time or java.sql.Date first, and then to LocalTime, LocalDate or OffsetTime; these translations roil timezone handling and lose precision.

MyBatis already has specific handlers for java.sql.Time and java.sql.Date so the current type handlers force those translations, and the problems they come with, upon everybody wishing to use new features of JDBC & Java. Arguably, if you're using/requiring an older driver, you're probably using the older types it properly supports.

Finally, you already made the decision not to support the drivers you mentioned by switching OffsetDateTime over to this method. This just completes the picture and provides symmetry, using JSR 310 objects means using the new methods to access them. Currently, some 310 objects do use the new methods and some don't, which came as a bit of a shock when I switched to 3.5.0.

@harawata

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

I noticed your contribution to pgjdbc-ng and pgjdbc when reviewing the earlier issue. :)

I did some tests and verified that the current LocalTimeTypeHandler loses the fractional seconds (nanoseconds) during conversion [1] (I have added a test case to this PR).
So, +1 to the LocalTimeTypeHandler change.

For the other two (i.e. LocalDateTypeHandler and LocalDateTimeTypeHandler), however, I couldn't find any problem with the current implementations.
Do you have any test case?

Unlike OffsetDatTime, OffsetTime and ZonedDateTime, LocalDate, LocalTime and LocalDateTime can be mapped to traditional types (i.e. DATE, TIME and TIMESTAMP).
So, if the current implementation works fine, I would prefer to keep them as they are for better compatibility with older drivers.
If other committers like the change, I wouldn't object to it, though.

[1] That's how java.sql.Time#valueOf() works according to the doc :

The nanosecond field from LocalTime is not part of the newly created Time object.

@raupachz

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

I am with @harawata on not changing the current implementation. Often jdbc drivers are just stored in some /lib folder of your application folder and forgotten. Changing this might break applications with an upgraded MyBatis, but an older jdbc driver.

@raupachz

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

@kdubb Thank you for contributing!

Would your proposed change work with the MariaDB Connector/J? I briefly looked at the source of the driver and it looks like even the most recent one does use the translations.

@kdubb

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2019

@raupachz Thats a pretty glib way to view development in this day and age. I would say most development is done using tools with dependency management like Maven, Gradle, Ivy, etc. and they're including their driver in that.

Also, clearly MariaDB Connector/J will support these changes; the fact that it internally uses conversion to other types is an implementation decision that should be left to the driver. Without these changes MyBatis is forcing the conversions and not leaving it to the driver.

Finally, again, without these changes you have some proper support for JSR-310 types and not other, as a developer I find it extremely surprising that its done this way. If somebody is actually using and storing JSR-310 types, you would expect them to use a driver that supports them. It doesn't affect anybody not using JSR-310 types. Considering these were introduced in Java 8, this is more than likely not to affect legacy applications.

@kdubb

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2019

@harawata PostgreSQL stores time & date values with microsecond precision & JSR-310 supports nanosecond precision. Forcing translation to java.sql.* causes them to lose precision that is otherwise supported by PostgreSQL. That's an unnecessary and unexpected outcome when the application has chosen to specifically use JSR-310 types.

@harawata

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

Hi @kdubb ,

I might have been unclear.

You modified three type handlers in this PR.
Let's discuss each type handler separately to avoid confusion.

Forcing translation to java.sql.* causes them to lose precision that is otherwise supported by PostgreSQL.

This statement is true only for LocalTimeTypeHandler and I already upvoted the change to this type handler.

The changes to the other two type handlers seem unnecessary to me because...

  • LocalDateTypeHandler does not need fractional seconds.
  • LocalDateTimeTypeHandler in 3.5.0 correctly stores/retrieves fractional seconds [1].

I have tested the 3.5.0 conversions with various DBs and couldn't find any problem.
If you have a scenario in which these two type handlers do not work as you expect, please provide a test case and I'll reinvestigate.

[1] Unlike java.sql.Time#valueOf(LocalTime), java.sql.Timestamp#valueOf(LocalDateTime) retains fractional seconds.

Obtains an instance of Timestamp from a LocalDateTime object, with the same year, month, day of month, hours, minutes, seconds and nanos date-time value as the provided LocalDateTime.

@kdubb

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

@harawata In my quick response to you I failed to mention on of the two reasons that this change should happen.

Microsecond precision is one of them but the other is timezone conversion. The rules for JDBC java.sql.* types require utilization of some value of Calendar; which when not provided is the default Calendar.getInstance(). Default Calendar instances utilize the default TimeZone.

This means calling setTimestamp causes a LocalDateTime to undergo assumption of being in the JVM's default timezone before passing it to the database. When the JVM and database utilize different default time zones it causes LocalDateTime to be treated as the incorrect time zone. The specification of LocalDateTime specifies it has no time zone and should always be in whatever default timezone is set (for either the JVM or the database).

While I used setTimestamp and LocalDateTime in the example above, all java.sql.* date/time types are related to a time zone. Evidence of these necessary conversions is provided by the JDBC spec and can easily be seen by looking at the extended versions of the setTimestamp, setTime & setDate methods that allow specifying a specific Calendar instance rather than relying on the default.

The only real way for LocalDateTime, LocalTime & LocalDate to be stored & read correctly in all time zones is to not convert them to java.sql.* types first.

OffsetTime isn't quite as affected, It still undergoes an unnecessary conversion but given that it carries a time zone it can be converted to an expected value.

@harawata

This comment has been minimized.

Copy link
Member

commented Feb 16, 2019

I actually ran the same tests with setting different default time zone from the server and they mostly worked [1].
I must be missing something, but I'm not sure what.
I'd really appreciate if you could provide a repro (e.g. an example project, test case or exact steps to reproduce the issue).
Please take your time as there is nothing urgent.

[1] With MySQL 5.7.25 + Connector/J 8.0.15, the current implementation failed when selecting DATE and DATETIME to LocalDate and LocalDateTime respectively, while setObject failed when inserting LocalDateTime into DATETIME.
The other drivers were not affected.

@harawata

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

Hi @kdubb ,

We're planning to release 3.5.1 soon and I would like to include your change to the LocalTimeTypeHandler.
As I am still not sure about the other two type handlers, may I ask you to exclude the changes to LocalDateTimeTypeHandler and LocalDateTypeHandler from this PR?
If you are busy, please just let me know and I'll make the change.

Thanks in advance!

@kdubb

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

@harawata I've attached a simple test project (using pgjdbc-ng and Postgres) that displays the issue... I'll paste the test method here because it contains a good explanation. Note the test fails on mainline; using this branch the test will succeed.

  public void testLocalDateTime() {

    // Force into Pacific Time timezone
    TimeZone.setDefault(TimeZone.getTimeZone("America/Los_Angeles"));

    // This following time is a valid "local date time" to be stored and retrieved, it should be unaltered when
    // inserted and selected from the database as a "local date time"
    //
    // Note: the time itself does not exist in Pacific Time (our selected time zone), since it during the
    // "time hole" created by setting clocks forward an hour at 2 am (thus becoming immediately 3 am) for
    // Daylight Saving
    //
    // If mybatis forces conversion to java.sql.Timestamp (which requires a timezone) it will use the current default
    // timezone. In this case it causes the value to change on its way into or out of the database.
    //
    // If mybatis uses LocalDateTime feature of JDBC 4.2 the value will not change on its way into or out of the
    // database.

    LocalDateTime value =
        LocalDateTime.of(LocalDate.of(2019, MARCH, 10), LocalTime.of(2, 30));

    TestMapper mapper = sqlSession.getMapper(TestMapper.class);

    mapper.insertLocalDateTime(value);

    LocalDateTime found = mapper.selectLocalDateTime();
    String foundRaw = mapper.selectRaw();

    // Will currently fail (3:30 gets inserted due to "fixing" time in Pacific time zone
    assertEquals(value, found);
    assertEquals("2019-03-10 02:30:00", foundRaw);
  }

mybatis-test-datetime.zip

@harawata

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

Thank you very much, @kdubb !
Now it's crystal clear. 💎
I'll add the test and merge this PR later.

@harawata harawata self-assigned this Apr 3, 2019

@harawata harawata added bug and removed waiting for feedback labels Apr 3, 2019

@harawata harawata added this to the 3.5.1 milestone Apr 3, 2019

Added edge case tests provided by @kdubb
`java.sql.Date` and `java.sql.Timestamp` cannot hold a value that does not exist in the system's time zone and alter it implicitly. e.g.

- In Pacific/Apia, `2011-12-30` becomes `2011-12-31`
- In America/Los_Angeles, `2019-03-10 02:30` becomes `2019-03-10 03:30`

@harawata harawata merged commit 2a157ae into mybatis:master Apr 3, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.