Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#32661 closed Cleanup/optimization (invalid)

An exception should be raised when trying to save datetime/time object and the UTC offset isn't known.

Reported by: Armin Stepanjan Owned by: Abhyudai
Component: Database layer (models, ORM) Version: 3.2
Severity: Normal Keywords: timezone, datetime, datetimefield, timefield, models
Cc: Aymeric Augustin Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

As tzinfo is currently dropped silently from TimeField it leads to tricky bugs for users who expect TimeField to have consistent behaviour with DateTimeField.

This issue suggests two changes, happy to make a PR if the community aggrees with them:

  1. Mention that TimeField drops timezone in Model field reference: https://docs.djangoproject.com/en/3.2/ref/models/fields/#timefield
  1. Throw a warning when a TimeField with tzinfo is saved (similarly as there's a warning when a naive DateTimeField is saved).

Change History (11)

in reply to:  description comment:1 by Mariusz Felisiak, 3 years ago

Cc: Aymeric Augustin added
Easy pickings: unset
Summary: Make lack of timezone support in TimeField more explicit.An exception should be raised when trying to save an aware time object.
Triage Stage: UnreviewedAccepted

This issue suggests two changes, happy to make a PR if the community aggrees with them:

  1. Mention that TimeField drops timezone in Model field reference: https://docs.djangoproject.com/en/3.2/ref/models/fields/#timefield

It's already documented that: "Django only supports naive time objects and will raise an exception if you attempt to save an aware time object, as a timezone for a time with no associated date does not make sense."

  1. Throw a warning when a TimeField with tzinfo is saved (similarly as there's a warning when a naive DateTimeField is saved).

Django should raise an exception if you attempt to save an aware time object (at least on Oracle, MySQL, and SQLite), however it looks that timezone.is_aware() doesn't work for datetime.time.

>>> t = datetime.time(21, 22, 23, 240000, tzinfo=timezone.get_current_timezone())
>>> t.tzinfo
<DstTzInfo 'America/Chicago' LMT-1 day, 18:09:00 STD>
>>> timezone.is_aware(t)
False

Maybe it's enough to check tzinfo.

comment:2 by Itay Keren, 3 years ago

Owner: changed from nobody to Itay Keren
Status: newassigned

comment:3 by Mariusz Felisiak, 3 years ago

Has patch: set
Needs tests: set
Patch needs improvement: set

comment:4 by Abhyudai, 3 years ago

Has patch: unset
Needs tests: unset
Owner: changed from Itay Keren to Abhyudai
Patch needs improvement: unset

comment:5 by Abhyudai, 3 years ago

Has patch: set

comment:6 by Mariusz Felisiak, 3 years ago

Summary: An exception should be raised when trying to save an aware time object.An exception should be raised when trying to save an aware datetime/time object even when the UTC offset isn't known.

I double checked, this issue is more tricky and DateTimeFields are also affected. tzinfo subclasses may not specify the UTC offset (e.g. pytz.timezone("Europe/Paris")), in such cases we need to also check tzinfo.

comment:7 by Mariusz Felisiak, 3 years ago

Needs tests: set
Patch needs improvement: set

comment:8 by Mariusz Felisiak, 3 years ago

comment:9 by Abhyudai, 3 years ago

Needs tests: unset
Patch needs improvement: unset

comment:10 by Mariusz Felisiak, 3 years ago

Resolution: invalid
Status: assignedclosed

datetime/time objects with tzinfo which don't declare utcoffset should be treated as naive. Closing as invalid per Aymeric's comment:

This code has been working well for over 10 years so let's be careful. Here's my analysis. 

The docs say:

> A datetime object d is aware if both of the following hold:
>
> 1. d.tzinfo is not None
> 2. d.tzinfo.utcoffset(d) does not return None
>
> Otherwise, d is naive.

This hasn't changed this Python 2.7. (I didn't check further back.)

This was explicitly the implementation before 432678dbc1dd4f80203468d83bb0eb6c20ed5247.

Also, the documentation of datetime.utcoffset, `value.utcoffset()` literally implemented as `None if value.tzinfo is None else value.tzinfo.utcoffset(value)`.

As a consequence of the above, each of the following propositions are strictly equivalent:

...

Therefore I believe the change proposed here is wrong for at least three reasons:

- Any logic changes in this area will diverge from the definition in the Python docs.
- The proposed logic is redundant — if `value.tzinfo is None`, then `value.utcoffset() is None`: this is the first six words of the documentation of datetime.utcoffset: "If tzinfo is None, returns None"; I don't see how adding this redundant logic is an improvement.
- The proposed logic is wrong — if you wanted to go back to the previous implementation, you should use `and` instead of `or` in `is_naive`.

Finally, I believe that have two one-liner convenience functions implemented as `value.utcoffset() is None` and `value.utcoffset() is not None` is fine. I don't see the need for adding a level of indirection and the overhead of a function call here.

comment:11 by Mariusz Felisiak, 3 years ago

Summary: An exception should be raised when trying to save an aware datetime/time object even when the UTC offset isn't known.An exception should be raised when trying to save datetime/time object and the UTC offset isn't known.
Note: See TracTickets for help on using tickets.
Back to Top