Opened 22 months ago
Closed 14 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 , 22 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 , 22 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 , 22 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 , 21 months ago
@Drew @Carlton is there anything we can do better in the Strawberry Django View? Happy to make changes there 😊
comment:6 by , 21 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 , 21 months ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
comment:8 by , 21 months ago
| Owner: | removed |
|---|---|
| Status: | assigned → new |
comment:9 by , 17 months ago
| Owner: | set to |
|---|---|
| Status: | new → assigned |
comment:10 by , 17 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 , 16 months ago
| Needs tests: | set |
|---|
comment:12 by , 16 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 , 16 months ago
| Needs tests: | unset |
|---|---|
| Patch needs improvement: | unset |
comment:15 by , 16 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 , 15 months ago
| Needs tests: | unset |
|---|---|
| Patch needs improvement: | unset |
comment:17 by , 14 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_decoratorso thedecoratorparam is tested foriscoroutinefunction.Accepting on that premise as a new feature.