CurrentUserMiddleware messes up cache #1265

Closed
pvanderlinden opened this Issue Jun 8, 2012 · 10 comments

Comments

Projects
None yet
4 participants
Contributor

pvanderlinden commented Jun 8, 2012

When django-cms is used you must add the CurrentUserMiddleware to your middleware setting.

This middleware messes up your caching:

  • When the CurrentUserMiddleware is included, the user object is accessed directly (not only on a request of this information)
  • When the user object is accessed, this is retrieved from your session data
  • When your session data is accessed, Cookie is added to your vary header, and if not already done, a session is created
  • This will make page-caching effectivly useless

A better aproach is to get the user object only when it's used. A fix is for example to save the request object to thread.local instead of the user object, and when the get_current_user function is called return the user attribute from the request object.

Contributor

beniwohli commented Jun 8, 2012

Hey Paul,

we were already semi-aware of this issue, but thanks for making a proper ticket with description!

I'm personally very resistant to put stuff into threadlocals, though. Anybody got a better idea to solve this?

Contributor

pvanderlinden commented Jun 8, 2012

The functions which call get_current_user are all related to the admin I think. Maybe it is better to do this in the admin class. For example override the save_model function: https://docs.djangoproject.com/en/1.4/ref/contrib/admin/#django.contrib.admin.ModelAdmin.save_model

There is a drawback to that solution. If you save the model yourself, not through the admin, these actions aren't executed. But I don't think this is necessary in this case.

Collaborator

ojii commented Jun 15, 2012

we could save a lazy object on the thread local (similar to request.user). Another solution would be to kill get_current_user and set_current_user completely. This would be preferable but I assume a bit harder.

Contributor

pvanderlinden commented Jun 15, 2012

If the drawback of my previous post is not a problem get_current_user can be killed completly.
Otherwise the lazy object can be a solution

Contributor

pvanderlinden commented Jun 15, 2012

These are all for the admin. They can all be handled without the thread.local, I can try to write a patch if you want?

Collaborator

ojii commented Jun 15, 2012

yes please <3<3<3

Contributor

pvanderlinden commented Jun 15, 2012

:) Coming as soon as possible (probably not today :( )

Contributor

pvanderlinden commented Mar 27, 2013

I looked into this again (totally forgot, sorry).

The bad news
It looks like it is not possible to remove the get_current_user in a clean way. There are handlers tied to the User save method. The only way to change those is overriding the default User Admin, which is not really clean.

The good news
When you use django 1.4 or newer this problem is solved.
<1.4:
https://github.com/django/django/blob/stable/1.3.x/django/contrib/auth/middleware.py
A descriptor causing it to be evaluated as soon as you get the object from the request object

=1.4:
https://github.com/django/django/blob/stable/1.4.x/django/contrib/auth/middleware.py
(django/django@1cfb00d#diff-1)
A LazyObject, causing it to be evaluated when you access and attribute of the user object

Member

digi604 commented Jul 11, 2013

is this correct: django solved this for us and we can close this?

digi604 closed this Feb 10, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment