adamlofts / mysql1_dart

MySQL driver for Dart
Other
134 stars 45 forks source link

The driver currently forces UTC logic into application code rather than being left in the database. #39

Closed chris-reynolds closed 3 years ago

chris-reynolds commented 4 years ago

My current project has a single time zone database. i.e. all dates are local. However, binary_data_packet.dart (line 144) and standard_data_packet.dart (line 70) assume that database dates are held as UTC. This can cause dates to drift a few hours between write and read back. I have currently forked the repository to get around the issue however, I feel a better solution might be to make the conversion conditional on a database connection parameter. If you agree, and you think it is possible for a packet to identify it's connection, then I am happy to do the changes.

adamlofts commented 4 years ago

Yes, the driver currently requires all DateTime values to be UTC. This is basically because non-utc support seems very complicated e.g. https://gist.github.com/suvozit/bf0200bf7b5029ebe5f4

How to other mysql drivers deal with non-utc servers? Do you specify a timezone when connecting?

chris-reynolds commented 4 years ago

Point 6 from your link above corresponds with what seems to be happening with me. While the timestamps may be stored in UTC form, they are only ever consumed and rendered with respect to the timezone of the current session. i.e. they are assuming local date-times. I think the data packets you are unpacking must be after the timezone adjustment, so by unpacking to the Date.UTC() constructor, we are getting the timezone adjustment twice. (Maybe?) Certainly, I have currently replaced those two calls to simple Date() constructor calls and I am now getting the behaviour I expect. The weird thing is that I would expect all users to complain if they weren't on GMT. I only picked it up because I was reading and rewriting rows multiple times and I am 12 hours from GMT so my days were flicking over.

adamlofts commented 4 years ago

Just so I understand

You save a UTC datetime value. You read it back.

  1. The value is not the same?
  2. This is because the mysql server is not configured to be in UTC timezone?
chris-reynolds commented 4 years ago

This is what I think is happening.

My app has a local date. I pass that date to Mariadb (not mysql I have just noticed) It uses the timezone of my session to convert it to a UTC date and then store it.

Sometime later.... I request the row with that date. It uses the timezone of my session to convert the stored UTC date back to an in-memory local date. Your driver unpacks that local date but instantiates it as if it was a UTC date. My app receives the date moved forward 13 hours (as I am sitting at GMT+13)

A couple of notes.

  1. Maybe Mariadb is different from Mysql but I'd be very surprised.
  2. Various query tools give me back the expected date. Only your driver gives the hours drift.
kralos commented 3 years ago

I've identified another related issue:

Database in UTC with dates stored as UTC dates.

Application in anything other than UTC.

Given application is in Brisbane (UTC+10) and 2021-04-29T05:58:32 (UTC) is stored in the database (2021-04-29 3:58pm Brisbane time).

Any query for the date in the database will yield 2021-04-28 19:58:32Z from mysql1.

This is because lib/src/query/standard_data_packet.dart readField uses DateTime.parse(s).toUtc() which assumes s is in localtime (in this scenario UTC+10) however it's actually in UTC+0.

Example:

#!/usr/bin/env dart

void main() async {
    print({
        'now': DateTime.now().toString(),
        'timeZoneOffset': DateTime.now().timeZoneOffset.toString(),
    });
    for (String parsable in [
        '2021-04-29',
        '2021-04-29 05:58:32',
    ]) {
        print([
            'DateTime.parse',
            parsable,
            DateTime.parse(parsable).toUtc(),
        ]);
    }
}

This test yields:

$ /usr/bin/dart test.dart 
{now: 2021-04-29 16:37:08.828074, timeZoneOffset: 10:00:00.000000}
[DateTime.parse, 2021-04-29, 2021-04-28 14:00:00.000Z]
[DateTime.parse, 2021-04-29 05:58:32, 2021-04-28 19:58:32.000Z]

Use case:

Although I don't condone having application servers in anything other than UTC, some developers code on machines which are in a local timezone (so their time / calendar etc works properly on their desktop). They might use a docker container or external server for their database which may be in UTC.

adamlofts commented 3 years ago

Maybe the driver should just run SET SESSION time_zone = '+0:00' on initialization...

On Thu, 29 Apr 2021 at 07:44, Joe Bennett @.***> wrote:

I've identified another related issue:

Database in UTC with dates stored as UTC dates.

Application in anything other than UTC.

Given application is in Brisbane (UTC+10) and 2021-04-29T05:58:32Z is stored in the database (2021-04-29 3:58pm Brisbane time).

Any query for the date in the database will yield 2021-04-28 19:58:32Z.

This is because lib/src/query/standard_data_packet.dart readField uses DateTime.parse(s).toUtc() which assumes s is in localtime (in this scenario UTC+10) however it's actually in UTC+0.

example test:

!/usr/bin/env dart

void main() async { print({ 'now': DateTime.now().toString(), 'timeZoneOffset': DateTime.now().timeZoneOffset.toString(), }); for (String parsable in [ '2021-04-29', '2021-04-29 05:58:32', ]) { print([ 'DateTime.parse', parsable, DateTime.parse(parsable).toUtc(), ]); } }

This test yields:

$ /usr/bin/dart test.dart {now: 2021-04-29 16:37:08.828074, timeZoneOffset: 10:00:00.000000} [DateTime.parse, 2021-04-29, 2021-04-28 14:00:00.000Z] [DateTime.parse, 2021-04-29 05:58:32, 2021-04-28 19:58:32.000Z]

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/adamlofts/mysql1_dart/issues/39#issuecomment-828980625, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAACWHGKP3EQNF66YU5LRADTLD56FANCNFSM4LO374GA .

kralos commented 3 years ago

The issue isn't the database timezone, it's that the driver (application node) explicitly writes dates in UTC then reads those dates as if they are local.

adamlofts commented 3 years ago

I am a bit confused by your comment because you say the database returns 2021-04-28 19:58:32Z which is a UTC timestamp but your test has timestamps with no timezone. I'm pretty sure DateTime.parse(2021-04-28 19:58:32Z).toUtc() will return the correct time since toUtc() will be a no-op.

Is there something simple to change which will work the same for UTC and fix local times? I'm very busy with other things right now and only use the driver with everything in UTC.

Ideally any change would come with an integration test with a server in a different timezone.

kralos commented 3 years ago

MySQL / MariaDB DATETIME doesn't return Z, mysql1 writes 2021-04-29 05:58:32 in UTC (enforced at the dart level). When it reads 2021-04-29 05:58:32 from the database, it treats it as a local time (even though it isn't) and incorrectly adjusts it by the offset.

case FIELD_TYPE_DATE: // date
case FIELD_TYPE_DATETIME: // datetime
case FIELD_TYPE_TIMESTAMP: // timestamp
    var s = utf8.decode(list);
    return DateTime.parse(s).toUtc();
adamlofts commented 3 years ago

One idea is to

  1. Add ConnectionSettings.timeZoneUtc=true so the driver knows which timezone to create DateTime objects in (utc or local).
  2. Change parsing code to
case FIELD_TYPE_DATE: // date
case FIELD_TYPE_DATETIME: // datetime
case FIELD_TYPE_TIMESTAMP: // timestamp
    var s = utf8.decode(list);
    var d = DateTime.parse(s);
    if (isUtc) {
        d =  DateTime.utc(d.year, d.month, d.day, ...);
    }
    return d;
chris-reynolds commented 3 years ago

I think this is an excellent solution as it allows legacy databases to continue unchanged. I have been running successfully in NZ on a fork for the last year with the code `

 case FIELD_TYPE_TIMESTAMP: // timestamp
    var s = utf8.decode(list);
    return DateTime.parse(s);
    break;

` However, that wouldn't help everyone else's legacy data. I think the idea of a connection switch is exactly what is required.

I am happy to attempt it if you can point me in the right direction.

I also note that this will cause some test rejigging.

kralos commented 3 years ago

One idea is to

1. Add `ConnectionSettings.timeZoneUtc=true` so the driver knows which timezone to create `DateTime` objects in (utc or local).

2. Change parsing code to
case FIELD_TYPE_DATE: // date
case FIELD_TYPE_DATETIME: // datetime
case FIELD_TYPE_TIMESTAMP: // timestamp
    var s = utf8.decode(list);
    var d = DateTime.parse(s);
    if (isUtc) {
        d =  DateTime.utc(d.year, d.month, d.day, ...);
    }
    return d;

I don't think we need a setting as suggested in item 1, we just need to update the parse as you suggested in item 2. This is a pure bugfix, we don't need to add any new features and this is not a BC break.

case FIELD_TYPE_DATE: // date
case FIELD_TYPE_DATETIME: // datetime
case FIELD_TYPE_TIMESTAMP: // timestamp
    var s = utf8.decode(list);
    // s is actually in UTC but doesn't specify an offset; parse assumes that means local, we need to fix it
    var localDateTime = DateTime.parse(s);
    return DateTime.utc(
        localDateTime.year,
        localDateTime.month,
        localDateTime.day,
        localDateTime.hour,
        localDateTime.minute,
        localDateTime.second,
        localDateTime.millisecond,
        localDateTime.microsecond,
    );

or more simply put:

case FIELD_TYPE_DATE: // date
case FIELD_TYPE_DATETIME: // datetime
case FIELD_TYPE_TIMESTAMP: // timestamp
    var s = utf8.decode(list);
    // s is actually in UTC but doesn't specify an offset; parse assumes that means local, we need to fix it
    var localDateTime = DateTime.parse(s);
    return localDateTime.add(localDateTime.timeZoneOffset).toUtc();
chris-reynolds commented 3 years ago

So it seems that the disagreement is whether utf8.decode(list) returns a local date or a UTC date.

I think the way to test that maybe to stop and restart mysql with a different timezone and see if the returned date string is the same or different. If it is the same, then it would appear to be a UTC date, but if it is different then mysql is doing a timezone shift before returning the date. (I think).

kralos commented 3 years ago

So it seems that the disagreement is whether utf8.decode(list) returns a local date or a UTC date.

No, var s = utf8.decode(list); returns a String of what is stored in MySQL / MariaDB. This string has no timezone. E.g:

Since mysql1 forces application developers to always give DateTime objects in UTC these DATE / DATETIME / TIMESTAMPs are in UTC.

When 2021-04-29 05:58:32 is given to DateTime.parse, dart assumes the timezone is the application's timezone (could be something other than UTC).

E.g. for an application in Australia/Brisbane; DateTime.parse('2021-04-29 05:58:32') == 2021-04-29 05:58:32+10:00 == 2021-04-28 19:58:32Z

This issue has nothing to do with the DBMS's timezone, only the application's timezone.

chris-reynolds commented 3 years ago

The plot thickens.

According to mysql 8 doco it would appear that UTC handling is different for datetime and timestamp.

This is a good read too. mysql datetime vs timestamp

adamlofts commented 3 years ago

I don't think we need a setting as suggested in item 1, we just need to update the parse as you suggested in item 2. This is a pure bugfix, we don't need to add any new features and this is not a BC break.

case FIELD_TYPE_DATE: // date
case FIELD_TYPE_DATETIME: // datetime
case FIELD_TYPE_TIMESTAMP: // timestamp
    var s = utf8.decode(list);
    // s is actually in UTC but doesn't specify an offset; parse assumes that means local, we need to fix it
    var localDateTime = DateTime.parse(s);
    return DateTime.utc(
        localDateTime.year,
        localDateTime.month,
        localDateTime.day,
        localDateTime.hour,
        localDateTime.minute,
        localDateTime.second,
        localDateTime.millisecond,
        localDateTime.microsecond,
    );

or more simply put:

case FIELD_TYPE_DATE: // date
case FIELD_TYPE_DATETIME: // datetime
case FIELD_TYPE_TIMESTAMP: // timestamp
    var s = utf8.decode(list);
    // s is actually in UTC but doesn't specify an offset; parse assumes that means local, we need to fix it
    var localDateTime = DateTime.parse(s);
    return localDateTime.add(localDateTime.timeZoneOffset).toUtc();

I agree this is a good fix to merge. It fixes the case when the client is not in UTC. The driver will carry on assuming that the server timezone is UTC for now.

Both these methods are trying to parse a timestamp in UTC right? I think the simplest way as shown here is:

DateTime.parse(dateTime + 'Z');
adamlofts commented 3 years ago

TC handling is different for datetime and timestamp.

This is a good read too.

I've not read this all in depth but it seems like the advice could be that if you are not running a server in UTC then you should run:

SET SESSION time_zone = '+0:00';

and mysql will give TIMESTAMP back in UTC which will match the assumption of the driver.

kralos commented 3 years ago

DateTime.parse(dateTime + 'Z');

This doesn't work for DATE. Dart can't DateTime.parse('2021-04-29Z')

adamlofts commented 3 years ago

@kralos Good point. Are you happy with https://github.com/adamlofts/mysql1_dart/pull/83?