Allow custom validation response

[hhamana]

This PR fixes 2 issues:

• A user sending invalid data in a UUID-based plugin should not result in a server error, but a client error with a 400 status code.
• Middlewares in Starlette should not raise Exceptions, they should abort the request cycle and send a response instead.

The exact error is customizable from user code, but provides a default 400 response with empty body.
Implemented on both ContextMiddleware and RawContextMiddleware.

[tomwojcik]

Thank you for your contribution! Within the next few days I'm afraid I won't have time to look into this PR but I will try to find some time later on.

[tomwojcik]

Sorry it took so long, I have a few personal things going on.

So this PR looks really well. You're right it'd be better if 4xx was returned instead.
It seems like you're right with the exceptions from the middleware as well.

I have a few comments though. Do you want me to accept it as is and change a few things or do you want to discuss comments?

[hhamana]

Thank you for the review. I agree there should be some changes as per your comments, I'll experiment with a callback-based implementation.

[hhamana]

with a deeper look, the mentioned rate-limiting middleware ends up doing the same weird double await send(dict with magic arguments) to send a response as a raw middleware.
https://github.com/abersheeran/asgi-ratelimit/blob/7aec8f771eb7cd7a6ec467930030ed60227592b2/ratelimit/core.py#L9
I don't think it's very user-friendly to make users implement that sort of thing, and would prefer the response be generated here as it is, with the bonus of offering a consistent interface for the different middlewares.

[tomwojcik]

Thanks. It looks very good now. I'll give it another look over the weekend.

[tomwojcik]

 I don't think it's very user-friendly to make users implement that sort of thing, and would prefer the response be generated here as it is, with the bonus of offering a consistent interface for the different middlewares.

Ehh... I was thinking about one thing and wrote something else. Of course, it's not user-friendly. A huge part of my review was not entirely correct, especially the callback/base exception thing. Now I gave it a thought and I think there's only one correct way of handling those.

To my knowledge, all web frameworks have a central place where they handle errors. Starlette is a framework so it imposes a certain way of handling them as well, in the ExceptionMiddleware. 

If you had middleware A, B , C and RawContextMiddleware, create a response in RawContextMiddleware, the rest of them will process the response as if it was a success response. So for instance ASGISentryMiddleware etc. wouldn't catch the error. It's just asking for trouble. We need to do things Starlette-way.

1. StarletteContextError is not needed. Starlette is responsible for handling errors and starlette-context has to raise errors Starlette can understand. All custom errors should inherit from HTTPException and have default arguments. 
   Example:

class InvalidUUIDError(HTTPException): 
    def __init__(self, status_code: int = status.HTTP_422_UNPROCESSABLE_ENTITY, detail: str = 'Invalid UUID') -> None: 
        super().__init__(status_code, detail)

2. Now there's no need for passing error body such as this one

          raise WrongUUIDError(  # should be default
                "Wrong uuid", error_response=self.error_response
            ) from e

3. ExceptionMiddleware handles HTTPException, which basically means it creates the Response.

Because of that:

• there's no need to accept response argument in plugins and middleware as they don't create any
• there's no need to handle ANY exception in middlewares. Non-HTTPException should result in 5xx.
• there's no need to do this await send weirdness as Starlette will handle that as well
• starlette-context users don't need to learn how it works. They just need to learn how exceptions in Starlette are handled (if they want to change a few things)

To recap, the problem you noticed is that Python built-in errors are raised which results in 5xx. As those are request-response errors, they can be handled by the framework. The only thing that has to be done is changing existing errors to HTTPException errors (or adding new ones) so the framework knows how to create a relevant response for the client.

What do you think?

[hhamana]

I had a slight concern that raising as an HTTPException would make it hard to customize the response on user side, but I think most apps would define their own error handling anyway, and this would integrate it nicely along with Starlette and such errors. That approach should be sufficient, and indeed simpler here too.

[hhamana]

My attempts to implement this based on HTTPException failed, for the exact same reason for the original implementation.
As per official Starlette documentation:

You should only raise HTTPException inside routing or endpoints. Middleware classes should instead just return appropriate responses directly.

The reason for this is, again from Starlette documentation:

the middleware stack of a Starlette application is configured like this:
- ServerErrorMiddleware - Returns 500 responses when server errors occur.
- Installed middleware
- ExceptionMiddleware - Deals with handled exceptions, and returns responses
- Router
- Endpoints

Due to this design, anything that is raised from a middleware will only bubble up to the ServerErrorMiddleware, which is only capable of 500 responses (or dirty hacks to override this and handle incorrect cases).
Middlewares MUST NOT raise. The HTTPException is no exception, as is it handled in the ExceptionMiddleware below in the stack.

The current implementation is correct.

[tomwojcik]

@hhamana I remember about your PR but I'm currently overwhelmed with non-side-projects responsibilities. I'm grateful for your contribution but I can't look into it right now. Sorry for that.

[tomwojcik]

I still don't like this optional response here and there but I think it is a must. I was playing with your branch and indeed I think what you did is the only way to handle plugin (middleware) error.

This PR looks really good. Thank you for your contribution and active discussion (and patience :D).

[hhamana]

Thank you very much.
I totally understand your feelings, sending a response object through the exception is weird, but I can't see any other way to make it work nicely with plugins.
Now that we agreed on that, I think some documentation would be in order. I hope I can write something soon-ish.