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

Async implementation #8617

Closed
wants to merge 18 commits into from
Closed
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions rest_framework/compat.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,13 @@ def distinct(queryset, base):
uritemplate = None


# async_to_sync is required for async view support
if django.VERSION >= (4, 1):
from asgiref.sync import async_to_sync
else:
async_to_sync = None

Choose a reason for hiding this comment

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

How about

def async_to_sync(*args, **kwargs):
    raise NotImplementedError("DRF async only supports Django >= 4.1")

Copy link

@dongyuzheng dongyuzheng Sep 1, 2022

Choose a reason for hiding this comment

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

A type checker would not be happy about async_to_sync = None.

Copy link
Author

Choose a reason for hiding this comment

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

async_to_sync is available in Django since version 3.1. In this case I'm using it just to run the test. If pytest-asyncio is added then it can be removed.

Copy link
Member

Choose a reason for hiding this comment

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

I would personally rather we take a testing dependency on pytest-asyncio rather than implement a test-only compat helper.

Choose a reason for hiding this comment

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

I believe it's best to defer the decision to add an additional dependency to project leads. Especially if the new dependency can be easily circumvented, such as in this case.

Copy link

Choose a reason for hiding this comment

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

As an a user of drf I would not happy to have any async tests related dependency in my project



# coreschema is optional
try:
import coreschema
Expand Down
37 changes: 36 additions & 1 deletion rest_framework/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -482,7 +482,7 @@ def raise_uncaught_exception(self, exc):
# Note: Views are made CSRF exempt from within `as_view` as to prevent
# accidental removal of this exemption in cases where `dispatch` needs to
# be overridden.
def dispatch(self, request, *args, **kwargs):
def sync_dispatch(self, request, *args, **kwargs):

Choose a reason for hiding this comment

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

We might want to keep this function named dispatch to prevent people's overrides from breaking.

Which means the only reasonable name for the other function is probably adispatch.

Copy link

@Archmonger Archmonger Aug 31, 2022

Choose a reason for hiding this comment

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

Scratch that, overriding dispatch would still work for most people with the current implementation, but would definitely lead to some unexpected behavior if they're trying to upgrade to async.

Don't think there's anything we can do about that.

However, the comment above sync_dispatch may need to be updated now.

em1208 marked this conversation as resolved.
Show resolved Hide resolved
"""
`.dispatch()` is pretty much the same as Django's regular dispatch,
but with extra hooks for startup, finalize, and exception handling.
Expand Down Expand Up @@ -511,6 +511,41 @@ def dispatch(self, request, *args, **kwargs):
self.response = self.finalize_response(request, response, *args, **kwargs)
return self.response

async def async_dispatch(self, request, *args, **kwargs):
"""
`.dispatch()` is pretty much the same as Django's regular dispatch,
but with extra hooks for startup, finalize, and exception handling.
"""

Choose a reason for hiding this comment

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

Needs a docstring update

Copy link
Author

Choose a reason for hiding this comment

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

Updated.

self.args = args
self.kwargs = kwargs
request = self.initialize_request(request, *args, **kwargs)
self.request = request
self.headers = self.default_response_headers # deprecate?

try:
self.initial(request, *args, **kwargs)

# Get the appropriate handler method
if request.method.lower() in self.http_method_names:
handler = getattr(self, request.method.lower(),
self.http_method_not_allowed)
else:
handler = self.http_method_not_allowed

response = await handler(request, *args, **kwargs)

except Exception as exc:
response = self.handle_exception(exc)

self.response = self.finalize_response(request, response, *args, **kwargs)
return self.response

def dispatch(self, request, *args, **kwargs):
if hasattr(self, 'view_is_async') and self.view_is_async:
tomchristie marked this conversation as resolved.
Show resolved Hide resolved
return self.async_dispatch(request, *args, **kwargs)
else:
return self.sync_dispatch(request, *args, **kwargs)

def options(self, request, *args, **kwargs):
"""
Handler method for HTTP 'OPTIONS' request.
Expand Down
85 changes: 81 additions & 4 deletions tests/test_views.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
import copy

import django
import pytest
from django.test import TestCase

from rest_framework import status
from rest_framework.compat import async_to_sync
from rest_framework.decorators import api_view
from rest_framework.response import Response
from rest_framework.settings import APISettings, api_settings
Expand All @@ -22,16 +25,24 @@ def post(self, request, *args, **kwargs):
return Response({'method': 'POST', 'data': request.data})


class BasicAsyncView(APIView):
async def get(self, request, *args, **kwargs):
return Response({'method': 'GET'})

async def post(self, request, *args, **kwargs):
return Response({'method': 'POST', 'data': request.data})


@api_view(['GET', 'POST', 'PUT', 'PATCH'])
def basic_view(request):
if request.method == 'GET':
return {'method': 'GET'}
return Response({'method': 'GET'})
elif request.method == 'POST':
return {'method': 'POST', 'data': request.data}
return Response({'method': 'POST', 'data': request.data})
elif request.method == 'PUT':
return {'method': 'PUT', 'data': request.data}
return Response({'method': 'PUT', 'data': request.data})
elif request.method == 'PATCH':
return {'method': 'PATCH', 'data': request.data}
return Response({'method': 'PATCH', 'data': request.data})
kevin-brown marked this conversation as resolved.
Show resolved Hide resolved


class ErrorView(APIView):
Expand Down Expand Up @@ -72,6 +83,22 @@ class ClassBasedViewIntegrationTests(TestCase):
def setUp(self):
self.view = BasicView.as_view()

def test_get_succeeds(self):
request = factory.get('/')
response = self.view(request)
assert response.status_code == status.HTTP_200_OK
assert response.data == {'method': 'GET'}

def test_post_succeeds(self):
request = factory.post('/', {'test': 'foo'})
response = self.view(request)
expected = {
'method': 'POST',
'data': {'test': ['foo']}
}
assert response.status_code == status.HTTP_200_OK
assert response.data == expected

def test_400_parse_error(self):
request = factory.post('/', 'f00bar', content_type='application/json')
response = self.view(request)
Expand All @@ -86,6 +113,22 @@ class FunctionBasedViewIntegrationTests(TestCase):
def setUp(self):
self.view = basic_view

def test_get_succeeds(self):
request = factory.get('/')
response = self.view(request)
assert response.status_code == status.HTTP_200_OK
assert response.data == {'method': 'GET'}

def test_post_succeeds(self):
request = factory.post('/', {'test': 'foo'})
response = self.view(request)
expected = {
'method': 'POST',
'data': {'test': ['foo']}
}
assert response.status_code == status.HTTP_200_OK
assert response.data == expected

def test_400_parse_error(self):
request = factory.post('/', 'f00bar', content_type='application/json')
response = self.view(request)
Expand All @@ -96,6 +139,40 @@ def test_400_parse_error(self):
assert sanitise_json_error(response.data) == expected


@pytest.mark.skipif(
django.VERSION < (4, 1),
kevin-brown marked this conversation as resolved.
Show resolved Hide resolved
reason="Async view support requires Django 4.1 or higher",
)
class ClassBasedAsyncViewIntegrationTests(TestCase):
def setUp(self):
self.view = BasicAsyncView.as_view()

def test_get_succeeds(self):
request = factory.get('/')
response = async_to_sync(self.view)(request)

Choose a reason for hiding this comment

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

Should this pytest ext be added?

https://pypi.org/project/pytest-asyncio/

I'd expect more async code be added to this repo in the future.

Copy link
Author

Choose a reason for hiding this comment

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

I'll be happy to add it if there is an agreement instead of using async_to_sync

Choose a reason for hiding this comment

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

I'm personally indifferent. I tend to avoid additional dependencies when possible, so I personally support using asgiref.async_to_sync instead of pytest-asyncio.

I don't expect this repo to have much more in terms of asyncio related tests beyond this, since we're already covering Django's CBV and FBV.

Unless Tom Christie feels otherwise, I'd say it's safe to stay as-is.

assert response.status_code == status.HTTP_200_OK
assert response.data == {'method': 'GET'}

def test_post_succeeds(self):
request = factory.post('/', {'test': 'foo'})
response = async_to_sync(self.view)(request)
expected = {
'method': 'POST',
'data': {'test': ['foo']}
}
assert response.status_code == status.HTTP_200_OK
assert response.data == expected

def test_400_parse_error(self):
request = factory.post('/', 'f00bar', content_type='application/json')
response = async_to_sync(self.view)(request)
expected = {
'detail': JSON_ERROR
}
assert response.status_code == status.HTTP_400_BAD_REQUEST
assert sanitise_json_error(response.data) == expected


class TestCustomExceptionHandler(TestCase):
def setUp(self):
self.DEFAULT_HANDLER = api_settings.EXCEPTION_HANDLER
Expand Down