[Django] #31520: ManifestStaticFilesStorage should not raise ValueError on missing file when manifest_strict=False
Django
manifest_strict=False
-----------------------------------------+------------------------
Reporter: thenewguy | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 3.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
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)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31520>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
manifest_strict=False
Reporter: thenewguy | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 3.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by thenewguy):
Created draft PR https://github.com/django/django/pull/12810
--
Ticket URL: <https://code.djangoproject.com/ticket/31520#comment:1>
Django
manifest_strict=False
Reporter: thenewguy | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 3.0
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
* stage: Unreviewed => Accepted
Comment:
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!
--
Ticket URL: <https://code.djangoproject.com/ticket/31520#comment:2>
Django
manifest_strict=False
Reporter: thenewguy | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 3.0
Keywords: | Triage Stage: Accepted
Comment (by thenewguy):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/31520#comment:3>
Django
manifest_strict=False
Reporter: thenewguy | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 3.0
Keywords: | Triage Stage: Accepted
Comment (by thenewguy):
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
--
Ticket URL: <https://code.djangoproject.com/ticket/31520#comment:4>
Django
manifest_strict=False
Reporter: thenewguy | Owner: thenewguy
Type: Uncategorized | Status: assigned
Component: Uncategorized | Version: 3.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by thenewguy):
* owner: nobody => thenewguy
* needs_better_patch: 1 => 0
* status: new => assigned
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/31520#comment:5>
Django
manifest_strict=False
Reporter: thenewguy | Owner: thenewguy
Type: Uncategorized | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by thenewguy):
* component: Uncategorized => contrib.staticfiles
--
Ticket URL: <https://code.djangoproject.com/ticket/31520#comment:6>
Django
manifest_strict=False
Reporter: thenewguy | Owner: thenewguy
Type: Uncategorized | Status: assigned
Component: contrib.staticfiles | Version: 3.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by thenewguy):
@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!
--
Ticket URL: <https://code.djangoproject.com/ticket/31520#comment:7>
Django
manifest_strict=False
Reporter: thenewguy | Owner: thenewguy
Type: Uncategorized | Status: assigned
Component: contrib.staticfiles | Version: 3.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by thenewguy):
* has_patch: 0 => 1
Comment:
Doesn't look like I ever said I think this is ready
--
Ticket URL: <https://code.djangoproject.com/ticket/31520#comment:8>
Django
manifest_strict=False
Reporter: thenewguy | Owner: thenewguy
Component: contrib.staticfiles | Version: 3.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by thenewguy):
* type: Uncategorized => Bug
--
Ticket URL: <https://code.djangoproject.com/ticket/31520#comment:9>
Django
manifest_strict=False
Reporter: thenewguy | Owner: thenewguy
Component: contrib.staticfiles | Version: 3.0
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* status: assigned => closed
* type: Bug => New feature
* has_patch: 1 => 0
* resolution: => wontfix
* stage: Accepted => Unreviewed
Comment:
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
[https://github.com/django/django/commit/1d32bdd3c9586ff10d0799264105850fa7e3f512
#diff-c7242dedd7c93b857a668acec1e310feR65-R69 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/31520#comment:10>
Django
manifest_strict=False
Reporter: thenewguy | Owner: thenewguy
Type: New feature | Status: closed
Component: contrib.staticfiles | Version: 3.0
Severity: Normal | Resolution: wontfix
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by thenewguy):
Replying to [comment:10 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/31520#comment:11>
Django
manifest_strict=False
Reporter: thenewguy | Owner: thenewguy
Type: New feature | Status: closed
Component: contrib.staticfiles | Version: 3.0
Severity: Normal | Resolution: wontfix
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/31520#comment:12>
Django
manifest_strict=False
Reporter: thenewguy | Owner: thenewguy
Type: New feature | Status: closed
Component: contrib.staticfiles | Version: 3.0
Severity: Normal | Resolution: wontfix
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Sym Roe):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/31520#comment:13>