Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Timezone is wrong on client when reading records from SQL server running in UTC time #262

Closed
jasonhillier opened this issue Feb 2, 2016 · 33 comments

Comments

@jasonhillier
Copy link

Using mysql2 library on my machine running in Pacific time, the records, when deserialized from SQL response, seem to be applying local timezone offset but still keeping date object marked at UTC. This means the timezone offset is actually applied backwards:

Time in db record (SQL server set to UTC): 2016-02-01 21:39:46
Time returned by mysql2 (running on a machine in Pacific TZ): 2016-02-02T05:39:46.000Z (this is a UTC date in future, which is wrong)
Time returned by mysql2 when running on a machine with TZ set to UTC: 2016-02-01T21:39:46.000Z (this is correct time)

Expected result: I would expect toISOString() to return the same UTC date value regardless of client TZ, when reading the same record from the same server.

I believe this code is responsible for inserting SQL's UTC date into a JS object which is instantiated in local time:

Packet.prototype.readDateTime: ... return new Date(y, m-1, d, H, M, S, ms);

@sidorares
Copy link
Owner

Server does not store TZ value in the data row, the only information we have at the time we receive row is "2016-02-01 21:39:46"

Also the client does not now which TZ server is running.

Expected result: I would expect toISOString() to return the same UTC date value regardless of client TZ, when reading the same record from the same server.

I need to double check if this is supported or not, but the only solution I know is to pass TZ to a client, I think this is what node-mysql is doing. This way you do "this client would treat dates as TZ, because I know that server has it's timezone set TZ"

@sidorares
Copy link
Owner

see 'timezone' option in https://github.com/felixge/node-mysql#connection-options

@jasonhillier
Copy link
Author

Hi sidorares,

I tried setting the 'timezone' option on the SQL connection string, it didn't make a difference.

MySQL does have a way of querying it to find out the SQL connection session's timezone:

More info here:
http://stackoverflow.com/questions/930900/how-to-set-time-zone-of-mysql

@sidorares
Copy link
Owner

yes, you can get offset by doing SELECT TIMEDIFF(NOW(), UTC_TIMESTAMP) but I don't want all clients to perform this query at connection time just in case this information is needed later.

I tried setting the 'timezone' option on the SQL connection string, it didn't make a difference.

@jasonhillier - looks like this functionality is missing, I'll try to add it

@dougwilson , you did a lot of tz -related refactoring in node-mysql, what do you think about this idea: if timezone client parameter is set to 'auto', ( or maybe 'server' ) then do SELECT TIMEDIFF(NOW(), UTC_TIMESTAMP) query as first command automatically and apply this value to all received dates

@codecvlt
Copy link

Setting the timezone option in mysql2 to "utc", I would expect date objects returned to have a UTC timezone set. Doing this work post query seems like a hack, so I would definitely love to see this resolved.

+1

@dougwilson
Copy link
Collaborator

then do SELECT TIMEDIFF(NOW(), UTC_TIMESTAMP) query as first command automatically and apply this value to all received dates

The problem is when a connection exists during a daylight time change, the offset would then be wrong until the connection is re-created.

In addition, I think this is starting to move from a basic driver module that just does whatever commands the user sends to more of a ORM, which I think should be built as a different module on top of this module. What this module does is actually exactly what the C mysql command line client is doing in regards to time zone handling. If only JavaScript had a richer Date object that could represent "unknown timezone", that would have been best.

@embedthis
Copy link

embedthis commented Apr 26, 2016

I think the issue is packet.readDateTime is always using:

return new Date(y, m-1, d, H, M, S, ms);

when called from:

(function(){ return function BinaryRow(packet) {

this interprets the mysql date/time as local regardless of the global/session timezone of the server.

@sidorares
Copy link
Owner

@embedthis main problem is that 1) DateTime packet does not have timezone 2) JS does not have built-in "Date without known TZ" object. As a result, this library builds local Date leaving conversion to user

@embedthis
Copy link

But currently it is not symmetrical. Put one date in and you get a different value back. If you leave it up to the user, it should not get changed on only one side ie. changed on read but not on write.

@rickyk586
Copy link

Given that you can do dateStrings: true in the connection options, I think the conversion is ok to leave to the user. However I think it would be very helpful to have this in the documentation somewhere.

@embedthis
Copy link

Yes, the user can do it. The problem is that the code currently is inconsistent and not symmetrical. It converts on only one side.

@sidorares
Copy link
Owner

I'm a bit lost. Can you provide simple code examples to highlight problems @embedthis ? I'm keen to improve this, and if it requires major/minor version bumps it's good time to do now before 1.0.0 release

@rickyk586
Copy link

rickyk586 commented Aug 23, 2016

The timezone of the date (whatever the server is set to) is lost when you insert it into the DB. The only way to pull the date and set it back to the original timezone is to explicitly tell the mysql2 library which timezone to convert all dates to, and this would require the addition of an extra timezone library (if done with javascript). There's also the option to set the actual connection timezone, so maybe that's an option: SET time_zone = 'UTC'; https://dev.mysql.com/doc/refman/5.5/en/time-zone-support.html
I'm guessing the latter is how https://github.com/mysqljs/mysql does it.

@sidorares
Copy link
Owner

I don't think https://github.com/mysqljs/mysql sets server time, there is only option to tell client what's your local timezone is so server DATETIME columns are converted into js Date according to tz offset from option

@rickyk586
Copy link

Yeah that's what I meant, running SET time_zone = 'UTC'; affects the timezone for just that one client's connection. Could be a solution for @embedthis or others.

@roylee0704
Copy link

roylee0704 commented Oct 27, 2016

@sidorares I'm having similar issue. In database, datetime was stored in UTC+0, but datetime returned by node-mysql2 marks that time according to host machine's timezone.

For example, if I stores 10:00:00 UTC+0 to mysql, say node is deployed to machine with timezone: GMT+8, theDate object is returned as 10:00:00 GMT+8.

Question, how do I force to parse in UTC format?

@sidorares
Copy link
Owner

Question, how do I force to parse in UTC format?

hi @roylee0704 , right now the only option is to pass typeCast function as a connection or query parameter and do something similar to what mysqljs/mysql does here. Note that currently timezone is not passed from config to typeCast, you'll have to hard code it or use closure variable

@sidorares
Copy link
Owner

I'm keen to have this behave similar to mysqljs/mysql so that in the future you should be able to use timezone parameter

@terry-b
Copy link

terry-b commented Nov 23, 2016

I worked around this by returning unix_timestamp(date_field) which is always in UTC, and in the client turning it back into a Javascript date, which displays using the client's timezone.

@JD-V
Copy link

JD-V commented Feb 20, 2017

Well, i think this issues relates with this and the fix provided was using datestring param like,
mysql.createConnection({ user: 'foo', password: 'bar', dateStrings: true });

correct me if i am wrong !

@ozyman42
Copy link

ozyman42 commented Mar 9, 2017

Is there a timezone parameter yet?

@ozyman42
Copy link

ozyman42 commented Mar 9, 2017

@terry-b what is the unix_timestamp function?

@ozyman42
Copy link

ozyman42 commented Mar 9, 2017

Ah, you meant the SQL function. Thanks.

@ozyman42
Copy link

ozyman42 commented Mar 9, 2017

For anyone who cannot / does not want to change their SQL or other code, here is an example of fixing with a connection-level typeCast.
Connection config before:

{
    host: 'localhost',
    user: 'example_user',
    port: '3306',
    password: secrets.db_password,
    database: 'example_db'
}

Connection config after:

{
    host: 'localhost',
    user: 'example_user',
    port: '3306',
    password: secrets.db_password,
    database: 'example_db',
    typeCast(field, next) {
        if(field.type == 'DATETIME') {
            const utcTime = Math.floor((new Date(field.string() + " UTC")).getTime() / 1000);
            const fixedDate = new Date(0);
            fixedDate.setUTCSeconds(utcTime);
            return fixedDate;
        }
        return next();
    }
}

Hopefully the priority of the missing timezone option gets moved up soon, because otherwise anyone migrating from a rails-generated database will find that all of their created_at and updated_at fields return off times.

@sidorares
Copy link
Owner

@alexleung I wish people stop using DATETIME in mysql, it's pretty useless due to lack of timezone information. Personally I prefer unix timestamp

Can't promise when I have enough time to work on timezone option but happy to review PRs on that

@ozyman42
Copy link

ozyman42 commented Mar 10, 2017

I'm not a fan of DATETIME either for that reason. Rails' ActiveRecord just uses it by default though.

@ozyman42
Copy link

I may take you up on that offer for a PR. I'll have some time in 2 weeks to do that.

@leearmstrong
Copy link

@alexleung Did you manage to do this?

@Neonit
Copy link

Neonit commented Oct 9, 2017

So this is still an issue?

@alexleung Why would I prefer your date conversion function over the following?

{
    host: 'localhost',
    user: 'example_user',
    port: '3306',
    password: secrets.db_password,
    database: 'example_db',
    typeCast(field, next) {
        if(field.type == 'DATETIME') {
            return new Date(field.string() + 'Z');
        }
        return next();
    }
}

It appears to be a bit overcomplicated. Also, the documentation on setUTCSeconds states the given value should be between 0 and 59.

aikar added a commit to aikar/node-mysql2 that referenced this issue Feb 16, 2018
This resolves sidorares#262

Also discovered missing config keys that affect packet parsers not
being included in the cache key for packet parser cache, so added them.
aikar added a commit to aikar/node-mysql2 that referenced this issue Feb 16, 2018
This resolves sidorares#262

if timezone is set, we change the timezone of the connection upon opening.

If timezone is set to utc and not using dateStrings, we create Date objects
using UTC mode instead.

Also discovered missing config keys that affect packet parsers not
being included in the cache key for packet parser cache, so added them.
aikar added a commit to aikar/node-mysql2 that referenced this issue Feb 20, 2018
This resolves sidorares#262

if timezone is set, we change the timezone of the connection upon opening.

If timezone is set to utc and not using dateStrings, we create Date objects
using UTC mode instead.

Also discovered missing config keys that affect packet parsers not
being included in the cache key for packet parser cache, so added them.
aikar added a commit to aikar/node-mysql2 that referenced this issue Feb 20, 2018
This resolves sidorares#262

Also discovered missing config keys that affect packet parsers not
being included in the cache key for packet parser cache, so added them.
aikar added a commit to aikar/node-mysql2 that referenced this issue Feb 20, 2018
This resolves sidorares#262

Also discovered missing config keys that affect packet parsers not
being included in the cache key for packet parser cache, so added them.
aikar added a commit to aikar/node-mysql2 that referenced this issue Feb 20, 2018
This resolves sidorares#262

if timezone is set, we change the timezone of the connection upon opening.

If timezone is set to utc and not using dateStrings, we create Date objects
using UTC mode instead.

Also discovered missing config keys that affect packet parsers not
being included in the cache key for packet parser cache, so added them.
aikar added a commit to aikar/node-mysql2 that referenced this issue Feb 21, 2018
This resolves sidorares#262

Also discovered missing config keys that affect packet parsers not
being included in the cache key for packet parser cache, so added them.
aikar added a commit to aikar/node-mysql2 that referenced this issue Feb 21, 2018
This resolves sidorares#262

Also discovered missing config keys that affect packet parsers not
being included in the cache key for packet parser cache, so added them.
@mdierolf
Copy link

mdierolf commented Nov 18, 2018

In my not at all humble opinion it is a terrible mistake for a developer to rely on MySQL default time/timezone behavior. Their are too many ways for it to go wrong and in nearly every case it introduces silent errors that occur infrequently that the developer will most likely not be aware of.

MySQL is fundamentally flawed in that it allows inserting a time without a well defined timezone. I.E. the database "guesses" what timezone you meant instead of forcing you to send the timezone with the time.

For example, during daylight savings transition you have a repeat of one hour of local time, and the timezone on client/server silently changes on you, could be DST or not.

So it's pretty easy to accidentally insert a date that is one hour off, and if the clocks on server and client are not synced well, you could even end up inserting a date 2 hours off in unusual edge cases.

Javascript has it's own issues too which makes this even worse. As far as I know their is no guarantee in the language spec that the local date functions Date.getHours(), Date.getTimezoneOffset(), and others will work atomically and use the same calculated timezone offset during DST changes. So their is no practical way to extract local date & time & timezone in a deterministic way.

As such, I think all the Date.getXXX() functions are unwise to use in the real world - they are not reliable.

When storing date/times I use something similar to the following to avoid bad timezone behavior:
JS: dateField = dateField.getTime() / 1000;
SQL: INSERT INTO foo (dateField) VALUES (FROM_UNIXTIME(?));

When retrieving date/times I use the following:
SQL: SELECT UNIX_TIMESTAMP(dateField) as dateField FROM foo;
JS: dateField = new Date(dateField * 1000);

It would not be unreasonable for the library print an error advising the developer they are doing something bad when encountering a date/time field being sent or received to MySQL.

But I guess you could use the Date.getUTCXXX() functions to output the time and force the timezone to UTC on each connection if you want to be nicer.

To go further down the rabbit hole, I don't think most people should store information in DATE/DATETIME/TIMESTAMP fields at all.

What's with the stupid 0000-00-00 00:00:00 timestamp. Why do we need a zero time? I have no idea. Should be NULL. Causes me nothing but problems.

DATE & DATETIME only go back to the year 1000, so no good for historical dates, they use more storage than timestamp, and are slow to convert to epoch time, which is the only time which MySQL, operating systems, and most programming languages agree upon.

TIMESTAMP is 32 bit unix epoch time in disguise, and runs out into big problems in 2038, and doesn't support dates before 1970.

None of them support millisecond or smaller units.

Presumably when TIMESTAMP runs out of storage MySQL will switch to a 64 bit format.

So why not use 64 bit numbers today? If you store milliseconds since epoch in a SIGNED BIGINT, it is deterministic, easy to parse with lots of programming languages, easy to convert to date formats in MySQL using FROM_UNIXTIME(field / 1000), you can store dates that are hundreds of thousands of years old, can work on it with integer math, and it fits in the registers on 64 bit cpus which are now the norm. You won't be suffering when TIMESTAMP runs out of range in less than 20 years.

@ozyman42
Copy link

I'm dubious about using this 64 signed bigint approach. For example imagine all the software that relies on MySQL in year 292,473,178 which will break. Much better to use two bigints together to represent a signed 128 bit representation of nanoseconds since epoch time.

@sidorares
Copy link
Owner

@alexleung 640kb should be enough for everybody

aikar added a commit to aikar/node-mysql2 that referenced this issue Feb 28, 2019
This resolves sidorares#262

Also discovered missing config keys that affect packet parsers not
being included in the cache key for packet parser cache, so added them.
aikar added a commit to aikar/node-mysql2 that referenced this issue Feb 28, 2019
This resolves sidorares#262

Also discovered missing config keys that affect packet parsers not
being included in the cache key for packet parser cache, so added them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.