Opened 10 years ago

Closed 9 years ago

#4071 closed (fixed)

cache_page decorator sets wrong Cache-Control header

Reported by: Ionut Ciocirlan (xlotlu) <upstaked@…> Owned by: Tomáš Kopeček
Component: Core (Cache system) Version: master
Severity: Keywords: sprintsept14
Cc: permonik@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:


If trying to set cache_page with CUSTOM_SECONDS, the CACHE_MIDDLEWARE_SECONDS Cache-Control header will look like:(notice the underscore)

The first problem is that patch_cache_control changes existing values to underscores, but never changes them back, and this is what the patch addresses.

The second problem is that even with this fixed, the max-age will still be set (appended to the existing value) to the default CACHE_MIDDLEWARE_SECONDS, probably later on by the cache middleware.

Attachments (2)

django_cache.diff (495 bytes) - added by Ionut Ciocirlan (xlotlu) <upstaked@…> 10 years ago.
2django_cache.diff (566 bytes) - added by anonymous 10 years ago.

Download all attachments as: .zip

Change History (15)

Changed 10 years ago by Ionut Ciocirlan (xlotlu) <upstaked@…>

Attachment: django_cache.diff added

comment:1 Changed 10 years ago by Simon G. <dev@…>

Triage Stage: UnreviewedReady for checkin

Well spotted! Can you create a new ticket for the second issue - it's easier to manage for us.

comment:2 Changed 10 years ago by Malcolm Tredinnick

Triage Stage: Ready for checkinAccepted

Is this really fixing the whole problem? It looks like line 57 might also have a problem: if kwargs contains {'max_age': 100} , then line 57 wil put in a "max-age" key to the dictionary and line 58 will also convert any existing max_age key to "max-age", so it will appear twice.

Is there an error in this logic? If somebody can explain why I'm wrong, feel free to move back to "ready for checkin", but I'm not happy that it's currently fixing the real problem.

comment:3 Changed 10 years ago by Ionut Ciocirlan (xlotlu) <upstaked@…>

No, it's not fixing it all, as i mentioned in the initial comment; keys will be duplicated.

It however provides a fix when calling patch_cache_control multiple times with *different* keys, e.g: decorating the view with cache_control(must_revalidate=True), and getting the max_age set later by the cache middleware.

I think Simon G.'s suggestion to create another ticket is a good one, since (imho) patch_cache_control needs some design decisions and probably a major overhaul; properly fixing the second issue also needs changing the cache middleware (i.e.: have it not overwrite the max-age if the response already has it set) -- so maybe 2 tickets.

comment:4 Changed 10 years ago by njharman

I don't even see why code is bothering to convert existing keys to underscores. New patch attached that solves first problem better.

I wonder if the original intent was to only add keys if they didn't already exist. In effect making whoever set first wins. Might be indirect solution to 2nd problem as it looks like cache_page decorator is sets it values before cache manager middleware

Changed 10 years ago by anonymous

Attachment: 2django_cache.diff added

comment:5 Changed 10 years ago by njharman

I forgot to mention I couldn't find the unit test for this. I really don't understand django's test dir structure. If someone would point out where the unittest for django/utils/ is or should be it might be a grand thing cause I actually like writing unittests.

comment:6 Changed 9 years ago by Tomáš Kopeček

I've encountered this bug today and I've ended with the same solution as the second patch (removing conversions in dictitem()). It covers problem as is. The second part of the bug (cache_control value 'max-age' get rewritten by cache_middleware) is still the issue - I also vote for new ticket.

comment:7 Changed 9 years ago by anonymous

Cc: permonik@… added

comment:8 in reply to:  6 Changed 9 years ago by anonymous

Oh, I've forgotten one thing - in the documentation is said that cache_control takes precedence before cache_middleware. If there is some design decision about rewriting patch_cache_control leading to not changing actual behaviour for some time, then it would be helpful to mind it in documentation also (

comment:9 Changed 9 years ago by Tomáš Kopeček

Owner: changed from nobody to Tomáš Kopeček
Status: newassigned

Patch is sufficient and working.

comment:10 Changed 9 years ago by Tomáš Kopeček

Keywords: sprintsept14 added

comment:11 Changed 9 years ago by Tomáš Kopeček

Triage Stage: AcceptedReady for checkin

comment:12 Changed 9 years ago by Adrian Holovaty

I'm checking this in.

comment:13 Changed 9 years ago by Adrian Holovaty

Resolution: fixed
Status: assignedclosed

(In [6222]) Fixed #4071 -- Fixed bug in cache_page decorator, which was setting the wrong header. Thanks, Ionut Ciocirlan (xlotlu) and permon

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