Closed TheSuperiorStanislav closed 2 years ago
Hi @TheSuperiorStanislav, we added a couple of "improvements" which include more robust retrieval of method types. As a matter of fact this was correct for you only by accident. Before, the logic was unable to retrieve the "method arg type" and silently defaulted to str
. In the new version this is "detected" and a subsequent last resort attempt is made on the model which then creates the warning.
Since we cannot run that method we must rely on type hints. You found one possible solution, while the other one is:
def filter_by_availability_status(self, queryset, name, value: str): # notice the str hint
I would say this is more correct than before, however I think we can mitigate this particular case by leveraging the choices. Of course this only applies if there are any. I'll try to work this out.
Hi @tfranzel, thank you for responding. The other solution won't work if your choices are Model
.
class ResultFilter(filters.FilterSet):
codes = ModelInFilter(
queryset=Code.objects.all(),
method="filter_codes",
)
class Meta:
model = models.Result
fields = {}
@extend_schema_field(OpenApiTypes.INT) # <- this is what we acually want
def filter_codes(self, queryset, name, value: list[Code]): # <- this is what django-filters passing
"""Filter results by codes."""
return queryset.filter_by_codes(
value,
)
Also, I have another problem. Say you have some logic in get_queryset
which needs user to be authenticated
. Now it fails with this error. (Maybe add traceback, because as of now it's hard to understand where exception happened)
Exception raised while trying resolve model field for django-filter field "code". Defaulting to string (Exception: 'AnonymousUser' object has no attribute 'has_access_to')
Maybe fallback to queryset
attr?
Another case would be when you created action for ViewSet
which is using different queryset
and filter
@extend_schema_view(
start=extend_schema(
filters=True,
request=serializers.StartJobRequestSerializer,
responses={
status.HTTP_201_CREATED:
serializers.StartJobResponseSerializer,
},
),
)
class ExportJobViewSet(
mixins.ListModelMixin,
mixins.RetrieveModelMixin,
viewsets.GenericViewSet,
):
"""API viewset for Job model."""
permission_classes = (permissions.IsAdminUser,)
queryset = models.Job.objects.all()
serializer_class = serializers.JobSerializer
filterset_class: django_filters.rest_framework.FilterSet = None
@action(
methods=["POST"],
detail=False,
queryset=model.SomeOtherModel,
filterset_class=SomeOtherModelFilter,
filter_backends=[
django_filters.rest_framework.DjangoFilterBackend,
],
)
def start(self, request: Request):
"""Validate request data and start Job."""
serializer = self.get_serializer(
data=request.data,
filter_kwargs=request.query_params,
)
serializer.is_valid(raise_exception=True)
job = serializer.save()
return response.Response(
data=self.get_detail_serializer_class()(
instance=job,
).data,
status=status.HTTP_201_CREATED,
)
Exception raised while trying resolve model field for django-filter field "code". Defaulting to string (Exception: Cannot resolve keyword 'created_by' into field. Choices are: created, code, modified, id)
I think it's because it's trying to use View's main queryset, instead of action's specific.
codes = ModelInFilter(
This looks like a custom filter that is unknown to django-filter. filter fields that accept multiple values are usually derived from BaseCSVFilter
or MultipleChoiceFilter
. I think BaseCSVFilter
is more appropriate here. If you add that base class to your filter, we detect the "list" nature of your field automatically. No list[Code]
required!
Also, I have another problem. Say you have some logic in get_queryset which needs user to be authenticated.
We have a "fix" for that. I just found that we never added a FAQ entry for it and its only mentioned in the docs under the yasg section. So here were go: https://drf-spectacular.readthedocs.io/en/latest/faq.html#my-get-queryset-depends-on-some-attributes-not-available-at-schema-generation-time
I think it's because it's trying to use View's main queryset, instead of action's specific.
Oh that is a good question. I believe an explicit get_queryset
method takes precedence over the queryset
view attribute (and likely also action queryset
parameter), which is why we call it. Not sure if queryset
attribute is even ever directly used. default get_queryset
just does return self.queryset
.
This looks like a custom filter that is unknown to django-filter.
About ModelInFilter
, it's same kind of filter for field that is generated by django-filters for lookup_expr that is equal in
. BaseInFilter
comes from BaseCSVFilter
.
from django_filters import rest_framework as filters
class ModelInFilter(filters.BaseInFilter, filters.ModelChoiceFilter):
"""Custom filter to filter by `in` expression.
It generates same type of filter that django_filters filter generates
when you specify field and `in` as lookup in Meta class.
It allows to sent choices like this `?field=1,2,3`
"""
We have a "fix" for that.
That is nice, but can't drf-spectacular
try to check viewset's queryset attr? Because usually, I think, most people do it like this ⇾ set queryset attr(do everything that doesn't depend on request) and then override get_queryset
to add logic dependent on request. Asking that, because adding extra case for views in which there are no way unauthenticated user would get in, I think can be avoided.
class ModelInFilter(filters.BaseInFilter, filters.ModelChoiceFilter):
well in that case it should already be a list by definition: ModelInFilter -> BaseInFilter -> BaseCSVFilter
and it should show up as a list.
That is nice, but can't drf-spectacular try to check viewset's queryset attr?
That is probably what most people do, but it's not guaranteed to be correct. You can set the attr but then do something different in get_queryset
while the attr is never actually used. I know this is a academic argument, but making false assumptions will lead to unexpected behavior and people do all kinds of crazy stuff in their views. Also we do not call it easily. It is already in the fallback code path and only used when other approaches fail.
Asking that, because adding extra case for views in which there are no way unauthenticated user would get in, I think can be avoided.
return YourModel.objects.none()
should already be pretty safe. It just returns an empty queryset which by definition does not leak objects.
Hey @TheSuperiorStanislav,
so this fix should address your filter_by_availability_status
issue, where we now also look into choice value types when nothing else is available. Your annotation should not be required anymore in this particular case.
regarding ModelInFilter
, I see no good reason why isinstance(filter_field, filters.BaseCSVFilter)
would not take effect, if the field is derived like you said. The tests even cover subclasses like CustomBaseInFilter(BaseInFilter)
. We would need more information here to continue.
Hi @tfranzel, just checking it out -> filter_by_availability_status
is fixed, but ModelInFilter is still not. I even checked isinstance(filter_field, filters.BaseCSVFilter)
, so i don't really know what else i can give that will help.
P.S. Noticed that warnings numerations is starting from zero
when I converted you snippets into a test, it works as expected for me
@pytest.mark.contrib('django_filter')
def test_filters_derived_base_in_with_method(no_warnings):
class ModelInFilter(BaseInFilter, ModelChoiceFilter):
""" your custom filter """
pass
class XFilterSet(FilterSet):
codes = ModelInFilter(
queryset=SimpleModel.objects.all(),
method="filter_codes",
)
@extend_schema_field(OpenApiTypes.INT)
def filter_codes(self, queryset, name, value):
"""Filter results by codes."""
return queryset.filter_by_codes(value)
class Meta:
model = SimpleModel
fields = '__all__'
class XViewset(viewsets.ReadOnlyModelViewSet):
queryset = SimpleModel.objects.all()
serializer_class = SimpleSerializer
filterset_class = XFilterSet
filter_backends = [DjangoFilterBackend]
schema = generate_schema('/x', XViewset)
assert schema['paths']['/x/']['get']['parameters'][0] == {
'in': 'query',
'name': 'codes',
'schema': {'type': 'array', 'items': {'type': 'integer'}}, # <--- exactly as expected
'description': 'Multiple values may be separated by commas.',
'explode': False,
'style': 'form'
}
notice the OpenApiTypes.INT
annotation but since BaseInFilter
is a BaseCSVFilter
it gets detected as list and subsequently wrapped in an array
. As far as I am concerned that is working correctly. If that issue persists for you, you need to provide a bug reproduction as I don't know what else to try here.
Noticed that warnings numerations is starting from zero
we are computer scientists and we start counting at zero :smile: no but seriously is that supposed to be of importance here?
closing this issue for now. feel free to comment if anything is missing or not working and we will follow-up.
I just ran into this same problem and stumbled upon this issue. Unfortunately while the fix here would fix my problem exactly, it is only applied for ChoiceFilter
and not for MultipleChoiceFilter
and thus does not help me. I will send a pull request shortly.
Additionally I found another strange thing about this auto-detection: it is actually able to override an enum
setting given by @extend_schema_field
, which seems strange to me. I would expect @extend_schema
and friends to override any and all automatic detection. Lmk if I should make a separate issue about that.
@diesieben07 please provide a reproduction for your second point in a separate issue and we will follow up. It does indeed sound strange and is most likely a bug since the decorators are supposed to always trump auto-detection.
Describe the bug
drf-spectacular==0.22.0
fails to recognize the type of filter if its name doesn't match with one of the fields of model. Works fine withdrf-spectacular==0.21.*
.To Reproduce
Which produces the following message while running
spectacular
commandExpected behavior No warnings and
python manage.py spectacular --file .tmp/schema.yaml --validate --fail-on-warn
command is not failingQuick fix You can fix by setting field type manually via
extend_schema_field