From 4897f5e811f915fcf7fdcebc352b318821e1d0ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20N=C3=A9grier?= Date: Mon, 7 Oct 2019 14:11:41 +0200 Subject: [PATCH 1/4] Updating code for aggregate exceptions --- src/Exceptions/GraphQLAggregateException.php | 18 +++++++++++ src/Parameters/MissingArgumentException.php | 31 ++++++++++++++++++- .../GraphQLAggregateExceptionTest.php | 8 +++-- 3 files changed, 53 insertions(+), 4 deletions(-) diff --git a/src/Exceptions/GraphQLAggregateException.php b/src/Exceptions/GraphQLAggregateException.php index 8fb436c75d..15e3f2b616 100644 --- a/src/Exceptions/GraphQLAggregateException.php +++ b/src/Exceptions/GraphQLAggregateException.php @@ -7,6 +7,8 @@ use Exception; use GraphQL\Error\ClientAware; use Throwable; +use function array_map; +use function max; class GraphQLAggregateException extends Exception implements GraphQLAggregateExceptionInterface { @@ -31,6 +33,7 @@ public function add(ClientAware $exception): void { $this->exceptions[] = $exception; $this->message .= "\n" . $exception->getMessage(); + $this->updateCode(); } /** @@ -45,4 +48,19 @@ public function hasExceptions(): bool { return ! empty($this->exceptions); } + + /** + * By convention, the aggregated code is the highest code of all exceptions + */ + private function updateCode(): void + { + if (empty($this->exceptions)) { + return; + } + + $codes = array_map(static function (Throwable $t) { + return $t->getCode(); + }, $this->exceptions); + $this->code = max($codes); + } } diff --git a/src/Parameters/MissingArgumentException.php b/src/Parameters/MissingArgumentException.php index 35dba77a4c..f2ed11ee32 100644 --- a/src/Parameters/MissingArgumentException.php +++ b/src/Parameters/MissingArgumentException.php @@ -5,12 +5,13 @@ namespace TheCodingMachine\GraphQLite\Parameters; use BadMethodCallException; +use TheCodingMachine\GraphQLite\Exceptions\GraphQLExceptionInterface; use function get_class; use function is_array; use function is_string; use function sprintf; -class MissingArgumentException extends BadMethodCallException +class MissingArgumentException extends BadMethodCallException implements GraphQLExceptionInterface { public static function create(string $argumentName): self { @@ -66,4 +67,32 @@ private static function toMethod(callable $callable): string return $factoryName . '::' . $callable[1] . '()'; } + + /** + * Returns true when exception message is safe to be displayed to a client. + */ + public function isClientSafe(): bool + { + return true; + } + + /** + * Returns string describing a category of the error. + * + * Value "graphql" is reserved for errors produced by query parsing or validation, do not use it. + */ + public function getCategory(): string + { + return 'graphql'; + } + + /** + * Returns the "extensions" object attached to the GraphQL error. + * + * @return array + */ + public function getExtensions(): array + { + return []; + } } diff --git a/tests/Exceptions/GraphQLAggregateExceptionTest.php b/tests/Exceptions/GraphQLAggregateExceptionTest.php index e38d58d859..da561f870e 100644 --- a/tests/Exceptions/GraphQLAggregateExceptionTest.php +++ b/tests/Exceptions/GraphQLAggregateExceptionTest.php @@ -10,11 +10,13 @@ class GraphQLAggregateExceptionTest extends TestCase public function testAggregateException() { - $error = new Error('foo'); + $error = new GraphQLException('foo', 12); + $error2 = new GraphQLException('bar', 42); $exceptions = new GraphQLAggregateException([$error]); - $exceptions->add($error); + $exceptions->add($error2); - $this->assertSame([$error, $error], $exceptions->getExceptions()); + $this->assertSame([$error, $error2], $exceptions->getExceptions()); + $this->assertSame(42, $exceptions->getCode()); $this->assertTrue($exceptions->hasExceptions()); } } From 6e43c5cc98556baa2483ecc53f9b999ff8302faf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20N=C3=A9grier?= Date: Mon, 7 Oct 2019 15:05:56 +0200 Subject: [PATCH 2/4] Aggregating many exceptions --- src/Exceptions/GraphQLAggregateException.php | 21 +++++++++++++++++++ src/QueryField.php | 15 ++++++++++--- .../ResolvableMutableInputObjectType.php | 9 ++++++++ 3 files changed, 42 insertions(+), 3 deletions(-) diff --git a/src/Exceptions/GraphQLAggregateException.php b/src/Exceptions/GraphQLAggregateException.php index 15e3f2b616..0485a00811 100644 --- a/src/Exceptions/GraphQLAggregateException.php +++ b/src/Exceptions/GraphQLAggregateException.php @@ -8,7 +8,9 @@ use GraphQL\Error\ClientAware; use Throwable; use function array_map; +use function count; use function max; +use function reset; class GraphQLAggregateException extends Exception implements GraphQLAggregateExceptionInterface { @@ -63,4 +65,23 @@ private function updateCode(): void }, $this->exceptions); $this->code = max($codes); } + + /** + * Throw the exceptions passed in parameter. + * If only one exception is passed, it is thrown. + * If many exceptions are passed, they are bundled in the GraphQLAggregateException + * + * @param (ClientAware&Throwable)[] $exceptions + */ + public static function throwExceptions(array $exceptions): void + { + $count = count($exceptions); + if ($count === 0) { + return; + } + if ($count === 1) { + throw reset($exceptions); + } + throw new self($exceptions); + } } diff --git a/src/QueryField.php b/src/QueryField.php index adba377eda..f2f0fa5e5e 100644 --- a/src/QueryField.php +++ b/src/QueryField.php @@ -5,6 +5,7 @@ namespace TheCodingMachine\GraphQLite; use GraphQL\Deferred; +use GraphQL\Error\ClientAware; use GraphQL\Type\Definition\FieldDefinition; use GraphQL\Type\Definition\ListOfType; use GraphQL\Type\Definition\NonNull; @@ -12,6 +13,7 @@ use GraphQL\Type\Definition\ResolveInfo; use GraphQL\Type\Definition\Type; use InvalidArgumentException; +use TheCodingMachine\GraphQLite\Exceptions\GraphQLAggregateException; use TheCodingMachine\GraphQLite\Middlewares\MissingAuthorizationException; use TheCodingMachine\GraphQLite\Parameters\MissingArgumentException; use TheCodingMachine\GraphQLite\Parameters\ParameterInterface; @@ -237,12 +239,19 @@ public static function externalField(QueryFieldDescriptor $fieldDescriptor): sel */ private function paramsToArguments(array $parameters, ?object $source, array $args, $context, ResolveInfo $info, callable $resolve): array { - return array_values(array_map(function (ParameterInterface $parameter) use ($source, $args, $context, $info, $resolve) { + $toPassArgs = []; + $exceptions = []; + foreach ($parameters as $parameter) { try { - return $parameter->resolve($source, $args, $context, $info); + $toPassArgs[] = $parameter->resolve($source, $args, $context, $info); } catch (MissingArgumentException $e) { throw MissingArgumentException::wrapWithFieldContext($e, $this->name, $resolve); + } catch (ClientAware $e) { + $exceptions[] = $e; } - }, $parameters)); + } + GraphQLAggregateException::throwExceptions($exceptions); + + return $toPassArgs; } } diff --git a/src/Types/ResolvableMutableInputObjectType.php b/src/Types/ResolvableMutableInputObjectType.php index 045e71b1b6..868b7695eb 100644 --- a/src/Types/ResolvableMutableInputObjectType.php +++ b/src/Types/ResolvableMutableInputObjectType.php @@ -4,8 +4,10 @@ namespace TheCodingMachine\GraphQLite\Types; +use GraphQL\Error\ClientAware; use GraphQL\Type\Definition\ResolveInfo; use ReflectionMethod; +use TheCodingMachine\GraphQLite\Exceptions\GraphQLAggregateException; use TheCodingMachine\GraphQLite\FieldsBuilder; use TheCodingMachine\GraphQLite\InputTypeGenerator; use TheCodingMachine\GraphQLite\InputTypeUtils; @@ -102,13 +104,17 @@ public function resolve(?object $source, array $args, $context, ResolveInfo $res $parameters = $this->getParameters(); $toPassArgs = []; + $exceptions = []; foreach ($parameters as $parameter) { try { $toPassArgs[] = $parameter->resolve($source, $args, $context, $resolveInfo); } catch (MissingArgumentException $e) { throw MissingArgumentException::wrapWithFactoryContext($e, $this->name, $this->resolve); + } catch (ClientAware $e) { + $exceptions[] = $e; } } + GraphQLAggregateException::throwExceptions($exceptions); $resolve = $this->resolve; @@ -123,8 +129,11 @@ public function resolve(?object $source, array $args, $context, ResolveInfo $res $toPassArgs[] = $parameter->resolve($source, $args, $context, $resolveInfo); } catch (MissingArgumentException $e) { throw MissingArgumentException::wrapWithDecoratorContext($e, $this->name, $decorator); + } catch (ClientAware $e) { + $exceptions[] = $e; } } + GraphQLAggregateException::throwExceptions($exceptions); $object = $decorator(...$toPassArgs); } From 455c00baaa73ba85ba86b9bf1c141df882f6852c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20N=C3=A9grier?= Date: Mon, 7 Oct 2019 17:27:52 +0200 Subject: [PATCH 3/4] Improving code coverage --- src/Exceptions/GraphQLAggregateException.php | 4 - .../Mappers/Parameters/HardCodedParameter.php | 27 ++++++ .../MissingArgumentExceptionTest.php | 4 + .../ResolvableMutableInputObjectTypeTest.php | 91 +++++++++++++++++++ 4 files changed, 122 insertions(+), 4 deletions(-) create mode 100644 tests/Mappers/Parameters/HardCodedParameter.php diff --git a/src/Exceptions/GraphQLAggregateException.php b/src/Exceptions/GraphQLAggregateException.php index 0485a00811..0f943a86dc 100644 --- a/src/Exceptions/GraphQLAggregateException.php +++ b/src/Exceptions/GraphQLAggregateException.php @@ -56,10 +56,6 @@ public function hasExceptions(): bool */ private function updateCode(): void { - if (empty($this->exceptions)) { - return; - } - $codes = array_map(static function (Throwable $t) { return $t->getCode(); }, $this->exceptions); diff --git a/tests/Mappers/Parameters/HardCodedParameter.php b/tests/Mappers/Parameters/HardCodedParameter.php new file mode 100644 index 0000000000..83773315aa --- /dev/null +++ b/tests/Mappers/Parameters/HardCodedParameter.php @@ -0,0 +1,27 @@ +value = $value; + } + + public function resolve(?object $source, array $args, $context, ResolveInfo $info) + { + return $this->value; + } +} \ No newline at end of file diff --git a/tests/Parameters/MissingArgumentExceptionTest.php b/tests/Parameters/MissingArgumentExceptionTest.php index b78133c02f..9c0ba460f0 100644 --- a/tests/Parameters/MissingArgumentExceptionTest.php +++ b/tests/Parameters/MissingArgumentExceptionTest.php @@ -18,5 +18,9 @@ public function testWrapWithFactoryContext(): void $e3 = MissingArgumentException::wrapWithFactoryContext($e, 'Input', function() {}); $this->assertEquals('Expected argument \'foo\' was not provided in GraphQL input type \'Input\' used in factory \'\'', $e3->getMessage()); + + $this->assertTrue($e->isClientSafe()); + $this->assertSame([], $e->getExtensions()); + $this->assertSame('graphql', $e->getCategory()); } } diff --git a/tests/Types/ResolvableMutableInputObjectTypeTest.php b/tests/Types/ResolvableMutableInputObjectTypeTest.php index 758ebe2b1a..80f4c95add 100644 --- a/tests/Types/ResolvableMutableInputObjectTypeTest.php +++ b/tests/Types/ResolvableMutableInputObjectTypeTest.php @@ -2,15 +2,21 @@ namespace TheCodingMachine\GraphQLite\Types; +use DateTimeImmutable; +use GraphQL\Error\Error; use GraphQL\Type\Definition\ResolveInfo; use stdClass; use TheCodingMachine\GraphQLite\AbstractQueryProviderTest; +use TheCodingMachine\GraphQLite\Exceptions\GraphQLAggregateException; +use TheCodingMachine\GraphQLite\FieldsBuilder; use TheCodingMachine\GraphQLite\Fixtures\TestObject; use TheCodingMachine\GraphQLite\Fixtures\TestObject2; use TheCodingMachine\GraphQLite\Fixtures\TestObjectWithRecursiveList; use TheCodingMachine\GraphQLite\Fixtures\Types\TestFactory; use TheCodingMachine\GraphQLite\GraphQLRuntimeException; +use TheCodingMachine\GraphQLite\Mappers\Parameters\HardCodedParameter; use TheCodingMachine\GraphQLite\Parameters\MissingArgumentException; +use TheCodingMachine\GraphQLite\Parameters\ParameterInterface; class ResolvableMutableInputObjectTypeTest extends AbstractQueryProviderTest { @@ -100,4 +106,89 @@ public function testListResolve(): void $this->assertInstanceOf(TestObject2::class, $obj); $this->assertSame('2018-12-25-foo-bar-1', $obj->getTest2()); } + + public function testExceptions(): void + { + $fieldsBuilder = $this->createMock(FieldsBuilder::class); + + $fieldsBuilder->method('getParameters')->willReturn([ + new class implements ParameterInterface { + public function resolve(?object $source, array $args, $context, ResolveInfo $info) + { + throw new Error('boum'); + } + }, + new class implements ParameterInterface { + public function resolve(?object $source, array $args, $context, ResolveInfo $info) + { + throw new Error('boum'); + } + } + ]); + + $inputType = new ResolvableMutableInputObjectType('InputObject2', + $fieldsBuilder, + new TestFactory(), + 'myListFactory', + null, + false); + + $this->expectException(GraphQLAggregateException::class); + $obj = $inputType->resolve(new stdClass(), ['date' => '2018-12-25', 'stringList' => + [ + 'foo', + 'bar' + ], + 'dateList' => [ + '2018-12-25' + ]], null, $this->createMock(ResolveInfo::class)); + } + + public function testExceptionsInDecorator(): void + { + $fieldsBuilder = $this->createMock(FieldsBuilder::class); + + $fieldsBuilder->method('getParameters')->willReturn([ + new HardCodedParameter(new DateTimeImmutable('now')), + new HardCodedParameter([]), + new HardCodedParameter([]), + ]); + + + $fieldsBuilder->method('getParametersForDecorator')->willReturn([ + new class implements ParameterInterface { + public function resolve(?object $source, array $args, $context, ResolveInfo $info) + { + throw new Error('boum'); + } + }, + new class implements ParameterInterface { + public function resolve(?object $source, array $args, $context, ResolveInfo $info) + { + throw new Error('boum'); + } + } + ]); + + $testFactory = new TestFactory(); + $inputType = new ResolvableMutableInputObjectType('InputObject2', + $fieldsBuilder, + $testFactory, + 'myListFactory', + null, + false); + + $inputType->decorate([$testFactory, 'myDecorator']); + + $this->expectException(GraphQLAggregateException::class); + $obj = $inputType->resolve(new stdClass(), ['date' => '2018-12-25', 'stringList' => + [ + 'foo', + 'bar' + ], + 'dateList' => [ + '2018-12-25' + ]], null, $this->createMock(ResolveInfo::class)); + } + } From 44f24b7c1fdb84b54b1c28941f82ee85b93ddef9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20N=C3=A9grier?= Date: Mon, 7 Oct 2019 18:20:11 +0200 Subject: [PATCH 4/4] Improving code coverage --- src/Exceptions/GraphQLAggregateException.php | 4 ++- src/QueryField.php | 2 -- .../GraphQLAggregateExceptionTest.php | 10 +++++++ tests/QueryFieldTest.php | 30 +++++++++++++++++++ 4 files changed, 43 insertions(+), 3 deletions(-) create mode 100644 tests/QueryFieldTest.php diff --git a/src/Exceptions/GraphQLAggregateException.php b/src/Exceptions/GraphQLAggregateException.php index 0f943a86dc..394eb0503a 100644 --- a/src/Exceptions/GraphQLAggregateException.php +++ b/src/Exceptions/GraphQLAggregateException.php @@ -76,7 +76,9 @@ public static function throwExceptions(array $exceptions): void return; } if ($count === 1) { - throw reset($exceptions); + /** @var Throwable $exception */ + $exception = reset($exceptions); + throw $exception; } throw new self($exceptions); } diff --git a/src/QueryField.php b/src/QueryField.php index f2f0fa5e5e..0524860d9b 100644 --- a/src/QueryField.php +++ b/src/QueryField.php @@ -20,9 +20,7 @@ use TheCodingMachine\GraphQLite\Parameters\PrefetchDataParameter; use TheCodingMachine\GraphQLite\Parameters\SourceParameter; use Webmozart\Assert\Assert; -use function array_map; use function array_unshift; -use function array_values; use function get_class; use function is_object; diff --git a/tests/Exceptions/GraphQLAggregateExceptionTest.php b/tests/Exceptions/GraphQLAggregateExceptionTest.php index da561f870e..ed8ea86c59 100644 --- a/tests/Exceptions/GraphQLAggregateExceptionTest.php +++ b/tests/Exceptions/GraphQLAggregateExceptionTest.php @@ -19,4 +19,14 @@ public function testAggregateException() $this->assertSame(42, $exceptions->getCode()); $this->assertTrue($exceptions->hasExceptions()); } + + public function testOnlyOneAggregateExceptionThrowsTheSameException() + { + $error = new GraphQLException('foo', 12); + + $this->expectException(GraphQLException::class); + GraphQLAggregateException::throwExceptions([$error]); + + } + } diff --git a/tests/QueryFieldTest.php b/tests/QueryFieldTest.php new file mode 100644 index 0000000000..194203636f --- /dev/null +++ b/tests/QueryFieldTest.php @@ -0,0 +1,30 @@ +resolveFn; + + $this->expectException(Error::class); + $resolve(new TestObject('foo'), ['arg' => 12], null, $this->createMock(ResolveInfo::class)); + } +}