drift icon indicating copy to clipboard operation
drift copied to clipboard

DateTimeColumn values are always returned as local time, timezone/UTC gets lost

Open enloc-port opened this issue 1 year ago • 6 comments

When storing a UTC DateTime object in a DateTimeColumn, it gets stored as Unix timestamp. When retrieving the value, it is returned as local time aka the isUtc in the created DateTime object is false.

That's because the SqlTypes._readDateTime() method calls DateTime.fromMillisecondsSinceEpoch() without the isUtc parameter.

This is bad, because when an app is dependent on Unix timestamps, this means a loss of information.

If a DateTime object is stored as UTC, it must be possible to retrieve it as UTC from the database to avoid information loss. This could be achived with a bool parameter/config, either in the select request or on the respective column(s).

enloc-port avatar Aug 07 '24 12:08 enloc-port

I just found a very similar issue: https://github.com/simolus3/drift/issues/286

However, my suggestion would not be to change the way, the data is stored. Rather I'd suggest to give the developers the option to define, whether a DateTimeColumn value should be retrieved as a local or UTC value.

Let's call this option asUtc, with default value false. That way, existing code will not be affected.

enloc-port avatar Aug 07 '24 12:08 enloc-port

Same here. A simple insert & select unit test fails because the returned date shifted by the timezone. With custom converter that uses fromMillisecondsSinceEpoch(isUtc: true) works like a charm. I think this parameter should be used at: https://github.com/simolus3/drift/blob/58f4504e9203b15a6a34a1306961d2c41234f243/drift/lib/src/runtime/types/mapping.dart#L174

Edit Using isUtc is worse and breaks "normal" function. The problem occurs only when you use DateTime.utc in Dart code. Drift always returns Local time. So you have to compare it to local. It doesn't matter you pass UTC or Local time to insert/update, the timestamp integer will be the same. The problem comes when you want to compare it after query. If you used DateTime.utc you can't compare back to it! You have to use default DateTime constructor in code (local time). I updated my tests eliminating .utc constructors and the problem went away!

westito avatar Aug 07 '24 15:08 westito

Storing date times as unix timestamps is the default for backwards compatibility reasons, but for this reason (and others, like these getters potentially returning unexpected results), I recommend switching to the text-based format. Due to the way they're interpreted by sqlite3, drift also only stores unix timestamps with a second precision, the text format supports microseconds.

However, my suggestion would not be to change the way, the data is stored.

Do you see any advantage that timestamps have over the text-based format? I'm not strictly opposed to the parameter controlling whether date times are reported in UTC, but:

  1. As @westito pointed out, this can be done with a type converter (you can actually write a TypeConverter<DateTime, DateTime> that just calls toUtc in both directions, that should work).
  2. It's easy to enable this option and then forget about it. Then later if one is inserting some values with the local timezone, it can be surprising to see them coming out as UTC on the other end.

Given that a proper solution exists in the form of the text format, I'm not too convinced about (ultimately incorrect) patches to the timestamp format.

simolus3 avatar Aug 07 '24 15:08 simolus3

The minor advantage is the size of the information. A Unix timestamp is smaller than an ISO string.

For us though, it's about the migration. Right now, our database uses DateTimeColumn for DateTime columns. Migrating this to TextColumn is cumbersome, because it means migrating both the table definition and the existing data.

That's why I was rooting for a solution which keeps the status quo intact while giving an additional option to serve the described purpose.

enloc-port avatar Aug 07 '24 16:08 enloc-port

@simolus3 Yeah, meanwhile I do some tests and I found it better to leave as is. Using .toUtc() in converter is not solves the problem, only reverses it. (.utc unit tests will work, but local date tests will fail). This information simply lost.

@enloc-port If you need complex date functions you should use text version. Timezones is more than just shifting hours. Think about leap seconds, leap years, daylight saving. If you need UTC you should simply call .toUtc() after retrieve from database.

There is a detailed description about date time and timezones in this article. It is Java but you will understand the differences between timezones, zoned time and timestamp. https://medium.com/elca-it/how-to-get-time-zones-right-with-java-8dea13aabe5c

westito avatar Aug 07 '24 16:08 westito

For us though, it's about the migration. Right now, our database uses DateTimeColumn for DateTime columns. Migrating this to TextColumn is cumbersome, because it means migrating both the table definition and the existing data.

Sorry, I should have mentioned this right away. You can actually instruct drift to use text columns internally when using DateTimeColumns with a compile-time option. The docs also contain information on how to write a migration converting between the formats. My intention was to make this as easy as possible, please let me know if you run into any problems with that approach.

simolus3 avatar Aug 07 '24 20:08 simolus3