[GraphQL] Adding custom error format support

[mahmoodbazdar]

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	Fixes #3024
License	MIT
Doc PR	api-platform/docs#986

Allows developer to format exceptions based on exception type. For now I just added ValidationException support built in. I think we can add several others but I thought first its better to receive your feedback and then add them.
These are the exceptions I think its good to add builtin support:

• NotFoundException in here:

  core/src/GraphQl/Resolver/Stage/ReadStage.php

  Line 75 in 0354379

  throw Error::createLocatedError(sprintf('Item "%s" not found.', $args['input']['id']), $info->fieldNodes, $info->path);

• AccessDeniedException in here:

  core/src/GraphQl/Resolver/Stage/SecurityPostDenormalizeStage.php

  Line 61 in 0354379

  throw Error::createLocatedError($resourceMetadata->getGraphqlAttribute($operationName, 'security_post_denormalize_message', 'Access Denied.'), $info->fieldNodes, $info->path);

  
  and here:

  core/src/GraphQl/Resolver/Stage/SecurityStage.php

  Line 53 in 0354379

  throw Error::createLocatedError($resourceMetadata->getGraphqlAttribute($operationName, 'security_message', 'Access Denied.'), $info->fieldNodes, $info->path);

• InvalidArgumentException? in here:

  core/src/GraphQl/Resolver/Factory/ItemMutationResolverFactory.php

  Line 109 in 0354379

  throw Error::createLocatedError(sprintf('Custom mutation resolver "%s" has to return an item of class %s but returned an item of class %s.', $mutationResolverId, $resourceMetadata->getShortName(), (new \ReflectionClass($itemClass))->getShortName()), $info->fieldNodes, $info->path);

[lukasluecke]

@mahmoodbazdar Looks good, I like the change to have clearer validation messages. What do you think about moving the part you added in EntryPointAction to the Executor directly?

[mahmoodbazdar]

@lukasluecke I think its good Idea. Do you think we should apply to these exceptions too?

core/src/GraphQl/Action/EntrypointAction.php

Lines 77 to 79 in 0354379

	} catch (\Exception $e) {
	$executionResult = new ExecutionResult(null, [new Error($e->getMessage(), null, null, null, null, $e)]);
	}

If we want to support that we can't move it to Executor

[lukasluecke]

@mahmoodbazdar Good question - I'm not sure. Do you know which Exceptions this catch block actually applies to?

However this doesn't change anything - if we want to handle this part as well we need to extract the code somehow anyway. What do you think about a new global service that accepts an Error/Exception and just returns the correct formatted output? Then we could pass that to the Executor as a formatter, and also call it manually for the one-off exception in the catch. (Hope this is understandable 🙈)

[mahmoodbazdar]

@lukasluecke Its the exceptions that happen during GraphQL schema generation(Before GraphQL library calls for operation). They mainly happen because of user misconfiguration (500 errors).I'm not sure if we want to allow developers to format these kind of errors.

We can create this separate service but it should return a callable because setErrorFormatter only accept callable.

Simpler solution will be something like this:

$formatter = function (Error $error) {
      $formatters = $this->exceptionFormatterFactory->getExceptionFormatters();
      usort($formatters, function ($a, $b) {
          /**
           * @var ExceptionFormatterInterface
           * @var ExceptionFormatterInterface $b
           */
          if ($a->getPriority() == $b->getPriority()) {
              return 0;
          }

          return ($a->getPriority() > $b->getPriority()) ? -1 : 1;
      });
      /** @var ExceptionFormatterInterface $exceptionFormatter */
      foreach ($formatters as $exceptionFormatter) {
          if (null !== $error->getPrevious() && $exceptionFormatter->supports($error->getPrevious())) {
              return $exceptionFormatter->format($error);
          }
      }

      // falling back to default GraphQL error formatter
      return FormattedError::createFromException($error);
};
try {
    $executionResult = $this->executor->executeQuery($this->schemaBuilder->getSchema(), $query, null, null, $variables, $operation)
      ->setErrorFormatter($formatter);
} catch (\Exception $e) {
    $executionResult = (new ExecutionResult(null, [new Error($e->getMessage(), null, null, null, null, $e)]))
      ->setErrorFormatter($formatter);
}

[lukasluecke]

@mahmoodbazdar If the service has an __invoke-method it should be able to be used as a callable (this would be similar to the custom resolvers for queries and mutations). I think this would be the cleanest solution, as it would not pollute either the Executor or the EntrypointAction unnecessarily.

[mahmoodbazdar]

@lukasluecke You are right. I'll do that.

[lukasluecke]

Looks good! Maybe @alanpoulain can chime in on the additional exceptions listed in the issue description? I think it would be good to have more control over these as well 👍

[mahmoodbazdar]

@lukasluecke Thanks for your help and review.😊

[alanpoulain]

Thank you very much for your work @mahmoodbazdar!

After playing with your code, I realized using normalizers was preferable than introducing new interfaces and an exception formatter: it's a lot less code to maintain and it's more in the "API Platform's way" of doing this kind of things.

I've kept your code in the first commit and I added mine in the second one. Sorry for doing this, I know it would have been better if I commented your code instead of adding mine, but I needed to play and try some things to understand it correctly and I ended up in this state. I hope you will take this well but please tell me if you don't.

Could you also tell me if this approach is correct to you? @lukasluecke could you have a look too?

As you suggested, I added more normalizers:

• a HTTP exception one,
• a runtime exception one,

and I changed all the Error exceptions by classical PHP exceptions.

[mahmoodbazdar]

@alanpoulain I think your approach is better. Thanks fo updating the code.
About the error format I think I was wrong to add extra data it in first level of the array and we should add status and violations in extensions based on here:
https://github.com/graphql/graphql-spec/blob/master/spec/Section%207%20--%20Response.md
to follow the graphql spec.
What do you think?

[alanpoulain]

Oh yes you're right! Would you want to take care of it?

[mahmoodbazdar]

@alanpoulain Sure, Should I add tests for other normalizers (to fix code coverage failure)?

[alanpoulain]

I've added some tests for the other normalizers so I don't really understand if there is an issue or not (but I don't think there is).

[alanpoulain]

Merged at last! Thank you very much @mahmoodbazdar, I think this feature is very useful to have.

[CvekCoding]

@alanpoulain what do you think when this feature will be released?

[soyuka]

Hi @CvekCoding this was merged on master, master will be the next minor cycle, for now we're still on 2.5. We don't really have a due date for a new 2.6 release in the meantime you could use our dev-master version directly :/.

[CvekCoding]

@soyuka is it safe to use dev-master?