cache_page decorator sets wrong Cache-Control header

[Ionut Ciocirlan (xlotlu) <upstaked@…>]

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.

[Simon G. <dev@…>]

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

[Malcolm Tredinnick]

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.

[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.

[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

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

[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.

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

[Tomáš Kopeček]

Patch is sufficient and working.

[Adrian Holovaty]

I'm checking this in.

[Adrian Holovaty]

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