~petersanchez/public-inbox

1

[PATCH django-impersonate] optimization | preserve request.user laziness

Details
Message ID
<CAJF=k3=D3vguoC6ZhGJDQaRinJTU0H60zRyCQoT+fFVq7nonzQ@mail.gmail.com>
DKIM signature
missing
Download raw message
Hello. By default django request.user is lazy object so when we don't
access it - it doesn't hit database. But in ImpersonateMiddleware we
have 2 places that "disable" this behaviour:

class ImpersonateMiddleware(MiddlewareMixin):
    def process_request(self, request):
        request.user.is_impersonate = False  # <----- this one
        request.impersonator = None

        if is_authenticated(request.user) and \  # <----- and this one
           '_impersonate' in request.session:
            new_user_id = request.session['_impersonate']
            .....

As a quick-and-dirty fix we can just remove the first line - but in
client code we would access this param as `getattr(request.user,
'is_impersonate', False)`.

The second one is solved by placing `'_impersonate' in
request.session` as a first condition.

I think for many people impersonated requests are a very little amount
of the whole load so it could help to improve performance.

Thanks.
Details
Message ID
<20201222011618.r4g5lhd7y5rhffsp@thinkpad>
In-Reply-To
<CAJF=k3=D3vguoC6ZhGJDQaRinJTU0H60zRyCQoT+fFVq7nonzQ@mail.gmail.com> (view parent)
DKIM signature
missing
Download raw message
Interesting catch. I have some thoughts below.

On 12/21, Илья wrote:
>class ImpersonateMiddleware(MiddlewareMixin):
>    def process_request(self, request):
>        request.user.is_impersonate = False  # <----- this one
>        request.impersonator = None
>
>        if is_authenticated(request.user) and \  # <----- and this one
>           '_impersonate' in request.session:
>            new_user_id = request.session['_impersonate']
>            .....
>
>As a quick-and-dirty fix we can just remove the first line - but in
>client code we would access this param as `getattr(request.user,
>'is_impersonate', False)`.

The issue with changing this is it will break the "API" (if we want to
call it that) of the app and no doubt break deployments for end users
who are expecting this value.

>The second one is solved by placing `'_impersonate' in
>request.session` as a first condition.

This makes sense. I've pushed this change.

>I think for many people impersonated requests are a very little amount
>of the whole load so it could help to improve performance.

Agreed that every query counts. Though the single query probably isn't
the end of the world for 99.9% of cases.
Reply to thread Export thread (mbox)