From b8ccb7e4fb3d85c450d7ad9c0cbad40f27ad99f3 Mon Sep 17 00:00:00 2001 From: Jeremiah VALERIE Date: Wed, 20 Dec 2017 10:42:05 +0100 Subject: [PATCH 1/9] Improve executor using Events --- Command/GraphQLDumpSchemaCommand.php | 2 +- Controller/GraphController.php | 9 +- .../OverblogGraphQLExtension.php | 16 +- Error/ErrorHandler.php | 2 +- Event/Events.php | 2 + Event/ExecutorContextEvent.php | 18 +- Event/ExecutorEvent.php | 85 +++++++ Event/ExecutorResultEvent.php | 25 ++ EventListener/DebugListener.php | 28 +++ EventListener/ErrorHandlerListener.php | 32 +++ EventListener/RequestFilesListener.php | 1 - Request/Executor.php | 214 +++++++++--------- Resources/config/services.yml | 14 +- Tests/Functional/App/config/config.yml | 3 + Tests/Functional/App/config/debug/config.yml | 7 + .../EventListener/DebugListenerTest.php | 26 +++ Tests/Functional/TestCase.php | 18 +- Tests/Request/ExecutorTest.php | 31 +-- composer.json | 1 + 19 files changed, 375 insertions(+), 159 deletions(-) create mode 100644 Event/ExecutorEvent.php create mode 100644 Event/ExecutorResultEvent.php create mode 100644 EventListener/DebugListener.php create mode 100644 EventListener/ErrorHandlerListener.php create mode 100644 Tests/Functional/App/config/debug/config.yml create mode 100644 Tests/Functional/EventListener/DebugListenerTest.php diff --git a/Command/GraphQLDumpSchemaCommand.php b/Command/GraphQLDumpSchemaCommand.php index 9bb40d7fe..a40c37f49 100644 --- a/Command/GraphQLDumpSchemaCommand.php +++ b/Command/GraphQLDumpSchemaCommand.php @@ -94,7 +94,7 @@ private function createFile(InputInterface $input) $modern = $this->useModernJsonFormat($input); $result = $this->getRequestExecutor() - ->execute($request, [], $schemaName) + ->execute($schemaName, $request) ->toArray(); $content = json_encode($modern ? $result : $result['data'], \JSON_PRETTY_PRINT); diff --git a/Controller/GraphController.php b/Controller/GraphController.php index 5022e2bdd..6dacdd5fa 100644 --- a/Controller/GraphController.php +++ b/Controller/GraphController.php @@ -95,12 +95,7 @@ private function processBatchQuery(Request $request, $schemaName = null) foreach ($queries as $query) { $payloadResult = $this->requestExecutor->execute( - [ - 'query' => $query['query'], - 'variables' => $query['variables'], - ], - [], - $schemaName + $schemaName, ['query' => $query['query'], 'variables' => $query['variables']] ); $payloads[] = $apolloBatching ? $payloadResult->toArray() : ['id' => $query['id'], 'payload' => $payloadResult->toArray()]; } @@ -112,6 +107,6 @@ private function processNormalQuery(Request $request, $schemaName = null) { $params = $this->requestParser->parse($request); - return $this->requestExecutor->execute($params, [], $schemaName)->toArray(); + return $this->requestExecutor->execute($schemaName, $params)->toArray(); } } diff --git a/DependencyInjection/OverblogGraphQLExtension.php b/DependencyInjection/OverblogGraphQLExtension.php index 5704573ba..bda34d9b6 100644 --- a/DependencyInjection/OverblogGraphQLExtension.php +++ b/DependencyInjection/OverblogGraphQLExtension.php @@ -5,7 +5,9 @@ use GraphQL\Type\Schema; use Overblog\GraphQLBundle\CacheWarmer\CompileCacheWarmer; use Overblog\GraphQLBundle\Config\Processor\BuilderProcessor; +use Overblog\GraphQLBundle\Event\Events; use Overblog\GraphQLBundle\EventListener\ClassLoaderListener; +use Overblog\GraphQLBundle\EventListener\DebugListener; use Symfony\Component\Cache\Adapter\ArrayAdapter; use Symfony\Component\Config\FileLocator; use Symfony\Component\DependencyInjection\ContainerBuilder; @@ -35,7 +37,7 @@ public function load(array $configs, ContainerBuilder $container) $this->setErrorHandlerArguments($config, $container); $this->setSecurity($config, $container); $this->setConfigBuilders($config, $container); - $this->setShowDebug($config, $container); + $this->setDebugListener($config, $container); $this->setDefinitionParameters($config, $container); $this->setClassLoaderListener($config, $container); $this->setCompilerCacheWarmer($config, $container); @@ -87,6 +89,7 @@ private function setClassLoaderListener(array $config, ContainerBuilder $contain $this->getAlias().'.event_listener.classloader_listener', new Definition(ClassLoaderListener::class) ); + $definition->setPublic(true); $definition->setArguments([new Reference($this->getAlias().'.cache_compiler')]); $definition->addTag('kernel.event_listener', ['event' => 'kernel.request', 'method' => 'load', 'priority' => 255]); $definition->addTag('kernel.event_listener', ['event' => 'console.command', 'method' => 'load', 'priority' => 255]); @@ -117,9 +120,16 @@ private function setExpressionLanguageDefaultParser(ContainerBuilder $container) $container->setDefinition($this->getAlias().'.cache_expression_language_parser.default', $definition); } - private function setShowDebug(array $config, ContainerBuilder $container) + private function setDebugListener(array $config, ContainerBuilder $container) { - $container->getDefinition($this->getAlias().'.request_executor')->replaceArgument(4, $config['definitions']['show_debug_info']); + if ($config['definitions']['show_debug_info']) { + $definition = $container->setDefinition( + DebugListener::class, + new Definition(DebugListener::class) + ); + $definition->addTag('kernel.event_listener', ['event' => Events::PRE_EXECUTOR, 'method' => 'onPreExecutor']); + $definition->addTag('kernel.event_listener', ['event' => Events::POST_EXECUTOR, 'method' => 'onPostExecutor']); + } } private function setConfigBuilders(array $config, ContainerBuilder $container) diff --git a/Error/ErrorHandler.php b/Error/ErrorHandler.php index 6cc21ac30..a4f2144f2 100644 --- a/Error/ErrorHandler.php +++ b/Error/ErrorHandler.php @@ -156,7 +156,7 @@ public function logException($exception, $errorLevel = LogLevel::ERROR) public function handleErrors(ExecutionResult $executionResult, $throwRawException = false) { - $executionResult->setErrorFormatter(sprintf('\%s::formatError', GraphQLError::class)); + $executionResult->setErrorFormatter([GraphQLError::class, 'formatError']); $exceptions = $this->treatExceptions($executionResult->errors, $throwRawException); $executionResult->errors = $exceptions['errors']; if (!empty($exceptions['extensions']['warnings'])) { diff --git a/Event/Events.php b/Event/Events.php index 02f137226..102ff3fde 100644 --- a/Event/Events.php +++ b/Event/Events.php @@ -5,4 +5,6 @@ final class Events { const EXECUTOR_CONTEXT = 'graphql.executor.context'; + const PRE_EXECUTOR = 'graphql.pre_executor'; + const POST_EXECUTOR = 'graphql.post_executor'; } diff --git a/Event/ExecutorContextEvent.php b/Event/ExecutorContextEvent.php index eb800f720..53e871a6d 100644 --- a/Event/ExecutorContextEvent.php +++ b/Event/ExecutorContextEvent.php @@ -6,26 +6,22 @@ class ExecutorContextEvent extends Event { - private $executorContext = []; + /** @var \ArrayObject */ + private $executorContext; - public function __construct(array $executorContext) + /** + * @param \ArrayObject $executorContext + */ + public function __construct(\ArrayObject $executorContext) { $this->executorContext = $executorContext; } /** - * @return array + * @return \ArrayObject */ public function getExecutorContext() { return $this->executorContext; } - - /** - * @param array $executionContext - */ - public function setExecutorContext(array $executionContext) - { - $this->executorContext = $executionContext; - } } diff --git a/Event/ExecutorEvent.php b/Event/ExecutorEvent.php new file mode 100644 index 000000000..30165c28c --- /dev/null +++ b/Event/ExecutorEvent.php @@ -0,0 +1,85 @@ +schema = $schema; + $this->requestString = $requestString; + $this->rootValue = $rootValue; + $this->contextValue = $contextValue; + $this->variableValue = $variableValue; + $this->operationName = $operationName; + } + + /** + * @return Schema + */ + public function getSchema() + { + return $this->schema; + } + + /** + * @return string + */ + public function getRequestString() + { + return $this->requestString; + } + + /** + * @return \ArrayObject + */ + public function getRootValue() + { + return $this->rootValue; + } + + /** + * @return \ArrayObject + */ + public function getContextValue() + { + return $this->contextValue; + } + + /** + * @return array|null + */ + public function getVariableValue() + { + return $this->variableValue; + } + + /** + * @return null|string + */ + public function getOperationName() + { + return $this->operationName; + } +} diff --git a/Event/ExecutorResultEvent.php b/Event/ExecutorResultEvent.php new file mode 100644 index 000000000..1e27d5400 --- /dev/null +++ b/Event/ExecutorResultEvent.php @@ -0,0 +1,25 @@ +result = $result; + } + + /** + * @return ExecutionResult + */ + public function getResult() + { + return $this->result; + } +} diff --git a/EventListener/DebugListener.php b/EventListener/DebugListener.php new file mode 100644 index 000000000..7fc0157b4 --- /dev/null +++ b/EventListener/DebugListener.php @@ -0,0 +1,28 @@ +startTime = microtime(true); + $this->startMemoryUsage = memory_get_usage(true); + } + + public function onPostExecutor(ExecutorResultEvent $executorResultEvent) + { + $executorResultEvent->getResult()->extensions['debug'] = [ + 'executionTime' => sprintf('%d ms', round(microtime(true) - $this->startTime, 3) * 1000), + 'memoryUsage' => sprintf('%.2F MiB', (memory_get_usage(true) - $this->startMemoryUsage) / 1024 / 1024), + ]; + } +} diff --git a/EventListener/ErrorHandlerListener.php b/EventListener/ErrorHandlerListener.php new file mode 100644 index 000000000..e5b52a0cd --- /dev/null +++ b/EventListener/ErrorHandlerListener.php @@ -0,0 +1,32 @@ +errorHandler = $errorHandler; + $this->throwException = $throwException; + } + + public function setThrowException($throwException) + { + $this->throwException = $throwException; + } + + public function onPostExecutor(ExecutorResultEvent $executorResultEvent) + { + $result = $executorResultEvent->getResult(); + $this->errorHandler->handleErrors($result, $this->throwException); + } +} diff --git a/EventListener/RequestFilesListener.php b/EventListener/RequestFilesListener.php index d01a193ba..b3036abca 100644 --- a/EventListener/RequestFilesListener.php +++ b/EventListener/RequestFilesListener.php @@ -26,6 +26,5 @@ public function onExecutorContextEvent(ExecutorContextEvent $event) $context = $event->getExecutorContext(); $context['request_files'] = $request->files; - $event->setExecutorContext($context); } } diff --git a/Request/Executor.php b/Request/Executor.php index a3e04183a..9f06c96c6 100644 --- a/Request/Executor.php +++ b/Request/Executor.php @@ -3,14 +3,16 @@ namespace Overblog\GraphQLBundle\Request; use GraphQL\Executor\ExecutionResult; +use GraphQL\Executor\Promise\Promise; use GraphQL\Executor\Promise\PromiseAdapter; use GraphQL\Type\Schema; use GraphQL\Validator\DocumentValidator; use GraphQL\Validator\Rules\QueryComplexity; use GraphQL\Validator\Rules\QueryDepth; -use Overblog\GraphQLBundle\Error\ErrorHandler; use Overblog\GraphQLBundle\Event\Events; use Overblog\GraphQLBundle\Event\ExecutorContextEvent; +use Overblog\GraphQLBundle\Event\ExecutorEvent; +use Overblog\GraphQLBundle\Event\ExecutorResultEvent; use Overblog\GraphQLBundle\Executor\ExecutorInterface; use Overblog\GraphQLBundle\Executor\Promise\PromiseAdapterInterface; use Symfony\Component\EventDispatcher\EventDispatcherInterface; @@ -26,15 +28,6 @@ class Executor /** @var EventDispatcherInterface|null */ private $dispatcher; - /** @var bool */ - private $throwException; - - /** @var ErrorHandler|null */ - private $errorHandler; - - /** @var bool */ - private $hasDebugInfo; - /** @var ExecutorInterface */ private $executor; @@ -46,18 +39,12 @@ class Executor public function __construct( ExecutorInterface $executor, - EventDispatcherInterface $dispatcher = null, - $throwException = false, - ErrorHandler $errorHandler = null, - $hasDebugInfo = false, + EventDispatcherInterface $dispatcher, PromiseAdapter $promiseAdapter = null, callable $defaultFieldResolver = null ) { $this->executor = $executor; $this->dispatcher = $dispatcher; - $this->throwException = (bool) $throwException; - $this->errorHandler = $errorHandler; - $hasDebugInfo ? $this->enabledDebugInfo() : $this->disabledDebugInfo(); $this->promiseAdapter = $promiseAdapter; $this->defaultFieldResolver = $defaultFieldResolver; } @@ -78,6 +65,9 @@ public function setPromiseAdapter(PromiseAdapter $promiseAdapter = null) /** * @param string $name + * @param Schema $schema + * + * @return $this */ public function addSchema($name, Schema $schema) { @@ -86,23 +76,27 @@ public function addSchema($name, Schema $schema) return $this; } - public function enabledDebugInfo() - { - $this->hasDebugInfo = true; - - return $this; - } - - public function disabledDebugInfo() + /** + * @param string|null $name + * + * @return Schema + */ + public function getSchema($name = null) { - $this->hasDebugInfo = false; + if (empty($this->schemas)) { + throw new \RuntimeException('At least one schema should be declare.'); + } - return $this; - } + if (null === $name) { + $schema = array_values($this->schemas)[0]; + } else { + if (!isset($this->schemas[$name])) { + throw new NotFoundHttpException(sprintf('Could not found "%s" schema.', $name)); + } + $schema = $this->schemas[$name]; + } - public function hasDebugInfo() - { - return $this->hasDebugInfo; + return $schema; } public function setMaxQueryDepth($maxQueryDepth) @@ -120,113 +114,121 @@ public function setMaxQueryComplexity($maxQueryComplexity) } /** - * @param bool $throwException + * @param null|string $schemaName + * @param array $request + * @param null|array|\ArrayObject|object $rootValue + * @param null|array|\ArrayObject|object $contextValue * - * @return $this + * @return ExecutionResult */ - public function setThrowException($throwException) + public function execute($schemaName, array $request, $rootValue = null, $contextValue = null) { - $this->throwException = (bool) $throwException; - - return $this; - } + $executorEvent = $this->preExecute( + $this->getSchema($schemaName), + isset($request[ParserInterface::PARAM_QUERY]) ? $request[ParserInterface::PARAM_QUERY] : null, + self::createArrayObject($rootValue), + self::createArrayObject($contextValue), + $request[ParserInterface::PARAM_VARIABLES], + isset($request[ParserInterface::PARAM_OPERATION_NAME]) ? $request[ParserInterface::PARAM_OPERATION_NAME] : null + ); - public function execute(array $data, array $context = [], $schemaName = null) - { - if (null !== $this->dispatcher) { - $event = new ExecutorContextEvent($context); - $this->dispatcher->dispatch(Events::EXECUTOR_CONTEXT, $event); - $context = $event->getExecutorContext(); - } + $result = $this->executor->execute( + $executorEvent->getSchema(), + $executorEvent->getRequestString(), + $executorEvent->getRootValue(), + $executorEvent->getContextValue(), + $executorEvent->getVariableValue(), + $executorEvent->getOperationName() + ); - if ($this->promiseAdapter) { - if (!$this->promiseAdapter instanceof PromiseAdapterInterface && !is_callable([$this->promiseAdapter, 'wait'])) { - throw new \RuntimeException( - sprintf( - 'PromiseAdapter should be an object instantiating "%s" or "%s" with a "wait" method.', - PromiseAdapterInterface::class, - PromiseAdapter::class - ) - ); - } - } + $result = $this->postExecute($result); - $schema = $this->getSchema($schemaName); + return $result; + } - $startTime = microtime(true); - $startMemoryUsage = memory_get_usage(true); + /** + * @param Schema $schema + * @param string $requestString + * @param \ArrayObject $rootValue + * @param \ArrayObject $contextValue + * @param array|null $variableValue + * @param string|null $operationName + * + * @return ExecutorEvent + */ + private function preExecute( + Schema $schema, $requestString, + \ArrayObject $rootValue, + \ArrayObject $contextValue, + array $variableValue = null, + $operationName = null + ) { + $this->checkPromiseAdapter(); $this->executor->setPromiseAdapter($this->promiseAdapter); // this is needed when not using only generated types if ($this->defaultFieldResolver) { $this->executor->setDefaultFieldResolver($this->defaultFieldResolver); } + $this->dispatcher->dispatch(Events::EXECUTOR_CONTEXT, new ExecutorContextEvent($contextValue)); - $result = $this->executor->execute( - $schema, - isset($data[ParserInterface::PARAM_QUERY]) ? $data[ParserInterface::PARAM_QUERY] : null, - $context, - $context, - $data[ParserInterface::PARAM_VARIABLES], - isset($data[ParserInterface::PARAM_OPERATION_NAME]) ? $data[ParserInterface::PARAM_OPERATION_NAME] : null + return $this->dispatcher->dispatch( + Events::PRE_EXECUTOR, + new ExecutorEvent($schema, $requestString, $rootValue, $contextValue, $variableValue, $operationName) ); - - if ($this->promiseAdapter) { - $result = $this->promiseAdapter->wait($result); - } - - if (!is_object($result) || !$result instanceof ExecutionResult) { - throw new \RuntimeException( - sprintf('Execution result should be an object instantiating "%s".', ExecutionResult::class) - ); - } - - return $this->prepareResult($result, $startTime, $startMemoryUsage); } /** - * @param ExecutionResult $result - * @param int $startTime - * @param int $startMemoryUsage + * @param ExecutionResult|Promise $result * * @return ExecutionResult */ - private function prepareResult($result, $startTime, $startMemoryUsage) + private function postExecute($result) { - if ($this->hasDebugInfo()) { - $result->extensions['debug'] = [ - 'executionTime' => sprintf('%d ms', round(microtime(true) - $startTime, 3) * 1000), - 'memoryUsage' => sprintf('%.2F MiB', (memory_get_usage(true) - $startMemoryUsage) / 1024 / 1024), - ]; + if ($this->promiseAdapter) { + $result = $this->promiseAdapter->wait($result); } - if (null !== $this->errorHandler) { - $this->errorHandler->handleErrors($result, $this->throwException); - } + $this->checkExecutionResult($result); - return $result; + $event = $this->dispatcher->dispatch( + Events::POST_EXECUTOR, + new ExecutorResultEvent($result) + ); + + return $event->getResult(); } - /** - * @param string|null $name - * - * @return Schema - */ - public function getSchema($name = null) + private function checkPromiseAdapter() { - if (empty($this->schemas)) { - throw new \RuntimeException('At least one schema should be declare.'); + if ($this->promiseAdapter && !$this->promiseAdapter instanceof PromiseAdapterInterface && !is_callable([$this->promiseAdapter, 'wait'])) { + throw new \RuntimeException( + sprintf( + 'PromiseAdapter should be an object instantiating "%s" or "%s" with a "wait" method.', + PromiseAdapterInterface::class, + PromiseAdapter::class + ) + ); } + } - if (null === $name) { - $schema = array_values($this->schemas)[0]; + private function checkExecutionResult($result) + { + if (!is_object($result) || !$result instanceof ExecutionResult) { + throw new \RuntimeException( + sprintf('Execution result should be an object instantiating "%s".', ExecutionResult::class) + ); + } + } + + private static function createArrayObject($data) + { + if (is_array($data) || is_object($data)) { + $object = new \ArrayObject($data); } else { - if (!isset($this->schemas[$name])) { - throw new NotFoundHttpException(sprintf('Could not found "%s" schema.', $name)); - } - $schema = $this->schemas[$name]; + $object = new \ArrayObject(); } - return $schema; + return $object; } } diff --git a/Resources/config/services.yml b/Resources/config/services.yml index b0d6a640d..69b882a9a 100644 --- a/Resources/config/services.yml +++ b/Resources/config/services.yml @@ -11,6 +11,15 @@ services: - [] - false + Overblog\GraphQLBundle\EventListener\ErrorHandlerListener: + class: Overblog\GraphQLBundle\EventListener\ErrorHandlerListener + public: true + arguments: + - "@overblog_graphql.error_handler" + - "%kernel.debug%" + tags: + - { name: kernel.event_listener, event: graphql.post_executor, method: onPostExecutor } + overblog_graphql.executor.default: class: Overblog\GraphQLBundle\Executor\Executor public: false @@ -21,9 +30,6 @@ services: arguments: - "@overblog_graphql.executor" - "@event_dispatcher" - - "%kernel.debug%" - - "@overblog_graphql.error_handler" - - false - "@overblog_graphql.promise_adapter" - "%overblog_graphql.default_resolver%" calls: @@ -104,7 +110,7 @@ services: - ["addImplement", ["Overblog\\GraphQLBundle\\Definition\\Type\\GeneratedTypeInterface"]] - ["setExpressionLanguage", ["@overblog_graphql.expression_language"]] - overblog_graphql.event_listener.request_file_listener: + Overblog\GraphQLBundle\EventListener\RequestFilesListener: class: Overblog\GraphQLBundle\EventListener\RequestFilesListener public: true arguments: diff --git a/Tests/Functional/App/config/config.yml b/Tests/Functional/App/config/config.yml index d3d1ad89a..932e6083c 100644 --- a/Tests/Functional/App/config/config.yml +++ b/Tests/Functional/App/config/config.yml @@ -22,3 +22,6 @@ services: #disable twig error pages twig.exception_listener: class: stdClass + error_handler_listener: + public: true + alias: Overblog\GraphQLBundle\EventListener\ErrorHandlerListener diff --git a/Tests/Functional/App/config/debug/config.yml b/Tests/Functional/App/config/debug/config.yml new file mode 100644 index 000000000..dd7932985 --- /dev/null +++ b/Tests/Functional/App/config/debug/config.yml @@ -0,0 +1,7 @@ +imports: + - { resource: ../connection/config.yml } + +overblog_graphql: + definitions: + class_namespace: "Overblog\\GraphQLBundle\\Debug\\__DEFINITIONS__" + show_debug_info: true diff --git a/Tests/Functional/EventListener/DebugListenerTest.php b/Tests/Functional/EventListener/DebugListenerTest.php new file mode 100644 index 000000000..55a3796f3 --- /dev/null +++ b/Tests/Functional/EventListener/DebugListenerTest.php @@ -0,0 +1,26 @@ + 'connection']); + $response = $this->sendRequest($client, Introspection::getIntrospectionQuery(), true); + $this->assertArrayNotHasKey('extensions', $response); + } + + public function testEnabledDebugInfo() + { + $client = static::createClient(['test_case' => 'debug']); + $response = $this->sendRequest($client, Introspection::getIntrospectionQuery(), true); + $this->assertArrayHasKey('extensions', $response); + $this->assertArrayHasKey('debug', $response['extensions']); + $this->assertArrayHasKey('executionTime', $response['extensions']['debug']); + $this->assertArrayHasKey('memoryUsage', $response['extensions']['debug']); + } +} diff --git a/Tests/Functional/TestCase.php b/Tests/Functional/TestCase.php index 4076cec2e..a8d28c5fa 100644 --- a/Tests/Functional/TestCase.php +++ b/Tests/Functional/TestCase.php @@ -3,6 +3,7 @@ namespace Overblog\GraphQLBundle\Tests\Functional; use Overblog\GraphQLBundle\Tests\Functional\App\TestKernel; +use Symfony\Bundle\FrameworkBundle\Client; use Symfony\Bundle\FrameworkBundle\Test\WebTestCase; use Symfony\Component\Filesystem\Filesystem; use Symfony\Component\HttpFoundation\Request; @@ -65,9 +66,8 @@ protected static function executeGraphQLRequest($query, $rootValue = [], $throwE $request->query->set('query', $query); $req = static::getContainer()->get('overblog_graphql.request_parser')->parse($request); - $executor = static::getContainer()->get('overblog_graphql.request_executor'); - $executor->setThrowException($throwException); - $res = $executor->execute($req, $rootValue); + static::getContainer()->get('error_handler_listener')->setThrowException($throwException); + $res = static::getContainer()->get('overblog_graphql.request_executor')->execute(null, $req, $rootValue); return $res->toArray(); } @@ -119,12 +119,18 @@ protected static function createClientAuthenticated($username, $testCase, $passw protected static function assertResponse($query, array $expected, $username, $testCase, $password = self::DEFAULT_PASSWORD) { $client = self::createClientAuthenticated($username, $testCase, $password); - $client->request('GET', '/', ['query' => $query]); - - $result = $client->getResponse()->getContent(); + $result = self::sendRequest($client, $query); static::assertEquals($expected, json_decode($result, true), $result); return $client; } + + protected static function sendRequest(Client $client, $query, $isDecoded = false) + { + $client->request('GET', '/', ['query' => $query]); + $result = $client->getResponse()->getContent(); + + return $isDecoded ? json_decode($result, true) : $result; + } } diff --git a/Tests/Request/ExecutorTest.php b/Tests/Request/ExecutorTest.php index fff81684d..8ad903f0b 100644 --- a/Tests/Request/ExecutorTest.php +++ b/Tests/Request/ExecutorTest.php @@ -9,17 +9,24 @@ use Overblog\GraphQLBundle\Executor\Executor; use Overblog\GraphQLBundle\Request\Executor as RequestExecutor; use PHPUnit\Framework\TestCase; +use Symfony\Component\EventDispatcher\EventDispatcher; class ExecutorTest extends TestCase { /** @var RequestExecutor */ private $executor; + /** @var EventDispatcher|\PHPUnit_Framework_MockObject_MockObject */ + private $dispatcher; + private $request = ['query' => 'query debug{ myField }', 'variables' => [], 'operationName' => null]; public function setUp() { - $this->executor = new RequestExecutor(new Executor()); + $this->dispatcher = $this->getMockBuilder(EventDispatcher::class)->setMethods(['dispatch'])->getMock(); + $this->dispatcher->expects($this->any())->method('dispatch')->willReturnArgument(1); + + $this->executor = new RequestExecutor(new Executor(), $this->dispatcher); $queryType = new ObjectType([ 'name' => 'Query', 'fields' => [ @@ -41,7 +48,7 @@ public function setUp() public function testInvalidExecutorReturnNotObject() { $this->executor->setExecutor($this->createExecutorExecuteMock(false)); - $this->executor->execute($this->request); + $this->executor->execute(null, $this->request); } /** @@ -51,7 +58,7 @@ public function testInvalidExecutorReturnNotObject() public function testInvalidExecutorReturnInvalidObject() { $this->executor->setExecutor($this->createExecutorExecuteMock(new \stdClass())); - $this->executor->execute($this->request); + $this->executor->execute(null, $this->request); } /** @@ -61,21 +68,7 @@ public function testInvalidExecutorReturnInvalidObject() public function testInvalidExecutorAdapterPromise() { $this->executor->setPromiseAdapter(new ReactPromiseAdapter()); - $this->executor->execute($this->request); - } - - public function testDisabledDebugInfo() - { - $this->assertArrayNotHasKey('debug', $this->executor->disabledDebugInfo()->execute($this->request)->extensions); - } - - public function testEnabledDebugInfo() - { - $result = $this->executor->enabledDebugInfo()->execute($this->request); - - $this->assertArrayHasKey('debug', $result->extensions); - $this->assertArrayHasKey('executionTime', $result->extensions['debug']); - $this->assertArrayHasKey('memoryUsage', $result->extensions['debug']); + $this->executor->execute(null, $this->request); } /** @@ -84,7 +77,7 @@ public function testEnabledDebugInfo() */ public function testGetSchemaNoSchemaFound() { - (new RequestExecutor(new Executor()))->getSchema('fake'); + (new RequestExecutor(new Executor(), $this->dispatcher))->getSchema('fake'); } private function createExecutorExecuteMock($returnValue) diff --git a/composer.json b/composer.json index ac8c552d5..a3b3107b3 100644 --- a/composer.json +++ b/composer.json @@ -35,6 +35,7 @@ "symfony/cache": "^3.1 || ^4.0", "symfony/config": "^3.1 || ^4.0", "symfony/dependency-injection": "^3.1 || ^4.0", + "symfony/event-dispatcher": "^3.1 || ^4.0", "symfony/expression-language": "^3.1 || ^4.0", "symfony/framework-bundle": "^3.1 || ^4.0", "symfony/options-resolver": "^3.1 || ^4.0", From ed25b73cf888c6eb3a03fd981b3f75a6a887fe20 Mon Sep 17 00:00:00 2001 From: Jeremiah VALERIE Date: Wed, 20 Dec 2017 21:19:50 +0100 Subject: [PATCH 2/9] Enable error formatter customization --- DependencyInjection/Configuration.php | 3 +- .../OverblogGraphQLExtension.php | 1 + Error/ErrorHandler.php | 124 +++++++++++------- Error/UserFacingError.php | 22 +++- Resources/doc/security/errors-handling.md | 48 +++++++ Tests/Error/CustomErrorFormatter.php | 37 ++++++ Tests/Error/ErrorHandlerTest.php | 40 ++++++ .../App/Exception/ExampleException.php | 2 +- .../App/config/exception/config.yml | 7 +- Tests/Functional/Exception/ExceptionTest.php | 2 + Tests/Functional/Security/AccessTest.php | 4 + .../Security/QueryComplexityTest.php | 2 + .../Functional/Security/QueryMaxDepthTest.php | 2 + 13 files changed, 240 insertions(+), 54 deletions(-) create mode 100644 Tests/Error/CustomErrorFormatter.php diff --git a/DependencyInjection/Configuration.php b/DependencyInjection/Configuration.php index 4cb70f409..9ae74a4fd 100644 --- a/DependencyInjection/Configuration.php +++ b/DependencyInjection/Configuration.php @@ -43,7 +43,8 @@ public function getConfigTreeBuilder() ->arrayNode('definitions') ->addDefaultsIfNotSet() ->children() - ->scalarNode('internal_error_message')->defaultNull()->end() + ->scalarNode('internal_error_message')->defaultValue(ErrorHandler::DEFAULT_ERROR_MESSAGE)->end() + ->variableNode('error_formatter')->defaultValue(ErrorHandler::DEFAULT_ERROR_FORMATTER)->end() ->variableNode('default_resolver')->defaultValue([Resolver::class, 'defaultResolveFn'])->end() ->scalarNode('class_namespace')->defaultValue('Overblog\\GraphQLBundle\\__DEFINITIONS__')->end() ->scalarNode('cache_dir')->defaultValue($this->cacheDir.'/overblog/graphql-bundle/__definitions__')->end() diff --git a/DependencyInjection/OverblogGraphQLExtension.php b/DependencyInjection/OverblogGraphQLExtension.php index bda34d9b6..2c81446ed 100644 --- a/DependencyInjection/OverblogGraphQLExtension.php +++ b/DependencyInjection/OverblogGraphQLExtension.php @@ -187,6 +187,7 @@ private function setErrorHandlerArguments(array $config, ContainerBuilder $conta ->replaceArgument(2, $this->buildExceptionMap($config['definitions']['exceptions'])) ->addMethodCall('setUserWarningClass', [$config['definitions']['exceptions']['types']['warnings']]) ->addMethodCall('setUserErrorClass', [$config['definitions']['exceptions']['types']['errors']]) + ->addMethodCall('setErrorFormatter', [$config['definitions']['error_formatter']]) ; } } diff --git a/Error/ErrorHandler.php b/Error/ErrorHandler.php index a4f2144f2..8fdda8b69 100644 --- a/Error/ErrorHandler.php +++ b/Error/ErrorHandler.php @@ -3,6 +3,7 @@ namespace Overblog\GraphQLBundle\Error; use GraphQL\Error\Error as GraphQLError; +use GraphQL\Error\FormattedError; use GraphQL\Error\UserError as GraphQLUserError; use GraphQL\Executor\ExecutionResult; use Psr\Log\LoggerInterface; @@ -14,6 +15,8 @@ class ErrorHandler const DEFAULT_ERROR_MESSAGE = 'Internal server Error'; const DEFAULT_USER_WARNING_CLASS = UserWarning::class; const DEFAULT_USER_ERROR_CLASS = UserError::class; + /** callable */ + const DEFAULT_ERROR_FORMATTER = [FormattedError::class, 'createFromException']; /** @var LoggerInterface */ private $logger; @@ -30,6 +33,9 @@ class ErrorHandler /** @var string */ private $userErrorClass = self::DEFAULT_USER_ERROR_CLASS; + /** @var callable|null */ + private $errorFormatter; + /** @var bool */ private $mapExceptionsToParent; @@ -62,15 +68,52 @@ public function setUserErrorClass($userErrorClass) return $this; } + public function setErrorFormatter(callable $errorFormatter = null) + { + $this->errorFormatter = $errorFormatter; + + return $this; + } + + /** + * @param \Exception|\Error $exception + * @param string $errorLevel + */ + public function logException($exception, $errorLevel = LogLevel::ERROR) + { + $message = sprintf( + '%s: %s[%d] (caught exception) at %s line %s.', + get_class($exception), + $exception->getMessage(), + $exception->getCode(), + $exception->getFile(), + $exception->getLine() + ); + + $this->logger->$errorLevel($message, ['exception' => $exception]); + } + + public function handleErrors(ExecutionResult $executionResult, $throwRawException = false) + { + $errorFormatter = $this->errorFormatter ? $this->errorFormatter : self::DEFAULT_ERROR_FORMATTER; + $executionResult->setErrorFormatter($errorFormatter); + FormattedError::setInternalErrorMessage($this->internalErrorMessage); + $exceptions = $this->treatExceptions($executionResult->errors, $throwRawException); + $executionResult->errors = $exceptions['errors']; + if (!empty($exceptions['extensions']['warnings'])) { + $executionResult->extensions['warnings'] = array_map($errorFormatter, $exceptions['extensions']['warnings']); + } + } + /** * @param GraphQLError[] $errors * @param bool $throwRawException * * @return array * - * @throws \Exception + * @throws \Error|\Exception */ - protected function treatExceptions(array $errors, $throwRawException) + private function treatExceptions(array $errors, $throwRawException) { $treatedExceptions = [ 'errors' => [], @@ -80,7 +123,7 @@ protected function treatExceptions(array $errors, $throwRawException) ]; /** @var GraphQLError $error */ - foreach ($errors as $error) { + foreach ($this->flattenErrors($errors) as $error) { $rawException = $this->convertException($error->getPrevious()); // raw GraphQL Error or InvariantViolation exception @@ -89,9 +132,19 @@ protected function treatExceptions(array $errors, $throwRawException) continue; } + // recreate a error with converted exception + $errorWithConvertedException = new GraphQLError( + $error->getMessage(), + $error->nodes, + $error->getSource(), + $error->getPositions(), + $error->path, + $rawException + ); + // user error if ($rawException instanceof $this->userErrorClass) { - $treatedExceptions['errors'][] = $error; + $treatedExceptions['errors'][] = $errorWithConvertedException; if ($rawException->getPrevious()) { $this->logException($rawException->getPrevious()); } @@ -100,68 +153,49 @@ protected function treatExceptions(array $errors, $throwRawException) // user warning if ($rawException instanceof $this->userWarningClass) { - $treatedExceptions['extensions']['warnings'][] = $error; + $treatedExceptions['extensions']['warnings'][] = $errorWithConvertedException; if ($rawException->getPrevious()) { $this->logException($rawException->getPrevious(), LogLevel::WARNING); } continue; } - // multiple errors - if ($rawException instanceof UserErrors) { - $rawExceptions = $rawException; - foreach ($rawExceptions->getErrors() as $rawException) { - $treatedExceptions['errors'][] = GraphQLError::createLocatedError($rawException, $error->nodes); - } - continue; - } - - // if is a try catch exception wrapped in Error + // if is a catch exception wrapped in Error if ($throwRawException) { throw $rawException; } $this->logException($rawException, LogLevel::CRITICAL); - $treatedExceptions['errors'][] = new GraphQLError( - $this->internalErrorMessage, - $error->nodes, - $error->getSource(), - $error->getPositions(), - $error->path, - $rawException - ); + $treatedExceptions['errors'][] = $errorWithConvertedException; } return $treatedExceptions; } /** - * @param \Exception|\Error $exception - * @param string $errorLevel + * @param GraphQLError[] $errors + * + * @return GraphQLError[] */ - public function logException($exception, $errorLevel = LogLevel::ERROR) + private function flattenErrors(array $errors) { - $message = sprintf( - '%s: %s[%d] (caught exception) at %s line %s.', - get_class($exception), - $exception->getMessage(), - $exception->getCode(), - $exception->getFile(), - $exception->getLine() - ); + $flattenErrors = []; - $this->logger->$errorLevel($message, ['exception' => $exception]); - } - - public function handleErrors(ExecutionResult $executionResult, $throwRawException = false) - { - $executionResult->setErrorFormatter([GraphQLError::class, 'formatError']); - $exceptions = $this->treatExceptions($executionResult->errors, $throwRawException); - $executionResult->errors = $exceptions['errors']; - if (!empty($exceptions['extensions']['warnings'])) { - $executionResult->extensions['warnings'] = array_map([GraphQLError::class, 'formatError'], $exceptions['extensions']['warnings']); + foreach ($errors as $error) { + $rawException = $error->getPrevious(); + // multiple errors + if ($rawException instanceof UserErrors) { + $rawExceptions = $rawException; + foreach ($rawExceptions->getErrors() as $rawException) { + $flattenErrors[] = GraphQLError::createLocatedError($rawException, $error->nodes, $error->path); + } + } else { + $flattenErrors[] = $error; + } } + + return $flattenErrors; } /** @@ -172,7 +206,7 @@ public function handleErrors(ExecutionResult $executionResult, $throwRawExceptio * * @return \Exception|\Error */ - protected function convertException($rawException = null) + private function convertException($rawException = null) { if (null === $rawException) { return; diff --git a/Error/UserFacingError.php b/Error/UserFacingError.php index 6237ce6cc..4129218d0 100644 --- a/Error/UserFacingError.php +++ b/Error/UserFacingError.php @@ -2,9 +2,23 @@ namespace Overblog\GraphQLBundle\Error; -/** - * Class UserFacingError. - */ -abstract class UserFacingError extends \Exception +use GraphQL\Error\ClientAware; + +abstract class UserFacingError extends \RuntimeException implements ClientAware { + /** + * {@inheritdoc} + */ + public function isClientSafe() + { + return true; + } + + /** + * {@inheritdoc} + */ + public function getCategory() + { + return 'user'; + } } diff --git a/Resources/doc/security/errors-handling.md b/Resources/doc/security/errors-handling.md index dd44950d0..0a78cc2ae 100644 --- a/Resources/doc/security/errors-handling.md +++ b/Resources/doc/security/errors-handling.md @@ -120,3 +120,51 @@ overblog_graphql: The message of those exceptions are then shown to the user like other `UserError`s or `UserWarning`s. + +Custom Error Formatting +------------------------- + +It is possible to define custom formatter for result errors. +Let's say we want to add `code` entry to each error. + +```php +getCode(); + if ($e instanceof Error && $e->getPrevious()) { + $code = $e->getPrevious()->getCode(); + } + + $formattedError = FormattedError::createFromException($e); + $formattedError['code'] = $code; + + return $formattedError; + } +} +``` + +Change configuration to use custom `error_formatter`: + +```yaml +#app/config/config.yml +overblog_graphql: + definitions: + error_formatter: 'App\GraphQL\MyErrorFormatter::format' +``` + +**Note:** +- `error_formatter` is also impacting the formatting `extensions.warnings`. diff --git a/Tests/Error/CustomErrorFormatter.php b/Tests/Error/CustomErrorFormatter.php new file mode 100644 index 000000000..bd849146f --- /dev/null +++ b/Tests/Error/CustomErrorFormatter.php @@ -0,0 +1,37 @@ +getCode(); + if ($e instanceof Error && $e->getPrevious()) { + $code = $e->getPrevious()->getCode(); + } + + $formattedError = FormattedError::createFromException($e); + $formattedError['code'] = $code; + + return $formattedError; + } + + /** + * @param \Throwable $e + * + * @return array + */ + public function __invoke($e) + { + return static::format($e); + } +} diff --git a/Tests/Error/ErrorHandlerTest.php b/Tests/Error/ErrorHandlerTest.php index 6b67aae3b..350f01bac 100644 --- a/Tests/Error/ErrorHandlerTest.php +++ b/Tests/Error/ErrorHandlerTest.php @@ -41,30 +41,38 @@ public function testMaskErrorWithThrowExceptionSetToFalse() 'errors' => [ [ 'message' => 'Error without wrapped exception', + 'category' => 'graphql', ], [ 'message' => ErrorHandler::DEFAULT_ERROR_MESSAGE, + 'category' => 'internal', ], [ 'message' => 'Error with wrapped user error', + 'category' => 'user', ], [ 'message' => 'My User Error 1', + 'category' => 'user', ], [ 'message' => 'My User Error 2', + 'category' => 'user', ], [ 'message' => 'My User Error 3', + 'category' => 'user', ], [ 'message' => 'Invalid value!', + 'category' => 'user', ], ], 'extensions' => [ 'warnings' => [ [ 'message' => 'Error with wrapped user warning', + 'category' => 'user', ], ], ], @@ -114,6 +122,7 @@ public function testMaskErrorWithWrappedUserErrorAndThrowExceptionSetToTrue() 'errors' => [ [ 'message' => 'Error with wrapped user error', + 'category' => 'user', ], ], ]; @@ -136,6 +145,7 @@ public function testMaskErrorWithoutWrappedExceptionAndThrowExceptionSetToTrue() 'errors' => [ [ 'message' => 'Error without wrapped exception', + 'category' => 'graphql', ], ], ]; @@ -161,6 +171,7 @@ public function testConvertExceptionToUserWarning() 'warnings' => [ [ 'message' => 'Error with invalid argument exception', + 'category' => 'user', ], ], ], @@ -204,6 +215,29 @@ public function testConvertExceptionUsingParentExceptionMatchesAlwaysFirstExactE } } + public function testCustomFormatterError() + { + $executionResult = new ExecutionResult( + null, + [ + new GraphQLError('Error with wrapped exception', null, null, null, null, new \Exception('My Exception message', 123)), + ] + ); + + $this->errorHandler->setErrorFormatter(new CustomErrorFormatter()); + + $this->errorHandler->handleErrors($executionResult); + $this->assertEquals([ + 'errors' => [ + [ + 'message' => 'Internal server Error', + 'category' => 'internal', + 'code' => 123, + ], + ], + ], $executionResult->toArray()); + } + /** * @return array */ @@ -219,6 +253,7 @@ public function parentExceptionMappingDataProvider() 'errors' => [ [ 'message' => 'Error with invalid argument exception', + 'category' => 'user', ], ], ], @@ -244,6 +279,7 @@ public function parentExceptionMappingDataProvider() 'errors' => [ [ 'message' => 'Error with invalid argument exception', + 'category' => 'user', ], ], ], @@ -258,6 +294,7 @@ public function parentExceptionMappingDataProvider() 'warnings' => [ [ 'message' => 'Error with invalid argument exception', + 'category' => 'user', ], ], ], @@ -273,6 +310,7 @@ public function parentExceptionMappingDataProvider() 'errors' => [ [ 'message' => 'Error with invalid argument exception', + 'category' => 'user', ], ], ], @@ -288,6 +326,7 @@ public function parentExceptionMappingDataProvider() 'warnings' => [ [ 'message' => 'Error with invalid argument exception', + 'category' => 'user', ], ], ], @@ -303,6 +342,7 @@ public function parentExceptionMappingDataProvider() 'errors' => [ [ 'message' => 'Error with invalid argument exception', + 'category' => 'user', ], ], ], diff --git a/Tests/Functional/App/Exception/ExampleException.php b/Tests/Functional/App/Exception/ExampleException.php index e2c29e93b..8538506ff 100644 --- a/Tests/Functional/App/Exception/ExampleException.php +++ b/Tests/Functional/App/Exception/ExampleException.php @@ -6,6 +6,6 @@ class ExampleException { public function __invoke() { - throw new \InvalidArgumentException('Invalid argument exception'); + throw new \InvalidArgumentException('Invalid argument exception', 321); } } diff --git a/Tests/Functional/App/config/exception/config.yml b/Tests/Functional/App/config/exception/config.yml index 6a0a5b609..d9d933d8a 100644 --- a/Tests/Functional/App/config/exception/config.yml +++ b/Tests/Functional/App/config/exception/config.yml @@ -4,10 +4,11 @@ imports: overblog_graphql: definitions: - class_namespace: "Overblog\\GraphQLBundle\\Exception\\__DEFINITIONS__" + error_formatter: 'Overblog\GraphQLBundle\Tests\Error\CustomErrorFormatter::format' + class_namespace: 'Overblog\GraphQLBundle\Exception\__DEFINITIONS__' exceptions: errors: - - "InvalidArgumentException" + - 'InvalidArgumentException' schema: query: Query mutation: ~ @@ -15,4 +16,4 @@ overblog_graphql: types: - type: yaml - dir: "%kernel.root_dir%/config/exception/mapping" + dir: '%kernel.root_dir%/config/exception/mapping' diff --git a/Tests/Functional/Exception/ExceptionTest.php b/Tests/Functional/Exception/ExceptionTest.php index 97862f2a2..2674fd0ee 100644 --- a/Tests/Functional/Exception/ExceptionTest.php +++ b/Tests/Functional/Exception/ExceptionTest.php @@ -35,6 +35,8 @@ public function testExceptionIsMappedToAWarning() ], ], 'path' => ['test'], + 'category' => 'user', + 'code' => 321, ], ]; diff --git a/Tests/Functional/Security/AccessTest.php b/Tests/Functional/Security/AccessTest.php index a7bd64d1d..825d03f48 100644 --- a/Tests/Functional/Security/AccessTest.php +++ b/Tests/Functional/Security/AccessTest.php @@ -80,6 +80,7 @@ public function testNotAuthenticatedUserAccessToUserName() 'message' => 'Access denied to this field.', 'locations' => [['line' => 1, 'column' => 16]], 'path' => ['user', 'name'], + 'category' => 'user', ], ], ], @@ -141,6 +142,7 @@ public function testUserForbiddenField() ], ], 'path' => ['user', 'forbidden'], + 'category' => 'user', ], ], ], @@ -212,6 +214,7 @@ public function testMutationAllowedButNoRightsToDisplayPayload() ], ], 'path' => ['simpleMutationWithThunkFields', 'result'], + 'category' => 'user', ], ], ], @@ -237,6 +240,7 @@ public function testMutationNotAllowedUser() ], ], 'path' => ['simpleMutationWithThunkFields'], + 'category' => 'user', ], ], ]; diff --git a/Tests/Functional/Security/QueryComplexityTest.php b/Tests/Functional/Security/QueryComplexityTest.php index 71bba36ae..2f67add50 100644 --- a/Tests/Functional/Security/QueryComplexityTest.php +++ b/Tests/Functional/Security/QueryComplexityTest.php @@ -40,6 +40,7 @@ public function testComplexityReachLimitation() 'errors' => [ [ 'message' => 'Max query complexity should be 10 but got 54.', + 'category' => 'graphql', ], ], ]; @@ -53,6 +54,7 @@ public function testComplexityReachLimitationEnv() 'errors' => [ [ 'message' => 'Max query complexity should be 10 but got 54.', + 'category' => 'graphql', ], ], ]; diff --git a/Tests/Functional/Security/QueryMaxDepthTest.php b/Tests/Functional/Security/QueryMaxDepthTest.php index 71cde4a2d..43ee3eb69 100644 --- a/Tests/Functional/Security/QueryMaxDepthTest.php +++ b/Tests/Functional/Security/QueryMaxDepthTest.php @@ -47,6 +47,7 @@ public function testMaxDepthReachLimitation() 'errors' => [ [ 'message' => 'Max query depth should be 3 but got 6.', + 'category' => 'graphql', ], ], ]; @@ -60,6 +61,7 @@ public function testMaxDepthReachLimitationEnv() 'errors' => [ [ 'message' => 'Max query depth should be 3 but got 6.', + 'category' => 'graphql', ], ], ]; From 416f9ba299ede8d035d596dc0e9ac611e08eec7d Mon Sep 17 00:00:00 2001 From: Jeremiah VALERIE Date: Sun, 24 Dec 2017 20:23:55 +0100 Subject: [PATCH 3/9] Use event to handle error formatting --- DependencyInjection/Configuration.php | 360 +++++++++++------- .../OverblogGraphQLExtension.php | 62 ++- Error/ErrorHandler.php | 78 ++-- Event/ErrorFormattingEvent.php | 34 ++ Event/Events.php | 1 + Event/ExecutorContextEvent.php | 2 +- Event/ExecutorEvent.php | 2 +- Event/ExecutorResultEvent.php | 2 +- EventListener/ClassLoaderListener.php | 2 +- EventListener/DebugListener.php | 2 +- EventListener/ErrorHandlerListener.php | 15 +- EventListener/ErrorLoggerListener.php | 73 ++++ EventListener/RequestFilesListener.php | 2 +- README.md | 2 +- Resources/config/services.yml | 18 - .../index.md} | 45 +-- Resources/doc/security/index.md | 3 +- .../OverblogGraphQLTypesExtensionTest.php | 2 +- Tests/Error/CustomErrorFormatter.php | 37 -- Tests/Error/ErrorHandlerTest.php | 58 +-- Tests/Functional/App/config/config.yml | 5 +- .../App/config/exception/config.yml | 6 +- .../Functional/App/config/mutation/config.yml | 3 +- Tests/Functional/Exception/ExceptionTest.php | 1 - Tests/Functional/TestCase.php | 5 +- UPGRADE-0.11.md | 20 + 26 files changed, 488 insertions(+), 352 deletions(-) create mode 100644 Event/ErrorFormattingEvent.php create mode 100644 EventListener/ErrorLoggerListener.php rename Resources/doc/{security/errors-handling.md => error-handling/index.md} (76%) delete mode 100644 Tests/Error/CustomErrorFormatter.php diff --git a/DependencyInjection/Configuration.php b/DependencyInjection/Configuration.php index 9ae74a4fd..4e0133374 100644 --- a/DependencyInjection/Configuration.php +++ b/DependencyInjection/Configuration.php @@ -5,12 +5,18 @@ use GraphQL\Validator\Rules\QueryComplexity; use GraphQL\Validator\Rules\QueryDepth; use Overblog\GraphQLBundle\Error\ErrorHandler; +use Overblog\GraphQLBundle\EventListener\ErrorLoggerListener; use Overblog\GraphQLBundle\Resolver\Resolver; +use Symfony\Component\Config\Definition\Builder\ArrayNodeDefinition; +use Symfony\Component\Config\Definition\Builder\EnumNodeDefinition; +use Symfony\Component\Config\Definition\Builder\ScalarNodeDefinition; use Symfony\Component\Config\Definition\Builder\TreeBuilder; use Symfony\Component\Config\Definition\ConfigurationInterface; class Configuration implements ConfigurationInterface { + const NAME = 'overblog_graphql'; + /** bool */ private $debug; @@ -32,163 +38,257 @@ public function __construct($debug, $cacheDir = null) public function getConfigTreeBuilder() { $treeBuilder = new TreeBuilder(); - $rootNode = $treeBuilder->root('overblog_graphql'); + $rootNode = $treeBuilder->root(self::NAME); $rootNode ->children() - ->enumNode('batching_method') - ->values(['relay', 'apollo']) - ->defaultValue('relay') - ->end() - ->arrayNode('definitions') + ->append($this->batchingMethodSection()) + ->append($this->definitionsSection()) + ->append($this->errorsHandlerSection()) + ->append($this->servicesSection()) + ->append($this->securitySection()) + ->end(); + + return $treeBuilder; + } + + private function batchingMethodSection() + { + $builder = new TreeBuilder(); + /** @var EnumNodeDefinition $node */ + $node = $builder->root('batching_method', 'enum'); + + $node + ->values(['relay', 'apollo']) + ->defaultValue('relay') + ->end(); + + return $node; + } + + private function errorsHandlerSection() + { + $builder = new TreeBuilder(); + /** @var ArrayNodeDefinition $node */ + $node = $builder->root('errors_handler'); + $node + ->treatFalseLike(['enabled' => false]) + ->treatTrueLike(['enabled' => true]) + ->treatNullLike(['enabled' => true]) + ->addDefaultsIfNotSet() + ->children() + ->booleanNode('enabled')->defaultTrue()->end() + ->scalarNode('internal_error_message')->defaultValue(ErrorHandler::DEFAULT_ERROR_MESSAGE)->end() + ->booleanNode('rethrow_internal_exceptions')->defaultFalse()->end() + ->booleanNode('debug')->defaultValue($this->debug)->end() + ->booleanNode('log')->defaultTrue()->end() + ->scalarNode('logger_service')->defaultValue(ErrorLoggerListener::DEFAULT_LOGGER_SERVICE)->end() + ->booleanNode('map_exceptions_to_parent')->defaultFalse()->end() + ->arrayNode('exceptions') ->addDefaultsIfNotSet() ->children() - ->scalarNode('internal_error_message')->defaultValue(ErrorHandler::DEFAULT_ERROR_MESSAGE)->end() - ->variableNode('error_formatter')->defaultValue(ErrorHandler::DEFAULT_ERROR_FORMATTER)->end() - ->variableNode('default_resolver')->defaultValue([Resolver::class, 'defaultResolveFn'])->end() - ->scalarNode('class_namespace')->defaultValue('Overblog\\GraphQLBundle\\__DEFINITIONS__')->end() - ->scalarNode('cache_dir')->defaultValue($this->cacheDir.'/overblog/graphql-bundle/__definitions__')->end() - ->booleanNode('use_classloader_listener')->defaultTrue()->end() - ->booleanNode('auto_compile')->defaultTrue()->end() - ->booleanNode('show_debug_info')->defaultFalse()->end() - ->booleanNode('config_validation')->defaultValue($this->debug)->end() - ->arrayNode('schema') - ->beforeNormalization() - ->ifTrue(function ($v) { - return isset($v['query']) && is_string($v['query']) || isset($v['mutation']) && is_string($v['mutation']); - }) - ->then(function ($v) { - return ['default' => $v]; - }) - ->end() - ->useAttributeAsKey('name') - ->prototype('array') - ->addDefaultsIfNotSet() - ->children() - ->scalarNode('query')->defaultNull()->end() - ->scalarNode('mutation')->defaultNull()->end() - ->scalarNode('subscription')->defaultNull()->end() - ->end() - ->end() + ->arrayNode('warnings') + ->treatNullLike([]) + ->prototype('scalar')->end() ->end() - ->arrayNode('auto_mapping') - ->treatFalseLike(['enabled' => false]) - ->treatTrueLike(['enabled' => true]) - ->treatNullLike(['enabled' => true]) - ->addDefaultsIfNotSet() - ->children() - ->booleanNode('enabled')->defaultTrue()->end() - ->arrayNode('directories') - ->info('List of directories containing GraphQL classes.') - ->prototype('scalar')->end() - ->end() - ->end() - ->end() - ->arrayNode('mappings') - ->children() - ->arrayNode('auto_discover') - ->treatFalseLike(['bundles' => false, 'root_dir' => false]) - ->treatTrueLike(['bundles' => true, 'root_dir' => true]) - ->treatNullLike(['bundles' => true, 'root_dir' => true]) - ->addDefaultsIfNotSet() - ->children() - ->booleanNode('bundles')->defaultTrue()->end() - ->booleanNode('root_dir')->defaultTrue()->end() - ->end() - ->end() - ->arrayNode('types') - ->prototype('array') - ->addDefaultsIfNotSet() - ->beforeNormalization() - ->ifTrue(function ($v) { - return isset($v['type']) && 'yml' === $v['type']; - }) - ->then(function ($v) { - $v['type'] = 'yaml'; - - return $v; - }) - ->end() - ->children() - ->enumNode('type')->values(['yaml', 'xml'])->defaultNull()->end() - ->scalarNode('dir')->defaultNull()->end() - ->scalarNode('suffix')->defaultValue(OverblogGraphQLTypesExtension::DEFAULT_TYPES_SUFFIX)->end() - ->end() - ->end() - ->end() - ->end() + ->arrayNode('errors') + ->treatNullLike([]) + ->prototype('scalar')->end() ->end() - ->booleanNode('map_exceptions_to_parent')->defaultFalse()->end() - ->arrayNode('exceptions') + ->arrayNode('types') ->addDefaultsIfNotSet() ->children() - ->arrayNode('warnings') - ->treatNullLike([]) - ->prototype('scalar')->end() - ->end() - ->arrayNode('errors') - ->treatNullLike([]) - ->prototype('scalar')->end() + ->scalarNode('warnings') + ->defaultValue(ErrorHandler::DEFAULT_USER_WARNING_CLASS) ->end() - ->arrayNode('types') - ->addDefaultsIfNotSet() - ->children() - ->scalarNode('warnings') - ->defaultValue(ErrorHandler::DEFAULT_USER_WARNING_CLASS) - ->end() - ->scalarNode('errors') - ->defaultValue(ErrorHandler::DEFAULT_USER_ERROR_CLASS) - ->end() - ->end() + ->scalarNode('errors') + ->defaultValue(ErrorHandler::DEFAULT_USER_ERROR_CLASS) ->end() ->end() ->end() + ->end() + ->end() + ->end(); - ->arrayNode('builders') - ->children() - ->append($this->addBuilderSection('field')) - ->append($this->addBuilderSection('args')) - ->end() - ->end() + return $node; + } + private function definitionsSection() + { + $builder = new TreeBuilder(); + /** @var ArrayNodeDefinition $node */ + $node = $builder->root('definitions'); + $node + ->addDefaultsIfNotSet() + ->children() + ->variableNode('default_resolver')->defaultValue([Resolver::class, 'defaultResolveFn'])->end() + ->scalarNode('class_namespace')->defaultValue('Overblog\\GraphQLBundle\\__DEFINITIONS__')->end() + ->scalarNode('cache_dir')->defaultValue($this->cacheDir.'/overblog/graphql-bundle/__definitions__')->end() + ->booleanNode('use_classloader_listener')->defaultTrue()->end() + ->booleanNode('auto_compile')->defaultTrue()->end() + ->booleanNode('show_debug_info')->info('Show some performance stats in extensions')->defaultFalse()->end() + ->booleanNode('config_validation')->defaultValue($this->debug)->end() + ->append($this->definitionsSchemaSection()) + ->append($this->definitionsAutoMappingSection()) + ->append($this->definitionsMappingsSection()) + ->arrayNode('builders') + ->children() + ->append($this->builderSection('field')) + ->append($this->builderSection('args')) ->end() ->end() - ->arrayNode('services') + + ->end() + ->end(); + + return $node; + } + + private function servicesSection() + { + $builder = new TreeBuilder(); + /** @var ArrayNodeDefinition $node */ + $node = $builder->root('services'); + $node + ->addDefaultsIfNotSet() + ->children() + ->scalarNode('executor') + ->defaultValue(self::NAME.'.executor.default') + ->end() + ->scalarNode('promise_adapter') + ->defaultValue(self::NAME.'.promise_adapter.default') + ->end() + ->scalarNode('expression_language') + ->defaultValue(self::NAME.'.expression_language.default') + ->end() + ->scalarNode('cache_expression_language_parser') + ->defaultValue(self::NAME.'.cache_expression_language_parser.default') + ->end() + ->end() + ->end(); + + return $node; + } + + private function securitySection() + { + $builder = new TreeBuilder(); + /** @var ArrayNodeDefinition $node */ + $node = $builder->root('security'); + $node + ->addDefaultsIfNotSet() + ->children() + ->append($this->securityQuerySection('query_max_depth', QueryDepth::DISABLED)) + ->append($this->securityQuerySection('query_max_complexity', QueryComplexity::DISABLED)) + ->booleanNode('handle_cors')->defaultFalse()->end() + ->end() + ->end(); + + return $node; + } + + private function definitionsSchemaSection() + { + $builder = new TreeBuilder(); + /** @var ArrayNodeDefinition $node */ + $node = $builder->root('schema'); + $node + ->beforeNormalization() + ->ifTrue(function ($v) { + return isset($v['query']) && is_string($v['query']) || isset($v['mutation']) && is_string($v['mutation']); + }) + ->then(function ($v) { + return ['default' => $v]; + }) + ->end() + ->useAttributeAsKey('name') + ->prototype('array') + ->addDefaultsIfNotSet() + ->children() + ->scalarNode('query')->defaultNull()->end() + ->scalarNode('mutation')->defaultNull()->end() + ->scalarNode('subscription')->defaultNull()->end() + ->end() + ->end() + ->end(); + + return $node; + } + + private function definitionsAutoMappingSection() + { + $builder = new TreeBuilder(); + /** @var ArrayNodeDefinition $node */ + $node = $builder->root('auto_mapping'); + $node + ->treatFalseLike(['enabled' => false]) + ->treatTrueLike(['enabled' => true]) + ->treatNullLike(['enabled' => true]) + ->addDefaultsIfNotSet() + ->children() + ->booleanNode('enabled')->defaultTrue()->end() + ->arrayNode('directories') + ->info('List of directories containing GraphQL classes.') + ->prototype('scalar')->end() + ->end() + ->end() + ->end(); + + return $node; + } + + private function definitionsMappingsSection() + { + $builder = new TreeBuilder(); + /** @var ArrayNodeDefinition $node */ + $node = $builder->root('mappings'); + $node + ->children() + ->arrayNode('auto_discover') + ->treatFalseLike(['bundles' => false, 'root_dir' => false]) + ->treatTrueLike(['bundles' => true, 'root_dir' => true]) + ->treatNullLike(['bundles' => true, 'root_dir' => true]) ->addDefaultsIfNotSet() ->children() - ->scalarNode('executor') - ->defaultValue('overblog_graphql.executor.default') - ->end() - ->scalarNode('promise_adapter') - ->defaultValue('overblog_graphql.promise_adapter.default') - ->end() - ->scalarNode('expression_language') - ->defaultValue('overblog_graphql.expression_language.default') - ->end() - ->scalarNode('cache_expression_language_parser') - ->defaultValue('overblog_graphql.cache_expression_language_parser.default') - ->end() + ->booleanNode('bundles')->defaultTrue()->end() + ->booleanNode('root_dir')->defaultTrue()->end() ->end() ->end() - ->arrayNode('security') - ->addDefaultsIfNotSet() - ->children() - ->append($this->addSecurityQuerySection('query_max_depth', QueryDepth::DISABLED)) - ->append($this->addSecurityQuerySection('query_max_complexity', QueryComplexity::DISABLED)) - ->booleanNode('handle_cors')->defaultFalse()->end() + ->arrayNode('types') + ->prototype('array') + ->addDefaultsIfNotSet() + ->beforeNormalization() + ->ifTrue(function ($v) { + return isset($v['type']) && 'yml' === $v['type']; + }) + ->then(function ($v) { + $v['type'] = 'yaml'; + + return $v; + }) + ->end() + ->children() + ->enumNode('type')->values(['yaml', 'xml'])->defaultNull()->end() + ->scalarNode('dir')->defaultNull()->end() + ->scalarNode('suffix')->defaultValue(OverblogGraphQLTypesExtension::DEFAULT_TYPES_SUFFIX)->end() + ->end() ->end() ->end() - ->end(); + ->end() + ->end(); - return $treeBuilder; + return $node; } /** * @param string $name + * + * @return ArrayNodeDefinition */ - private function addBuilderSection($name) + private function builderSection($name) { $builder = new TreeBuilder(); + /** @var ArrayNodeDefinition $node */ $node = $builder->root($name); $node->beforeNormalization() ->ifTrue(function ($v) { @@ -221,10 +321,14 @@ private function addBuilderSection($name) /** * @param string $name + * @param bool $disabledValue + * + * @return ScalarNodeDefinition */ - private function addSecurityQuerySection($name, $disabledValue) + private function securityQuerySection($name, $disabledValue) { $builder = new TreeBuilder(); + /** @var ScalarNodeDefinition $node */ $node = $builder->root($name, 'scalar'); $node->beforeNormalization() ->ifTrue(function ($v) { @@ -250,7 +354,7 @@ private function addSecurityQuerySection($name, $disabledValue) ->ifTrue(function ($v) { return is_int($v) && $v < 0; }) - ->thenInvalid('"overblog_graphql.security.'.$name.'" must be greater or equal to 0.') + ->thenInvalid(sprintf('"%s.security.%s" must be greater or equal to 0.', self::NAME, $name)) ->end() ; diff --git a/DependencyInjection/OverblogGraphQLExtension.php b/DependencyInjection/OverblogGraphQLExtension.php index 2c81446ed..9d23079f1 100644 --- a/DependencyInjection/OverblogGraphQLExtension.php +++ b/DependencyInjection/OverblogGraphQLExtension.php @@ -5,12 +5,16 @@ use GraphQL\Type\Schema; use Overblog\GraphQLBundle\CacheWarmer\CompileCacheWarmer; use Overblog\GraphQLBundle\Config\Processor\BuilderProcessor; +use Overblog\GraphQLBundle\Error\ErrorHandler; use Overblog\GraphQLBundle\Event\Events; use Overblog\GraphQLBundle\EventListener\ClassLoaderListener; use Overblog\GraphQLBundle\EventListener\DebugListener; +use Overblog\GraphQLBundle\EventListener\ErrorHandlerListener; +use Overblog\GraphQLBundle\EventListener\ErrorLoggerListener; use Symfony\Component\Cache\Adapter\ArrayAdapter; use Symfony\Component\Config\FileLocator; use Symfony\Component\DependencyInjection\ContainerBuilder; +use Symfony\Component\DependencyInjection\ContainerInterface; use Symfony\Component\DependencyInjection\Definition; use Symfony\Component\DependencyInjection\Extension\PrependExtensionInterface; use Symfony\Component\DependencyInjection\Loader\YamlFileLoader; @@ -34,7 +38,7 @@ public function load(array $configs, ContainerBuilder $container) $this->setServicesAliases($config, $container); $this->setSchemaBuilderArguments($config, $container); $this->setSchemaArguments($config, $container); - $this->setErrorHandlerArguments($config, $container); + $this->setErrorHandler($config, $container); $this->setSecurity($config, $container); $this->setConfigBuilders($config, $container); $this->setDebugListener($config, $container); @@ -58,7 +62,7 @@ public function prepend(ContainerBuilder $container) public function getAlias() { - return 'overblog_graphql'; + return Configuration::NAME; } public function getConfiguration(array $config, ContainerBuilder $container) @@ -167,28 +171,44 @@ private function setSecurity(array $config, ContainerBuilder $container) } } - private function setErrorHandlerArguments(array $config, ContainerBuilder $container) + private function setErrorHandler(array $config, ContainerBuilder $container) { - $errorHandlerDefinition = $container->getDefinition($this->getAlias().'.error_handler'); - - if (isset($config['definitions']['internal_error_message'])) { - $errorHandlerDefinition->replaceArgument(0, $config['definitions']['internal_error_message']); - } - - if (isset($config['definitions']['map_exceptions_to_parent'])) { - $errorHandlerDefinition->replaceArgument( - 3, - $config['definitions']['map_exceptions_to_parent'] - ); - } + if ($config['errors_handler']['enabled']) { + $id = $this->getAlias().'.error_handler'; + $errorHandlerDefinition = $container->setDefinition($id, new Definition(ErrorHandler::class)); + $errorHandlerDefinition->setPublic(false) + ->setArguments( + [ + new Reference('event_dispatcher'), + $config['errors_handler']['internal_error_message'], + $this->buildExceptionMap($config['errors_handler']['exceptions']), + $config['errors_handler']['map_exceptions_to_parent'], + ] + ) + ->addMethodCall('setUserWarningClass', [$config['errors_handler']['exceptions']['types']['warnings']]) + ->addMethodCall('setUserErrorClass', [$config['errors_handler']['exceptions']['types']['errors']]) + ; - if (isset($config['definitions']['exceptions'])) { - $errorHandlerDefinition - ->replaceArgument(2, $this->buildExceptionMap($config['definitions']['exceptions'])) - ->addMethodCall('setUserWarningClass', [$config['definitions']['exceptions']['types']['warnings']]) - ->addMethodCall('setUserErrorClass', [$config['definitions']['exceptions']['types']['errors']]) - ->addMethodCall('setErrorFormatter', [$config['definitions']['error_formatter']]) + $errorHandlerListenerDefinition = $container->setDefinition(ErrorHandlerListener::class, new Definition(ErrorHandlerListener::class)); + $errorHandlerListenerDefinition->setPublic(true) + ->setArguments([new Reference($id), $config['errors_handler']['rethrow_internal_exceptions'], $config['errors_handler']['debug']]) + ->addTag('kernel.event_listener', ['event' => Events::POST_EXECUTOR, 'method' => 'onPostExecutor']) ; + + if ($config['errors_handler']['log']) { + $loggerServiceId = $config['errors_handler']['logger_service']; + $invalidBehavior = ErrorLoggerListener::DEFAULT_LOGGER_SERVICE === $loggerServiceId ? ContainerInterface::NULL_ON_INVALID_REFERENCE : ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE; + $errorHandlerListenerDefinition = $container->setDefinition(ErrorLoggerListener::class, new Definition(ErrorLoggerListener::class)); + $errorHandlerListenerDefinition->setPublic(true) + ->setArguments([ + new Reference($loggerServiceId, $invalidBehavior), + $config['errors_handler']['exceptions']['types']['errors'], + $config['errors_handler']['exceptions']['types']['warnings'], + ] + ) + ->addTag('kernel.event_listener', ['event' => Events::ERROR_FORMATTING, 'method' => 'onErrorFormatting']) + ; + } } } diff --git a/Error/ErrorHandler.php b/Error/ErrorHandler.php index 8fdda8b69..e6fdd3145 100644 --- a/Error/ErrorHandler.php +++ b/Error/ErrorHandler.php @@ -2,24 +2,24 @@ namespace Overblog\GraphQLBundle\Error; +use GraphQL\Error\ClientAware; +use GraphQL\Error\Debug; use GraphQL\Error\Error as GraphQLError; use GraphQL\Error\FormattedError; use GraphQL\Error\UserError as GraphQLUserError; use GraphQL\Executor\ExecutionResult; -use Psr\Log\LoggerInterface; -use Psr\Log\LogLevel; -use Psr\Log\NullLogger; +use Overblog\GraphQLBundle\Event\ErrorFormattingEvent; +use Overblog\GraphQLBundle\Event\Events; +use Symfony\Component\EventDispatcher\EventDispatcherInterface; class ErrorHandler { const DEFAULT_ERROR_MESSAGE = 'Internal server Error'; const DEFAULT_USER_WARNING_CLASS = UserWarning::class; const DEFAULT_USER_ERROR_CLASS = UserError::class; - /** callable */ - const DEFAULT_ERROR_FORMATTER = [FormattedError::class, 'createFromException']; - /** @var LoggerInterface */ - private $logger; + /** @var EventDispatcherInterface */ + private $dispatcher; /** @var string */ private $internalErrorMessage; @@ -33,19 +33,16 @@ class ErrorHandler /** @var string */ private $userErrorClass = self::DEFAULT_USER_ERROR_CLASS; - /** @var callable|null */ - private $errorFormatter; - /** @var bool */ private $mapExceptionsToParent; public function __construct( + EventDispatcherInterface $dispatcher, $internalErrorMessage = null, - LoggerInterface $logger = null, array $exceptionMap = [], $mapExceptionsToParent = false ) { - $this->logger = (null === $logger) ? new NullLogger() : $logger; + $this->dispatcher = $dispatcher; if (empty($internalErrorMessage)) { $internalErrorMessage = self::DEFAULT_ERROR_MESSAGE; } @@ -68,36 +65,10 @@ public function setUserErrorClass($userErrorClass) return $this; } - public function setErrorFormatter(callable $errorFormatter = null) - { - $this->errorFormatter = $errorFormatter; - - return $this; - } - - /** - * @param \Exception|\Error $exception - * @param string $errorLevel - */ - public function logException($exception, $errorLevel = LogLevel::ERROR) - { - $message = sprintf( - '%s: %s[%d] (caught exception) at %s line %s.', - get_class($exception), - $exception->getMessage(), - $exception->getCode(), - $exception->getFile(), - $exception->getLine() - ); - - $this->logger->$errorLevel($message, ['exception' => $exception]); - } - - public function handleErrors(ExecutionResult $executionResult, $throwRawException = false) + public function handleErrors(ExecutionResult $executionResult, $throwRawException = false, $debug = false) { - $errorFormatter = $this->errorFormatter ? $this->errorFormatter : self::DEFAULT_ERROR_FORMATTER; + $errorFormatter = $this->createErrorFormatter($debug); $executionResult->setErrorFormatter($errorFormatter); - FormattedError::setInternalErrorMessage($this->internalErrorMessage); $exceptions = $this->treatExceptions($executionResult->errors, $throwRawException); $executionResult->errors = $exceptions['errors']; if (!empty($exceptions['extensions']['warnings'])) { @@ -105,6 +76,21 @@ public function handleErrors(ExecutionResult $executionResult, $throwRawExceptio } } + private function createErrorFormatter($debug = false) + { + $debugMode = false; + if ($debug) { + $debugMode = Debug::INCLUDE_TRACE | Debug::INCLUDE_DEBUG_MESSAGE; + } + + return function (GraphQLError $error) use ($debugMode) { + $event = new ErrorFormattingEvent($error, FormattedError::createFromException($error, $debugMode, $this->internalErrorMessage)); + $this->dispatcher->dispatch(Events::ERROR_FORMATTING, $event); + + return $event->getFormattedError()->getArrayCopy(); + }; + } + /** * @param GraphQLError[] $errors * @param bool $throwRawException @@ -145,18 +131,12 @@ private function treatExceptions(array $errors, $throwRawException) // user error if ($rawException instanceof $this->userErrorClass) { $treatedExceptions['errors'][] = $errorWithConvertedException; - if ($rawException->getPrevious()) { - $this->logException($rawException->getPrevious()); - } continue; } // user warning if ($rawException instanceof $this->userWarningClass) { $treatedExceptions['extensions']['warnings'][] = $errorWithConvertedException; - if ($rawException->getPrevious()) { - $this->logException($rawException->getPrevious(), LogLevel::WARNING); - } continue; } @@ -165,8 +145,6 @@ private function treatExceptions(array $errors, $throwRawException) throw $rawException; } - $this->logException($rawException, LogLevel::CRITICAL); - $treatedExceptions['errors'][] = $errorWithConvertedException; } @@ -208,8 +186,8 @@ private function flattenErrors(array $errors) */ private function convertException($rawException = null) { - if (null === $rawException) { - return; + if (null === $rawException || $rawException instanceof ClientAware) { + return $rawException; } $errorClass = $this->findErrorClass($rawException); diff --git a/Event/ErrorFormattingEvent.php b/Event/ErrorFormattingEvent.php new file mode 100644 index 000000000..bdddc574c --- /dev/null +++ b/Event/ErrorFormattingEvent.php @@ -0,0 +1,34 @@ +error = $error; + $this->formattedError = new \ArrayObject($formattedError); + } + + public function getError() + { + return $this->error; + } + + /** + * @return \ArrayObject + */ + public function getFormattedError() + { + return $this->formattedError; + } +} diff --git a/Event/Events.php b/Event/Events.php index 102ff3fde..095a73f65 100644 --- a/Event/Events.php +++ b/Event/Events.php @@ -7,4 +7,5 @@ final class Events const EXECUTOR_CONTEXT = 'graphql.executor.context'; const PRE_EXECUTOR = 'graphql.pre_executor'; const POST_EXECUTOR = 'graphql.post_executor'; + const ERROR_FORMATTING = 'graphql.error_formatting'; } diff --git a/Event/ExecutorContextEvent.php b/Event/ExecutorContextEvent.php index 53e871a6d..86e111166 100644 --- a/Event/ExecutorContextEvent.php +++ b/Event/ExecutorContextEvent.php @@ -4,7 +4,7 @@ use Symfony\Component\EventDispatcher\Event; -class ExecutorContextEvent extends Event +final class ExecutorContextEvent extends Event { /** @var \ArrayObject */ private $executorContext; diff --git a/Event/ExecutorEvent.php b/Event/ExecutorEvent.php index 30165c28c..95978b77e 100644 --- a/Event/ExecutorEvent.php +++ b/Event/ExecutorEvent.php @@ -5,7 +5,7 @@ use GraphQL\Type\Schema; use Symfony\Component\EventDispatcher\Event; -class ExecutorEvent extends Event +final class ExecutorEvent extends Event { /** @var Schema */ private $schema; diff --git a/Event/ExecutorResultEvent.php b/Event/ExecutorResultEvent.php index 1e27d5400..377fce487 100644 --- a/Event/ExecutorResultEvent.php +++ b/Event/ExecutorResultEvent.php @@ -5,7 +5,7 @@ use GraphQL\Executor\ExecutionResult; use Symfony\Component\EventDispatcher\Event; -class ExecutorResultEvent extends Event +final class ExecutorResultEvent extends Event { /** @var ExecutionResult */ private $result; diff --git a/EventListener/ClassLoaderListener.php b/EventListener/ClassLoaderListener.php index ee4db36e6..8d57a68c3 100644 --- a/EventListener/ClassLoaderListener.php +++ b/EventListener/ClassLoaderListener.php @@ -4,7 +4,7 @@ use Overblog\GraphQLBundle\Generator\TypeGenerator; -class ClassLoaderListener +final class ClassLoaderListener { private $typeGenerator; diff --git a/EventListener/DebugListener.php b/EventListener/DebugListener.php index 7fc0157b4..fa3fab846 100644 --- a/EventListener/DebugListener.php +++ b/EventListener/DebugListener.php @@ -4,7 +4,7 @@ use Overblog\GraphQLBundle\Event\ExecutorResultEvent; -class DebugListener +final class DebugListener { /** @var float */ private $startTime; diff --git a/EventListener/ErrorHandlerListener.php b/EventListener/ErrorHandlerListener.php index e5b52a0cd..a94005171 100644 --- a/EventListener/ErrorHandlerListener.php +++ b/EventListener/ErrorHandlerListener.php @@ -5,7 +5,7 @@ use Overblog\GraphQLBundle\Error\ErrorHandler; use Overblog\GraphQLBundle\Event\ExecutorResultEvent; -class ErrorHandlerListener +final class ErrorHandlerListener { /** @var ErrorHandler */ private $errorHandler; @@ -13,20 +13,19 @@ class ErrorHandlerListener /** @var bool */ private $throwException; - public function __construct(ErrorHandler $errorHandler, $throwException = false) - { - $this->errorHandler = $errorHandler; - $this->throwException = $throwException; - } + /** @var bool */ + private $debug; - public function setThrowException($throwException) + public function __construct(ErrorHandler $errorHandler, $throwException = false, $debug = false) { + $this->errorHandler = $errorHandler; $this->throwException = $throwException; + $this->debug = $debug; } public function onPostExecutor(ExecutorResultEvent $executorResultEvent) { $result = $executorResultEvent->getResult(); - $this->errorHandler->handleErrors($result, $this->throwException); + $this->errorHandler->handleErrors($result, $this->throwException, $this->debug); } } diff --git a/EventListener/ErrorLoggerListener.php b/EventListener/ErrorLoggerListener.php new file mode 100644 index 000000000..72e9f72f6 --- /dev/null +++ b/EventListener/ErrorLoggerListener.php @@ -0,0 +1,73 @@ +logger = null === $logger ? new NullLogger() : $logger; + $this->userErrorClass = $userErrorClass; + $this->userWarningClass = $userWarningClass; + } + + public function onErrorFormatting(ErrorFormattingEvent $event) + { + $error = $event->getError(); + if ($error->getPrevious()) { + $exception = $error->getPrevious(); + if ($exception->getPrevious()) { + if ($exception instanceof $this->userErrorClass) { + $this->logException($exception->getPrevious()); + + return; + } + + if ($exception instanceof $this->userWarningClass) { + $this->logException($exception->getPrevious(), LogLevel::WARNING); + + return; + } + } + $this->logException($error->getPrevious(), LogLevel::CRITICAL); + } + } + + /** + * @param \Throwable $exception + * @param string $errorLevel + */ + public function logException($exception, $errorLevel = LogLevel::ERROR) + { + $message = sprintf( + '%s: %s[%d] (caught exception) at %s line %s.', + get_class($exception), + $exception->getMessage(), + $exception->getCode(), + $exception->getFile(), + $exception->getLine() + ); + + $this->logger->$errorLevel($message, ['exception' => $exception]); + } +} diff --git a/EventListener/RequestFilesListener.php b/EventListener/RequestFilesListener.php index b3036abca..bb96c32d5 100644 --- a/EventListener/RequestFilesListener.php +++ b/EventListener/RequestFilesListener.php @@ -6,7 +6,7 @@ use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\RequestStack; -class RequestFilesListener +final class RequestFilesListener { /** @var RequestStack */ private $requestStack; diff --git a/README.md b/README.md index 6b149e9ab..b2985a827 100644 --- a/README.md +++ b/README.md @@ -54,7 +54,7 @@ Documentation - [Fields public control](Resources/doc/security/fields-public-control.md) - [Limiting query depth](Resources/doc/security/limiting-query-depth.md) - [Query complexity analysis](Resources/doc/security/query-complexity-analysis.md) - - [Errors handling](Resources/doc/security/errors-handling.md) +- [Errors handling](Resources/doc/error-handling/index.md) Talks and slides to help you start ---------------------------------- diff --git a/Resources/config/services.yml b/Resources/config/services.yml index 69b882a9a..dae45a3cf 100644 --- a/Resources/config/services.yml +++ b/Resources/config/services.yml @@ -2,24 +2,6 @@ parameters: overblog_graphql_types.config: [] services: - overblog_graphql.error_handler: - class: Overblog\GraphQLBundle\Error\ErrorHandler - public: false - arguments: - - ~ - - "@?logger" - - [] - - false - - Overblog\GraphQLBundle\EventListener\ErrorHandlerListener: - class: Overblog\GraphQLBundle\EventListener\ErrorHandlerListener - public: true - arguments: - - "@overblog_graphql.error_handler" - - "%kernel.debug%" - tags: - - { name: kernel.event_listener, event: graphql.post_executor, method: onPostExecutor } - overblog_graphql.executor.default: class: Overblog\GraphQLBundle\Executor\Executor public: false diff --git a/Resources/doc/security/errors-handling.md b/Resources/doc/error-handling/index.md similarity index 76% rename from Resources/doc/security/errors-handling.md rename to Resources/doc/error-handling/index.md index 0a78cc2ae..fd00b2750 100644 --- a/Resources/doc/security/errors-handling.md +++ b/Resources/doc/error-handling/index.md @@ -124,47 +124,4 @@ The message of those exceptions are then shown to the user like other Custom Error Formatting ------------------------- -It is possible to define custom formatter for result errors. -Let's say we want to add `code` entry to each error. - -```php -getCode(); - if ($e instanceof Error && $e->getPrevious()) { - $code = $e->getPrevious()->getCode(); - } - - $formattedError = FormattedError::createFromException($e); - $formattedError['code'] = $code; - - return $formattedError; - } -} -``` - -Change configuration to use custom `error_formatter`: - -```yaml -#app/config/config.yml -overblog_graphql: - definitions: - error_formatter: 'App\GraphQL\MyErrorFormatter::format' -``` - -**Note:** -- `error_formatter` is also impacting the formatting `extensions.warnings`. +TODO diff --git a/Resources/doc/security/index.md b/Resources/doc/security/index.md index 415a68a57..be362d504 100644 --- a/Resources/doc/security/index.md +++ b/Resources/doc/security/index.md @@ -6,4 +6,5 @@ Security * [Fields public control](fields-public-control.md) * [Limiting query depth](limiting-query-depth.md) * [Query complexity analysis](query-complexity-analysis.md) -* [Errors handling](errors-handling.md) + +Next step [handling errors](../error-handling/index.md) diff --git a/Tests/DependencyInjection/OverblogGraphQLTypesExtensionTest.php b/Tests/DependencyInjection/OverblogGraphQLTypesExtensionTest.php index 9665f7c51..f1522c28b 100644 --- a/Tests/DependencyInjection/OverblogGraphQLTypesExtensionTest.php +++ b/Tests/DependencyInjection/OverblogGraphQLTypesExtensionTest.php @@ -86,7 +86,7 @@ public function testCustomExceptions() $ext->load( [ [ - 'definitions' => [ + 'errors_handler' => [ 'exceptions' => [ 'warnings' => [ ResourceNotFoundException::class, diff --git a/Tests/Error/CustomErrorFormatter.php b/Tests/Error/CustomErrorFormatter.php deleted file mode 100644 index bd849146f..000000000 --- a/Tests/Error/CustomErrorFormatter.php +++ /dev/null @@ -1,37 +0,0 @@ -getCode(); - if ($e instanceof Error && $e->getPrevious()) { - $code = $e->getPrevious()->getCode(); - } - - $formattedError = FormattedError::createFromException($e); - $formattedError['code'] = $code; - - return $formattedError; - } - - /** - * @param \Throwable $e - * - * @return array - */ - public function __invoke($e) - { - return static::format($e); - } -} diff --git a/Tests/Error/ErrorHandlerTest.php b/Tests/Error/ErrorHandlerTest.php index 350f01bac..b374c06a6 100644 --- a/Tests/Error/ErrorHandlerTest.php +++ b/Tests/Error/ErrorHandlerTest.php @@ -10,15 +10,21 @@ use Overblog\GraphQLBundle\Error\UserErrors; use Overblog\GraphQLBundle\Error\UserWarning; use PHPUnit\Framework\TestCase; +use Symfony\Component\EventDispatcher\EventDispatcher; class ErrorHandlerTest extends TestCase { /** @var ErrorHandler */ private $errorHandler; + /** @var EventDispatcher|\PHPUnit_Framework_MockObject_MockObject */ + private $dispatcher; + public function setUp() { - $this->errorHandler = new ErrorHandler(); + $this->dispatcher = $this->getMockBuilder(EventDispatcher::class)->setMethods(['dispatch'])->getMock(); + $this->dispatcher->expects($this->any())->method('dispatch')->willReturnArgument(1); + $this->errorHandler = new ErrorHandler($this->dispatcher); } public function testMaskErrorWithThrowExceptionSetToFalse() @@ -130,6 +136,29 @@ public function testMaskErrorWithWrappedUserErrorAndThrowExceptionSetToTrue() $this->assertEquals($expected, $executionResult->toArray()); } + public function testDebugEnabled() + { + try { + throw new \Exception(); + } catch (\Exception $exception) { + $executionResult = new ExecutionResult( + null, + [ + new GraphQLError('Error wrapped exception', null, null, null, null, $exception), + ] + ); + + $this->errorHandler->handleErrors($executionResult, false, true); + + $errors = $executionResult->toArray()['errors']; + $this->assertCount(1, $errors); + $this->assertArrayHasKey('debugMessage', $errors[0]); + $this->assertEquals('Error wrapped exception', $errors[0]['debugMessage']); + $this->assertEquals(ErrorHandler::DEFAULT_ERROR_MESSAGE, $errors[0]['message']); + $this->assertArrayHasKey('trace', $errors[0]); + } + } + public function testMaskErrorWithoutWrappedExceptionAndThrowExceptionSetToTrue() { $executionResult = new ExecutionResult( @@ -155,7 +184,7 @@ public function testMaskErrorWithoutWrappedExceptionAndThrowExceptionSetToTrue() public function testConvertExceptionToUserWarning() { - $errorHandler = new ErrorHandler(null, null, [\InvalidArgumentException::class => UserWarning::class]); + $errorHandler = new ErrorHandler($this->dispatcher, null, [\InvalidArgumentException::class => UserWarning::class]); $executionResult = new ExecutionResult( null, @@ -189,7 +218,7 @@ public function testConvertExceptionToUserWarning() */ public function testConvertExceptionUsingParentExceptionMatchesAlwaysFirstExactExceptionOtherwiseMatchesParent(array $exceptionMap, $mapExceptionsToParent, $expectedUserError) { - $errorHandler = new ErrorHandler(null, null, $exceptionMap, $mapExceptionsToParent); + $errorHandler = new ErrorHandler($this->dispatcher, null, $exceptionMap, $mapExceptionsToParent); $executionResult = new ExecutionResult( null, @@ -215,29 +244,6 @@ public function testConvertExceptionUsingParentExceptionMatchesAlwaysFirstExactE } } - public function testCustomFormatterError() - { - $executionResult = new ExecutionResult( - null, - [ - new GraphQLError('Error with wrapped exception', null, null, null, null, new \Exception('My Exception message', 123)), - ] - ); - - $this->errorHandler->setErrorFormatter(new CustomErrorFormatter()); - - $this->errorHandler->handleErrors($executionResult); - $this->assertEquals([ - 'errors' => [ - [ - 'message' => 'Internal server Error', - 'category' => 'internal', - 'code' => 123, - ], - ], - ], $executionResult->toArray()); - } - /** * @return array */ diff --git a/Tests/Functional/App/config/config.yml b/Tests/Functional/App/config/config.yml index 932e6083c..f2e9aaa7c 100644 --- a/Tests/Functional/App/config/config.yml +++ b/Tests/Functional/App/config/config.yml @@ -9,6 +9,8 @@ framework: enabled: false overblog_graphql: + errors_handler: + debug: false definitions: config_validation: true auto_mapping: false @@ -22,6 +24,3 @@ services: #disable twig error pages twig.exception_listener: class: stdClass - error_handler_listener: - public: true - alias: Overblog\GraphQLBundle\EventListener\ErrorHandlerListener diff --git a/Tests/Functional/App/config/exception/config.yml b/Tests/Functional/App/config/exception/config.yml index d9d933d8a..fc1cd0533 100644 --- a/Tests/Functional/App/config/exception/config.yml +++ b/Tests/Functional/App/config/exception/config.yml @@ -3,12 +3,12 @@ imports: - { resource: services.yml } overblog_graphql: - definitions: - error_formatter: 'Overblog\GraphQLBundle\Tests\Error\CustomErrorFormatter::format' - class_namespace: 'Overblog\GraphQLBundle\Exception\__DEFINITIONS__' + errors_handler: exceptions: errors: - 'InvalidArgumentException' + definitions: + class_namespace: 'Overblog\GraphQLBundle\Exception\__DEFINITIONS__' schema: query: Query mutation: ~ diff --git a/Tests/Functional/App/config/mutation/config.yml b/Tests/Functional/App/config/mutation/config.yml index f098943c9..976d8c041 100644 --- a/Tests/Functional/App/config/mutation/config.yml +++ b/Tests/Functional/App/config/mutation/config.yml @@ -3,9 +3,10 @@ imports: - { resource: services.yml } overblog_graphql: + errors_handler: + internal_error_message: "Mutation has failed." definitions: class_namespace: "Overblog\\GraphQLBundle\\Mutation\\__DEFINITIONS__" - internal_error_message: "Mutation has failled." schema: query: RootMutation mutation: RootMutation diff --git a/Tests/Functional/Exception/ExceptionTest.php b/Tests/Functional/Exception/ExceptionTest.php index 2674fd0ee..5e60a6dfe 100644 --- a/Tests/Functional/Exception/ExceptionTest.php +++ b/Tests/Functional/Exception/ExceptionTest.php @@ -36,7 +36,6 @@ public function testExceptionIsMappedToAWarning() ], 'path' => ['test'], 'category' => 'user', - 'code' => 321, ], ]; diff --git a/Tests/Functional/TestCase.php b/Tests/Functional/TestCase.php index a8d28c5fa..183f95034 100644 --- a/Tests/Functional/TestCase.php +++ b/Tests/Functional/TestCase.php @@ -60,13 +60,12 @@ protected function tearDown() static::$kernel = null; } - protected static function executeGraphQLRequest($query, $rootValue = [], $throwException = false) + protected static function executeGraphQLRequest($query, $rootValue = []) { $request = new Request(); $request->query->set('query', $query); $req = static::getContainer()->get('overblog_graphql.request_parser')->parse($request); - static::getContainer()->get('error_handler_listener')->setThrowException($throwException); $res = static::getContainer()->get('overblog_graphql.request_executor')->execute(null, $req, $rootValue); return $res->toArray(); @@ -74,7 +73,7 @@ protected static function executeGraphQLRequest($query, $rootValue = [], $throwE protected static function assertGraphQL($query, array $expectedData = null, array $expectedErrors = null, $rootValue = []) { - $result = static::executeGraphQLRequest($query, $rootValue, true); + $result = static::executeGraphQLRequest($query, $rootValue/*, true*/); $expected = []; diff --git a/UPGRADE-0.11.md b/UPGRADE-0.11.md index fbeccf796..2438e1056 100644 --- a/UPGRADE-0.11.md +++ b/UPGRADE-0.11.md @@ -4,6 +4,7 @@ UPGRADE FROM 0.10 to 0.11 # Table of Contents - [GraphiQL](#graphiql) +- [Error Handler](#error-handler) ### GraphiQL @@ -40,3 +41,22 @@ UPGRADE FROM 0.10 to 0.11 ``` bin/console graphql:dump-schema --modern ``` + +### Errors Handler + + * Made errors handler more customizable + + Upgrading: + - Move `internal_error_message`, `map_exceptions_to_parent` and `exceptions` configurations + from `definitions` to new dedicated `error_handler` section. + ```diff + overblog_graphql: + definitions: + - internal_error_message: ~ + - map_exceptions_to_parent: ~ + - exceptions: ~ + + errors_handler: + + internal_error_message: ~ + + map_exceptions_to_parent: ~ + + exceptions: ~ + ``` From 349228016c280c568f0b8b1ddfbbbf7a1575c81e Mon Sep 17 00:00:00 2001 From: Jeremiah VALERIE Date: Mon, 25 Dec 2017 18:29:39 +0100 Subject: [PATCH 4/9] Remove useless configurations entries --- DependencyInjection/Configuration.php | 11 -------- .../OverblogGraphQLExtension.php | 22 ++++++---------- Error/ErrorHandler.php | 26 ++----------------- EventListener/ErrorLoggerListener.php | 19 ++++---------- Executor/Executor.php | 14 +++------- Executor/ExecutorInterface.php | 2 +- Request/Executor.php | 13 +++------- Tests/Request/ExecutorTest.php | 15 +++++++++-- UPGRADE-0.11.md | 23 ++++++++++++++++ composer.json | 1 + 10 files changed, 61 insertions(+), 85 deletions(-) diff --git a/DependencyInjection/Configuration.php b/DependencyInjection/Configuration.php index 4e0133374..25bee13e7 100644 --- a/DependencyInjection/Configuration.php +++ b/DependencyInjection/Configuration.php @@ -95,17 +95,6 @@ private function errorsHandlerSection() ->treatNullLike([]) ->prototype('scalar')->end() ->end() - ->arrayNode('types') - ->addDefaultsIfNotSet() - ->children() - ->scalarNode('warnings') - ->defaultValue(ErrorHandler::DEFAULT_USER_WARNING_CLASS) - ->end() - ->scalarNode('errors') - ->defaultValue(ErrorHandler::DEFAULT_USER_ERROR_CLASS) - ->end() - ->end() - ->end() ->end() ->end() ->end(); diff --git a/DependencyInjection/OverblogGraphQLExtension.php b/DependencyInjection/OverblogGraphQLExtension.php index 9d23079f1..f69cf556e 100644 --- a/DependencyInjection/OverblogGraphQLExtension.php +++ b/DependencyInjection/OverblogGraphQLExtension.php @@ -6,6 +6,8 @@ use Overblog\GraphQLBundle\CacheWarmer\CompileCacheWarmer; use Overblog\GraphQLBundle\Config\Processor\BuilderProcessor; use Overblog\GraphQLBundle\Error\ErrorHandler; +use Overblog\GraphQLBundle\Error\UserError; +use Overblog\GraphQLBundle\Error\UserWarning; use Overblog\GraphQLBundle\Event\Events; use Overblog\GraphQLBundle\EventListener\ClassLoaderListener; use Overblog\GraphQLBundle\EventListener\DebugListener; @@ -185,8 +187,6 @@ private function setErrorHandler(array $config, ContainerBuilder $container) $config['errors_handler']['map_exceptions_to_parent'], ] ) - ->addMethodCall('setUserWarningClass', [$config['errors_handler']['exceptions']['types']['warnings']]) - ->addMethodCall('setUserErrorClass', [$config['errors_handler']['exceptions']['types']['errors']]) ; $errorHandlerListenerDefinition = $container->setDefinition(ErrorHandlerListener::class, new Definition(ErrorHandlerListener::class)); @@ -200,12 +200,7 @@ private function setErrorHandler(array $config, ContainerBuilder $container) $invalidBehavior = ErrorLoggerListener::DEFAULT_LOGGER_SERVICE === $loggerServiceId ? ContainerInterface::NULL_ON_INVALID_REFERENCE : ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE; $errorHandlerListenerDefinition = $container->setDefinition(ErrorLoggerListener::class, new Definition(ErrorLoggerListener::class)); $errorHandlerListenerDefinition->setPublic(true) - ->setArguments([ - new Reference($loggerServiceId, $invalidBehavior), - $config['errors_handler']['exceptions']['types']['errors'], - $config['errors_handler']['exceptions']['types']['warnings'], - ] - ) + ->setArguments([new Reference($loggerServiceId, $invalidBehavior)]) ->addTag('kernel.event_listener', ['event' => Events::ERROR_FORMATTING, 'method' => 'onErrorFormatting']) ; } @@ -256,15 +251,14 @@ private function setServicesAliases(array $config, ContainerBuilder $container) private function buildExceptionMap(array $exceptionConfig) { $exceptionMap = []; - $typeMap = $exceptionConfig['types']; + $errorsMapping = [ + 'errors' => UserError::class, + 'warnings' => UserWarning::class, + ]; foreach ($exceptionConfig as $type => $exceptionList) { - if ('types' === $type) { - continue; - } - foreach ($exceptionList as $exception) { - $exceptionMap[$exception] = $typeMap[$type]; + $exceptionMap[$exception] = $errorsMapping[$type]; } } diff --git a/Error/ErrorHandler.php b/Error/ErrorHandler.php index e6fdd3145..b7e185fdd 100644 --- a/Error/ErrorHandler.php +++ b/Error/ErrorHandler.php @@ -15,8 +15,6 @@ class ErrorHandler { const DEFAULT_ERROR_MESSAGE = 'Internal server Error'; - const DEFAULT_USER_WARNING_CLASS = UserWarning::class; - const DEFAULT_USER_ERROR_CLASS = UserError::class; /** @var EventDispatcherInterface */ private $dispatcher; @@ -27,12 +25,6 @@ class ErrorHandler /** @var array */ private $exceptionMap; - /** @var string */ - private $userWarningClass = self::DEFAULT_USER_WARNING_CLASS; - - /** @var string */ - private $userErrorClass = self::DEFAULT_USER_ERROR_CLASS; - /** @var bool */ private $mapExceptionsToParent; @@ -51,20 +43,6 @@ public function __construct( $this->mapExceptionsToParent = $mapExceptionsToParent; } - public function setUserWarningClass($userWarningClass) - { - $this->userWarningClass = $userWarningClass; - - return $this; - } - - public function setUserErrorClass($userErrorClass) - { - $this->userErrorClass = $userErrorClass; - - return $this; - } - public function handleErrors(ExecutionResult $executionResult, $throwRawException = false, $debug = false) { $errorFormatter = $this->createErrorFormatter($debug); @@ -129,13 +107,13 @@ private function treatExceptions(array $errors, $throwRawException) ); // user error - if ($rawException instanceof $this->userErrorClass) { + if ($rawException instanceof UserError) { $treatedExceptions['errors'][] = $errorWithConvertedException; continue; } // user warning - if ($rawException instanceof $this->userWarningClass) { + if ($rawException instanceof UserWarning) { $treatedExceptions['extensions']['warnings'][] = $errorWithConvertedException; continue; } diff --git a/EventListener/ErrorLoggerListener.php b/EventListener/ErrorLoggerListener.php index 72e9f72f6..7d81fa413 100644 --- a/EventListener/ErrorLoggerListener.php +++ b/EventListener/ErrorLoggerListener.php @@ -2,7 +2,8 @@ namespace Overblog\GraphQLBundle\EventListener; -use Overblog\GraphQLBundle\Error\ErrorHandler; +use Overblog\GraphQLBundle\Error\UserError; +use Overblog\GraphQLBundle\Error\UserWarning; use Overblog\GraphQLBundle\Event\ErrorFormattingEvent; use Psr\Log\LoggerInterface; use Psr\Log\LogLevel; @@ -15,20 +16,10 @@ final class ErrorLoggerListener /** @var LoggerInterface */ private $logger; - /** @var string */ - private $userErrorClass; - - /** @var string */ - private $userWarningClass; - public function __construct( - LoggerInterface $logger = null, - $userErrorClass = ErrorHandler::DEFAULT_USER_ERROR_CLASS, - $userWarningClass = ErrorHandler::DEFAULT_USER_WARNING_CLASS + LoggerInterface $logger = null ) { $this->logger = null === $logger ? new NullLogger() : $logger; - $this->userErrorClass = $userErrorClass; - $this->userWarningClass = $userWarningClass; } public function onErrorFormatting(ErrorFormattingEvent $event) @@ -37,13 +28,13 @@ public function onErrorFormatting(ErrorFormattingEvent $event) if ($error->getPrevious()) { $exception = $error->getPrevious(); if ($exception->getPrevious()) { - if ($exception instanceof $this->userErrorClass) { + if ($exception instanceof UserError) { $this->logException($exception->getPrevious()); return; } - if ($exception instanceof $this->userWarningClass) { + if ($exception instanceof UserWarning) { $this->logException($exception->getPrevious(), LogLevel::WARNING); return; diff --git a/Executor/Executor.php b/Executor/Executor.php index c839f64c9..3495d7707 100644 --- a/Executor/Executor.php +++ b/Executor/Executor.php @@ -17,21 +17,15 @@ class Executor implements ExecutorInterface public function execute(Schema $schema, $requestString, $rootValue = null, $contextValue = null, $variableValues = null, $operationName = null) { $args = func_get_args(); + array_unshift($args, $this->promiseAdapter); - if (null === $this->promiseAdapter) { - $method = 'executeQuery'; - } else { - array_unshift($args, $this->promiseAdapter); - $method = 'promiseToExecute'; - } - - return call_user_func_array(sprintf('\%s::%s', GraphQL::class, $method), $args); + return call_user_func_array([GraphQL::class, 'promiseToExecute'], $args); } /** * {@inheritdoc} */ - public function setPromiseAdapter(PromiseAdapter $promiseAdapter = null) + public function setPromiseAdapter(PromiseAdapter $promiseAdapter) { $this->promiseAdapter = $promiseAdapter; } @@ -41,6 +35,6 @@ public function setPromiseAdapter(PromiseAdapter $promiseAdapter = null) */ public function setDefaultFieldResolver(callable $fn) { - call_user_func_array(sprintf('\%s::setDefaultFieldResolver', GraphQL::class), func_get_args()); + call_user_func_array([GraphQL::class, 'setDefaultFieldResolver'], func_get_args()); } } diff --git a/Executor/ExecutorInterface.php b/Executor/ExecutorInterface.php index c152029a0..1e9da510e 100644 --- a/Executor/ExecutorInterface.php +++ b/Executor/ExecutorInterface.php @@ -24,7 +24,7 @@ public function execute(Schema $schema, $requestString, $rootValue = null, $cont /** * @param PromiseAdapter|null $promiseAdapter */ - public function setPromiseAdapter(PromiseAdapter $promiseAdapter = null); + public function setPromiseAdapter(PromiseAdapter $promiseAdapter); /** * @param callable $fn diff --git a/Request/Executor.php b/Request/Executor.php index 9f06c96c6..e489ba19d 100644 --- a/Request/Executor.php +++ b/Request/Executor.php @@ -40,7 +40,7 @@ class Executor public function __construct( ExecutorInterface $executor, EventDispatcherInterface $dispatcher, - PromiseAdapter $promiseAdapter = null, + PromiseAdapter $promiseAdapter, callable $defaultFieldResolver = null ) { $this->executor = $executor; @@ -56,7 +56,7 @@ public function setExecutor(ExecutorInterface $executor) return $this; } - public function setPromiseAdapter(PromiseAdapter $promiseAdapter = null) + public function setPromiseAdapter(PromiseAdapter $promiseAdapter) { $this->promiseAdapter = $promiseAdapter; @@ -185,18 +185,13 @@ private function preExecute( */ private function postExecute($result) { - if ($this->promiseAdapter) { + if ($result instanceof Promise) { $result = $this->promiseAdapter->wait($result); } $this->checkExecutionResult($result); - $event = $this->dispatcher->dispatch( - Events::POST_EXECUTOR, - new ExecutorResultEvent($result) - ); - - return $event->getResult(); + return $this->dispatcher->dispatch(Events::POST_EXECUTOR, new ExecutorResultEvent($result))->getResult(); } private function checkPromiseAdapter() diff --git a/Tests/Request/ExecutorTest.php b/Tests/Request/ExecutorTest.php index 8ad903f0b..42d719be1 100644 --- a/Tests/Request/ExecutorTest.php +++ b/Tests/Request/ExecutorTest.php @@ -3,6 +3,7 @@ namespace Overblog\GraphQLBundle\Tests\Request; use GraphQL\Executor\Promise\Adapter\ReactPromiseAdapter; +use GraphQL\Executor\Promise\PromiseAdapter; use GraphQL\Type\Definition\ObjectType; use GraphQL\Type\Definition\Type; use GraphQL\Type\Schema; @@ -26,7 +27,7 @@ public function setUp() $this->dispatcher = $this->getMockBuilder(EventDispatcher::class)->setMethods(['dispatch'])->getMock(); $this->dispatcher->expects($this->any())->method('dispatch')->willReturnArgument(1); - $this->executor = new RequestExecutor(new Executor(), $this->dispatcher); + $this->executor = $this->createRequestExecutor(); $queryType = new ObjectType([ 'name' => 'Query', 'fields' => [ @@ -77,7 +78,7 @@ public function testInvalidExecutorAdapterPromise() */ public function testGetSchemaNoSchemaFound() { - (new RequestExecutor(new Executor(), $this->dispatcher))->getSchema('fake'); + $this->createRequestExecutor()->getSchema('fake'); } private function createExecutorExecuteMock($returnValue) @@ -90,4 +91,14 @@ private function createExecutorExecuteMock($returnValue) return $mock; } + + private function createRequestExecutor() + { + /** @var PromiseAdapter|\PHPUnit_Framework_MockObject_MockObject $promiseAdapter */ + $promiseAdapter = $this->getMockBuilder(PromiseAdapter::class) + ->setMethods(array_merge(['wait'], get_class_methods(PromiseAdapter::class))) + ->getMock(); + + return new RequestExecutor(new Executor(), $this->dispatcher, $promiseAdapter); + } } diff --git a/UPGRADE-0.11.md b/UPGRADE-0.11.md index 2438e1056..51507c54b 100644 --- a/UPGRADE-0.11.md +++ b/UPGRADE-0.11.md @@ -5,6 +5,7 @@ UPGRADE FROM 0.10 to 0.11 - [GraphiQL](#graphiql) - [Error Handler](#error-handler) +- [Promise adapter interface](#promise-adapter-interface) ### GraphiQL @@ -47,6 +48,15 @@ UPGRADE FROM 0.10 to 0.11 * Made errors handler more customizable Upgrading: + - Delete configuration to override base user exception classes. + ```diff + overblog_graphql: + definitions: + - exceptions: + - types: + - warnings: ~ + - errors: ~ + ``` - Move `internal_error_message`, `map_exceptions_to_parent` and `exceptions` configurations from `definitions` to new dedicated `error_handler` section. ```diff @@ -60,3 +70,16 @@ UPGRADE FROM 0.10 to 0.11 + map_exceptions_to_parent: ~ + exceptions: ~ ``` + + +### Promise adapter interface + + * Changed the promise adapter interface (`Overblog\GraphQLBundle\Executor\ExecutorInterface`) + as the promiseAdapter is not nullable in the bundle context. + + Upgrading: + - `setPromiseAdapter` method no more nullable. + ```diff + - public function setPromiseAdapter(PromiseAdapter $promiseAdapter = null); + + public function setPromiseAdapter(PromiseAdapter $promiseAdapter); + ``` diff --git a/composer.json b/composer.json index a3b3107b3..7fb4d336d 100644 --- a/composer.json +++ b/composer.json @@ -49,6 +49,7 @@ }, "require-dev": { "phpunit/phpunit": "^5.5 || ^6.0", + "psr/log": "^1.0", "react/promise": "^2.5", "sensio/framework-extra-bundle": "^3.0", "symfony/asset": "^3.1 || ^4.0", From fd63801d71b6c26e223240c7b79d98ea16ec1300 Mon Sep 17 00:00:00 2001 From: Jeremiah VALERIE Date: Tue, 26 Dec 2017 16:39:31 +0100 Subject: [PATCH 5/9] Improve tests coverage for ErrorLoggerListener --- .../OverblogGraphQLExtension.php | 2 +- Error/ErrorHandler.php | 4 +- Error/UserError.php | 4 +- Error/UserErrors.php | 2 +- Error/UserFacingError.php | 24 ------ Error/UserWarning.php | 19 ++++- EventListener/ErrorLoggerListener.php | 51 ++++++++----- .../OverblogGraphQLTypesExtensionTest.php | 2 +- Tests/Error/ErrorHandlerTest.php | 10 +-- .../EventListener/ErrorLoggerListenerTest.php | 74 +++++++++++++++++++ UPGRADE-0.11.md | 5 +- 11 files changed, 140 insertions(+), 57 deletions(-) delete mode 100644 Error/UserFacingError.php create mode 100644 Tests/EventListener/ErrorLoggerListenerTest.php diff --git a/DependencyInjection/OverblogGraphQLExtension.php b/DependencyInjection/OverblogGraphQLExtension.php index f69cf556e..364be8854 100644 --- a/DependencyInjection/OverblogGraphQLExtension.php +++ b/DependencyInjection/OverblogGraphQLExtension.php @@ -2,11 +2,11 @@ namespace Overblog\GraphQLBundle\DependencyInjection; +use GraphQL\Error\UserError; use GraphQL\Type\Schema; use Overblog\GraphQLBundle\CacheWarmer\CompileCacheWarmer; use Overblog\GraphQLBundle\Config\Processor\BuilderProcessor; use Overblog\GraphQLBundle\Error\ErrorHandler; -use Overblog\GraphQLBundle\Error\UserError; use Overblog\GraphQLBundle\Error\UserWarning; use Overblog\GraphQLBundle\Event\Events; use Overblog\GraphQLBundle\EventListener\ClassLoaderListener; diff --git a/Error/ErrorHandler.php b/Error/ErrorHandler.php index b7e185fdd..a8e40f88d 100644 --- a/Error/ErrorHandler.php +++ b/Error/ErrorHandler.php @@ -91,7 +91,7 @@ private function treatExceptions(array $errors, $throwRawException) $rawException = $this->convertException($error->getPrevious()); // raw GraphQL Error or InvariantViolation exception - if (null === $rawException || $rawException instanceof GraphQLUserError) { + if (null === $rawException) { $treatedExceptions['errors'][] = $error; continue; } @@ -107,7 +107,7 @@ private function treatExceptions(array $errors, $throwRawException) ); // user error - if ($rawException instanceof UserError) { + if ($rawException instanceof GraphQLUserError) { $treatedExceptions['errors'][] = $errorWithConvertedException; continue; } diff --git a/Error/UserError.php b/Error/UserError.php index 7d5af861a..521d682d8 100644 --- a/Error/UserError.php +++ b/Error/UserError.php @@ -2,9 +2,11 @@ namespace Overblog\GraphQLBundle\Error; +use GraphQL\Error\UserError as BaseUserError; + /** * Class UserError. */ -class UserError extends UserFacingError +class UserError extends BaseUserError { } diff --git a/Error/UserErrors.php b/Error/UserErrors.php index 2268db060..31d94d309 100644 --- a/Error/UserErrors.php +++ b/Error/UserErrors.php @@ -5,7 +5,7 @@ /** * Class UserErrors. */ -class UserErrors extends UserFacingError +class UserErrors extends \RuntimeException { /** @var UserError[] */ private $errors = []; diff --git a/Error/UserFacingError.php b/Error/UserFacingError.php deleted file mode 100644 index 4129218d0..000000000 --- a/Error/UserFacingError.php +++ /dev/null @@ -1,24 +0,0 @@ -getError(); + if ($error->getPrevious()) { $exception = $error->getPrevious(); - if ($exception->getPrevious()) { - if ($exception instanceof UserError) { - $this->logException($exception->getPrevious()); - - return; + if ($exception instanceof UserError) { + if ($exception->getPrevious()) { + $this->log($exception->getPrevious()); } - if ($exception instanceof UserWarning) { - $this->logException($exception->getPrevious(), LogLevel::WARNING); + return; + } - return; + if ($exception instanceof UserWarning) { + if ($exception->getPrevious()) { + $this->log($exception->getPrevious(), LogLevel::WARNING); } + + return; } - $this->logException($error->getPrevious(), LogLevel::CRITICAL); + $this->log($error->getPrevious(), LogLevel::CRITICAL); } } /** - * @param \Throwable $exception + * @param \Throwable $throwable * @param string $errorLevel */ - public function logException($exception, $errorLevel = LogLevel::ERROR) + public function log($throwable, $errorLevel = LogLevel::ERROR) + { + $this->logger->$errorLevel(self::serializeThrowableObject($throwable), ['throwable' => $throwable]); + } + + /** + * @param \Throwable $throwable + * + * @return string + */ + private static function serializeThrowableObject($throwable) { $message = sprintf( - '%s: %s[%d] (caught exception) at %s line %s.', - get_class($exception), - $exception->getMessage(), - $exception->getCode(), - $exception->getFile(), - $exception->getLine() + '[GraphQL] %s: %s[%d] (caught throwable) at %s line %s.', + get_class($throwable), + $throwable->getMessage(), + $throwable->getCode(), + $throwable->getFile(), + $throwable->getLine() ); - $this->logger->$errorLevel($message, ['exception' => $exception]); + return $message; } } diff --git a/Tests/DependencyInjection/OverblogGraphQLTypesExtensionTest.php b/Tests/DependencyInjection/OverblogGraphQLTypesExtensionTest.php index f1522c28b..59d260168 100644 --- a/Tests/DependencyInjection/OverblogGraphQLTypesExtensionTest.php +++ b/Tests/DependencyInjection/OverblogGraphQLTypesExtensionTest.php @@ -2,10 +2,10 @@ namespace Overblog\GraphQLBundle\Tests\DependencyInjection; +use GraphQL\Error\UserError; use Overblog\GraphQLBundle\Config\Processor\InheritanceProcessor; use Overblog\GraphQLBundle\DependencyInjection\OverblogGraphQLExtension; use Overblog\GraphQLBundle\DependencyInjection\OverblogGraphQLTypesExtension; -use Overblog\GraphQLBundle\Error\UserError; use Overblog\GraphQLBundle\Error\UserWarning; use Overblog\GraphQLBundle\Tests\DependencyInjection\Builder\PagerArgs; use Overblog\GraphQLBundle\Tests\DependencyInjection\Builder\RawIdField; diff --git a/Tests/Error/ErrorHandlerTest.php b/Tests/Error/ErrorHandlerTest.php index b374c06a6..818d6c818 100644 --- a/Tests/Error/ErrorHandlerTest.php +++ b/Tests/Error/ErrorHandlerTest.php @@ -35,9 +35,9 @@ public function testMaskErrorWithThrowExceptionSetToFalse() new GraphQLError('Error without wrapped exception'), new GraphQLError('Error with wrapped exception', null, null, null, null, new \Exception('My Exception message')), new GraphQLError('Error with wrapped user error', null, null, null, null, new UserError('My User Error')), + new GraphQLError('Error with wrapped base user error', null, null, null, null, new GraphQLUserError('My bases User Error')), new GraphQLError('', null, null, null, null, new UserErrors(['My User Error 1', 'My User Error 2', new UserError('My User Error 3')])), new GraphQLError('Error with wrapped user warning', null, null, null, null, new UserWarning('My User Warning')), - new GraphQLError('Invalid value!', null, null, null, null, new GraphQLUserError('Invalid value!')), ] ); @@ -58,19 +58,19 @@ public function testMaskErrorWithThrowExceptionSetToFalse() 'category' => 'user', ], [ - 'message' => 'My User Error 1', + 'message' => 'Error with wrapped base user error', 'category' => 'user', ], [ - 'message' => 'My User Error 2', + 'message' => 'My User Error 1', 'category' => 'user', ], [ - 'message' => 'My User Error 3', + 'message' => 'My User Error 2', 'category' => 'user', ], [ - 'message' => 'Invalid value!', + 'message' => 'My User Error 3', 'category' => 'user', ], ], diff --git a/Tests/EventListener/ErrorLoggerListenerTest.php b/Tests/EventListener/ErrorLoggerListenerTest.php new file mode 100644 index 000000000..4dcc76bc9 --- /dev/null +++ b/Tests/EventListener/ErrorLoggerListenerTest.php @@ -0,0 +1,74 @@ +logger = $this->getMockBuilder(LoggerInterface::class)->getMock(); + $this->listener = new ErrorLoggerListener($this->logger); + } + + /** + * @param Error $error + * @param Invocation $loggerExpects + * @param Constraint|string $loggerMethod + * @param array|null $with + * + * @dataProvider fixtures + */ + public function testOnErrorFormatting(Error $error, $loggerExpects, $loggerMethod, array $with = null) + { + $mock = $this->logger->expects($loggerExpects)->method($loggerMethod); + if ($with) { + $mock->with(...$with); + } + + $this->listener->onErrorFormatting(new ErrorFormattingEvent($error, [])); + } + + public function fixtures() + { + try { + throw new \Exception('Ko!'); + } catch (\Exception $exception) { + } + $with = [ + sprintf('[GraphQL] %s: %s[%d] (caught throwable) at %s line %s.', \Exception::class, 'Ko!', 0, __FILE__, $exception->getLine()), + ['throwable' => $exception], + ]; + + return [ + [self::createError('Basic error'), $this->never(), $this->anything()], + [self::createError('Wrapped Base UserError without previous', new \GraphQL\Error\UserError('User error message')), $this->never(), $this->anything()], + [self::createError('Wrapped UserError without previous', new UserError('User error message')), $this->never(), $this->anything()], + [self::createError('Wrapped UserWarning without previous', new UserWarning('User warning message')), $this->never(), $this->anything()], + [self::createError('Wrapped unknown exception', $exception), $this->once(), 'critical', $with], + [self::createError('Wrapped Base UserError with previous', new \GraphQL\Error\UserError('User error message', 0, $exception)), $this->once(), 'error', $with], + [self::createError('Wrapped UserError with previous', new UserError('User error message', 0, $exception)), $this->once(), 'error', $with], + [self::createError('Wrapped UserWarning with previous', new UserWarning('User warning message', 0, $exception)), $this->once(), 'warning', $with], + ]; + } + + private static function createError($message, \Exception $exception = null) + { + return new Error($message, null, null, null, null, $exception); + } +} diff --git a/UPGRADE-0.11.md b/UPGRADE-0.11.md index 51507c54b..9103d5a8c 100644 --- a/UPGRADE-0.11.md +++ b/UPGRADE-0.11.md @@ -4,7 +4,7 @@ UPGRADE FROM 0.10 to 0.11 # Table of Contents - [GraphiQL](#graphiql) -- [Error Handler](#error-handler) +- [Errors Handler](#errors-handler) - [Promise adapter interface](#promise-adapter-interface) ### GraphiQL @@ -48,11 +48,12 @@ UPGRADE FROM 0.10 to 0.11 * Made errors handler more customizable Upgrading: + - User - Delete configuration to override base user exception classes. ```diff overblog_graphql: definitions: - - exceptions: + exceptions: - types: - warnings: ~ - errors: ~ From c8ad8ea4d745e90e584530ee5218e20cbad0c76d Mon Sep 17 00:00:00 2001 From: Jeremiah VALERIE Date: Sat, 30 Dec 2017 11:08:40 +0100 Subject: [PATCH 6/9] Refactor ExecutorEvent --- ...orEvent.php => ExecutorArgumentsEvent.php} | 64 ++++++++++++++++--- Request/Executor.php | 45 +++++-------- 2 files changed, 73 insertions(+), 36 deletions(-) rename Event/{ExecutorEvent.php => ExecutorArgumentsEvent.php} (53%) diff --git a/Event/ExecutorEvent.php b/Event/ExecutorArgumentsEvent.php similarity index 53% rename from Event/ExecutorEvent.php rename to Event/ExecutorArgumentsEvent.php index 95978b77e..dcc9ee4db 100644 --- a/Event/ExecutorEvent.php +++ b/Event/ExecutorArgumentsEvent.php @@ -5,7 +5,7 @@ use GraphQL\Type\Schema; use Symfony\Component\EventDispatcher\Event; -final class ExecutorEvent extends Event +final class ExecutorArgumentsEvent extends Event { /** @var Schema */ private $schema; @@ -13,7 +13,7 @@ final class ExecutorEvent extends Event /** @var string */ private $requestString; - /** @var \ArrayObject */ + /** @var mixed */ private $rootValue; /** @var \ArrayObject */ @@ -25,14 +25,62 @@ final class ExecutorEvent extends Event /** @var null|string */ private $operationName; - public function __construct(Schema $schema, $requestString, \ArrayObject $rootValue, \ArrayObject $contextValue, $variableValue = null, $operationName = null) + public static function create( + Schema $schema, + $requestString, + \ArrayObject $contextValue, + $rootValue = null, + array $variableValue = null, + $operationName = null + ) { + $instance = new static(); + $instance->setSchema($schema); + $instance->setRequestString($requestString); + $instance->setContextValue($contextValue); + $instance->setRootValue($rootValue); + $instance->setVariableValue($variableValue); + $instance->setOperationName($operationName); + + return $instance; + } + + /** + * @param null|string $operationName + */ + public function setOperationName($operationName = null) + { + $this->operationName = $operationName; + } + + public function setContextValue(\ArrayObject $contextValue = null) { - $this->schema = $schema; - $this->requestString = $requestString; - $this->rootValue = $rootValue; $this->contextValue = $contextValue; + } + + /** + * @param mixed $rootValue + */ + public function setRootValue($rootValue = null) + { + $this->rootValue = $rootValue; + } + + /** + * @param string $requestString + */ + public function setRequestString($requestString) + { + $this->requestString = $requestString; + } + + public function setVariableValue(array $variableValue = null) + { $this->variableValue = $variableValue; - $this->operationName = $operationName; + } + + public function setSchema(Schema $schema) + { + $this->schema = $schema; } /** @@ -52,7 +100,7 @@ public function getRequestString() } /** - * @return \ArrayObject + * @return array|null */ public function getRootValue() { diff --git a/Request/Executor.php b/Request/Executor.php index e489ba19d..fd88db1f2 100644 --- a/Request/Executor.php +++ b/Request/Executor.php @@ -10,8 +10,8 @@ use GraphQL\Validator\Rules\QueryComplexity; use GraphQL\Validator\Rules\QueryDepth; use Overblog\GraphQLBundle\Event\Events; +use Overblog\GraphQLBundle\Event\ExecutorArgumentsEvent; use Overblog\GraphQLBundle\Event\ExecutorContextEvent; -use Overblog\GraphQLBundle\Event\ExecutorEvent; use Overblog\GraphQLBundle\Event\ExecutorResultEvent; use Overblog\GraphQLBundle\Executor\ExecutorInterface; use Overblog\GraphQLBundle\Executor\Promise\PromiseAdapterInterface; @@ -117,28 +117,27 @@ public function setMaxQueryComplexity($maxQueryComplexity) * @param null|string $schemaName * @param array $request * @param null|array|\ArrayObject|object $rootValue - * @param null|array|\ArrayObject|object $contextValue * * @return ExecutionResult */ - public function execute($schemaName, array $request, $rootValue = null, $contextValue = null) + public function execute($schemaName, array $request, $rootValue = null) { - $executorEvent = $this->preExecute( + $executorArgumentsEvent = $this->preExecute( $this->getSchema($schemaName), isset($request[ParserInterface::PARAM_QUERY]) ? $request[ParserInterface::PARAM_QUERY] : null, - self::createArrayObject($rootValue), - self::createArrayObject($contextValue), + new \ArrayObject(), + $rootValue, $request[ParserInterface::PARAM_VARIABLES], isset($request[ParserInterface::PARAM_OPERATION_NAME]) ? $request[ParserInterface::PARAM_OPERATION_NAME] : null ); $result = $this->executor->execute( - $executorEvent->getSchema(), - $executorEvent->getRequestString(), - $executorEvent->getRootValue(), - $executorEvent->getContextValue(), - $executorEvent->getVariableValue(), - $executorEvent->getOperationName() + $executorArgumentsEvent->getSchema(), + $executorArgumentsEvent->getRequestString(), + $executorArgumentsEvent->getRootValue(), + $executorArgumentsEvent->getContextValue(), + $executorArgumentsEvent->getVariableValue(), + $executorArgumentsEvent->getOperationName() ); $result = $this->postExecute($result); @@ -149,17 +148,18 @@ public function execute($schemaName, array $request, $rootValue = null, $context /** * @param Schema $schema * @param string $requestString - * @param \ArrayObject $rootValue * @param \ArrayObject $contextValue + * @param mixed $rootValue * @param array|null $variableValue * @param string|null $operationName * - * @return ExecutorEvent + * @return ExecutorArgumentsEvent */ private function preExecute( - Schema $schema, $requestString, - \ArrayObject $rootValue, + Schema $schema, + $requestString, \ArrayObject $contextValue, + $rootValue = null, array $variableValue = null, $operationName = null ) { @@ -174,7 +174,7 @@ private function preExecute( return $this->dispatcher->dispatch( Events::PRE_EXECUTOR, - new ExecutorEvent($schema, $requestString, $rootValue, $contextValue, $variableValue, $operationName) + ExecutorArgumentsEvent::create($schema, $requestString, $contextValue, $rootValue, $variableValue, $operationName) ); } @@ -215,15 +215,4 @@ private function checkExecutionResult($result) ); } } - - private static function createArrayObject($data) - { - if (is_array($data) || is_object($data)) { - $object = new \ArrayObject($data); - } else { - $object = new \ArrayObject(); - } - - return $object; - } } From f1619202e044e2d97b2353ed8b78aa44dc8a3bbf Mon Sep 17 00:00:00 2001 From: Jeremiah VALERIE Date: Sat, 30 Dec 2017 11:15:38 +0100 Subject: [PATCH 7/9] Add events doc --- README.md | 1 + Resources/doc/events/index.md | 162 ++++++++++++++++++++++++++++++++++ 2 files changed, 163 insertions(+) create mode 100644 Resources/doc/events/index.md diff --git a/README.md b/README.md index b2985a827..7e29b42f4 100644 --- a/README.md +++ b/README.md @@ -55,6 +55,7 @@ Documentation - [Limiting query depth](Resources/doc/security/limiting-query-depth.md) - [Query complexity analysis](Resources/doc/security/query-complexity-analysis.md) - [Errors handling](Resources/doc/error-handling/index.md) +- [Events](Resources/doc/events/index.md) Talks and slides to help you start ---------------------------------- diff --git a/Resources/doc/events/index.md b/Resources/doc/events/index.md new file mode 100644 index 000000000..566931819 --- /dev/null +++ b/Resources/doc/events/index.md @@ -0,0 +1,162 @@ +Events +========= + +[Events](http://symfony.com/doc/master/event_dispatcher.html) are a way to hook +into the execution flow. This bundle provides various events from executor +and context value initialization, executor result formatting (data, errors, extensions), +errors/warnings formatting. + +With this in mind we now turn to explain each one of them. + +Executor context value +---------------------- + +*Event:* `graphql.executor.context` + +This event can be listen to initialize executor `contextValue` argument. + + +Executor initialisation +----------------------- + +*Event:* `graphql.pre_executor` + +This event can be listen to initialize executor execute arguments +(`schema`, `requestString`, `rootValue`, `contextValue`, `variableValue`, `operationName`). + +For example: + +* Initializing `rootValue` with the current user (we assume that user is fully authenticated) + + ```yaml + App\EventListener\RootValueInitialization: + tags: + - { name: kernel.event_listener, event: graphql.pre_executor, method: onPreExecutor } + ``` + + ```php + token = $token; + } + + public function onPreExecutor(ExecutorArgumentsEvent $event) + { + $event->setRootValue($this->token->getUser()); + } + } + +Executor result +--------------- + +*Event:* `graphql.post_executor` + +This event can be listen to set custom error handling and formatting, it can also be use to +add information to `extensions` section. + +For example: + +* How to add `credits` entry in `extensions` section + + ```yaml + App\EventListener\Credits: + tags: + - { name: kernel.event_listener, event: graphql.post_executor, method: onPostExecutor } + ``` + + ```php + getResult()->extensions['credits'] = 'This api was powered by "OverblogGraphQLBundle".'; + } + } + ``` + + result: + ```json + { + "data": {"foo": "bar"}, + "extensions": { + "credits": "This api was powered by \"OverblogGraphQLBundle\"." + } + } + ``` + +Error formatting +---------------- + +*Event:* `graphql.error_formatting` + +This event can be listen to add or remove fields from result `errors` and `extensions.warnings` +sections, it can also be use to log errors or exception. Each single error or warning will trigger +an event. + +For example: + +* How to add error code: + + ```yaml + App\EventListener\ErrorCode: + tags: + - { name: kernel.event_listener, event: graphql.error_formatting, method: onErrorFormatting } + ``` + + ```php + getError(); + if ($error->getPrevious()) { + $code = $error->getPrevious()->getCode(); + } else { + $code = $error->getCode(); + } + $formattedError = $event->getFormattedError(); + $formattedError->offsetSet('code', $code); // or $formattedError['code'] = $code; + } + } + ``` + + result: + ```json + { + "data": null, + "errors": [ + { + "code": 123, + "category": "internal", + "message": "Internal server Error" + } + ] + } + ``` + +*Note:* +- This event is dispatch by this bundle default error handler, that the reason why, disabling +error handler will also disable this event. From 2033fd5b6f99f353204066b75bfc2556e6048000 Mon Sep 17 00:00:00 2001 From: Jeremiah VALERIE Date: Sat, 30 Dec 2017 11:25:07 +0100 Subject: [PATCH 8/9] Allow UserErrors to accept base user error --- Error/UserErrors.php | 6 +++--- Tests/Error/ErrorHandlerTest.php | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Error/UserErrors.php b/Error/UserErrors.php index 31d94d309..1e87c5670 100644 --- a/Error/UserErrors.php +++ b/Error/UserErrors.php @@ -27,7 +27,7 @@ public function setErrors(array $errors) } /** - * @param string|UserError $error + * @param string|\GraphQL\Error\UserError $error * * @return $this */ @@ -35,8 +35,8 @@ public function addError($error) { if (is_string($error)) { $error = new UserError($error); - } elseif (!is_object($error) || !$error instanceof UserError) { - throw new \InvalidArgumentException(sprintf('Error must be string or instance of %s.', UserError::class)); + } elseif (!is_object($error) || !$error instanceof \GraphQL\Error\UserError) { + throw new \InvalidArgumentException(sprintf('Error must be string or instance of %s.', \GraphQL\Error\UserError::class)); } $this->errors[] = $error; diff --git a/Tests/Error/ErrorHandlerTest.php b/Tests/Error/ErrorHandlerTest.php index 818d6c818..87bbaf39c 100644 --- a/Tests/Error/ErrorHandlerTest.php +++ b/Tests/Error/ErrorHandlerTest.php @@ -105,7 +105,7 @@ public function testMaskErrorWithWrappedExceptionAndThrowExceptionSetToTrue() public function testInvalidUserErrorsItem() { $this->expectException(\InvalidArgumentException::class); - $this->expectExceptionMessage('Error must be string or instance of Overblog\GraphQLBundle\Error\UserError.'); + $this->expectExceptionMessage(sprintf('Error must be string or instance of %s', GraphQLUserError::class)); new UserErrors([ 'Some Error', From 9f468039880e1161f18d933517da4eeb852eeb4f Mon Sep 17 00:00:00 2001 From: Jeremiah VALERIE Date: Sat, 30 Dec 2017 11:47:42 +0100 Subject: [PATCH 9/9] Improve error handler documentation --- Resources/doc/error-handling/index.md | 74 ++++++++++++++++++++++----- 1 file changed, 60 insertions(+), 14 deletions(-) diff --git a/Resources/doc/error-handling/index.md b/Resources/doc/error-handling/index.md index fd00b2750..a6159fbba 100644 --- a/Resources/doc/error-handling/index.md +++ b/Resources/doc/error-handling/index.md @@ -1,7 +1,7 @@ Errors handling =============== -In no debug mode all errors will be logged and replace by a generic error message. +By default in no debug mode all errors will be logged and replace by a generic error message. Only query parsed error won't be replaced. If you want to send explicit error or warnings messages to your users you can use exceptions: @@ -89,21 +89,10 @@ class CharacterResolver Warnings can be found in the response under `extensions.warnings` map. -You can also custom the generic error message - -```yaml -#app/config/config.yml -overblog_graphql: - #... - definitions: - internal_error_message: "An error occurred, please retry later or contact us!" -``` - If you want to map your own exceptions to warnings and errors you can define a custom exception mapping: ```yaml -#app/config/config.yml overblog_graphql: #... definitions: @@ -118,10 +107,67 @@ overblog_graphql: - "InvalidArgumentException" ``` +You can custom the default errors handler using configuration: + +```yaml +overblog_graphql: + errors_handler: + enabled: true # false will totally disabled errors handling + internal_error_message: ~ # custom generic error message + rethrow_internal_exceptions: false # re-throw internal exception + debug: false # will add trace stack and debugMessage to error + log: true # false will disabled the default logging behavior + logger_service: logger # the service to use to log +``` + The message of those exceptions are then shown to the user like other `UserError`s or `UserWarning`s. -Custom Error Formatting +Custom error Formatting ------------------------- -TODO +see [error formatting Event](../events/index.md#error-formatting) + +Custom error handling / formatting +----------------------------------- + +This can also be done by using events. +* First totally disabled default errors handler: + ```yaml + overblog_graphql: + errors_handler: false + ``` +* Listen to [executor result event](../events/index.md#executor-result) + ```yaml + App\EventListener\MyErrorHandler: + tags: + - { name: kernel.event_listener, event: graphql.post_executor, method: onPostExecutor } + ``` + + ```php + getResult() + ->setErrorFormatter($myErrorFormatter) + ->setErrorsHandler($myErrorHandler); + } + } + ```