Edgewall Software
Modify

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#10863 closed defect (fixed)

fix to_datetime() when given timezone naive datetime input

Reported by: Christian Boos Owned by: Christian Boos
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 Christian Boos 3 years ago.
rework to_datetime

Download all attachments as: .zip

Change History (11)

comment:1 Changed 3 years ago by Christian Boos

  • Milestone set to 0.12.5

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

comment:2 Changed 3 years ago by Christian Boos

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 3 years ago by Christian Boos

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 3 years ago by Jun Omae

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 3 years ago by Christian Boos

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

comment:6 Changed 3 years ago by Christian Boos

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 3 years ago by Christian Boos

rework to_datetime

comment:7 Changed 3 years ago by Jun Omae

I tried the latest patch and it works good!

comment:8 Changed 3 years ago by Christian Boos

  • 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 3 years ago by Genie

  • Keywords pytz timezone added

comment:10 Changed 3 years ago by Christian Boos

  • Owner set to Christian Boos

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Christian Boos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Christian Boos to the specified user.

Add Comment


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

 
Note: See TracTickets for help on using tickets.