[Django] #22557: staticfiles.json keeps deleted entries when collectstatic is run
Django
-------------------------+-------------------------------------------------
Reporter: | Owner: nobody
tedtieken | Status: new
Type: Bug | Version: 1.7-beta-2
Component: | Keywords: hashed_files, collectstatic,
contrib.staticfiles | manifest
Severity: Normal | Has patch: 0
Triage Stage: | UI/UX: 0
Unreviewed |
Easy pickings: 0 |
-------------------------+-------------------------------------------------
There is some surprising behavior when the new HashedFilesMixin is used
with the new ManifestFilesMixin.
When you run {{{manage.py collectstatic --clear}}}:
1) a copy of the old manifest.json is loaded from disk into memory at
{{{ManifestFilesMixin.hashed_files}}}. This happens at the very beginning
of collectstatic (when {{{ManifestFilesMixin.__init__}}} is called, which
happens when the storage is initialized, which happens during
{{{Command.__init__}}})
2) the old manifest.json is deleted (which would lead most people to
believe the old manifest information is deleted as well)
3) new files are added to the {{{ManifestFilesMixin.hashed_files}}} dict,
and updated files get their records updated. But, this is building on top
of the last version of manifest.json. Keys for deleted files are never
removed and the deleted file mappings persist in the new manifest.json
which gets written back to disk at the end of collectstatic's post_process
phase.
There are at least a few problems caused by the current setup:
1) It can lead to hard-to-find asset bugs. If you rename the mypage.css
file to mynewpage.css, but forget to update all of the templates, the {{{
{% static %} }}} template tag will happily serve you the stale cache
record. This could lead to submarine bugs that aren't caught until
production as few sites have full enough test suites to catch missing css.
And the issue could get buried even deeper if you concatenate your CSS.
2) staticfiles.json has a memory leak. New files get added, but deleted
files never get deleted unless you physically delete the staticfiles.json
record.
3) If you write code that does anything with
{{{ManifestFilesMixin.hashed_files}}}, you can't trust the cache and you
have no way of cleaning it up when you find cache misses. (how I found
the bug) I'm writing a css concatenator and because staticfiles.json never
gets cleared, I have to keep a separate manifest for my mappings.
This problem may also exist when you use the CachedFilesMixin. I haven't
tested that, but a quick read over the code suggests similar behavior
exists there. 2 is less of an issue because caches are built to drop data
over time. 3 is less of an issue because you can easily delete stale cache
keys from anywhere in your application.
I see a few ways to fix the problem, and I'm sure there are others:
Option 1: clear the hashed files cache when collectstatic is run with
--clear.
If the code in
{{{django.contrib.staticfiles.management.commands.collectstatic}}} on
lines 88-89 is changed from:
{{{
if self.clear:
self.clear_dir('')
}}}
to
{{{
if self.clear:
self.clear_dir('')
if hasattr(self.storage, "hashed_files"):
self.storage.hashed_files.clear()
}}}
This is pretty simple when you're using a manifest but, when a cache
backend is used without a separate cache defined for staticfiles, this
looks like it will clear _THE ENTIRE_ default cache.
Option 2: Move the cleanup work into the StaticFilesStorage, add a call
from collectstatic.
If we added something like an {{{on_collectstatic}}} method to staticfiles
storages and call it during collectstatic, we could have better
encapsulation of this kind of cleanup that is also more futureproof.
in {{{django.contrib.staticfiles.management.commands.collectstatic}}} add
the following somewhere before post_process (~ line 107):
{{{
if hasattr(self.storage, "on_collectstatic"):
self.storage.on_collectstatic(self)
}}}
Which would allow us to add to the ManifestFilesMixin:
{{{
def on_collectstatic(command_object, *args, **kwargs):
if command_object.clear:
self.hashed_files = OrderedDict()
}}}
This lets us to do manifest files related cleanup work in the
ManifestFilesMixin, and anyone who needs to can do custom cleanup of their
own storage.
Option 3: add a warning to the documentation,
Of the options, I like some version of 2 the best.
I don't think this is necessarily a release blocker, but as soon as people
start using the ManifestFilesMixin they're going to start bumping into
this whether they realize it or not.
--
Ticket URL: <https://code.djangoproject.com/ticket/22557>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: tedtieken | Owner: nobody
Type: Bug | Status: new
Component: contrib.staticfiles | Version:
Severity: Normal | 1.7-beta-2
Keywords: hashed_files, | Resolution:
collectstatic, manifest | Triage Stage:
Has patch: 1 | Unreviewed
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by tedtieken):
* needs_better_patch: => 0
* has_patch: 0 => 1
* needs_tests: => 0
* needs_docs: => 0
Comment:
Patch is in this pull request:
https://github.com/django/django/pull/2627
--
Ticket URL: <https://code.djangoproject.com/ticket/22557#comment:1>
Django
Reporter: tedtieken | Owner: nobody
Type: Bug | Status: new
Component: contrib.staticfiles | Version:
Keywords: hashed_files, | Resolution:
collectstatic, manifest | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by timo):
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
Comment:
The release blocker flag is typically warranted for bugs in new features.
I have looked at this a little, but it would be great if Jannis could do a
final review.
--
Ticket URL: <https://code.djangoproject.com/ticket/22557#comment:2>
Django
Reporter: tedtieken | Owner: apollo13
Type: Bug | Status: assigned
Component: contrib.staticfiles | Version:
Severity: Release blocker | 1.7-beta-2
Keywords: hashed_files, | Resolution:
collectstatic, manifest | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => apollo13
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/22557#comment:3>
Django
Reporter: tedtieken | Owner: syphar
Type: Bug | Status: assigned
Component: contrib.staticfiles | Version:
Severity: Release blocker | 1.7-beta-2
Keywords: hashed_files, | Resolution:
collectstatic, manifest | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: apollo13 => syphar
--
Ticket URL: <https://code.djangoproject.com/ticket/22557#comment:4>
Django
Reporter: tedtieken | Owner: syphar
Type: Bug | Status: assigned
Component: contrib.staticfiles | Version:
Severity: Release blocker | 1.7-beta-2
Keywords: hashed_files, | Resolution:
collectstatic, manifest | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by syphar):
updated pull-request: https://github.com/django/django/pull/2662
--
Ticket URL: <https://code.djangoproject.com/ticket/22557#comment:5>
Django
Reporter: tedtieken | Owner: syphar
Component: contrib.staticfiles | Version:
Severity: Release blocker | 1.7-beta-2
collectstatic, manifest | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"3bec38888f6f4ee9245b004fcb9fe15b35cef469"]:
{{{
#!CommitTicketReference repository=""
revision="3bec38888f6f4ee9245b004fcb9fe15b35cef469"
Fixed #22557 -- ManifestStaticFilesStorage did not cleanup deleted files.
When using ManifestStaticFilesStorage, deleted static files would be
correctly cleaned up by "collectstatic --clear", but the manifest file
would still contain the stale entries.
Thanks to tedtieken for the report
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22557#comment:6>
Django
Reporter: tedtieken | Owner: syphar
Type: Bug | Status: closed
Component: contrib.staticfiles | Version:
Severity: Release blocker | 1.7-beta-2
Keywords: hashed_files, | Resolution: fixed
collectstatic, manifest | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Florian Apolloner <florian@…>):
In [changeset:"0007a43198670a1716bc5ff2d0a659daa4c7cf63"]:
{{{
#!CommitTicketReference repository=""
revision="0007a43198670a1716bc5ff2d0a659daa4c7cf63"
[1.7.X] Fixed #22557 -- ManifestStaticFilesStorage did not cleanup deleted
files.
When using ManifestStaticFilesStorage, deleted static files would be
correctly cleaned up by "collectstatic --clear", but the manifest file
would still contain the stale entries.
Thanks to tedtieken for the report
Backport of 3bec38888f6f4ee9245b004fcb9fe15b35cef469 from master.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22557#comment:7>