#31520 closed New feature (wontfix)
ManifestStaticFilesStorage should not raise ValueError on missing file when manifest_strict=False
Reported by: | thenewguy | Owned by: | thenewguy |
---|---|---|---|
Component: | contrib.staticfiles | Version: | 3.0 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Unreviewed | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
I am using ManifestStaticFilesStorage and was under the impression that subclassing and setting manifest_strict=False
would allow pages to render if a static file is missing (perhaps due to a template typo) instead of raising a ValueError.
However, if the file is missing from disk a ValueError is raised.
I believe this is an oversight as it would be preferrable for a typo to cause a 404 instead of preventing a page from rendering.
Adding this try/except block lets the page render and the tests below pass. Otherwise they fail when the files do not exist.
if cache_name is None: if self.manifest_strict: raise ValueError("Missing staticfiles manifest entry for '%s'" % clean_name) try: hashed = self.hashed_name(name) except ValueError: hashed = name cache_name = self.clean_name(hashed)
This is what the reworked function looks like:
def stored_name(self, name): parsed_name = urlsplit(unquote(name)) clean_name = parsed_name.path.strip() hash_key = self.hash_key(clean_name) cache_name = self.hashed_files.get(hash_key) if cache_name is None: if self.manifest_strict: raise ValueError("Missing staticfiles manifest entry for '%s'" % clean_name) try: hashed = self.hashed_name(name) except ValueError: hashed = name cache_name = self.clean_name(hashed) unparsed_name = list(parsed_name) unparsed_name[2] = cache_name # Special casing for a @font-face hack, like url(myfont.eot?#iefix") # http://www.fontspring.com/blog/the-new-bulletproof-font-face-syntax if '?#' in name and not unparsed_name[3]: unparsed_name[2] += '?' return urlunsplit(unparsed_name)
And here are the tests:
from os.path import exists from django.conf import settings from django.templatetags.static import static from django.test import SimpleTestCase, override_settings @override_settings(STATIC_URL='/static/') class StaticResolveTest(SimpleTestCase): def test_existing_static_path_resolves(self): location = 'admin/js/vendor/jquery/jquery.js' path = join(settings.STATIC_ROOT, location) self.assertTrue(exists(path), 'Path "%s" did not exist' % path) static_location = static(location) static_location_parts = static_location.split('.') self.assertEqual(static_location_parts[0], '/static/admin/js/vendor/jquery/jquery') self.assertEqual(static_location_parts[-1], 'js') def test_missing_static_path_resolves(self): location = 'does-not-exist.txt' path = join(settings.STATIC_ROOT, location) self.assertFalse(exists(path), 'Path "%s" was not supposed to exist' % path) self.assertEqual(static(location), '/static/does-not-exist.txt') def test_served_static_response(self): location = 'admin/js/vendor/jquery/jquery.js' path = join(settings.STATIC_ROOT, location) self.assertTrue(exists(path), 'Path "%s" did not exist' % path) response = self.client.get(static(location)) self.assertEqual(response.status_code, 200) self.assertTrue(response.streaming) response_content = b''.join(response.streaming_content) with open(path, 'rb') as fp: disk_content = fp.read() self.assertEqual(response_content, disk_content) def test_missing_static_response(self): location = 'does-not-exist.txt' path = join(settings.STATIC_ROOT, location) self.assertFalse(exists(path), 'Path "%s" was not supposed to exist' % path) response = self.client.get(static(location)) self.assertEqual(response.status_code, 404)
Change History (13)
comment:1 by , 5 years ago
comment:2 by , 5 years ago
Needs tests: | set |
---|---|
Patch needs improvement: | set |
Triage Stage: | Unreviewed → Accepted |
Hi @thenewguy.
I'll accept this provisionally, since it looks correct on read through — though I didn't yet run the test cases.
Can you add your new tests to the PR (perhaps just as a first commit so it's easy to see those failing.)
The suggested patch causes the existing tests to fail staticfiles_tests.test_storage.TestCollectionManifestStorage.test_missing_entry
, so can you adjust that too?
Uncheck the flags when it's ready and I'll take a look.
Thanks!
comment:3 by , 5 years ago
Two things - maybe a different issue, but shouldn't we log an ERROR or at least WARNING that the file was missing from the manifest so that it can be handled? Since we would be hashing the files on every call it isn't free. TBH I think it probably makes more sense here not to hash at all and just return the unmodified name plus the error/warning message?
Edit:
The docs seem to indicate the behavior should be as I've described: This behavior can be disabled by subclassing ManifestStaticFilesStorage and setting the manifest_strict attribute to False – nonexistent paths will remain unchanged. (https://docs.djangoproject.com/en/3.0/ref/contrib/staticfiles/#manifeststaticfilesstorage)
Replacing
try: hashed = self.hashed_name(name) except ValueError: hashed = name cache_name = self.clean_name(hashed)
with
cache_name = self.clean_name(name)
would behave as currently documented and seems like the better way to handle it. In addition to adding log output at the ERROR level.
comment:4 by , 5 years ago
Clean pull request now that I have less going on around me. PR https://github.com/django/django/pull/12816
Will submit the fix and mark ready for review once these tests complete showing failure
comment:5 by , 5 years ago
Needs tests: | unset |
---|---|
Owner: | changed from | to
Patch needs improvement: | unset |
Status: | new → assigned |
comment:6 by , 5 years ago
Component: | Uncategorized → contrib.staticfiles |
---|
comment:7 by , 5 years ago
@Carlton Gibson
I left https://github.com/django/django/pull/12816 intact since it is what was originally discussed and I went ahead and submitted https://github.com/django/django/pull/12817 since I think it is the better fix. Let me know what you'd like me to do. Thanks!
comment:9 by , 4 years ago
Type: | Uncategorized → Bug |
---|
follow-up: 11 comment:10 by , 4 years ago
Has patch: | unset |
---|---|
Resolution: | → wontfix |
Status: | assigned → closed |
Triage Stage: | Accepted → Unreviewed |
Type: | Bug → New feature |
Hi Gordon.
Thanks for the effort here. On review, I don't think we can pull this in as a breaking change — it's just too well established:
- That
hashed_name()
raises for a missing file has been in place since CachedStaticFilesStorage was added for #15252. - That
hashed_name()
is used here was part of the design when `manifest_strict` was added for #24452.
They'll be too many people expecting this behaviour to just change it.
So, we could add a new feature file_exists_strict
or something, but on balance I don't thing that's worth the complication.
...it would be preferrable for a typo to cause a 404 instead of preventing a page from rendering.
Concluding I think I have to say that failing hard wins. At the very least it stops you sneaking that typo into production when you failed to notice that the referenced static file didn't load.
Alternative would be to subclass the storage to implement the more lax behaviour.
I hope that makes sense.
comment:11 by , 4 years ago
Replying to Carlton Gibson:
Hi Carlton,
Would you merge if this was behind a feature flag as you suggested?
A 500 error makes debugging difficult when templates can be updated outside of the main code release process - for example, tenant specific templates. The template developer typically does not have access to the traceback in this case. A 404 is easy to debug with a browser console but a 500 error doesn't give any detail to correct the problem.
comment:12 by , 4 years ago
I think it would need to be with an extra flag, yes.
Then my thought was it wasn't worth the effort, in fact that erroring hard was better. ... However... 😀
If you do in fact really need this, I'd add the extra flag in a subclass to begin (since the development version is the earliest it can land), then adjust the PR to add it and re-open this saying "Go on, it's actually useful". At that point I'd be inclined to accept it yes.
comment:13 by , 2 years ago
I'm picking up on this issue as I've found the existing behaviour to be problematic. When using the static
template tag, it's far too easy for a typo to be introduced that is hard to spot in development, but causes a 500 for the whole page in production.
I think this contrast between DEBUG
being True
or False
is too great, when in most cases Django should try to serve the page even if a single asset can't be served for whatever reason.
I understand this is tagged as wontfix
, but I wonder if a PR with the above file_exists_strict
flag would be acceptable? I'm happy to adapt the above code and open a new PR if so.
Created draft PR https://github.com/django/django/pull/12810