search

carltongibson / django-filter

A generic system for filtering Django QuerySets based on user selections
https://django-filter.readthedocs.io/en/main/
Other
4.45k stars 766 forks source link

Model is not set on the view, not allowing the `MultipleObjectMixin`s default `get_queryset` to be called #1681

Closed yaakovLowenstein closed 3 weeks ago

yaakovLowenstein commented 2 months ago

The issue is that because the model is never set on the view from the meta of the filterset_class the default get_queryset cannot be called.

The use case for this would be a call to super().get_queryset from a view that inherits from FilterView.

carltongibson commented 2 months ago

https://github.com/carltongibson/django-filter/blob/4a3eb8b1c0f55f0928ec99f00f76bb1681da63a8/django_filters/views.py#L123

If you'd like to explain a bit more maybe there's something here to help with, but the description is a bit terse as it is.

yaakovLowenstein commented 2 months ago

yes, sorry. In middle of submitting a PR as I thought it would be hard to explain but I think the PR will make it clearer

yaakovLowenstein commented 2 months ago

This should solve the issue https://github.com/carltongibson/django-filter/pull/1682

yaakovLowenstein commented 2 months ago

@carltongibson does the PR help understand the issue. I was hoping the test would provide some clarity about it. In summary, the issue is that you cannot call super.get_queryset() from a FilterView.

carltongibson commented 2 months ago

I'd expect you to set model as you would for ≈ any view extending MultipleObjectMixin

So in your test case: 

        class TestView(FilterView):
             model = Book
             ...
yaakovLowenstein commented 2 months ago

That would solve the issue, however I think what's a little confusing is that django-filters makes it so that you do not need to set the model because its already set on the filterset as demonstrated here, https://github.com/carltongibson/django-filter/blob/4a3eb8b1c0f55f0928ec99f00f76bb1681da63a8/django_filters/views.py#L48-L57, it implicitly knows the model form the filterset.

If the model is not defined resulting in an exception, it's caught because as the comment says, you do not need the model because you can get it form the filterset. Therefore why not set the model using the filterset's model.

Also because it catches the exception and ignores it, it actually hides the issue I describe above when you try calling super().get_queryset.

yaakovLowenstein commented 3 weeks ago

Hey, wanted to follow up here. Any thoughts on this?

I could try to clarify more of what I'm saying isn't making sense.

carltongibson commented 3 weeks ago

Hi, yes. As per my previous reply, you're meant to set model on your view.

If you want to add this fallback in your own project you're welcome, but it's not a change I want to add here. Thanks.