Opened 8 years ago

Closed 8 years ago

#4071 closed (fixed)

cache_page decorator sets wrong Cache-Control header

Reported by: Ionut Ciocirlan (xlotlu) <upstaked@…> Owned by: permon
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:

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

Download all attachments as: .zip

Change History (15)

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

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

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Ready for checkin

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

comment:2 Changed 8 years ago by mtredinnick

  • Triage Stage changed from Ready for checkin to Accepted

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

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

comment:6 follow-up: Changed 8 years ago by permon

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

  • Cc permonik@… added

comment:8 in reply to: ↑ 6 Changed 8 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 (http://www.djangoproject.com/documentation/cache/#controlling-cache-using-other-headers).

comment:9 Changed 8 years ago by permon

  • Owner changed from nobody to permon
  • Status changed from new to assigned

Patch is sufficient and working.

comment:10 Changed 8 years ago by permon

  • Keywords sprintsept14 added

comment:11 Changed 8 years ago by permon

  • Triage Stage changed from Accepted to Ready for checkin

comment:12 Changed 8 years ago by adrian

I'm checking this in.

comment:13 Changed 8 years ago by adrian

  • Resolution set to fixed
  • Status changed from assigned to closed

(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