alex-oleshkevich / starception

Beautiful exception page for Starlette apps.
MIT License
95 stars 6 forks source link

Should this become ASGI framework agnostic? Nearly there #30

Open florimondmanca opened 1 year ago

florimondmanca commented 1 year ago

Hi there,

This was flagged on https://github.com/florimondmanca/awesome-asgi/pull/84 as a tool providing "amazing output in the developement phase". It surely looks so!

awesome-asgi is focused on tools and libraries that are as close to the ASGI spec as possible, i.e. "framework agnostic".

StarceptionMiddleware is already a pure ASGI middleware so it's already fairly agnostic.

From a quick analysis, it seems the only bit preventing this middleware from being usable with any ASGI framework is this line:

https://github.com/alex-oleshkevich/starception/blob/master/starception/middleware.py#L42

This would notably work with Starlette, which sets scope["app"] and exposes a .debug attribute. But this is not guaranteed to work for other frameworks if they don't comply with this interface (which will almost certainly be the case).

Maybe this middleware could keep the "automatic debug detection" functionality for Starlette-based framework, and rely on a debug=... parameter for other cases? Something like this...

class StarceptionMiddleware:
    def __init__(self, app, *, debug=None):
        self.app = app
        self._debug = debug

    async def __call__(self, scope, receive, send):
        ...

        try:
            debug = request.app.debug
        except AttributeError:
            if self._debug is None:
                raise RuntimeError(
                    "Could not infer whether the app is running in debug mode, as "
                    "'request.app.debug' is unavailable. "
                    "Hint: you can pass a debug flag explicitly using "
                    "'StarceptionMiddleware(app, debug=...)'."
                )
            debug = self._debug

Then, I believe the Integration with other frameworks documentation could hint at using StarceptionMiddleware(app, debug=...) instead of hinting users at using the exception_handler.

I believe that would work better because a) most (all?) of the ASGI frameworks have some middleware mounting API, but b) most frameworks that have an error handler API won't accept a Starlette Request instance.

Then I think the library would be fully framework-agnostic! On top of allowing support for the widest userbase possible, in my experience this also reduces the maintenance burden. Starlette can be hidden as an implementation detail, and not exposed in the library public API at all.

What do you think? I'll be happy to provide a PR.

euri10 commented 1 year ago

Interested : I was using this package a lot on a particular project that used FastAPI then switched to Litestar and cant use it anymore which is too bad because it saves alot of time, still using it on legacy projects though.

I remember having tried to adapt it and failed but can't remember where the issue was.

florimondmanca commented 1 year ago

@euri10 Well, it looks like starception works out of the box with Litestar:

from litestar import Litestar, get
import starception
from starlette.requests import Request as StarletteRequest

@get("/")
async def hello_world() -> str:
    raise Exception("Oops")
    return "Hello, world!"

app = Litestar(
    [hello_world],
    exception_handlers={Exception: starception.exception_handler}
)

The reason it works is a combination of luck and the fact that Litestar embraces ASGI (should it be featured on awesome-asgi?).

  1. Litestar applications put themselves as "app" in the scope and they have a .debug attribute.
  2. Litestar's Request is similar to Starlette's Request, as far as attributes used by starception go. If it wasn't, it would have still been possible to wrap the request: StarletteRequest(request.scope).
  3. Litestar is more liberal on the response side since it expects exception handlers to return an ASGI callable, which a Starlette Response is, so it works.

I tried applying StarceptionMiddleware, and that actually won't work. Litestar processes exceptions and then passes te resulting response to middleware, so Litestar handles any exceptions before StarceptionMiddleware gets a chance to do so.

So, I was wrong to assume that an ASGI middleware would work in all situations while an exception handler wouldn't.

As long as the framework has a Request model that accepts a scope, and expects an ASGI callable response in return, an exception middleware should work.

The .debug aspect is still valid though, I think.

euri10 commented 1 year ago

Oo I should revisit then !

euri10 commented 1 year ago

The reason it works is a combination of luck and the fact that Litestar embraces ASGI (should it be featured on awesome-asgi?).

oh and a big yes !

alex-oleshkevich commented 1 year ago

Thanks for your interest! I don't want to use StarceptionMiddleware and will remove it someday. Because to work properly this middleware must be at the very top of the stack.

The advised way to work is exception handlers. I think the library can drop starlette dependency and become pure ASGI if it would define a request protocol and return ASGI callable as responses. However, different frameworks have their own quirks and I am not sure if there is a generic solution. So per-framework exception handlers are good here.