Add typing test for Websockets exception_handler #2042
Conversation
fcdbda3 to
c542f6d
Compare
c542f6d to
d0af507
Compare
tests/test_applications.py
Outdated
| @@ -81,7 +81,8 @@ async def websocket_raise_custom(websocket: WebSocket): | |||
| raise CustomWSException() | |||
|
|
|||
|
|
|||
| def custom_ws_exception_handler(websocket: WebSocket, exc: CustomWSException): | |||
| def custom_ws_exception_handler(websocket: WebSocket, exc: Exception) -> None: | |||
There was a problem hiding this comment.
Was there an issue with the typing on exc?
There was a problem hiding this comment.
The type was not being checked. Once type checking was enabled, it was clear that having the CustomWSException is incorrect (as the type spec on Starlette(exception_handlers=...) specifies that the second argument can be any Exception instance, not just CustomWSException.
There was a problem hiding this comment.
Mypy complains that element 4 of the dict literal is the wrong type.
Look, if you specify a callable which takes a certain type as an argument, you cannot provide a callable which expects a narrower type.
This here is invalid typing:
from typing import Callable
def set_callback(callback: Callable[[Exception], None]) -> None:
pass
def my_callback(arg: RuntimeError)->None:
pass
set_callback(my_callback) # errorsrunning mypy on this results in:
test.py:8: error: Argument 1 to "set_callback" has incompatible type "Callable[[RuntimeError], None]"; expected "Callable[[Exception], None]" [arg-type]
tests/test_applications.py
Outdated
| exception_handlers={ | ||
| 500: error_500, | ||
| 405: method_not_allowed, | ||
| HTTPException: http_exception, | ||
| CustomWSException: custom_ws_exception_handler, | ||
| }, |
There was a problem hiding this comment.
I'm no expert on MyPy, and how to make it test types, but without it, the exception_handlers variable was apparently untyped, and mypy didn't complain about the contents of it not matching the specification. Maybe there is a better way tom make mypy be more strict in checking types in the unittests. the --strict mode does exists but that is completely overboard for unittests, IMHO.
By putting the dict in-line, mypy kicks into gear and starts testing to see if the contents of the dict actually matches what the type hints for Starlette() specify.
There was a problem hiding this comment.
I don't think that makes sense. How did you prove this assumption?
There was a problem hiding this comment.
by running scripts/lint.
My guess is the following: MyPy will check that the type of a literal matches the argument list, but it will not automatically and implicitly assign a type to a variable (exception_handers) based on the literal used to initialize it. Since the variable has no type, it won't be checked against the argument list, and mypy isn't in --strict mode and doesn't complain about it.
Anyway, you can clearly see that the type of custom_ws_exception_handler, does not match the hints, (For one thing, it specifies a WebSocket argument where Starlette() expects callbacks taking a Request) yet mypy does not complain about this dict.
There was a problem hiding this comment.
The problem was that some of the exception handlers were untyped (ANY) and then the whole list collapses to that and even your carefully typed exception handlers don't get any testing.
|
I think there are two issues that maybe should be solved independently:
|
|
Ok, I think I have figured out what is happening, why we are not getting warnings. In other words: MyPy has implicitly typed By adding type hints to all of the exception handlers, MyPy gets clued in and it fails: I think the best course of action here is to excplicitly define exception handlers as being of type |
|
This file demonstrates the issue. I guess I'll need to raise an issue over at MyPy. from typing import Callable, Any, Mapping
def api(handlers: Mapping[Any, Callable[[str], int]]) -> None:
pass
def handler1(arg):
return 1
def handler2(arg:int) -> str:
return "foo"
handlers = {
1: handler1,
2: handler2
}
api(handlers) |
|
Well, MyPy promptly closed the issue without discussion. I guess that's that then. My suggestion, then, is to simply make sure that we don't add callables to intermediate dicts, to ensure that the typing information of the individual handlers is actually checked against the spec. |
|
We've kinda been through this before: #1308. I think the best thing we could do is something like this: https://mypy-play.net/?mypy=latest&python=3.11&gist=ace307dc705717b1ef1fedc21708b8f3 Which would be a pure addition, nothing inside Or we could not inherit from |
Yes, a TypeVar would allow to have typed handlers with their own exception subclasses. |
starlette/applications.py
Outdated
| typing.Union[ | ||
| typing.Callable[ | ||
| [Response, Exception], | ||
| typing.Union[Response, typing.Awaitable[Response]], | ||
| ], | ||
| typing.Callable[ | ||
| [WebSocket, Exception], | ||
| typing.Union[None, typing.Awaitable[None]], | ||
| ], |
There was a problem hiding this comment.
Can we revert everything else in this PR and only maintain this change?
There was a problem hiding this comment.
sure. Except, in stead of Exception here I think what you really want is Any. Otherwise, you cannot define an exception handler taking an exception class other than Exception without type checking complaining.
This reverts commit 5dfe686.
This is the expected behavior because mypy infers the common type that unionizes with all the values of the |
Yes. This was explained to me over at MyPy too, basically, an untyped method silently poisons the dict. |
|
This is outdated. We have created the |
Version 0.21.0 added support for handing errors in WebSockets.
However, the signature of
Starlette(exception_handlers)keyword argument was not updated and therewere no type checks catching the mismatch.
test_applications.pyto get mypy to check typesexception_handlerskeyword.class StarletteInternally, the type hints don't match. And they don't even allow for async functions. In light of this, I propose a
different approach, to change the type to
Callable[[Union[Request, Websocket], Any], Any]. UsingExceptionas the second arg in the type strictly precludes us from defining handers with a more specific excepty type argument. And the variable type of the first argument, variable return argument, and allowing both sync and async, well, maybe this is one place where type strictness could be relaxed.