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

Add GetResponseAsyncCallable? / typing of MiddlewareMixin #1269

Closed
blueyed opened this issue Nov 22, 2022 · 4 comments · Fixed by #1425
Closed

Add GetResponseAsyncCallable? / typing of MiddlewareMixin #1269

blueyed opened this issue Nov 22, 2022 · 4 comments · Fixed by #1425
Labels
bug Something isn't working enhancement New feature or request stubs Issues in stubs files (.pyi)

Comments

@blueyed
Copy link
Contributor

blueyed commented Nov 22, 2022

Given the following middleware, type checking fails:

error: Incompatible types in "await" (actual type "HttpResponse", expected type "Awaitable[Any]") [misc]
error: Returning Any from function declared to return "HttpResponse" [no-any-return]

from django.utils.decorators import async_only_middleware
from django.utils.deprecation import GetResponseCallable


@async_only_middleware
def my_middleware(get_response: "GetResponseCallable"):
    async def middleware(request: "HttpRequest") -> "HttpResponse":
        response = await get_response(request)
        # …
        return response
    return middleware

Adding and using AsyncGetResponseCallable fixes this:

diff --git i/django-stubs/utils/deprecation.pyi w/django-stubs/utils/deprecation.pyi
index e15e0ae0..fa5a8d5d 100644
--- i/django-stubs/utils/deprecation.pyi
+++ w/django-stubs/utils/deprecation.pyi
@@ -1,5 +1,5 @@
 from collections.abc import Callable
-from typing import Any, Protocol
+from typing import Any, Awaitable, Protocol

 from django.http.request import HttpRequest
 from django.http.response import HttpResponse
@@ -32,6 +32,9 @@ class DeprecationInstanceCheck(type):
 class GetResponseCallable(Protocol):
     def __call__(self, __request: HttpRequest) -> HttpResponse: ...

+class AsyncGetResponseCallable(Protocol):
+    def __call__(self, __request: HttpRequest) -> Awaitable[HttpResponse]: ...
+
 class MiddlewareMixin:
     get_response: GetResponseCallable
     def __init__(self, get_response: GetResponseCallable = ...) -> None: ...

This could also be handled in GetResponseCallable directly (via union), but would be less explicit then.

Maybe it is also a good idea to add support to it to MiddlewareMixin, where it would be typed based on the input of get_response passed to __init__ then.

@blueyed blueyed added the bug Something isn't working label Nov 22, 2022
@intgr intgr added enhancement New feature or request stubs Issues in stubs files (.pyi) labels Dec 6, 2022
@intgr
Copy link
Collaborator

intgr commented Dec 6, 2022

Sounds good. I think this type should live in django.utils.decorators, not deprecation.

In fact, the sync_and_async_middleware, sync_only_middleware and async_only_middleware decorators could have a stricter bound to ensure that the decorated function accepts the correct type of argument.

@RyanWalker277
Copy link
Contributor

RyanWalker277 commented Apr 4, 2023

In fact, the sync_and_async_middleware, sync_only_middleware and async_only_middleware decorators could have a stricter bound to ensure that the decorated function accepts the correct type of argument.

Looking at the definition here

https://github.com/django/django/blob/073b5fd40035759afaebe5c77de5269aee851643/django/utils/decorators.py#LL166-L190C16

sync_and_async_middleware, sync_only_middleware and async_only_middleware being decorators, take in a function and return the same so the type is preserved, hence the current definition looks good to me.

def sync_and_async_middleware(func: _CallableType) -> _CallableType: ...
def sync_only_middleware(func: _CallableType) -> _CallableType: ...
def async_only_middleware(func: _CallableType) -> _CallableType: ...

Please let me know if I am missing something. I'll raise a PR adding the AsyncGetResponseCallable stub after this.

@intgr
Copy link
Collaborator

intgr commented Apr 27, 2023

Huh? I think the #1425 PR is incomplete.

AsyncGetResponseCallable was added, but was not used anywhere. This isn't something that exists in Django, but the point was to use it to typehint Django's decorators, no?

@intgr intgr reopened this Apr 27, 2023
GabDug added a commit to GabDug/django-stubs that referenced this issue Jun 26, 2023
@intgr
Copy link
Collaborator

intgr commented Jun 27, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request stubs Issues in stubs files (.pyi)
Development

Successfully merging a pull request may close this issue.

3 participants