Error handling with 3.2 is inconsistent & difficult

[j-schumann]

Description
With 3.2 there are two different ways in which error responses are created:

1. When the problem occurs while routing or authentication, Symfony's own ProblemNormalizer is used.
2. When the problem occurs within ApiPlatforms Processors/Providers an ApiResource\Error is created and serialized. (At least when using no deprecated features / the new problem responses)

This leads to multiple problems:

• returned content + headers change for similar errors / same status codes: see Inconsistent content-type header & JSON structure for error/problem responses in 3.2.x #5961
• We can no longer decorate 'api_platform.hydra.normalizer.error' to modify all returned error responses, e.g. to translate the returned error 'detail' / 'hydra:description'.
  • instead we have to create a new Processor to decorate 'api_platform.state_processor.serialize' to modify the ApiResource\Error error before it is serialized
  • and modify our existing serializer to instead decorate 'serializer.normalizer.problem' to do the same thing but on an array
• ApiResource\Error is immutable, which requires us to create a new instance to modify a single value, e.g. the 'detail'
  • Why are there no setters, and if it is required to be immutable, can we please get ->with...() clone methods like most of the ApiPlatform\OpenApi\Model classes have (withParameters, withTrace, withDescription, ...)? Also the ORM|ODM|ElasticSearch\State\Options or the MetaData class(es) have them.

The feature request would be to simplify the modification of the error results, for all error sources together and to better document how this can be done. Currently the docs (https://api-platform.com/docs/core/errors/) neither shows how to use a Provider nor a Serializer to modify the response, and it does not mention the (current) differences in error handling & output.

Example
We want to translate the error messages generated by Symfony's routing / Lexik JWT / Validators / custom exceptions to be parseable by the API-Client.
Currently we have to use:

    $clone = new Error(
        $data->getTitle(),
        $this->translator->trans($data->getDetail(), [], 'validators'),
        $data->getStatus(),
        $data->originalTrace,
        $data->getInstance(),
        $data->getType(),
        $data->getHeaders(),
    );

and

        $result['detail'] = $this->translator
            ->trans($result['detail'], [], 'validators');

        // currently not available because of #5961 
        //$result['hydra:description'] = $this->translator
        //    ->trans($result['[detail](hydra:description)'], [], 'validators');

which also causes the Error constructor to iterate over the whole stacktrace again to remove the (already removed) args. (yes, we could first provide an empty array as trace and later simply set $data->originalTrace, see below)

Additional context

    defaults:
        extra_properties:
          rfc_7807_compliant_errors: true
          skip_deprecated_exception_normalizers: true
    event_listeners_backward_compatibility_layer: false
    keep_legacy_inflector: false

Additional questions / feedback

• Why is (only) the originalTrace public in ApiResource\Error? What was the reason to not use a getter like with all other properties and an additional setter?
• What is the reason behind making all properties private instead of protected? This prevents extending the Error with a custom subclass as it cannot access those props. It is neither marked final nor internal (which is a good thing imho).
  • This is a theme I see often in ApiPlatform (but also in Symfony in newer history): Classes are marked final or most/all properties and methods are private which prevents inheritance, which often causes repetition as we only want to change a small portion of the code but need to copy everything else as it cannot be re-used.

[nesl247]

#4997 is related. It got auto close unfortunately :(

[soyuka]

I really need to document this better as you don't need so much complexity. For now you can disable the feature rfc_7807_compliant_errors: true right?

Why is (only) the originalTrace public in ApiResource\Error? What was the reason to not use a getter like with all other properties and an additional setter?

Because you can't create an exception from another exception, the trace will be replaced by PHP.

What is the reason behind making all properties private instead of protected? This prevents extending the Error with a custom subclass as it cannot access those props. It is neither marked final nor internal (which is a good thing imho).

IMHO it should be internal, I really got submerged with a bit too much work (especially working on #5882) and I should've spent more time into documenting this. My idea was that you should be able to own your exceptions and as these can be resources you can put everything you need without having to add normalizers (which are a pain).

Maybe that you can help me document this and see if something like this works (I don't have much time to try this out until next week).

1. Declare your own error resource (just like ApiPlatform/ApiResource/Error)
2. If you want to change how data looks create a Provider on this Error and use $context->attributes->get('exception') to get the actual exception.
3. You may need to link the actual Exception class to your Error (see

   core/src/Symfony/EventListener/ErrorListener.php

   Line 80 in b1b4f14

   if ($this->resourceClassResolver?->isResourceClass($exception::class)) {

   ) and this is probably the "hard part" I see a few solutions though:

• specify class inside the ErrorResource attribute
• catch exceptions with a symfony listener and transform them to your own exception which is a resource
• add a configuration to be able to use your own Error resource instead of ours ?

Let's help each others implementing this, I wish this was a little more mature and the whole work behind errors was meant to fix issues like #4997 not make things more complicated...

[j-schumann]

Thanks Antoine for your detailed answer!

I could help with documenting this, but first I need to fully understand this.

For now you can disable the feature rfc_7807_compliant_errors: true right?

This does not help, because even then Symfony's ProblemNormalizer is called:

ProblemNormalizer.php:63, Symfony\Component\Serializer\Normalizer\ProblemNormalizer->normalize()
ErrorNormalizer.php:31, App\Serializer\ErrorNormalizer->normalize()
Serializer.php:159, Symfony\Component\Serializer\Serializer->normalize()
Serializer.php:138, Symfony\Component\Serializer\Serializer->serialize()
SerializerErrorRenderer.php:60, Symfony\Component\ErrorHandler\ErrorRenderer\SerializerErrorRenderer->render()
ErrorController.php:41, Symfony\Component\HttpKernel\Controller\ErrorController->__invoke()
HttpKernel.php:181, Symfony\Component\HttpKernel\HttpKernel->handleRaw()
HttpKernel.php:76, Symfony\Component\HttpKernel\HttpKernel->handle()
ErrorListener.php:108, Symfony\Component\HttpKernel\EventListener\ErrorListener->onKernelException()
WrappedListener.php:116, Symfony\Component\EventDispatcher\Debug\WrappedListener->__invoke()
EventDispatcher.php:220, Symfony\Component\EventDispatcher\EventDispatcher->callListeners()
EventDispatcher.php:56, Symfony\Component\EventDispatcher\EventDispatcher->dispatch()
TraceableEventDispatcher.php:139, Symfony\Component\EventDispatcher\Debug\TraceableEventDispatcher->dispatch()
HttpKernel.php:239, Symfony\Component\HttpKernel\HttpKernel->handleThrowable()
HttpKernel.php:91, Symfony\Component\HttpKernel\HttpKernel->handle()
Kernel.php:197, Symfony\Component\HttpKernel\Kernel->handle()
[... phpunit]

I'm not sure if this is because of another configuration that should be changed. But actually I think it is a good thing to have this new standards compliant responses. But for all the existing clients to our API there should be no b/c break, because they all check for 'hydra:description', which currently is not present for routing errors (see #5961).

And this ProblemNormalizer creates the rfc_7807 JSON structure (but without the problem content-type header) that is later not decorated with the required Hydra props.

Because you can't create an exception from another exception, the trace will be replaced by PHP.

Yes, creating a new ApiResource\Error creates a new internal trace, but we are talking about the originalTrace which is already supplied to the constructor as array. So it could hold any arbitrary content already + the property is not overwritten from the base \Exception, what prevents making the property private and adding getters/setters?

My idea was that you should be able to own your exceptions and as these can be resources you can put everything you need without having to add normalizers (which are a pain).

It seems we misunderstand each other here: We do not want to modify the result just for some special endpoint/use case/exception. But (be able to) translate all error messages, regardless of source & type. Please see my example for reference. E.g. we translate symfony's "Invalid credentials." from BadCredentialsException to a parseable string as well as generic validation errors or custom exception messages.
Or did you mean we should create our own Error resource and register it so that it replaces ApiPlatform\ApiResource\Error completely? I would guess that is not fully possible, for example the ElasticSearch collection/item providers throw a new ApiResource\Error with no means to change it to another class.

Declare your own error resource (just like ApiPlatform/ApiResource/Error)

As above: I don't think that solves the problem. The main problem imho is, that currently not all errors that can occur are transformed to an ApiResource\Error which is then appropriatly handled / decorated with Hydra props / sent with the problem header by ApiPlatform. But that some (routing & auth) errors go through symfony's own error handling/serialization.

catch exceptions with a symfony listener and transform them to your own exception which is a resource

Probably this should be a new functionality in ApiPlatform: Intercept all errors thrown in Symfony (404, 405 from routing problems; 400, 401 from auth problems etc) and feed them into APIP's processor chain.

[Update:]
I looked at the code and saw that ApiPlatform\Symfony\EventListener\ExceptionListener is the reason that opens up two different ways of error handling, when it decides: "Normalize exceptions only for routes managed by API Platform"

Sure, APIP should not impose itself on other parts of the application that handle other routes. But that's difficult to decide: Currently, the ExceptionListener logic also decides that calling an APIP endpoint with the wrong HTTP method is not handled, which is (for me atleast) an unpreferable result. This also causes the problem detailed in #4997 as authentication routes - by lexikJwt or other auth systems - are exempt. But this also has two sides: LexikJwt answers in application/json for success responses and authentication errors per default. Simply disabling the check in the ExceptionListener only causes "error 400" responses (because of missing credentials) to be enhanced as Hydra responses, not all (success or error) responses.

I tried simply disabling the route check in the ExceptionListener, this already helped so far as 405 errors get the correct Hydra response. But they still don't get the problem content-header.
[end update]

Also, my problem with the ApiResource\Error itself would go away if there was a way to intercept APIP's serialization process of it. But currently the SerializeProcessor takes the Error object and spews out a JSON string. So to translate the 'detail' or 'hydra:description' we would have to go back into symfony's serializer chain - probably with another Normalizer (that you wanted to get rid of) - or decode the JSON, translate and encode again, which is not a good solution either.

[acirulis]

We really need a way to preserve old behavior (before 3.2). API is used by frontend client, which relays on specific error format (hydra:description, etc)

Currently both config options:
skip_deprecated_exception_normalizers: false
rfc_7807_compliant_errors: false
does not help.

[soyuka]

Hi everyone sorry really got caught up but we have lots of issues with errors right now so tomorrow I'll provide the 3.1 ErrorListener back with a flag to use the new system. This will give me a bit more time to work on this as there are too many things that are not working properly! Sorry about this, I really wanted to push forward this Error to Resource thing (which should be really helpful) but it looks like the implementation is too flaky right now.

In the meantime you can override our ErrorListener service with something like:

    protected function duplicateRequest(\Throwable $exception, Request $request): Request
    {
        $this->controller = 'error_controller';
        return parent::duplicateRequest($exception, $request);
     }

But I'd recommend to stay on 3.1 while I provide a proper fix. Thank you for your patience!

[soyuka]

Please update to 3.2.5 and use rfc_7807_compliant_errors: false nothing should break as I fallback on the 3.1 code. I removed the deprecated normalizers for now and will introduce this back in 3.3 if needed.
Let me know if using false works.

I also found a way to make rfc_7807_compliant_errors: true actually usable but my priority is on fixing the API Platform website right now.

[mbrodala]

Not trying to complain too much for such an awesome platform given to us for free but really the latest error changes are a mess, especially because they are too complex to understand and not really well documented.

For example the latest update from 3.2.4 to 3.2.5 broke all of our tests because validation errors did not contain a violations key anymore. This could be fixed by this:

api_platform:
    defaults:
        extra_properties:
            rfc_7807_compliant_errors: true

So we need to enable the RFC7807 errors to restore the violations key and thus restore the structure from 3.1.x. You can imagine that I don't understand anything anymore. 😅

[soyuka]

Indeed I'm reproducing that I tested manually I will add this to the test matrix!

[soyuka]

Can someone try #5974 for the bc layer? I still need a bit more time to add this to our test suite but it looks promising with manual tests.

[j-schumann]

Can someone try #5974 for the bc layer? I still need a bit more time to add this to our test suite but it looks promising with manual tests.

A very quick (!) test fixes the default behavior for me. Just tested a simple 404 to a non-existant route.

[soyuka]

well it took quite some time but I'm close to something better which handles a proper bc layer and also makes that 3.2 system actually usable (it's not if you can't customize exceptions). It should be out soon I need to try a few more things manually. In my patch I reverted everything to the previous error handling when the flag is false and I added a way to customize exceptions using true!

Really sorry for the delay, I had to work (a lot) on the documentation as we had issues using Next.js and therefore we couldn't update the documentation pages. By the end of the week there will be a proper documentation page to help with the new Error stuff.

Stay tuned!

[acirulis]

So, tried "update to 3.2.5 and use rfc_7807_compliant_errors: false":

BEFORE 3.2.x:

Lets take login 401 error (APP_DEBUG=false):

{
    "@context": "/api/contexts/Error",
    "@type": "hydra:Error",
    "hydra:title": "An error occurred",
    "hydra:description": "The presented password is invalid."
}

WITH 3.2.5 and rfc_7807_compliant_errors: false:

{
    "type": "https://tools.ietf.org/html/rfc2616#section-10",
    "title": "An error occurred",
    "status": 401,
    "detail": "Unauthorized"
}

So completely different result and even error response "The presented password is invalid" is removed.

WITH 3.2.5 and rfc_7807_complient_errors: true:

{
    "title": "An error occurred",
    "detail": "The presented password is invalid.",
    "status": 401,
    "type": "/errors/401"
}

better, but frontend client needs rewrite.