Skip to content

Commit 5aa59f6

Browse files
JimblyJimbly
committed
Do not use local-timezone dates for doing date to string conversions to specific timezones.
This is certainly not valid because, if your local timezone includes daylight saving time, there is no possible Javascript date value which could return the string, for instance, "2015-03-08 02:30". Instead, use the getUTC methods of the Date object after simply adjusting the timestamp by the desired timezone offset. I had difficultly understanding what in the world the test_timezone.js test was doing, so I wrote a new one. The new test is specifically: No matter what date is used, no matter what system timezone, no matter what node-mysql timezone, no matter what server-side timezone, if you insert a Javascript Date object, you should get the same Date object back (this was previously failing if that date object happened to be near daylight saving time boundaries in the local system timezone). Additionally, the string representation of the date in MySQL should be in the timezone specified by the node-mysql timezone. As far as I know the server-side MySQL timezone variable doesn't affect DATETIME objects (however it does affect TIMESTAMP objects, which will currently fail a bunch of these tests, but that's probably a more significant change for another PR another day...). After doing all of this, I went back to read through the old test_timezone, and saw it was suffering from the same general issues as SqlString.js - it was using the local timezone string representation of a date to store arbitrary dates (that typeCast function would just generate an invalid date object if run during one of the selected timezone offsets from 2:30am on the day daylight saving time kicks in, among other problems). Rule of thumb with Javascript dates: if you're calling .getTimezoneOffset() on a date object, you're probably doing it wrong.
1 parent 93bf3be commit 5aa59f6

File tree

2 files changed

+103
-43
lines changed

2 files changed

+103
-43
lines changed

‎lib/protocol/SqlString.js

+22-12
Original file line numberDiff line numberDiff line change
@@ -84,24 +84,34 @@ SqlString.format = function(sql, values, stringifyObjects, timeZone) {
8484
SqlString.dateToString = function(date, timeZone) {
8585
var dt = new Date(date);
8686

87-
if (timeZone != 'local') {
87+
var year;
88+
var month;
89+
var day;
90+
var hour;
91+
var minute;
92+
var second = dt.getUTCSeconds();
93+
var millisecond = dt.getUTCMilliseconds();
94+
95+
if (timeZone === 'local') {
96+
year = dt.getFullYear();
97+
month = dt.getMonth() + 1;
98+
day = dt.getDate();
99+
hour = dt.getHours();
100+
minute = dt.getMinutes();
101+
} else {
88102
var tz = convertTimezone(timeZone);
89-
90-
dt.setTime(dt.getTime() + (dt.getTimezoneOffset() * 60000));
91103
if (tz !== false) {
92104
dt.setTime(dt.getTime() + (tz * 60000));
93105
}
106+
year = dt.getUTCFullYear();
107+
month = dt.getUTCMonth() + 1;
108+
day = dt.getUTCDate();
109+
hour = dt.getUTCHours();
110+
minute = dt.getUTCMinutes();
94111
}
95112

96-
var year = dt.getFullYear();
97-
var month = zeroPad(dt.getMonth() + 1, 2);
98-
var day = zeroPad(dt.getDate(), 2);
99-
var hour = zeroPad(dt.getHours(), 2);
100-
var minute = zeroPad(dt.getMinutes(), 2);
101-
var second = zeroPad(dt.getSeconds(), 2);
102-
var millisecond = zeroPad(dt.getMilliseconds(), 3);
103-
104-
return year + '-' + month + '-' + day + ' ' + hour + ':' + minute + ':' + second + '.' + millisecond;
113+
return year + '-' + zeroPad(month, 2) + '-' + zeroPad(day, 2) + ' ' +
114+
zeroPad(hour, 2) + ':' + zeroPad(minute, 2) + ':' + zeroPad(second, 2) + '.' + zeroPad(millisecond, 3);
105115
};
106116

107117
SqlString.bufferToString = function bufferToString(buffer) {

‎test/integration/connection/test-timezones.js

+81-31
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,17 @@ var assert = require('assert');
22
var common = require('../../common');
33

44
var table = 'timezone_test';
5-
var tests = [0, 1, 5, 12, 26, -1, -5, -20, 'Z', 'local'];
5+
var pre_statements = ['', 'SET TIME_ZONE="+00:00"', 'SET TIME_ZONE="SYSTEM"'];
6+
var pre_idx = 0;
7+
var test_days = ['01-01', '03-07', '03-08', '03-09', '12-31'].map(function (day) {
8+
// Choosing this because 2015-03-08 02:30 Pacific does not exist (due to DST),
9+
// so if anything is using a local date object it will fail (at least if the
10+
// test system is in Pacific Time).
11+
return '2015-' + day + 'T02:32:11.000Z';
12+
});
13+
var day_idx = 0;
14+
var test_timezones = ['Z', 'local', 0, 1, 5, 12, 23, -1, -5, -20];
15+
var tz_idx = 0;
616

717
common.getTestConnection(function (err, connection) {
818
assert.ifError(err);
@@ -11,61 +21,101 @@ common.getTestConnection(function (err, connection) {
1121

1222
connection.query([
1323
'CREATE TEMPORARY TABLE ?? (',
14-
'`offset` varchar(10),',
24+
'`day` varchar(24),',
25+
'`timezone` varchar(10),',
1526
'`dt` datetime',
1627
') ENGINE=InnoDB DEFAULT CHARSET=utf8'
1728
].join('\n'), [table], assert.ifError);
1829

30+
if (pre_statements[pre_idx]) {
31+
connection.query(pre_statements[pre_idx], assert.ifError);
32+
}
1933
testNextDate(connection);
2034
});
2135

2236
function testNextDate(connection) {
23-
if (tests.length === 0) {
24-
connection.end(assert.ifError);
25-
return;
37+
if (tz_idx === test_timezones.length || day_idx === test_days.length) {
38+
++pre_idx;
39+
if (pre_idx === pre_statements.length) {
40+
connection.end(assert.ifError);
41+
return;
42+
} else {
43+
connection.query(pre_statements[pre_idx], assert.ifError);
44+
day_idx = tz_idx = 0;
45+
}
2646
}
2747

28-
var dt = new Date();
29-
var offset = tests.pop();
48+
var day = test_days[day_idx];
49+
var offset = test_timezones[tz_idx];
3050

31-
// datetime will round fractional seconds up, which causes this test to fail
32-
// depending on when it is executed. MySQL 5.6.4 and up supports datetime(6)
33-
// which would not require this change.
34-
// http://dev.mysql.com/doc/refman/5.6/en/fractional-seconds.html
35-
dt.setSeconds(0);
36-
dt.setMilliseconds(0);
51+
++day_idx;
52+
if (day_idx === test_days.length) {
53+
day_idx = 0;
54+
++tz_idx;
55+
}
3756

38-
if (offset === 'Z' || offset === 'local') {
39-
connection.config.timezone = offset;
57+
var timezone;
58+
if (offset === 'Z') {
59+
timezone = offset;
60+
offset = 0;
61+
} else if (offset === 'local') {
62+
timezone = offset;
4063
} else {
41-
connection.config.timezone = (offset < 0 ? "-" : "+") + pad2(Math.abs(offset)) + ":00";
64+
timezone = (offset < 0 ? "-" : "+") + pad2(Math.abs(offset)) + ":00";
4265
}
4366

44-
connection.query('INSERT INTO ?? SET ?', [table, {offset: offset, dt: dt}], assert.ifError);
67+
var dt = new Date(day);
68+
assert.strictEqual(day, dt.toISOString());
4569

46-
if (offset === 'Z') {
47-
dt.setTime(dt.getTime() + (dt.getTimezoneOffset() * 60000));
48-
} else if (offset !== 'local') {
49-
dt.setTime(dt.getTime() + (dt.getTimezoneOffset() * 60000) + (offset * 3600000));
70+
var expected_date_string;
71+
if (offset === 'local') {
72+
// If using a local timezone, it should be the same day/hour/etc as the Javascript date formatter
73+
// Beware Daylight Saving Time though, using a "local" timezone is never a good idea, it maps
74+
// multiple unique UTC dates to the same string.
75+
expected_date_string = dt.getFullYear() + '-' + pad2(dt.getMonth() + 1) + '-' + pad2(dt.getDate()) + ' ' +
76+
pad2(dt.getHours()) + ':' + pad2(dt.getMinutes()) + ':' + pad2(dt.getSeconds());
77+
} else {
78+
// If using a specific timezone, it should be a simple offset from the UTC date
79+
var expected_dt = new Date(dt.getTime() + offset * 60*60*1000);
80+
expected_date_string = expected_dt.getUTCFullYear() + '-' +
81+
pad2(expected_dt.getUTCMonth() + 1) + '-' +
82+
pad2(expected_dt.getUTCDate()) + ' ' +
83+
pad2(expected_dt.getUTCHours()) + ':' +
84+
pad2(expected_dt.getUTCMinutes()) + ':' +
85+
pad2(expected_dt.getUTCSeconds());
5086
}
5187

52-
dt.setSeconds(0);
53-
dt.setMilliseconds(0);
88+
connection.config.timezone = timezone;
89+
connection.query('INSERT INTO ?? SET ?', [table, {day: day, timezone: timezone, dt: dt}], assert.ifError);
5490

5591
var options = {
56-
sql: 'SELECT * FROM ?? WHERE offset = ?',
92+
sql: 'SELECT * FROM ?? WHERE timezone = ? AND day = ?',
93+
values: [table, timezone, day],
5794
typeCast: function (field, next) {
58-
if (field.type != 'DATETIME') return next();
59-
60-
return new Date(field.string());
95+
if (field.type !== 'DATETIME') {
96+
return next();
97+
}
98+
return field.string();
6199
},
62-
values: [table, offset]
63100
};
64101

65-
connection.query(options, function (err, rows) {
102+
connection.query(options, function (err, rows_raw) {
66103
assert.ifError(err);
67-
assert.strictEqual(dt.toString(), rows[0].dt.toString());
68-
testNextDate(connection);
104+
delete options.typeCast;
105+
connection.query(options, function (err, rows) {
106+
assert.ifError(err);
107+
if (dt.getTime() !== rows[0].dt.getTime() || expected_date_string !== rows_raw[0].dt) {
108+
console.log('Failure while testing date: ' + day + ', Timzone: ' + timezone);
109+
console.log('Pre-statement: ' + pre_statements[pre_idx]);
110+
console.log('Expected raw string: ' + expected_date_string);
111+
console.log('Received raw string: ' + rows_raw[0].dt);
112+
console.log('Expected date object: ' + dt.toISOString() + ' (' + dt.getTime() + ', ' + dt.toLocaleString() + ')');
113+
console.log('Received date object: ' + rows[0].dt.toISOString() + ' (' + rows[0].dt.getTime() + ', ' + rows[0].dt.toLocaleString() + ')');
114+
assert.strictEqual(expected_date_string, rows_raw[0].dt);
115+
assert.strictEqual(dt.toISOString(), rows[0].dt.toISOString());
116+
}
117+
testNextDate(connection);
118+
});
69119
});
70120
}
71121

0 commit comments

Comments
 (0)