Opened 14 months ago

Closed 14 months ago

Last modified 14 months ago

#22567 closed Bug (invalid)

FileSystemStorage.modified_time() is not timezone aware

Reported by: jaylett Owned by: nobody
Component: File uploads/storage Version: 1.6
Severity: Normal Keywords:
Cc: denis.cornehl@… Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I'm using the S3BotoStorage backend from django_storages, and collecstatic does not always upload new copies of static files. S3BotoStorage modified times come back in UTC (verified against Amazon's S3 console):

from django.contrib.staticfiles import finders, storage
f = list(finders.get_finders())
pss = list(f[0].list([]))
ps = pss[-1]

from django.contrib.staticfiles.storage import staticfiles_storage as sf
sf._setup()
tlm = sf.modified_time(ps[0])
slm = ps[1].modified_time(ps[0])
print tlm
print slm

gives:

2014-05-02 10:25:33
2014-05-02 09:57:57

However the local machine is in PDT (it's a Heroku dyno on an Amazon west coast instance somewhere):

>>> import os
>>> os.system('ls -l static/css')
total 8
-rw------- 1 u29977 29977 5838 2014-05-02 09:57 base.css
0
>>> os.system('date')
Fri May  2 11:39:28 PDT 2014
0

So the correct modified time in UTC for the local static/css/base.css is 2014-05-02 16:57:57, or several hours *after* the target file on S3 was last modified.

Because FileSystemStorage isn't timezone aware, the comparison for "newer than target" in collectstatic fails, and only updates that are more than seven hours (currently) after the previous successful update actually get deployed.

I'm pretty sure the actual problem is that datetime.datetime.fromtimestamp converts to localtime but returns a naive object. I *believe* that forcing it into UTC by using …fromtimestamp(since_epoch, django.utils.timezone.utc) would be appropriate (in the methods at the end of django/core/files/storage.py), but timezones are fiddly things and I don't even have the brainpower right now to figure out how to write a simple test for this that can work no matter which underlying TZ the machine is configured for.

Change History (7)

comment:1 follow-up: Changed 14 months ago by syphar

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Actually Heroku's dynos are set to UTC by default. You can change it by setting the environment, inside Django you see the timezone you define in your settings (even when you call os.system).
I just dug into it and I think django-storages doesn't even use the original modified-time of the file, it's just the upload-time in UTC (and it returns a naive datetime).

Timezone-awareness of the returned timestamps (change tz_info to the currently used timezone) would lead to collectstatic somehow having to compare an aware and naive timestamp (or breaking). Just expecting naive=UTC would be just wrong. Even when modified_time would be timezone-aware, django-storages would have to return an aware datetime too, or has to convert it to the current timezone.

But this still raised the question why the returned modified-times aren't timezone aware. I'll try to figure out if there is a valid reason.

comment:2 Changed 14 months ago by syphar

  • Cc denis.cornehl@… added

comment:3 in reply to: ↑ 1 Changed 14 months ago by jaylett

Replying to syphar:

Actually Heroku's dynos are set to UTC by default. You can change it by setting the environment, inside Django you see the timezone you define in your settings (even when you call os.system).

Ah, then that's it. Apologies for the confusion — Django's TIME_ZONE is set to Pacific for this app, so that's where the discrepancy is coming from.

I just dug into it and I think django-storages doesn't even use the original modified-time of the file, it's just the upload-time in UTC (and it returns a naive datetime).

S3 probably doesn't have a concept of modification time (because you don't modify objects in S3, you replace them), so creation time is broadly equivalent. It's naive, which isn't great, but it's in UTC when Django's ones honour TZ. At the least I feel it should be made explicit what the "correct" (current) behaviour is in the [custom storage backend docs](https://docs.djangoproject.com/en/dev/howto/custom-file-storage/) — probably in the [storage backend API docs](https://docs.djangoproject.com/en/dev/ref/files/storage/), I'm guessing).

Timezone-awareness of the returned timestamps (change tz_info to the currently used timezone) would lead to collectstatic somehow having to compare an aware and naive timestamp (or breaking). Just expecting naive=UTC would be just wrong.

Yeah, that's a problem and I think answers your final question — it would appear to be a BC break to make them timezone-aware, so probably needs new API methods and a deprecation policy?

Even when modified_time would be timezone-aware, django-storages would have to return an aware datetime too, or has to convert it to the current timezone.

I think this has convinced me that at least right now the more pressing issue is that django-storages' S3Boto backend (and likely others) doesn't honour settings.TIME_ZONE, and so this should be considered a bug against that and not Django.

comment:4 Changed 14 months ago by jaylett

There's [a bug filed against django-storages](https://bitbucket.org/david/django-storages/pull-request/83/converting-s3-last-modified-time-to-local) already, which references [a post to django-users](https://groups.google.com/forum/#!topic/django-users/pEIKxsHYN74) asking about how this all "should" work. The analysis there appears to be that the system timezone is in play, which doesn't appear to be the case.

comment:5 Changed 14 months ago by jaylett

And, erm, apologies for just remembering that this is trac, and doesn't use Markdown :-(

comment:6 follow-up: Changed 14 months ago by syphar

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

Thanks jaylett for figuring this out.

I think when one of the authors stated in the PR that this is a valid bug to django-storages we can see this ticket as closed.

The question why these datetimes are naive still is open and still valid, but is a general question.

comment:7 in reply to: ↑ 6 Changed 14 months ago by jaylett

Replying to syphar:

I think when one of the authors stated in the PR that this is a valid bug to django-storages we can see this ticket as closed.

+1

The question why these datetimes are naive still is open and still valid, but is a general question.

I think it'd be good to see current behaviour documented, still (since I can't see changing to TZ-aware being a fast process) and everyone has to integrate with FileSystemStorage for the purposes of collectstatic if nothing else.

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