-
-
Notifications
You must be signed in to change notification settings - Fork 104
Handle start_timezone and end_timezone during (de)serialization #2018
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
base: main
Are you sure you want to change the base?
Changes from all commits
338ab4e
deecfd0
198e31b
a5494c9
563bb02
2811085
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -22,14 +22,25 @@ Basic Python data types that have a corresponding type in JSON, such as integers | |||||||||||||||||
| ## Dates and Times | ||||||||||||||||||
|
|
||||||||||||||||||
| Since JSON does not have native support for dates and times, the Python and Zope `datetime` types will be serialized to an ISO 8601 date string. | ||||||||||||||||||
|
|
||||||||||||||||||
| | Python | JSON | | ||||||||||||||||||
| | ------------------------------------ | ----------------------- | | ||||||||||||||||||
| | `time(19, 45, 55)` | `"19:45:55"` | | ||||||||||||||||||
| | `date(2015, 11, 23)` | `"2015-11-23"` | | ||||||||||||||||||
| | `datetime(2015, 11, 23, 19, 45, 55)` | `"2015-11-23T19:45:55"` | | ||||||||||||||||||
| | `DateTime("2015/11/23 19:45:55")` | `"2015-11-23T19:45:55"` | | ||||||||||||||||||
|
|
||||||||||||||||||
| Timezone-aware values are converted to UTC. | ||||||||||||||||||
|
|
||||||||||||||||||
| | Python | JSON | | ||||||||||||||||||
| | ------------------------------------------------------------------ | ----------------------------- | | ||||||||||||||||||
| | `time(19, 45, 55)` | `"19:45:55"` | | ||||||||||||||||||
| | `date(2015, 11, 23)` | `"2015-11-23"` | | ||||||||||||||||||
| | `datetime(2015, 11, 23, 19, 45, 55)` | `"2015-11-23T19:45:55"` | | ||||||||||||||||||
| | `datetime(2015, 11, 23, 19, 45, tzinfo=ZoneInfo("Europe/Vienna"))` | `"2015-11-23T17:45:00+00:00"` | | ||||||||||||||||||
| | `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. | ||||||||||||||||||
|
|
||||||||||||||||||
| | field name | Python | JSON | | ||||||||||||||||||
| | ---------------- | ---------------------------------------------------------------- | ----------------------------- | | ||||||||||||||||||
| | `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"` | | ||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To complete a round-trip, how will a POST, PUT, or PATCH request handle a JSON object with or without a
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See above
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
How about this?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK on the first point. I'll tweak that tonight. On the second point, you originally wrote this:
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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. |
||||||||||||||||||
|
|
||||||||||||||||||
| ## Decimal Type | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| Add `start_timezone` and `end_timezone` to serialized Events. | ||
| During deserialization, store values in these timezones if specified. | ||
| @davisagli |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -129,6 +129,11 @@ def get_schema_data(self, data, validate_all, create=False): | |
| if deserializer is None: | ||
| continue | ||
|
|
||
| if name == "start" and "start_timezone" in data: | ||
| deserializer.requested_timezone = data["start_timezone"] | ||
| elif name == "end" and "end_timezone" in data: | ||
| deserializer.requested_timezone = data["end_timezone"] | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know that the original issue requests adding timezone serialization for events, but shouldn't we do this for all datetime fields?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wouldn't assume that. We've only been thinking about events as we design the solution.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (I'll keep this in mind, but first I want to make sure everything works end to end with the frontend widgets.)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, we should do it for all at one point. But for now out of scope I think. |
||
| try: | ||
| value = deserializer(data[name]) | ||
| except ValueError as e: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
| from plone.restapi.interfaces import IFieldDeserializer | ||
| from plone.restapi.services.content.tus import TUSUpload | ||
| from pytz import timezone | ||
| from pytz import UnknownTimeZoneError | ||
| from pytz import utc | ||
| from z3c.form.interfaces import IDataManager | ||
| from zope.component import adapter | ||
|
|
@@ -89,6 +90,9 @@ def __call__(self, value): | |
| @implementer(IFieldDeserializer) | ||
| @adapter(IDatetime, IDexterityContent, IBrowserRequest) | ||
| class DatetimeFieldDeserializer(DefaultFieldDeserializer): | ||
|
|
||
| requested_timezone = None | ||
|
|
||
| def __call__(self, value): | ||
| # This happens when a 'null' is posted for a non-required field. | ||
| if value is None: | ||
|
|
@@ -119,6 +123,14 @@ def __call__(self, value): | |
| # The IPublication adapter is a special case that expects | ||
| # a timezone-naive local datetime | ||
| value = dt.astimezone().replace(tzinfo=None) | ||
| elif self.requested_timezone is not None: | ||
| # Use the requested timezone if set | ||
| try: | ||
| tz = timezone(self.requested_timezone) | ||
| except UnknownTimeZoneError: | ||
| raise ValueError(f"Unknown timezone: {self.requested_timezone}") | ||
| else: | ||
| value = tz.normalize(dt.astimezone(tz)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. |
||
| else: | ||
| # Otherwise let's check what is currently stored. | ||
| dm = queryMultiAdapter((self.context, self.field), IDataManager) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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.