Opened 11 months ago
Closed 3 months ago
#35083 closed New feature (fixed)
Make django.utils.decorators.method_decorator work with async functions.
Reported by: | Drew Winstel | Owned by: | Vaarun Sinha |
---|---|---|---|
Component: | Utilities | Version: | dev |
Severity: | Normal | Keywords: | async |
Cc: | Carlton Gibson | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
I first discovered this while trying to enforce a CSRF cookie on a subclass of the [standard strawberry GraphQL view](https://strawberry.rocks/docs/integrations/django), but I was able to replicate this using exclusively plain Django code:
When trying to use a @method_decorator
to enforce a CSRF cookie on a parent class's method, the iscoroutinefunction
logic appears to incorrectly detect that the decorated view is indeed async (https://github.com/django/django/blob/74f7deec9e334a69bfbfdd068285618534198bd5/django/utils/decorators.py#L162-L192), leading to an exception: AttributeError: 'coroutine' object has no attribute 'set_cookie'
Steps to replicate:
- Create a simple project with the below views and URLs.
- GET
/view1/
and observe that you see "hi" in the response - GET
/view2/
and observe the above attribute error
Expected behavior:
- Steps 2 and 3 return identical results
# views.py from typing import Any from django.http.request import HttpRequest from django.http.response import HttpResponse from django.views import View from django.utils.decorators import method_decorator from django.views.decorators.csrf import ensure_csrf_cookie class View1(View): @method_decorator(ensure_csrf_cookie) async def dispatch( self, request: HttpRequest, *args: Any, **kwargs: Any ) -> HttpResponse: return await super().dispatch(request, *args, **kwargs) async def get(self, request: HttpRequest): return HttpResponse("hi") @method_decorator(ensure_csrf_cookie, name="dispatch") class View2(View1): pass
# urls.py import views # snip urlpatterns += [ path("/view1/", views.View1.as_view()), path("/view2/", view.View2.as_view()), ]
Thanks!
Change History (18)
comment:1 by , 11 months ago
Cc: | added |
---|---|
Component: | Uncategorized → Utilities |
Summary: | @method_decorator and iscoroutinefunction() fail to interact properly on async views → Make django.utils.decorators.method_decorator work with async functions. |
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → New feature |
comment:3 by , 11 months ago
This ticket seems correct (method_decorator
should work with async def
methods).
Out of interest though, View
standardly has a sync dispatch
, which you've overridden here:
class View1(View): @method_decorator(ensure_csrf_cookie) async def dispatch( self, request: HttpRequest, *args: Any, **kwargs: Any ) -> HttpResponse: return await super().dispatch(request, *args, **kwargs)
... so would method_decorator
work already for the usual cases, decorating dispatch
? 🤔
Likely off-topic but just for future context, Drew could you maybe briefly say how it comes up — I guess the Strawberry base class is async def
all the way... or ... 🤔?
Thanks.
comment:4 by , 11 months ago
Out of interest though, View standardly has a sync dispatch, which you've overridden here:
Right, that's because strawberry's dispatch()
is overridden as well: https://github.com/strawberry-graphql/strawberry/blob/main/strawberry/django/views.py#L266-L277
Likely off-topic but just for future context, Drew could you maybe briefly say how it comes up — I guess the Strawberry base class is async def all the way... or ... 🤔?
It does inherit from django.views.generic.View
, but dispatch()
is a clean break from the generic view's version.
Strawberry's view includes the @csrf_exempt()
decorator, but we (at my day job) need @ensure_csrf_cookie()
applied to that view because of weird node.js things that I don't even pretend to understand. :D
comment:5 by , 10 months ago
@Drew @Carlton is there anything we can do better in the Strawberry Django View? Happy to make changes there 😊
comment:6 by , 10 months ago
@Patrick — I don't think you need to. There's no reason why you can't reimplement dispatch
. (Of course if you can get by without doing so, that's less code, but I don't think it's an issue per se, and this is still an improvement we can make in Django.)
comment:7 by , 10 months ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:8 by , 10 months ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:9 by , 5 months ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:10 by , 5 months ago
I have a patch ready I am just making it ready (writing tests and comments for it) I will make a PR this week hopefully!
comment:11 by , 5 months ago
Needs tests: | set |
---|
comment:12 by , 5 months ago
Has patch: | set |
---|---|
Needs documentation: | set |
Patch needs improvement: | set |
Comments on the PR. A release note would also be needed.
comment:13 by , 4 months ago
Needs tests: | unset |
---|---|
Patch needs improvement: | unset |
comment:15 by , 4 months ago
Needs tests: | set |
---|---|
Patch needs improvement: | set |
Version: | 5.0 → dev |
Submitted PR would benefit from a wider test coverage and removing some of the (now) duplicated comments and code.
comment:16 by , 3 months ago
Needs tests: | unset |
---|---|
Patch needs improvement: | unset |
comment:17 by , 3 months ago
Triage Stage: | Accepted → Ready for checkin |
---|
Hello Drew, thanks for your report!
From my understanding of the code, the decorators in django.utils.decorators do not work with async functions (except for
make_middleware_decorator
). I believe this ticket is similar to #31949, #33882, and #35030. What's needed is to growmethod_decorator
so thedecorator
param is tested foriscoroutinefunction
.Accepting on that premise as a new feature.