set responses parm for extend_schema will make auto-discovered query parameters disappeared

[elonzh]

Describe the bug
set responses parm for extend_schema will make auto-discovered query parameters disappeared

To Reproduce

...
    filterset_fields = {
        "id": ["in"],
        "foo": ["exact"],
    }
    filter_backends = [DjangoFilterBackend]

    @extend_schema(
        parameters=[
            OpenApiParameter(
                "format",
                OpenApiTypes.STR,
                location=OpenApiParameter.QUERY,
                required=True,
            )
        ],
        responses={(200, "*/*"): OpenApiTypes.STR},
    )
    def list(self, request, *args, **kwargs):
		...

Expected behavior

Auto-discovered query parameters exist as normal

[tfranzel]

hi! is the parameter really missing? i can see the parameter in the operation, however i see a different issue. there happens some kind of name collision for the operation. i get x_retrieve_2 instead of x_list. can you confirm?

looks like the responses declaration creates that problem but i have to investigate.

def test_viewset_list_override(no_warnings):
    class XViewset(viewsets.ReadOnlyModelViewSet):
        queryset = SimpleModel.objects.all()
        serializer_class = SimpleSerializer

        @extend_schema(
            parameters=[
                OpenApiParameter(
                    "format",
                    OpenApiTypes.INT,
                    location=OpenApiParameter.QUERY,
                    required=True,
                )
            ],
            responses={(200, "*/*"): OpenApiTypes.STR},
        )
        def list(self, request, *args, **kwargs):
            pass  # pragma: no cover

    schema = generate_schema('/x', XViewset)
    assert schema['paths']['/x/']['get']['parameters'][0]['name'] == 'format' # OK
    assert schema['paths']['/x/']['get']['operationId'] == 'x_list'  # FAIL
    assert schema['paths']['/x/{id}/']['get']['operationId'] == 'x_retrieve'  #FAIL

[tfranzel]

so it turns out that you override the list detection by doing OpenApiTypes.STR, which is obviously not a list. this then generates a x_retrieve name instead of x_list. then you get a collision for 2 x_retrieve in that viewset.

we cannot change this, as it breaks a dozen other things. what you can do is set an explicit operationId='x_list', which prevents the collision from happening. then everything should be as expected. you basically undo the (name) change that resulted from your responses=....

[elonzh]

Set an explicit operationId='x_list' is not working, the _get_filter_parameters will detect the view type and it will be return a False if the response is_basic_type.

So this is definitely a bug.

    def _get_filter_parameters(self):
        if not self._is_list_view():
            return []

Maybe we can change the detection order，for example:

from

        if is_basic_type(serializer):
            return False
        if hasattr(self.view, 'action'):
            return self.view.action == 'list'

to

        if hasattr(self.view, 'action'):
            return self.view.action == 'list'
        if is_basic_type(serializer):
            return False

The best solution is providing an option to include the filter parameters because filter_backends are not restricted to use in list view.

rest_framework.schemas.openapi.AutoSchema.get_filter_parameters

    def get_filter_parameters(self, path, method):
        if not self.allows_filters(path, method):
            return []
        parameters = []
        for filter_backend in self.view.filter_backends:
            parameters += filter_backend().get_schema_operation_parameters(self.view)
        return parameters

    def allows_filters(self, path, method):
        """
        Determine whether to include filter Fields in schema.

        Default implementation looks for ModelViewSet or GenericAPIView
        actions/methods that cause filtering on the default implementation.
        """
        if getattr(self.view, 'filter_backends', None) is None:
            return False
        if hasattr(self.view, 'action'):
            return self.view.action in ["list", "retrieve", "update", "partial_update", "destroy"]
        return method.lower() in ["get", "put", "patch", "delete"]

[tui95]

Hi @tfranzel, I also encountered the same issue of filterset query parameters not being detected when using manually set responses via @extend_schema for file download API.

To Reproduce

import django_filters
from django_filters.rest_framework import DjangoFilterBackend

class PDFRenderer(renderers.BaseRenderer):
    media_type = 'application/pdf'
    format = 'pdf'
    charset = None
    render_style = 'binary'

    def render(self, data, media_type=None, renderer_context=None):
        return data

class SimpleFilterSet(django_filters.FilterSet):

    class Meta:
        model = SimpleModel
        fields = {
            'id': ['in'],
        }

class XViewset(viewsets.GenericViewSet):
    queryset = SimpleModel.objects.all()
    serializer_class = SimpleSerializer
    filterset_class = SimpleFilterSet
    filter_backends = [DjangoFilterBackend]
    renderers = [PDFRenderer]

    @extend_schema(
        # without explicit operation_id, this operation is detected as non-list and thus
        # will be named "x_retrieve", which create a collision with the actual retrieve.
        operation_id='x_list',
        responses={(200, 'application/pdf'): OpenApiTypes.BINARY},
    )
    def list(self, request, *args, **kwargs):
        pass  # pragma: no cover

Which produces the following schema

paths:
  /x/:
    get:
      operationId: x_list
      responses:
        '200':
          content:
            application/pdf:
              schema:
                format: binary
                type: string
          description: ''
      security:
      - cookieAuth: []
      - basicAuth: []
      - {}
      tags:
      - x

[tfranzel]

So I had another look and I think I now have a better undestanding of the problem. It is a bit of an unfortunate situation though.

Any changes to _is_list_view will likely break a lot of people. For better or worse we are stuck with the heuristic as is. However it works as expected here because the response is indeed not a list.

It is true that filter_queryset is also applied to retrieve. However, we made the simplifiying assumption that filters usually only make sense for lists (e.g. pagination). I imagine it to be very confusing having all those django-filters on all Viewset methods (retrieve, list, patch, put) all the time by default. Given that this has only come up twice in 2 years, I assume for most users this currently works just as they would expect.

Imho the problem is that we currently have no way of enabling the detected filters (not manually defined) when the response is clearly not a list. So basically a selective way to disable our "simplifiying assumption".

The operation_id is only partially relevant here as it is more of a side effect.

[tui95]

Thanks for the response. That's unfortunate though.

[tfranzel]

Not saying we won't fix it! Actually I think its not too bad. We can fix this. It's just not going to be an "automatic" fix, but that is not really a issue since your are already doing something manually by using @extend_schema.

[tfranzel]

        @extend_schema(
            responses={(200, 'application/pdf'): OpenApiTypes.BINARY},
            filters=True,
        )

is the preferred way to enable filters for non-list responses. This can also be used as suppression with False.

This does not touch list detection itself, since spectacular imho does the right thing here already. OpenApiTypes.BINARY is by definition not a list. By default the filters stay coupled to list detection, as this does the right thing for the vast majority of cases.

Depending on your case, you may also need the operation_id override to prevent the warning. This is intended behavior and is orthogonal to this issue.

[tui95]

Thank you so much! I've checked and it works wonderfully for me.

[tfranzel]

closing this issue for now. feel free to comment if anything is missing or not working and we will follow-up.