Aligns timezone behavior with JDBC #240

mirromutth · 2024-02-19T05:44:31Z

Motivation:

Aligns timezone behavior with mysql-connector-j

See also #190

Modification:

Add preserveInstants, connectionTimeZone and forceConnectionTimeZoneToSession support
Default connectionTimeZone to "LOCAL"
Mark serverZoneId as deprecated. It will notice users to use connectionTimeZone instead
Add TimeZoneIntegrationTest to test JDBC alignment of time zone behavior
Correct OffsetTimeCodec to not convert time zone, OffsetTime is not an instant value

Result:

Users can now specify the local time zone to connection, allowing forcing the specified connection time zone on session.

jchrys · 2024-02-19T06:52:38Z

Would it be possible to consider setting the default value for the connectionTimeZone property to 'LOCAL', aligning with the default used by mysql-connector-j? to enhance consistency for developers familiar with mysql-connector-j's defaults.

mirromutth · 2024-02-19T09:59:35Z

Would it be possible to consider setting the default value for the connectionTimeZone property to 'LOCAL', aligning with the default used by mysql-connector-j? to enhance consistency for developers familiar with mysql-connector-j's defaults.

This will break the current behavior. Currently, the configuration option serverZoneId=null would be equivalent to connectionTimeZone=SERVER&preserveInstants=true&forceConnectionTimeZoneToSession=false.

If we do, existing users (without serverZoneId configured) upgrade r2dbc-mysql to 1.1.2, it may unexpectedly cause date time types to use the local time zone. But, they are sent/responded to/from server, so they should be server time zone.

mirromutth · 2024-02-20T01:28:57Z

In my personal opinion, I think we only need to write a migration guide wiki for new users who switch from jdbc to r2dbc.

It should contain:

What attributes we have but jdbc does not
What attributes we don’t have but jdbc does, why not, and how to migrate
Which attributes we have with jdbc, but are different (such as naming and values), how to migrate them (like time zone attributes)

This also helps us clarify what we should do next.

WDYT? @jchrys

jchrys · 2024-02-20T08:30:58Z

I 100% agree with your solution. I cannot deny that is the right way.
However, given the ongoing issue(a year long) with Micronaut, as highlighted here (micronaut-projects/micronaut-data#2388), I face a challenge. Suggestions to set the default connectionTimeZone to LOCAL (as well as to match the behavior of mysql-connector-j) or to switch the default codec for TIMESTAMP and DATETIME fields to LocalDateTime.class(to aligned with mariadb-r2dbc and dev.miku's) were not accepted. (I think not accepting is reasonable)
I think we need another solution. But I don't have one right now for it. I'm not trying to insist, but I would genuinely like to hear your thoughts on this. (including request changes on their end.) Could you please join the discussion and share your insights?

mirromutth · 2024-02-21T02:22:42Z

@jchrys , the default connectionTimeZone is LOCAL now

The default type mappings of DATETIME will be fixed at other PR

jchrys · 2024-02-21T05:24:01Z

@mirromutth
I really appreciate for your efforts on this issue. Could you kindly elaborate on the rationale behind setting the default connectionTimeZone to LOCAL and adjusting DATETIME type mappings in the upcoming PR?

mirromutth · 2024-02-21T06:56:49Z

@mirromutth I really appreciate for your efforts on this issue. Could you kindly elaborate on the rationale behind setting the default connectionTimeZone to LOCAL and adjusting DATETIME type mappings in the upcoming PR?

@jchrys Based on the discussion in micronaut-projects/micronaut-data#2388, I realized that if we tried to use different time zone modes by default, it might cause more complicated problems for some existing projects.

Like I said, in my opinion, ZonedDateTime and server time zone makes more sense. However, if LocalDateTime and the JVM local time zone are widely accepted by other existing projects, we can't really change about it.

So, keep things simple.

jchrys · 2024-02-21T07:55:34Z

@mirromutth I really appreciate for your efforts on this issue. Could you kindly elaborate on the rationale behind setting the default connectionTimeZone to LOCAL and adjusting DATETIME type mappings in the upcoming PR?

@jchrys Based on the discussion in micronaut-projects/micronaut-data#2388, I realized that if we tried to use different time zone modes by default, it might cause more complicated problems for some existing projects.

Like I said, in my opinion, ZonedDateTime and server time zone makes more sense. However, if LocalDateTime and the JVM local time zone are widely accepted by other existing projects, we can't really change about it.

So, keep things simple.

@mirromutth
Thank you for your explanation. I got it.

jchrys

overall lgtm.
just a nit and suggestions.

r2dbc-mysql/src/main/java/io/asyncer/r2dbc/mysql/ConnectionContext.java

r2dbc-mysql/src/main/java/io/asyncer/r2dbc/mysql/MySqlConnectionConfiguration.java

r2dbc-mysql/src/main/java/io/asyncer/r2dbc/mysql/codec/LocalDateTimeCodec.java

r2dbc-mysql/src/main/java/io/asyncer/r2dbc/mysql/codec/ZonedDateTimeCodec.java

r2dbc-mysql/src/main/java/io/asyncer/r2dbc/mysql/internal/util/StringUtils.java

r2dbc-mysql/src/test/java/io/asyncer/r2dbc/mysql/TimeZoneIntegrationTest.java

- Add `preserveInstants`, `connectionTimeZone` and `forceConnectionTimeZoneToSession` - Default `connectionTimeZone` to "LOCAL" - Mark `serverZoneId` as deprecated. It will notice users to use `connectionTimeZone` instead - Add `TimeZoneIntegrationTest` to test JDBC alignment of time zone behavior - Correct `OffsetTimeCodec` to not convert time zone

jchrys

LGTM.
Given this changes default behavior(?), which version do you think best suits this change? (wonder if 1.1.2 would be enough for this changes WDYT?)

mirromutth · 2024-02-22T07:52:17Z

I think we can release it and #243 on 1.1.2. This version mainly deals with date time and time zone problems.

While this may bring some breaking changes, we can add some wiki for it.

jchrys · 2024-02-22T08:21:57Z

I think we can release it and #243 on 1.1.2. This version mainly deals with date time and time zone problems.

While this may bring some breaking changes, we can add some wiki for it.

Got it. Thanks :D

mirromutth added the enhancement New feature or request label Feb 19, 2024

mirromutth added this to the 1.1.2 milestone Feb 19, 2024

mirromutth linked an issue Feb 19, 2024 that may be closed by this pull request

Aligning serverZoneId Behavior with connectionTimeZone(mysql-connector-j) #190

Closed

mirromutth marked this pull request as draft February 19, 2024 06:12

mirromutth force-pushed the 190-aligning-serverzoneid-behavior-with-connectiontimezonemysql-connector-j branch 4 times, most recently from 2c87dd5 to 3dfebff Compare February 19, 2024 09:54

mirromutth requested a review from jchrys February 19, 2024 10:06

mirromutth marked this pull request as ready for review February 19, 2024 10:06

mirromutth changed the title ~~Add connectionTimeZone and forceConnectionTimeZoneToSession support~~ Aligns timezone behavior with JDBC Feb 19, 2024

mirromutth force-pushed the 190-aligning-serverzoneid-behavior-with-connectiontimezonemysql-connector-j branch 2 times, most recently from d776881 to 819c1b7 Compare February 20, 2024 05:17

mirromutth force-pushed the 190-aligning-serverzoneid-behavior-with-connectiontimezonemysql-connector-j branch 6 times, most recently from 8399327 to cfe05a2 Compare February 21, 2024 02:21

jchrys reviewed Feb 22, 2024

View reviewed changes

mirromutth force-pushed the 190-aligning-serverzoneid-behavior-with-connectiontimezonemysql-connector-j branch from cfe05a2 to 5a9b931 Compare February 22, 2024 06:58

jchrys approved these changes Feb 22, 2024

View reviewed changes

mirromutth merged commit c0f72df into trunk Feb 22, 2024
15 checks passed

mirromutth deleted the 190-aligning-serverzoneid-behavior-with-connectiontimezonemysql-connector-j branch February 22, 2024 08:36

jchrys mentioned this pull request Feb 22, 2024

Default to use LocalDateTime for DATETIME #243

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aligns timezone behavior with JDBC #240

Aligns timezone behavior with JDBC #240

mirromutth commented Feb 19, 2024 •

edited

Loading

jchrys commented Feb 19, 2024 •

edited

Loading

mirromutth commented Feb 19, 2024 •

edited

Loading

mirromutth commented Feb 20, 2024 •

edited

Loading

jchrys commented Feb 20, 2024 •

edited

Loading

mirromutth commented Feb 21, 2024 •

edited

Loading

jchrys commented Feb 21, 2024

mirromutth commented Feb 21, 2024

jchrys commented Feb 21, 2024 •

edited

Loading

jchrys left a comment

jchrys left a comment •

edited

Loading

mirromutth commented Feb 22, 2024

jchrys commented Feb 22, 2024

Aligns timezone behavior with JDBC #240

Aligns timezone behavior with JDBC #240

Conversation

mirromutth commented Feb 19, 2024 • edited Loading

jchrys commented Feb 19, 2024 • edited Loading

mirromutth commented Feb 19, 2024 • edited Loading

mirromutth commented Feb 20, 2024 • edited Loading

jchrys commented Feb 20, 2024 • edited Loading

mirromutth commented Feb 21, 2024 • edited Loading

jchrys commented Feb 21, 2024

mirromutth commented Feb 21, 2024

jchrys commented Feb 21, 2024 • edited Loading

jchrys left a comment

Choose a reason for hiding this comment

jchrys left a comment • edited Loading

Choose a reason for hiding this comment

mirromutth commented Feb 22, 2024

jchrys commented Feb 22, 2024

mirromutth commented Feb 19, 2024 •

edited

Loading

jchrys commented Feb 19, 2024 •

edited

Loading

mirromutth commented Feb 19, 2024 •

edited

Loading

mirromutth commented Feb 20, 2024 •

edited

Loading

jchrys commented Feb 20, 2024 •

edited

Loading

mirromutth commented Feb 21, 2024 •

edited

Loading

jchrys commented Feb 21, 2024 •

edited

Loading

jchrys left a comment •

edited

Loading