Provide minimal spec-compliant error format

[spawnia]

I strongly believe we should fully comply with http://spec.graphql.org/draft/#sec-Errors.Error-result-format:

GraphQL services should not provide any additional entries to the error format since they could conflict with additional entries that may be added in future versions of this specification.

If individual services decide to add such entries, that is not even as bad. Given this library is supposed to be a non-opinionated base implemenation of the spec, we absolutely should not.

I also propose to remove the category field entirely. It bloats the implementation of the ClientAware interface unnecessarily.

As a replacement, i added the ProvidesExtensions interface, which allows any error class to add arbitrary additional data to the formatted error.

[vladar]

I agree with you but only in part. Errors are pretty loosely defined in GraphQL and it is probably one of the most obvious areas for improvement in the spec.

At the moment everyone deals with errors on their own but I think the spec will eventually come up with some common way to express various categories of errors, error ids, etc.

And I would like to avoid the churn of breaking changes until we know the direction of the spec in this regard.

[spawnia]

My motivation for this PR is that we are about to completely revamp error handling in our large application. Our goal is to introduce discrete classes for every single error.

Since we do use GraphQL heavily, we want to implement the ClientAware interface in every one of them. All we want to do is say if the message is safe to show or not. We currently neither care about the category, nor do we plan to introduce such a concept in our implementation. However, the way the interface is currently compounding those two things forces us to implement it and have it shown in the response (or hack around it with a formatter).

For existing users of the category field, there is a clear migration path: they can implement the ProvidesExtensions interface and add such a field there. Other users won't have to change anything, their ClientAware classes will continue to work as before. Adding category back in through a custom formatter won't be too hard to do either, we could even provide an example implementation of that as part of the upgrade guide.

As to the spec, I found two issues related to error codes:

• [RFC] Standard Error Codes graphql/graphql-spec#708
• Discussion: Generic error codes in the spec graphql/graphql-spec#698

Both talk about adding a key code, never once do I see any mention of the term category. If our implementation were to exactly coincide with an ongoing RFC, I could see the point of keeping what we have now. I think the chances of that happening are slim to none, so we are better off starting from a clean slate.

[sebastienbarre]

@spawnia @vladar Sorry I missed that discussion. I would love for guidance on this one, I'm only finding about it now as I'm trying to provide feedback to #914, and having to fix our code in the process after the changes in #906 and this one #830 :)

I found getCategory() pretty useful, but if there is another way, I'd be willing to give it a shot, though I'm not sure how to do so with the new ProvidesExtensions, I'm not trying to provide my own categories, but I'm relying on your categorization instead.

We have an error handler attached to ExecutionResult right now and that handler tries to cut down the noise for our users, by filtering out all errors of category CATEGORY_GRAPHQL, which according to the doc were "The "graphql" category is reserved for errors produced by query parsing or validation".

Our handler keeps the other errors, and replaces all CATEGORY_GRAPHQL errors with one single error suggesting our user to reload their client. Essentially the server GraphQL API changed, but the user has not updated their web client yet (never reloaded), and it is making requests breaking validation left and right. Instead of drowning them with errors, we only send one.

How can this be achieved now please?

   // in our executor subclass...
    if (!Configure::read('debug')) {
      $result = $result->setErrorsHandler(new CategoryInterceptorErrorHandler());
    }

or handler:

/**
 * Custom GraphQL error handler that will intercept errors of the 'graphql' category
 * and replace them with one single error suggesting to reload the client.
 *
 * The "graphql" category is reserved for errors produced by query parsing or validation.
 */
class CategoryInterceptorErrorHandler
{
  /**
   * The __invoke() method is called when a script tries to call an object as a function.
   *
   * @param array $errors Errors.
   * @param callable $formatter Formatter.
   * @return array of formatted errors.
   */
  public function __invoke(array $errors, callable $formatter)
  {
    $count = count($errors);
    $errors = array_filter($errors, static fn ($e) => $e->getCategory() != Error::CATEGORY_GRAPHQL);
    if ($count != count($errors)) {
      // phpcs:ignore Generic.Files.LineLength
      $msg = 'An error occurred while talking to the server. Reloading your client may address the issue. Please contact us if it persists.';
      $errors[] = new Error($msg);
    }
    return array_map($formatter, $errors);
  }
}

Thanks

[spawnia]

You can examine errors by checking if they:

• implement GraphQL\Error\ClientAware and isClientSafe() returns true
• are instanceof GraphQL\Error\Error

Implementing ProvidesExtensions gives you an even more clean way to communicate something to clients in a structured manner, perhaps a field such as possibly_resolved_by_reload: true | false?

[sebastienbarre]

Thanks @spawnia, this seems to be doing the trick :)