Edgewall Software
Modify

Opened 2 years ago

Closed 2 years ago

Last modified 22 months ago

#10863 closed defect (fixed)

fix to_datetime() when given timezone naive datetime input

Reported by: cboos Owned by: cboos
Priority: normal Milestone: 0.12.5
Component: general Version: 0.12-stable
Severity: normal Keywords: datetime pytz timezone
Cc:
Release Notes:
API Changes:

to_datetime now always return timezone aware datetime objects, which are "normalized" in the pytz sense.

Description

While looking at some specifics of the trac.util.datefmt.to_datetime function, I discovered that a particular usage pattern is problematic with pytz timezones:

>>> from trac.util.datefmt import *
>>> from datetime import datetime
>>> t = to_datetime(datetime(2012,1,1), timezone("Europe/Paris"))
>>> t.utcoffset()
datetime.timedelta(0, 540)

(from gmessage:trac-dev:V0HeDyfa_pk/5r33UatF3nEJ)

The utcoffset() plays a role when converting a time to the number of ms (to_utimestamp()). Here, it has a value of 9 minutes, which is quite wrong for our purpose.

The workaround is to always specify a timezone aware datetime object to to_datetime, but this is not documented, and shouldn't be necessary either (assuming a timezone naive datetime should actually be the same as a datetime in utc).

The following should

  • trac/util/datefmt.py

    diff --git a/trac/util/datefmt.py b/trac/util/datefmt.py
    index b59a276..d88d6b1 100644
    a b def to_datetime(t, tzinfo=None): 
    6666    elif isinstance(t, datetime): 
    6767        if tzinfo is not None: 
    6868            if t.tzinfo is None: 
    69                 t = t.replace(tzinfo=tzinfo) 
     69                t = tzinfo.localize(t) 
    7070            else: 
    7171                t = tzinfo.normalize(t.astimezone(tzinfo)) 
    7272        return t 

With this change, the t.utcoffset() for the example above would give 3600 seconds, i.e. the one hour we would expect.

Attachments (1)

t10863-to_datetime-normalized.diff (4.6 KB) - added by cboos 2 years ago.
rework to_datetime

Download all attachments as: .zip

Change History (11)

comment:1 Changed 2 years ago by cboos

  • Milestone set to 0.12.5

I think it's OK for 0.12-stable. Feedback welcome!

comment:2 Changed 2 years ago by cboos

To be complete, it seems that datetime.fromtimestamp() doesn't have the problem that the datetime constructor has:

>>> datetime.fromtimestamp(1325376000, timezone("Europe/Paris")).utcoffset()
datetime.timedelta(0, 3600)

So we don't need to change the other parts of to_datetime().

comment:3 Changed 2 years ago by cboos

Same for datetime.now(), it's OK as well:

>>> datetime.now(timezone("Europe/Paris")).utcoffset()
datetime.timedelta(0, 7200)

(see also r8857)

comment:4 Changed 2 years ago by jomae

It looks good except datetime between DST boundaries. tzinfo.localize() generates invalid datetime between DST boundaries. I think we should use tzinfo.normalize().

>>> tz = timezone('Europe/Paris')
>>> t = datetime(2012, 3, 25, 2, 15)
>>> tz.localize(t)
datetime.datetime(2012, 3, 25, 2, 15, tzinfo=<DstTzInfo 'Europe/Paris' CET+1:00:00 STD>)
>>> tz.localize(t).utcoffset()
datetime.timedelta(0, 3600)
>>> tz.normalize(tz.localize(t))
datetime.datetime(2012, 3, 25, 3, 15, tzinfo=<DstTzInfo 'Europe/Paris' CEST+2:00:00 DST>)
>>> tz.normalize(tz.localize(t)).utcoffset()
datetime.timedelta(0, 7200)
  • trac/util/datefmt.py

    diff --git a/trac/util/datefmt.py b/trac/util/datefmt.py
    index b59a276..0fd4aa8 100644
    a b def to_datetime(t, tzinfo=None): 
    6666    elif isinstance(t, datetime): 
    6767        if tzinfo is not None: 
    6868            if t.tzinfo is None: 
    69                 t = t.replace(tzinfo=tzinfo) 
     69                t = tzinfo.localize(t) 
     70                t = tzinfo.normalize(t) 
    7071            else: 
    7172                t = tzinfo.normalize(t.astimezone(tzinfo)) 
    7273        return t 

comment:5 Changed 2 years ago by cboos

Ah, yes! Thanks for the example, I'll integrate it into an unit-test.

comment:6 Changed 2 years ago by cboos

I've tried a more "radical" approach, by making to_datetime(t, tz) always:

  • map the given input time to the given timezone (or localtz if no tz is specified)
  • return a normalized datetime (also going in the direction of #10864)

I think it's a more consistent API and it's easier to use and to remember what it does. All tests still pass.

Changed 2 years ago by cboos

rework to_datetime

comment:7 Changed 2 years ago by jomae

I tried the latest patch and it works good!

comment:8 Changed 2 years ago by cboos

  • API Changes modified (diff)
  • Resolution set to fixed
  • Status changed from new to closed
  • Version set to 0.12-stable

Applied in r11397. Thanks for the review!

comment:9 Changed 2 years ago by Genie

  • Keywords pytz timezone added

comment:10 Changed 22 months ago by cboos

  • Owner set to cboos

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed The owner will remain cboos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from cboos to the specified user.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.