Opened 6 years ago

Closed 6 years ago

#29754 closed New feature (fixed)

Trunc() should allow passing is_dst resolution to avoid NonExistentTimeError/AmbiguousTimeError

Reported by: Alexander Holmbäck Owned by: Alexander Holmbäck
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: pytz, Trunc, is_dst
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Alexander Holmbäck)

When Trunc() truncates to a nonexisting or ambiguous datetime, the exception raised by pytz remains unhandled. The expected behavior would, IMO, be to not check the validity of truncated dates.

This test for example:

import datetime
import pytz

from django.db.models.functions import Trunc
from django.test import TestCase
from django.utils import timezone

from .models import Log


class TestTruncateToInvalidTime(TestCase):

    def test_truncate_to_dst_ends_stockholm(self):
        tzinfo = pytz.timezone('Europe/Stockholm')
        timestamp = datetime.datetime(2018, 10, 28, 2, tzinfo=tzinfo)
        Log.objects.create(timestamp=timestamp)
        logs = Log.objects.annotate(day=Trunc('timestamp', 'hour')).all()

        timezone.activate(tzinfo)
        self.assertEqual(logs[0].day.day, 28)

Results in the following error:

======================================================================
ERROR: test_truncate_to_dst_ends_stockholm (trunc.tests.TestTruncateInvalidTime)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/alex/tickets/trunc/tests.py", line 47, in test_truncate_to_dst_ends_stockholm
    self.assertEqual(logs[0].day.day, 28)
  File "/home/alex/django/django/db/models/query.py", line 303, in __getitem__
    qs._fetch_all()
  File "/home/alex/django/django/db/models/query.py", line 1190, in _fetch_all
    self._result_cache = list(self._iterable_class(self))
  File "/home/alex/django/django/db/models/query.py", line 64, in __iter__
    for row in compiler.results_iter(results):
  File "/home/alex/django/django/db/models/sql/compiler.py", line 1013, in apply_converters
    value = converter(value, expression, connection)
  File "/home/alex/django/django/db/models/functions/datetime.py", line 225, in convert_value
    value = timezone.make_aware(value, self.tzinfo)
  File "/home/alex/django/django/utils/timezone.py", line 270, in make_aware
    return timezone.localize(value, is_dst=is_dst)
  File "/home/alex/.virtualenvs/djangodev/lib/python3.6/site-packages/pytz/tzinfo.py", line 363, in localize
    raise AmbiguousTimeError(dt)
pytz.exceptions.AmbiguousTimeError: 2018-10-28 02:00:00

Change History (15)

comment:1 by Alexander Holmbäck, 6 years ago

Description: modified (diff)
Summary: Trunc() doesn't handle NonExistingTimeError/AmbiguousTimeErrorTrunc() doesn't handle NonExistentTimeError/AmbiguousTimeError

comment:2 by Tim Graham, 6 years ago

I don't have much expertise but reading the documentation, it sounds like you may be creating invalid data. Django converts datetimes when USE_TZ is activate, I don't think it can hide that exception. Did you carefully review the pytz documentation which states, "Unfortunately using the tzinfo argument of the standard datetime constructors ‘’does not work’’ with pytz for many timezones."

comment:3 by Simon Charette, 6 years ago

The AmbiguousTimeError case could be worked around by adding a Trunc(is_dst=None) optional argument that would be passed down to localize. That would make the API a bit more usable when tzinfo is provided and you know what you're after.

I'm afraid there isn't much that can be done for NonExistentTimeError though.

comment:4 by Alexander Holmbäck, 6 years ago

Thanks Tim for the valid input, but the situation remains even with dates constructed according to pytz documentation, which I hadn't carefully read ;-)

Btw, here's possible solution: PR.

If this is a no-go, I think the test admin_views.tests.AdminViewBasicTest.test_date_hierarchy_timezone_dst should be changed so that it expects an exception. Otherwise the date hierarchy tag is bound to only deal with UTC (which leads to #29724).

Last edited 6 years ago by Alexander Holmbäck (previous) (diff)

comment:5 by Simon Charette, 6 years ago

Has patch: set
Keywords: Trunc is_dst added; Trunc() removed
Summary: Trunc() doesn't handle NonExistentTimeError/AmbiguousTimeErrorTrunc() should allow passing is_dst resolution to avoid NonExistentTimeError/AmbiguousTimeError
Triage Stage: UnreviewedAccepted
Type: BugNew feature

Accepting on the basis that allowing to pass is_dst=(True|False) to Trunc and friends is a feature request analogous to #22598.

comment:6 by Simon Charette, 6 years ago

Needs documentation: set
Patch needs improvement: set

comment:7 by Alexander Holmbäck, 6 years ago

Owner: changed from nobody to Alexander Holmbäck
Status: newassigned

comment:8 by Alexander Holmbäck, 6 years ago

Needs documentation: unset
Patch needs improvement: unset

comment:10 by Simon Charette, 6 years ago

Alexander, I think this should be handled in a separate ticket.

comment:11 by Alexander Holmbäck, 6 years ago

Patch needs improvement: set

Ok I'v been digging a bit deeper into this, here's a recap and a few proposals.

When make_aware() is fed a datetime that isn't valid in the current timezone, it convert it to standard time if is_dst=False and daylight saving time if is_dst=True. If is_dst=None, which is default, it raises an exception. All this seems reasonable, it departs from pytz default which is to convert the invalid datetime to normal time, but that's justifieable and well documented.

In the case of Trunc() however, having make_aware() raise an excpetion when a truncated date is invalid is overly drastic. Hence, the current patch introduces a new optional flag is_dst to TruncBase() so that developers can decide for themselves how invalid times should be treated. But the consequences of not setting that flag can be dire and predicting those consequences would be difficult for all but the must timezone savvy developers (I know I wouldn't).

That said, I've been thinking about alternative solutions.

1) Pass is_dst=False to make_aware(). This would resonate with the behavior of pytz but it covers up invalid datetimes and a developer would need to re-localize them to know which datetime is valid and which isn't. I's also odd to explicitly choose standard time for an invalid datetime, even if that's what pytz does.

2) Keep invalid datetimes naive. This would make invalid datetimes easier to distinguish from valid datetimes but as they are naive they wouldn't always behave like aware datetimes which could cause other problems down the line.

3) Create a subclass from pytz.tzinfo.DstTzInfo called InvalidDstTzInfo and attach that to tzinfo for invalid dates. This would make invalid datetimes behave just like valid datetimes but also easy to recognize. This could be neat but it entangles django's timezone functionality with pytz in ways that will make it more difficult to maintain.

I'm leaning slightly towards the first alternative. If we want to help developers checking for invalid dates we could do as dateutil does and provide timezone.datetime_ambiguous(dt) and timezone.datetime_exists(dt) as helper functions.

Thoughts?

comment:12 by Simon Charette, 6 years ago

In my opinion we should being by landing a patch to allows passing is_dst and discussing changing the default value to False for both this function and make_aware should be discussed in another ticket.

Version 0, edited 6 years ago by Simon Charette (next)

comment:13 by Alexander Holmbäck, 6 years ago

Patch needs improvement: unset

Ok I agree with that.

PR is ready for review.

comment:14 by Simon Charette, 6 years ago

Triage Stage: AcceptedReady for checkin

PR LGTM except for the versionchanged and release notes that were targeting 2.2 but this can be adjusted by the committers.

Thanks for the work and discussion Alexander. If you want to discuss making is_dst default to False I suggest you discuss it on the developer mailing list where you'd likely to attract much more feedback from the community than this ticket tracker. Thanks!

comment:15 by Tim Graham <timograham@…>, 6 years ago

Resolution: fixed
Status: assignedclosed

In d527639:

Fixed #29754 -- Added is_dst parameter to Trunc database functions.

Note: See TracTickets for help on using tickets.
Back to Top