Do not assume dates with no timezone specifier are UTC #226

Closed
wants to merge 2 commits into
from

Projects

None yet

3 participants

@strk
Contributor
strk commented Dec 7, 2012

Fixes #225

@strk
Contributor
strk commented Dec 7, 2012

a possible improvement would be to enhance parseDate to take an additional argument (hasTimezone) that could default to true for backward compatibility and only set to off while parsing values of type oid 1114

@strk strk referenced this pull request in CartoDB/CartoDB-SQL-API Dec 7, 2012
Closed

Date fields in CSV are written as strings #77

@brianc
Owner
brianc commented Dec 7, 2012

Hey thanks! Working with timezones is a pain. Working with timezones in JavaScript is even worse. I appreciate this. Maybe we could do 2 things real quick before I merge.

  1. discuss a scenario where you've been bitten by this or why you'd like it changed? I'm not opposed to changing it, just want to make sure I understand the motivation for changing this. I think there was a reason way back when I initially wrote the date parsing to convert to UTC if there was a timezone...but it's been over 2 years, and I'm having a hard time remembering. 😸 I think it might have had to do with putting dates into your database from one timezone, let's say eastern time, and then retrieving them on an app server in pacific time. I wasn't sure how that would be handled. That doesn't really seem common though...who has multi-timezone datacenters and doesn't store dates in UTC? Yikes.

  2. The second thing is do you think there's a good way to test for this functionality? Changing it initially didn't break the tests in travis so it might be missing a test covering this scenario. Timezones!!!!! 😦

@strk
Contributor
strk commented Dec 7, 2012

My scenario is I have a "timestamp with no timezone" field in postgresql
(this is cartodb.com code) and a testcase that adds a value for that field
and then checks it back.

I needed this patch because the value extracted wasn't the same as the
value entered, which is unexpected for "timestamp with no timezone".

I think the PostgreSQL manual says that "timestamp with no timezone"
is assumed to be in the same timezone as the reader (yes, subject
to error, but that's by definition).

So since the writer and the reader (in my case) are on the same timezone
I would expect input and output value to match, and they do in postgresql
itself. But they don't with node-postgres, because it insists in considering
the date as UTC, and I'm not on UTC.

About testing, I think you can set a TZ env variable to something and verify
that the output doesn't change. NOTE: the testsuite is failng for me with
the change so I guess Travis is running with GMT timezone ?

@brianc
Owner
brianc commented Dec 7, 2012

Thanks for the explanation. I'm totally on board for changing this. I'll get the tests passing (I'm not in GMT either) and then merge it this weekend.

@strk
Contributor
strk commented Dec 10, 2012

Any luck with those tests ?
I can confirm that setting:

``process.env['TZ'] = 'GMT';``

fixes the test for dateParser (query-tests.js line 6)

--strk;

@brianc
Owner
brianc commented Dec 10, 2012

Sorry it took longer than I thought to get around to it. I'm going to get
it merged in today w/ a new version to NPM as well.

On Mon, Dec 10, 2012 at 8:10 AM, strk notifications@github.com wrote:

Any luck with those tests ?
I can confirm that setting:

process.env['TZ'] = 'GMT';

fixes the test for dateParser (query-tests.js line 6)

--strk;


Reply to this email directly or view it on GitHubhttps://github.com/brianc/node-postgres/pull/226#issuecomment-11195312.

@brianc
Owner
brianc commented Dec 11, 2012

merged! Bumped minor version up since it changes the way dates come back.

@brianc brianc closed this Dec 11, 2012
@brianc
Owner
brianc commented Dec 11, 2012

thanks so much for the pull req! 🎉

@nnarhinen

Ok.. so wtf..

"2012-10-10T10:30:00+03:00" goes to database as "2012-10-10-07:30:00" and comes out as "2012-10-10-07:30:00+03:00" IMO this is a regression..

@nnarhinen

So this means:

var date = new Date();
console.log(date);
pg.query("INSERT INTO foo (timestamp_column) VALUES ($1)", [date], function(err, results) {
  pg.query("SELECT * FROM foo", function(err, results) {
    console.log(results.rows[0].timestamp_column);
  });
});

Produces:

Wed Oct 10 2012 10:30:00 GMT+0300 (EEST)
Wed Oct 10 2012 07:30:00 GMT+0300 (EEST)
@strk
Contributor
strk commented Dec 18, 2012

How does [date] expand to ?
My patch only dealt with the extraction part, not the insertion.
Is insertion type-dependent too ?
If it is it should distinguish between timestamp with timezone
and timestamp without timezone.

@brianc
Owner
brianc commented Dec 18, 2012

@nnarhinen could you give a full file (minus connection string) I could execute and see the problem? If you're using timestamp without timezone I don't think there's a way to persist the timezone information through saving. According to @strk postgres specifies timestamps without timezones should come out of the database in the timezone of the client. So maybe create a temp table in your script to show which data type you're using?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment