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.4k stars 266 forks source link

`adrf` (Async DRF) Support #947

Open johnthagen opened 1 year ago

johnthagen commented 1 year ago

It was recently decided that the "official" async support for DRF would be the adrf package:

Does drf-spectacular support this package? I didn't see it listed in the third party packages:

It provides a new Async class-based and functional views. These allow end users to use async functionalities to better scale their DRF applications.

Somewhat related to

tfranzel commented 1 year ago

Hey John,

On the topic: If this is the endorsed package, we should make an effort. I am not sure yet how exactly adrf works, so supporting it may range from trivial to really hard, depending on how many idioms and mechanics are broken by it. I hope interest will pick up. I'd like to see some buzz there before I can allocate time for it. However, others are invited to give it a first short if they like.

Regarding #931, I don't think it is really related. They used plain django (async) views. There is no structure around it to inspect. No "declared" parsers, renderes, serializers, pagination, etc. If adrf breaks all the conventions, this will be a steep uphill battle. If its only a minimally invasive patch to DRF, I can see this working.

"Off" the topic: Honestly, not sure if it is the best course of action for the DRF project. Outsourcing this essential feature to an external package not even under the @encode umbrella feels like ducking away. Is DRF kinda falling behind here? Django has had async support in the works for a long time and it is finally here. Django 4.2 will be the first version with psycopg3, enabling async ORM in addition to async view from 4.1. Only DRF is holding out, which is sad because the majority of the heavy lifting was already done by the awesome folks at @psycopg and @Django.

It kind of feels like drf-spectacular again, which was declined to be integrated into DRF. It was the right decision at the end of the day, but I guess cohesive async support is a different beast, but maybe I'm wrong here.

If I look over the fence at django-ninja or fastapi, I see fast frameworks with build-in async support as well as OpenAPI schema generation out of the box. We look a bit pale in comparison at the moment, don't you think? well that's my 2 cents.

johnthagen commented 1 year ago

I hope interest will pick up. I'd like to see some buzz there before I can allocate time for it.

Completely understand/agree. I mostly wanted you to be aware of the DRF decision as soon as I learned about it so you were aware/prepared since drf-spectacular is the defacto OpenAPI project for DRF at this point.

"Off" the topic

I agree with all of your sentiments. I asked this question to Tom and the response was:

If we're going to add complexity to a codebase, then we need to be confident that it's worth the trade-off. Dealing with that as a third party package is a more flexible way to explore the design space, without committing ourselves to it.

My feeling is that DRF is a bit in maintence mode and the maintainers are delegating new features many would consider "core" in 2023 (OpenAPI 3 support, Async, etc.) to third party packages to reduce maintence burden.

tfranzel commented 1 year ago

Completely understand/agree. I mostly wanted you to be aware of the DRF decision as soon as I learned about it so you were aware/prepared since drf-spectacular is the defacto OpenAPI project for DRF at this point.

Thanks for letting me know. I'm sure we can figure out some solution to this.

Dealing with that as a third party package is a more flexible way to explore the design space

I kind of agree with his statement there. It indeed keeps you flexible, but do you want 2 different async plugins for DRF like we have 2 OpenAPI plugins? That fragmentation is not good for essential features. Also, I don't think the design space is that large honestly. How many ways can this be implemented sensibly? Not that many I suppose. Also, not committing while others have async as core feature is not good look. As you rightly pointed out, it feels a bit like maintenance mode. Time will tell if people will vote with their feet.

ngnpope commented 1 year ago

Django has had async support in the works for a long time and it is finally here. Django 4.2 will be the first version with psycopg3, enabling async ORM in addition to async view from 4.1.

This is grossly misleading. Just adding support for psycopg3 does not mean that we have an async ORM.

The state in Django 4.2 is that we will have support for one async-capable database client (psycopg3) and we also have an async API for the high level bits of the ORM. But these currently only call synchronous code wrapped in @sync_to_async. There is a whole bunch of stuff to still be implemented, not least async support in database backends and low level bits of the ORM, e.g. the query compiler, etc.

There is also stuff around async signals (see here) and also request.auser() (see here) which won't land until 5.0. I don't expect that async support will be complete until maybe 5.2, to be honest, assuming work keeps progressing...

zbmott commented 1 year ago

Hello! I've been working on a Django REST Framework project using adrf and recently decided to leverage drf-spectacular for automatic OpenAPI schema generation.

For my use case, I'm only using adrf.viewsets.ViewSet to provide async viewset actions, so my experience is admittedly somewhat limited. I encountered one problem when using extend_schema to annotate my viewset actions.

Here's an (extremely) contrived example:

from adrf.viewsets import ViewSet as AsyncViewSet

from drf_spectacular.utils import extend_schema, extend_schema_view

from myproject.models.question import Question, QuestionDetail as QuestionDetailSerializer

class BaseViewSet(AsyncViewSet):
   serializer_class = None

    async def retrieve(self, request, *pos, **kw):
        obj = await self.aget_object()
        return Response(self.serializer_class(obj).data)

@extend_schema_view(
    retrieve=extend_schema(responses={200: QuestionDetailSerializer})
)
class QuestionViewSet(BaseViewSet):
    queryset = Question.objects.all()
    serializer_class = QuestionDetailSerializer

Trying to retrieve the details about a Question will raise the following Exception:

Traceback (most recent call last):
  File "/Users/zmott/.local/share/virtualenvs/myproject-1bp1QUqp/lib/python3.11/site-packages/django/core/handlers/exception.py", line 55, in inner
    response = get_response(request)
               ^^^^^^^^^^^^^^^^^^^^^
  File "/Users/zmott/.local/share/virtualenvs/myproject-1bp1QUqp/lib/python3.11/site-packages/django/core/handlers/base.py", line 197, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/zmott/.pyenv/versions/3.11.4/lib/python3.11/concurrent/futures/_base.py", line 449, in result
    return self.__get_result()
           ^^^^^^^^^^^^^^^^^^^
  File "/Users/zmott/.pyenv/versions/3.11.4/lib/python3.11/concurrent/futures/_base.py", line 401, in __get_result
    raise self._exception
    ^^^^^^^^^^^^^^^^^^^^^
  File "/Users/zmott/.local/share/virtualenvs/myproject-1bp1QUqp/lib/python3.11/site-packages/adrf/viewsets.py", line 115, in async_view
    return await self.dispatch(request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/zmott/.local/share/virtualenvs/myproject-1bp1QUqp/lib/python3.11/site-packages/adrf/views.py", line 68, in async_dispatch
    self.response = self.finalize_response(request, response, *args, **kwargs)
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/zmott/.local/share/virtualenvs/myproject-1bp1QUqp/lib/python3.11/site-packages/rest_framework/views.py", line 423, in finalize_response
    assert isinstance(response, HttpResponseBase), (
    ^

Exception Type: AssertionError at /api/v1/questions/12/
Exception Value: Expected a `Response`, `HttpResponse` or `HttpStreamingResponse` to be returned from the view, but received a `<class 'coroutine'>`

Since methods are only(?) isolated when they're derived from a base class, this is the only use case that breaks. Defining QuestionViewSet.retrieve, for example, is sufficient to fix the problem.

The root of the problem is in drf_spectacular.drainage.isolate_view_method, which is not sufficiently careful to preserve the asyncness of the methods it isolates. I was able to fix this problem for myself by introspecting the subject method and wrapping it in an async decorator if it is a coroutine function. In other words, this

def isolate_view_method(view, method_name):
    ...
    @functools.wraps(method)
    def wrapped_method(self, request, *args, **kwargs):
        return method(self, request, *args, **kwargs)
    ...
    return wrapped_method

becomes this

def isolate_view_method(view, method_name):
    ...
    if inspect.iscoroutinefunction(method):
        @functools.wraps(method)
        async def wrapped_method(self, request, *args, **kwargs):
            return await method(self, request, *args, **kwargs)
    else:
        @functools.wraps(method)
        def wrapped_method(self, request, *args, **kwargs):
            return method(self, request, *args, **kwargs)
    ...
    return wrapped_method

I'll see what I can do about adding test coverage for this fix and contributing it back as a PR.

tfranzel commented 1 year ago

@zbmott is this the only failure with regards to adrf? That would be an easy fix and I have zero problems integrating that. I just wanted to say that refactoring half of spectacular is not an option, but a couple of minor changes shouldn't be an issue. If you attempt a PR, get early feedback to prevent wasting your time.

@ngnpope yeah sorry, you are absolutely right. My statement was very optimistic with a good dose of wishful thinking. Django is not there yet, but at least it is moving in the right direction with a reasonable velocity imho. Even if Django is trying to stay on top of the state of the art, by declaring DRF maintenance only, it will create a growing functionality gap and thus devaluing the whole stack imho.

kalsheriff commented 7 months ago

Guys i need help i am having this error using adrf: RuntimeWarning: coroutine 'User_Serializer.create' was never awaited for x in reversed(rel.split(sep)): when trying to create user. below is

class User_Serializer(Serializer):

class Meta:
    model = User
    fields = '__all__'

def get_fields(self, *args, **kwargs):
    fields = super().get_fields(*args, **kwargs)
    fields.pop('new_password', None)
    fields.pop('old_password', None)
    return fields

async def create(self, validated_data):
    # Hash the password asynchronously
    validated_data['password'] = await sync_to_async(make_password)(validated_data.get('password'))
    return await super().create(validated_data)

class User_Viewsets(ViewSet):

# 
async def post(self, request):
    serializer = User_Serializer(data=request.data)
    if serializer.is_valid():
        try:
            # Save valid data to the database asynchronously
            saved_instance = await sync_to_async(serializer.save)()

            # Check if the saved_instance is an instance of your model
            if isinstance(saved_instance, User):
                return Response({"message": "User created successfully"}, status=status.HTTP_201_CREATED)
            else:
                # Handle unexpected behavior (e.g., database error)
                return Response({"message": "Unexpected error during save"}, status=status.HTTP_500_INTERNAL_SERVER_ERROR)
        except Exception as e:
            # Handle specific exceptions (e.g., validation error)
            return Response({"message": str(e)}, status=status.HTTP_400_BAD_REQUEST)
    else:
        # Handle invalid data case
        return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)