Edgewall Software
Modify

Opened 12 years ago

Closed 11 years ago

Last modified 11 years ago

#10864 closed defect (fixed)

consistent use of tzinfo.normalize()

Reported by: Christian Boos Owned by: Jun Omae
Priority: normal Milestone: 0.12.5
Component: general Version:
Severity: normal Keywords: datetime pytz timezone
Cc: Branch:
Release Notes:

Fix of datetime arithmetic across DST boundaries with pytz timezone

API Changes:

parse_date now always returns "normalized" datetime objects across DST boundaries

Internal Changes:

Description

As a follow-up to #10863, I had a closer look to our use of tz.normalize().

According to http://pytz.sourceforge.net/#localized-times-and-date-arithmetic, using normalize() is necessary after having performed arithmetic on localized datetime, and when switching timezones (t.astimezone(tz), except when tz is utc).

There are several places in trac/util/datefmt.py where we use astimezone() or do arithmetic without wrapping this in normalize().

There's also a place in to_datetime() where we use normalize(localize()), which seems unnecessary.

Beyond datefmt.py, there are other places where we do arithmetic on localized dates, we should handle them as well when the end result will likely end up being displayed (e.g. a milestone's duedate). It could be left unchanged when the computed datetime will be used for its "absolute" value (e.g. start in the TimelineModule).

Attachments (2)

t10864-normalize-r11349.diff (3.0 KB ) - added by Christian Boos 12 years ago.
adding calls to normalize where I think we should call it, removing it when it shouldn't be needed
t10864-normalize-r11349.2.diff (3.1 KB ) - added by Christian Boos 12 years ago.
second take, now only adding some calls to normalize

Download all attachments as: .zip

Change History (17)

by Christian Boos, 12 years ago

adding calls to normalize where I think we should call it, removing it when it shouldn't be needed

comment:1 by Christian Boos, 12 years ago

Milestone: 0.12.5

As for #10863, I think this could be OK for 0.12-stable but I'd like to get some reviewing first!

The patch is for trunk and lacks extra tests for now.


In the meantime, I saw jomae comment on #10863, so I'll adapt the patch shortly…

by Christian Boos, 12 years ago

second take, now only adding some calls to normalize

in reply to:  description comment:2 by Jun Omae, 11 years ago

I confirmed the patch. If accoss a DST boundary, it would suggest 17:00 or 19:00 in trac/ticket/roadmap.py. Using datetime.replace makes similar problems, e.g. _parse_relative_time in trac/util/datefmt.py.

My suggested patch to use to_datetime instead of datetime.replace is based on r11388 with t10863-to_datetime-normalized.diff.

  • trac/ticket/roadmap.py

    diff --git a/trac/ticket/roadmap.py b/trac/ticket/roadmap.py
    index dbbb5a4..f6e67f2 100644
    a b from trac.search import ISearchSource, search_to_sql, shorten_result  
    3333from trac.util import as_bool
    3434from trac.util.datefmt import parse_date, utc, to_utimestamp, \
    3535                              get_datetime_format_hint, format_date, \
    36                               format_datetime, from_utimestamp, user_time
     36                              format_datetime, from_utimestamp, user_time, \
     37                              to_datetime
    3738from trac.util.text import CRLF
    3839from trac.util.translation import _, tag_
    3940from trac.ticket import Milestone, Ticket, TicketSystem, group_milestones
    class MilestoneModule(Component):  
    793794
    794795    def _render_editor(self, req, milestone):
    795796        # Suggest a default due time of 18:00 in the user's timezone
    796         default_due = datetime.now(req.tz).replace(hour=18, minute=0, second=0,
    797                                                    microsecond=0)
    798         if default_due <= datetime.now(utc):
    799             default_due += timedelta(days=1)
    800 
     797        now = datetime.now(req.tz)
     798        default_due = to_datetime(datetime(now.year, now.month, now.day, 18),
     799                                  tzinfo=req.tz)
     800        if default_due <= now:
     801            default_due = req.tz.normalize(default_due + timedelta(days=1))
     802            if default_due.hour != 18: # if across a DST boundary
     803                default_due += timedelta(hours=18 - default_due.hour)
     804
    801805        data = {
    802806            'milestone': milestone,
    803807            'datetime_hint': get_datetime_format_hint(req.lc_time),
Last edited 11 years ago by Jun Omae (previous) (diff)

comment:3 by Christian Boos, 11 years ago

Shouldn't the last line be more something like:

default_due = to_datetime(default_due.replace(hours=18))

i.e. have a normalize step after doing arithmetics with timedelta or like here, a .replace() (the latter should achieve the same effect, but more directly).

Also for _parse_relative_time, it's only used in parse_date which does call to_datetime() on the returned value of _parse_relative_time, so I think it's OK.

in reply to:  3 ; comment:4 by Jun Omae, 11 years ago

Replying to cboos:

Shouldn't the last line be more something like:

default_due = to_datetime(default_due.replace(hours=18))

i.e. have a normalize step after doing arithmetics with timedelta or like here, a .replace() (the latter should achieve the same effect, but more directly).

Oh, I think it would be simple to call to_datetime at the end to localize and normalize.

    def _render_editor(self, req, db, milestone):
        # Suggest a default due time of 18:00 in the user's timezone
        now = datetime.now()
        default_due = now.replace(hour=18, minute=0, second=0, microsecond=0)
        if default_due <= now:
            default_due = default_due + timedelta(days=1)
        default_due = to_datetime(default_due, req.tz)

Also for _parse_relative_time, it's only used in parse_date which does call to_datetime() on the returned value of _parse_relative_time, so I think it's OK.

Sure. But we don't use the returned value of to_datetime….

And, the following code introduced in r6318 does not work at trunk/trac/util/datefmt.py@11332:479-486#L451.

    # Make sure we can convert it to a timestamp and back - fromtimestamp()
    # may raise ValueError if larger than platform C localtime() or gmtime()
    try:
        to_datetime(to_timestamp(dt), tzinfo)
    except ValueError:
>>> from trac.util.datefmt import parse_date, to_timestamp, to_datetime, timezone
>>> t = parse_date('2100-01-01T01:01:01Z')
>>> t
datetime.datetime(2100, 1, 1, 1, 1, 1, tzinfo=<FixedOffset "UTC" 0:00:00>)
>>> to_timestamp(t)
4102448461L
>>> to_datetime(to_timestamp(t), timezone('Europe/Paris'))
datetime.datetime(1970, 1, 1, 2, 8, 22, 448461, tzinfo=<DstTzInfo 'Europe/Paris' CET+1:00:00 STD>)
Version 0, edited 11 years ago by Jun Omae (next)

in reply to:  4 comment:5 by Christian Boos, 11 years ago

Replying to jomae:

And, the following code introduced in r6318 does not work at trunk/trac/util/datefmt.py@11332:479-486#L451.

This seems related to r9789: the values for _min_ts and _max_ts seem rather arbitrary there, as those values are rather useful for avoiding to raise the ValueError when using datetime.fromtimestamp() in the first place.

The boundary values we should check before attempting to "convert" from usec to sec should rather be to_timestamp(to_datetime(datetime.{min,max})).

  • trac/util/datefmt.py

     
    6161    elif isinstance(t, date):
    6262        dt = tz.localize(datetime(t.year, t.month, t.day))
    6363    elif isinstance(t, (int, long, float)):
    64         if not (_min_ts <= t <= _max_ts):
     64        if not (_min_ts_dt <= t <= _max_ts_dt):
    6565            # Handle microsecond timestamps for 0.11 compatibility
    6666            t *= 0.000001
    6767        if t < 0 and isinstance(t, float):
     
    444444_zero = timedelta(0)
    445445_min_ts = -(1 << 31)
    446446_max_ts = (1 << 31) - 1
     447_min_ts_dt = -62135600400L # to_timestamp(to_datetime(datetime.min))
     448_max_ts_dt = 253402297199L # to_timestamp(to_datetime(datetime.max))
    447449
    448450localtz = LocalTimezone()
>>> from trac.util.datefmt import *
>>> t = parse_date('2100-01-01T01:01:01Z')
Traceback (most recent call last):
  File "<stdin>", line 1, in ?
  File "trac\util\datefmt.py", line 282, in parse_date
    _('Invalid Date'))
trac.core.TracError: The date "2100-01-01T01:01:01Z" is outside valid range. Try a date closer to present time.

FIXED (see comment:12)

Last edited 11 years ago by Christian Boos (previous) (diff)

comment:6 by Genie, 11 years ago

Keywords: timezone added

comment:7 by Jun Omae, 11 years ago

I worked in repos:jomae.git:ticket10864/0.12, fixed some defects and added some unit tests for datefmt.*. Would somebody please review it?

comment:8 by Christian Boos, 11 years ago

Still got one failure with [4ab8ae8df/jomae.git], on Windows 7, Python 2.7.2:

FAIL: test_max_timestamp (trac.util.tests.datefmt.ParseDateValidRangeTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "c:\Trac\repos\0.12-stable\trac\util\tests\datefmt.py", line 455, in test_max_timestamp
    '2038-01-19T03:14:08Z')
AssertionError: TracError not raised

Suggesting something like:

  • trac/util/tests/datefmt.py

    diff --git a/trac/util/tests/datefmt.py b/trac/util/tests/datefmt.py
    index 3c8c546..9008da1 100644
    a b class ParseDateValidRangeTestCase(unittest.TestCase):  
    459460        self.assertRaises(TracError, datefmt.parse_date,
    460461                          '1901-12-13T20:45:51Z')
    461462
    462     if os.name == 'nt': # negative timestamps not supported on Windows
    463         del test_min_timestamp
     463    if os.name == 'nt':
     464        if sys.version_info < (2, 6):
     465            del test_min_timestamp
     466        else:
     467            # maximal timestamp is much higher on Windows after 2.6
     468            def test_max_timestamp(self):
     469                datefmt.parse_date('3001-01-01T20:59:59Z')
     470                self.assertRaises(TracError, datefmt.parse_date,
     471                                  '3001-01-01T21:00:00Z')
     472
     473            # negative timestamps supported a tiny bit on Windows after 2.6
     474            def test_min_timestamp(self):
     475                datefmt.parse_date('1969-12-31T12:00:00Z')
     476                self.assertRaises(TracError, datefmt.parse_date,
     477                                  '1969-12-31T11:59:59Z')
    464478
    465479
    466480class DateFormatTestCase(unittest.TestCase):

(see [2240fdf2/cboos.git])

The above works with 2.4.4, 2.5.4, 2.6.5, and 2.7.2. I have no idea if the change happened for 2.6 strict or in versions between 2.5.4 and 2.6.5…

comment:9 by Christian Boos, 11 years ago

Another remark: what about using a bit of "Hungarian notation" for the datetime objects in timeline/web_ui.py and ticket/roadmap.py? It could make the usage clearer if we track the nature of the datetime object in the name (e.g. naive_yesterday instead of just yesterday).

Note that I'm not suggesting to do that in datefmt, as there we are supposed to be careful and know what we're doing. Elsewhere, it would remind there's something special going on.

in reply to:  8 comment:10 by Christian Boos, 11 years ago

Replying to cboos:

… (see [2240fdf2/cboos.git])

And that's not even enough: on Linux x86_64, Python 2.7.3, the assertions are not raised either.

comment:11 by Christian Boos, 11 years ago

I confirm that with [b3524a99/jomae.git], the tests pass on my platforms (Python 2.5.4, 2.6.5, 2.7.2 (32-bits) and Python 2.7.2 (64-bits) on Windows 7 (x64), Python 2.7.3 64-bits on Linux x86_64).

What about comment:5 and comment:9? And are you planning more changes? Otherwise we could start with applying what you have so far here, refining if necessary and then merge #10912 (but you may have a better plan, so I let you "drive" in the end ;-) ).

in reply to:  11 comment:12 by Jun Omae, 11 years ago

Replying to cboos:

I confirm that with [b3524a99/jomae.git], the tests pass on my platforms (Python 2.5.4, 2.6.5, 2.7.2 (32-bits) and Python 2.7.2 (64-bits) on Windows 7 (x64), Python 2.7.3 64-bits on Linux x86_64).

Thanks for your confirmations! I've tested with my environments:

  • Python 2.7.1 (64-bits) on Ubuntu 12.10
  • Python 2.5.5 and 2.6.6 (64-bits) on Debian 6.0.6
  • Python 2.4.3, 2.5.6, 2.6.8 and 2.7.2 (32-bits) on CentOS 5.8
  • Python 2.7.2 (64-bits) on FreeBSD 8.3
  • Python 2.4.4, 2.5.4, 2.6.6 and 2.7.1 (32-bits) on Windows XP sp3

What about comment:5 and comment:9? And are you planning more changes?

No, I'm not planning.

About comment:5, using datetime.utcfromtimestamp instead of to_datetime fixes the issue in comment:4.

It could make the usage clearer if we track the nature of the datetime object in the name (e.g. naive_yesterday instead of just yesterday).

yesterday is an aware datetime object….

(but you may have a better plan, so I let you "drive" in the end ;-) ).

Many good suggestions to me. Thanks ;-)

comment:13 by Jun Omae, 11 years ago

Resolution: fixed
Status: newclosed

Applied in [11415-11417]!

comment:14 by Jun Omae, 11 years ago

API Changes: modified (diff)
Owner: set to Jun Omae
Release Notes: modified (diff)

comment:15 by Remy Blank, 11 years ago

Thanks Jun for all these fixes! Who would have thought that time was so complicated ;)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Jun Omae.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Jun Omae 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.