Exception system usability

[elazar]

In the process of incorporating the middleware from this package into a Slim-based service I'm building, I wrote a separate middleware to handle errors from this one. I've enclosed what I've come up with so far below.

My middleware is admittedly more complex than I would like, but I think this is reflective of this package's current exception system. I developed this middleware iteratively by throwing requests at the service for basic use cases: an unrecognized method, an unsupported method, a body that didn't conform to the endpoint's schema, etc.

Here are a few specific points of note.

1. There is no central base class from which all other exceptions extend, making it more difficult to implement a comprehensive handler for exceptions from this package.
2. While it's good that exception classes make use of the $previous parameter, exception chains can make it tedious to get to the crux of a problem.
3. While there are some areas that could be subject to debate (e.g. using a 422 response code for an invalid request payload, as opposed to say 400), there are also areas with an intuitive established behavior (e.g. 404 for an unknown path, 405 for an unsupported method, etc.). It would be nice if this package could handle commonly agreed upon cases, perhaps with an additional middleware.

I don't have a concrete solution to this, so I'm opening this issue mainly to spark discussion around this subject, whether in the form of BC-breaking changes to the exception system in this package's next major version or an additional middleware to encapsulate the complexity of handling exceptions for common use cases.

use League\OpenAPIValidation\PSR15\Exception\InvalidServerRequestMessage;
use League\OpenAPIValidation\PSR7\Exception\NoOperation;
use League\OpenAPIValidation\PSR7\Exception\ValidationFailed;
use League\OpenAPIValidation\PSR7\Exception\Validation\InvalidBody;
use League\OpenAPIValidation\Schema\Exception\SchemaMismatch;
use Fig\Http\Message\StatusCodeInterface as Status;
use Psr\Http\Message\ResponseFactoryInterface as ResponseFactory;
use Psr\Http\Message\ResponseInterface as Response;
use Psr\Http\Message\ServerRequestInterface as Request;
use Psr\Http\Server\MiddlewareInterface;
use Psr\Http\Server\RequestHandlerInterface;
use Slim\Exception\HttpException;
use Throwable;

class ErrorMiddleware implements MiddlewareInterface
{
    private ResponseFactory $responseFactory;

    public function __construct(ResponseFactory $responseFactory)
    {
        $this->responseFactory = $responseFactory;
    }

    public function process(Request $request, RequestHandlerInterface $handler): Response
    {
        try {
            return $handler->handle($request);
        } catch (Throwable $e) {
            return $this->handleException($request, $e);
        }
    }

    private function handleException(Request $request, Throwable $exception): Response
    {
        while ($exception instanceof InvalidServerRequestMessage
            || $exception instanceof InvalidBody) {
            $exception = $exception->getPrevious();
        }

        if ($exception instanceof NoOperation) {
            $route = $request->getAttribute('route');
            if (empty($route)) {
                $code = Status::STATUS_NOT_FOUND;
                $message = 'Path not found: ' . $request->getUri()->getPath();
            } else {
                $code = Status::STATUS_METHOD_NOT_ALLOWED;
                $message = 'Path does not support method: ' . $request->getMethod();
            }
        } elseif ($exception instanceof ValidationFailed || $exception instanceof SchemaMismatch) {
            $code = Status::STATUS_UNPROCESSABLE_ENTITY;
            $message = $exception->getMessage();
        } else {
            $code = Status::STATUS_INTERNAL_SERVER_ERROR;
            $message = get_class($exception) . ': ' . $exception->getMessage();
        }

        $response = $this->responseFactory->createResponse($code)
            ->withHeader('Content-Type', 'application/json');
        $response->getBody()->write(json_encode(['message' => $message]));
        return $response;
    }
}

[scaytrase]

Hi, sorry for long delay

There is no central base class from which all other exceptions extend, making it more difficult to implement a comprehensive handler for exceptions from this package.

What about ValidationFailed ? If SchemaMismatch is leaking out in PSR7 validator - this must be fixed. This code clearly states that MessageValidator throws only ValidationFailed

https://github.com/thephpleague/openapi-psr7-validator/blob/c7539387382fd3217cf2e254ed8c32ed44ab4f2e/src/PSR7/MessageValidator.php#L15

(NoPath is a typo and subtype of ValidationFailed so it can be omitted)

While it's good that exception classes make use of the $previous parameter, exception chains can make it tedious to get to the crux of a problem.

I didn't get this point at all, sorry. Chaining exceptions is best practice. What is your suggestion?

While there are some areas that could be subject to debate (e.g. using a 422 response code for an invalid request payload, as opposed to say 400), there are also areas with an intuitive established behavior (e.g. 404 for an unknown path, 405 for an unsupported method, etc.). It would be nice if this package could handle commonly agreed upon cases, perhaps with an additional middleware.

We've already discussed it with @lezhnev74 and we won't provide any logic about rendering validation results in this package. Slim middleware is historical legacy here, so we're not removing it for now.

But we will be happy if one is built around this package as a separate package.

[elazar]

What about ValidationFailed ? If SchemaMismatch is leaking out in PSR7 validator - this must be fixed. This code clearly states that MessageValidator throws only ValidationFailed

Fair enough, I stand corrected on this point.

I didn't get this point at all, sorry. Chaining exceptions is best practice. What is your suggestion?

Best practice for allowing debugging to be conducted through an application layer and multiple levels of supporting libraries, sure.

However, chaining within the same library can make it more difficult to get to the exception that initially caused a given issue. It requires a loop similar to this one to get to the root exception.

 while ($exception instanceof InvalidServerRequestMessage
    || $exception instanceof InvalidBody) {
    $exception = $exception->getPrevious();
}

The last exception on the chain (i.e. the one that ends up bubbling up) should have a message that makes the root problem and how to fix it obvious.

For example, why are exceptions chained in these places, rather than just letting the originally thrown exceptions bubble up? What value does that add?

src/PSR7/Validators/ArrayValidator.php: throw InvalidParameter::becauseValueDidNotMatchSchema($name, $argumentValue, $e);
src/PSR7/Validators/BodyValidator/MultipartValidator.php: throw InvalidBody::becauseBodyDoesNotMatchSchema($this->contentType, $addr, $e);
src/PSR7/Validators/BodyValidator/MultipartValidator.php: throw InvalidHeaders::becauseValueDoesNotMatchSchemaMultipart($partName, $headerName, $headerValue, $addr, $e);
src/PSR7/Validators/BodyValidator/MultipartValidator.php: throw InvalidBody::becauseBodyDoesNotMatchSchema($this->contentType, $addr, $e);
src/PSR7/Validators/BodyValidator/UnipartValidator.php: throw InvalidBody::becauseBodyDoesNotMatchSchema($this->contentType, $addr, $e);
src/PSR7/Validators/BodyValidator/FormUrlencodedValidator.php: throw InvalidBody::becauseBodyDoesNotMatchSchema($this->contentType, $addr, $e);
src/PSR7/Validators/CookiesValidator/RequestCookieValidator.php: throw InvalidCookies::becauseValueDoesNotMatchSchema($cookieName, $cookie, $addr, $e);
src/PSR7/Validators/QueryArgumentsValidator.php: throw InvalidQueryArgs::becauseOfMissingRequiredArgument($e->name(), $addr, $e);
src/PSR7/Validators/QueryArgumentsValidator.php: throw InvalidQueryArgs::becauseValueDoesNotMatchSchema($e->name(), $e->value(), $addr, $e);
src/PSR7/Validators/PathValidator.php: throw InvalidPath::becauseValueDoesNotMatchSchema($e->name(), $e->value(), $addr, $e);

We've already discussed it with @lezhnev74 and we won't provide any logic about rendering validation results in this package. Slim middleware is historical legacy here, so we're not removing it for now.

But we will be happy if one is built around this package as a separate package.

Was this discussed on an issue that I could review? I'm trying to understand the reasoning behind this.

As it stands now, any new user of this library has to go through the same process I did of iteratively writing code to handle common exceptions (e.g. 404, 405) with corresponding responses. A lot of reasonable behaviors could be implemented in a separate middleware in this library (to make the behavior opt-in and not break BC) that would make it more user-friendly.

Nothing says this has to be Slim-specific; my Slim example just happened to be the one I had most readily available to illustrate my points. There may be a PSR-7/15-compatible way to handle this.

[scaytrase]

A lot of reasonable behaviors could be implemented in a separate middleware in this library (to make the behavior opt-in and not break BC) that would make it more user-friendly.

There is no real difference between writing separate backage depending on this one and implementing as a part of this one for end-users. The real difference is for maintainers. Each middleware can represent it's own use-case. I.e we can have psr-7, slim, symfony middlewares. For each middleware we can be asked to do any kind of improvements like logging (with customizable levels). You can be asked to tune your response (i.e add custom message, or other content to the response body, headers, and so on). This potentinally will lead to the maintenance pit to make code suitable for each usecase not breaking others.

So I'm not saying that implementing any kind of logic around validator is bad, no. I'm just saying custom use-cases shouldn't be the part of this package.

For example @osteel implemented this package for testing purposes around validator and sf/http-foundation https://github.com/osteel/openapi-httpfoundation-testing

Back to the story

For example, why are exceptions chained in these places, rather than just letting the originally thrown exceptions bubble up? What value does that add?

The same reasoning I've mentioned in first message. SchemaMismatch is thrown for validating custom data against object spec. You cannot use it in PSR-7 validation, since there you're validating psr-7 message against OAS3 Spec. Because you can get invalid message in any part of http message - body, headers (cookies as special case), path, query, etc.

There are also some unique cases, for example when your message matches multiple paths, but doesn't match any of them by schema. You cannot clearly say which schema validation failed.

Also there is cases when data from the message was not validated at all. It happens when your http message doesn't match any given route in spec.

So in general there is the following logic:

ValidationFailed is the top level exception for validating HTTP messages against OAS3 spec. In particulart there are following subcases for now:

• Routing failed - which leads to NoPath (404), NoOperation (405) (if we speaking of request) and also NoResponseCode (for responses)
• Ambiguous routingbut doesn't match any of matched operation schema - MultipleOperationsMismatchForRequest
• Routing ok, matches single operation - AddressValidationFailed

AddressValidationFailed in turn has several cases depending on what part of schema failed: Headers, Path, Cookies, Body, etc. There is one outlier - RequiredParameterMissing. This one should be rewritten to match one of the parts validator.

AddressValidationFailed in general has two use cases

• when there is no required value
• when value (data) doesn't match object spec

For second we come to the SchemaMismatch. There are a lot of subcases, but in general on this level you only need message and breadcrumb from the exception.

So in general with PSR-7 validator you always have chain of ValidationFailed (i.e InvalidBody ) -> SchemaMismatch (if applicable, i.e KeywordMismatch)

Using PSR-15 middleware gives you two more cases - RequestValidationFailed and ResponseValidationFailed, since middleware process both of them at once, and you need to know which body - request or response - has failed. So for middleware you always get a chain of

ResponseValidationFailed|InvalidServerRequestMessage (to know which one failed) -> ValidationFailed (i.e InvalidBody ) -> SchemaMismatch (if applicable, i.e KeywordMismatch)

So If you not misusing use-cases you should always have the same parsing path for the exception.

[osteel]

I'm actually considering adding my own layer of exception instead of just passing on this package's like I do at the moment, because it's true that the root cause of the validation issues is quite buried at present