Closed JestemStefan closed 1 year ago
The contract for get_queryset()
is to return YourModel.objects.none()
for the None
case. The correct place to bulletproof your code is there. (A combination of unit tests and coverage should be sufficient.)
The BaseFilterSet
leverages providing just the model
meta argument, and is mirrored across the Django world: admins, forms, DRF model views, and so on.
I agree on the part that get_queryset
should return YourModel.objects.none()
and as I said it was a mistake on my end. Which is fixed already and not relevant to this issue.
My point is that, for example, DRF viewsets already require you to configure queryset
or get_queryset
or it will raise:
E AssertionError: 'SomeViewSet' should either include a queryset attribute, or override the get_queryset() method.
So in this case queryset
returning None can be only result of an error, but it's silenced by BaseFilterSet init which is "hidden" few layers deep into inheritance.
Also like I said earlier in viewset module there is FilterMixin
which silence ImproperlyConfigured raised by Django Viewsets.
Those are two examples were it doesn't seems like it's in line with Django/DRF.
In my opinion, requiring explicit queryset will be beneficial, because it's less error prone and in line with "Explicit is better than implicit"
Hi.
FilterMixin
isn't for use with DRF... — rather you use the filter backend as per the Integration with DRF docs.
For me, it's not the filter set's responsibility to catch this kind of weird error in the view. (It's what static type checking would solve, FWIW)
More though, changing BaseFilterSet
to require a queryset here would be a breaking change. It's a feature to be able to just set the model
.
I hope that makes sense.
You'd be welcome to subclass the DjangoFilterBackend
class and add a check there though...
def filter_queryset(self, request, queryset, view):
assert queryset is not None
return super().filter_queryset(request, queryset, view)
I know that FilterMixin in not meant to be used with DRF. If you check I didn't said that in my previous comment. I just pointed that it's silencing ImproperlyConfigured error when model is not provided in Django Viewset.
It's not even about filterset responsibility to raise this error. the issue is that it's silencing existing issue (error in get_queryset method) which normally will show up somewhere.
You'd be welcome to subclass the DjangoFilterBackend class and add a check there though...
We are using DjangoFilterBackend
together with DRF like mentioned in integration docs.
We went with this solution on our backend. I was just wondering if this can be solved at the source.
I understand your motivation. Thanks for the response and have a nice day.
When working on the new endpoint with DRF I made a mistake and my
get_queryset
method was returning None.I didn't noticed this at first, because endpoint was still working, but it was listing all the records.
After debugging it I find out that that in ListModelMixin from DRF there is a line:
queryset = self.filter_queryset(self.get_queryset())
In this case
self.get_queryset()
was returningNone
, butfilter_queryset
was pulling all records from somewhere.I dig deeper and I found that in the BaseFilterSet there is a code like:
In my opinion this is unexpected side effect and when provided with invalid queryset (like
None
), exception should be raised that Viewset is ImproperlyConfigured or queryset must be provided.Method in
DjangoFilterBackend
is calledfilter_queryset
and queryset is required param so this still looks like side effect. I checked the documentation and it seems that this behaviour is not documented.I also found that in
FilterMixin
for viewsets there is special handling forImproperlyConfigured
. It furthers points that this is not expected behaviour even by Django/DRFSince this is error prone in my opinion, I propose to enforce that queryset must be provided. This will also enforce proper configuration of viewsets etc.