Opened 5 years ago

Closed 4 years ago

Last modified 2 years ago

#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:2 by Carlton Gibson, 5 years ago

Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

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 thenewguy, 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.

Last edited 5 years ago by thenewguy (previous) (diff)

comment:4 by thenewguy, 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 thenewguy, 5 years ago

Needs tests: unset
Owner: changed from nobody to thenewguy
Patch needs improvement: unset
Status: newassigned

comment:6 by thenewguy, 5 years ago

Component: Uncategorizedcontrib.staticfiles

comment:7 by thenewguy, 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:8 by thenewguy, 4 years ago

Has patch: set

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

comment:9 by thenewguy, 4 years ago

Type: UncategorizedBug

comment:10 by Carlton Gibson, 4 years ago

Has patch: unset
Resolution: wontfix
Status: assignedclosed
Triage Stage: AcceptedUnreviewed
Type: BugNew 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:

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.

Last edited 4 years ago by Carlton Gibson (previous) (diff)

in reply to:  10 comment:11 by thenewguy, 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 Carlton Gibson, 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 Sym Roe, 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.

Note: See TracTickets for help on using tickets.
Back to Top