Support for UTC dates as a config param #289

BryanDonovan · 2012-09-12T22:40:48Z

This pull request adds support for an optional config param named "utc" (boolean), when, if set, causes dates in result packets to be treated as UTC. In particular, the RowDataPacket.prototype._typeCast() function does this:

      if (utc) {
        dateString += 'Z';
      }

This functionality is needed because setting process.env.TZ is not reliable. If any Date manipulation (E.g., new Date.getTimezoneOffset()) happens before process.env.TZ is set, then the Date class's timezone will be stuck at what it was before process.env.TZ is set. See related bug #128.

I've added two new integration tests that cover how node-mysql deals with dates and timezones.

I can't say I like adding another param to these functions, e.g.,:

RowDataPacket.prototype.parse = function(parser, fieldPackets, typeCast, nestTables, utc) {

Should I try adding an options hash as the third param in that function instead? It would hold the 'typeCast', 'nestTables', and 'utc' args.

BryanDonovan · 2012-09-12T22:51:58Z

I also made a different branch where the param name is 'tz' instead of 'utc' to possibly allow for a more complete timezone solution in the future.
https://github.com/BryanDonovan/node-mysql/tree/tz-param

dresende · 2012-10-25T18:20:40Z

Instead of having an utc flag, why not a tz option, where you could put something like "+01:30" or "-01:00" or even "Z"?

Use it like: ```js mysql.createConnection("mysql://.....?timezone=+02:00") ``` ("Z" is also possible for UTC dates)

BryanDonovan · 2012-10-26T19:54:14Z

dresende, yes, that's a good idea. I headed down that path with this version: BryanDonovan@76842d7

dresende · 2012-10-26T22:31:39Z

I did this on master but I'm still fighting with the code on a local branch to make all dates synched from/to database. I'll merge it as soon as it's stable and tested.

BryanDonovan · 2012-10-26T22:33:41Z

Maybe just copy the timezone tests from my branch over, and modify?

dresende · 2012-10-26T22:38:19Z

I'll check them out soon :)

dresende · 2012-10-29T10:37:39Z

I pushed the branch, you can check it here 9c6775f.

Can anyone test this? I did some queries locally with several timezones (like +22:00 or -13:45). Default is "Z" (+0000).

All tests seem to pass. Can anyone verify? @BryanDonovan @felixge

BryanDonovan · 2012-10-29T15:56:53Z

I get a test failure during "make test":

[0:00:13 2 43/50 90.0% node test/integration/connection/test-type-casting.js]                               

  assert.js:102
    throw new assert.AssertionError({
          ^
  AssertionError: got: "Sat May 12 2012 04:00:23 GMT-0700 (PDT)" expected: "Sat May 12 2012 11:00:23 GMT-0700 (PDT)" test: timestamp
      at /Users/bryan/code/node/node-mysql/test/integration/connection/test-type-casting.js:114:14
      at Array.forEach (native)
      at process.<anonymous> (/Users/bryan/code/node/node-mysql/test/integration/connection/test-type-casting.js:91:9)
      at process.EventEmitter.emit (events.js:93:17)

[0:00:14 3 47/50 100.0% node test/unit/protocol/test-SqlString.js]                  
make: *** [test] Error 1
bryan ~/code/node/node-mysql(master) $ mysql -v
Welcome to the MySQL monitor.  Commands end with ; or \g.
Your MySQL connection id is 162971
Server version: 5.1.45 MySQL Community Server (GPL)

BryanDonovan · 2012-10-29T16:05:49Z

Also, it looks like UTC is now the default? I personally prefer that, but it would be a dangerous change for people not expecting it.

dresende · 2012-10-29T16:08:22Z

About your test, that's weird. I'm on GMT+1 but changed the test/run to use UTC-5:00 and it worked. I'm going to look a bit more to that test.

About UTC being default, you're probably right, or not.. I don't know. I think people not complaining about not having UTC by default don't even know the problem their raising in the future. The others needing UTC will thank us :)

But yeah, it's probably better to default to local.

dresende · 2012-10-29T16:10:58Z

Are you sure you tested the branch or the master? I'm having the same error on the master but not on the timezone branch.

dresende · 2012-10-29T16:11:55Z

If you need assistance on doing that, do:

cd /your/node-mysql/repo
git pull
git checkout timezone
node test/run

BryanDonovan · 2012-10-29T16:12:31Z

Oh, nevermind.. I tested master. Sorry about that.

BryanDonovan · 2012-10-29T16:16:34Z

I personally think it would be better to leave the timezone out of the test/common.js file and set it explicitly in a test that only concerns itself with timezones. That way you have a test that covers the default situation and one that covers when you set it to something. I'd go as far as having three tests.. one for default, one for UTC, and one for an offset time.

For what it's worth, my tests pass with your code (after changing the config signature in the test of course).

dresende · 2012-10-29T16:29:19Z

Ok, when I update the tests I'll ask you to test again :)

dresende · 2012-10-29T19:28:24Z

Could you test now? This branch also fixes a regression bug on a type cast test. Default timezone in this branch is 'local'. UTC timezone can be set by using timezone=Z or timezone=+00:00. Other timezones work :)

In the tests, no timezone is enforced on common, local is used. There's a new test that tries some timezones and uses a custom typeCast function to avoid the timezone shifting when reading from the database and test if it's ok.

This custom typeCast works like this:

con.query({
  sql: "SELECT * ...",
  typeCast: function (field, parser, timeZone, next) {
    if (field.type != ...) return next(); // using default typeCast

    // do your thing...
    return ...;
  }
});

This was needed to check raw date from the database and I think some other people will like to use it for custom types.

dresende · 2012-10-29T19:29:17Z

@felixge : any objection on merging this branch (https://github.com/felixge/node-mysql/tree/timezone) if everything looks ok?

felixge · 2012-10-29T20:53:11Z

@dresende can you turn that branch into a pull request? difficult to review otherwise.

BryanDonovan · 2012-10-29T20:55:42Z

I get a test failure:

AssertionError: "Mon Oct 29 2012 13:54:07 GMT-0700 (PDT)" === "Mon Oct 29 2012 20:54:07 GMT-0700 (PDT)"
at Query._callback (/Users/bryan/code/node/node-mysql/test/integration/connection/test-timezones.js:59:10)

.. that's when the offset is undefined. It also fails when it's 'Z':
AssertionError: "Mon Oct 29 2012 06:53:32 GMT-0700 (PDT)" === "Mon Oct 29 2012 13:53:32 GMT-0700 (PDT)"

dresende · 2012-10-29T21:22:11Z

@felixge : ok
@BryanDonovan : i'll try to change my local zone and test again

dresende · 2012-10-29T21:33:46Z

I forgot my zone switched off DST last Sunday so now I'm on UTC. That's why my tests passed :P

dresende · 2012-10-29T22:14:36Z

@BryanDonovan : could you test now? I think it's ok now.

BryanDonovan · 2012-10-30T02:37:38Z

Your test passes, but this does not, which concerns me a bit..

var common = require('../../common');
var assert = require('assert');
var connection = common.createConnection();

common.useTestDb(connection);

var table = 'insert_test';
connection.query([
  'CREATE TEMPORARY TABLE `' + table + '` (',
  '`id` int(11) unsigned NOT NULL AUTO_INCREMENT,',
  '`created_at` datetime,',
  'PRIMARY KEY (`id`)',
  ') ENGINE=InnoDB DEFAULT CHARSET=utf8'
].join('\n'));

var result;
var now = new Date();
// Removing the decimal portion of the date string for Travis CI.
var nowString = now.toISOString().replace(/\.\d\d\dZ/, '');

connection.query('INSERT INTO ' + table + ' SET ?', {created_at: nowString}, function(err, _result) {
  if (err) throw err;

  var sql = 'SELECT * FROM ' + table + ' WHERE id = ?';
  connection.query(sql, [_result.insertId], function(err, _result) {
      console.log(_result);
    connection.end();
    if (err) throw err;

    result = _result[0].created_at;
  });
});


process.on('exit', function() {
  var tzOffsetHours = new Date().getTimezoneOffset() / 60;
  var expectedHours = now.getHours() + tzOffsetHours;
  var actualHours = result.getHours();
  console.log(result);
  assert.strictEqual(actualHours, expectedHours);
});

I might be missing something though.

BryanDonovan · 2012-10-30T03:42:38Z

This business of passing the timezone around bothers me to, but it's not my project, so as long as it works, it's good by me :)

    return SqlString.escape(values.shift(), false, timeZone);

I really hate passing in three arguments to a method, and hate passing booleans in even more. the second argument should be an options hash. It would obviously take a lot of refactoring to make that happen though :).

dresende · 2012-10-30T09:39:29Z

I agree with you about the options argument, maybe later. Timezone is needed to escape dates and you don't need to pass it, the exported .escape() will use the connection config timezone.

Can you post the output of your test (the assert.stringEqual result) ?

BryanDonovan · 2012-10-30T16:06:16Z

You mean post what the values are here?

  assert.strictEqual(actualHours, expectedHours);

dresende · 2012-10-30T16:09:07Z

No, I meant the error, to see the time diff :)

BryanDonovan · 2012-10-30T16:12:55Z

Well, it's passing now. Yesterday the time diff was 24 hours, so I imagine this test will fail for me later today after midnight UTC.

Here's the output from yesterday when it failed:

AssertionError: 2 === 26
    at process.<anonymous> (/Users/bryan/code/node/node-mysql/test/integration/connection/test-timezone-default.js:39:10)

# Ran it again an hour later and got this:
AssertionError: 3 === 27

BryanDonovan · 2012-10-30T16:14:31Z

Ah, I bet this is just a misuse of Date().getTimezoneOffset() or Date().getHours() in my test. I'll try to debug again tonight.

dresende · 2012-10-30T16:19:08Z

Ok, please do. Timezones are freaking bad to debug. I spent a couple of hours to do this right. And I tested with +30:00 and ridiculous offsets (so hours and even days can be different) and it worked.

BryanDonovan · 2012-10-30T16:21:10Z

Will do. Yeah, timezones are very painful (which is why everyone should save as UTC in their databases :)).

BryanDonovan · 2012-10-31T02:40:54Z

Everything appears to be working fine..

dresende · 2012-10-31T09:34:44Z

Ok, I'm making a pull request out of the branch.

Philmod · 2012-10-31T10:14:23Z

So gooood news! 👍

Add support for returning table alias on Columns()

BryanDonovan added 3 commits September 12, 2012 15:00

working utc config option

4d95a9f

Can't set this.utc. Passing it as a param instead

244dede

splitting timezone tests into two files

cbbd76d

BryanDonovan added 3 commits September 12, 2012 15:59

Removing the decimal portion of the date string for Travis CI

d76fb08

trying to get a date string that Travis CI's MySQL server likes

8c5879c

fixing date format in test-timezone-default.js

4bde2a8

dresende added a commit that referenced this pull request Oct 25, 2012

Adds "timezone" config param (related to #291 and #289)

8d7d814

Use it like: ```js mysql.createConnection("mysql://.....?timezone=+02:00") ``` ("Z" is also possible for UTC dates)

joeferner mentioned this pull request Oct 30, 2012

Incorrect datetime value for migrations table insert on Travis-CI db-migrate/node-db-migrate#54

Closed

dresende closed this Oct 31, 2012

dveeden pushed a commit to dveeden/mysql that referenced this pull request Jan 31, 2023

Merge pull request mysqljs#289 from chrismoos/expose_aliases

511937c

Add support for returning table alias on Columns()

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for UTC dates as a config param #289

Support for UTC dates as a config param #289

BryanDonovan commented Sep 12, 2012

BryanDonovan commented Sep 12, 2012

dresende commented Oct 25, 2012

BryanDonovan commented Oct 26, 2012

dresende commented Oct 26, 2012

BryanDonovan commented Oct 26, 2012

dresende commented Oct 26, 2012

dresende commented Oct 29, 2012

BryanDonovan commented Oct 29, 2012

BryanDonovan commented Oct 29, 2012

dresende commented Oct 29, 2012

dresende commented Oct 29, 2012

dresende commented Oct 29, 2012

BryanDonovan commented Oct 29, 2012

BryanDonovan commented Oct 29, 2012

dresende commented Oct 29, 2012

dresende commented Oct 29, 2012

dresende commented Oct 29, 2012

felixge commented Oct 29, 2012

BryanDonovan commented Oct 29, 2012

dresende commented Oct 29, 2012

dresende commented Oct 29, 2012

dresende commented Oct 29, 2012

BryanDonovan commented Oct 30, 2012

BryanDonovan commented Oct 30, 2012

dresende commented Oct 30, 2012

BryanDonovan commented Oct 30, 2012

dresende commented Oct 30, 2012

BryanDonovan commented Oct 30, 2012

BryanDonovan commented Oct 30, 2012

dresende commented Oct 30, 2012

BryanDonovan commented Oct 30, 2012

BryanDonovan commented Oct 31, 2012

dresende commented Oct 31, 2012

Philmod commented Oct 31, 2012

Support for UTC dates as a config param #289

Support for UTC dates as a config param #289

Conversation

BryanDonovan commented Sep 12, 2012

BryanDonovan commented Sep 12, 2012

dresende commented Oct 25, 2012

BryanDonovan commented Oct 26, 2012

dresende commented Oct 26, 2012

BryanDonovan commented Oct 26, 2012

dresende commented Oct 26, 2012

dresende commented Oct 29, 2012

BryanDonovan commented Oct 29, 2012

BryanDonovan commented Oct 29, 2012

dresende commented Oct 29, 2012

dresende commented Oct 29, 2012

dresende commented Oct 29, 2012

BryanDonovan commented Oct 29, 2012

BryanDonovan commented Oct 29, 2012

dresende commented Oct 29, 2012

dresende commented Oct 29, 2012

dresende commented Oct 29, 2012

felixge commented Oct 29, 2012

BryanDonovan commented Oct 29, 2012

dresende commented Oct 29, 2012

dresende commented Oct 29, 2012

dresende commented Oct 29, 2012

BryanDonovan commented Oct 30, 2012

BryanDonovan commented Oct 30, 2012

dresende commented Oct 30, 2012

BryanDonovan commented Oct 30, 2012

dresende commented Oct 30, 2012

BryanDonovan commented Oct 30, 2012

BryanDonovan commented Oct 30, 2012

dresende commented Oct 30, 2012

BryanDonovan commented Oct 30, 2012

BryanDonovan commented Oct 31, 2012

dresende commented Oct 31, 2012

Philmod commented Oct 31, 2012