dresende / node-orm2

Object Relational Mapping
http://github.com/dresende/node-orm2
MIT License
3.07k stars 379 forks source link

Cannot store/retrieve UTC dates in sqlite #568

Open apoco opened 9 years ago

apoco commented 9 years ago

Version: 2.1.19

When retrieving a Date previously stored in an SQLite database, the date comes back with an offset tacked on, presumably based on the local timezone:

var now = new Date();
things.create({ id: 1, someDate: new Date() });

things.get(1, function(err, thing) {
  var diff = now.getTime() - thing.someDate.getTime();
  // expected: 0, actual: 25200000 (7 hours)
});

I saw in the docs that some of the database engines accept a timestamp option, but SQlite is not one of those engines. Indeed, adding a ?timezone=Z did not remedy the situation.

Is there a way to treat dates as UTC using SQLite? Better yet, is there any way to treat all dates as UTC (as all dates should always be) for all database engines? I realize that as a workaround I can use string dates or make the column numeric and store just the timestamp, but it would be super convenient to just use Date objects and not worry about it. I would expect the same Date I stored would be the same Date coming back, not a new Date with an offset applied.

apoco commented 9 years ago

I think I tracked down the problem to the valueToProperty function. Here's my findings:

valueToProperty then messes up this value. It does the following sequence:

  1. value = new Date(value) = good, gives the expected Date.
  2. value.setTime(value.getTime() - (value.getTimezoneOffset() * 60000)); = wrong; injects an offset into a date that is already correct.
  3. value.setTime(value.getTime() - (tz * 60000)); = does nothing in my case, since my tz is 0.

It looks like the intent for this logic is to be the inverse of the sql-query dateToString method, but it's not quite right.

It seems to me that this could all be greatly simplified:

  1. When storing a Date, store the result of value.toISOString().
  2. When retrieving a Date, do a new Date(value).

This way, there is no need to worry about local timezones at all. What are your thoughts?

apoco commented 9 years ago

FYI, verified that the property-timezones.js tests fail for sqlite (using a file database). Would you be opposed to me implementing a fix as described above? Basically, the timezone option would not be supported for sqlite, and no timezone offsets would be applied.

apoco commented 9 years ago

Prerequisite for fixing this is to update the sql-query module. I've created this pull request: https://github.com/dresende/node-sql-query/pull/36. This would of course be incompatible before there are corresponding changes to orm.

dxg commented 9 years ago

Timezones have been a consistent problem in ORM. At work we also store dates as strings to avoid all the timezone issues.

As you mentioned in dresende/node-sql-query#36 we have two options: 1/ Always store dates without timezones 2/ add a new isodate type.

Whilst #2 would be backwards compatible, it will also cause confusion. We should have sane defaults.

I propose we fix the date problems:

This may annoy a few people, so let's leave this open to discussion for the time being.

apoco commented 9 years ago

Here's the changes I originally made. I will look into adding the other proposed changes, which I really like.

https://github.com/dresende/node-orm2/pull/574

erikreppen commented 9 years ago

@dxg UTC with timezone or without should make no difference to the JS Date object as long as there were no shenanigans like the ones described by apoco and there's nothing in the server environment causing confusion about local time when the date object set in the first place.I agree though. Why have it there by default. As raw UTC will help more devs learn how to handle timestamps properly by handling them less outside of the client.