Opened 6 years ago

Closed 3 years ago

Last modified 3 years ago

#13285 closed Bug (fixed)

Generic views call populate_xheaders, which breaks caching

Reported by: carljm Owned by: ambv
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 6 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to duplicate
  • Status changed from new to closed

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 6 years ago by russellm

  • Resolution duplicate deleted
  • Status changed from closed to reopened
  • Triage Stage changed from Unreviewed to Accepted

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 carljm

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 5 years ago by julien

  • Severity set to Normal
  • Type set to Bug

comment:5 Changed 5 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:6 Changed 5 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:7 Changed 3 years ago by aaugustin

  • Status changed from reopened to new

comment:8 Changed 3 years ago by anonymous

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

comment:9 Changed 3 years ago by ambv

  • Owner changed from anonymous to ambv

comment:10 Changed 3 years ago by aaugustin

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 3 years ago by aaugustin

  • Triage Stage changed from Accepted to Ready 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 3 years ago by aaugustin (previous) (diff)

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

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

In 64e11a68f19793d11915e83574b1bb693da3980e:

Fixed #13285: populate_xheaders breaks caching

comment:14 Changed 3 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