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

[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

[Tim Graham]

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."

[Simon Charette]

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.

[Alexander Holmbäck]

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).

[Simon Charette]

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

[Alexander Holmbäck]

Should ​https://docs.djangoproject.com/en/2.1/ref/models/querysets/#datetimes also take a tz_info?

[Simon Charette]

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

[Alexander Holmbäck]

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?

[Simon Charette]

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.

[Alexander Holmbäck]

Ok I agree with that.

​PR is ready for review.

[Simon Charette]

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!

[Tim Graham <timograham@…>]

In d527639:

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