Opened 15 years ago

Closed 10 years ago

Last modified 9 years ago

#13008 closed Bug (fixed)

@never_cache decorator should add 'no-cache' & 'no-store'

Reported by: harm Owned by: Markus Bertheau
Component: Core (Cache system) Version: dev
Severity: Normal Keywords:
Cc: Oliver Beattie, net147 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

I noticed that the never_cache django/views/decorators/cache.py does not add all the proper http headers to prevent that a page is cached.
It just adds a max-age=0. This does _not_ prevent browsers to actually cache that page.
For example when hitting back, firefox will happily show such a page if the connection to the server disappeared.

proposal, let never_cache() act as advertised: never cache. Let it add 'no-cache, must-revalidate' to the http response.

something like:

--- virtualenv/src/django/django/utils/cache.py 2010-03-02 15:11:54.000000000 +0100
+++ /tmp/cache.py       2010-03-02 15:11:48.000000000 +0100
@@ -104,6 +104,8 @@
         cache_timeout = settings.CACHE_MIDDLEWARE_SECONDS
     if cache_timeout < 0:
         cache_timeout = 0 # Can't have max-age negative
+        patch_cache_control(response, no_cache=True)
+        patch_cache_control(response, must_revalidate=True)
     if not response.has_header('ETag'):
         response['ETag'] = '"%s"' % md5_constructor(response.content).hexdigest()
     if not response.has_header('Last-Modified'):

Attachments (1)

13008.diff (507 bytes ) - added by Chris Beaven 12 years ago.

Download all attachments as: .zip

Change History (25)

comment:1 by Russell Keith-Magee, 15 years ago

Needs tests: set
Triage Stage: UnreviewedAccepted

comment:2 by Luke Plant, 15 years ago

Personally, I really hate it when I press the back button and the browser tells me I can't view that page any longer. Adding this behaviour to all the admin pages could be a pain in the neck, especially in the common work flow of "list of objects -> click one to edit/view -> go back to the list because you don't need to change it". Oon the other hand I am probably a 'power user' who knows when I am likely to be dealing with old data, and I can always use new tabs. But I do think we should think about the usability impact on the admin, which has 'never_cache' applied everywhere.

in reply to:  2 comment:3 by Karen Tracey, 15 years ago

Replying to lukeplant:

Personally, I really hate it when I press the back button and the browser tells me I can't view that page any longer.

Me too. But I don't see that behavior (from Firefox 3.5.7 or 3.6, or Chrome 4.0) if I run with the proposed change. In fact I don't even see either of those browsers re-requesting the page on using the back and forward buttons if I run with the proposed change, so I'm not entirely sure the proposed change accomplishes what it was intended to.

I do see IE8 re-request the page with the proposed change. However, for the others I have to also add: patch_cache_control(response, no_store=True), before they will start re-fetching the page when accessed via the back/forward buttons. But when they refetch, they all do it automatically and not with any error message about not being able to access the page anymore (unless I stop the server and then I get the standard can't establish connection message).

comment:4 by Oliver Beattie, 15 years ago

Cc: Oliver Beattie added

comment:5 by Luke Plant, 14 years ago

Severity: Normal
Type: Bug

@kmtracey - I just realised, we don't see those annoying pages about not being able to view the page because they happen when the page has been set to never cache/store AND was the result of a POST. Since we always re-direct after a POST in Django admin, we don't see those pages, and the web browser is happy to silently re-request the page, since the pages in history are always the result of GET requests. Well done us!

comment:6 by harm, 14 years ago

Besides the no_cache and must_revalidate this also needs no_store. (For Firefox)

+          patch_cache_control(response, no_store=True)

comment:7 by Aymeric Augustin, 13 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:8 by Aymeric Augustin, 13 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:9 by David Eyk, 12 years ago

Shouldn't this also set the Pragma: no-cache header?

Version 0, edited 12 years ago by David Eyk (next)

comment:10 by harm, 12 years ago

I think it should not be needed. Pragma: no-cache is intended for http requests, not http responses. (See http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.9)

comment:11 by Chris Beaven, 12 years ago

More explicitly, http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.32 says:

Note: because the meaning of "Pragma: no-cache as a response
header field is not actually specified, it does not provide a
reliable replacement for "Cache-Control: no-cache" in a response

I can't see must-revalidate being necessary, either. But maybe I'm overlooking something.

by Chris Beaven, 12 years ago

Attachment: 13008.diff added

comment:12 by net147, 12 years ago

Cc: net147 added

comment:13 by harm, 12 years ago

confirmed, must-revalidate is not necessary.

tested adding cache control headers with

@cache_control(max_age=0, no_cache=True, no_store=True)

on FF 19.0 + FF 19.02 + chrome 25 (on mac)
A 'back' button in the browser, does NOT reload a view.

So I think your patch 13008.diff is correct.

Last edited 12 years ago by harm (previous) (diff)

comment:14 by harm, 12 years ago

Has patch: set

comment:15 by Susan Tan, 11 years ago

Owner: changed from nobody to Susan Tan
Status: newassigned
Summary: @never_cache decorator should add 'no-cache' & 'must-revalidate'@never_cache decorator should add 'no-cache' & 'no-store'

comment:16 by Susan Tan, 11 years ago

Where should I write a test? Should it be in cache/tests.py? The function add_never_cache_headers is never tested in thetests/ directory.

comment:17 by thenewguy, 10 years ago

This issue is related to https://code.djangoproject.com/ticket/23755 (patch_cache_control should special case "no-cache")

comment:18 by Markus Bertheau, 10 years ago

Owner: changed from Susan Tan to Markus Bertheau

comment:19 by Markus Holtermann, 10 years ago

Needs tests: unset
Version: 1.2-betamaster

comment:20 by Claude Paroz, 10 years ago

Triage Stage: AcceptedReady for checkin

The patch looks rather good. Tim, a last review?

comment:21 by Tim Graham <timograham@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

In 4a438e4:

Fixed #13008 -- Added more Cache-Control headers to never_cache() decorator.

comment:22 by Tim Graham <timograham@…>, 9 years ago

In b51086d:

[1.8.x] Fixed #13008 -- Added more Cache-Control headers to never_cache() decorator.

Backport of 4a438e400b7ce0ab9d0b6876196cbe8d620a4171 from master

comment:23 by Tim Graham <timograham@…>, 9 years ago

In a5317539:

Refs #13008 -- Forwardported 1.8.8 release note.

Forwardport of b51086d57313e7ea857f4b96b62d25e600ee0a8d from stable/1.8.x

comment:24 by Tim Graham <timograham@…>, 9 years ago

In 85159b9:

[1.9.x] Refs #13008 -- Forwardported 1.8.8 release note.

Forwardport of b51086d57313e7ea857f4b96b62d25e600ee0a8d from stable/1.8.x

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