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

Generic ExceptionHandler type #2403

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

tekumara
Copy link

@tekumara tekumara commented Jan 8, 2024

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

  • 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.

@@ -139,8 +140,8 @@ def add_middleware(

def add_exception_handler(
self,
exc_class_or_status_code: int | typing.Type[Exception],
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NB: this has now been tightened so that the Exception for exc_class_or_status_code must match the Exception in the handler Callable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kludex isn't this a deprecated function?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. We never deprecated add_exception_handler... 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, to be more precise, the Exception in the handler Callable is still contravariant, so this passes as one would hope:

def fallback_exception_handler(request: Request, exc: Exception) -> JSONResponse:
    ...

app.add_exception_handler(Exception, fallback_exception_handler)
app.add_exception_handler(MyException, fallback_exception_handler)

ie: the ExceptionType TypeVar changes the handler Callable Exception to a type parameter. The value of this type parameter is provided at the call site, eg: in the example here its Exception in the first call, and MyException in the second. But the contravariance of Callable remains.

And also the Callable is still not covariant, so this fails as one might expect:

def my_exception_handler(request: Request, exc: MyException) -> JSONResponse:
    ...

# this fails, because subtypes of the exc_class_or_status_code Exception in the Callable 
# aren't permitted ie: Callable is not covariant in the function's input parameter types
app.add_exception_handler(Exception, my_exception_handler)

@Kludex
Copy link
Member

Kludex commented Jan 8, 2024

Can you remove the trailing commas, please?

@tekumara
Copy link
Author

tekumara commented Jan 8, 2024

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

Copy link
Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nice, btw 👍

Thanks.

@@ -21,10 +21,10 @@
]
Lifespan = typing.Union[StatelessLifespan[AppType], StatefulLifespan[AppType]]

E = typing.TypeVar("E", bound=Exception, contravariant=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we call it ExceptionType?

Kludex
Kludex previously approved these changes Jan 8, 2024
@adriangb
Copy link
Member

adriangb commented Jan 8, 2024

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
Copy link
Author

tekumara commented Jan 8, 2024

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
Copy link
Member

adriangb commented Jan 8, 2024

For the record, this has been attempted before and the conclusion at the time was that typing this correctly is not possible: #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
Copy link
Member

adriangb commented Jan 8, 2024

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

@tekumara
Copy link
Author

tekumara commented Jan 8, 2024

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.

@Kludex Kludex mentioned this pull request Jan 8, 2024
1 task
@ThirVondukr
Copy link

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
Copy link
Author

tekumara commented Jan 9, 2024

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 Kludex dismissed their stale review February 3, 2024 08:52

Reviewing with more care.

@Kludex
Copy link
Member

Kludex commented Feb 3, 2024

There's an even bigger discussion on #1456.

@Kludex
Copy link
Member

Kludex commented Feb 3, 2024

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
Copy link
Member

Kludex commented Feb 4, 2024

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

@tekumara
Copy link
Author

tekumara commented Feb 5, 2024

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
Copy link

Is there any traction on this fix?

This is embarassing for Starlette/FastAPI users using mypy

@tekumara
Copy link
Author

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
Copy link

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

@Kludex
Copy link
Member

Kludex commented May 16, 2024

I need to read the reply and review again.

callebtc added a commit to cashubtc/nutshell that referenced this pull request Jul 24, 2024
@LincolnPuzey
Copy link

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)

@HenriBlacksmith
Copy link

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)

One thing is to fully validate with the type system, one other thing is to make it go through type validation.

The minimum is to allow valid exception handlers to not fail mypy checks and this should be a top-priority for Starlette maintainers for me.

The quality of type annotations can be improved as much as possible over time but this is less urgent for me.

@Kludex
Copy link
Member

Kludex commented Sep 1, 2024

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!

On the 1., if I add any other exception to the dictionary with a proper exception handler, then it fails. What's the point on fixing the behavior for 1 element?

Does someone want to propose an alternative API for exception handlers?

@tekumara
Copy link
Author

tekumara commented Sep 2, 2024

On the 1., if I add any other exception to the dictionary with a proper exception handler, then it fails. What's the point on fixing the behavior for 1 element?

Just to make sure I understand correctly, could you share the code for the failing example you mention here?

@Kludex
Copy link
Member

Kludex commented Sep 2, 2024

On the 1., if I add any other exception to the dictionary with a proper exception handler, then it fails. What's the point on fixing the behavior for 1 element?

Just to make sure I understand correctly, could you share the code for the failing example you mention here?

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"

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

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

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

    handlers = {Exception: my_exc_handler, PotatoException: my_exc_handler2}
    Starlette(debug=True, routes=[], exception_handlers=handlers)

    multiple_handlers = {MyException: my_exc_handler, Exception: my_exc_handler, PotatoException: my_exc_handler2}
    Starlette(debug=True, routes=[], exception_handlers=multiple_handlers)

    Starlette(exception_handlers={Exception: my_exc_handler, PotatoException: my_exc_handler2})

Extending the code above.

@adriangb
Copy link
Member

adriangb commented Sep 2, 2024

Does someone want to propose an alternative API for exception handlers?

I've proposed https://mypy-play.net/?mypy=latest&python=3.11&gist=ace307dc705717b1ef1fedc21708b8f3 before. I think it's the only way to make it actually work for all cases, and it's backwards compatible.

IMO we should do something like that and simplify the existing type hints to be very loose and accept all cases.

@tekumara
Copy link
Author

tekumara commented Sep 4, 2024

Gotcha thanks.. so this in-lined dictionary case:

Starlette(exception_handlers={Exception: my_exc_handler, PotatoException: my_exc_handler2})

Fails with:

Argument of type "dict[type[Exception] | type[PotatoException], ((request: Request, exc: MyException) -> JSONResponse) | ((request: Request, exc: PotatoException) -> JSONResponse)]" cannot be assigned to parameter "exception_handlers" of type "Mapping[Any, ExceptionHandler[ExceptionType@__init__]] | None" in function "__init__"
    Type "dict[type[Exception] | type[PotatoException], ((request: Request, exc: MyException) -> JSONResponse) | ((request: Request, exc: PotatoException) -> JSONResponse)]" is not assignable to type "Mapping[Any, ExceptionHandler[ExceptionType@__init__]] | None"
      "dict[type[Exception] | type[PotatoException], ((request: Request, exc: MyException) -> JSONResponse) | ((request: Request, exc: PotatoException) -> JSONResponse)]" is not assignable to "Mapping[Any, ExceptionHandler[ExceptionType@__init__]]"
        Type parameter "_VT_co@Mapping" is covariant, but "((request: Request, exc: MyException) -> JSONResponse) | ((request: Request, exc: PotatoException) -> JSONResponse)" is not a subtype of "ExceptionHandler[ExceptionType@__init__]"
      "dict[type[Exception] | type[PotatoException], ((request: Request, exc: MyException) -> JSONResponse) | ((request: Request, exc: PotatoException) -> JSONResponse)]" is not assignable to "None" (reportArgumentType)

This can be supported with overloads on __init__ see 292259c as an example.

@tekumara
Copy link
Author

tekumara commented Sep 4, 2024

IMO we should do something like that and simplify the existing type hints to be very loose and accept all cases

This makes sense to me. Its more practical and simpler than overloads. See how it looks for the exception_handlers dict in ebcd61f.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants