ManifestStaticFilesStorage should not raise ValueError on missing file when manifest_strict=False

[thenewguy]

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)

[thenewguy]

Created draft PR ​https://github.com/django/django/pull/12810

[Carlton Gibson]

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!

[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?

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.

[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

[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!

[thenewguy]

Doesn't look like I ever said I think this is ready

[Carlton Gibson]

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.

[thenewguy]

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.

[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.

[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.