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

Serializer DateTimeField has unexpected timezone information #3732

Closed
jonathan-golorry opened this issue Dec 13, 2015 · 34 comments · Fixed by #5435
Closed

Serializer DateTimeField has unexpected timezone information #3732

jonathan-golorry opened this issue Dec 13, 2015 · 34 comments · Fixed by #5435

Comments

@jonathan-golorry
Copy link

I have TIME_ZONE = 'Asia/Kolkata' and USE_TZ = True in my settings.

When I create a new object with the browsable api, the serializer displays the newly created object with datetimes that have a trailing +5:30, indicating the timezone. The database is storing the times in UTC.

The unexpected behavior is that when the object is serialized again later, the datetimes are all in UTC format with a trailing Z. According to the Django docs on current and default time zone, I would expect the serializer to convert the datetimes to the current time zone, which defaults to the default time zone, which is set by TIME_ZONE = 'Asia/Kolkata'.

Am I missing something?

@jonathan-golorry
Copy link
Author

I asked on stack overflow and the conclusion is that the current timezone is only used for user-provided datetimes, as the datetime is serialized as-is after validation (ie after create or update). From the Django documentation, I find it hard to believe that this is intended behavior, but this is an issue with Django itself rather than the Rest Framework, so I will close this issue.

@vpistis
Copy link

vpistis commented Dec 16, 2016

Hi @tomchristie ,

I think this is a DRF bug.
IMHO, the natural use of DRF is to use it as an output like a django templating.
In a django template the timezone is correctly showed, why the drf serializer do not respects the django timezone setting?

@vpistis
Copy link

vpistis commented Dec 17, 2016

Hi,

looking into the code in fields.py at DatetimeField class I discover that Django timezone setting is well considered only for the internal value (to_internal_value()).

I have seen that using model serializer like this:

class MyModelSerializer(ModelSerializer):

    class Meta:
        model = MyModel
        depth = 1
        fields = ('some_field', 'my_date_time_field')

for the field my_date_time_field (that i think it's a DateTimeField :) ) the timezone for the representation is None for default (to_representation()).

In otherwords, if I'm not wrong, the Django timezone is considered only when the value is wrote into the storage backend.

IMHO I think the return value of to_representation() should be like this:
return self.enforce_timezone(value).strftime(output_format)
according to the Django USE_TZ setting.

I write a pull request for this.

Ciao
Valentino

@tomchristie
Copy link
Member

@vpistis It'd be easier to review this if you could give an example of the API behavior you currently see, and what you'd expect to see (rather than discussing the implementation aspects first)

@jonathan-golorry
Copy link
Author

jonathan-golorry commented Dec 19, 2016

Set your TIME_ZONE = 'Asia/Kolkata' and create a model serializer with a single datetime field, appointment.

During create/update, send:

{
    "appointment": "2016-12-19T10:00:00"
}

and get back:

{
    "appointment": "2016-12-19T10:00:00+5:30"
}

But if you retrieve or list that object again, you get:

{
    "appointment": "2016-12-19T04:30:00Z"
}

During creation, if the Z isn't specified, DRF assumes the client is using the timezone specified by TIME_ZONE and returns a time formatted to that timezone (by adding the +5:30 at the end, which would actually be invalid if it were a client-provided time). It would probably make more sense if future accesses also returned a time formatted to that timezone, so the response during retrieval/list were the same as during create/update.

There is also the question of whether to return times in the configured timezone when the trailing Z is provided during creation/update, such that sending:

{
    "appointment": "2016-12-19T04:30:00Z"
}

returns:

{
    "appointment": "2016-12-19T10:00:00+5:30"
}

I would be for this, as it keeps the response consistent with the response for lists/retrievals.

Another option entirely would be to always return UTC times, even during creation/updates, but I find that less useful. Either way, having consistent timezones would be preferable to the 50/50ish situation we have right now.

@vpistis
Copy link

vpistis commented Dec 19, 2016

Set your TIME_ZONE = 'Asia/Kolkata' and create a model serializer with a single datetime field, appointment.

During create/update, send:

{
"appointment": "2016-12-19T10:00:00"
}
and get back:

{
"appointment": "2016-12-19T10:00:00+5:30"
}
But if you retrieve or list that object again, you get:

{
"appointment": "2016-12-19T04:30:00Z"
}

thanx @jonathan-golorry this is the exact behavior that I actually see.
For me the behavior should be like this (using @jonathan-golorry example :) ):

During create/update with default DATETIME_FORMAT, send:

{
    "appointment": "2016-12-19T10:00:00"
}

and get back:

{
    "appointment": "2016-12-19T10:00:00+5:30"
}

If you retrieve or list that object again , you get:

{
    "appointment": "2016-12-19T10:00:00+5:30"
}

IMHO maybe should be a DRF setting to manage this behavior, for example, a setting to force the DateTimeField to be represented with the default timezone.

@vpistis
Copy link

vpistis commented Dec 19, 2016

thanx a lot @tomchristie

@tomchristie
Copy link
Member

The inconsistency between differing actions is the clincher for reopening here. I'd not realized that was the case. I'd expect us to use UTC throughout by default, although we could make that optional.

@vpistis
Copy link

vpistis commented Dec 20, 2016

For a production web app that use DRF 3.4.6, we have resolved with this workaround:
vpistis@be62db9

@tomchristie
Copy link
Member

If anyone wants to submit a pull request that either includes:

  • just includes failing test cases, or...
  • test case + fix

that'd be most welcome.

@vpistis
Copy link

vpistis commented Jan 4, 2017

I have wrote some code to test, but I'm not sure how use drf test cases. I don't know how can manage django settings to change Timezone and others settings at runtime.
Please, link me some specific example or guide.

thanx

@rpkilby
Copy link
Member

rpkilby commented Jan 4, 2017

If you're looking to modify the global test settings, they're located here.

If you're trying to override the settings during testing, you can use the override_settings decorator.

@tomchristie tomchristie removed this from the 3.6.3 Release milestone May 12, 2017
@michaelaelise
Copy link

michaelaelise commented Jul 12, 2017

I see this is closed but I am running drf 3.6.3 and in my postgres database I have this timestamp "2017-07-12 14:26:00-06" but when I get the data using postman I get this "timestamp": "2017-07-12T20:26:00Z". I looks like it is adding on the -06 hours.

My django settings use tzlocal to set the timezone TIME_ZONE = str(get_localzone()). So the timezone is set on startup.

I am using a basic modelSerializer

class SnapshotSerializer(serializers.ModelSerializer):
    class Meta:
        model = Snapshot
        resource_name = 'snapshot' 
        read_only_fields = ('id',)
        fields = ('id', 'timestamp', 'snapshot')

Am I missing something?

@vpistis
Copy link

vpistis commented Jul 12, 2017

not closed, it's still open :)
the red "closed button" that you see is for the reference issue.
...and you are right, the "bug" is still there ;(
The milestone is changed form 3.6.3 to 3.6.4.

@michaelaelise
Copy link

michaelaelise commented Jul 13, 2017 via email

@RichardForshaw
Copy link

Hi,

I'm not sure if this is totally related to the original question, but should DRF be honouring any timezone overrides which are set, as per https://docs.djangoproject.com/en/1.11/ref/utils/#django.utils.timezone.activate ?

I have a system where my users are associated with a timezone, and the API receives naiive datetimes. I expect to be able to convert those datetimes into the current user's timezone, but I notice that ../rest_framework/fields.py is applying the default timezone (i.e. the one from the django setting file:

    def enforce_timezone(self, value):
        field_timezone = getattr(self, 'timezone', self.default_timezone())

        if (field_timezone is not None) and not timezone.is_aware(value):
            try:
                return timezone.make_aware(value, field_timezone)

[...]

    def default_timezone(self):
        return timezone.get_default_timezone() if settings.USE_TZ else None

Should this really be using timezone.get_current_timezone() as a preference in case the application has set an override, such as in my case?

@carltongibson
Copy link
Collaborator

Hi @RichardForshaw that seems like a distinct issue, but same ball park certainly.

If we can get a decent set of test cases covering the expected behaviour, I think we’d certainly look at a PR here.

My first thought is beyond that is to make sure you’re send the time zone info to the API, rather than relying on the server’s configured time zone. Beyond that I’m also inclined to think you need to be prepared to localise a time on the client.

But yes, there’s an inconsistency here to be addressed. (Did I mention PRs Welcome? 🙂)

@rpkilby
Copy link
Member

rpkilby commented Sep 4, 2017

carltongibson/django-filter#750 should be relevant here. I originally based django-filter's timezone handling on DRF, so the changes in 750 could easily be applied here.

@michaelaelise
Copy link

Sorry for my newbie-ness but what exactly is the issue here? The timestamp in my psql database is correct and Django is set to use the correct timezone. Is there a DRF settings to just make it not convert timestamps?

@carltongibson
Copy link
Collaborator

carltongibson commented Sep 5, 2017

Hi @michaelaelise — if you look at the example of the data (near the top):

  1. They're sending a datetime without timezone info. (This is a bad move in my book.)
  2. The server is applying its local timezone and it comes back as that (+5:30 in this case)
  3. But later, when you fetch it, it comes back as UTC (Z, for "Zulu" I suppose).

There's no real issue on the assumption your client will handle the timezone conversion for you. (Except maybe No1, because who's to say your client has the same timezone setting as your server...?)

But it's a little inconsistency, surely 2 and 3 should have the same format? (Even though a client will, correctly, accept either as equivalent values.)

@carltongibson
Copy link
Collaborator

carltongibson commented Sep 5, 2017

I'm inclined to close this.

  • There's no logic error here.
    • These are the same time: "2016-12-19T10:00:00+5:30" and "2016-12-19T04:30:00Z" — to a certain extent, who cares how they come back?
  • Thus, it's not something I can justify allocating time to really.
  • The ticket is 2 years old and no-one has offered a PR.

I'm happy to look at a PR but I'm not sure I want to really consider this an Open Issue.

@michaelaelise
Copy link

michaelaelise commented Sep 5, 2017

Oh, I did not realize that in my original post in this issue thread that the "Z" meant that.
So DRF is converting the aware datetime to UTC to be handled by the UI/caller?
Thank you for that clarification.

@michaelaelise
Copy link

One last thing.
What if we would like "2016-12-19T10:00:00+5:30" to be returned because we are polling devices in different timezones.
Could this be a setting "RETURN_DATETIME_WITH_TIMEZONE"?

We are using django/drf on edge devices. So all datetimes being inserted do not care if it is naive or not because the edge device timezone is configured and postgres field will always be accurate for that devices datetime.

The cloud server in the current scenario would then need to know the timezone of each device, it probably will, that just shifts the work from django/drf to the cloud app to handle.

@carltongibson
Copy link
Collaborator

Assuming USE_TZ, DRF is already returning date times with the time zone info. So it’s already doing what you need there.

The only issue here is whether the same DT is formatted as in one time zone or another. (But they’re still the same time.)

@vpistis
Copy link

vpistis commented Sep 7, 2017

@carltongibson

There's no logic error here.
These are the same time: "2016-12-19T10:00:00+5:30" and "2016-12-19T04:30:00Z" — to a certain extent, who cares how they come back?

IMHO this is the problem: the returned string!
I use Django Time zone settings and all templates return the correct time "2016-12-19T10:00:00+5:30" like we expected, but DRF not. DRF return "2016-12-19T04:30:00Z".
Into the client, that consumes my REST apis, ther's no logic, no times conversions or datetime string interpretation.
In other words, I expect that the datetime from a DRF response is identical to the Django Template "response": the server prepare all data for the client and the client only shows it.

Anyway, thanx a lot for your patience, support and your great work to this fantastic project!

@carltongibson
Copy link
Collaborator

@vpistis my point here is just that the represented date is correct, just the representation Is not expected. As soon as you parse that to a native Date, however your language handles that, there is no difference.

I would expect users to be parsing the date string to a Date, however their client language provides for that, rather than consuming the raw string.

I accept if you’re consuming the raw string your expectations wont be met here. (But don’t do that: imagine if we sent UNIX timestamps; there’s no way you’d consume those raw. Convert to a proper Date object, whatever that is in your client language.)

I’m really happy to take a PR on this. (I haven’t closed it yet!)

But it’s been nearly two years since reported and nine months since the first comment (yours, a year later). Nobody has even given us a failing test case. It can’t be that important to anyone. As such it’s hard to allocate it time.

(As such I’m inclined to close it on the basis that we’ll take a PR if one ever turns up)

@rpkilby
Copy link
Member

rpkilby commented Sep 11, 2017

Hi all, this should be fixed by #5408. If you have the time to install the branch and verify that everything is working as expected, that would be fantastic. Thanks!

@diegueus9
Copy link

diegueus9 commented Feb 27, 2019

I think the issue was somehow, re-introduced:

When I changed the default TZ from UTC to Europe/Amsterdam, one of the tests failed and I noticed DRF is serializing to something different from the default TZ

edit: issue was related to test/factory setup.

Test setup below.

model

class Something(StampedModelMixin):
    MIN_VALUE = 1
    MAX_VALUE = 500

    id = models.BigAutoField(primary_key=True)  # pylint: disable=blacklisted-name
    product_id = models.BigIntegerField(db_index=True)
    start_time = models.DateTimeField()
    end_time = models.DateTimeField()
    percentage = models.IntegerField()
    enabled = models.IntegerField()

factory

class SomethingFactory(factory.django.DjangoModelFactory):
    """ Base Factory to create records for Something

    """
    start_time = factory.Faker('date_time', tzinfo=get_default_timezone())
    end_time = factory.Faker('date_time', tzinfo=get_default_timezone())
    percentage = factory.Faker('random_int', min=1, max=500)
    enabled = factory.Faker('random_element', elements=[0, 1])

    class Meta:  # pylint: disable=missing-docstring
        model = Something

unit test

class TestSomething:
    def test__get__empty(self):
        # preparation of data
        series = SeriesFactory.create(product_id=2)
        SomethingFactory.create_batch(3, product_id=1)

        # prepare request params
        url = reverse('series-somethings', kwargs={'pk': series.pk})

        # call the endpoint
        response = self.client.get(url)

        # asserts
        assert response.data == []

    def test__get__single(self):
        # preparation of data
        series = SeriesFactory.create(product_id=1)
        old_somethings = SomethingFactory.create_batch(1, product_id=1)

        # prepare request params
        url = reverse('series-somethings', kwargs={'pk': series.pk})

        # call the endpoint
        response = self.client.get(url)

        # asserts
        assert SomethingSerializer(old_somethings, many=True).data == response.data

view

class SomethingElseView(APILogMixin, ModelViewSet):
    @action(detail=True, methods=['get'])
    def somethings(self, request, pk=None):
        """ GET endpoint for Somethings

        .. seealso:: :func:`rest_framework.decorators.action`
        """
        otherthings = self.get_object()
        something_qs = Something.objects.all()
        something_qs = something_qs.filter(product_id=otherthings.product_id)
        serializer = self.something_serializer_class(something_qs, many=True)
        return Response(serializer.data)

Serializer

class SomethingSerializer(serializers.ModelSerializer):

    class Meta:
        model = Something
        list_serializer_class = SomethingListSerializer
        fields = '__all__'
        extra_kwargs = {
            'percentage': {'min_value': Something.MIN_VALUE,
                           'max_value': Something.MAX_VALUE}
        }
        read_only_fields = ('id',
                            'ts_activated',
                            'ts_created',
                            'ts_updated')

Test ipdb result

ipdb> old_somethings
[{'product_id': 1, 'start_time': datetime.datetime(2011, 7, 13, 1, 10, 33, tzinfo=<DstTzInfo 'Europe/Amsterdam' LMT+0:20:00 STD>), 'end_time': datetime.datetime(2003, 3, 10, 9, 31, tzinfo=<DstTzInfo 'Europe/Amsterdam' LMT+0:20:00 STD>), 'percentage': 103, 'enabled': 0}]
ipdb> response.data
[OrderedDict([('id', 1), ('ts_created', '2019-02-27 14:16:33'), ('ts_updated', '2019-02-27 14:16:33'), ('product_id', 1), ('start_time', '2011-07-13 02:50:33'), ('end_time', '2003-03-10 10:11:00'), ('percentage', 103), ('enabled', 0)])]

Test Result

E       AssertionError: assert [OrderedDict(...nabled', 0)])] == [OrderedDict([...nabled', 0)])]
E         At index 0 diff: OrderedDict([('id', 1), ('ts_created', '2019-02-27 14:38:15'), ('ts_updated', '2019-02-27 14:38:15'), ('product_id', 1), ('start_time', '2011-07-13 01:10:33'), ('end_time', '2003-03-10 09:31:00'), ('percentage', 103), ('enabled', 0)]) != OrderedDict([('id', 1), ('ts_created', '2019-02-27 14:38:15'), ('ts_updated', '2019-02-27 14:38:15'), ('product_id', 1), ('start_time', '2011-07-13 02:50:33'), ('end_time', '2003-03-10 10:11:00'), ('percentage', 103), ('enabled', 0)])
E         Full diff:
E         - [OrderedDict([('id', 1), ('ts_created', '2019-02-27 14:38:15'), ('ts_updated', '2019-02-27 14:38:15'), ('product_id', 1), ('start_time', '2011-07-13 01:10:33'), ('end_time', '2003-03-10 09:31:00'), ('percentage', 103), ('enabled', 0)])]
E         ?                                                                                                                                                       ^^^^^^^^^^                          ^^^^^^^^^^^
E         + [OrderedDict([('id', 1), ('ts_created', '2019-02-27 14:38:15'), ('ts_updated', '2019-02-27 14:38:15'), ('product_id', 1), ('start_time', '2011-07-13 02:50:33'), ('end_time', '2003-03-10 10:11:00'), ('percentage', 103), ('enabled', 0)])]
E         ?

Stack:

Django==2.0.10
djangorestframework==3.9.0
factory-boy==2.11.1
Faker==0.8.18
pytz==2018.9

@rpkilby
Copy link
Member

rpkilby commented Mar 1, 2019

Hi @diegueus9 - could you possible reduce this to a simpler test case? You're comparing serialized fake data vs a view's response data. So it's not clear what the actual expected value is. I'd recommend comparing some result to a hardcoded value. e.g.,

assert SomethingSerializer(old_somethings, many=True).data == {'blah':'blah'}

@diegueus9
Copy link

@rpkilby thanks for your answer. It was my mistake, the problem with my unit tests is that I was using factory-boy/faker for it without refreshing from DB, hence the difference, I just added

    for old_something in old_somethings:
        old_something.refresh_from_db()

Should I remove my previous comment or should I leave it in case anyone else experiences the same false positive?

@rpkilby
Copy link
Member

rpkilby commented Mar 6, 2019

Hi @diegueus9, no worries - I just hid the code in a details tag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants