Replace error handling with error handler middleware

[fritz-gerneth]

In the current implementation there little to no control about error handling and logging. This PR adds a fully configurable error handling middleware to the processor. Opening this PR now already for open discussion about this approach etc.. The changes are BC, with the middleware configured by default simply replicates the previous behavior (to add all errors to the ExecutionContext's error array).

The new ErrorHandler middleware can receive a stack of error handlers to call in sequence. The invocation sequence is simply defined by the order passed array. Developers can pass their own error handler (thus error handling logic) to the processor.

Potential uses:

• I have added a sample Logger middleware, logging all errors to an PSR compliant logger, and than continuing the chain :

  errorHandler = new ErrorHandler([
  new Logger($myLogger),
  new AddToErrorList(),
  );
  

• Add error filters (not implemented, too application specific). Middlewares can opt to not invoke the next middleware in the chain, effectively breaking execution at that point. This can come in handy for filtering errors:

  errorHandler = new ErrorHandler([
  new MyFilter(),
  new Logger($myLogger),
  new AddToErrorList(),
  );
  
  lass MyFilter implements ErrorHandlerMiddlewareInterface
  
  public function execute($error, ExecutionContext $context, callable $next)
  {
  	if ($error instanceof \RuntimeException) {
  		return;
  	}
  
  	$next($error, $context);
  }
  
  

  In this case errors first get filtered, than logged (if not filtered out), and added to the error list. We simply can switch the chain order to alter this behavior: Log all exceptions, than filter, and only add filtered errors to the error list:

  errorHandler = new ErrorHandler([
  new Logger($myLogger),
  new MyFilter(),
  new AddToErrorList(),
  );
  

  We sure could add a second filter instance with different settings in here too (e.g. filter all errors first, then log, filter further, add to array).
• Common interface and configurability allows developers to add their own logic.

This change tries to achieve lowest changes and keeping BC. Changing the entire processor to be a middleware greatly would increase extensability, but this would be a major BC break :)

Any thoughts / suggestions on this approach?

[fritz-gerneth]

Also, just added a new chain element Filter that can receive a filter callback to continue the middleware chain or not:

	$myFilter = new Filter(function ($error, $context) {
		if ($error instanceof DatableException) {
			return true;
		}
		if ($error instanceof LocationableExceptionInterface) {
			return true;
		}
		return false;
	});
	
	$errorHandler = new ErrorHandler([
                $myFilter,
		new Logger($myLogger),
		new AddToErrorList(),
	]);

This only would add errors implementing either interface to the error list (and thus to the graphql response). Other errors are not added. As per middleware philosophy, this plugin is chainable in any combination.

This would first log all errors, than filter them out respectively:

	$errorHandler = new ErrorHandler([
		new Logger($myLogger),
                $myFilter,
		new AddToErrorList(),
	]);

[fritz-gerneth]

As a further point of improvement, we could add the current AstField as an optional 3rd argument to the ResolveException. This would allow error handlers to have access to the field causing the exception for further information. In addition if the AstField were to have a reference to its parent, this would allow error handlers to create a hierarchical array for error messages.

[viniychuk]

Thanks @fritz-gerneth, looks good, should be merged this weekend!

[fritz-gerneth]

@viniychuk any update on when/if this is merged? Don't want to put this library into action without proper control about which exceptions are published :)

[viniychuk]

@fritz-gerneth .. in progress right now (finally). Expect it 05.07 but a little bit refactored

[viniychuk]

@fritz-gerneth up. working on it in 1.5, adding strong type hinting and changing processor signature like it will be in 2.0. should finish tomorrow. Sorry for the delay.

[viniychuk]

@fritz-gerneth got an update.. please take a look at https://github.com/Youshido/GraphQL/tree/1.5, let me know if it works for you. Basically, I just simplified what you did by trying to make it more simple and because technically ExecutionContext can exist without a custom error handler it might not be needed in the constructor — so it's being added by using addErrorHandler. Of course you can have your own execution context that have exactly your functionality... One of the major thing (that is a long due) — Processor is starting to accept ExecutionContextInterface instead of Schema which makes the whole architecture better and allow us to make changes for 2.0.. Anyway, let mw know what you think and if needed — I'm back on gitter.

[fritz-gerneth]

With the changes you have applied, having some error handler indeed is no longer required. To be honest I find the new approach more difficult to use and implement over a well-established middleware approach:

• Splits up action (handle) & decission to continue (isFinal) into two methods.
  • Any properly isolated error handler only needs to implement either method, almost making these two methods mutualy exclusive.
  • Those two methods are closely related and are called right after each other. Having them split up into two methods splits up any logic into two methods, though it should be in one
• I find it a bit inconsistent to have an error handler stack to consult for, but then use a completly different and out-of-band mechanism to add the error . Imho this should be an error handler on its own, added by default (unless specified by providing a custom stack).

I certainly can get the results I need with the current approach; just find it a bit more difficult to use than regular middleware. But ultimatively, that's your call to make. Would have appreciated some feedback on this so I could have updated the PR respectively.

(As a side-note I'm no fan of changing the Processor signature, this is a BC break and should not be in a minor version)

[fritz-gerneth]

It has been a while and neither the suggested changes here nor the ones on the (now-deleted) 1.5 branch have made it into any release. Any chance either approach will get released any time soon? Imho it is a huge show-stopper to not have any sensible way of handling errors and find it rather uncomprehensible that neither approach is implemented after 9 months. For now we'll have to use a fork but would be great to have this merged any time soon.

[viniychuk]

@fritz-gerneth Sorry, it was indeed a long time. Will try to release the v2 asap.

[skyzyx]

@fritz-gerneth: Is this something you have some documentation for leveraging? We're running into the same issue, and like you, we've had to fork the project in order to merge some of the PRs posted here.

Would love to get clarity on how a consuming engineer could leverage this. Thanks!

[fritz-gerneth]

@skyzyx If you are using my original implementation most of what I have written in the initial PR is still valid. To provide some further idea, this is how we have set up our error handling ($container is our PSR compatible container):

General processor:

$processor = new Processor(
            $container->get(Schema::class),
            new ErrorHandler([ 
                 // Error handlers are executed top to bottom
                new LogErrorErrorHandler(),
                new SanitizedExceptionsErrorHandler(),
            ])
        );

First all exceptions are logged

class LogErrorErrorHandler implements ErrorHandlerPluginInterface
{
    public function execute($error, ExecutionContext $executionContext, callable $next)
    {
        error_log($error);

        return $next($error, $executionContext);
    }
}

Then we decide if the error message should be returned or only a sanitized error message is returned:

class SanitizedExceptionsErrorHandler implements ErrorHandlerPluginInterface
{
    public function execute($error, ExecutionContext $executionContext, callable $next)
    {
        $executionContext->addError($this->sanitizeException($error));

        return $next($error, $executionContext);
    }

    public function sanitizeException(\Exception $exception)
    {
        if ($exception instanceof DomainException) {
            return $exception;
        } else if ($exception instanceof ResolveException) {
            return $exception;
        } else {
            return new RuntimeException('Internal server error', 500);
        }
    }
}

Feel free to add your own logic & re-order as you see it fit for your use-case.