Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migration to drf_spectacular #4210

Merged
merged 26 commits into from
Feb 12, 2022
Merged

Migration to drf_spectacular #4210

merged 26 commits into from
Feb 12, 2022

Conversation

Marishka17
Copy link
Contributor

@Marishka17 Marishka17 commented Jan 20, 2022

Motivation and context

Resolves #3015
Resolves #2006

How has this been tested?

Checklist

License

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.
  • I have updated the license header for each file (see an example below)
# Copyright (C) 2022 Intel Corporation
#
# SPDX-License-Identifier: MIT

@Marishka17 Marishka17 changed the title Mk/drf spectacular [WIP][Dependent] Migration to drf_spectacular Jan 25, 2022
@Marishka17 Marishka17 marked this pull request as ready for review January 25, 2022 10:51
@Marishka17 Marishka17 changed the title [WIP][Dependent] Migration to drf_spectacular [WIP] Migration to drf_spectacular Feb 4, 2022
@Marishka17 Marishka17 changed the title [WIP] Migration to drf_spectacular Migration to drf_spectacular Feb 4, 2022
@Marishka17 Marishka17 changed the title Migration to drf_spectacular [WIP] Migration to drf_spectacular Feb 7, 2022
@Marishka17 Marishka17 changed the title [WIP] Migration to drf_spectacular Migration to drf_spectacular Feb 8, 2022
@nmanovic
Copy link
Contributor

@Marishka17 , I have a couple of questions:

  • Is it possible to detect the current protocol automatically? (http or https)
  • Is it possible to detect the server host and port automatically? Why do we need the choice?

@Marishka17 Marishka17 changed the title Migration to drf_spectacular [WIP] Migration to drf_spectacular Feb 11, 2022
@Marishka17 Marishka17 changed the title [WIP] Migration to drf_spectacular Migration to drf_spectacular Feb 11, 2022
@Marishka17
Copy link
Contributor Author

@nmanovic, all comments have been fixed.

@Marishka17 Marishka17 mentioned this pull request Feb 11, 2022
2 tasks
@nmanovic nmanovic closed this Feb 12, 2022
@nmanovic nmanovic reopened this Feb 12, 2022
Copy link
Contributor

@nmanovic nmanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nmanovic nmanovic merged commit d098e42 into develop Feb 12, 2022
@nmanovic nmanovic deleted the mk/drf_spectacular branch February 12, 2022 06:19
Copy link

@tfranzel tfranzel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, I sometime look at big projects using spectacular to discover new usage patterns and possible bugs.

found quite a few unnecessarily complicated usages throughout. probably muscle-memory from yasg 😄 here are a few comments for improvement if you are interested. only commented each thing once but the patterns do repeat.

otherwise great PR, enjoy!

@@ -16,6 +18,37 @@
MembershipReadSerializer, MembershipWriteSerializer,
OrganizationReadSerializer, OrganizationWriteSerializer)


@extend_schema_view(retrieve=extend_schema(
Copy link

@tfranzel tfranzel Feb 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is more complicated than it needs to be. following snippet generates exactly the same schema. some notes fyi:

  • common annotations (tags) can be done once with @extend_schema on top.
  • no point in overriding responses when it is just default behavior. in fact it's misleading since you use the automatic request estimation while overriding responses
  • the version annotation is actually counter-productive because you are scoping the decorators. if someone could request 3.0, the default behavior would run, i.e. these decorators would be simply skipped. it's advisable to only "scope-decorate" non-DEFAULT_VERSION behavior.
  • also the view does not make use of different versions anyway, thus the default behavior does exactly the right thing already.
  • @extend_schema_view takes multiple args. no need for duplication
@extend_schema(tags=["organizations"])
@extend_schema_view(
    retrieve=extend_schema(
        summary="Method returns details of an organization"
    ),
    list=extend_schema(
        summary="Method returns a paginated list of organizatins according to query parameters"
    ),
    update=extend_schema(
        summary="Method updates an organization by id",
    ),
    partial_update=extend_schema(
        summary="Methods does a partial update of chosen fields in an organization",
    ),
    create=extend_schema(
        summary="Method creates an organization",
    ),
    destroy=extend_schema(
        summary="Method deletes an organization",
        responses={204: OpenApiResponse(description="The organization has been deleted")},
    ),
)
class OrganizationViewSet(viewsets.ModelViewSet):
   ....

@swagger_auto_schema(method='get', operation_summary='Method provides basic CVAT information',
responses={'200': AboutSerializer})
@extend_schema(summary='Method provides basic CVAT information',
responses={

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • due to giving serializer_class=AboutSerializer, the serializer is detected automatically.
  • 200 is the default code for actions (for native viewsets methods (list/retrieve/etc) 200, 201, 204 are detected automatically)

so this responses=... is not needed. spectacular gets this right automatically. you would only need it if many=True were used, like in some other places.

'url': openapi.Schema(type=openapi.TYPE_STRING)
@extend_schema_view(post=extend_schema(
summary='This method signs URL for access to the server',
description='Signed URL contains a token which authenticates a user on the server.'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fyi: spectacular does support docstring extraction just the same as yasg.

this would only be useful if you wanted to split summary and description while keeping them close together.

@Marishka17
Copy link
Contributor Author

@tfranzel, Hello!
Thanks for the comments! I will take them into account shortly in the next step of improving our schema.

@nmanovic nmanovic mentioned this pull request Mar 4, 2022
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate from drf-yasg to drf-spectacular HTTPS in Swagger
3 participants