[Django] #22567: FileSystemStorage.modified_time() is not timezone aware
Django
--------------------------------------+--------------------
Reporter: jaylett | Owner: nobody
Type: Bug | Status: new
Component: File uploads/storage | Version: 1.6
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22567>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: jaylett | Owner: nobody
Type: Bug | Status: new
uploads/storage | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by syphar):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22567#comment:1>
Django
Reporter: jaylett | Owner: nobody
Type: Bug | Status: new
uploads/storage | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by syphar):
* cc: denis.cornehl@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/22567#comment:2>
Django
Reporter: jaylett | Owner: nobody
Type: Bug | Status: new
uploads/storage | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jaylett):
Replying to [comment:1 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22567#comment:3>
Django
Reporter: jaylett | Owner: nobody
Type: Bug | Status: new
uploads/storage | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22567#comment:4>
Django
Reporter: jaylett | Owner: nobody
Type: Bug | Status: new
uploads/storage | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jaylett):
And, erm, apologies for just remembering that this is trac, and doesn't
use Markdown :-(
--
Ticket URL: <https://code.djangoproject.com/ticket/22567#comment:5>
Django
Reporter: jaylett | Owner: nobody
Type: Bug | Status: closed
Component: File | Version: 1.6
uploads/storage | Resolution: invalid
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by syphar):
* status: new => closed
* resolution: => invalid
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22567#comment:6>
Django
Reporter: jaylett | Owner: nobody
Type: Bug | Status: closed
Component: File | Version: 1.6
uploads/storage | Resolution: invalid
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jaylett):
Replying to [comment:6 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22567#comment:7>