Opened 15 years ago

Closed 11 years ago

Last modified 11 years ago

#13285 closed Bug (fixed)

Generic views call populate_xheaders, which breaks caching

Reported by: Carl Meyer Owned by: Łukasz Langa
Component: Core (Cache system) Version: 1.1
Severity: Normal Keywords:
Cc: 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

Many of the Django generic views (as well as the flatpage view) call django.core.xheaders.populate_xheaders on the response. Populate_xheaders checks request.user, in order to include xheaders if the logged-in user is a staff member. Checking request.user triggers session access, which causes SessionMiddleware to add Vary: Cookie to the response, which makes caching much less efficient.

There should be some way to turn off populate_xheaders in production (or at least turn off the user-is-staff check), so efficient caching can be achieved.

Change History (14)

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

Resolution: duplicate
Status: newclosed

I'm going to close this as a dupe of #13283; I know the presentation is different, but the root cause is the same - we need to be able to determine the login/access status of the current user without marking the session as dirty.

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

Resolution: duplicate
Status: closedreopened
Triage Stage: UnreviewedAccepted

Apologies - I got myself confused here. Original reporter was correct - the issue here is that populate_xheaders is a debugging tool that breaks caching.

comment:3 by Carl Meyer, 14 years ago

The function-based generic views are now deprecated, and the class-based views that replaced them do not call populate_xheaders. So the only remaining non-deprecated view in the Django source tree that calls populate_xheaders is the flatpage view.

My take at this point is that the populate_xheaders function itself is a) of marginal usefulness, since these sorts of debugging headers can easily be added without a generic function for it, b) now barely used within Django, c) breaks caching as written, and d) completely undocumented. So, barring major objections, I think it should just be removed from Django entirely. Since it's undocumented I don't think a deprecation path is strictly needed, but to err on the side of caution it could be deprecated along the same timeline as the function-based generic views, since they all call it.

comment:4 by Julien Phalip, 14 years ago

Severity: Normal
Type: Bug

comment:5 by Aymeric Augustin, 13 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:6 by Aymeric Augustin, 13 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:7 by Aymeric Augustin, 12 years ago

Status: reopenednew

comment:8 by anonymous, 11 years ago

Owner: changed from nobody to anonymous
Status: newassigned

comment:9 by Łukasz Langa, 11 years ago

Owner: changed from anonymous to Łukasz Langa

comment:10 by Aymeric Augustin, 11 years ago

Two years later, it's probably time to remove it outright (and, since we're nice people, document it as a backwards incompatible change).

comment:11 by Aymeric Augustin, 11 years ago

Triage Stage: AcceptedReady for checkin

Łukasz will salvage the tests and re-add them in another PR moving XViewMiddleware to d.c.admindocs, as it's only used by admindocs and not obviously useful otherwise.

Last edited 11 years ago by Aymeric Augustin (previous) (diff)

comment:13 by Łukasz Langa <lukasz@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In 64e11a68f19793d11915e83574b1bb693da3980e:

Fixed #13285: populate_xheaders breaks caching

comment:14 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

In e73cb6391dd4f178e430c424eda6d9c97a70c7f6:

Merge pull request #1122 from ambv/issue13285

Fixed #13285: populate_xheaders breaks caching

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