-
-
Notifications
You must be signed in to change notification settings - Fork 76
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
Change the serialization of the datetime objects, and store them as o… #394
Changes from 6 commits
d6f3edd
f7c4dec
776ff65
8ee4b32
d578263
4dd3ff0
9112b29
7f5dd84
ba84c0f
7169bee
66d3e22
d7290c0
00e5871
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 |
---|---|---|
@@ -0,0 +1,87 @@ | ||
Deserialization | ||
=============== | ||
|
||
It's worth to note an special case when deserializing Datetimes objects, and how plone.restapi will handle them. | ||
|
||
Although not supported by Plone itself yet, plone.restapi will store all the Datetimes that will be handling along with its timezone converted to UTC. | ||
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. "all the Datetimes" --> "datetime objects" |
||
This will provide a common ground for all the datetimes operations. | ||
|
||
There is an special case when using datetimes objects in p.a.event, and its behavior is different due to implementation differences for versions 1.x (Plone 4) and 2.x and above (Plone 5). | ||
|
||
.. warning:: | ||
In case of using zope.schema date validators you should also use a datetime object that also contains offset-aware object as the validator value. | ||
|
||
.. note:: | ||
This does not applies in case that you are using Plone 4 with no Dexterity support at all or not p.a.event installed. | ||
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. @sneridagh type "applies" -> "apply" / better -> "or if you don't have p.a.event installed" |
||
|
||
p.a.event 1.x in Plone 4 | ||
------------------------ | ||
|
||
The implementation of p.a.event in 1.x requires to provide a `timezone` schema property along with the Event type information, otherwise the creation fails, because it's a required field, like this:: | ||
|
||
.. code-block:: json | ||
|
||
{ | ||
"@type": "Event", | ||
"id": "myevent2", | ||
"title": "My Event", | ||
"start": "2018-01-01T10:00:00", | ||
"end": "2018-01-01T12:00:00", | ||
"timezone": "Asia/Saigon" | ||
} | ||
|
||
The final stored datetime takes this field into account and adds the correct offset to the content type (abbreviated JSON response):: | ||
|
||
.. code-block:: json | ||
|
||
{ | ||
"@id": "http://localhost:55001/plone/folder1/myevent2", | ||
"@type": "Event", | ||
"UID": "bcfc3914ea174cc1aa8042147edfa33a", | ||
"creators": ["admin"], | ||
"description": "", | ||
"end": "2018-01-01T12:00:00+07:00", | ||
"id": "myevent2", | ||
"start": "2018-01-01T10:00:00+07:00", | ||
"timezone": "Asia/Saigon", | ||
"title": "My Event", | ||
} | ||
|
||
and builds the `start` and `end` fields with the proper timezone, depending on the `timezone` field. It also returns the datetime object with the proper timezone offset. | ||
|
||
If using Plone 4 and p.a.event 1.x you should construct the Event type using this approach, otherwise the Event object will be created with a wrong timezones. | ||
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. typo: -> "with a wrong timezone." 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. Fixed. |
||
|
||
This approach was counterintuitive, and thus, it was changed it Plone 5 version of p.a.event. | ||
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. maybe better: it was changed for Plone 5 by p.a.event version 2 |
||
|
||
p.a.event 2.x in Plone 5 | ||
------------------------ | ||
|
||
The implementation of p.a.event in 2.x no longer requires to provide a `timezone` schema property, because the timezone is computed taking the timezone already existent in dates supplied:: | ||
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. "p.a.event in 2.x no longer" --> p.a.event 2.x no longer" |
||
|
||
.. code-block:: json | ||
|
||
{ | ||
"@type": "Event", | ||
"id": "myevent2", | ||
"title": "My Event", | ||
"start": "2018-01-01T10:00:00+07:00", | ||
"end": "2018-01-01T12:00:00+07:00", | ||
} | ||
|
||
You should pass the timezone information in the ISO8601 format, otherwise the system will fallback to UTC. The response given is also computed given the timezone information supplied, but this time it's UTC based: | ||
|
||
.. code-block:: json | ||
|
||
{ | ||
"@id": "http://localhost:55001/plone/folder1/myevent2", | ||
"@type": "Event", | ||
"UID": "4c031960718246db86c97685f83047ee", | ||
"creators": ["admin"], | ||
"description": "", | ||
"end": "2018-01-01T05:00:00+00:00", | ||
"id": "myevent2", | ||
"start": "2018-01-01T03:00:00+00:00", | ||
"title": "My Event", | ||
} | ||
|
||
This approach is better because all Javascript serializers/deserializers works with UTC based dates (e.g. .toJSON Javascript API). |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,14 @@ | |
try: | ||
pkg_resources.get_distribution('plone.app.contenttypes') | ||
HAS_PLONE_APP_CONTENTTYPES = True | ||
|
||
event_version = pkg_resources.get_distribution('plone.app.event').version | ||
if pkg_resources.parse_version(event_version) > \ | ||
pkg_resources.parse_version('1.99'): | ||
HAS_PLONE_APP_EVENT_20 = True | ||
else: | ||
HAS_PLONE_APP_EVENT_20 = False | ||
|
||
except pkg_resources.DistributionNotFound: # pragma: no cover | ||
HAS_PLONE_APP_CONTENTTYPES = False | ||
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. shouldn't in the except case also |
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -78,7 +78,14 @@ def __call__(self, validate_all=False, data=None): # noqa: ignore=C901 | |
'message': e.doc(), 'field': name, 'error': e}) | ||
else: | ||
field_data[name] = value | ||
if value != dm.get(): | ||
try: | ||
if value != dm.get(): | ||
dm.set(value) | ||
modified = True | ||
except TypeError: | ||
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 am sorry, but I did not like the usage of an Error Handler to apply a value I would suggest something like:
instead of the try except clause. 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, but you have to detect first if dm.get() returns a datetime right? if not, value.tzinfo will fail. I wanted to prevent to infer more conditional clauses here, which in turn, increases the complexity ratio of the method (and then plone.recipe.codeanalysis complains about it). 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. yes you are right, there needs to be an 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. Well the complexity of an aditional if level is better than an try except. It could also be an idea to split this test aus into a separate function. 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. @sneridagh @loechel For me the main objective of this code snippet is readability. I don't mind a try/except if it makes the code more readable. Though, without a few more comments I don't really understand what goes on here anyways. :) |
||
# Most probably due to offset-naive and offset | ||
# aware objects, set the value as they most likely | ||
# are not the same | ||
dm.set(value) | ||
modified = True | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,8 +46,7 @@ def __call__(self, value): | |
try: | ||
# Parse ISO 8601 string with Zope's DateTime module | ||
# and convert to a timezone naive datetime in local time | ||
value = DateTime(value).toZone(DateTime().localZone()).asdatetime( | ||
).replace(tzinfo=None) | ||
value = DateTime(value).toZone('UTC').asdatetime() | ||
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 just tried a new approach and change the code to store the datetime objects as datetime offset-aware and referenced always to UTC. It felt bad to convert it to the local time of the server each time, and it caused a bad offset when the API waas used by people in other timezones than the server's one. 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. That is perfect, It should always be in an isoformat with UTC timezone. 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. Again like above, I wouldn't use 'UTC' but the portal's default timezone. 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 think that the concept of having a "portal" timezone is wrong. In which case is p.a.event using it? It's ok if you always have users in the same timezone, but this is hardly the case in most user cases. All datetimes should be referenced using a common baseline. Then the UI convert it to the user's timezone. If not, a user in India will see always the dates offsetted with the portal timezone, which is pretty useless for him. This is what happened before and that's the reason of this change. 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. @thet I thought you agree on that too when you said: #253 (comment) 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. Yes, true. I was suggesting that because I thought you do not have a timezone information in the JSON object available - besides the offset. Fact is, plone.app.event 2.x does not convert it's datetimes to UTC but stores them in the default timezone - which is:
I agree that users from anywhere want to have the option to show the datetimes in their local zone. But that's a UI thing and should be done via JS with the option to show the datetimes in the original zone. So: converting to UTC and storing in UTC is OK, if you have no timezone identifier (offsets are a bit useless) and exchange in UTC anyways. 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. @sneridagh If you want, we can have a short hangout if you think I can help here... 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. @thet sure, whenever you are available, we can have a talk about it. We should find a common agreement on this. Agreed that the implementation makes an assumption that p.a.event is not (yet) doing but IMHO should do and we should fix it upstream in Plone. That is, save the internal dates with timezone information under a common frame of reference (say UTC). Clients (and existing UI) should provide always datetimes depending on the user's timezone (we are already doing that in a client's project all Angular UI. Current libraries always transform to UTC on serializing and it's easy to be user's timezone aware. Setting a portal default timezone (or even a user timezone) it's useless and error prone. The current implementation now it's broken, as you can see, the provided tests are not passing using the current version of plone.restapi. The more sensible solution (as agreed in #253 seems to store always the timezone information) but then, use a common reference for that (using server's timezone will also be error prone and dates could not be the same if you migrate one server from timezone). @tisto @lukasgraf @buchi any insights on this? 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 would say, if the convention in plone.restapi is to always export dates as UTC that is an convention. If other modules for other export formats uses another convetion that might not be consistent, but possible. I would prefer to always export datetime objects as timezone aware objects, and that transformed to UTC, but also to always import all native datetime input as new timezone aware datetime objects. Presentation in the UI should be bound to user settings. |
||
except (SyntaxError, DateTimeError) as e: | ||
raise ValueError(e.message) | ||
|
||
|
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.
@sneridagh type "an" -> "a"
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.
Still a typo: "to note an special" --> to note a special"