Use MiddlewareType as a type hint for middlewares

[Viicos]

This might brake things in other repositories (fastapi for instance), but this improves type safety in my opinion.

[Kludex]

I think this was already discussed before. 🤔

[Viicos]

I think this was already discussed before. 🤔

I've tried looking in the already existing issues/PRs/discussions but couldn't find anything 😕

[Kludex]

• [typing] Allow non-class callables to be used as middleware #1176

EDIT: I think my comment says that we should be doing what you are trying to improve, but that PR was doing too many things... 👀

EDIT 2: This doesn't mean that I agree with those changes, I didn't even check them yet... jfyk

[Viicos]

Note to self: import Protocol for typing_extensions to support 3.7

[Viicos]

Waiting for directives regarding the coverage fail.

[Kludex]

This might brake things in other repositories (fastapi for instance), but this improves type safety in my opinion.

What does this break on FastAPI?

[alex-oleshkevich]

Middlewares are just ASGI applications. ASGIApp type hint works for them as well.

[Viicos]

This might brake things in other repositories (fastapi for instance), but this improves type safety in my opinion.

What does this break on FastAPI?

Sorry I wasn't clear on that point, it doesn't break anything for projects that depend on starlette, but it might break type checking if one is passing an unknown type to add_middleware

Middlewares are just ASGI applications. ASGIApp type hint works for them as well.

True indeed, I went for the Protocol definition but it looks like using Callable is equivalent (and doesn't require typing_extensions). Having an explicit type for middlewares improve readability though, so maybe we could use something like:

Middleware: TypeAlias = ASGIApp

This is up to you

[Kludex]

Is TypeAlias needed?

[Viicos]

See https://peps.python.org/pep-0613/. Here it does not add value, as it would be implicitly determined as a TypeAlias any way, but as typing_extensions is used in this project I figured why not use it, that way we know explicitly it is a TypeAlias.

[Kludex]

Should we just use ASGIApp instead of creating this new type? 🤔

[Viicos]

Should we just use ASGIApp instead of creating this new type? 🤔

This is purely to avoid confusion, and to avoid having people thinking they need to pass in a ASGIApp in the Middleware's __init__ constructor, even though as stated by @alex-oleshkevich they are the same 🙂

[alex-oleshkevich]

@Kludex after thinking about this, I found that it makes sense to have dedicated types for such components. It makes things very specific. It should not affect only middlewares, but any other component we may find important.
MiddlewareType, ContextProcessorType, another other callback type, etc. In the same way, we have types for Receive and Send.

I would not drop this PR but rather initiate a deeper discussion on this topic.

[odiseo0]

Is MiddewareType the correct name? It has a typo in it, or is intended?

[abersheeran]

https://github.com/encode/starlette/blob/337ae243b10d3c20db53b77bc3c7718bb4f1a164/starlette/applications.py#L103-L104

Considering this code, should we use

class MiddlewareType(Protocol):
    def __call__(
        self,
        app: ASGIApp,
        **options: Any
    ) -> ASGIApp:
        ...

[Viicos]

If you look at the __iter__ method of the Middleware class, you will see that it returns a tuple whose first element is the middleware class, which corresponds to the cls variable here

[abersheeran]

cls is MiddlewareType.

[Viicos]

Yes indeed sorry, there's some confusion here between the __init__ and the __call__ method. When doing app = cls(app=app, **options), we are actually instantiating the middleware, not using the __call__ method. (That is because cls is Type[MiddlewareType], not MiddlewareType).

Defining the MiddlewareType as a Callable or as a Protocol implementing __call__ is equivalent, as Callable is an ABC implementing __call__

[abersheeran]

This is a ASGI middleware that can be used in starlette.

class ASGIMiddleware:
    def __init__(app: ASGIApp, p0: str, p1: int) -> None:
        ...
    async def __call__(self, scope, receive, send) -> None:
        ...

This also can be used in starlette.

def asgi_middleware(app, p0: str, p1: int) -> ASGIApp:
    async def _asgi(scope, receive, send) -> None:
        ...
    return _asgi

But this can't be used in starlette. Even though P passes the type check you're giving now.

class P:
    def __init__(self) -> None: ...
    def __call__(self, scope, receive, send) -> None: ...

[abersheeran]

class MyMiddleware:
    def __init__(self, app: ASGIApp, **options: Any) -> None:
        ...

    async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:
        ...

This class passes the following type checks.

class MiddlewareType(Protocol):
    def __call__(
        self,
        app: ASGIApp,
        **options: Any
    ) -> ASGIApp:
        ...

[Viicos]

It might, but imo this is not the way to go:

• You are using the __call__ method as a way to define the signature of the __init__ method of a middleware, which is confusing
• the MiddlewareType you created does not have any signature for the real async __call__ method that should be defined on a middleware class

[abersheeran]

If you don't know how to use typing.Protocol, please consider using mypy to give it a try. 😊

[Viicos]

I know how Protocols work, but you haven't explained why you are using these parameters for the __call__ method (see my answers above). As I understand it currently, you are using the __call__ method of your protocol as a way to define the __init__ method of a middleware class, right?

[Viicos]

Will continue discussion in main thread for clarity. Sorry for the confusion, it seems your suggestion is working, I couldn't get it to work since I was still on mypy==0.991.

[Viicos]

@abersheeran sorry for the delay, this is how it looks now:

class MiddlewareType(Protocol):
    __name__: str

    def __call__(self, *args: typing.Any, **kwargs: typing.Any) -> ASGIApp:  # pragma: no cover
        ...

Unfortunately, I can't enforce the app argument to be there, see the linked mypy issue for more detail. If you have a better solution, do not hesitate

[abersheeran]

class MiddlewareType(Protocol):
    def __call__(
        self,
        app: ASGIApp,
        **options: Any
    ) -> ASGIApp:
        ...

Be more precise.

[Viicos]

class MiddlewareType(Protocol):
    def __call__(
        self,
        app: ASGIApp,
        **options: Any
    ) -> ASGIApp:
        ...

Be more precise.

This doesn't work, as I said, more details in the linked mypy issue. For instance, it won't match against this one:

https://github.com/encode/starlette/blob/01aa49a379520d3bbe6bc1aecc48a48169e6b004/starlette/middleware/errors.py#L125-L142

as it would expect ServerErrorMiddleware to have **kwargs in the __init__ method

[abersheeran]

https://github.com/encode/starlette/blob/01aa49a379520d3bbe6bc1aecc48a48169e6b004/starlette/applications.py#L95-L107

We can modify its definition a bit. Because it is like this when it is used.

[Viicos]

We can modify its definition a bit. Because it is like this when it is used.

Apart from moving handler: typing.Optional[typing.Callable] = None, debug: bool = False to **kwargs, it won't match the MiddlewareType with **options: Any, and I don't think it's convenient to have it this way 🤔

My opinion on this is while the __init__ method is incomplete, and middlewares without an app: ASGIApp argument will pass type checking, we at least make sure the __call__ method is correct (or the returned callable in the case of a nested callable). Plus it makes a step towards supporting #1176, which had a similar solution, apart from the fact that it was missing the __name__ attribute.

[abersheeran]

My meaning is to modify the code to

    def __init__( 
         self, 
         app: ASGIApp, 
         *,
         handler: typing.Optional[typing.Callable] = None, 
         debug: bool = False, 
     ) -> None: 

[Kludex]

I'm not following the discussion, but I guess is @Viicos turn. 👀

[Viicos]

Yes I'll probably take a look this afternoon 👌

[Viicos]

Unfortunately this doesn't seem to work either:
https://mypy-play.net/?mypy=master&python=3.11&gist=0f955a8ee146e68a1410d7bd9167726d

main.py:20: note: Revealed type is "def (builtins.str)"
main.py:22: note: Revealed type is "None"
main.py:24: error: Argument 1 to "test" has incompatible type "type[ExampleMiddleware]"; expected "MiddlewareType"  [arg-type]
main.py:24: note: Following member(s) of "ExampleMiddleware" have conflicts:
main.py:24: note:     Expected:
main.py:24: note:         def __call__(app: int, **options: Any) -> Callable[[str], None]
main.py:24: note:     Got:
main.py:24: note:         def __init__(app: int, *, other_arg: int | None = ...) -> ExampleMiddleware

But even if it did, I'm not sure having people defining their own middlewares to explicitly use the * argument is good idea 🤔

[abersheeran]

Unfortunately this doesn't seem to work either: https://mypy-play.net/?mypy=master&python=3.11&gist=0f955a8ee146e68a1410d7bd9167726d

main.py:20: note: Revealed type is "def (builtins.str)"
main.py:22: note: Revealed type is "None"
main.py:24: error: Argument 1 to "test" has incompatible type "type[ExampleMiddleware]"; expected "MiddlewareType"  [arg-type]
main.py:24: note: Following member(s) of "ExampleMiddleware" have conflicts:
main.py:24: note:     Expected:
main.py:24: note:         def __call__(app: int, **options: Any) -> Callable[[str], None]
main.py:24: note:     Got:
main.py:24: note:         def __init__(app: int, *, other_arg: int | None = ...) -> ExampleMiddleware

But even if it did, I'm not sure having people defining their own middlewares to explicitly use the * argument is good idea 🤔

😯 The results of mypy are a bit beyond my imagination.

[abersheeran]

Limited by the inference ability of mypy, I temporarily agree with the solution given by this PR. 🙏 Thank you all for your patient communication.

[Viicos]

Thanks for the reviews @abersheeran and sorry for taking some time to respond. Typing callables can sometimes be a pain, but hopefully with ParamSpec and Concatenate this will be easier in the future

[Kludex]

Pyright doesn't seem to like the proposed changes from this PR. 🤔

[abersheeran]

Pyright doesn't seem to like the proposed changes from this PR. 🤔

Are there plans to add pyright to CI?

[Viicos]

Pyright doesn't seem to like the proposed changes from this PR. 🤔

Pyright doesn't seem to support *args: Any, **kwargs: Any treated as any callable (mypy has it with python/mypy#5876). We could switch to MiddlewareType = Callable[..., ASGIApp], but then loose the __name__ attribute that is required by starlette 😕

[Kludex]

Pyright doesn't seem to like the proposed changes from this PR. 🤔

Are there plans to add pyright to CI?

No, but since httpx is now using mypy strict, we can follow it and do the same here.

[Viicos]

Pyright doesn't seem to support *args: Any, **kwargs: Any treated as any callable (mypy has it with python/mypy#5876). We could switch to MiddlewareType = Callable[..., ASGIApp], but then loose the __name__ attribute that is required by starlette 😕

The other alternative would be to switch back to what I did at first:

class MiddlewareType(Protocol):
    async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:
        ...

This is equivalent to ASGIApp, but needs to be defined as a Protocol and not a Callable, otherwise Type[] couldn't be used.

We can also keep things as is if we don't want to support pyright yet.

[Kludex]

We can also keep things as is if we don't want to support pyright yet.

Let's first do what httpx did, and use mypy on strict mode. Then we can see what errors we have with pyright.

For the time being, I'll close this PR.

Thanks for the patience @Viicos