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

Already on GitHub? Sign in to your account

datetime -> epoch ignores timezone #119

Closed
ngandhy opened this Issue Nov 5, 2013 · 15 comments

Comments

Projects
None yet
3 participants
Contributor

ngandhy commented Nov 5, 2013

Hi. First, thanks for ultrajson and ultramysql!

Problem

When given a UTC time and a timezoned version of the same value (e.g America/New_York) they give different values when "jsonified". They should give you the same epoch value.

Repro:

import datetime, pytz, ujson
a = datetime.datetime.utcfromtimestamp(1383647400)
a = a.replace(tzinfo=pytz.utc)
b = a.astimezone(pytz.timezone('America/New_York'))
c = b.astimezone(pytz.utc)
d = {'a':a, 'b':b, 'c':c}
ujson.dumps(d)

Result: (Notice 'b' is wrong)

'{"a":1383647400,"c":1383647400,"b":1383629400}'

Should be:

'{"a":1383647400,"c":1383647400,"b":1383647400}'

Contributor

ngandhy commented Nov 5, 2013

I took a quick try at fixing the issue. (Ill start using it and testing it now). Feedback would be appreciated.

https://github.com/ngandhy/ultrajson/compare

Contributor

ngandhy commented Dec 16, 2013

ping. any feedback?

Member

jskorpan commented Dec 16, 2013

Sorry for late reply

So the fix is to make datetime object to always encode as UTC?

Contributor

ngandhy commented Dec 16, 2013

No worries. Just wanted to check in.

Yup. I believe unix time is generally always UTC (as its seconds since Jan 1 1970 00:00:00 UTC). Also it seems to be common convention in other libraries to store timestamps as UTC if timezone isn't stored with it.

I believe that makes sense for ujson as well. What you think?

Member

jskorpan commented Dec 16, 2013

It would seem anyone handling timestamps in non UTC would be doing so for a specific reason, maybe we shouldn’t interfere
//JT

From: ngandhy [mailto:notifications@github.com]
Sent: den 16 december 2013 10:22
To: esnme/ultrajson
Cc: Jonas Tärnström
Subject: Re: [ultrajson] datetime -> epoch ignores timezone (#119)

No worries. Just wanted to check in.

Yup. I believe unix time is generally always UTC (as its seconds since Jan 1 1970 00:00:00 UTC). Also it seems to be common convention in other libraries to store timestamps as UTC if timezone isn't stored with it.

I believe that makes sense for ujson as well. What you think?


Reply to this email directly or view it on GitHubhttps://github.com/esnme/ultrajson/issues/119#issuecomment-30646098.

Contributor

ngandhy commented Dec 16, 2013

Datetime is often handled in non-utc for server side display purposes, converting them back to UTC before jsonify-ing is a non-obvious thing to do which makes all this confusing to track down.

Right now we potentially pass around a unix time stamp which will incorrectly convert back to a date time in any default timestamp converter (like JS).

^ timestamps are from the initial example ^

var date = new Date(1383647400_1000);
Tue Nov 05 2013 02:30:00 GMT-0800 (PST)
var date2 = new Date(1383629400_1000);
Mon Nov 04 2013 21:30:00 GMT-0800 (PST)

Contributor

ngandhy commented Jan 29, 2014

Wanted to add this is inconsistent with how the sister library ultramysql works.

Also to rephrase - unix timestamps are supposed to be UTC by definition. "Unix time, or POSIX time, is a system for describing instants in time, defined as the number of seconds that have elapsed since 00:00:00 Coordinated Universal Time (UTC)". By not respecting that when converting datetimes to unix time stamps we are breaking the definition of a unix timestamp and corrupting the data.

Member

jskorpan commented Apr 14, 2014

Please provide test case and pull request and I'll throw it in

@jskorpan jskorpan closed this Apr 14, 2014

YAmikep commented Nov 7, 2014

@jskorpan @ngandhy
Is this bug fixed? Just tested with ujson 1.33 and there is still the issue.

Thanks

Contributor

ngandhy commented Nov 7, 2014

Nope. The fix is in my pull request tho.

Just added the test case and opening a pull request in a minute. @YAmikep if you can take a look at it and code review it would probably help expedite the process when @jskorpan looks at this project again.

YAmikep commented Nov 8, 2014

I wish I could help but I am not familiar with how dates and timezones are handled in C. I hope someone more familiar with it could look at it.

Thanks

Contributor

ngandhy commented Nov 8, 2014

@YAmikep try running the tests for now. I think @jskorpan already did the code review of the changes.

YAmikep commented Nov 8, 2014

By the way, since the dates are converted into int, you lose the information that one was a date.

ujson.loads(ujson.dumps(d))
{u'a': 1383647400, u'b': 138629400, u'c': 1383647400}

Is there a way to get datetimes object back with ujson so that you get back the exact same thing?

Contributor

ngandhy commented Dec 16, 2014

thats status quo and expected behavior.

ujson.dumps knows that datetime objects need to be converted to a simple variable type to be json-ized . However an int doesn't necessarily mean its supposed to be a datetime so it can't make that assumption.

p.s - @jskorpan this should probably be re-opened until the pull request goes through.

Contributor

ngandhy commented Aug 8, 2015

@Jahaja - can we get this reopened? Also Ive had the pull request in since last year. Any chance we can get it merged in?

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