convert date/times to UTC when exporting, use python-datetime for parsing date strings #493
Conversation
|
ping @tisto |
|
@sneridagh can you please have a look. I won't be able to look into it this week... |
|
@thet I don't see any changes in the docs. Does that mean that we only changed internal and the API stays the same? I am trying to understand if this is an internal bugfix or a breaking change. |
|
@buchi would you mind having a quick look here? I would like to hear your opinion as well since you also worked on this... |
|
@thet ping :) |
|
@tisto IMO this is a breaking change, because as the following test shows, all date-times are converted to UTC before serializing: Is this change important? Yes, if date arithmetics should be done. E.g. for recurring events or calculating the duration of two date/times and ONLY if a DST change is in between (which we have to assume that there is). Will a conversion to UTC fix DST change issues? Actually no. It just delegates the issue to the consumer of the serialized value. In case of Plone there are some hints how to handle it, because we (partly) support timezones since 5.0. I'm not 100% confident about the relevance of this PR again. Some more resources about this topic: https://codeofmatt.com/content/images/best-dates-ever.pdf |
|
@thet @sneridagh @buchi I would like to release plone.restapi 2.0.0 soon. Any chance we can sort this out? As said, this goes over my head since I haven't worked with date/times lately. The pull request looks ok to me. I'd like to have a second or third opinion and approval on this before merging and releasing. |
|
This is definitely a breaking change! First, let's recapitulate what the ideal solution for handling of dates and times would be:
Unfortunately Plone does not store all dates and times in UTC. plone.app.event and Archetypes store them timezone aware. This is something that can't be fixed in plone.restapi. So returning dates and times in UTC seems to be the way to go. But there are a few caveats because of not storing them in UTC:
Btw, whats the reason for switching to python-dateutil for parsing of ISO8601 datetime strings? Anything that Zope's DateTime doesn't right? |
|
@buchi I don't agree with the ideal solution:
When de/serializing from/to JSON it would probably be good to store the IANA/Olson timezone identifier for each UTC-converted date. The switch to python-dateuti (which is a dependency of plone.app.event) l for parsing date strings is mostly to get rid of DateTime in the long run. It's not needed anymore in Python |
|
@thet Storing dates/times in UTC has the following benefits:
IMHO timezones or local time is something that's needed only for input and output (display). Also have a look at the documentation of pytz regarding local time: But let's get back to your PR. I'm fine if we merge it. But there are still the two issues I mentioned earlier:
Regarding Zope's DateTime I don't believe that we will get rid of it that soon. But if python-dateutil is a dependency of Plone anyway, I don't mind. |
|
"If you do a GET and then a PATCH with the data from the GET, you potentially modify the dates, which is not what one would expect." That is indeed a problem because it breaks the REST assumptions, caching, etc. Do we really modify content, when reading a content object in Plone? That sounds really wrong... |
|
@buchi ok, that's a relief. :) |
|
Please, also allow me to weigh in, IMO, I'm also +1 with @buchi ideal solution... Handling UTC is always desirable. Libraries also expect somehow that, then convert it to local TZ (e.g momentjs). What about the differences of implementation on p.a.e. for Plone 4/5? When I tried to fix this, I always got hit by this wall... @buchi which detail of your solution is then out with the current PR? |
|
@sneridagh @buchi @thet I would like to make a 2.0.0 release in the next days. This is the only issue that holds me back from merging all the open PRs with breaking changes and do a release. It would be really cool if we could figure that out in the next days. |
Convert all datetime, DateTime and time instances to UTC before serializing. Use python-dateutil instead of DateTime to parse date strings when de-serializing.
| except (SyntaxError, DateTimeError): | ||
| # TODO: should really a timezone naive time be returned? | ||
| # using ``timetz()`` would be timezone aware. | ||
| value = dateutil.parser.parse(value).time() |
There was a problem hiding this comment.
@buchi @sneridagh @tisto I'm not sure, if time should always converted to timezone-naive. Pythons datetime.time and thus zope.schema Time field supports the tzinfo objects.
| value = value.replace(microsecond=0) | ||
| iso = value.isoformat() | ||
| # if value.tzinfo: | ||
| # # Use "Z" instead of a timezone offset of "+00:00" to indicate UTC. |
There was a problem hiding this comment.
@buchi @sneridagh @tisto I'm favoring the "Z" notation of indicating the UTC, but that's maybe a personal flavor and I don't have a strong preference here. I'll remove this code block, agreed?
|
I might begin to agree that dates stored in UTC are more practical. While discussing the issue with @Rotonen he brought up an argument that date calculations with two different versions of pytz can produce different results. Which is bad. Although the timezone rules are part of the tzinfo objects and thus not necessarily prone to errors with different versions of pytz ... UTC dates have advantages. However the UTC vs timezoned isn't actually an issue as @buchi mentioned above: Do we need to hande Archetypes content also? |
|
@thet would you mind adding a few lines to the upgrade guide regarding this? http://plonerestapi.readthedocs.io/en/latest/upgrade-guide.html# |
in docs and tests. With the latest plone.restapi version all datetimes are now converted to UTC datetimes (see plone/plone.restapi#493 for more information)
in docs and tests. With the latest plone.restapi version all datetimes are now converted to UTC datetimes (see plone/plone.restapi#493 for more information)
in docs and tests. With the latest plone.restapi version all datetimes are now converted to UTC datetimes (see plone/plone.restapi#493 for more information)
in docs and tests. With the latest plone.restapi version all datetimes are now converted to UTC datetimes (see plone/plone.restapi#493 for more information)
Most of the test failures for Plone 5.2 were related to missing initialization of ZServer, which is fixed but not yet merged here: plone/plone.testing#45
Other Plone 5.2 failures were related to date/timezone related offsets which did not match (I do not yet understand why this failures only were apparent for Plone 5.2).
Investigating this, I found out that the date/times were not consistently converted to UTC when exporting. I recall some discussions about this including some contradictory statements including from myself in #253
However, I think since ISO8601 lacks of a concept of timezone names which are necessary to do date math when daylight saving time boundaries are included (timezone offsets do not allow to conclude to DST dates), the most practical thing would be to convert all date/times to UTC when exporting.
This PR addresses this:
please comment, if this approach is going in the right direction!