Opened 10 months ago

Last modified 9 months ago

#22972 new Bug

HashedFilesMixin.patterns should limit URL matches to their respective filetypes

Reported by: alex.ehlke@… Owned by: aehlke
Component: contrib.staticfiles Version: master
Severity: Normal Keywords: HashedFilesMixin CachedFilesMixin staticfiles
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

HashedFilesMixin contains a patterns property which maps file extensions to regex patterns. This works great for the default case that Django ships with "*.css" as the only filetype, but results in surprising behavior once extended with multiple filetypes. I would expect the regex patterns to only apply to the filetype they're categorized under, but once I added "*.js" to patterns (via subclassing), the CSS rules also applied to my JS files.

My use-case of extending this is to add URL rewriting to JS source map references, e.g. //# sourceMappingURL=foo.js.map which can appear at the end of JS files. I need to be able to rewrite these URLs in the same way that Django's staticfiles can rewrite URLs in CSS, for e.g. adding hashes to filenames.

Once I tried extending patterns with this:

    patterns = HashedFilesMixin.patterns + (
        ("*.js", (
            (r"""(//# sourceMappingURL=(\s*))""", """//# sourceMappingURL=%s"""),
        )),
    )

The surprising and broken behavior was that Django tried to rewrite "URLs" for this JS (from Backbone.js): this.loadUrl(window.location.hash), which matched the CSS pattern for rewriting url(foo) because the regex patterns are case-insensitive.

This also applies to the previous CachedFilesMixin from before 1.7.

Please let me know if a fix for this would be accepted, and I'll put together a test case and patch.

Change History (12)

comment:1 Changed 10 months ago by anonymous

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from HashedFilesMixin to HashedFilesMixin.patterns should limit URL matches to their respective filetypes

comment:2 Changed 10 months ago by aehlke

  • Owner changed from nobody to aehlke
  • Status changed from new to assigned

comment:3 Changed 10 months ago by aehlke

I'm preparing a test case and patch.

comment:4 Changed 10 months ago by aehlke

I've submitted a pull request with test coverage (a test which fails before applying the patch) and patch for the fix. Please let me know if anything needs changing.

https://github.com/django/django/pull/2895

Last edited 10 months ago by aehlke (previous) (diff)

comment:5 Changed 10 months ago by aehlke

  • Has patch set

comment:6 Changed 10 months ago by aehlke

  • Type changed from Uncategorized to Bug

comment:7 Changed 10 months ago by aehlke

  • Version changed from 1.7-rc-1 to master

comment:8 Changed 10 months ago by aehlke

  • Resolution set to fixed
  • Status changed from assigned to closed

comment:9 Changed 10 months ago by aehlke

This patterns property is also documented in the Django docs, which I think adds priority to getting this fixed as it makes it sound like something you can extend (where in current versions you cannot actually extend without subtly broken behavior.)

comment:10 Changed 10 months ago by timo

  • Resolution fixed deleted
  • Status changed from closed to new

A ticket shouldn't be closed until the fix is committed to Django.

comment:11 Changed 10 months ago by aehlke

timo: Ah ok, sorry about that. Thanks.

comment:12 Changed 9 months ago by timgraham

  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

I left comments for improvement on the PR. Please uncheck "Patch needs improvement" when you update it, thanks.

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