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

Question about the date type #510

Closed
jrf0110 opened this issue Jan 22, 2014 · 11 comments
Closed

Question about the date type #510

jrf0110 opened this issue Jan 22, 2014 · 11 comments

Comments

@jrf0110
Copy link
Contributor

jrf0110 commented Jan 22, 2014

The Postgres date data type has no information about timezone. That wouldn't make any sense. However, the dateParser in node-postgres defaults to Greenwich timezone due to the way it's instantiating the object.

According to this MDN article, when creating a new Date, the constructor uses the Date.parse which, if passed a string without a timezone, will default to the systems timezone. This is not the behavior I'm seeing in the node.

Consider the following on my system in GMT-0600

new Date('2014-01-24').getDate() // => 23
new Date('2014-01-24 00:00:00').getDate() // => 24
new Date( 2014, 0, 24 ).getDate() // => 24

It's my feeling that when the data type is date then we should use one of the latter two methods so that it actually defaults to system timezone. The first method is being employed by node-postgres right now:

https://github.com/brianc/node-postgres/blob/master/lib/types/textParsers.js#L17

Thoughts?

@lalitkapoor
Copy link
Contributor

@jrf0110 I think you might be right about this. According to the postgres docs (Section 8.5.3): http://www.postgresql.org/docs/9.1/static/datatype-datetime.html

To address these difficulties, we recommend using date/time types that contain both date and time when using time zones. We do not recommend using the type time with time zone (though it is supported by PostgreSQL for legacy applications and for compliance with the SQL standard). PostgreSQL assumes your local time zone for any type containing only date or time.

I think the last sentence there is probably the one to focus on.

Something else to think about

SELECT date_part('day', '2014-01-24'::date); -- returns 24

I would assume that the behavior should be the same when calling getDate in JavaScript. If this is all correct I think this would be a breaking change btw.

@benighted
Copy link
Contributor

I think the problem is that javascript is inconsistent about parsing dates (this is from GMT-5):

> new Date('2014-01-24').getDate();
23
> new Date('2014/01/24').getDate();
24
> new Date('2014-01-24').toISOString();
'2014-01-24T00:00:00.000Z'
> new Date('2014/01/24').toISOString();
'2014-01-24T05:00:00.000Z'

So its not a pg problem, but a js problem. Here is one workaround:

> new Date('2014-01-24').getDate();
23
> new Date('2014-01-24'.replace('-','/')).getDate();
24
> new Date('2014-01-24'.replace('-','/')).toISOString();
'2014-01-24T05:00:00.000Z'
> new Date('2014/01/24'.replace('-','/')).toISOString();
'2014-01-24T05:00:00.000Z'

If this is acceptable I can do a PR later to implement it.

@lalitkapoor
Copy link
Contributor

@benighted I agree it's a js issue not a postgres one. I had no idea there was that discrepancy with parsing dates. Crazy! Good to know.

@jrf0110
Copy link
Contributor Author

jrf0110 commented Jan 23, 2014

Right. Not a PG problem but a node-pg problem. My fix:

pg.types.setTypeParser( 1082, 'text', function( val ){
  return new Date( val + ' 00:00:00' );
});

@benighted
Copy link
Contributor

I was curious so I looked into this some more, and apparently the problem is that ISO-8601 dates are parsed as UTC by default while non-ISO dates are parsed using local time zone, which results in the discrepancy we are seeing.

Example:

> new Date('2014-01-24').toISOString(); // valid ISO-8601 date, assumed UTC
'2014-01-24T00:00:00.000Z'
>  new Date('2014-01-24T00:00:00').toISOString(); // valid ISO-8601 date and time, assumed UTC
'2014-01-24T00:00:00.000Z'
>  new Date('2014-01-24 00:00:00').toISOString(); // not valid ISO-8601 format, assumed local time
'2014-01-24T05:00:00.000Z'

So, the question is, do we want to assume local time or UTC when the time zone is not specified?

@lalitkapoor
Copy link
Contributor

Just some more clarification on this matter as to why this happens:
From: http://www.ecma-international.org/ecma-262/5.1/#sec-15.9.1.15

All numbers must be base 10. If the MM or DD fields are absent “01” is used as the value. If the HH, mm, or ss fields are absent “00” is used as the value and the value of an absent sss field is “000”. The value of an absent time zone offset is “Z”.

I'm not sure replacing the dashes with slashes is the best idea because It's not a supported format:

ECMAScript defines a string interchange format for date-times based upon a simplification of the ISO 8601 Extended Format. The format is as follows: YYYY-MM-DDTHH:mm:ss.sssZ

I think we should pass in the date parts via Date(year, month, date)

@benighted
Copy link
Contributor

Changing dashes to slashes or explicitly adding the " 00:00:00" time portion (with no "T") has the same effect, to break out of the ISO-8601 format, which causes js to assume local time input instead of UTC. Passing in the date parts also assumes local time, but would require parsing the pieces out of the postgres date string (which might be as simple as .split('-') and subtracting 1 from the month value) so then it would just come down to a question of performance between the 3 methods.

@lalitkapoor
Copy link
Contributor

I'm cool w/going with best performance. In that case: http://jsperf.com/fastest-way-to-parse-date-only-part-in-local-timezone

@benighted
Copy link
Contributor

Very cool @lalitkapoor 😄

Let's see what @brianc thinks about this issue, since it is a breaking change for anyone counting on the date to be interpreted as UTC.

@brianc
Copy link
Owner

brianc commented Jan 25, 2014

Yeah it's a breaking change, but I'm definitely all in favor of it to bring node-postgres more in line with expected & documented behavior -- honestly it wouldn't be the first time we've broken backwards compatibility for timezone stuff. Timezones & string encodings are hard! Also, we can merge this pull request as well & group everything up into a single major version bump -- ship all the breaking changes at once.

👍

Thanks for the good discussion in here too! 😄

@polobo
Copy link

polobo commented Sep 21, 2014

This issue appears to be resolved and able to be closed.

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

No branches or pull requests

5 participants