Opened 14 years ago
Closed 13 years ago
#18260 closed Bug (fixed)
Cache tag doesn't handle filter expresssions
| Reported by: | FunkyBob | Owned by: | regebro |
|---|---|---|---|
| Component: | Template system | Version: | dev |
| Severity: | Normal | Keywords: | cache |
| Cc: | charette.s@…, bmispelon@… | Triage Stage: | Accepted |
| Has patch: | yes | Needs documentation: | yes |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | yes | UI/UX: | no |
Description
I found the cache tag didn't well handle when I passed, for instance:
{% cache ..... request.GET.foo %}
As it would raise an exception if GETfoo wasn't set... so I tried passing a default:
{% cache ..... request.GET.foo|default:"bar" %}
Only to see it not only didn't fix the issue, but didn't appear to be parsing filter expressions, either.
I've updated the cache tag to use parser.compile_filter for all its arguments.
Attachments (5)
Change History (13)
by , 14 years ago
| Attachment: | cachetag.diff added |
|---|
by , 14 years ago
| Attachment: | cachetag.2.diff added |
|---|
comment:1 by , 14 years ago
| Triage Stage: | Unreviewed → Ready for checkin |
|---|
Looks good to me. Nice job FunkyBob
comment:2 by , 14 years ago
| Cc: | added |
|---|---|
| Needs tests: | set |
| Triage Stage: | Ready for checkin → Accepted |
IMHO this should be tested.
by , 13 years ago
| Attachment: | cachetag4.diff added |
|---|
comment:3 by , 13 years ago
I was looking through the easy pickings and thought I'd write a regression test for this one. Whilst working on the test I discovered that because the cache tag now does parser.compile_filter for all its arguments, it was converting the fragment name into a filter expression. This caused the cache tag to treat the fragment name as a variable and attempt to look it up in its context. The lookup would return nothing, and so the resulting cache key would be missing the fragment name. This would then cause the wrong fragments to be returned from the cache in certain cases.
I've attached a new patch (which doesn't treat the fragment name as a filter expression) plus the regression tests.
comment:4 by , 13 years ago
| Needs documentation: | set |
|---|---|
| Needs tests: | unset |
You can drop the u':' at line 27 since your using module wide unicode literals: from __future__ import unicode_literals.
Maybe we should add a release note for that? All tests pass on Python 2.7.3rc1 sqlite3.
by , 13 years ago
| Attachment: | cachetag5.diff added |
|---|
Removed trailing whitespace and a unicode literal
comment:5 by , 13 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
comment:6 by , 13 years ago
I "accidentally" fixed this while working on a different bug.
I have a pull request for this and #6271 at https://github.com/django/django/pull/751
comment:7 by , 13 years ago
| Cc: | added |
|---|
comment:8 by , 13 years ago
| Resolution: | → fixed |
|---|---|
| Status: | assigned → closed |
Changed to using token.split_contents() instead of token.contents.split() [thanks, brad!]