tfranzel / drf-spectacular

Sane and flexible OpenAPI 3 schema generation for Django REST framework.
https://drf-spectacular.readthedocs.io
BSD 3-Clause "New" or "Revised" License
2.39k stars 264 forks source link

Documentation (extend_schema) via mixins. #1259

Closed andreykryazhev closed 4 months ago

andreykryazhev commented 4 months ago

Hello, I would like to add documentation to some mixin classes in order to clean main views.py a little bit.

In other words, instead of this:

class EmployeeViewSet(viewsets.ModelViewSet):
    queryset = Employee.objects.all()

    @extend_schema(
        parameters=[
            OpenApiParameter(name="company_id",  required=True, type=int),
            OpenApiParameter(name="search", required=False, type=str),
        ],
        request=ListEmployeeSerializer,
        responses={status.HTTP_200_OK: ListEmployeeSerializer},
    )
    def list(self, request: Request, *args: P.args, **kwargs: P.kwargs) -> Response:
        response = super().list(request, *args, **kwargs)
        return get_columns_response(response, employee_table_columns)

I would like to try this:

class EmployeeViewSetDocsMixin:
    @extend_schema(
        parameters=[
            OpenApiParameter(name="company_id", description="Filter by company", required=True, type=int),
            OpenApiParameter(name="search", description="Поиск по имени/фамилии", required=False, type=str),
        ],
        request=ListEmployeeSerializer,
        responses={status.HTTP_200_OK: ListEmployeeSerializer},
    )
    def list(self, request: Request, *args: P.args, **kwargs: P.kwargs) -> Response:
        return super().list(request, *args, **kwargs)

class EmployeeViewSet(EmployeeViewSetDocsMixin, viewsets.ModelViewSet):
    queryset = Employee.objects.all()

    def list(self, request: Request, *args: P.args, **kwargs: P.kwargs) -> Response:
        response = super().list(request, *args, **kwargs)
        return get_columns_response(response, employee_table_columns)

This does not work by default and I tried to use metaclass to automatically pass schema from mixin (EmployeeViewSetDocsMixin) to targeted class (EmployeeViewSet). Here is what I did:

class MyCompanyBaseViewSetMcs(type):

    def __new__(mcs: MyCompanyBaseViewSetMcs, name: str, bases: tuple, attrs: dict):
        new_class = super().__new__(mcs, name, bases, attrs)

        mcs._collect_swagger_docs(new_class)
        return new_class

    @classmethod
    def _collect_swagger_docs(cls, decorated_class: MyCompanyBaseView) -> None:
        action_attrs = set(itertools.chain(*AutoSchema.method_mapping.items(), {"list", }))
        if hasattr(decorated_class, "get_extra_actions"):
            action_attrs |= {action.__name__ for action in decorated_class.get_extra_actions()}
        for action in action_attrs:
            for klass in decorated_class.mro():
                if klass is decorated_class or not hasattr(klass, action):
                    continue
                action_func = getattr(klass, action)

                has_schema = "schema" in getattr(action_func, "kwargs", {})
                if has_schema:
                    original_function = getattr(decorated_class, action)
                    if not hasattr(original_function, "kwargs"):
                        original_function.kwargs = {}
                    # here I am trying to pass schema from mixin to original function (list, get, create, etc)
                    original_function.kwargs["schema"] = action_func.kwargs["schema"]
                    break

class MyCompanyBaseView(metaclass=MyCompanyBaseViewSetMcs):
    pass

After that EmployeeViewSet looks so:

class EmployeeViewSet(EmployeeViewSetDocsMixin, viewsets.ModelViewSet, MyCompanyBaseView):
    queryset = Employee.objects.all()

    def list(self, request: Request, *args: P.args, **kwargs: P.kwargs) -> Response:
        response = super().list(request, *args, **kwargs)
        return get_columns_response(response, employee_table_columns)

But in this case I get the following error when start swagger page:

Traceback (most recent call last):
  File "/Users/ak/Work/mycompany-backend/.venv/lib/python3.9/site-packages/asgiref/sync.py", line 518, in thread_handler
    raise exc_info[1]
  File "/Users/ak/Work/mycompany-backend/.venv/lib/python3.9/site-packages/django/core/handlers/exception.py", line 42, in inner
    response = await get_response(request)
  File "/Users/ak/Work/mycompany-backend/.venv/lib/python3.9/site-packages/django/core/handlers/base.py", line 253, in _get_response_async
    response = await wrapped_callback(
  File "/Users/ak/Work/mycompany-backend/.venv/lib/python3.9/site-packages/asgiref/sync.py", line 468, in __call__
    ret = await asyncio.shield(exec_coro)
  File "/Users/ak/Work/mycompany-backend/.venv/lib/python3.9/site-packages/asgiref/current_thread_executor.py", line 40, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/Users/ak/Work/mycompany-backend/.venv/lib/python3.9/site-packages/asgiref/sync.py", line 522, in thread_handler
    return func(*args, **kwargs)
  File "/Users/ak/Work/mycompany-backend/.venv/lib/python3.9/site-packages/sentry_sdk/integrations/django/views.py", line 84, in sentry_wrapped_callback
    return callback(request, *args, **kwargs)
  File "/Users/ak/Work/mycompany-backend/.venv/lib/python3.9/site-packages/django/views/decorators/csrf.py", line 56, in wrapper_view
    return view_func(*args, **kwargs)
  File "/Users/ak/Work/mycompany-backend/.venv/lib/python3.9/site-packages/django/views/generic/base.py", line 104, in view
    return self.dispatch(request, *args, **kwargs)
  File "/Users/ak/Work/mycompany-backend/.venv/lib/python3.9/site-packages/rest_framework/views.py", line 509, in dispatch
    response = self.handle_exception(exc)
  File "/Users/ak/Work/mycompany-backend/.venv/lib/python3.9/site-packages/rest_framework/views.py", line 469, in handle_exception
    self.raise_uncaught_exception(exc)
  File "/Users/ak/Work/mycompany-backend/.venv/lib/python3.9/site-packages/rest_framework/views.py", line 480, in raise_uncaught_exception
    raise exc
  File "/Users/ak/Work/mycompany-backend/.venv/lib/python3.9/site-packages/rest_framework/views.py", line 506, in dispatch
    response = handler(request, *args, **kwargs)
  File "/Users/ak/Work/mycompany-backend/.venv/lib/python3.9/site-packages/drf_spectacular/views.py", line 83, in get
    return self._get_schema_response(request)
  File "/Users/ak/Work/mycompany-backend/.venv/lib/python3.9/site-packages/drf_spectacular/views.py", line 91, in _get_schema_response
    data=generator.get_schema(request=request, public=self.serve_public),
  File "/Users/ak/Work/mycompany-backend/.venv/lib/python3.9/site-packages/drf_spectacular/generators.py", line 271, in get_schema
    paths=self.parse(request, public),
  File "/Users/ak/Work/mycompany-backend/.venv/lib/python3.9/site-packages/drf_spectacular/generators.py", line 236, in parse
    assert isinstance(view.schema, AutoSchema), (
AssertionError: Incompatible AutoSchema used on View <class 'accounts.api.views.EmployeeViewSet'>. Is DRF's DEFAULT_SCHEMA_CLASS pointing to "drf_spectacular.openapi.AutoSchema" or any other drf-spectacular compatible AutoSchema?

The issue that drf_spectacular expects instance but not a class. But as I can see in extend_schema the very class in needed to pass: https://github.com/tfranzel/drf-spectacular/blob/cc916372a0e3fafc3af48f94dd1a985f5fc466e9/drf_spectacular/utils.py#L565.

Is it possible to resolve (or bypass this check somehow)? Just in case I tried temporary:

class SchemaGenerator(BaseSchemaGenerator):

     ...

     def parse(self, input_request, public):
            ...
            # instantiate if schema not an intance
            import inspect
            if inspect.isclass(view.schema):
                view.schema = view.schema()

            assert isinstance(view.schema, AutoSchema), (
                f'Incompatible AutoSchema used on View {view.__class__}. Is DRF\'s '
                f'DEFAULT_SCHEMA_CLASS pointing to "drf_spectacular.openapi.AutoSchema" '
                f'or any other drf-spectacular compatible AutoSchema?'
            )

Of course it is just for checking reason, but as I can see this change finally allows me to see documentation on swagger-page from mixins. But is there a way to do it right way?

Versions:

tfranzel commented 4 months ago

Hello,

so mixins do actually work and the decoration inside a mixin will get picked up properly. However your expectation does not align with python semantics.

You correctly mix in a class with a decoration, but then proceed to override that (decorated) mixed-in method with your own. Essentially hiding the decoration because now your EmployeeViewSet. list will get used, which makes sense because that is what is used when calling list on the instance.

Just imagine wanting to replace/override the decoration and spectacular would use some magic to resurrect that decoration that you explicitly overridden. That would be very bad.

I would recommend to rename list to EmployeeViewSet._list and call that instead of super(). That way you are not overriding list but still have you generic mixed in functionality.

Not gonna comment on the meta class stuff because it is complicated and dragons are ahead.

andreykryazhev commented 4 months ago

Thank you very much for your answer.

The cause for exception was a code like this:

class EmployeeViewSet(viewsets.ModelViewSet):

    ...

    @action(["get"], detail=False)
    def profile(self, request: Request, *args: P.args, **kwargs: P.kwargs) -> Response:
        ...

    @profile.mapping.delete
    def delete_me(self, request: Request, *args: P.args, **kwargs: P.kwargs) -> Response:
        ...

Mapping actions somehow leaded to exception from above.

Looks like this code does not lead to any errors:

class MyCompanyBaseViewSetMcs(type):

    def __new__(mcs: MyCompanyBaseViewSetMcs, name: str, bases: tuple, attrs: dict):
        new_class = super().__new__(mcs, name, bases, attrs)
        mcs._collect_mixin_docs(new_class)
        return new_class

    @classmethod
    def _collect_mixin_docs(cls, decorated_class: MyCompanyBaseView) -> None:
        action_attrs = cls._collect_action_attrs(decorated_class)
        for action in action_attrs:
            for klass in decorated_class.mro():

                # if decorated class does not contain some action
                # it will be picked from mixin without any issues
                if klass is decorated_class and not hasattr(klass, action):
                    break

                if klass is decorated_class or not hasattr(klass, action):
                    continue

                # in other way try to collect schema from docs-mixin
                # and put it manually to action in decorated class
                action_func = getattr(klass, action)
                has_schema = "schema" in getattr(action_func, "kwargs", {})
                if has_schema:
                    original_function = getattr(decorated_class, action)
                    if not hasattr(original_function, "kwargs"):
                        original_function.kwargs = {}
                    original_function.kwargs["schema"] = action_func.kwargs.pop("schema")
                    break

    @classmethod
    def _collect_action_attrs(cls, decorated_class: MyCompanyBaseView) -> set[str]:
        action_attrs = set(itertools.chain(*AutoSchema.method_mapping.items()))
        # by unknown reason "list" is not included to "AutoSchema.method_mapping"
        action_attrs |= {"list", }
        if hasattr(decorated_class, "get_extra_actions"):
            action_attrs |= {action.__name__ for action in decorated_class.get_extra_actions()}

        # also collect mappings
        mapped_actions = set()
        for action in action_attrs:
            if not hasattr(decorated_class, action):
                continue
            func = getattr(decorated_class, action)
            if hasattr(func, "mapping"):
                mapped_actions |= set(func.mapping.values())
        action_attrs |= mapped_actions

        return action_attrs

So, look like technically it works (but it require much more testing, of course). But if authors/maintainers from spectacular team think this approach is dangerous and risky and do not approve it then of course we refuse it too.

Thank you very much for your suggested way with renaming action to _action. I am not sure it will cover all cases: actions, actions mappings, etc. But it is definitely what I need to check since this way is much much more safe.

andreykryazhev commented 4 months ago

We finally ended with much simpler in-box-solution (extend_schema_view):

docs.py

from drf_spectacular.utils import OpenApiParameter, extend_schema, extend_schema_view

EmployeeViewSetDocs = extend_schema_view(
    list=extend_schema(
        parameters=[
            OpenApiParameter(name="company_id", required=True, type=int),
            OpenApiParameter(name="search", required=False, type=str),
        ],
        request=ListEmployeeSerializer,
        responses={status.HTTP_200_OK: ListEmployeeSerializer},
    ),
    create=extend_schema(
        request=CreateEmployeeSerializer,
        responses={status.HTTP_200_OK: CreateEmployeeSerializer},
        parameters=[CompanyIdParam],
    ),
    ...
)

And we using it this way:

views.py

@EmployeeViewSetDocs
class EmployeeViewSet(viewsets.ModelViewSet, MyCompanyBaseView):
    queryset = Employee.objects.all()

    def create(self, request: Request, *args: P.args, **kwargs: P.kwargs) -> Response:
         ...

    def list(self, request: Request, *args: P.args, **kwargs: P.kwargs) -> Response:
        ...

As I can see it greatly works with actions too (except action mappings but it is not a big deal).