Opened 7 years ago

Closed 4 years ago

Last modified 4 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


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 Changed 7 years ago by Russell Keith-Magee

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 Changed 7 years ago by Russell Keith-Magee

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 Changed 6 years ago by Carl Meyer

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 Changed 6 years ago by Julien Phalip

Severity: Normal
Type: Bug

comment:5 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:6 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:7 Changed 4 years ago by Aymeric Augustin

Status: reopenednew

comment:8 Changed 4 years ago by anonymous

Owner: changed from nobody to anonymous
Status: newassigned

comment:9 Changed 4 years ago by Łukasz Langa

Owner: changed from anonymous to Łukasz Langa

comment:10 Changed 4 years ago by Aymeric Augustin

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 Changed 4 years ago by Aymeric Augustin

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 4 years ago by Aymeric Augustin (previous) (diff)

comment:12 Changed 4 years ago by Aymeric Augustin

Has patch: set

comment:13 Changed 4 years ago by Łukasz Langa <lukasz@…>

Resolution: fixed
Status: assignedclosed

In 64e11a68f19793d11915e83574b1bb693da3980e:

Fixed #13285: populate_xheaders breaks caching

comment:14 Changed 4 years ago by Aymeric Augustin <aymeric.augustin@…>

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