Allow base queryset to be overridden via `ModelViewSet` to be used in the views

[laymonage]

Is your proposal related to a problem?

This is a similar problem described in wagtail-nest/wagtail-modeladmin#21, but for snippets.

When you override SnippetViewSet.get_queryset(request), the resulting queryset is only used for the index view. This means that if you put certain filters to prevent users from accessing certain objects, the "detail" view e.g. edit, delete, inspect, etc. can still be accessed via the URL.

The documentation does mention that the get_queryset() method override is for the index view, but for the reasons stated above, it would be nice if this extends to the other views as well.

Describe the solution you'd like

One way to achieve this would be to allow the queryset to be defined as a class/instance attribute on those views (some of them that extend from Django likely support this already), then pass the SnippetViewSet.get_queryset function down to the view as the queryset, then update the view code to check if the queryset is a callable. If so, call it with the request object, then use it to do the rest of the view logic.

Describe alternatives you've considered

Alternatively, just like ModelAdmin, we could leverage the fact that snippet models have the viewset accessible via model.snippet_viewset, and use it directly in the view. However, this does mean that the view now "knows" about the existence of the viewset, which is undesirable as the viewset is a higher abstraction on top of the view. Not to mention that this doesn't sit well with our work on making snippets features more generic in #10740.

[th3hamm0r]

+1 Just stumbled over this while using ModelViewSet and SnippetViewSet. I've automatically assumed that the overridden get_queryset() works similar to django's ModelAdmin 😰

Re-using that existing pattern used for the IndexView sounds good, maybe we could also move that logic into the parent ModelViewSet?
Maybe it makes sense to add some kind of get_instance_specific_view_kwargs(), which could be used by all views based on a single object.

[th3hamm0r]

I was thinking about trying a PR here, so I tried to get a better overview of the used classes. Of course there are multiple ways to solve it, so I would like to see if my plan seems good to you:

1. The BaseObjectMixin (link) seems quite a good place to implement the queryset-resolution (check if set, check if callable, use it in get_object()).
2. Find all used classes within the ModelViewSet and SnippetViewSet, which do not yet inherit from the BaseObjectMixin. The InspectView (link) is one example, but there are more (PreviewRevision,...) where I think this would fit quite well. This would reduce duplicate code (e.g. things like pk_url_kwarg).
3. Move SnippetViewSet's get_queryset() (link) into ModelViewSet, and set the queryset-kwarg in ModelViewSet.get_index_view_kwargs() instead of SnippetViewSet.get_index_view_kwargs().
4. Find the existing get_*_view_kwargs() or *_view() and pass queryset=self.get_queryset.
   I thought I would not implement a new get_instance_specific_view_kwargs for now, contrary to my suggestion from my last comment(?).
5. Tests:
   • Actually, the tests should not fail after the change, as the default was always to use the models default manager/queryset.
   • I would then add new test-classes which override the default queryset and then add simple GET-tests for all the used view classes, which should result in 404s.

The change does affect many places, but it seems quite straight forward, but I'm not sure if I oversee something here 😅

[laymonage]

Thank you for breaking it down @th3hamm0r!

I think my comment in #10621 (comment) and my recent PR #11005 are of relevance here.

A few points:

1. I'm starting to think whether we could/should ditch BaseObjectMixin in favour of Django's SingleObjectMixin. I know this somewhat goes against my preference of keeping things flat in my linked comment. And it does mean we lose control over the code.
   However, at least we're only making use of a mixin, not a base view class, so it only gives us methods/properties without really "controlling" the flow of the view process other than the things that happen in get_queryset. Thus, it doesn't suffer from issues such as get_object being called later in the view process when we might need it in setup()/dispatch().

Never mind, let me start over.

1. Yeah I think BaseObjectMixin is a good place to put it. I was considering using Django's SingleObjectMixin instead, but it seems to have a bunch of other stuff we don't really need. It'd probably be a good idea to follow the signatures though, so you can benefit from the familiarity of Django's views.
2. Yep that is a good idea.
3. Might be better to move it to get_common_view_kwargs, but we need to be sure as there might be cases where we don't want the queryset to be passed automatically.
4. See 3.
5. We'll just need to add tests for the other views in the tests I added in Allow IndexView's base queryset to be customised via ModelViewSet.get_base_queryset() #11005.

I think having the base queryset customisable at the ViewSet level is a valid use case for reasons already outlined in this issue. The queryset isn't final, the views can add their own default additions on top (e.g. search and filtering, adding annotations).

I think having the request available to the customisation point (like the current SnippetViewSet.get_queryset) would be important for most use cases, as otherwise this customisation isn't really useful. I previously discussed in #10851 (comment) whether it we should allow access to the view instance itself (so that you have access to other things, not just the request), but as per the discussion there, it's not a really good idea.

Before we proceed with this, I'd like to highlight this comment:

...I think we're better off embracing the fact that methods on the viewset aren't going to be as capable as methods defined on the views themselves, and that if you want to do anything that relies on tapping into the state of the views, you should override the view instead.

I think we should allow exposing the base queryset (with access to the request) in the viewset be an exception here. Of course, with the queryset being used in multiple views, developers have to be mindful of the returned queryset (ensuring it's not tailored to a specific view). What do you think @gasman?

For reference, DRF has something like this, but their infrastructure is much different. Their viewset plays a role during the views' lifecycle, so it can have access to the request via self.request: https://www.django-rest-framework.org/api-guide/viewsets/#modelviewset. This isn't in-line with Wagtail's viewsets that only act as a wrapper for the views' configuration and registration.

Let's also discuss this at the next core team meeting.

[th3hamm0r]

1. Yeah I think BaseObjectMixin is a good place to put it. I was considering using Django's SingleObjectMixin instead, but it seems to have a bunch of other stuff we don't really need. It'd probably be a good idea to follow the signatures though, so you can benefit from the familiarity of Django's views.

👍

2. Yep that is a good idea.

A special example is the HistoryView, which is both a detail view for the model and an IndexView, but it still seems to make sense to use the BaseObjectMixin for the get_object()-functionality here. The get_base_queryset() is already a custom version, which does not use the queryset attribute. So this shouldn't collide with the BaseObjectMixin I think.

3. Might be better to move it to get_common_view_kwargs, but we need to be sure as there might be cases where we don't want the queryset to be passed automatically.
4. See 3.

You're right 👍. Initially I thought just of the CreateView as an example which probably does not need the queryset, so it spoke against get_common_view_kwargs for me, but actually, most views could benefit from getting the base-queryset passed, and if not, you could still overwrite the kwarg when you instantiate your custom view (if it requires a different queryset).

5. We'll just need to add tests for the other views in the tests I added in Allow IndexView's base queryset to be customised via ModelViewSet.get_base_queryset() #11005.

👍

I think we should allow exposing the base queryset (with access to the request) in the viewset be an exception here. Of course, with the queryset being used in multiple views, developers have to be mindful of the returned queryset (ensuring it's not tailored to a specific view). What do you think @gasman?

Even though Django's modeladmin is structured a little differently, it still feels natural when the base-queryset is defined via the viewset. At the end, every view can build on it or not, but adapting the core views to consistently use it seems to be a good direction for me.

[gasman]

+1 for this feature.

I'd like to question whether passing the request object is the right choice - it seems to me that the primary (only?) motivation for sharing the get_queryset method between views is access control, in which case just passing the user object ought to be sufficient. I think it's better to be conservative about the amount of context we give to get_queryset, to discourage developers from filtering on things like URL parameters that are really specific to IndexView. It's not so much "I don't trust developers to use this responsibly", and more about nudging them to think about the correct place to put the logic - "is this dealing with access control, or the UI of the index view?"

Happy to give way on this if we can think of other scenarios for SnippetViewSet.get_queryset() beyond limiting the objects globally and per-user. Maybe something like how the locale URL parameter is passed around? (But even there, I don't think there's a need to filter the queryset anywhere but IndexView.)

Incidentally, I'd like to see this using the inject_view_methods mechanism introduced in #10851. Having the views implement a special case for "this object claims to be a queryset, but might actually be a callable" feels hacky, and obviously it's better from a code quality POV if we don't have multiple mechanisms for doing the same thing in different places.

[th3hamm0r]

TL;DR: I understand your points, but I would vote for making an exception with get_queryset and pass the request to it. It defines what the ModelViewSet manages and I would allow the developers adapt it centrally based on the request, not only the user object.

Looking at the current passed kwargs of all the views in the ModelViewSet here, beside many args that won't change for a viewset once instantiated, there are multiple cases, where passing a request would be nice, e.g. to quickly adapt the "list_display" for the index view based on the request. But here, I'm 100% with you, I wouldn't start passing the request object just because it is a central place. Its very easy to add a IndexView-subclass and adapt the list_display there based on the request (softly directing the dev to put the code where it belongs to 😉 ).

On the other hand, changing the central base queryset, which is probably one thing that is required by nearly every view class of a ModelViewSet (except the CreateView), based on the request, would require to adapt and think of every view class that is used in the viewset. And since it's so common to have a central get_queryset with a passed request, I would make this one exception.

[ababic]

I'd like to question whether passing the request object is the right choice - it seems to me that the primary (only?) motivation for sharing the get_queryset method between views is access control, in which case just passing the user object ought to be sufficient.

Over the years, I've used various values from request to customise the return value for a get_queryset() method:

• Values from request.GET (e.g. to avoid complex query optimisations when the q param is present, to get around search backend limitations)
• request.resolver_match (e.g. to apply certain filters for some views and not others, without having to customise multiple view classes)
• Multi-site/multi-tenancy related lookups (where it's common to pass the request on to identify the site/tenant of interest)
• Attribute values set by custom middleware

While it's certainly good to think about what we pass around and why, the downside of that is the restrictions it can introduce. While there could sometimes be a better way to do something, getting the job done quickly is often just as important (maybe you're working on a quick POC, or maybe you're new to Django/Wagtail and simply aren't aware of the alternatives). Being able to count on having access to the request in situations like this offers a lot security to devs, especially if they've spent a significant amount of time working with Django views or modeladmin (where passing request around is as common as day).

[gasman]

OK, I'm persuaded :-) Let's go with request!