Opened 13 years ago

Closed 12 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)

cachetag.diff (1.6 KB ) - added by anonymous 13 years ago.
cachetag.2.diff (1.6 KB ) - added by FunkyBob 13 years ago.
Changed to using token.split_contents() instead of token.contents.split() [thanks, brad!]
cachetag.3.diff (2.1 KB ) - added by FunkyBob 13 years ago.
Fixed error message
cachetag4.diff (3.3 KB ) - added by wkeeling 12 years ago.
cachetag5.diff (3.3 KB ) - added by Simon Charette 12 years ago.
Removed trailing whitespace and a unicode literal

Download all attachments as: .zip

Change History (13)

by anonymous, 13 years ago

Attachment: cachetag.diff added

by FunkyBob, 13 years ago

Attachment: cachetag.2.diff added

Changed to using token.split_contents() instead of token.contents.split() [thanks, brad!]

by FunkyBob, 13 years ago

Attachment: cachetag.3.diff added

Fixed error message

comment:1 by Bradley Ayers, 13 years ago

Triage Stage: UnreviewedReady for checkin

Looks good to me. Nice job FunkyBob

comment:2 by Simon Charette, 13 years ago

Cc: charette.s@… added
Needs tests: set
Triage Stage: Ready for checkinAccepted

IMHO this should be tested.

by wkeeling, 12 years ago

Attachment: cachetag4.diff added

comment:3 by wkeeling, 12 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 Simon Charette, 12 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? Running the full test suite against the patch ATM.

Version 0, edited 12 years ago by Simon Charette (next)

by Simon Charette, 12 years ago

Attachment: cachetag5.diff added

Removed trailing whitespace and a unicode literal

comment:5 by regebro, 12 years ago

Owner: changed from nobody to regebro
Status: newassigned

comment:6 by Baptiste Mispelon, 12 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 Baptiste Mispelon, 12 years ago

Cc: bmispelon@… added

comment:8 by Baptiste Mispelon <bmispelon@…>, 12 years ago

Resolution: fixed
Status: assignedclosed

In 069280a6893d0f496c8f15922807939c68459ec3:

Used token.split_contents() for tokenisation in template tags accepting variables.

Fixed #6271, #18260.

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