Opened 11 years ago

Closed 8 years ago

#3415 closed (fixed)

django allows invalid TIME_ZONE in

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


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 always returned the wrong time, but only in shell, not in a normal python shell).


Attachments (2)

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

Download all attachments as: .zip

Change History (18)

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

Triage Stage: UnreviewedDesign decision needed

Can you propose how to check the TIME_ZONE for correctness?

comment:2 Changed 11 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 returns a UTC time when TIME_ZONE is set incorrectly (at least on my system). So, if the difference between 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.


comment:3 Changed 11 years ago by forest@…

Regarding comparing 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.


comment:4 Changed 11 years ago by Malcolm Tredinnick

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 Changed 11 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
    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. :)


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

comment:6 Changed 11 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 11 years ago by Malcolm Tredinnick

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 Changed 10 years ago by Leo Shklovskii

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

comment:9 Changed 10 years ago by Malcolm Tredinnick

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

comment:10 Changed 10 years ago by Malcolm Tredinnick


comment:11 Changed 10 years ago by Malcolm Tredinnick

Triage Stage: AcceptedSomeday/Maybe

Changed 9 years ago by Dennis Kaarsemaker

Variant one: check time.tzname

Changed 9 years ago by Dennis Kaarsemaker

Variant 2: check /usr/share/zoneinfo

comment:12 Changed 9 years ago by Dennis Kaarsemaker

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

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

comment:14 Changed 9 years ago by Dennis Kaarsemaker

Triage Stage: Someday/MaybeAccepted

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

comment:15 Changed 8 years ago by Adam Nelson

Triage Stage: AcceptedReady for checkin

Has a patch and should be ready for checkin.

comment:16 Changed 8 years ago by Malcolm Tredinnick

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