-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
Async implementation #8617
Changes from 15 commits
79ce07b
67ebe90
7d2b73a
8fa1b7c
daac20a
0a40010
ae84d97
351ea7d
0400834
5e3140a
472ca5b
7b18380
053eabf
f65e859
f16767f
c6cf2d7
ec1d754
de6df46
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -482,9 +482,9 @@ 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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We might want to keep this function named Which means the only reasonable name for the other function is probably There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Scratch that, overriding Don't think there's anything we can do about that. However, the comment above
em1208 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
""" | ||
`.dispatch()` is pretty much the same as Django's regular dispatch, | ||
`.sync_dispatch()` is pretty much the same as Django's regular dispatch, | ||
but with extra hooks for startup, finalize, and exception handling. | ||
""" | ||
self.args = args | ||
|
@@ -511,6 +511,46 @@ 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): | ||
""" | ||
`.async_dispatch()` is pretty much the same as Django's regular dispatch, | ||
except for awaiting the handler function and with extra hooks for startup, | ||
finalize, and exception handling. | ||
""" | ||
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): | ||
""" | ||
Dispatch checks if the view is async or not and uses the respective | ||
async or sync dispatch method. | ||
""" | ||
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. | ||
|
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 | ||
|
@@ -22,16 +25,36 @@ 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 Response({'method': 'POST', 'data': request.data}) | ||
elif request.method == 'PUT': | ||
return Response({'method': 'PUT', 'data': request.data}) | ||
elif request.method == 'PATCH': | ||
return Response({'method': 'PATCH', 'data': request.data}) | ||
kevin-brown marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
|
||
@api_view(['GET', 'POST', 'PUT', 'PATCH']) | ||
async def basic_async_view(request): | ||
if request.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}) | ||
|
||
|
||
class ErrorView(APIView): | ||
|
@@ -72,6 +95,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) | ||
|
@@ -86,6 +125,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) | ||
|
@@ -96,6 +151,74 @@ 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 | ||
|
||
|
||
@pytest.mark.skipif( | ||
django.VERSION < (4, 1), | ||
reason="Async view support requires Django 4.1 or higher", | ||
) | ||
em1208 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
class FunctionBasedAsyncViewIntegrationTests(TestCase): | ||
def setUp(self): | ||
self.view = basic_async_view | ||
|
||
def test_get_succeeds(self): | ||
request = factory.get('/') | ||
response = async_to_sync(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 = 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 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about
There was a problem hiding this comment.
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
.There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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