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:

  1. Create a simple project with the below views and URLs.
  2. GET /view1/ and observe that you see "hi" in the response
  3. 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 Natalia Bidart, 11 months ago

Cc: Carlton Gibson added
Component: UncategorizedUtilities
Summary: @method_decorator and iscoroutinefunction() fail to interact properly on async viewsMake django.utils.decorators.method_decorator work with async functions.
Triage Stage: UnreviewedAccepted
Type: UncategorizedNew feature

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 grow method_decorator so the decorator param is tested for iscoroutinefunction.

Accepting on that premise as a new feature.

comment:2 by Drew Winstel, 11 months ago

That makes sense. Thanks, Natalia!

comment:3 by Carlton Gibson, 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 Drew Winstel, 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 with a fully async def version: 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

Last edited 11 months ago by Drew Winstel (previous) (diff)

comment:5 by patrick, 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 Carlton Gibson, 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 Gaurav sah, 10 months ago

Owner: changed from nobody to Gaurav sah
Status: newassigned

comment:8 by Gaurav sah, 10 months ago

Owner: Gaurav sah removed
Status: assignednew

comment:9 by Vaarun Sinha, 5 months ago

Owner: set to Vaarun Sinha
Status: newassigned

comment:10 by Vaarun Sinha, 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 Carlton Gibson, 5 months ago

Needs tests: set

comment:12 by Carlton Gibson, 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 Vaarun Sinha, 5 months ago

Needs tests: unset
Patch needs improvement: unset

comment:14 by Carlton Gibson, 5 months ago

Needs documentation: unset

New PR looks ready for a review.

comment:15 by Natalia Bidart, 5 months ago

Needs tests: set
Patch needs improvement: set
Version: 5.0dev

Submitted PR would benefit from a wider test coverage and removing some of the (now) duplicated comments and code.

comment:16 by Vaarun Sinha, 4 months ago

Needs tests: unset
Patch needs improvement: unset

comment:17 by Natalia Bidart, 3 months ago

Triage Stage: AcceptedReady for checkin

comment:18 by nessita <124304+nessita@…>, 3 months ago

Resolution: fixed
Status: assignedclosed

In 884ce37:

Fixed #35083 -- Updated method_decorator to handle async methods.

Co-authored-by: Natalia <124304+nessita@…>
Co-authored-by: Carlton Gibson <carlton.gibson@…>

Note: See TracTickets for help on using tickets.
Back to Top