Opened 17 years ago

Closed 14 years ago

#3415 closed (fixed)

django allows invalid TIME_ZONE in settings.py

Reported by: forest@… Owned by: nobody
Component: Uncategorized Version: dev
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: no UI/UX: no

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 Dennis Kaarsemaker 15 years ago.
Variant one: check time.tzname
t3415-variant2-check-zoneinfo.diff (836 bytes ) - added by Dennis Kaarsemaker 15 years ago.
Variant 2: check /usr/share/zoneinfo

Download all attachments as: .zip

Change History (18)

comment:1 by Michael Radziej <mir@…>, 17 years ago

Triage Stage: UnreviewedDesign decision needed

Can you propose how to check the TIME_ZONE for correctness?

comment:2 by forest@…, 17 years ago

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 by forest@…, 17 years ago

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 by Malcolm Tredinnick, 17 years ago

Triage Stage: Design decision neededAccepted

-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 by Winston Lee <lee.winston@…>, 17 years ago

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 by tsal <tsal@…>, 17 years ago

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 by Malcolm Tredinnick, 17 years ago

Version: 0.95SVN

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 by Leo Shklovskii, 16 years ago

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

comment:9 by Malcolm Tredinnick, 16 years ago

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

comment:10 by Malcolm Tredinnick, 16 years ago

s/patch/bug/

comment:11 by Malcolm Tredinnick, 16 years ago

Triage Stage: AcceptedSomeday/Maybe

by Dennis Kaarsemaker, 15 years ago

Variant one: check time.tzname

by Dennis Kaarsemaker, 15 years ago

Variant 2: check /usr/share/zoneinfo

comment:12 by Dennis Kaarsemaker, 15 years ago

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 by Dennis Kaarsemaker, 15 years ago

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

comment:14 by Dennis Kaarsemaker, 15 years ago

Triage Stage: Someday/MaybeAccepted

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

comment:15 by Adam Nelson, 14 years ago

Triage Stage: AcceptedReady for checkin

Has a patch and should be ready for checkin.

comment:16 by Malcolm Tredinnick, 14 years ago

Resolution: fixed
Status: newclosed

(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