mirromutth / r2dbc-mysql

R2DBC MySQL Implementation
Apache License 2.0
656 stars 98 forks source link

Add OffsetDateTimeCodec #105

Closed hfhbd closed 4 years ago

hfhbd commented 4 years ago

In R2DBC H2 Implementation OffsetDateTimeCodec is available, with adding OffsetDataTimeCodec to MysSql R2DBC switching between databases is possible without domain code changes.

mirromutth commented 4 years ago

Hi, there. Thanks for your suggestion.

There have a problem: the server of the MySQL will not actively return its timezone and time-offset. And MySQL do never support DATETIME WITH TIMEZONE, which is using the database timezone of global/session configurations (so it can be change anytime). Anyway, we cannot query DB-server timezone for each time.

Maybe I should support this like mysql-connector-j, it ignores the timezone of the DB-server, and is configured by connection properties like useTimezone=true&serverTimezone=UTC+9.

The mysql-connector-j timezone feature likes this: if you try to use difference timezone between driver and DB-server, mysql-connector-j will always convert all datetimes, even LocalDateTime. This means that we may use some database-tools to query the value of the datetime, then we get the original value A, but the driver will tell you that the value is B (the converted value). If timezone feature has been disabled, it would use the JVM timezone to generate OffsetDateTime by default.

And maybe we can log a warning message when user try to set a wrong timezone which is not equal to the server timezone when connection initializing.

mp911de commented 4 years ago

This issue reads to me as two issues. First one is general conversion of date/time values by considering/ignoring the server timezone. Secondly, OffsetDateTime support. If MySQL does not support DATETIME WITH TIMEZONE, then there's no point in adding OffsetDateTime as such support would lead to expectations that cannot be met.

The other issue regarding timezones it worth investigating.

hfhbd commented 4 years ago

As I understood, MySQL has two types: Timestamp and DateTime. Timestamp is a 32Bit Int, stored at the server in UTC, and converted into a LocalDateTime mapping the timezone of the configuration setting at the server. DateTime at the other hand is stored without timezone support, and is returned without mapping, aka like a String.

Problem: The session @@time_zone can be changed during a session (SELECT @@GLOBAL.time_zone, @@SESSION.time_zone;).

Honestly, I don't know exactly an implementation, but @mirromutth idea, ignoring the server time zone and enforce a time zone at connection string from mysql-connector-j sounds good. IntelliJ Database plugin enforces this also, before connecting to a MySQL DB, you have to set the serverTimeZone connection setting.

So there are four cases:

  1. Timestamp and connection without Timezone settings: Return LocalDateTime with the timezone of the server, so no conversion.
  2. Timestamp with timezone connection settings: Convert the timestamp into a OffsetDateTime
  3. DateTime without tz connection setting: return localdatetime, so no conversion
  4. DataTime with connection setting: convert into offsetdatime mapping with the offset of the connection string.

Background, I switched from a H2 DB to a MySQL DB and noticed, the MySQL driver does not support OffsetDateTime, so I had to change my domain code also, which I would like to prevent. I thought, every r2dbc driver should match the r2dbc specs, so as a Applikation developer, switching the database should not effect my code ;)

cbornet commented 4 years ago

:+1: It's a common use case to use H2 for tests and there's currently no shared date type between H2 and MySQL. It would be great to at least support j.t.Instant that you can store without timezone and assume implicitly that TZ is UTC.

benarena commented 4 years ago

I would also like to see the Java Instant class supported.

Example to test using Spring Boot 2.3.1: https://github.com/benarena/spring-r2dbc-date-error

mirromutth commented 4 years ago

@benarena Instant, OffsetTime, ZonedDateTime and OffsetDateTime will be supported. Even I'm considering to support ChronoLocalDateTime, ChronoZonedDateTime.

mirromutth commented 4 years ago

I need to correct my words: mysql-connector-j will only convert unix-time-based datetimes, e.g. java.util.Date, java.sql.Date, java.sql.Timestamp, etc.

If we try to read a LocalDateTime with difference timezone between JVM and MySQL, we will get origin value.

jdbcOperations.execute("CREATE TEMPORARY TABLE test (id INT PRIMARY KEY AUTO_INCREMENT, value TIMESTAMP)");
jdbcOperations.update("INSERT INTO test VALUE (DEFAULT, '2020-6-1 12:00:00')");
LocalDateTimevalue = jdbcOperations.query("SELECT value FROM test", result -> {
    if (result.next()) {
        return result.getObject(1, LocalDateTime.class);
    }
    throw new IllegalArgumentException();
});
System.out.println(value); // also 2020-6-1 12:00:00, not converted

If we change LocalDateTime.class to java.sql.Timestamp.class in the above code, we will get converted value.

Until 8.0.20, mysql-connector-j wrote all Java 8 time support, but OffsetDateTime and ZonedDateTime will be parse failed (because no timezone) and go to fallback route (throw exception).

So, Instant will to be converted and LocalDateTime will not. ZonedDateTime and OffsetDateTime will use the server timezone.

Then, another question: why all the date/time values ​​we get are converted (in classic project)?

Because this is done by ORM, as I have tested, most ORM will try to get java.sql.Timestamp (it is converted) first because it is default type, then convert the Timestamp value to LocalDateTime with JVM timezone. So that is ORM behavior.

So, because of this reason, I'm considering to change the default type of DATETIME/TIMESTAMP with ZonedDateTime or Instant, after this feature completed.

mirromutth commented 4 years ago

It is supported by PR and published to 0.8.2.BUILD-SNAPSHOT. Target version is 0.8.2.RELEASE.

If want to use snapshot versions, add Sonatype snapshots repository to Maven file, see also README