Opened 8 years ago

Closed 8 years ago

#5047 closed (fixed)

max-age set by cache_page is overwritten by cache_middleware

Reported by: permon Owned by: jacob
Component: Core (Cache system) Version: master
Severity: Keywords: cache_control, cache, middleware, 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

When setting 'max-age' by @cache_page (more generally by patch_response_headers) is later overwritten by cache_middleware. In the documentation (http://www.djangoproject.com/documentation/cache/#controlling-cache-using-other-headers) is said that it should work. The problem is in order of calling function. Decorator is called before middleware's process_response. Simple patch against r5783 is attached, but it probably needs some design decision and folllowing rewrite.

Attachments (3)

patch_cache_control.patch (672 bytes) - added by permon 8 years ago.
previous patch in patch format
patch_cache_controll.diff (1.5 KB) - added by anonymous 8 years ago.
svn patch
max_age.diff (524 bytes) - added by permon 8 years ago.
update

Download all attachments as: .zip

Change History (15)

Changed 8 years ago by permon

previous patch in patch format

Changed 8 years ago by anonymous

svn patch

comment:1 Changed 8 years ago by permon

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to permon
  • Patch needs improvement unset
  • Status changed from new to assigned

comment:2 Changed 8 years ago by permon

  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 8 years ago by permon

  • Triage Stage changed from Accepted to Unreviewed

comment:4 Changed 8 years ago by permon

  • Triage Stage changed from Unreviewed to Accepted

comment:5 Changed 8 years ago by permon

  • Keywords sprintsept14 added

comment:6 Changed 8 years ago by permon

  • Triage Stage changed from Accepted to Ready for checkin

comment:7 follow-up: Changed 8 years ago by jacob

  • Triage Stage changed from Ready for checkin to Accepted

I'm not sure this "rewrite" argument is needed -- shouldn't patch_cache_control() simply respect an existing max-age argument and not overrite it? Or perhaps set it to max(old, new)?

Changed 8 years ago by permon

update

comment:8 in reply to: ↑ 7 Changed 8 years ago by permon

Replying to jacob:

I'm not sure this "rewrite" argument is needed -- shouldn't patch_cache_control() simply respect an existing max-age argument and not overrite it? Or perhaps set it to max(old, new)?

Yes, I look at the patch and it was really ineffective :-( So my change is to use minimum of these two values. It makes me more sense than maximum of them. I think that nearest cache invalidation time should be the effective one.

comment:9 Changed 8 years ago by permon

  • Triage Stage changed from Accepted to Ready for checkin

comment:10 follow-up: Changed 8 years ago by jacob

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

Yeah, that's looking more like what I had in mind -- thanks!

comment:11 in reply to: ↑ 10 Changed 8 years ago by permon

Replying to jacob:

Yeah, that's looking more like what I had in mind -- thanks!

I've thinked little bit more about it and here are (i think) typical usecases:

  • I don't have cache middleware. Everything is ok
  • I have cache middleware with some caching time e.g. 300 seconds
  • * I have some special view, which could be cached for one hour, I'll use @cache_page decorator with 60*60
  • * I have another special view which should be cached only for ten seconds. Again simplest way is to use decorator with parameter 10

In last two cases is important, that I need to _rewrite_ default behaviour. So, the first patch solves the problem in most expected way. Otherwise (with min value) will be always one of these cases unreachable.

comment:12 Changed 8 years ago by jacob

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

(In [6434]) Fixed #5047: patch_cache_control now respects existing max-age settings. Thanks, permon.

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