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

drop error_response, add error detail and support for serialization #59

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

Conversation

tomwojcik
Copy link
Owner

@tomwojcik tomwojcik commented Dec 28, 2021

Closes #58

@tomwojcik tomwojcik added bug Something isn't working enhancement New feature or request labels Dec 28, 2021
@tomwojcik tomwojcik requested a review from hhamana December 28, 2021 15:49
@tomwojcik tomwojcik self-assigned this Dec 28, 2021
@hhamana
Copy link
Collaborator

hhamana commented Dec 29, 2021

I'll have a look at it over the new year

@hhamana
Copy link
Collaborator

hhamana commented Dec 30, 2021

I believe changes from this implementation will do the trick.

It doesn't. My API needs to have a consistent error schema with the rest of the system, which is a quite custom JSON response.
This kills any way for user to customize it to their needs, and enforces a hard-coded non-JSON error message without alternative.
I get that you don't like passing a response object through the exception, and I'd want to agree there, but the way plugins work, that was the most straightforward way to escalate it to the handler
As this pr hints at, the exception itself can define the response. Sure, but then we need to allow customizing the exception raised, and also modify headers (I'll want the "Content-Type" header to be application/json for a proper JSON response at least) and we'll have to build the appropriate response from scratch in the middleware given the exception's data.
I think it's more convenient to have it already build for us there instead, but you'll notice this'll get us back to pretty much the current situation.

Having the default error message as f"Invalid UUID in request header {self.key}" is better for sure though.

@tomwojcik
Copy link
Owner Author

My API needs to have a consistent error schema with the rest of the system, which is a quite custom JSON response.

AFAIK that's impossible. Unless you have another layer on top of your starlette-based app, starlette errors return 500 plain text.
possibly related encode/starlette#1175 (comment)

But I get you. I don't see why both solutions shouldn't coexist, so a default error message and optionally a custom response for errors. I will think about it.

@tomwojcik
Copy link
Owner Author

My API needs to have a consistent error schema with the rest of the system, which is a quite custom JSON response.
This kills any way for user to customize it to their needs, and enforces a hard-coded non-JSON error message without alternative.

https://github.com/tomwojcik/starlette-context/pull/59/files#diff-4a0aecbfe7f993c2281b848a8b4269204420d30388706ddea3bc26dbfd2b1171R51-R64

wdyt?

@hhamana
Copy link
Collaborator

hhamana commented Jan 2, 2022

Unless you have another layer on top of your starlette-based app, starlette errors return 500 plain text.

that's if you leave those as errors though. If you handle those and make a response from there somehow, the Starlette layer won't even see it's been an exception.

@hhamana
Copy link
Collaborator

hhamana commented Jan 2, 2022

wdyt?

I guess the factory approach is workable. It reduces the plugin-specific response to a singular handler on the middleware, but one use I can see is raising the exception internally, to except and extract the exact exception (as different plugins will still raise different subclasses), and a custom plugin can still have its own exception mixed in too.

@tomwojcik tomwojcik changed the title wip: drop error_response, add error detail and support for serialization drop error_response, add error detail and support for serialization Feb 6, 2022
@tomwojcik tomwojcik requested a review from hhamana February 6, 2022 16:17
Copy link
Collaborator

@hhamana hhamana left a comment

Choose a reason for hiding this comment

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

the error handler approach is promising, but I feel might need some refinement

def __init__(
self,
error_handler: Optional[
Callable[[Request, StarletteContextException], Response]
Copy link
Collaborator

@hhamana hhamana Feb 8, 2022

Choose a reason for hiding this comment

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

I see where you're going now, with this signature, it's really an invitation to reuse custom exception handlers.
A first instinct would be to literally pass the app's existing exception handler, but then only the StarletteContextException are caught, so it's actually about writing a new exception handler, surely inspired by existing custom ones, and passing it there.
typing won't even help in making this caveat obvious here, as a general handler built for the already existing HTTPException, which is a fairly common thing to write, passes typecheck as the StarletteContextException now is a subclass.

def general_error_handler(req: Request, exc: HTTPException) -> Response:
    # built for all HTTPException
    return Response()

m = StarletteContextMiddlewareMixin(error_handler=general_error_handler)

One might think that all HTTPException are handled there.
That can get more confusing with custom plugins who might raise HTTPException as they might be used to everywhere else, and to only get burned as they are not actually caught by the middleware.

if passing such an handler is to be considered canon, then the middlewares should except on HTTPException instead of the StarletteContextException, and type the callback as taking the HTTPException. the StarletteContextException we raise can be considered as internal fine-grained exceptions.
if not, then the StarletteContextException shouldn't inherit from HTTPException to disallow that usage from typing, and enforce (and document) the usage of StarletteContextException exclusively for plugins.

Copy link
Collaborator

Choose a reason for hiding this comment

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

also, it takes an async function, so it should be an Callable[[Request, StarletteContextException], Awaitable[Response]]

Copy link
Owner Author

@tomwojcik tomwojcik Feb 20, 2022

Choose a reason for hiding this comment

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

One might think that all HTTPException are handled there.

If a custom exception handler is needed and the docs state how to use it, why this is needed, and what kind of errors it supports, then everything should be pretty clear, or at least as clear as it gets.

That can get more confusing with custom plugins who might raise HTTPException as they might be used to everywhere else, and to only get burned as they are not actually caught by the middleware.

Well, indeed that's a disadvantage I didn't consider earlier. I was thinking if this could solve it

    async def dispatch(
        self, request: Request, call_next: RequestResponseEndpoint
    ) -> Response:
        context = await self.set_context(request)
        token: Token = _request_scope_context_storage.set(context)

        try:
            response = await call_next(request)
        except StarletteContextException as e:
            response = await self.create_response_from_exception(request, e)
        else:
            for plugin in self.plugins:
                try:
                    await plugin.enrich_response(response)
                except Exception as e:
                    response = await self.create_response_from_exception(request, e)
                    break
        finally:
            _request_scope_context_storage.reset(token)

        return response

WDYT?

I think it's possible that the application state will be "success" and then it fails on response enrichment. If that happens, there's no clear rollback/cleanup. 🤔 Actually that's the problem in the current implementation as well, I think.

Copy link
Collaborator

Choose a reason for hiding this comment

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

an Exception raised during enrich_response would rise up to the plain exception handler. As data isn't coming from the user by that point (or should have been validated before), it's really a faulty implementation, so getting a 500 seems correct to me . I'm not sure that's something to be concerned about.
And in any case, the situation is fairly different, I don't think it's okay to simply reuse the same handler expecting validation errors from input.

finally:
_request_scope_context_storage.reset(token)

except StarletteContextException as e:
Copy link
Collaborator

Choose a reason for hiding this comment

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

moving the handling to wrap everything? no, that can't be right.
StarletteContextException should only happen during plugin execution anyway.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Well I'm glad I didn't merge it yet. Thanks!

error_handler: Optional[
Callable[
[Request, StarletteContextException],
Union[Response, Awaitable[Response]],
Copy link
Collaborator

Choose a reason for hiding this comment

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

return await self.error_handler(request, exc)
that looks very much like only awaitables are supported. why the union?
(and i'd rather support only one than digging into the can of worms of supporting either)

@hhamana hhamana mentioned this pull request Jun 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improved logging
2 participants