feat: error as resources, jsonld errors are now problem-compliant #5433

JacquesDurand · 2023-03-06T16:09:32Z

Q	A
Branch?	main
Tickets	#4994
License	MIT
Doc PR	api-platform/docs#...

Hello !

This is an attempt to implement RFC-7807 for hydra error responses

My current (draft) try on the implementation is the following:

In case 'jsonld' === $config['error_format']:
- From the thrown \Exception, construct a new kind of exception: ErrorException, and pass it to the ControllerArgumentEvent (cf the diff on src/Symfony/EventListener/ErrorListener.php. The goal of this operation is to progressively break the dependency on Symfony's FlattenException (and then potentially also link stuff from Laravel for instance)
- From this ErrorException, instantiate a new Error ApiResource in ExceptionAction, and normalize it in Hydra\ErrorNormalizer. This is mainly for '@id' purposes, and also maybe to enable user to create their own Error resources based on this one.

Note: this case is probably non RFC-7807 compliant, but still enables clearer Error serialization than there currently is

In case 'jsonproblem' === $config['error_format']:
- The same operation concerning theErrorException happens
- instantiate a ProblemError ApiResource instead, which matches the json schema required by the RFC in case the server returns an 'application/problem+json' response.
- Normalize it for hydra in Problem\ErrorNormalizer

BC Layer for now: enable/disable these error serializations via an extraProperty field on each resource (false by default), e.g.

#[ApiResource(extraProperties: ['hydra_errors' => true])]
class MyResource
{
...
}

As for now, what remains to be done (if this kind of implementation suits you):

A lot of tests
Documenting this feature
Add needed headers via AddLinkListener
Suggestion: creating an #[ErrorResource] Attribute to allow users to easily define their own errors with more precise details based on their needs ?

This is still a pretty large work in progress, if I am going in the completely wrong direction i'd be happy to change anything !

soyuka

This implementation looks good to me, please keep the ApiErrorResource for another PR we just want to be closer to the specification for now.

soyuka · 2023-03-06T16:17:50Z

src/Action/ExceptionAction.php

@@ -46,7 +49,7 @@ public function __construct(private readonly SerializerInterface $serializer, pr
    /**
     * Converts an exception to a JSON response.
     */
-    public function __invoke(FlattenException $exception, Request $request): Response
+    public function __invoke(FlattenException|ErrorExceptionInterface $exception, Request $request): Response


Suggested change

public function __invoke(FlattenException|ErrorExceptionInterface $exception, Request $request): Response

/**

* @param FlattenException|ErrorExceptionInterface $exception

*/

public function __invoke($exception, Request $request): Response

To avoid breaking if this class is used by users.

Is this considered breaking to add a union type ? I'm also not sure how this class could be directly used by users ?
One problem with your suggestion is that in ErrorListener::onControllerArgument, I need to check the types of the controller argument (with Reflection) to transform the \Throwable in an instance of ErrorExceptionInterface.
I am definitely open to other possibilities though

As long as a class is not marked @internal you can't change a public method argument type (https://symfony.com/doc/current/contributing/code/bc.html). If we can't, you need to deprecate this controller and create a new one.

soyuka · 2023-03-06T16:25:05Z

src/Problem/Serializer/ErrorNormalizer.php

@@ -35,7 +37,7 @@ final class ErrorNormalizer implements NormalizerInterface, CacheableSupportsMet
        self::TITLE => 'An error occurred',
    ];

-    public function __construct(private readonly bool $debug = false, array $defaultContext = [])
+    public function __construct(private readonly IriConverterInterface $iriConverter, private readonly bool $debug = false, array $defaultContext = [])


you can't break this class, the argument must be last position and nullable + add an expect deprecation although I'm quite sure we can handle it with a default IriConverter to avoid the deprecation alltogether.

soyuka · 2023-03-06T16:25:30Z

src/Action/ExceptionAction.php

@@ -71,7 +74,13 @@ public function __invoke(FlattenException $exception, Request $request): Respons
        $headers['X-Content-Type-Options'] = 'nosniff';
        $headers['X-Frame-Options'] = 'deny';

-        return new Response($this->serializer->serialize($exception, $format['key'], ['statusCode' => $statusCode]), $statusCode, $headers);
+        $error = match ($format['key']) {
+            'jsonproblem' => $exception instanceof FlattenException ? $exception : new ProblemError($exception, $request->getPathInfo()),


why the $request->getPathInfo() ? looks like a security issue

You're most probably right, I wasn't too sure of how to hydrate the 'instance' part of the problem+json response, this is just a "filler" part while waiting for a global opinion on this

Can you show me an example, I'm not sure I understand.

soyuka · 2023-03-06T16:26:41Z

src/Symfony/EventListener/ErrorListener.php

@@ -34,4 +41,47 @@ protected function duplicateRequest(\Throwable $exception, Request $request): Re

        return $dup;
    }
+
+    public function onControllerArguments(ControllerArgumentsEvent $event): void


is that code from Symfony? It looks quite heavy could you explain to me why you needed that?

It is indeed base on code from Symfony.
When an Exception is thrown, Symfony replays the request by duplicating it, adding an 'exception' key to it, and redirecting it to the Controller supposed to return a Response from the combo of the request and the exception (in our case ExceptionAction handles the duplicated request). Just before handling it though (onControllerArguments), Symfony transform the original Exception in a FlattenException, which is then serialized as part of the Response.
From one of our discussions, we wanted to break this dependency on FlattenException, reconstruct our own kind (here ErrorException), and build Error/ProblemError from it, hence the need of overriding Symfony's ErrorListener::onControllerArguments

Then again, I'm open to any suggestion as to make this in a cleaner way !

I didn't need to do this to get the original exception in my own implementation.

+++ ../src/Action/ExceptionAction.php @@ -20,8 +20,10 @@ use ApiPlatform\Util\OperationRequestInitiatorTrait; use ApiPlatform\Util\RequestAttributesExtractor; use Symfony\Component\ErrorHandler\Exception\FlattenException; +use Symfony\Component\HttpFoundation\Exception\RequestExceptionInterface; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; +use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface; use Symfony\Component\Serializer\SerializerInterface; /** @@ -46,12 +48,20 @@ /** * Converts an exception to a JSON response. */ - public function __invoke(FlattenException $exception, Request $request): Response + public function __invoke(\Throwable $exception, Request $request): Response { $operation = $this->initializeOperation($request); - $exceptionClass = $exception->getClass(); - $statusCode = $exception->getStatusCode(); + $exceptionClass = $exception::class; + $statusCode = 500; + $headers = []; + if ($exception instanceof HttpExceptionInterface) { + $statusCode = $exception->getStatusCode(); + $headers = $exception->getHeaders(); + } elseif ($exception instanceof RequestExceptionInterface) { + $statusCode = 400; + } + $exceptionToStatus = array_merge( $this->exceptionToStatus, $operation ? $operation->getExceptionToStatus() ?? [] : $this->getOperationExceptionToStatus($request) @@ -65,7 +75,6 @@ } } - $headers = $exception->getHeaders(); $format = ErrorFormatGuesser::guessErrorFormat($request, $this->errorFormats); $headers['Content-Type'] = sprintf('%s; charset=utf-8', $format['value'][0]); $headers['X-Content-Type-Options'] = 'nosniff';

This was all of the change that I needed to be able to get the actual exception.

alanpoulain · 2023-03-06T16:42:45Z

src/ApiResource/Error.php

+
+    private array $trace;
+
+    public function __construct(ErrorExceptionInterface $exception)


Use public readonly promoted constructor parameters instead of private with getter properties.

alanpoulain · 2023-03-06T16:43:09Z

src/ApiResource/Error.php

+use ApiPlatform\Metadata\NotExposed;
+
+#[NotExposed(uriTemplate: '/errors/{statusCode}')]
+class Error


Suggested change

class Error

final class Error

I was hesitant about this, should we (or not) allow users to extend it for their custom Errors ?

not the resource itself no

alanpoulain · 2023-03-06T16:45:42Z

src/Metadata/Resource/Factory/StaticResourceNameCollectionFactory.php

+
+use ApiPlatform\Metadata\Resource\ResourceNameCollection;
+
+final class StaticResourceNameCollectionFactory implements ResourceNameCollectionFactoryInterface


I don't like the "static" wording. Internal? Included?

I think that this will get removed as its not used? It is kind of static as it declares classes directly within the ResourceNameCollectionFactory. I'll use that for the Playground for example.

alanpoulain · 2023-03-06T16:46:51Z

src/Problem/Serializer/ErrorNormalizer.php

+            'title' => $object instanceof ProblemError ? $object->getTitle() : $context[self::TITLE] ?? $this->defaultContext[self::TITLE],
+            'detail' => $object instanceof ProblemError ? $object->getDetail() : $this->getErrorMessage($object, $context, $this->debug),
+            'instance' => $object instanceof ProblemError ? $object->getInstance() : null,
+            'status' => $object instanceof ProblemError ? $object->getStatus() : $context['statusCode'] ?? $object->getStatusCode(),


Isn't adding the status a BC?

Yeah I think adding 'status' and 'instance' breaks the current return responses, I should try to add more checks about the existence of the extraProperties: ['hydra_errors' => true]) for the current request

nesl247 · 2023-03-06T18:03:39Z

I'm not seeing type here, which for me is the biggest feature of RFC 7807.

That brings to me to what I think the big missing piece here, which is how the user is going to populate the type, title, detail, instance, etc. My suggestion is to introduce a new interface called ProblemException, which mandates the getters. If the exception is already a ProblemError, use those values, otherwise convert the exception to a ProblemError (extends \Exception), and then normalize the ProblemError.

soyuka · 2023-03-07T07:25:13Z

@nesl247 yes there is one: https://github.com/api-platform/core/pull/5433/files#diff-877c6e433ecaadc292cdac7d6b42f139dc48e86ab831a8fc489e7976a908a630R57

which is how the user is going to populate the type, title, detail, instance, etc.

We'll implement that in a second PR, as you can see the ProblemError is an ApiResource. A user that wants to extend this will be able to declare its own ProblemError and fill that as he wants. For now I'm not a huge fan of the "routing" (https://github.com/api-platform/core/pull/5433/files#diff-0c4b94869c82da23ed2573571540cf3f511737767c8f8a3462dc4106f6a16421R78) as if you want multiple ProblemError resources we'll need to find a way to use the correct one.

JacquesDurand · 2023-03-07T08:47:42Z

For now I'm not a huge fan of the "routing" (https://github.com/api-platform/core/pull/5433/files#diff-0c4b94869c82da23ed2573571540cf3f511737767c8f8a3462dc4106f6a16421R78) as if you want multiple ProblemError resources we'll need to find a way to use the correct one.

Not too fond of it either, any idea on how to make it more extensible for users ?

Edit (suggestion):

Would you be ok with some kind of "Exception to Error" mapping (similar to the current "Exception to Status" one) ?
For instance (pseudo code):

# config/packages/api_platform.yaml
api_platform:
    exception_to_error: 
        - App\Exception\MyCustomException: App\ProblemError\MyCustomError
...

class MyCustomException extends ProblemException implements ProblemExceptionInterface
{
   public $title;
   public $type;
   public $detail;
   public $instance;            // these values are assured by ProblemException
   public $status;

  
...
}

#[NotExposed(uriTemplate: '/my/custom/error')]
class MyCustomError extends ProblemError
{
   public $title;
   public $type;
   public $detail;
   public $instance;
   public $status;
...
}

Workflow:

// src/State/Processor/MyCustomProcessor.php

public function process( ...)
{
   // do stuff
  if ( $someCondition) {
    throw new MyCustomException($title, $type, $detail, $status, $instance)
  }
}

A bit later, in ExceptionAction :

public function __invoke(ErrorExceptionInterface|FlattenException $exception, Request $request)
{
        $operation = $this->initializeOperation($request);
        $exceptionClass = $exception->getClass();
        if ($errorClass = $this->matchExceptionToError($exceptionClass)) {
         $error = new $errorClass($exception);
    } else {
        $error = new ProblemError($exception);
    }
....
       return new Response($this->serializer->serialize($error), ....)

This might allow users, with relatively simple configuration, to create their own Exception and map them to Custom ProblemError, defaulting to the basic ProblemError for not setup exceptions.

If that suits the needs, I think it might be "easier" to implement it at the same time than the base specification, wdyt ?

nesl247 · 2023-03-07T14:33:42Z

@nesl247 yes there is one: #5433 (files)

which is how the user is going to populate the type, title, detail, instance, etc.

We'll implement that in a second PR, as you can see the ProblemError is an ApiResource. A user that wants to extend this will be able to declare its own ProblemError and fill that as he wants. For now I'm not a huge fan of the "routing" (#5433 (files)) as if you want multiple ProblemError resources we'll need to find a way to use the correct one.

I missed that because it's not in the ProblemError class, which doesn't make sense to me why it isn't.

One thing that seems like there is some confusion on is the difference between what the response is, and what the ProblemError ApiResource is supposed to be, unless I'm really reading the RFC wrong.

"type" (string) - A URI reference [RFC3986] that identifies the
      problem type.  This specification encourages that, when
      dereferenced, it provide human-readable documentation for the
      problem type (e.g., using HTML [W3C.REC-html5-20141028]).  When
      this member is not present, its value is assumed to be
      "about:blank".

As you can see, it's just supposed to be human-readable documentation. That doesn't necessitate having a status code, the details can't be there because that is only available when the response occurs, and the type must be static IIRC, the instance, and trace couldn't be there for the same reasons.

Semi-related: to keep it simple, internally we actually used a URN so it was an id, but didn't need to be browseable. A URN is a valid URI, so it kept it compliant.

JacquesDurand · 2023-03-07T14:52:02Z

I missed that because it's not in the ProblemError class, which doesn't make sense to me why it isn't.

@nesl247 Imo the type is going to be inferred from the ProblemError ApiResource itself, e.g. :

#[NotExposed(uriTemplate: '/problems/out-of-credit')]
class OutOfCreditError extends ProblemError
{
}

should directly produce a response with

{
    "type": "https://example.com/problems/out-of-credit"
...
}

Also this PR is mainly a draft prototype open to discussion, a lot might still change :)

Edit: did not see your edit.
I might have missed the 'human-readable' / HTML part, which indeed changes a few things

Edit 2

Maybe something like this ?

#[ApiResource(uriTemplate: '/problems/out-of-credit')]
class OutOfCreditError extends ProblemError
{
   // the other attributes needed for the error Response

   #[Group('some_serialization_group_available_only_through_the_type_uri')]
   public string $typeDescription = 'A generic description for an OutOfCreditError without specific details'
}

nesl247 · 2023-03-07T15:17:15Z

This is why I would scope this to using a urn by default, and letting the URI be manually set if a developer wants to link to a specific page somewhere. If it was wanted to support hosting this as part of api-platform, then I would do that as a separate feature. The problem with having to generate the documentation is how should it look, how will you support making it themeable and integrated into a company's UI, etc.

https://gist.github.com/nesl247/3ae6b1f9575e4dcd0960ca4a9016434e - this is a gist of how we implemented it. It could be extended to having a mapper if you need to control third party exceptions.

soyuka · 2023-03-08T08:44:47Z

Type is also an URI here so no problem on that. About the specification by end users, we want to handle this in another PR but basically you'd need to implement your own ErrorResource.

@JacquesDurand for the "routing" or "mapping" can't an exception be marked with the ErrorResource attribute
?

nesl247 · 2023-03-08T14:40:59Z

Type is also an URI here so no problem on that. About the specification by end users, we want to handle this in another PR but basically you'd need to implement your own ErrorResource.

@JacquesDurand for the "routing" or "mapping" can't an exception be marked with the ErrorResource attribute ?

I still think there is some confusion here @soyuka, but I think @JacquesDurand understands it.

There's 2 parts to the RFC, the response and the documentation endpoint (what type is supposed to point to if not a URN). The first part, which IMO should be the main focus, is the error response. The current implementation conflates the two. The error response is the only thing that really needs to be implemented by api-platform.

Having a default ErrorResource IMO will be 99% useless IMO, as the point is for human readable documentation. If the default is just "Unknown error", there is no point to having said documentation. Also as I mentioned earlier, dealing with the human readable documentation aspect of this will bring a host of other problems regarding appearance customization (we need twig, etc.). Also, I would imagine most uses would rather have this link to a knowledge base or something of the sort.

Also, the ErrorResource should only be a path and return some sort of human readable documentation. It should not have status, detail, instance, etc, as the type is not supposed to change, as it's an id of the "problem" "type", not of the "problem" "instance".

I'm happy to discuss this more in Slack as well if that would be easier. I'm nesl247 in the Symfony Devs Slack community.

nesl247 · 2023-03-13T14:40:30Z

src/ApiResource/ProblemError.php

+        return $this->instance;
+    }
+
+    public function setInstance(?string $instance): void


Wouldn't it be better to make the object immutable either by never allowing it to be changed, or to use withers instead?

soyuka · 2023-04-28T12:19:35Z

src/State/DefaultProblemErrorProvider.php

+        $identifiers = [
+            'uri_variables' => ['status' => $problem->getStatus()],
+        ];
+        $problem->setType($this->iriConverter->getIriFromResource($problem, context: $identifiers));


isn't that done by the normalizer already?

soyuka mentioned this pull request Mar 6, 2023

RFC-7807 compatible hydra error response #4994

Closed

soyuka reviewed Mar 6, 2023

View reviewed changes

alanpoulain reviewed Mar 6, 2023

View reviewed changes

JacquesDurand force-pushed the feat/hydra-errors branch 13 times, most recently from 80a51a8 to 6a33ba7 Compare March 13, 2023 13:55

nesl247 reviewed Mar 13, 2023

View reviewed changes

JacquesDurand force-pushed the feat/hydra-errors branch 2 times, most recently from 534117d to 43c7ed4 Compare March 14, 2023 09:53

JacquesDurand force-pushed the feat/hydra-errors branch 7 times, most recently from b23baaf to c6a0309 Compare April 28, 2023 07:26

soyuka reviewed Apr 28, 2023

View reviewed changes

JacquesDurand force-pushed the feat/hydra-errors branch from f4e78cc to 0d0d83b Compare May 3, 2023 09:36

soyuka force-pushed the feat/hydra-errors branch from 92f08eb to ee67a66 Compare May 11, 2023 14:10

soyuka changed the title ~~Feat/hydra errors~~ feat: error as resources, jsonld errors are now problem-compliant May 11, 2023

soyuka force-pushed the feat/hydra-errors branch from ee67a66 to 1486ab6 Compare May 11, 2023 14:18

soyuka marked this pull request as ready for review May 11, 2023 14:38

soyuka force-pushed the feat/hydra-errors branch from 1486ab6 to c890572 Compare May 11, 2023 14:41

soyuka added the [RFR] Ready for Review label May 22, 2023

soyuka added this to the 3.2 milestone May 22, 2023

JacquesDurand and others added 5 commits June 5, 2023 10:14

feat: exception as resource

d4c3f9d

tests

40cf82f

phpstan

f79e22e

last fixes

e8ecc76

self review

44d50ad

soyuka force-pushed the feat/hydra-errors branch from c24eb36 to 44d50ad Compare June 5, 2023 08:16

soyuka added 4 commits June 5, 2023 10:22

oops

556a67d

tests

1ecf496

move jsonapi

1e97c6d

tests

8c304c6

soyuka merged commit 4ef0ef8 into api-platform:main Jun 5, 2023
15 of 27 checks passed

weaverryan mentioned this pull request Aug 3, 2023

Warning: Undefined array key "previous_data" from DefaultErrorProvider #5715

Closed

jotwea mentioned this pull request Sep 22, 2023

exclude error ressources from graphql #5848

Merged

GwendolenLynch mentioned this pull request Mar 22, 2024

Multiple error routes although format is not configured #6214

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: error as resources, jsonld errors are now problem-compliant #5433

feat: error as resources, jsonld errors are now problem-compliant #5433

JacquesDurand commented Mar 6, 2023 •

edited

Loading

soyuka left a comment

soyuka Mar 6, 2023

JacquesDurand Mar 7, 2023

soyuka Mar 8, 2023

soyuka Mar 6, 2023

soyuka Mar 6, 2023

JacquesDurand Mar 7, 2023

soyuka Mar 8, 2023

soyuka Mar 6, 2023

JacquesDurand Mar 7, 2023

nesl247 Mar 7, 2023

alanpoulain Mar 6, 2023

alanpoulain Mar 6, 2023

JacquesDurand Mar 7, 2023

soyuka Mar 8, 2023

alanpoulain Mar 6, 2023

soyuka Mar 7, 2023 •

edited

Loading

alanpoulain Mar 6, 2023

JacquesDurand Mar 7, 2023

nesl247 commented Mar 6, 2023

soyuka commented Mar 7, 2023

JacquesDurand commented Mar 7, 2023 •

edited

Loading

nesl247 commented Mar 7, 2023 •

edited

Loading

JacquesDurand commented Mar 7, 2023 •

edited

Loading

nesl247 commented Mar 7, 2023 •

edited

Loading

soyuka commented Mar 8, 2023 •

edited

Loading

nesl247 commented Mar 8, 2023

nesl247 Mar 13, 2023

soyuka Apr 28, 2023

-    public function __invoke(FlattenException|ErrorExceptionInterface $exception, Request $request): Response
+    /**
+     * @param FlattenException|ErrorExceptionInterface $exception
+     */
+    public function __invoke($exception, Request $request): Response


		private array $trace;

		public function __construct(ErrorExceptionInterface $exception)


		use ApiPlatform\Metadata\Resource\ResourceNameCollection;

		final class StaticResourceNameCollectionFactory implements ResourceNameCollectionFactoryInterface

feat: error as resources, jsonld errors are now problem-compliant #5433

feat: error as resources, jsonld errors are now problem-compliant #5433

Conversation

JacquesDurand commented Mar 6, 2023 • edited Loading

soyuka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soyuka Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nesl247 commented Mar 6, 2023

soyuka commented Mar 7, 2023

JacquesDurand commented Mar 7, 2023 • edited Loading

nesl247 commented Mar 7, 2023 • edited Loading

JacquesDurand commented Mar 7, 2023 • edited Loading

nesl247 commented Mar 7, 2023 • edited Loading

soyuka commented Mar 8, 2023 • edited Loading

nesl247 commented Mar 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacquesDurand commented Mar 6, 2023 •

edited

Loading

soyuka Mar 7, 2023 •

edited

Loading

JacquesDurand commented Mar 7, 2023 •

edited

Loading

nesl247 commented Mar 7, 2023 •

edited

Loading

JacquesDurand commented Mar 7, 2023 •

edited

Loading

nesl247 commented Mar 7, 2023 •

edited

Loading

soyuka commented Mar 8, 2023 •

edited

Loading