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

Add proper type hints to add_exception_handler #1308

Closed
wants to merge 2 commits into from

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

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)?

@@ -129,7 +131,7 @@ def add_middleware(self, middleware_class: type, **options: typing.Any) -> None:
def add_exception_handler(
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
Member

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
Member

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

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