axnsan12 / drf-yasg

Automated generation of real Swagger/OpenAPI 2.0 schemas from Django REST Framework code.
https://drf-yasg.readthedocs.io/en/stable/
Other
3.41k stars 438 forks source link

Wrong handeling of combination "@swagger_auto_schema(... responses={... XSerializer(many=True)})" and "@action(detail=True, ...)" #787

Open kiblik opened 2 years ago

kiblik commented 2 years ago

Bug Report

Description

In our project, I was trying to write the definition of a new API endpoint that combines Serializer(many=True) and detail=True.

I was able to write the expected definition for drf_spectacular but drf_yasg has a different result.

Response for drf_spectacular

{
  "count": 123,
  "next": "http://api.example.org/accounts/?offset=400&limit=100",
  "previous": "http://api.example.org/accounts/?offset=200&limit=100",
  "results": [
    {
      "model": "string",
      "id": 0,
      "name": "string"
    }
  ]
}

Response for drf_yasg

[
  {
    "model": "string",
    "id": 0,
    "name": "string"
  }
]

Is it a bug or did I miss something in the documentation?

Is this a regression?

I don't know, I'm writing a new implementation

Minimal Reproduction

class DeletePreviewModelMixin:
    @extend_schema(
        methods=['GET'],
        responses={status.HTTP_200_OK: serializers.DeletePreviewSerializer(many=True)}
    )
    @swagger_auto_schema(
        method='get',
        responses={status.HTTP_200_OK: serializers.DeletePreviewSerializer(many=True)}
    )
    @action(detail=True, methods=["get"], filter_backends=[])
    def delete_preview(self, request, pk=None):
        ...
...
class DeletePreviewSerializer(serializers.Serializer):
    model = serializers.CharField(read_only=True)
    id = serializers.IntegerField(read_only=True, allow_null=True)
    name = serializers.CharField(read_only=True)

Your Environment

Django==3.2.12
djangorestframework==3.13.1
django-filter==21.1
drf_yasg==1.20.0

Full context

https://github.com/DefectDojo/django-DefectDojo/pull/5612

tfranzel commented 2 years ago
kiblik commented 2 years ago

TL;DR:

Check 'default' and suffix

    @swagger_auto_schema(
        method='get',
        responses={'default': serializers.DeletePreviewSerializer(many=True)}
    )
    @action(detail=True, methods=["get"], filter_backends=[], suffix='List')

Long version

Hi @tfranzel,

Analysis

I did a quite deep dive. I was trying to find out, why yasg doesn't evaluate my definition as I expect.

  1. I knew that response is already array/list. Originally, I thought that behavior is different because the condition of these lines is False https://github.com/axnsan12/drf-yasg/blob/d9700dbf8cc80d725a7db485c2d4446c19c29840/src/drf_yasg/inspectors/view.py#L216-L217 Actually, I found out, that these lines are not even executed. Moreover, the whole function get_default_responses is not executed.
  2. So I started to debug get_response_serializers (which calls get_default_responses). I found out that get_default_responses is not called because I used status.HTTP_200_OK in responses (which is one of the "success responses"). https://github.com/axnsan12/drf-yasg/blob/d9700dbf8cc80d725a7db485c2d4446c19c29840/src/drf_yasg/inspectors/view.py#L230-L235 So I used 'default' as HTTP response code.
  3. Perfect, get_default_responses is executed right now. But my feeling from point 1. is right,
    • self.should_page is returning False because it calls self.has_list_response and it is returning False as well.
    • self.has_list_response is returning False because it calls self.is_list_view and it is returning False as well.
    • self.is_list_view is returning False because it calls utils.is_list_view and it is returning False as well.
  4. Now, when we are deep enough, let's find the real reason why https://github.com/axnsan12/drf-yasg/blob/d9700dbf8cc80d725a7db485c2d4446c19c29840/src/drf_yasg/utils.py#L219-L229 This implementation expects that if the user defines @action(default=True), the result shouldn't be listed. 😢

    a detail action is surely not a list route

So, because I'm not able to change the action name and detail has to be True, I decided to use a "dirty hack" and set suffix = 'List'. Thankfully there is not any side effect.

tfranzel commented 2 years ago

@kiblik I can't comment on the other things but detail works 100% like I said, trust me :smiley:

/x/   # list
/x/{id}/   # retrieve
/x/action # action with detail=False
/x/{id}/action # action with detail=True

DRF does not specify what comes out of the action endpoint, i.e. if it is a list or not. Spectacular defaults to non-list with actions and you need to provide many=True if you want your action to be a list.