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

Fix timezone mismatch in dateBone.py #240

Merged
merged 6 commits into from
Apr 7, 2021
Merged

Conversation

AchimSchumacher
Copy link
Contributor

Changed to always use timezone aware datetime objects, with UTC as default.
Fixed usage of localize flag.
Fixed documentation for singleValueFromClient.
Removed Workaround for a Python 2.7-only bug.
Removed datetimeToTimestamp, since it is never used. Additionally, it would convert local time into Unixtime, maintaining timezone time shifts. The posix timestamp should always be UTC-time, since no assumptions about timezone can be made.
Made pytz library mandatory.

Changed to always use timezone aware datetime objects, with UTC as default.
Fixed usage of localize flag.
Fixed documentation for singleValueFromClient.
Removed Workaround for a Python 2.7-only bug.
Removed datetimeToTimestamp, since it is never used. Additionally, it would convert local time into Unixtime, maintaining timezone time shifts. The posix timestamp should always be UTC-time, since no assumptions about timezone can be made.
Made pytz library mandatory.
@AchimSchumacher
Copy link
Contributor Author

This pull request might break existing projects due to different timezone assumptions, but always using timezone-aware datetime with UTC per default is the only way to prevent chaos.

dateBone(localize=True) should be used with care and further tested:
render/json/default might be needed to adapt to include time zone or time offset into the string, or render datetime as integer timestamp (seconds from posix time zero).

@tsteinruecken
Copy link
Contributor

Can you explain which issues you encounterd? We have a known bug that the values of datebones in add/editSuccess are always UTC, regardless of localize. But this looks like a different issue. Localizion of datebones is very tricky, f.e. we can't just always localize every datebone as suggested. If you set time to false, hour/minute/seconds will be all set to zero, if you move then to a TZ with a negative offset you'll end up with the wrong date. Likewise, if date is set to false we can't localize either, as we can't determine daylight-saving.

@AchimSchumacher
Copy link
Contributor Author

Problems encountered: in a pyodide chat-app, timestamps of messages stored to viur-backend are shifted by the timezone-offset when read back again. Sorting messages by creation time is not possible. Switching localize from True to False has no effect.

Yes, localizion of datetime objects is very tricky. One part of the problem is to use naive datetime, i.e. w/o specified timezone. This leads to ambiguity. My proposal is to always use timezone aware datetime within datebone, and from that premise start resolving single problems. This is a change of paradigm, so it should be done now before porting viur3 to more projects.

In the current version, the localize parameter is stored in init but then never used. Rather, "if self.date and self.time" is used to switch to locale behavior. In my app I need to keep timestamps in UTC time and only localize at the time of rendering. This is the recommended way, and not possible with the current datebone version.

My proposal is not to localize every datebone, but rather the opposite, always setting a fixed timezone. Setting localize=True then means to use local timezone rather than UTC and render in local time. localize=True should be generally avoided and localization (rendering in local time) should preferably be done in the client.

As in your examples:
you set time to false, then localize is False. In my proposal, timezone is then UTC. Since localize is False, you will never move to a different timezone and never apply a time offset. When written to datastore, it is UTC, when read from datastore, you can rely on it being UTC. In current version you deal with a naive datetime object and it's easy at some point to try and localize this with 'guessTimeZone'. In strftime() it is not clear if it will apply the local time offset to a naive datetime or not. Even read/write from/to datastore may implicitly add a time offset. Current behavior creates the problem, preventing naive datetimes solves it.
If date is set to false, my proposal prevents from localization, again.

Further tests may be needed for rendering localized times in Jinja via the default json renderer when localize=True. It might be better to include the timezone or time offset in the json string, so the timezone on the client side will become unambigous.

Furthermore, singleValueFromClient might need to be further enhanced. As is, it converts even datetime objects to string, and then parses the string w/o taking into account the timezone. Native datetime objects with included tzinfo should simply be returned without changes.

@tsteinruecken
Copy link
Contributor

After extensible tests im okay with this one. I'd suggest we made the json-render adding the UTC-Offset so it can be corrected in client-side applications if needed. However this will break the current VI, so we should discuss this tomorrow in the ViUR meeting.

@@ -138,10 +138,9 @@ def renderSingleBoneValue(self, value, bone, skel, key):
if bone.type == "date" or bone.type.startswith("date."):
if value:
if bone.date and bone.time:
return value.strftime("%d.%m.%Y %H:%M:%S")
return value.strftime("%d.%m.%Y %H:%M:%S%z")
Copy link
Contributor Author

@AchimSchumacher AchimSchumacher Mar 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be easier and sufficient to simply use datetime.isoformat(' '), which is equivalent to __str__, for all cases, i.e.:

if value:
  return str(value)

@akelch
Copy link
Member

akelch commented Apr 1, 2021

tzlocal module needs to be added to the Project and cores hashed requirements.txt

@tsteinruecken
Copy link
Contributor

@XeoN-GHMB This went into a separate PR #248

@tsteinruecken tsteinruecken merged commit 25e2fe2 into master Apr 7, 2021
@tsteinruecken tsteinruecken deleted the AchimSchumacher-patch-1 branch April 7, 2021 16:23
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

Successfully merging this pull request may close these issues.

None yet

3 participants