Time zone is double-shifted when saving

[dirkmc]

Note that I ran this code at 19:21:01 EDT (New York time), which is 23:21:01 UTC (NY is four hours behind UTC). But when the value is stored to the database it has been shifted four hours again, to 03:21:01 UTC.

var now = new Date();
db.Test.create({
    created_at: now,
    mytext: 'test'
}, function(err, data) {
    db.Test.get(data.id, function(err, newObj) {
        console.log(JSON.stringify(now));
        // "2013-08-18T23:21:01.979Z"
        console.log(JSON.stringify(newObj.created_at));
        // "2013-08-19T03:21:01.000Z"
    });
});

mysql> select id, created_at, UNIX_TIMESTAMP(created_at) from test where id = 5;
+----+---------------------+----------------------------+
| id | created_at          | UNIX_TIMESTAMP(created_at) |
+----+---------------------+----------------------------+
|  5 | 2013-08-18 23:21:01 |                 1376882461 |
+----+---------------------+----------------------------+

Taking the unix timestamp value above and putting it into the browser console shows that it is 23:21:01 EDT (NY time) when it should be 19:21:01 EDT:

new Date(1376882461 * 1000);
Sun Aug 18 2013 23:21:01 GMT-0400 (EDT)

[dresende]

How do you have mysql server configured? Using local time or utc?

[dirkmc]

I'm at work now, I can check when I get home. But it doesn't really matter, if I save the date and then read it again, the seconds since the epoch should always be the same regardless of how mysql is configured.

[dirkmc]

mysql> SELECT @@global.time_zone, @@session.time_zone;
+--------------------+---------------------+
| @@global.time_zone | @@session.time_zone |
+--------------------+---------------------+
| SYSTEM             | SYSTEM              |
+--------------------+---------------------+

From the mysql manual:
The value 'SYSTEM' indicates that the time zone should be the same as the system time zone.

[dresende]

Which is... local? I'm not sure if mysql does any kind of translation when storing/fetching...

[dirkmc]

Yeah I think when they say "System" they mean the timezone on the computer. So on my laptop that would be New York time (EDT). Note that if you think in terms of unix timestamp, you can ignore time zones because a unix timestamp is in seconds since the epoch UTC, regardless of what system you're on. So in my example above, I send the unix timestamp 1376868061 and get back the unix timestamp 1376882461.

[dxg]

Whilst working on something I also hit timezone issues but with postgres. Gonna see if I can get postgres working right.

Edit1:
It seems to be double shifting time with postgres too.
Time goes into database as time - offset and then when it comes back out, time - offset is applied again..

Edit2:
My postgres is running in UTC, node is running in local time.
Creating a timestamp field with timezone in the database makes it work correctly, but without it it fails.
Will investigate more another day, but I think the escapeVal function will need to be aware of if the property is defined as with timezone or without..

[dirkmc]

Yes, looking at the code the problems seem to be:

• In ORM.connect() the timezone option is not being read from the connect string
  https://github.com/dresende/node-orm2/blob/master/lib/ORM.js#L64
  See https://github.com/felixge/node-mysql
• In the Mysql Driver DML, the timezone is being set by default to Z instead of local
  https://github.com/dresende/node-orm2/blob/master/lib/Drivers/DML/mysql.js#L13
• In sql-query, the InsertQuery class doesn't pass the timezone to Dialect.escapeVal(val, tz)
  https://github.com/dresende/node-sql-query/blob/master/lib/Insert.js#L17

[notheotherben]

An alternative to attempting to store timezone information in the database is to just standardize the way we handle this case, say we always store in UTC and all returned date objects are initialized using UTC as the baseline. That way you can allow the Node runtime to handle localizing the time information, you remove any DB engine specific behavioral constraints etc.

For example:

• We get 2013-06-02 20:16:05 +02:00 as our input (Node.js runtime assignment to an ORM property)
• When storing into the database, we adjust to UTC 2013-06-02 18:16:05 +00:00
• When retrieving from the database, we create the date object explicitly using UTC 2013-06-02 18:16:05 UTC
• When accessing the property through Node.js, it will appear in the local time format

[dirkmc]

As far as I can tell from the code that's pretty much how it works already. It just has a few things missing that I outlined above. It's not a problem for timestamp, because (in mysql at least) that includes the timezone. But for datetime it doesn't include timezone.

[dirkmc]

I should qualify my previous comment - timestamp stores timezone information, but I just checked and it seems the same bug exists whether the column is datetime or timestamp

[dirkmc]

Also Ben would the scheme you're describing above work with a legacy database? I'm porting a rails project to node so I don't have much control over how the existing data has been stored

[notheotherben]

Hmm, that's a good point. I think, if you were looking to run off a legacy database which made use of a different timezone offset, it would be best to use a custom plugin to provide that functionality. Wouldn't be difficult to write one which could pre/post process the data if we get this implemented.

I currently don't have the time to do so, but when I do it is on my TODO list.

[dirkmc]

In practice I don't think it's that complicated. The main issue seems to be that by default orm assumes a UTC timezone database, whereas the vanilla install of mysql is local time. If I just hardcode timezone to be 'local' in Dialect.escapeVal() everything works fine.

[dresende]

When I first added the timezone support it was intended for mysql. I added the timezone support code in mysql module and so I added the possibility for people to define it too. What it should do is convert a date to the timezone you want and store in database and when fetching it should do the opposite.

[dirkmc]

It seems to do that fine.

The only problem is that it assumes that the timezone of the database is UTC, but the vanilla install of mysql uses the system time zone (eg EDT for me). So it probably makes sense for that to be the default, and if the user wants to have his database in UTC time he can set an option in orm.

Did you look at my message about where in the code the problems are?

• In ORM.connect() the timezone option is not being read from the connect string
  https://github.com/dresende/node-orm2/blob/master/lib/ORM.js#L64
  See https://github.com/felixge/node-mysql
• In the Mysql Driver DML, the timezone is being set by default to Z instead of local
  https://github.com/dresende/node-orm2/blob/master/lib/Drivers/DML/mysql.js#L13
• In sql-query, the InsertQuery class doesn't pass the timezone to Dialect.escapeVal(val, tz)
  https://github.com/dresende/node-sql-query/blob/master/lib/Insert.js#L17

[notheotherben]

Should be possible to specify the timezone as something along the lines of (not actually implemented yet):

db.define('dates', {
    utc_date: { type: 'date', time: true, timezone: 'utc' },
    local_date: { type: 'date', time: true, timezone: 'local' },
    explicit_tz: { type: 'date', time: true, timezone: +2 }
});

[dresende]

@dirkmc your points are valid, those 3 zones must match their behaviour. @spartan563 I'm not sure if people want that entropy, we can add it, but first we need to make all dates behave correctly for the global tz option.

[notheotherben]

Sounds good to me, thinking over it again - it should be possible for the functionality I've defined to be provided by a plugin anyway. Will look into that when I get a chance.

[dkushner]

As long as the local time zone is configured correctly when the session is initiated, and the time stamp is stored as a timestamptz, at least Postgres will automatically convert to GMT on the way in and from GMT (to the session TZ) on the way out. Currently date values are being stored without timezone. Am I wrong in thinking that simply changing that will fix this issue or am I missing something?

[dxg]

Yes it will fix it; I've tried and it worked.

[dkushner]

@dxg, excellent news! I just happened to have run into that issue with pg a few months ago. Fix is inbound?

[dkushner]

I'm not comfortable enough writing tests at this scale, but I can confirm that changing this line to:

def += " TIMESTAMP WITH TIME ZONE";

Did indeed fix my issue.

[dxg]

I merged your PR and added a few tests. Should be all good now.
For anyone else reading, this is only fixed in mysql for now.
Edit: Now fixed for both mysql & postgres.

[dirkmc]

Sounds good, thanks

[dresende]

[email protected] fixes date timezones in sqlite and postgresql.

[dxg]

It does work for postgres, but the tests fail for sqlite and I haven't had time to investigate why yet.

[dresende]

I pulled the latest code and the tests pass for me. I'm in UTC+1 now (DST). What is the test that is failing? Can you show me the error?

[dxg]

The failing tests are disabled for sqlite:
https://github.com/dresende/node-orm2/blob/master/test/integration/property-timezones.js#L7
If you re-enable, they should fail.