Handle start_timezone and end_timezone during (de)serialization#2018
Handle start_timezone and end_timezone during (de)serialization#2018davisagli wants to merge 6 commits into
Conversation
|
@davisagli thanks for creating this Pull Request and helping to improve Plone! TL;DR: Finish pushing changes, pass all other checks, then paste a comment: To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically. Happy hacking! |
Documentation build overview
|
|
@robgietema FYI, we are adding support for serializing and deserializing |
|
@jenkins-plone-org please run jobs |
| deserializer.requested_timezone = data["start_timezone"] | ||
| elif name == "end" and "end_timezone" in data: | ||
| deserializer.requested_timezone = data["end_timezone"] | ||
|
|
There was a problem hiding this comment.
I know that the original issue requests adding timezone serialization for events, but shouldn't we do this for all datetime fields?
There was a problem hiding this comment.
I wouldn't assume that. We've only been thinking about events as we design the solution.
There was a problem hiding this comment.
(I'll keep this in mind, but first I want to make sure everything works end to end with the frontend widgets.)
There was a problem hiding this comment.
Good point, we should do it for all at one point. But for now out of scope I think.
thet
left a comment
There was a problem hiding this comment.
LGTM! This unlocks the feature of separate start/end timezones for restapi clients, great 🎉
I have added a commit with more documentation on datetime serialization and specifically the handling of start/end fields with a start_timezone/end_timezone field, as it is a bit confusing that the start/end value is converted to UTC but the start_timezone/end_timezone is normally not UTC. The docs should make that clear.
Please take a look.
IMO good to merge!
| except UnknownTimeZoneError: | ||
| raise ValueError(f"Unknown timezone: {self.requested_timezone}") | ||
| else: | ||
| value = tz.normalize(dt.astimezone(tz)) |
There was a problem hiding this comment.
@davisagli I was chewing on this, as I told you at the Buschenschanksprint - but checking the rest api and your PR there, I recognize that all datetimes are always converted to UTC for serialization/deserialization.
So, calculating the correct start/end time for the requested timezone here is absolutely right.
👍
| | `DateTime("2015/11/23 19:45:55 UTC")` | `"2015-11-23T19:45:55+00:00"` | | ||
|
|
||
| Event types additionally expose `start_timezone` and `end_timezone` fields. | ||
| The `start` and `end` datetime values are still serialized and deserialized as UTC values with an offset of `+00:00`, but `start_timezone` and `end_timezone` allow the client (or plone.restapi) to convert the datetime values to the requested timezone. |
There was a problem hiding this comment.
The ambiguity of whether the client or plone.restapi converts values to a requested timezone leaves it open for double conversion. This should be disambiguated, making it clear where the responsibility of conversion occurs. If both may convert values, then we should include the conditions for conversion.
There was a problem hiding this comment.
The REST API accepts datetime values as an ISO 8601 datetime with any offset. (If no offset was specified, we assume it was intended to be UTC.) It then converts these for storage as Python datetimes as follows:
- If it's a start value with start_timezone specified, localize it to that timezone. If it's an end value with end_timezone specified, localize it to that timezone.
- Otherwise, if there's already a timezone-aware value stored in the field, localize to its timezone.
- Otherwise (no value stored yet, and no specific timezone specified in the request), store as a timezone-aware datetime in UTC.
So: a client may send values with whatever offset is convenient, but it should specify the offset and it should also specify start_timezone or end_timezone to make sure that the backend can store the intended timezone.
| | `start` | `datetime(2026, 5, 26, 10, 0, tzinfo=ZoneInfo("Europe/Vienna"))` | `"2026-05-26T08:00:00+00:00"` | | ||
| | `start_timezone` | `"Europe/Vienna"` | `"Europe/Vienna"` | | ||
| | `end` | `datetime(2026, 5, 31, 10, 0, tzinfo=ZoneInfo("Europe/Vienna"))` | `"2026-05-31T08:00:00+00:00"` | | ||
| | `end_timezone` | `"Europe/Vienna"` | `"Europe/Vienna"` | |
There was a problem hiding this comment.
To complete a round-trip, how will a POST, PUT, or PATCH request handle a JSON object with or without a start_timezone or end_timezone?
There was a problem hiding this comment.
| | `end_timezone` | `"Europe/Vienna"` | `"Europe/Vienna"` | | |
| | `end_timezone` | `"Europe/Vienna"` | `"Europe/Vienna"` | | |
| During deserialization, plone.restapi processes both the ISO 8601 string, and `start_timezone` and `end_timezone` if present, from the JSON object, storing the field's value as described below. | |
| - If the field is either a start value with `start_timezone` specified, or an end value with `end_timezone` specified, localize it to the timezone from the request. | |
| - If the request lacks a specified `start_timezone` or `end_timezone` for the respective start or end value of the field, and if the field already has a timezone-aware value, localize the string to the field's timezone. | |
| - Finally, if the field both has no value and there's no timezone in the request, store the value as a timezone-aware datetime in UTC. |
How about this?
There was a problem hiding this comment.
When the client sends a POST, PUT, or PATCH request
Do we have to be quite so specific here? This information is about deserialization, which happens for data in those kinds of requests, but also when the deserializer is called in other contexts that aren't tied to a request method (such as from import tools). I would just say "During deserialization" here and make sure the beginning of the chapter explains what that means.
to the field's timezone
That sounds like the field is configured with a timezone which isn't the case. I would say "to the timezone of the existing value"
Otherwise this looks good
There was a problem hiding this comment.
OK on the first point. I'll tweak that tonight.
On the second point, you originally wrote this:
- Otherwise, if there's already a timezone-aware value stored in the field, localize to its timezone.
I want to make sure that "its timezone" doesn't refer to "the field" as the sentence's subject (that's how I interpreted it) but something external to that sentence, specifically the ISO 8601 string value. Can you confirm?
Words are hard.
There was a problem hiding this comment.
@stevepiercy If there is already a timezone-aware datetime value stored and there was no timezone specified in the serialized data, then it localizes the new value to the timezone of the existing stored value. https://github.com/plone/plone.restapi/pull/2018/files#diff-739c4c63bfbbf88cb8766b29dba0b84591a8709e67a5ed5bb55954539e4e7e27L126-L131
There was a problem hiding this comment.
@davisagli I edited the first bullet point, and added a new suggestion higher up the page.
Can you please double-check for the second and third bullet points? The first bullet point describes the code in your link. The second and third bullet points address the code at https://github.com/plone/plone.restapi/pull/2018/changes#diff-739c4c63bfbbf88cb8766b29dba0b84591a8709e67a5ed5bb55954539e4e7e27R134-R149.
| | `start` | `datetime(2026, 5, 26, 10, 0, tzinfo=ZoneInfo("Europe/Vienna"))` | `"2026-05-26T08:00:00+00:00"` | | ||
| | `start_timezone` | `"Europe/Vienna"` | `"Europe/Vienna"` | | ||
| | `end` | `datetime(2026, 5, 31, 10, 0, tzinfo=ZoneInfo("Europe/Vienna"))` | `"2026-05-31T08:00:00+00:00"` | | ||
| | `end_timezone` | `"Europe/Vienna"` | `"Europe/Vienna"` | |
There was a problem hiding this comment.
| | `end_timezone` | `"Europe/Vienna"` | `"Europe/Vienna"` | | |
| | `end_timezone` | `"Europe/Vienna"` | `"Europe/Vienna"` | | |
| During deserialization, plone.restapi processes both the ISO 8601 string, and `start_timezone` and `end_timezone` if present, from the JSON object, storing the field's value as described below. | |
| - If the field is either a start value with `start_timezone` specified, or an end value with `end_timezone` specified, localize it to the timezone from the request. | |
| - If the request lacks a specified `start_timezone` or `end_timezone` for the respective start or end value of the field, and if the field already has a timezone-aware value, localize the string to the field's timezone. | |
| - Finally, if the field both has no value and there's no timezone in the request, store the value as a timezone-aware datetime in UTC. |
How about this?
| Throughout the REST API, content needs to be serialized and deserialized to and from JSON representations. | ||
|
|
||
| In general, the format used for serializing content when reading from the API is the same as is used to submit content to the API for writing. | ||
|
|
There was a problem hiding this comment.
Per #2018 (comment)
| Deserialization takes place whenever the deserializer is called. | |
| This is usually when the client sends a request, but may occur in other contexts that aren't tied to a request method, such as from import tools. |
Fixes #1878
📚 Documentation preview 📚: https://plonerestapi--2018.org.readthedocs.build/