From 03629c1e3c657fa53151dbdc192c162bf8fd5877 Mon Sep 17 00:00:00 2001 From: Vladimir Razuvaev Date: Thu, 17 Aug 2017 16:42:28 +0700 Subject: [PATCH] Refactored error formatting (debugging part) --- src/Error/Debug.php | 10 ++ src/Error/FormattedError.php | 136 +++++++++++++++++----------- src/Executor/ExecutionResult.php | 18 +--- src/Server/Helper.php | 21 ++--- src/Server/ServerConfig.php | 48 ++++++---- tests/Server/QueryExecutionTest.php | 70 +++++++++++++- tests/Server/ServerConfigTest.php | 14 +++ tests/ServerTest.php | 4 +- 8 files changed, 225 insertions(+), 96 deletions(-) create mode 100644 src/Error/Debug.php diff --git a/src/Error/Debug.php b/src/Error/Debug.php new file mode 100644 index 0000000..221b58b --- /dev/null +++ b/src/Error/Debug.php @@ -0,0 +1,10 @@ + 0) { - if (!$e instanceof Error || $e->getPrevious()) { - throw $e; - } - } - - $debug = (int) $debug; $internalErrorMessage = $internalErrorMessage ?: self::$internalErrorMessage; if ($e instanceof ClientAware) { - if ($e->isClientSafe()) { - $result = [ - 'message' => $e->getMessage(), - 'category' => $e->getCategory() - ]; - } else { - $result = [ - 'message' => $internalErrorMessage, - 'category' => $e->getCategory() - ]; - } + $formattedError = [ + 'message' => $e->isClientSafe() ? $e->getMessage() : $internalErrorMessage, + 'category' => $e->getCategory() + ]; } else { - $result = [ + $formattedError = [ 'message' => $internalErrorMessage, 'category' => Error::CATEGORY_INTERNAL ]; } - if (($debug & self::INCLUDE_DEBUG_MESSAGE > 0) && $result['message'] === $internalErrorMessage) { - $result['debugMessage'] = $e->getMessage(); - } if ($e instanceof Error) { $locations = Utils::map($e->getLocations(), function(SourceLocation $loc) { @@ -81,48 +58,105 @@ class FormattedError }); if (!empty($locations)) { - $result['locations'] = $locations; + $formattedError['locations'] = $locations; } if (!empty($e->path)) { - $result['path'] = $e->path; - } - } else if ($e instanceof \ErrorException) { - if ($debug) { - $result += [ - 'file' => $e->getFile(), - 'line' => $e->getLine(), - 'severity' => $e->getSeverity() - ]; - } - } else if ($e instanceof \Error) { - if ($debug) { - $result += [ - 'file' => $e->getFile(), - 'line' => $e->getLine(), - ]; + $formattedError['path'] = $e->path; } } - if ($debug & self::INCLUDE_TRACE > 0) { + if ($debug) { + $formattedError = self::addDebugEntries($formattedError, $e, $debug); + } + + return $formattedError; + } + + /** + * @param array $formattedError + * @param \Throwable $e + * @param bool $debug + * @return array + * @throws \Throwable + */ + public static function addDebugEntries(array $formattedError, $e, $debug) + { + if (!$debug) { + return $formattedError; + } + + Utils::invariant( + $e instanceof \Exception || $e instanceof \Throwable, + "Expected exception, got %s", + Utils::getVariableType($e) + ); + + $debug = (int) $debug; + + if ($debug & Debug::RETHROW_INTERNAL_EXCEPTIONS) { + if (!$e instanceof Error) { + throw $e; + } else if ($e->getPrevious()) { + throw $e->getPrevious(); + } + } + + $isInternal = !$e instanceof ClientAware || !$e->isClientSafe(); + + if (($debug & Debug::INCLUDE_DEBUG_MESSAGE) && $isInternal) { + // Displaying debugMessage as a first entry: + $formattedError = ['debugMessage' => $e->getMessage()] + $formattedError; + } + + if ($debug & Debug::INCLUDE_TRACE) { + if ($e instanceof \ErrorException || $e instanceof \Error) { + $formattedError += [ + 'file' => $e->getFile(), + 'line' => $e->getLine(), + ]; + } + $isTrivial = $e instanceof Error && !$e->getPrevious(); if (!$isTrivial) { $debugging = $e->getPrevious() ?: $e; - $result['trace'] = static::toSafeTrace($debugging->getTrace()); + $formattedError['trace'] = static::toSafeTrace($debugging); } } + return $formattedError; + } - return $result; + /** + * Prepares final error formatter taking in account $debug flags. + * If initial formatter is not set, FormattedError::createFromException is used + * + * @param callable|null $formatter + * @param $debug + * @return callable|\Closure + */ + public static function prepareFormatter(callable $formatter = null, $debug) + { + $formatter = $formatter ?: function($e) { + return FormattedError::createFromException($e); + }; + if ($debug) { + $formatter = function($e) use ($formatter, $debug) { + return FormattedError::addDebugEntries($formatter($e), $e, $debug); + }; + } + return $formatter; } /** * Converts error trace to serializable array * - * @param array $trace + * @param \Throwable $error * @return array */ - private static function toSafeTrace(array $trace) + public static function toSafeTrace($error) { + $trace = $error->getTrace(); + // Remove invariant entries as they don't provide much value: if ( isset($trace[0]['function']) && isset($trace[0]['class']) && @@ -221,7 +255,7 @@ class FormattedError return [ 'message' => $e->getMessage(), 'severity' => $e->getSeverity(), - 'trace' => self::toSafeTrace($e->getTrace()) + 'trace' => self::toSafeTrace($e) ]; } } diff --git a/src/Executor/ExecutionResult.php b/src/Executor/ExecutionResult.php index b868b8f..73fc4e9 100644 --- a/src/Executor/ExecutionResult.php +++ b/src/Executor/ExecutionResult.php @@ -98,23 +98,13 @@ class ExecutionResult implements \JsonSerializable $result = []; if (!empty($this->errors)) { - if ($debug) { - $errorFormatter = function($e) use ($debug) { - return FormattedError::createFromException($e, $debug); - }; - } else if (!$this->errorFormatter) { - $errorFormatter = function($e) { - return FormattedError::createFromException($e, false); - }; - } else { - $errorFormatter = $this->errorFormatter; - } - $errorsHandler = $this->errorsHandler ?: function(array $errors, callable $formatter) { return array_map($formatter, $errors); }; - - $result['errors'] = $errorsHandler($this->errors, $errorFormatter); + $result['errors'] = $errorsHandler( + $this->errors, + FormattedError::prepareFormatter($this->errorFormatter, $debug) + ); } if (null !== $this->data) { diff --git a/src/Server/Helper.php b/src/Server/Helper.php index a3e032c..06d9cb4 100644 --- a/src/Server/Helper.php +++ b/src/Server/Helper.php @@ -141,21 +141,20 @@ class Helper ); } - $applyErrorFormatting = function (ExecutionResult $result) use ($config) { - if ($config->getDebug()) { - $errorFormatter = function($e) { - return FormattedError::createFromException($e, true); - }; - } else { - $errorFormatter = $config->getErrorFormatter() ?: function($e) { - return FormattedError::createFromException($e, false); - }; + $applyErrorHandling = function (ExecutionResult $result) use ($config) { + if ($config->getErrorsHandler()) { + $result->setErrorsHandler($config->getErrorsHandler()); + } + if ($config->getErrorFormatter() || $config->getDebug()) { + $result->setErrorFormatter( + FormattedError::prepareFormatter($config->getErrorFormatter(), + $config->getDebug()) + ); } - $result->setErrorFormatter($errorFormatter); return $result; }; - return $result->then($applyErrorFormatting); + return $result->then($applyErrorHandling); } /** diff --git a/src/Server/ServerConfig.php b/src/Server/ServerConfig.php index d211233..9bb0dd2 100644 --- a/src/Server/ServerConfig.php +++ b/src/Server/ServerConfig.php @@ -40,10 +40,15 @@ class ServerConfig private $rootValue; /** - * @var callable + * @var callable|null */ private $errorFormatter; + /** + * @var callable|null + */ + private $errorsHandler; + /** * @var bool */ @@ -131,7 +136,7 @@ class ServerConfig } /** - * @return callable + * @return callable|null */ public function getErrorFormatter() { @@ -150,6 +155,26 @@ class ServerConfig return $this; } + /** + * Expects function(array $errors, callable $formatter) : array + * + * @param callable $handler + * @return $this + */ + public function setErrorsHandler(callable $handler) + { + $this->errorsHandler = $handler; + return $this; + } + + /** + * @return callable|null + */ + public function getErrorsHandler() + { + return $this->errorsHandler; + } + /** * @return PromiseAdapter */ @@ -243,27 +268,14 @@ class ServerConfig } /** - * Settings this option has two effects: + * Set response debug flags, see GraphQL\Error\Debug class for a list of available flags * - * 1. Replaces current error formatter with the one for debugging (has precedence over `setErrorFormatter()`). - * This error formatter adds `trace` entry for all errors in ExecutionResult when it is converted to array. - * - * 2. All PHP errors are intercepted during query execution (including warnings, notices and deprecations). - * - * These PHP errors are converted to arrays with `message`, `file`, `line`, `trace` keys and then added to - * `extensions` section of ExecutionResult under key `phpErrors`. - * - * After query execution error handler will be removed from stack, - * so any errors occurring after execution will not be caught. - * - * Use this feature for development and debugging only. - * - * @param bool $set + * @param bool|int $set * @return $this */ public function setDebug($set = true) { - $this->debug = (bool) $set; + $this->debug = $set; return $this; } diff --git a/tests/Server/QueryExecutionTest.php b/tests/Server/QueryExecutionTest.php index 92ed101..a543327 100644 --- a/tests/Server/QueryExecutionTest.php +++ b/tests/Server/QueryExecutionTest.php @@ -2,6 +2,7 @@ namespace GraphQL\Tests\Server; use GraphQL\Deferred; +use GraphQL\Error\Debug; use GraphQL\Error\Error; use GraphQL\Error\InvariantViolation; use GraphQL\Error\UserError; @@ -136,7 +137,8 @@ class QueryExecutionTest extends \PHPUnit_Framework_TestCase public function testDebugExceptions() { - $this->config->setDebug(true); + $debug = Debug::INCLUDE_DEBUG_MESSAGE | Debug::INCLUDE_TRACE; + $this->config->setDebug($debug); $query = ' { @@ -649,6 +651,72 @@ class QueryExecutionTest extends \PHPUnit_Framework_TestCase $this->assertEquals('query', $operationType); } + public function testAppliesErrorFormatter() + { + $called = false; + $error = null; + $this->config->setErrorFormatter(function($e) use (&$called, &$error) { + $called = true; + $error = $e; + return ['test' => 'formatted']; + }); + + $result = $this->executeQuery('{fieldWithException}'); + $this->assertFalse($called); + $formatted = $result->toArray(); + $expected = [ + 'errors' => [ + ['test' => 'formatted'] + ] + ]; + $this->assertTrue($called); + $this->assertArraySubset($expected, $formatted); + $this->assertInstanceOf(Error::class, $error); + + // Assert debugging still works even with custom formatter + $formatted = $result->toArray(Debug::INCLUDE_TRACE); + $expected = [ + 'errors' => [ + [ + 'test' => 'formatted', + 'trace' => [] + ] + ] + ]; + $this->assertArraySubset($expected, $formatted); + } + + public function testAppliesErrorsHandler() + { + $called = false; + $errors = null; + $formatter = null; + $this->config->setErrorsHandler(function($e, $f) use (&$called, &$errors, &$formatter) { + $called = true; + $errors = $e; + $formatter = $f; + return [ + ['test' => 'handled'] + ]; + }); + + $result = $this->executeQuery('{fieldWithException,test: fieldWithException}'); + + $this->assertFalse($called); + $formatted = $result->toArray(); + $expected = [ + 'errors' => [ + ['test' => 'handled'] + ] + ]; + $this->assertTrue($called); + $this->assertArraySubset($expected, $formatted); + $this->assertInternalType('array', $errors); + $this->assertCount(2, $errors); + $this->assertInternalType('callable', $formatter); + $this->assertArraySubset($expected, $formatted); + } + private function executePersistedQuery($queryId, $variables = null) { $op = OperationParams::create(['queryId' => $queryId, 'variables' => $variables]); diff --git a/tests/Server/ServerConfigTest.php b/tests/Server/ServerConfigTest.php index 9acc43f..b1e83ea 100644 --- a/tests/Server/ServerConfigTest.php +++ b/tests/Server/ServerConfigTest.php @@ -17,6 +17,7 @@ class ServerConfigTest extends \PHPUnit_Framework_TestCase $this->assertEquals(null, $config->getContext()); $this->assertEquals(null, $config->getRootValue()); $this->assertEquals(null, $config->getErrorFormatter()); + $this->assertEquals(null, $config->getErrorsHandler()); $this->assertEquals(null, $config->getPromiseAdapter()); $this->assertEquals(null, $config->getValidationRules()); $this->assertEquals(null, $config->getDefaultFieldResolver()); @@ -77,6 +78,19 @@ class ServerConfigTest extends \PHPUnit_Framework_TestCase $this->assertSame($formatter, $config->getErrorFormatter()); } + public function testAllowsSettingErrorsHandler() + { + $config = ServerConfig::create(); + + $handler = function() {}; + $config->setErrorsHandler($handler); + $this->assertSame($handler, $config->getErrorsHandler()); + + $handler = 'date'; // test for callable + $config->setErrorsHandler($handler); + $this->assertSame($handler, $config->getErrorsHandler()); + } + public function testAllowsSettingPromiseAdapter() { $config = ServerConfig::create(); diff --git a/tests/ServerTest.php b/tests/ServerTest.php index 426fc93..ffe738f 100644 --- a/tests/ServerTest.php +++ b/tests/ServerTest.php @@ -1,6 +1,7 @@ setDebug(Server::DEBUG_EXCEPTIONS); $server->setExceptionFormatter(function($e) { - return FormattedError::createFromException($e, true); + $debug = Debug::INCLUDE_TRACE; + return FormattedError::createFromException($e, $debug); }); $result = $server->executeQuery('{withException}');