Opened 3 weeks ago

Last modified 12 hours ago

#37065 assigned Bug

@method_decorator can't be used on async view at class-level with name="dispatch"

Reported by: Jacob Walls Owned by: Vishy Algo
Component: Utilities Version: 5.2
Severity: Normal Keywords: async
Cc: Carlton Gibson Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

@method_decorator(..., name="dispatch") doesn't work for async views that don't override dispatch(), because dispatch() is sync, which is enough to fool the logic in #35083.

  • tests/decorators/test_cache.py

    diff --git a/tests/decorators/test_cache.py b/tests/decorators/test_cache.py
    index 1aca6967e0..c3b1668a7c 100644
    a b  
     1from http import HTTPStatus
    12from inspect import iscoroutinefunction
    23from unittest import mock
    34
    from django.http import HttpRequest, HttpResponse  
    56from django.test import SimpleTestCase
    67from django.utils.decorators import method_decorator
    78from django.views.decorators.cache import cache_control, cache_page, never_cache
     9from django.views.generic import View
    810
    911
    1012class HttpRequestProxy:
    class NeverCacheDecoratorTest(SimpleTestCase):  
    217219        with self.assertRaisesMessage(TypeError, msg):
    218220            await MyClass().async_view(HttpRequestProxy(request))
    219221
     222    async def test_never_cache_method_decorator_http_request_async_view(self):
     223        @method_decorator(never_cache, name="dispatch")
     224        class MyClass(View):
     225            async def get(self, request):
     226                return HttpResponse()
     227
     228        request = HttpRequest()
     229        request.method = "GET"
     230        response = await MyClass().dispatch(request)
     231        self.assertEqual(response.status_code, HTTPStatus.OK)
     232
    220233    def test_never_cache_decorator_http_request_proxy(self):
    221234        class MyClass:
    222235            @method_decorator(never_cache)
ERROR: test_never_cache_method_decorator_http_request_async_view (decorators.test_cache.NeverCacheDecoratorTest.test_never_cache_method_decorator_http_request_async_view)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jwalls/my314/lib/python3.14/site-packages/asgiref/sync.py", line 325, in __call__
    return call_result.result()
           ~~~~~~~~~~~~~~~~~~^^
  File "/Library/Frameworks/Python.framework/Versions/3.14/lib/python3.14/concurrent/futures/_base.py", line 443, in result
    return self.__get_result()
           ~~~~~~~~~~~~~~~~~^^
  File "/Library/Frameworks/Python.framework/Versions/3.14/lib/python3.14/concurrent/futures/_base.py", line 395, in __get_result
    raise self._exception
  File "/Users/jwalls/my314/lib/python3.14/site-packages/asgiref/sync.py", line 365, in main_wrap
    result = await awaitable
             ^^^^^^^^^^^^^^^
  File "/Users/jwalls/django/tests/decorators/test_cache.py", line 230, in test_never_cache_method_decorator_http_request_async_view
    response = await MyClass().dispatch(request)
                     ~~~~~~~~~~~~~~~~~~^^^^^^^^^
  File "/Users/jwalls/django/django/utils/decorators.py", line 47, in _wrapper
    return bound_method(*args, **kwargs)
  File "/Users/jwalls/django/django/views/decorators/cache.py", line 80, in _view_wrapper
    add_never_cache_headers(response)
    ~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^
  File "/Users/jwalls/django/django/utils/cache.py", line 294, in add_never_cache_headers
    patch_response_headers(response, cache_timeout=-1)
    ~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/jwalls/django/django/utils/cache.py", line 285, in patch_response_headers
    if not response.has_header("Expires"):
           ^^^^^^^^^^^^^^^^^^^
AttributeError: 'coroutine' object has no attribute 'has_header'

Moving the decorator to the method instead of the class works. We probably will prefer a test inside the test class added in #35083 rather than merging my sketched test.

Change History (9)

comment:1 by Vishy Algo, 3 weeks ago

Owner: set to Vishy Algo
Status: newassigned

comment:2 by Sarah Boyce, 3 weeks ago

Triage Stage: UnreviewedAccepted

comment:3 by Vishy Algo, 6 days ago

Has patch: set

comment:4 by JaeHyuckSa, 5 days ago

Patch needs improvement: set

in reply to:  description comment:5 by Vishy Algo, 21 hours ago

Hi Jacob,

Please take a look at the PR and let me know whether patching View with __init_subclass__ would be an appropriate fix, or if it would be considered out of scope for this ticket.

comment:6 by Carlton Gibson, 17 hours ago

Cc: Carlton Gibson added

comment:7 by Carlton Gibson, 15 hours ago

This is a doozy. I'm not convinced it's tractable... (at reasonable cost/complexity to method_decorator).

If the method being decorated is not async then it's not 100% clear to me that method_decorator should do any further introspection to determine if the return value should be marked as a coroutine.

We're asking it to decorate a method, and #35083 was about allowing those methods to be async. Both conjuncts work.

Moving the decorator to the method instead of the class works.

A further know significant details about the particulars of View strikes me as out of scope for method_decorator.

If I had to decorate dispatch for an async view class, I think this is probably the best option:

    @method_decorator(never_cache, name="dispatch")
    class MyClass(View):
        async def dispatch(self, *args, **kwargs):
            return super().dispatch(*args, **kwargs)    

 	async def get(self, request):
 	     return HttpResponse()

(Not tested. You get the idea)

This somewhat mitigates using the name="dispatch" — so I'd probably put it on the method directly, and I'd only need to do that if we also had say async def post as well.

But I think trying to automagically our way forward here is mistake. It's complex. It just is.

So I think a docs tweak is probably the way to go.

comment:8 by Vishy Algo, 14 hours ago

I completely agree, inspecting the return value and patching feels more like over-engineering than necessary.

But would async get dispatch cause any issues, if handled incorrectly! Cause, when just overriding dispatch (without http methods) also seems to be a mistake.

Probably as_view should also be overridden (again when all http methods are sync, view_is_async is False).

comment:9 by Carlton Gibson, 12 hours ago

It makes no sense to override dispatch to be async except in this case.

Version 0, edited 12 hours ago by Carlton Gibson (next)
Note: See TracTickets for help on using tickets.
Back to Top