nukedzn / node-informix

A node.js native client for IBM Informix
18 stars 10 forks source link

Library swaps the system timezone to UTC #48

Closed tboothman closed 6 years ago

tboothman commented 6 years ago

In connection.js the timezone is swapped to UTC when a connection is made. I'm sure this was probably to mask the fact that the timestamps coming out of Informix are always set as being UTC even though they might not be. https://github.com/nukedzn/node-informix/blob/master/src/ifx/workers/fetch.cpp#L115

Given that informix doesn't have timezones as part of its timestamps, you can't make an assumption about the timezone that the timestamp coming out of informix is, therefore putting a Z on the end of the timestamp isn't correct.

Oddly though http://www.ecma-international.org/ecma-262/5.1/#sec-15.9.1.15 says that if there is no time offset on a timestamp it defaults to Z, but node's parser uses local time:

> new Date('2017-06-21T13:53:58')
2017-06-21T12:53:58.000Z
> new Date('2017-06-21T13:53:58Z')
2017-06-21T13:53:58.000Z

In my opinion the Z should be removed from timestamps and the system timezone should not be modified by the library. https://github.com/nodejs/node/issues/3449 mentions (and I sadly have seen) that swapping the system timezone while node is executing causes the timezone to be basically unknowable.

tboothman commented 6 years ago

I'd certainly be willing to back down on removing the Z at the end of the timestamp as I can see that might cause compatibility problems for users. I've made two pull requests - whichever you like best.

uatuko commented 6 years ago

Thanks for looking into this, I'll take a look.

uatuko commented 6 years ago

I'm against removing 'Z', it makes the times ambiguous and I don't like that.

The intention of setting TZ environment variable was to force all communications with Informix to be in UTC. If I'm not mistaken this is used by the CSDK to do time conversions (TZ environment variable).

So, I assumed when TZ is set to UTC any time returned by Informix would be in UTC, hence the changes in src/ifx/workers/fetch.cpp:115 to format time with 'Z'. Admittedly I didn't test this much.

@tboothman, is this not the behaviour? It would be good to get an example where this is causing an issue.

tboothman commented 6 years ago

If it works like mysql, which i imagine it does, datetime columns are inserted as the database's system time (if you use a method) or whatever string you passed into the SQL query. It's storing a number which is a date and time but not tied to any particular time zone. https://www.ibm.com/support/knowledgecenter/en/SSGU8G_12.1.0/com.ibm.esqlc.doc/ids_esqlc_0722.htm doesn't mention timezones. In mysql if you do something like INSERT INTO t VALUES (NOW()) the value inserted will be in local time (of the db server, or you can override it on a connection) and if I connect and change my connection timezone I will get exactly the same time string out of it.

Since I'm not particularly competent in informix I don't know how to prove what I'm saying but we have a table that has a datetime column and the system that uses it always uses local time (without any timezone info) whenever we interact with it. When we queried what I knew was "2017-06-21 14:53:58" local time (Europe/London, so +1 in this instance) this library returned "2017-06-21T14:53:58Z" (which is wrong).

The code I had to deal with this was using moment (which is slow) and was something like moment.tz(frominformix.slice(0,-1), 'Europe/London').toISOString() which returns the expected "2017-06-21T15:53:58Z"

tboothman commented 6 years ago

I completely understand why you don't want to remove the Z. Timestamps without timezone information are basically nonsense, but in this instance informix has no timezone information stored with the data .. so there's not really anything this library can to do say exactly what moment in time a datetime column is. Best guess would be local time, which is what javascript assumes the timestamp without the Z is.

tboothman commented 6 years ago

Hi again. Could you merge the pull request please. I know it's not ideal but I really can't work around it and it shouldn't have an impact on anyone else using the library as that Z is still there in the timestamp.

I ran this query on dbaccess on an informix that is working in local time (Europe/London): select DBINFO ('utc_to_datetime', 1501545600 ) from t I got "2017-08-01 01:00:00" which was expected. I ran the same query using this library (unmodified) and got "'2017-08-01T01:00:00.000Z'" which proves that setting the processes timezone has no impact on the timezone that informix is using. I also ran the query on another informix running in UTC and I get "2017-08-01T00:00:00.000Z"

uatuko commented 6 years ago

Sorry for the delay, I was thinking whether removing 'Z' would be better after all but couldn't decide. I'll get it merged and release a new version.

tboothman commented 6 years ago

No problem. I appreciate there's no right answers here so it's a little awkward. Thanks for sorting this.

tboothman commented 6 years ago

Hi there. Don't want to be a pain, but you never made a new release tag for this change - it'd be much appreciated. Thanks

uatuko commented 6 years ago

Thanks for the reminder, I released v0.6.0 just now.