Opened 9 years ago

Closed 5 years ago

#3415 closed (fixed)

django allows invalid TIME_ZONE in settings.py

Reported by: forest@… Owned by: nobody
Component: Uncategorized Version: master
Severity: Keywords: datetime TIME_ZONE settings.py
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

django didn't complain at all when my TIME_ZONE was "America/New York" instead of "America/New_York". It'd be nice if django at least printed a warning somewhere. It would have saved me some time tracking down the issue (which appeared to be a bug, since datetime.datetime.now() always returned the wrong time, but only in manage.py shell, not in a normal python shell).

-Forest

Attachments (2)

t3415-variant1-check-tzname.diff (738 bytes) - added by seveas 6 years ago.
Variant one: check time.tzname
t3415-variant2-check-zoneinfo.diff (836 bytes) - added by seveas 6 years ago.
Variant 2: check /usr/share/zoneinfo

Download all attachments as: .zip

Change History (18)

comment:1 Changed 9 years ago by Michael Radziej <mir@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

Can you propose how to check the TIME_ZONE for correctness?

comment:2 Changed 9 years ago by forest@…

Well, the most obvious solution is to hard code a list of allowed values.

A more simple solution:

It seems that datetime.datetime.now() returns a UTC time when TIME_ZONE is set incorrectly (at least on my system). So, if the difference between datetime.datetime.now() and datetime.datetime.utcnow() is negligible, it's a safe bet that something is wrong with TIME_ZONE. I would just print a warning, not bring down the server, but a warning would be nice.

-Forest

comment:3 Changed 9 years ago by forest@…

Regarding comparing datetime.datetime.now() with datetime.datetime.utcnow():

Obviously, if TIME_ZONE == 'UTC' or an equivalent, you wouldn't print the warning. It does vastly reduce the number of TIME_ZONE values you have to explicitly check for (and thereby reduces the maintenance overhead that comes with maintaining a list of valid TIME_ZONE values).

Might be easier to just maintain the list of valid values -- I'll leave that to the django folks.

-Forest

comment:4 Changed 9 years ago by mtredinnick

  • Triage Stage changed from Design decision needed to Accepted

-1 on maintaining a list of values, because it would repeat information that is already known to the OS (and the list would be different for different operating systems).

Doing a sanity check against UTC for cases when we are not expecting to be in UTC is a reasonable idea. However, it may be fiddly because you can't raise errors for London and related time zones when they are not on summer time.

The "design decision" is not to keep the list, but if sanity checking is possible, then that seems reasonable. We may discover that sanity checking is really hard to get right, though.

comment:5 Changed 8 years ago by Winston Lee <lee.winston@…>

I think an easier method would be to keep a list of Timezones that would return ('UTC', 'UTC') when time.tzname is called.

I'm thinking something like this in Settings.init

if hasattr(time, 'tzset'):
    # Move the time zone info into os.environ. See ticket #2315 for why
    # we don't do this unconditionally (breaks Windows).
    os.environ['TZ'] = self.TIME_ZONE
    time.tzset()
    if time.tzname == ('UTC', 'UTC') and not self.UTC_TZ.__contains__(self.TIME_ZONE):
        # time.tzname may have fallen back to UTC. 
        # If the TIME_ZONE is indeed a UTC TZ then it should be added to self.UTC_TZ tuple.
        warnings.warn("Your TIME_ZONE of %s may have been incorrectly set. UTC will be used.' % (self.TIME_ZONE))

Define in Settings class (or somewhere more appropriate)

self.UTC_TZ = ('UTC', 'ETC/UTC')

Note that I'm not running Django on this Mac so I can't check/test the code. I have no idea what would go into self.UTC_TZ either. :)

Test:

>>> os.environ['TZ'] = 'Australia/Melbourne'
>>> time.tzset()
>>> time.tzname == ('UTC', 'UTC') and not UTC_TZ.__contains__('Australia/Melbourne')
False
>>> os.environ['TZ'] = 'UTC'
>>> time.tzset()
>>> time.tzname == ('UTC', 'UTC') and not UTC_TZ.__contains__('UTC')                
False
>>> os.environ['TZ'] = 'abc'
>>> time.tzset()
>>> time.tzname == ('UTC', 'UTC') and not UTC_TZ.__contains__('abc')   
True

comment:6 Changed 8 years ago by tsal <tsal@…>

Is the version really relative to this? I haven't tested this in SVN, but I will when I get a chance. If it's screwy in SVN, I'll see about turning Winston's suggestions into a patch of some sort.

comment:7 Changed 8 years ago by mtredinnick

  • Version changed from 0.95 to SVN

Winston's idea is not a bad one. However, the implementation should be a bit more careful. Check the values before trying to change the timezone and then again afterwards. Basically, I would not want a warning emitted for whose machine was set to a London timezone (which isn't UTC, because UTC doesn't have daylight savings), or any equivalent locale.

Also, emitting a warning is not correct. If we cannot effectively change the timezone, it should be an error. The current behaviour is like emitting a warning that the aircraft is missing a wing and then letting the pilot try to take off anyway.

This patch will need a lot of testing on different platforms in different configurations, so it may take a while to get right, but Winston's approach is the best idea I've seen yet for trying to get this correct without having to maintain a massive whitelist.

comment:8 Changed 7 years ago by Leo

Is including pytz as a library with Django completely out of the question?

comment:9 Changed 7 years ago by mtredinnick

Yes. It's way overkill to fix this patch.

comment:10 Changed 7 years ago by mtredinnick

s/patch/bug/

comment:11 Changed 7 years ago by mtredinnick

  • Triage Stage changed from Accepted to Someday/Maybe

Changed 6 years ago by seveas

Variant one: check time.tzname

Changed 6 years ago by seveas

Variant 2: check /usr/share/zoneinfo

comment:12 Changed 6 years ago by seveas

  • Has patch set

Attached are two patched: variant one implements Winston Lee's idea, which is nice but cannot be used to check the value before mangling the environment. Patch two checks whether the timezone is available in /usr/share/zoneinfo. This can be done before mangling the environment. Both patches do not work on windows, where the whole environment mangling is skipped anyway.

I have no idea how to create unittests/doctests for this, as there is afaik no way to test settings handling in django's test framework (correct me if I'm wrong please, pointers to existing tests would be helpful too). I have tested both variants manually. The timezones 'UTC' and 'America/Chicago' are seen as valid, whereas 'America/Chicagoops' was rejected.

comment:13 Changed 6 years ago by seveas

Of course, since the patches don't overlap/conflict it is also possible to apply both.

comment:14 Changed 6 years ago by seveas

  • Triage Stage changed from Someday/Maybe to Accepted

Setting stage back to accepted as it now has a patch.

comment:15 Changed 5 years ago by adamnelson

  • Triage Stage changed from Accepted to Ready for checkin

Has a patch and should be ready for checkin.

comment:16 Changed 5 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from new to closed

(In [13722]) When possible, sanity check the timezone setting to see if the Unix-like
box it's running on supports that setting. If checking isn't possible,
we skip it. Patch from seveas. Thanks.

Fixed #3415.

Note: See TracTickets for help on using tickets.
Back to Top