Opened 10 years ago

Closed 8 years ago

#22972 closed Bug (duplicate)

HashedFilesMixin.patterns should limit URL matches to their respective filetypes

Reported by: alex.ehlke@… Owned by: aehlke
Component: contrib.staticfiles Version: dev
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 (14)

comment:1 by anonymous, 10 years ago

Summary: HashedFilesMixinHashedFilesMixin.patterns should limit URL matches to their respective filetypes

comment:2 by aehlke, 10 years ago

Owner: changed from nobody to aehlke
Status: newassigned

comment:3 by aehlke, 10 years ago

I'm preparing a test case and patch.

comment:4 by aehlke, 10 years ago

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.

Version 0, edited 10 years ago by aehlke (next)

comment:5 by aehlke, 10 years ago

Has patch: set

comment:6 by aehlke, 10 years ago

Type: UncategorizedBug

comment:7 by aehlke, 10 years ago

Version: 1.7-rc-1master

comment:8 by aehlke, 10 years ago

Resolution: fixed
Status: assignedclosed

comment:9 by aehlke, 10 years ago

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 by Tim Graham, 10 years ago

Resolution: fixed
Status: closednew

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

comment:11 by aehlke, 10 years ago

timo: Ah ok, sorry about that. Thanks.

comment:12 by Tim Graham, 10 years ago

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

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

comment:13 by James Aylett, 8 years ago

Is this the same as #19670?

in reply to:  13 comment:14 by Claude Paroz, 8 years ago

Resolution: duplicate
Status: newclosed

Replying to jaylett:

Is this the same as #19670?

I think so.

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