Generic ExceptionHandler type

[tekumara]

Summary

Allows callables that handle Exception subtypes to pass the type checker (mypy/pyright), eg: this will now correctly type check:

from starlette.applications import Request, Starlette
from starlette.responses import JSONResponse

class MyException(Exception):
    def __init__(self, message: str) -> None:
        self.message = message
        self.extra = "extra"

def my_exception_handler(request: Request, exc: MyException) -> JSONResponse:
    return JSONResponse({"detail": exc.message, "extra": exc.extra}, status_code=400)

app = Starlette(debug=True, routes=[])
app.add_exception_handler(MyException, my_exception_handler) # <- correctly type checks

handlers = {MyException: my_exception_handler}
app = Starlette(debug=True, routes=[], exception_handlers=handlers) # <- correctly type checks

I think the cause of the problem is Callable is contravariant (for good reason) and so whilst MyException is a sub-type of Exception, the Callable is not. This PR uses a TypeVar bound to Exception to address this.

See this discussion comment.

Checklist

• [x] I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
• [ ] I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
• [ ] I've updated the documentation accordingly.

[Kludex]

Can you remove the trailing commas, please?

[tekumara]

Hi @Kludex these were added by ruff format . which I ran to satisfy the failing build, I can try and remove them.

[adriangb]

Does this catch the error cases?

from starlette.applications import Request, Starlette
from starlette.responses import JSONResponse

class MyException(Exception):
    def __init__(self, message: str) -> None:
        self.message = message
        self.extra = "extra"

def my_exception_handler(request: Request, exc: MyException) -> JSONResponse:
    return JSONResponse({"detail": exc.message, "extra": exc.extra}, status_code=400)

app = Starlette(debug=True, routes=[])
app.add_exception_handler(Exception, my_exception_handler) # should error

handlers = {Exception: my_exception_handler}
app = Starlette(debug=True, routes=[], exception_handlers=handlers) # should error

[tekumara]

Yes your example errors because of the tighter bound mentioned above, eg:

❯ mypy starlette/test.py
starlette/test.py:1: error: Module "starlette.applications" does not explicitly export attribute "Request"  [attr-defined]
starlette/test.py:13: error: Argument 2 to "add_exception_handler" of "Starlette" has incompatible type "Callable[[Request, MyException], JSONResponse]"; expected "Union[Callable[[Request, Exception], Union[Response, Awaitable[Response]]], Callable[[WebSocket, Exception], Awaitable[None]]]"  [arg-type]
starlette/test.py:16: error: Argument "exception_handlers" to "Starlette" has incompatible type "Dict[Type[Exception], Callable[[Request, MyException], JSONResponse]]"; expected "Optional[Mapping[Union[int, Type[Never]], Union[Callable[[Request, Never], Union[Response, Awaitable[Response]]], Callable[[WebSocket, Never], Awaitable[None]]]]]"  [arg-type]
Found 3 errors in 1 file (checked 1 source file)

[adriangb]

For the record, this has been attempted before and the conclusion at the time was that typing this correctly is not possible: https://github.com/encode/starlette/pull/2048

I didn't look in detail but I would suggest we understand why it is possible now when it wasn't a couple months ago, or what this PR does differently.

[adriangb]

What about if you have multiple types? handlers = {Exception: fallback_handler, MyException: my_exception_handler}

[tekumara]

Yes that will pass type checks. I haven't changed the dictionary key which is still Any ie: the key isn't tied to the handler Callable as it is with the add_exception_handler function parameters.

Thank you btw for pointing me at the previous attempt, I hadn't seen that. This PR differs by not introducing a new class for the handler function.

Instead it makes Exception a type parameter so it can vary. This allows handlers to provide a more specific type, so long as it's an Exception subtype. This overcomes the inherent contravariance in Callable type parameters, which currently prevents subtypes of Exception being accepted by the type checker.

[ThirVondukr]

Adding a generic exception handler should be relatively easy, or am I missing something?

import typing

from starlette.requests import Request
from starlette.responses import Response
from starlette.websockets import WebSocket


TException = typing.TypeVar("TException")

HTTPExceptionHandler = typing.Callable[
    ["Request", TException], typing.Union["Response", typing.Awaitable["Response"]]
]
WebSocketExceptionHandler = typing.Callable[
    ["WebSocket", TException], typing.Awaitable[None]
]
ExceptionHandler = typing.Union[HTTPExceptionHandler[TException], WebSocketExceptionHandler[TException]]


def add_exception_handler(
    exc_class_or_status_code: int | typing.Type[TException],
    handler: ExceptionHandler[TException],
) -> None:
    pass


def value_error_handler(request: Request, exc: ValueError) -> Response:
    return Response()

def exc_handler(request: Request, exc: Exception) -> Response:
    return Response()

add_exception_handler(Exception, value_error_handler) # Error, value_error_handler should accept `Exception`
add_exception_handler(ValueError, value_error_handler) # Ok

add_exception_handler(Exception, exc_handler) # Ok
add_exception_handler(TypeError, exc_handler) # Ok

[tekumara]

Hi @ThirVondukr your snippet is essentially what the PR implements. The one difference is the PR's ExceptionType is bound to Exception to prevent non-Exception types from being used in the handler.

[Kludex]

There's an even bigger discussion on https://github.com/encode/starlette/pull/1456.

[Kludex]

So... Does this test summarize what we need here?

def test_types() -> None:
    class MyException(Exception):
        def __init__(self, message: str) -> None:
            self.message = message
            self.extra = "extra"

    def my_exc_handler(request: Request, exc: MyException) -> JSONResponse:
        return JSONResponse(
            {"detail": exc.message, "extra": exc.extra}, status_code=400
        )

    app = Starlette(debug=True, routes=[])
    # should error (type ignore is needed)
    app.add_exception_handler(Exception, my_exc_handler)  # type: ignore[arg-type]
    # doesn't error
    app.add_exception_handler(MyException, my_exc_handler)

    # should error (type ignore is needed)
    handlers = {Exception: my_exc_handler}
    Starlette(debug=True, routes=[], exception_handlers=handlers)  # type: ignore[arg-type]

    # doesn't error
    my_exception_handlers = {MyException: my_exc_handler}
    Starlette(debug=True, routes=[], exception_handlers=my_exception_handlers)

    # should error (type ignore is needed)
    multiple_handlers = {MyException: my_exc_handler, Exception: my_exc_handler}
    Starlette(debug=True, routes=[], exception_handlers=multiple_handlers)  # type: ignore[arg-type]

    # doesn't error
    Starlette(exception_handlers={MyException: my_exc_handler})
    # should error (type ignore is needed)
    Starlette(exception_handlers={Exception: my_exc_handler})  # type: ignore[arg-type]

[Kludex]

Can we just have the add_exception_handler type hinted instead for now? Since I don't think there's any controversy...

[tekumara]

Hi @Kludex,

Thanks for taking a look again!

1. currently these of your above cases are not addressed in this PR and will not error as the comment indicates they should:

   from starlette.applications import Request, Starlette
   from starlette.responses import JSONResponse
   
   
   def test_types() -> None:
       class MyException(Exception):
           def __init__(self, message: str) -> None:
               self.message = message
               self.extra = "extra"
   
       def my_exc_handler(request: Request, exc: MyException) -> JSONResponse:
           return JSONResponse(
               {"detail": exc.message, "extra": exc.extra}, status_code=400
           )
   
       # should error (type ignore is needed)
       handlers = {Exception: my_exc_handler}
       Starlette(debug=True, routes=[], exception_handlers=handlers)  # type: ignore[arg-type]
   
        # should error (type ignore is needed)
       multiple_handlers = {MyException: my_exc_handler, Exception: my_exc_handler}
       Starlette(debug=True, routes=[], exception_handlers=multiple_handlers)  # type: ignore[arg-type]
   
       # should error (type ignore is needed)
       Starlette(exception_handlers={Exception: my_exc_handler})  # type: ignore[arg-type]
   

2. The PR doesn't change Starlette.__init__ except in so far as it changes ExceptionHandler and so now (correctly) allows this case which would previously error:

       # doesn't error
       my_exception_handlers = {MyException: my_exc_handler}
       Starlette(debug=True, routes=[], exception_handlers=my_exception_handlers)
   

Can we just have the add_exception_handler type hinted instead for now? Since I don't think there's any controversy...

By this do you mean don't worry about the cases in 1. or update the PR to no longer address the case in 2. ?

Whilst it would be possible to update the PR to preserve the current behaviour of Starlette.__init__, it would require creating a second ExceptionHandler type with the old behaviour. This doesn't seem like a great solution as it suggests we have two types of handler, which isn't the actual intent (its just a mechanism for preserving the old behaviour of Starlette.__init__). It would also complicate the codebase. Hopefully that makes sense, happy to explain further!

[HenriBlacksmith]

Is there any traction on this fix?

This is embarassing for Starlette/FastAPI users using mypy

[tekumara]

After merging in master again, the tests are failing on this PR (and other PRs too) because of a change unrelated to this PR:

• mypy starlette tests starlette/templating.py:125: error: Argument 1 to "FileSystemLoader" has incompatible type "Union[str, PathLike[bytes], Sequence[Union[str, PathLike[bytes]]]]"; expected "Union[str, PathLike[str], Sequence[Union[str, PathLike[str]]]]" [arg-type] Found 1 error in 1 file (checked 67 source files)

[meyer1994]

I am also having this issue. Is there any more work to be done? I´d be happy to help

[Kludex]

I need to read the reply and review again.

[LincolnPuzey]

exception_handlers: typing.Mapping[typing.Any, ExceptionHandler[ExceptionType]] | None = None,

I think the fundamental problem identified in the previous attemps is that for the exception_handlers mapping, the allowed type of each mapping value, depends on the value of each mapping key. This is impossible to fully validate/describe with just the type system.

e.g. for these handlers

exception_handlers = {
  MyException: my_exception_handler,
  404: my_404_handler,
}

the value of each mapping key defines the allowed type of each handler. The first handler's type must be Callable[[Request, MyException], Response], because the key value is MyException. The second handler's type must be Callable[[Request, HTTPException], Response], because the key value is 404.

You can't define a Mapping with a different value type per key value.

(You can define a TypedDict to do this, when the keys are strings and known beforehand, but that isn't the case here)