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 DateTimeField TZ handling #5435

Merged
merged 4 commits into from
Sep 20, 2017
Merged

Fix DateTimeField TZ handling #5435

merged 4 commits into from
Sep 20, 2017

Conversation

carltongibson
Copy link
Collaborator

Adds Release note, rebases and closes #5408. Closes #3732

@carltongibson carltongibson added this to the 3.6.5 Release milestone Sep 20, 2017
@carltongibson carltongibson merged commit 7d6d043 into master Sep 20, 2017
@carltongibson carltongibson deleted the pr/5408 branch September 20, 2017 10:15
@@ -44,7 +44,11 @@ You can determine your currently installed version using `pip freeze`:

* Fix `DjangoModelPermissions` to ensure user authentication before calling the view's `get_queryset()` method. As a side effect, this changes the order of the HTTP method permissions and authentication checks, and 405 responses will only be returned when authenticated. If you want to replicate the old behavior, see the PR for details. [#5376][gh5376]
* Deprecated `exclude_from_schema` on `APIView` and `api_view` decorator. Set `schema = None` or `@schema(None)` as appropriate. [#5422][gh5422]
* Timezone-aware `DateTimeField`s now respect active or default) `timezone` during serialization, instead of always using UTC.
Copy link
Member

Choose a reason for hiding this comment

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

Looks like an erroneous ")" here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

suutari-ai added a commit to suutari-ai/parkkihubi that referenced this pull request Dec 11, 2017
The last DRF update changed how timestamps are returned from the
endpoints.  Previously they were formatted as

    2017-12-11T13:01:23Z

and now they are formatted as

    2017-12-11T15:01:23+02:00

I.e. they use the timezone configured in the settings rather than UTC.

See encode/django-rest-framework#5435 for more
info.
@cristianocca
Copy link

This added new "inconsistencies" as well and I don't think there's really a better way to handle any of this and the client should always be able to handle timezones.

But the change that converts parsed datetime to the default timezone adds the following inconsistency:

  • Client sends an UTC datetime, for example, when using moment, iso format is always UTC
  • DRF serializer parses it, and converts it to local timezone (ie, from UTC to UTC-3)
  • Datetime is used in multiple places just fine, and at some point is returned back to the client, and client now receives a non UTC datetime.

At least previously, sending the server a date would always return the same date in the the timezone it was sent, now the interaction between a client side library like momentjs and DRF gets really ugly since datetimes are always sent in one timezone and received in another, when previously at least that would be kept consistent.

This is not a complaint or anything since client side should be able to handle this just fine, but just a warning about inconsistencies are still here and the previous behavior might have been the right one.

Another inconsistency would be mixing the renderer class and serializer, renderer will always use w/e timezone the date is (most of the time UTC if it was loaded by django) while the serializer will now always use a different timezone (unless UTC is the default one) so you might end up with two API endpoints that returns dates in different formats.

I think the serializer shouldn't mess around with the timezone at all, unless the given date had no timezone and timezone is enabled in which case it needs to be set, and any display logic should be purely client side, since this is a REST API after all.

@carltongibson
Copy link
Collaborator Author

Hey @cristianocca. I think you're right that there's no perfect way to handle this. IMO Setting the default timezone to UTC, and using UTC everywhere, is normally going to be your best option (if what you want is least surprises).

@alexcreid
Copy link

This has caused us issues after upgrading, especially since I think the release notes don't actually cover what the real behaviour was prior to the change. Previously, we were able to provide a datetime like datetime(2018, 1, 15, 12, tzinfo=pytz.timezone('Asia/Tokyo') to the serialiser, and it would serialise as "2018-01-15T12:00:00+09:00". Since the change, it instead outputs the same time localised to UTC ("2018-01-15T03:00:00Z"). While it's true that these both represent the same moment in time, the conversion is not lossless, as the timezone information was meaningful to our clients.

I plan to subclass DateTimeField to make the enforce_timezone method a no-op, but it would be nice to be able to return to the behaviour prior to this change either in the DRF settings, or as an option on the field.

@rpkilby
Copy link
Member

rpkilby commented Feb 7, 2018

Hi @alexcreid - this sounds like it might be a bug, given that non-UTC timezones are supported.

Would you mind opening a report so we don't lose track of this?

@carltongibson
Copy link
Collaborator Author

Hi @alexcreid.

Before this fix on create/update you would get back the same timezone you submitted with but on a later retrieve it would come back as UTC. This inconsistency was the body of #3732. Now the serialisation respects current or active timezone.

So, question, how's your DB storing the datetimes? Are the entries not normalised to a single timezone? If they are then, even with the old behaviour, there's no way of maintaining the extra timezone info.

Either way, as @rpkilby says, can you open a new ticket explaining your issue in full and we'll have a look. Ta.

@hakib
Copy link

hakib commented Feb 8, 2018

Hey, I just encountered this issue my self.

I had a serializer field defined like this:

reloaded_at = serializers.DateTimeField(
    source='reloading.created',
    default_timezone=timezone.get_current_timezone(),
    format=DISPLAY_DATETIME_FORMAT,
)

I expected the field to be rendered in local time (especially since I explicitly stated the desired timezone). To my surprise it was rendered in UTC (the database timezone).

Is there a reason the date is only made aware in to_representation if the format is ISO_8601?

@carltongibson
Copy link
Collaborator Author

@hakib Can you try adding a test to tests/test_fields.py that recreates your issue? That's the quickest way. Thanks

hakib added a commit to hakib/django-rest-framework that referenced this pull request Feb 8, 2018
@cristianocca
Copy link

cristianocca commented Feb 8, 2018

I think that a better fix (rather than changing the previous behavior) would have been fixing the browsable API to correctly parse and render datetimes on the client-side or server side configured timezone.

Right now you are even losing information when doing all those timezone changes to the datetime object, not like it really matters but if for some reason you want to know the original timezone a parsed date came in, you will need to add a new field or overwrite the serializer.

@carltongibson
Copy link
Collaborator Author

@cristianocca The issue here wasn't with the Browsable API, in except that it uses serializers, but rather the inconsistency of representation between different HTTP methods (create/update vs retrieve). We've gone with what we hope is a sensible solution for most users.

(#5821 looks like a bug not respecting the active timezone, so we'll look at that.)

If you do want to maintain the submitted timezone info, subclassing DateTimeField and implementing enforce_timezone would be the right way to go currently. (If you're confident in your settings and submitted data then a no-op may be viable there...)

Potentially we could look at a PR making this user-configurable. My worry would be introducing a complex change where the simple subclass is readily available. (We'd have to see the actual PR to say.)

@hakib
Copy link

hakib commented Feb 8, 2018

subclassing DateTimeField and implementing enforce_timezone would be the right way to go currently

Actually that wont work. The subclass would have to override to_representation and make the value timezone aware regardless of the format (currently make_aware is used only for ISO-8601 format).

@alexcreid
Copy link

@carltongibson wrote:

Before this fix on create/update you would get back the same timezone you submitted with but on a later retrieve it would come back as UTC. This inconsistency was the body of #3732. Now the serialisation respects current or active timezone.

So, question, how's your DB storing the datetimes? Are the entries not normalised to a single timezone? If they are then, even with the old behaviour, there's no way of maintaining the extra timezone info.

When we store datetimes, they're invariably UTC. Our application needs to deal with datetimes that are in multiple timezones which are in neither the client's nor the server's timezone, so this doesn't map neatly to the Django way of having a single 'active' timezone. Since we had logic to ensure that these datetimes always had the correct tzinfo attached, these would be serialised by the DRF as ISO 8601 strings with the correct offset from UTC at the end before this change. I'll open an issue with some more details when I get a chance. It might be that this is too niche to merit any code changes, but if so, it'd be nice to have the fact that datetimes will be output in the active Django timezone documented, since I think that counts as surprising behaviour.

carltongibson pushed a commit to carltongibson/django-rest-framework that referenced this pull request Feb 16, 2018
carltongibson added a commit that referenced this pull request Feb 16, 2018
* Add failing test for to_representation with explicit default timezone

See discussion here:
    #5435 (comment)

* Always run enforce_timezone
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
* Add failing test for to_representation with explicit default timezone

See discussion here:
    encode#5435 (comment)

* Always run enforce_timezone
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.

Serializer DateTimeField has unexpected timezone information
6 participants