Opened 17 years ago

Closed 17 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: dev
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: no UI/UX: no

Description

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

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@…> 17 years ago.
2django_cache.diff (566 bytes ) - added by anonymous 17 years ago.

Download all attachments as: .zip

Change History (15)

by Ionut Ciocirlan (xlotlu) <upstaked@…>, 17 years ago

Attachment: django_cache.diff added

comment:1 by Simon G. <dev@…>, 17 years ago

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 by Malcolm Tredinnick, 17 years ago

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 by Ionut Ciocirlan (xlotlu) <upstaked@…>, 17 years ago

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 by njharman, 17 years ago

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

by anonymous, 17 years ago

Attachment: 2django_cache.diff added

comment:5 by njharman, 17 years ago

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/cache.py is or should be it might be a grand thing cause I actually like writing unittests.

comment:6 by Tomáš Kopeček, 17 years ago

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 by anonymous, 17 years ago

Cc: permonik@… added

in reply to:  6 comment:8 by anonymous, 17 years ago

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 (http://www.djangoproject.com/documentation/cache/#controlling-cache-using-other-headers).

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

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

Patch is sufficient and working.

comment:10 by Tomáš Kopeček, 17 years ago

Keywords: sprintsept14 added

comment:11 by Tomáš Kopeček, 17 years ago

Triage Stage: AcceptedReady for checkin

comment:12 by Adrian Holovaty, 17 years ago

I'm checking this in.

comment:13 by Adrian Holovaty, 17 years ago

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