From 9ee27bda062a4d5dcc15496aa3b39a678ae855c1 Mon Sep 17 00:00:00 2001 From: Christoffer Niska Date: Wed, 24 Oct 2018 11:49:00 +0300 Subject: [PATCH 1/2] Add support for error handler middleware --- src/Error/ErrorHandler.php | 30 ------ src/Error/ErrorHandlerInterface.php | 13 --- src/Error/Handler/AddErrorMiddleware.php | 19 ++++ src/Error/Handler/ErrorHandler.php | 52 +++++++++ src/Error/Handler/ErrorHandlerInterface.php | 15 +++ .../Handler/ErrorMiddlewareInterface.php | 16 +++ src/Execution/Execution.php | 2 +- src/Execution/ExecutionInterface.php | 2 +- src/GraphQL.php | 25 ++--- src/api.php | 2 +- tests/Unit/Error/ErrorHandlerTest.php | 25 ----- tests/Unit/Error/Handler/ErrorHandlerTest.php | 101 ++++++++++++++++++ 12 files changed, 215 insertions(+), 87 deletions(-) delete mode 100644 src/Error/ErrorHandler.php delete mode 100644 src/Error/ErrorHandlerInterface.php create mode 100644 src/Error/Handler/AddErrorMiddleware.php create mode 100644 src/Error/Handler/ErrorHandler.php create mode 100644 src/Error/Handler/ErrorHandlerInterface.php create mode 100644 src/Error/Handler/ErrorMiddlewareInterface.php delete mode 100644 tests/Unit/Error/ErrorHandlerTest.php create mode 100644 tests/Unit/Error/Handler/ErrorHandlerTest.php diff --git a/src/Error/ErrorHandler.php b/src/Error/ErrorHandler.php deleted file mode 100644 index 5d8125be..00000000 --- a/src/Error/ErrorHandler.php +++ /dev/null @@ -1,30 +0,0 @@ -handleCallback = $handleCallback; - } - - /** - * @param ExecutionException $exception - */ - public function handleError(ExecutionException $exception) - { - \call_user_func($this->handleCallback, $exception); - } -} diff --git a/src/Error/ErrorHandlerInterface.php b/src/Error/ErrorHandlerInterface.php deleted file mode 100644 index 20f50fe9..00000000 --- a/src/Error/ErrorHandlerInterface.php +++ /dev/null @@ -1,13 +0,0 @@ -addError($exception); + + return $next($exception, $context); + } +} diff --git a/src/Error/Handler/ErrorHandler.php b/src/Error/Handler/ErrorHandler.php new file mode 100644 index 00000000..23022283 --- /dev/null +++ b/src/Error/Handler/ErrorHandler.php @@ -0,0 +1,52 @@ +addMiddleware($mw); + } + } + + /** + * @inheritdoc + */ + public function handle(ExecutionException $exception, ExecutionContext $context) + { + $next = function () { + // NO-OP + }; + + foreach (\array_reverse($this->middleware) as $mw) { + /** @var ErrorMiddlewareInterface $mw */ + $next = function (ExecutionException $exception, ExecutionContext $context) use ($mw, $next) { + return $mw->handle($exception, $context, $next); + }; + } + + \call_user_func($next, $exception, $context); + } + + /** + * @param ErrorMiddlewareInterface $middleware + */ + protected function addMiddleware(ErrorMiddlewareInterface $middleware) + { + $this->middleware[] = $middleware; + } +} diff --git a/src/Error/Handler/ErrorHandlerInterface.php b/src/Error/Handler/ErrorHandlerInterface.php new file mode 100644 index 00000000..f962fe26 --- /dev/null +++ b/src/Error/Handler/ErrorHandlerInterface.php @@ -0,0 +1,15 @@ +execute( $schema, $document, diff --git a/src/api.php b/src/api.php index f37ea3b2..b2e9e66c 100644 --- a/src/api.php +++ b/src/api.php @@ -2,7 +2,7 @@ namespace Digia\GraphQL; -use Digia\GraphQL\Error\ErrorHandlerInterface; +use Digia\GraphQL\Error\Handler\ErrorHandlerInterface; use Digia\GraphQL\Error\InvariantException; use Digia\GraphQL\Execution\ExecutionResult; use Digia\GraphQL\Language\Node\DocumentNode; diff --git a/tests/Unit/Error/ErrorHandlerTest.php b/tests/Unit/Error/ErrorHandlerTest.php deleted file mode 100644 index d0408a6a..00000000 --- a/tests/Unit/Error/ErrorHandlerTest.php +++ /dev/null @@ -1,25 +0,0 @@ -handleError(new ExecutionException('This is an exception.')); - - $this->assertTrue($wasCallbackInvoked); - } -} diff --git a/tests/Unit/Error/Handler/ErrorHandlerTest.php b/tests/Unit/Error/Handler/ErrorHandlerTest.php new file mode 100644 index 00000000..a762244b --- /dev/null +++ b/tests/Unit/Error/Handler/ErrorHandlerTest.php @@ -0,0 +1,101 @@ +mockContext(); + + $errorHandler->handle($exception, $context); + + $this->assertTrue($middleware->wasInvoked()); + } + + public function testMiddleware() + { + $result = []; + + $middlewareA = new LoggerMiddleware(function () use (&$result) { + $result[] = 'Middleware A invoked'; + }); + $middlewareB = new LoggerMiddleware(function () use (&$result) { + $result[] = 'Middleware B invoked'; + }); + $middlewareC = new LoggerMiddleware(function () use (&$result) { + $result[] = 'Middleware C invoked'; + }); + $noopMiddleware = new NoopMiddleware(); + $errorHandler = new ErrorHandler([$middlewareA, $middlewareB, $noopMiddleware, $middlewareC]); + $exception = new ExecutionException('This is an exception.'); + $context = $this->mockContext(); + + $errorHandler->handle($exception, $context); + + $this->assertSame([ + 'Middleware A invoked', + 'Middleware B invoked', + ], $result); + } + + /** + * @return ExecutionContext|\PHPUnit_Framework_MockObject_MockObject + */ + private function mockContext() + { + return $this->getMockBuilder(ExecutionContext::class) + ->disableOriginalConstructor() + ->getMock(); + } +} + +class WasInvokedMiddleware implements ErrorMiddlewareInterface +{ + private $wasInvoked = false; + + public function handle(ExecutionException $exception, ExecutionContext $context, callable $next) + { + $this->wasInvoked = true; + + return $next($exception, $context); + } + + public function wasInvoked(): bool + { + return $this->wasInvoked; + } +} + +class LoggerMiddleware implements ErrorMiddlewareInterface +{ + private $logCallback; + + public function __construct($logCallback) + { + $this->logCallback = $logCallback; + } + + public function handle(ExecutionException $exception, ExecutionContext $context, callable $next) + { + \call_user_func($this->logCallback, $exception, $context); + + return $next($exception, $context); + } +} + +class NoopMiddleware implements ErrorMiddlewareInterface +{ + public function handle(ExecutionException $exception, ExecutionContext $context, callable $next) + { + } +} From e8123a40313d936e4ff0468e730cbcc8ffa9de51 Mon Sep 17 00:00:00 2001 From: Christoffer Niska Date: Fri, 9 Nov 2018 15:01:35 +0200 Subject: [PATCH 2/2] Refactor error handling --- src/Error/Handler/AbstractErrorMiddleware.php | 27 +++++++ src/Error/Handler/AddErrorMiddleware.php | 19 ----- src/Error/Handler/CallableMiddleware.php | 31 ++++++++ src/Error/Handler/ErrorHandler.php | 31 ++++++-- src/Error/Handler/ErrorHandlerInterface.php | 11 ++- .../Handler/ErrorMiddlewareInterface.php | 9 ++- src/Execution/Execution.php | 8 +- src/GraphQL.php | 73 ++++++++++++++++++- src/api.php | 33 +++------ tests/Unit/Error/Handler/ErrorHandlerTest.php | 19 ++--- 10 files changed, 193 insertions(+), 68 deletions(-) create mode 100644 src/Error/Handler/AbstractErrorMiddleware.php delete mode 100644 src/Error/Handler/AddErrorMiddleware.php create mode 100644 src/Error/Handler/CallableMiddleware.php diff --git a/src/Error/Handler/AbstractErrorMiddleware.php b/src/Error/Handler/AbstractErrorMiddleware.php new file mode 100644 index 00000000..a89b95d3 --- /dev/null +++ b/src/Error/Handler/AbstractErrorMiddleware.php @@ -0,0 +1,27 @@ +addError($exception); - - return $next($exception, $context); - } -} diff --git a/src/Error/Handler/CallableMiddleware.php b/src/Error/Handler/CallableMiddleware.php new file mode 100644 index 00000000..95b79c86 --- /dev/null +++ b/src/Error/Handler/CallableMiddleware.php @@ -0,0 +1,31 @@ +handleCallback = $handleCallback; + } + + /** + * @inheritdoc + */ + public function handleExecutionError(ExecutionException $exception, ExecutionContext $context, callable $next) + { + return \call_user_func($this->handleCallback, $exception, $context, $next); + } +} diff --git a/src/Error/Handler/ErrorHandler.php b/src/Error/Handler/ErrorHandler.php index 23022283..36439b3e 100644 --- a/src/Error/Handler/ErrorHandler.php +++ b/src/Error/Handler/ErrorHandler.php @@ -10,7 +10,7 @@ class ErrorHandler implements ErrorHandlerInterface /** * @var ErrorMiddlewareInterface[] */ - protected $middleware; + protected $middleware = []; /** * ErrorHandler constructor. @@ -23,19 +23,36 @@ public function __construct(array $middleware) } } + /** + * @param \Throwable $exception + */ + public function handleError(\Throwable $exception) + { + $next = function () { + // NO-OP + }; + + foreach ($this->middleware as $middleware) { + $next = function (\Throwable $exception) use ($middleware, $next) { + return $middleware->handleError($exception, $next); + }; + } + + \call_user_func($next, $exception); + } + /** * @inheritdoc */ - public function handle(ExecutionException $exception, ExecutionContext $context) + public function handleExecutionError(ExecutionException $exception, ExecutionContext $context) { $next = function () { // NO-OP }; - foreach (\array_reverse($this->middleware) as $mw) { - /** @var ErrorMiddlewareInterface $mw */ - $next = function (ExecutionException $exception, ExecutionContext $context) use ($mw, $next) { - return $mw->handle($exception, $context, $next); + foreach ($this->middleware as $middleware) { + $next = function (ExecutionException $exception, ExecutionContext $context) use ($middleware, $next) { + return $middleware->handleExecutionError($exception, $context, $next); }; } @@ -47,6 +64,6 @@ public function handle(ExecutionException $exception, ExecutionContext $context) */ protected function addMiddleware(ErrorMiddlewareInterface $middleware) { - $this->middleware[] = $middleware; + \array_unshift($this->middleware, $middleware); } } diff --git a/src/Error/Handler/ErrorHandlerInterface.php b/src/Error/Handler/ErrorHandlerInterface.php index f962fe26..fae6e277 100644 --- a/src/Error/Handler/ErrorHandlerInterface.php +++ b/src/Error/Handler/ErrorHandlerInterface.php @@ -8,8 +8,13 @@ interface ErrorHandlerInterface { /** - * @param ExecutionException $exception - * @param ExecutionContext $context + * @param \Throwable $exception */ - public function handle(ExecutionException $exception, ExecutionContext $context); + public function handleError(\Throwable $exception); + + /** + * @param ExecutionException $exception + * @param ExecutionContext $context + */ + public function handleExecutionError(ExecutionException $exception, ExecutionContext $context); } diff --git a/src/Error/Handler/ErrorMiddlewareInterface.php b/src/Error/Handler/ErrorMiddlewareInterface.php index 1730e322..8d9e3017 100644 --- a/src/Error/Handler/ErrorMiddlewareInterface.php +++ b/src/Error/Handler/ErrorMiddlewareInterface.php @@ -7,10 +7,17 @@ interface ErrorMiddlewareInterface { + /** + * @param \Throwable $exception + * @param callable $next + * @return mixed + */ + public function handleError(\Throwable $exception, callable $next); + /** * @param ExecutionException $exception * @param ExecutionContext $context * @return mixed */ - public function handle(ExecutionException $exception, ExecutionContext $context, callable $next); + public function handleExecutionError(ExecutionException $exception, ExecutionContext $context, callable $next); } diff --git a/src/Execution/Execution.php b/src/Execution/Execution.php index 98d5b2b9..086e7e29 100644 --- a/src/Execution/Execution.php +++ b/src/Execution/Execution.php @@ -58,9 +58,9 @@ public function execute( }); } - if (null !== $errorHandler) { + if ($errorHandler instanceof ErrorHandlerInterface) { foreach ($context->getErrors() as $error) { - $errorHandler->handleError($error); + $errorHandler->handleExecutionError($error, $context); } } @@ -72,15 +72,13 @@ public function execute( * @param ExecutionContext $context * @param FieldCollector $fieldCollector * @param ValuesResolver $valuesResolver - * @param ErrorHandlerInterface|null $errorHandler * @return array|mixed|null|PromiseInterface */ protected function executeOperation( ?string $operationName, ExecutionContext $context, FieldCollector $fieldCollector, - ValuesResolver $valuesResolver, - ?ErrorHandlerInterface $errorHandler = null + ValuesResolver $valuesResolver ) { $strategy = $operationName === 'mutation' ? new SerialExecutionStrategy($context, $fieldCollector, $valuesResolver) diff --git a/src/GraphQL.php b/src/GraphQL.php index 8b4d3527..c02242e5 100644 --- a/src/GraphQL.php +++ b/src/GraphQL.php @@ -3,6 +3,7 @@ namespace Digia\GraphQL; use Digia\GraphQL\Error\Handler\ErrorHandlerInterface; +use Digia\GraphQL\Error\InvariantException; use Digia\GraphQL\Execution\ExecutionInterface; use Digia\GraphQL\Execution\ExecutionProvider; use Digia\GraphQL\Execution\ExecutionResult; @@ -14,6 +15,7 @@ use Digia\GraphQL\Language\NodePrinterInterface; use Digia\GraphQL\Language\ParserInterface; use Digia\GraphQL\Language\Source; +use Digia\GraphQL\Language\SyntaxErrorException; use Digia\GraphQL\Schema\Building\SchemaBuilderInterface; use Digia\GraphQL\Schema\Building\SchemaBuildingProvider; use Digia\GraphQL\Schema\Extension\SchemaExtenderInterface; @@ -248,7 +250,7 @@ public static function execute( array $variableValues = [], $operationName = null, callable $fieldResolver = null, - $errorHandler = null + ?ErrorHandlerInterface $errorHandler = null ): ExecutionResult { /** @var ExecutionInterface $execution */ $execution = static::make(ExecutionInterface::class); @@ -265,6 +267,75 @@ public static function execute( ); } + /** + * @param Schema $schema + * @param string $source + * @param mixed $rootValue + * @param mixed $contextValue + * @param array $variableValues + * @param null|string $operationName + * @param callable|null $fieldResolver + * @param ErrorHandlerInterface|null $errorHandler + * @return ExecutionResult + * @throws InvariantException + * @throws SyntaxErrorException + */ + public static function process( + Schema $schema, + string $source, + $rootValue = null, + $contextValue = null, + array $variableValues = [], + ?string $operationName = null, + ?callable $fieldResolver = null, + ?ErrorHandlerInterface $errorHandler = null + ): ExecutionResult { + $schemaValidationErrors = validateSchema($schema); + + if (!empty($schemaValidationErrors)) { + if (null !== $errorHandler) { + foreach ($schemaValidationErrors as $schemaValidationError) { + $errorHandler->handleError($schemaValidationError); + } + } + + return new ExecutionResult(null, $schemaValidationErrors); + } + + try { + $document = parse($source); + } catch (SyntaxErrorException $error) { + if (null !== $errorHandler) { + $errorHandler->handleError($error); + } + + return new ExecutionResult(null, [$error]); + } + + $validationErrors = validate($schema, $document); + + if (!empty($validationErrors)) { + if (null !== $errorHandler) { + foreach ($validationErrors as $validationError) { + $errorHandler->handleError($validationError); + } + } + + return new ExecutionResult(null, $validationErrors); + } + + return execute( + $schema, + $document, + $rootValue, + $contextValue, + $variableValues, + $operationName, + $fieldResolver, + $errorHandler + ); + } + /** * @param NodeInterface $node * @return string diff --git a/src/api.php b/src/api.php index b2e9e66c..11ede9a0 100644 --- a/src/api.php +++ b/src/api.php @@ -2,6 +2,8 @@ namespace Digia\GraphQL; +use Digia\GraphQL\Error\Handler\CallableMiddleware; +use Digia\GraphQL\Error\Handler\ErrorHandler; use Digia\GraphQL\Error\Handler\ErrorHandlerInterface; use Digia\GraphQL\Error\InvariantException; use Digia\GraphQL\Execution\ExecutionResult; @@ -162,14 +164,15 @@ function printNode(NodeInterface $node): string /** * @param Schema $schema * @param string $source - * @param mixed|null $rootValue - * @param mixed|null $contextValue + * @param mixed $rootValue + * @param mixed $contextValue * @param array $variableValues - * @param mixed|null $operationName + * @param string|null $operationName * @param callable|null $fieldResolver * @param ErrorHandlerInterface|callable|null $errorHandler * @return array * @throws InvariantException + * @throws SyntaxErrorException */ function graphql( Schema $schema, @@ -177,29 +180,17 @@ function graphql( $rootValue = null, $contextValue = null, array $variableValues = [], - $operationName = null, - callable $fieldResolver = null, + ?string $operationName = null, + ?callable $fieldResolver = null, $errorHandler = null ): array { - $schemaValidationErrors = validateSchema($schema); - if (!empty($schemaValidationErrors)) { - return (new ExecutionResult([], $schemaValidationErrors))->toArray(); + if (\is_callable($errorHandler)) { + $errorHandler = new ErrorHandler([new CallableMiddleware($errorHandler)]); } - try { - $document = parse($source); - } catch (SyntaxErrorException $error) { - return (new ExecutionResult([], [$error]))->toArray(); - } - - $validationErrors = validate($schema, $document); - if (!empty($validationErrors)) { - return (new ExecutionResult([], $validationErrors))->toArray(); - } - - $result = execute( + $result = GraphQL::process( $schema, - $document, + $source, $rootValue, $contextValue, $variableValues, diff --git a/tests/Unit/Error/Handler/ErrorHandlerTest.php b/tests/Unit/Error/Handler/ErrorHandlerTest.php index a762244b..d44cb4db 100644 --- a/tests/Unit/Error/Handler/ErrorHandlerTest.php +++ b/tests/Unit/Error/Handler/ErrorHandlerTest.php @@ -2,8 +2,8 @@ namespace Digia\GraphQL\Test\Unit\Error\Handler; +use Digia\GraphQL\Error\Handler\AbstractErrorMiddleware; use Digia\GraphQL\Error\Handler\ErrorHandler; -use Digia\GraphQL\Error\Handler\ErrorMiddlewareInterface; use Digia\GraphQL\Execution\ExecutionContext; use Digia\GraphQL\Execution\ExecutionException; use Digia\GraphQL\Test\TestCase; @@ -17,7 +17,7 @@ public function testHandle() $exception = new ExecutionException('This is an exception.'); $context = $this->mockContext(); - $errorHandler->handle($exception, $context); + $errorHandler->handleExecutionError($exception, $context); $this->assertTrue($middleware->wasInvoked()); } @@ -40,7 +40,7 @@ public function testMiddleware() $exception = new ExecutionException('This is an exception.'); $context = $this->mockContext(); - $errorHandler->handle($exception, $context); + $errorHandler->handleExecutionError($exception, $context); $this->assertSame([ 'Middleware A invoked', @@ -59,11 +59,11 @@ private function mockContext() } } -class WasInvokedMiddleware implements ErrorMiddlewareInterface +class WasInvokedMiddleware extends AbstractErrorMiddleware { private $wasInvoked = false; - public function handle(ExecutionException $exception, ExecutionContext $context, callable $next) + public function handleExecutionError(ExecutionException $exception, ExecutionContext $context, callable $next) { $this->wasInvoked = true; @@ -76,7 +76,7 @@ public function wasInvoked(): bool } } -class LoggerMiddleware implements ErrorMiddlewareInterface +class LoggerMiddleware extends AbstractErrorMiddleware { private $logCallback; @@ -85,7 +85,7 @@ public function __construct($logCallback) $this->logCallback = $logCallback; } - public function handle(ExecutionException $exception, ExecutionContext $context, callable $next) + public function handleExecutionError(ExecutionException $exception, ExecutionContext $context, callable $next) { \call_user_func($this->logCallback, $exception, $context); @@ -93,9 +93,6 @@ public function handle(ExecutionException $exception, ExecutionContext $context, } } -class NoopMiddleware implements ErrorMiddlewareInterface +class NoopMiddleware extends AbstractErrorMiddleware { - public function handle(ExecutionException $exception, ExecutionContext $context, callable $next) - { - } }