Add protection to some classes to prevent state leaking between requests?

I just wrote a post on a technique that I once employed to prevent state leaking between requests in the admin:

I wonder if this kind of protection could be added to ModelAdmin, MiddlewareMixin, and maybe other places. Please read the post and share your experiences/opinions!

2 Likes

@danilovmy touched on this in his talk in Porto IIRC. He showed how to make the admin view instances per-request. (Of the points he made, I think that was the one that saw nods in the audience.)

Ah yes. I guess that would be backwards compatible, but maybe introduce a performance regression for ModelAdmin classes that do something costly in their __init__.

I think the issue would still stand for MiddlewareMixin. I don’t think we can reasonably change to instantiating middleware per-request.

I’ve never seen a ModelAdmin class with heavy init, that’s why I offered to initiate a new instance on the fly.

I guess the main issue I see there is that it would make the API surface uglier. I mean we have the current well known behavior and then we have class based views. The suggested change would be a thing in between. :confused:

Yeah, that’s fair. It’s probably too hypothetical to worry about.

I don’t follow this comment @apollo13 , the suggestion you quoted is to initialize ModelAdmin per request, like class-based views.

I don’t know if you would consider this “heavy”, but there is this:

  • This is probably a good example of how not to do something
  • I almost hate to own up to this, despite how useful as it has been.
  • This shouldn’t be construed as any example or reason not to move forward with what you’re talking about here.
1 Like

Hi Adam,

There is imo a (small) difference between “initializing a class per request like class-based views” and actually making it a class-based view. For instance views designed as class-based views from the start don’t need to pass request to methods since they can rely on self.request being there. Now I understand that the admin views precede that time, but if we make them kinda like class-based views and put the request on the instance we are in a situation where new methods don’t need the request anymore but old ones still need to keep it for backwards compat. There are also a few get_ methods that probably could just be an instance attribute if we initialize per request etc…

I am not saying that those differences are showstoppers or anything, but I still think there will be subtle and possibly confusing differences to class based views.

I know this is probably (sadly) out of the question but I am wondering if we shouldn’t rewrite the admin based on class-based views before we retrofit a new API onto it (probably not going to happen, but worth to point out that the admin is not strongly covered by our backwards compat policy).

Cheers,
Florian

1 Like

One question, why came the idea to rewrite something in django.contrib.admin?

BTW. “in views designed as class-based views from the start don’t need to pass request to methods”: is the wrong approach to GCBV.

We can use self.request, but instead it you can see this construct: GCBV.handler(request, *args, **kwargs). And I accept this because self.request doesn’t work in GCBV if you skip GCBV.setup.

@danilovmy I think this is because the handlers are the view, and a view takes a request, and turns it into a response. It would be very strange not to have that (certainly in these latter days). (Skipping setup isn’t a supported usage, so I think that’s red-herring.)

(I think this is off-topic for this thread :wink:)

Did you typo on this? Or am I not mentally parsing this correctly?

1 Like

Senior moment. Thanks Ken. Will edit. :gift:

That’s my line. You’re far too young to be claiming that.

1 Like

Thank you, Сarlton. I find it is great, what we use the standard syntax like “handler(request, …)”

BTW. Missing a .setup() is a very common occurrence in my experience:

MyView().get(request, …). You call the view handler, but the .setup() is skipped.

Here Advanced testing topics | Django documentation | Django
the setup call is specifically added.

But I agree, offtopic.

Am I missing or misunderstading something here but the blog post says: But class-based views are instantiated per request, at least.
So even if you had class variables then those are not shared between the instances of class based views? Just asking because I’ve had leaking issues when I’ve used class variables on non-admin class based views and then semi-accidentally modified them later on.
Albeit this was on a very old Django version, but the linked code seemed to be similar between the old and newer versions or Django.

[quote=“KariWassholm, post:15, topic:30650, full:true”]
So even if you had class variables then those are not shared between the instances of class based views?[/quote]

As in the whole Python. In each instance of a class, you can get the class attributes and change them, these changes will be visible to all instances of that class in a given thread.

Aye, that’s why I’m confused as the blog post seemed to hint that non-admin Django views are not affected by that due to the way how the views are constructed…?

Each GCB view is constructed in .as_view(), and initargs, initkwargs are used there to instantiate each view, which you can’t easily change. The blog is about Django-Admin views, they are created in a different paradigm and are not programmatically coupled with GCBV. I hope I answered, or I didn’t understand the question.

I was referring to this part of the blog post https://adamj.eu/tech/2024/04/29/django-admin-prevent-leaking-requests/#should-django-do-something-here :

This issue isn’t specific to ModelAdmin. State can also persist between requests in other long-lived objects, at least in middleware classes. But class-based views are instantiated per request, at least.

This kind of hints that the class-based views are not affected by this issue, which is not true.

Django GCBV instances are not affected by the problem Adam highlighted. We have a new instance of the class on every request.

Django-ModelAdmins, or Middlewares instances are affected: We have the same instance of the class on every request.

1 Like