Skip to content

Add proper type hints to add_exception_handler#1308

Closed
phillipuniverse wants to merge 2 commits intoKludex:masterfrom
phillipuniverse:patch-1
Closed

Add proper type hints to add_exception_handler#1308
phillipuniverse wants to merge 2 commits intoKludex:masterfrom
phillipuniverse:patch-1

Conversation

@phillipuniverse
Copy link

I lost a bit of time due to a missing return statement and type hint in an exception handler.

In my case I had registered an HTTPException exception handler without a return. Here's the exception I ended up with:

Traceback (most recent call last):
  File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/myproj-0dUE00O2-py3.8/lib/python3.8/site-packages/starlette/exceptions.py", line 71, in __call__
    await self.app(scope, receive, sender)
  File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/myproj-0dUE00O2-py3.8/lib/python3.8/site-packages/starlette/routing.py", line 609, in __call__
    await self.default(scope, receive, send)
  File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/myproj-0dUE00O2-py3.8/lib/python3.8/site-packages/starlette/routing.py", line 497, in not_found
    raise HTTPException(status_code=404)
starlette.exceptions.HTTPException

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/myproj-0dUE00O2-py3.8/lib/python3.8/site-packages/starlette/middleware/errors.py", line 159, in __call__
    await self.app(scope, receive, _send)
  File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/myproj-0dUE00O2-py3.8/lib/python3.8/site-packages/starlette/middleware/cors.py", line 78, in __call__
    await self.app(scope, receive, send)
  File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/myproj-0dUE00O2-py3.8/lib/python3.8/site-packages/starlette/exceptions.py", line 93, in __call__
    await response(scope, receive, sender)
TypeError: 'NoneType' object is not callable

Here's what my exception handler and registration looked like:

...

async def http_exception_handler(request, exc):
   JSONResponse(
        status_code=exc.status_code,
        content=jsonable_encoder(
            {
                "errors": [
                    {
                        "id": uuid4(),
                        "title": "Internal Exception",
                        "detail": exc.detail,
                    }
                ]
            }
        ),
    )

application.add_exception_handler(HTTPException, http_exception_handler)

This is clearly an error but it would've been nice to get a type hint error on it before I went through and traced down the implementation.

Thanks for such a great library!

@phillipuniverse
Copy link
Author

I realized there are a lot of other callables in the Starlette application that are not correctly type hinted. I can expand this PR to type hint the rest of those if that is desirable.

Copy link
Owner

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

Yeah, PRs improving the type hints are welcome.

As for this PR, can we also add the type hint on the starlette/exceptions.py file as well (in the analogous signature)?

self,
exc_class_or_status_code: typing.Union[int, typing.Type[Exception]],
handler: typing.Callable,
handler: typing.Callable[[Request, Exception], typing.Union[Response, typing.Coroutine[typing.Any, typing.Any, Response]]],
Copy link
Owner

Choose a reason for hiding this comment

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

Should we perhaps use Awaitable instead? It looks better. 😗

Suggested change
handler: typing.Callable[[Request, Exception], typing.Union[Response, typing.Coroutine[typing.Any, typing.Any, Response]]],
handler: typing.Callable[[Request, Exception], typing.Union[Response, Awaitable[Response]]],

@aminalaee
Copy link
Contributor

@phillipuniverse Are you willing to address the changes requested?

@phillipuniverse
Copy link
Author

@aminalaee yes! Sorry, this fell off my radar a bit. I’ll get this back on track in the next couple of days.

@Kludex
Copy link
Owner

Kludex commented Feb 1, 2022

Thanks for the PR @phillipuniverse !

There's a discussion on #1456 about the idea we see here, so I'll close this PR.

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.

3 participants