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 view implementation #8978

Closed
wants to merge 1 commit into from
Closed

Conversation

jameshilliard
Copy link

Description

This enables async views for #8496. Tests are copied from #8617 but the implementation is different and more aggressively tries to avoid async request paths hitting sync_to_async by additionally autodetecting async permission and throttle methods while also avoiding a separate async/sync dispatch class method.

This auto-detection approach seems to be how django itself handles async views.

@alexandrubese
Copy link

alexandrubese commented May 13, 2023

Hi @jameshilliard ,
This is great work !
Is there a timeline as to when is this going to get merged and available ?

@jameshilliard jameshilliard force-pushed the async branch 3 times, most recently from 7108cae to 244732d Compare May 15, 2023 07:15
@@ -11,6 +11,10 @@
from django.test.client import Client as DjangoClient
from django.test.client import ClientHandler
from django.test.client import RequestFactory as DjangoRequestFactory

if django.VERSION >= (4, 1):
from django.test.client import RequestFactory as DjangoAsyncRequestFactory
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be AsyncRequestFactory.

Copy link
Author

Choose a reason for hiding this comment

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

fixed

@jameshilliard jameshilliard force-pushed the async branch 5 times, most recently from 304c154 to 38b166e Compare June 13, 2023 22:46
@auvipy auvipy self-requested a review June 14, 2023 06:06
@jameshilliard jameshilliard force-pushed the async branch 3 times, most recently from 830b266 to d269dbb Compare June 15, 2023 21:47
@jameshilliard
Copy link
Author

Is there a timeline as to when is this going to get merged and available ?

Not sure, I think it shouldn't cause regressions at least since the new codepaths only get used in cases that would currently trigger hard failures.

Comment on lines 248 to 349
default_format = api_settings.TEST_REQUEST_DEFAULT_FORMAT

def __init__(self, enforce_csrf_checks=False, **defaults):
self.enforce_csrf_checks = enforce_csrf_checks
self.renderer_classes = {}
for cls in self.renderer_classes_list:
self.renderer_classes[cls.format] = cls
super().__init__(**defaults)

def _encode_data(self, data, format=None, content_type=None):
"""
Encode the data returning a two tuple of (bytes, content_type)
"""

if data is None:
return ('', content_type)

assert format is None or content_type is None, (
'You may not set both `format` and `content_type`.'
)

if content_type:
# Content type specified explicitly, treat data as a raw bytestring
ret = force_bytes(data, settings.DEFAULT_CHARSET)

else:
format = format or self.default_format

assert format in self.renderer_classes, (
"Invalid format '{}'. Available formats are {}. "
"Set TEST_REQUEST_RENDERER_CLASSES to enable "
"extra request formats.".format(
format,
', '.join(["'" + fmt + "'" for fmt in self.renderer_classes])
)
)

# Use format and render the data into a bytestring
renderer = self.renderer_classes[format]()
ret = renderer.render(data)

# Determine the content-type header from the renderer
content_type = renderer.media_type
if renderer.charset:
content_type = "{}; charset={}".format(
content_type, renderer.charset
)

# Coerce text to bytes if required.
if isinstance(ret, str):
ret = ret.encode(renderer.charset)

return ret, content_type

def get(self, path, data=None, **extra):
r = {
'QUERY_STRING': urlencode(data or {}, doseq=True),
}
if not data and '?' in path:
# Fix to support old behavior where you have the arguments in the
# url. See #1461.
query_string = force_bytes(path.split('?')[1])
query_string = query_string.decode('iso-8859-1')
r['QUERY_STRING'] = query_string
r.update(extra)
return self.generic('GET', path, **r)

def post(self, path, data=None, format=None, content_type=None, **extra):
data, content_type = self._encode_data(data, format, content_type)
return self.generic('POST', path, data, content_type, **extra)

def put(self, path, data=None, format=None, content_type=None, **extra):
data, content_type = self._encode_data(data, format, content_type)
return self.generic('PUT', path, data, content_type, **extra)

def patch(self, path, data=None, format=None, content_type=None, **extra):
data, content_type = self._encode_data(data, format, content_type)
return self.generic('PATCH', path, data, content_type, **extra)

def delete(self, path, data=None, format=None, content_type=None, **extra):
data, content_type = self._encode_data(data, format, content_type)
return self.generic('DELETE', path, data, content_type, **extra)

def options(self, path, data=None, format=None, content_type=None, **extra):
data, content_type = self._encode_data(data, format, content_type)
return self.generic('OPTIONS', path, data, content_type, **extra)

def generic(self, method, path, data='',
content_type='application/octet-stream', secure=False, **extra):
# Include the CONTENT_TYPE, regardless of whether or not data is empty.
if content_type is not None:
extra['CONTENT_TYPE'] = str(content_type)

return super().generic(
method, path, data, content_type, secure, **extra)

def request(self, **kwargs):
request = super().request(**kwargs)
request._dont_enforce_csrf_checks = not self.enforce_csrf_checks
return request
Copy link
Contributor

Choose a reason for hiding this comment

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

This may be refactored along with APIRequestFactory to have a common mixin to avoid having duplicated code.

Copy link
Author

Choose a reason for hiding this comment

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

good idea, applied as suggested

Choose a reason for hiding this comment

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

Hi. I'd like to know if DRF is planning to include this contribuition @sevdog. I've browse the refered issues and/or pull request listed by @jameshilliard and his work looks pretty complete. Nevertheless, running async code with DRF in 2023 may be something expected by many people.

Copy link
Contributor

Choose a reason for hiding this comment

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

hi @ernestomancebo, I would like to have this feature (and maybe something more to handle async code also in DRF). However the final decision is up to those who owns/manage this repository, I am very interested in this project but as of now I am just a contributor.

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to know if DRF is planning to include this contribuition

It is not, no.

You can review the conversation around this at #8496.

@tomchristie
Copy link
Member

Closing as per #8496. See https://github.com/em1208/adrf for support here.

(Sorry. 😔 It's my job as maintainer to do a lot of saying "no", even when contributors are clearly enthusiastic and invested.)

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.

5 participants