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

Wrongly serialized utc datetime #8

Closed
haluzpav opened this issue Feb 16, 2019 · 10 comments
Closed

Wrongly serialized utc datetime #8

haluzpav opened this issue Feb 16, 2019 · 10 comments
Labels
feature A new feature that awaits implementation

Comments

@haluzpav
Copy link
Contributor

haluzpav commented Feb 16, 2019

The datetime instance is wrongly serialized if initialized from utcnow(). Example:

dt_local = datetime.datetime.now()
print(dt_local)
# >>> 2019-02-16 17:48:34.714603

print(jsons.dump(dt_local))
# >>> 2019-02-16T17:48:34+01:00

dt_utc = datetime.datetime.utcnow()
print(dt_utc)
# >>> 2019-02-16 16:48:34.715108

print(jsons.dump(dt_utc))
# >>> 2019-02-16T16:48:34+01:00
# this last one is clearly wrong
@ramonhagenaars
Copy link
Owner

That is actually expected behavior, use datetime.now(tz=timezone.utc) instead.

jsons.dump first checks for timezone info inside a datetime object. Unfortunately, datetime.utc provides none. For this reason, jsons will use your location to create localized timezone info, since it cannot know that you wanted UTC times. Check out the datetime docs and search for 'datetime.utcnow'.

Here is an example if you want UTC datetimes:

>>> from datetime import datetime
>>> from datetime import timezone
>>> import jsons
>>> datetime_inst = datetime.now(tz=timezone.utc)
>>> jsons.dump(datetime_inst)
'2019-02-16T22:01:19Z'

This could have been documented better in the README. I'm working on that.

@haluzpav
Copy link
Contributor Author

haluzpav commented Feb 17, 2019

I see. now() also doesn't provide tzinfo, that's messed up.

But it still seems wrong to assume that datetime without tzinfo is in local timezone. Wouldn't it be better to not include timezone information in serialization? And / or throw a warning if this happens?

@ramonhagenaars
Copy link
Owner

I understand your stance, because I have been pondering about this myself repeatedly. My thoughts are as follows:

  • If we decided that datetime instances without tzinfo should be outputted as UTC (i.e. suffixed with a 'Z'), then datetime.now would be outputted as UTC as well. That does not seem right either.
  • If we decided to simply omit the timezone in the output, it would no longer be a valid RFC3339 datetime pattern.
  • Throwing an exception or a warning seems rather harsh and people will probably not appreciate that, no matter our good reasons for it.
  • It seems reasonable to me to state that datetime.now means the current time at the current location.
  • It is more clear that 'something is going on' when a datetime pattern with a timezone is returned, so people might get informed about the behavior of Python's datetime.utcnow.

You can still steer the behavior of jsons though. Apart from creating datetime instances with tzinfo as mentioned earlier, you can override the datetime serialization:

from jsons import set_serializer, default_datetime_serializer as deflt

jsons.set_serializer(lambda obj, **_: deflt(obj.replace(tzinfo=timezone.utc)), datetime)
print(jsons.dump(datetime.now()))  # Notice the datetime.now without tzinfo

Prints:

'2019-02-17T15:36:34Z'  # Notice the 'Z'

So currently, I'd rather leave the datetime serializer as is. This behavior could have been documented better though. I will do that.

@haluzpav
Copy link
Contributor Author

I understand but I am still unconvinced. I think it would be better to not follow RFC3339, when there's not enough information, rather than introducing buggy behavior. Standard library provides datetime.utcnow() for getting utc datetime and jsons library should be able to correctly handle it.

@haluzpav
Copy link
Contributor Author

datetime.__str__() does not attempt to guess timezone.

LOCAL_TIMEZONE = datetime.datetime.now().astimezone().tzinfo

local = datetime.datetime.now()
local_tz = datetime.datetime.now(tz=LOCAL_TIMEZONE)
utc = datetime.datetime.utcnow()
utc_tz = datetime.datetime.now(tz=datetime.timezone.utc)

print(local)                 # >>> 2019-02-17 21:40:19.422873
print(jsons.dump(local))     # >>> 2019-02-17T21:40:19+01:00

print(local_tz)              # >>> 2019-02-17 21:40:19.422873+01:00
print(jsons.dump(local_tz))  # >>> 2019-02-17T21:40:19+01:00

print(utc)                   # >>> 2019-02-17 20:40:19.422873
print(jsons.dump(utc))       # >>> 2019-02-17T20:40:19+01:00

print(utc_tz)                # >>> 2019-02-17 20:40:19.422873+00:00
print(jsons.dump(utc_tz))    # >>> 2019-02-17T20:40:19Z

@cypreess
Copy link

cypreess commented Mar 4, 2019

I would be even more radical - I would not allow to serialize not-aware dates, as 99.99999% of time this is just bad design and error of use ;-) (you can always provide some custom un-aware dates serializer if you really need to deal with them)

No one ever should have to deal with un-aware datetime. I really regret that python even uses the same type for both. Somehow for unicode handling we have "bytes" and "strings" and those two types are never to be confused again. Also no one expects handling same methods for bytes as for strings (at least starting from python 3 :) )

@ramonhagenaars
Copy link
Owner

I agree that datetimes in Python are messed up. However, raising an Exception would be a bit rude in my opinion, even though I agree that using naive datetimes is a bad decision.

As a developer, you may not always have a choice. Suppose that for some project you are forced to call some API that was badly designed - an API that returns or requires naive datetimes. Just as you thought your day couldn't get any worse, jsons abandons you and forces you into catching the error or writing a custom datetime (de)serializer.

At most, jsons could trigger a warning message. I might consider that.

@cypreess
Copy link

cypreess commented Mar 6, 2019

@ramonhagenaars that is probably the most reasonable behavior.

@ramonhagenaars
Copy link
Owner

Noted! :-)

@ramonhagenaars ramonhagenaars added the feature A new feature that awaits implementation label Mar 6, 2019
@ramonhagenaars ramonhagenaars added the in progress This issue is worked on label Mar 15, 2019
@ramonhagenaars
Copy link
Owner

This issue has been solved in 0.8.0. Serializing datetimes without timezone info will now trigger a warning. You can additionally choose to suppress warnings, see the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature that awaits implementation
Projects
None yet
Development

No branches or pull requests

3 participants