Move exception handling logic to endpoints

[adriangb]

Fixes #493, partially resolves #1692 (comment), resolves #1649 (comment)

By moving handling of exceptions to the Route level we can use the same Request/WebSocket object for the endpoint and exception handlers.

I wrote this PR so that it is backwards compatible (and it also needs a lot of polish). There's a lot of duplicate logic between ExceptionMiddleware and routing.py. I see two options:

• Clean up the implementation to minimize duplicate code but keep ExceptionMiddleware around. This is less likely to break things like FastAPI that poke around into the internals of Starlette. It also means that ASIG middleware e.g. in an ASGI app mounted via Mount can continue to raise HTTPExceptions which will get caught by ExceptionMiddleware (I'd argue that anyone that is doing this should not be doing this but it's technically possible to do). (see note 1)
• Remove ExceptionMiddleware. We'd just need to move a bit of logic (separating the exception and status handlers and putting them into the scope for every request) into Starlette. This would eliminate all of the duplicate code.

Implementation aside, to me this makes sense conceptually: exception handlers operate on requests/webscokets not at the ASGI level. So they should live within the sphere of Request/WebSocket, not alongside generic ASGI middleware. I think this is the same principle that applies to FastAPI's dependencies and why they don't have the problems that BaseHTTPMiddleware has.

Notes:

1. Note that this does not apply to normal middleware installed via Starlette(..., middleware=...) since those middleware have never had any guarantee of HTTPExceptions they raise being caught.

[adriangb]

Tests aare failing for coverage reasons. I like how the diff shows very clearly what the real change is now so I opened a PR against this branch to show what a "cleaned up" version with no coverage issues would look like, but the diff is bigger: master...adriangb:starlette:cleanup-move-exc

[alex-oleshkevich]

Will it work if we just wrap self.handle in ExceptionMiddleware?

# starlette/routing.py
class BaseRoute:
    async def __call__(self, scope, receive, send):
        ...
        handler = ExceptionMiddleware(self.handle, handlers=scope['starlette.exception_handlers'], debug=scope['app'].debug),
        await handler(scope, receive, send)

If possible, this significantly reduces the code duplication.

[adriangb]

There’d still be two request instances. Reducing code duplication can be done easily with some refactoring (there is no duplicate code in master...adriangb:starlette:cleanup-move-exc)

[adriangb]

@tiangolo would you mind taking a look at this with respect to how it might impact FastAPI? You vendor request_response and some other bits but I think they could be ported over right?

[tiangolo]

I like it! 🚀 I agree it could/would be ported over to FastAPI.

Thanks @adriangb for pinging me (and @Kludex for DMing about the ping, I tend to lose these important pings among too many GitHub email notifications 😬 ).

---

A bit more from my point of view, probably taking this even further (in the future):

I want to be able to support exception handlers at the router level, to have some routers that handle some specific exceptions that might be relevant only to some portions of the code related to that router, instead of having to put all the exceptions together at the global app.

In FastAPI, this will also mean that I will have to refactor how app.include_router() works to not recreate routes but to actually store and re-use sub-routers. That's a larger refactor in FastAPI, but anyway, that's what I would like to target in the future, and I think this logic, moving exception handling away from middleware will bring us closer to that.

[adriangb]

I like it! 🚀 I agree it could/would be ported over to FastAPI.

Good to hear! I think if FastAPI is willing to adapt I don't see any blockers for moving forward with this; it's a backwards compatible change for pure-Starlette projects.

I want to be able to support exception handlers at the router level, to have some routers that handle some specific exceptions that might be relevant only to some portions of the code related to that router, instead of having to put all the exceptions together at the global app.

Interesting, this is not even something I had thought of! But yes of course this change would make it pretty straightforward to have per-route exception handlers that we could merge with the application or router level exception handlers using a ChainMap sort of thing. Great point!

[adriangb]

Closing this in favor of #2026 (see #2020 (comment))