From 33e3c9c338b3c77edcdc009d546015e8d74ae049 Mon Sep 17 00:00:00 2001 From: Yury Date: Wed, 28 Nov 2018 17:35:21 +0300 Subject: [PATCH] Error handling improvements --- src/Validator/Rules/ValuesOfCorrectType.php | 96 ++++++++++++-------- tests/Type/EnumTypeTest.php | 10 +-- tests/Validator/ValidationTest.php | 2 +- tests/Validator/ValuesOfCorrectTypeTest.php | 97 +++++++++------------ 4 files changed, 109 insertions(+), 96 deletions(-) diff --git a/src/Validator/Rules/ValuesOfCorrectType.php b/src/Validator/Rules/ValuesOfCorrectType.php index a08d99b..ad2468e 100644 --- a/src/Validator/Rules/ValuesOfCorrectType.php +++ b/src/Validator/Rules/ValuesOfCorrectType.php @@ -8,6 +8,7 @@ use Exception; use GraphQL\Error\Error; use GraphQL\Language\AST\BooleanValueNode; use GraphQL\Language\AST\EnumValueNode; +use GraphQL\Language\AST\FieldNode; use GraphQL\Language\AST\FloatValueNode; use GraphQL\Language\AST\IntValueNode; use GraphQL\Language\AST\ListValueNode; @@ -21,6 +22,7 @@ use GraphQL\Language\Printer; use GraphQL\Language\Visitor; use GraphQL\Type\Definition\EnumType; use GraphQL\Type\Definition\EnumValueDefinition; +use GraphQL\Type\Definition\FieldArgument; use GraphQL\Type\Definition\InputObjectType; use GraphQL\Type\Definition\ListOfType; use GraphQL\Type\Definition\NonNull; @@ -46,8 +48,14 @@ class ValuesOfCorrectType extends ValidationRule { public function getVisitor(ValidationContext $context) { + $fieldName = ''; return [ - NodeKind::NULL => static function (NullValueNode $node) use ($context) { + NodeKind::FIELD => [ + 'enter' => static function (FieldNode $node) use (&$fieldName) { + $fieldName = $node->name->value; + }, + ], + NodeKind::NULL => static function (NullValueNode $node) use ($context, &$fieldName) { $type = $context->getInputType(); if (! ($type instanceof NonNull)) { return; @@ -55,30 +63,31 @@ class ValuesOfCorrectType extends ValidationRule $context->reportError( new Error( - self::badValueMessage((string) $type, Printer::doPrint($node)), + self::getBadValueMessage((string) $type, Printer::doPrint($node), null, $context, $fieldName), $node ) ); }, - NodeKind::LST => function (ListValueNode $node) use ($context) { + NodeKind::LST => function (ListValueNode $node) use ($context, &$fieldName) { // Note: TypeInfo will traverse into a list's item type, so look to the // parent input type to check if it is a list. $type = Type::getNullableType($context->getParentInputType()); if (! $type instanceof ListOfType) { - $this->isValidScalar($context, $node); + $this->isValidScalar($context, $node, $fieldName); return Visitor::skipNode(); } }, - NodeKind::OBJECT => function (ObjectValueNode $node) use ($context) { + NodeKind::OBJECT => function (ObjectValueNode $node) use ($context, &$fieldName) { // Note: TypeInfo will traverse into a list's item type, so look to the // parent input type to check if it is a list. $type = Type::getNamedType($context->getInputType()); if (! $type instanceof InputObjectType) { - $this->isValidScalar($context, $node); + $this->isValidScalar($context, $node, $fieldName); return Visitor::skipNode(); } + unset($fieldName); // Ensure every required field exists. $inputFields = $type->getFields(); $nodeFields = iterator_to_array($node->fields); @@ -127,34 +136,36 @@ class ValuesOfCorrectType extends ValidationRule ) ); }, - NodeKind::ENUM => function (EnumValueNode $node) use ($context) { + NodeKind::ENUM => function (EnumValueNode $node) use ($context, &$fieldName) { $type = Type::getNamedType($context->getInputType()); if (! $type instanceof EnumType) { - $this->isValidScalar($context, $node); + $this->isValidScalar($context, $node, $fieldName); } elseif (! $type->getValue($node->value)) { $context->reportError( new Error( - self::badValueMessage( + self::getBadValueMessage( $type->name, Printer::doPrint($node), - $this->enumTypeSuggestion($type, $node) + $this->enumTypeSuggestion($type, $node), + $context, + $fieldName ), $node ) ); } }, - NodeKind::INT => function (IntValueNode $node) use ($context) { - $this->isValidScalar($context, $node); + NodeKind::INT => function (IntValueNode $node) use ($context, &$fieldName) { + $this->isValidScalar($context, $node, $fieldName); }, - NodeKind::FLOAT => function (FloatValueNode $node) use ($context) { - $this->isValidScalar($context, $node); + NodeKind::FLOAT => function (FloatValueNode $node) use ($context, &$fieldName) { + $this->isValidScalar($context, $node, $fieldName); }, - NodeKind::STRING => function (StringValueNode $node) use ($context) { - $this->isValidScalar($context, $node); + NodeKind::STRING => function (StringValueNode $node) use ($context, &$fieldName) { + $this->isValidScalar($context, $node, $fieldName); }, - NodeKind::BOOLEAN => function (BooleanValueNode $node) use ($context) { - $this->isValidScalar($context, $node); + NodeKind::BOOLEAN => function (BooleanValueNode $node) use ($context, &$fieldName) { + $this->isValidScalar($context, $node, $fieldName); }, ]; } @@ -165,7 +176,7 @@ class ValuesOfCorrectType extends ValidationRule ($message ? "; ${message}" : '.'); } - private function isValidScalar(ValidationContext $context, ValueNode $node) + private function isValidScalar(ValidationContext $context, ValueNode $node, $fieldName) { // Report any error at the full type expected by the location. $locationType = $context->getInputType(); @@ -179,10 +190,12 @@ class ValuesOfCorrectType extends ValidationRule if (! $type instanceof ScalarType) { $context->reportError( new Error( - self::badValueMessage( + self::getBadValueMessage( (string) $locationType, Printer::doPrint($node), - $this->enumTypeSuggestion($type, $node) + $this->enumTypeSuggestion($type, $node), + $context, + $fieldName ), $node ) @@ -199,32 +212,28 @@ class ValuesOfCorrectType extends ValidationRule // Ensure a reference to the original error is maintained. $context->reportError( new Error( - self::badValueMessage( + self::getBadValueMessage( (string) $locationType, Printer::doPrint($node), - $error->getMessage() + $error->getMessage(), + $context, + $fieldName ), - $node, - null, - null, - null, - $error + $node ) ); } catch (Throwable $error) { // Ensure a reference to the original error is maintained. $context->reportError( new Error( - self::badValueMessage( + self::getBadValueMessage( (string) $locationType, Printer::doPrint($node), - $error->getMessage() + $error->getMessage(), + $context, + $fieldName ), - $node, - null, - null, - null, - $error + $node ) ); } @@ -247,6 +256,12 @@ class ValuesOfCorrectType extends ValidationRule } } + public static function badArgumentValueMessage($typeName, $valueName, $fieldName, $argName, $message = null) + { + return sprintf('Field "%s" argument "%s" requires type %s, found %s', $fieldName, $argName, $typeName, $valueName) . + ($message ? sprintf('; %s', $message) : '.'); + } + public static function requiredFieldMessage($typeName, $fieldName, $fieldTypeName) { return sprintf('Field %s.%s of required type %s was not provided.', $typeName, $fieldName, $fieldTypeName); @@ -257,4 +272,15 @@ class ValuesOfCorrectType extends ValidationRule return sprintf('Field "%s" is not defined by type %s', $fieldName, $typeName) . ($message ? sprintf('; %s', $message) : '.'); } + + private static function getBadValueMessage($typeName, $valueName, $message = null, $context = null, $fieldName = null) + { + if ($context) { + $arg = $context->getArgument(); + if ($arg) { + return self::badArgumentValueMessage($typeName, $valueName, $fieldName, $arg->name, $message); + } + } + return self::badValueMessage($typeName, $valueName, $message); + } } diff --git a/tests/Type/EnumTypeTest.php b/tests/Type/EnumTypeTest.php index df920e2..9b2e8e6 100644 --- a/tests/Type/EnumTypeTest.php +++ b/tests/Type/EnumTypeTest.php @@ -231,7 +231,7 @@ class EnumTypeTest extends TestCase '{ colorEnum(fromEnum: "GREEN") }', null, [ - 'message' => 'Expected type Color, found "GREEN"; Did you mean the enum value GREEN?', + 'message' => 'Field "colorEnum" argument "fromEnum" requires type Color, found "GREEN"; Did you mean the enum value GREEN?', 'locations' => [new SourceLocation(1, 23)], ] ); @@ -268,7 +268,7 @@ class EnumTypeTest extends TestCase '{ colorEnum(fromEnum: GREENISH) }', null, [ - 'message' => 'Expected type Color, found GREENISH; Did you mean the enum value GREEN?', + 'message' => 'Field "colorEnum" argument "fromEnum" requires type Color, found GREENISH; Did you mean the enum value GREEN?', 'locations' => [new SourceLocation(1, 23)], ] ); @@ -283,7 +283,7 @@ class EnumTypeTest extends TestCase '{ colorEnum(fromEnum: green) }', null, [ - 'message' => 'Expected type Color, found green; Did you mean the enum value GREEN?', + 'message' => 'Field "colorEnum" argument "fromEnum" requires type Color, found green; Did you mean the enum value GREEN?', 'locations' => [new SourceLocation(1, 23)], ] ); @@ -313,7 +313,7 @@ class EnumTypeTest extends TestCase $this->expectFailure( '{ colorEnum(fromEnum: 1) }', null, - 'Expected type Color, found 1.' + 'Field "colorEnum" argument "fromEnum" requires type Color, found 1.' ); } @@ -325,7 +325,7 @@ class EnumTypeTest extends TestCase $this->expectFailure( '{ colorEnum(fromInt: GREEN) }', null, - 'Expected type Int, found GREEN.' + 'Field "colorEnum" argument "fromInt" requires type Int, found GREEN.' ); } diff --git a/tests/Validator/ValidationTest.php b/tests/Validator/ValidationTest.php index 05dbbf2..e823cdc 100644 --- a/tests/Validator/ValidationTest.php +++ b/tests/Validator/ValidationTest.php @@ -38,7 +38,7 @@ class ValidationTest extends ValidatorTestCase '; $expectedError = [ - 'message' => 'Expected type Invalid, found "bad value"; Invalid scalar is always invalid: bad value', + 'message' => 'Field "invalidArg" argument "arg" requires type Invalid, found "bad value"; Invalid scalar is always invalid: bad value', 'locations' => [['line' => 3, 'column' => 25]], ]; diff --git a/tests/Validator/ValuesOfCorrectTypeTest.php b/tests/Validator/ValuesOfCorrectTypeTest.php index 4de4df4..a174808 100644 --- a/tests/Validator/ValuesOfCorrectTypeTest.php +++ b/tests/Validator/ValuesOfCorrectTypeTest.php @@ -240,7 +240,7 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue('String', '1', 4, 39), + $this->badValueWithMessage('Field "stringArgField" argument "stringArg" requires type String, found 1.', 4, 39), ] ); } @@ -257,6 +257,11 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase ); } + private function badValueWithMessage($message, $line, $column) + { + return FormattedError::create($message, [new SourceLocation($line, $column)]); + } + /** * @see it('Float into String') */ @@ -272,7 +277,7 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue('String', '1.0', 4, 39), + $this->badValueWithMessage('Field "stringArgField" argument "stringArg" requires type String, found 1.0.', 4, 39), ] ); } @@ -294,7 +299,7 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue('String', 'true', 4, 39), + $this->badValueWithMessage('Field "stringArgField" argument "stringArg" requires type String, found true.', 4, 39), ] ); } @@ -314,7 +319,7 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue('String', 'BAR', 4, 39), + $this->badValueWithMessage('Field "stringArgField" argument "stringArg" requires type String, found BAR.', 4, 39), ] ); } @@ -334,7 +339,7 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue('Int', '"3"', 4, 33), + $this->badValueWithMessage('Field "intArgField" argument "intArg" requires type Int, found "3".', 4, 33), ] ); } @@ -354,7 +359,7 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue('Int', '829384293849283498239482938', 4, 33), + $this->badValueWithMessage('Field "intArgField" argument "intArg" requires type Int, found 829384293849283498239482938.', 4, 33), ] ); } @@ -376,7 +381,7 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue('Int', 'FOO', 4, 33), + $this->badValueWithMessage('Field "intArgField" argument "intArg" requires type Int, found FOO.', 4, 33), ] ); } @@ -396,7 +401,7 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue('Int', '3.0', 4, 33), + $this->badValueWithMessage('Field "intArgField" argument "intArg" requires type Int, found 3.0.', 4, 33), ] ); } @@ -416,7 +421,7 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue('Int', '3.333', 4, 33), + $this->badValueWithMessage('Field "intArgField" argument "intArg" requires type Int, found 3.333.', 4, 33), ] ); } @@ -436,7 +441,7 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue('Float', '"3.333"', 4, 37), + $this->badValueWithMessage('Field "floatArgField" argument "floatArg" requires type Float, found "3.333".', 4, 37), ] ); } @@ -456,7 +461,7 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue('Float', 'true', 4, 37), + $this->badValueWithMessage('Field "floatArgField" argument "floatArg" requires type Float, found true.', 4, 37), ] ); } @@ -478,7 +483,7 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue('Float', 'FOO', 4, 37), + $this->badValueWithMessage('Field "floatArgField" argument "floatArg" requires type Float, found FOO.', 4, 37), ] ); } @@ -498,7 +503,7 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue('Boolean', '2', 4, 41), + $this->badValueWithMessage('Field "booleanArgField" argument "booleanArg" requires type Boolean, found 2.', 4, 41), ] ); } @@ -518,7 +523,7 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue('Boolean', '1.0', 4, 41), + $this->badValueWithMessage('Field "booleanArgField" argument "booleanArg" requires type Boolean, found 1.0.', 4, 41), ] ); } @@ -540,7 +545,7 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue('Boolean', '"true"', 4, 41), + $this->badValueWithMessage('Field "booleanArgField" argument "booleanArg" requires type Boolean, found "true".', 4, 41), ] ); } @@ -560,7 +565,7 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue('Boolean', 'TRUE', 4, 41), + $this->badValueWithMessage('Field "booleanArgField" argument "booleanArg" requires type Boolean, found TRUE.', 4, 41), ] ); } @@ -580,7 +585,7 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue('ID', '1.0', 4, 31), + $this->badValueWithMessage('Field "idArgField" argument "idArg" requires type ID, found 1.0.', 4, 31), ] ); } @@ -600,7 +605,7 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue('ID', 'true', 4, 31), + $this->badValueWithMessage('Field "idArgField" argument "idArg" requires type ID, found true.', 4, 31), ] ); } @@ -622,7 +627,7 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue('ID', 'SOMETHING', 4, 31), + $this->badValueWithMessage('Field "idArgField" argument "idArg" requires type ID, found SOMETHING.', 4, 31), ] ); } @@ -642,7 +647,7 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue('DogCommand', '2', 4, 41), + $this->badValueWithMessage('Field "doesKnowCommand" argument "dogCommand" requires type DogCommand, found 2.', 4, 41), ] ); } @@ -662,7 +667,7 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue('DogCommand', '1.0', 4, 41), + $this->badValueWithMessage('Field "doesKnowCommand" argument "dogCommand" requires type DogCommand, found 1.0.', 4, 41), ] ); } @@ -684,13 +689,7 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue( - 'DogCommand', - '"SIT"', - 4, - 41, - 'Did you mean the enum value SIT?' - ), + $this->badValueWithMessage('Field "doesKnowCommand" argument "dogCommand" requires type DogCommand, found "SIT"; Did you mean the enum value SIT?', 4, 41), ] ); } @@ -710,7 +709,7 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue('DogCommand', 'true', 4, 41), + $this->badValueWithMessage('Field "doesKnowCommand" argument "dogCommand" requires type DogCommand, found true.', 4, 41), ] ); } @@ -730,7 +729,7 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue('DogCommand', 'JUGGLE', 4, 41), + $this->badValueWithMessage('Field "doesKnowCommand" argument "dogCommand" requires type DogCommand, found JUGGLE.', 4, 41), ] ); } @@ -750,13 +749,7 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue( - 'DogCommand', - 'sit', - 4, - 41, - 'Did you mean the enum value SIT?' - ), + $this->badValueWithMessage('Field "doesKnowCommand" argument "dogCommand" requires type DogCommand, found sit; Did you mean the enum value SIT?', 4, 41), ] ); } @@ -846,7 +839,7 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue('String', '2', 4, 55), + $this->badValueWithMessage('Field "stringListArgField" argument "stringListArg" requires type String, found 2.', 4, 55), ] ); } @@ -866,7 +859,7 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue('[String]', '1', 4, 47), + $this->badValueWithMessage('Field "stringListArgField" argument "stringListArg" requires type [String], found 1.', 4, 47), ] ); } @@ -1060,8 +1053,8 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue('Int!', '"two"', 4, 32), - $this->badValue('Int!', '"one"', 4, 45), + $this->badValueWithMessage('Field "multipleReqs" argument "req2" requires type Int!, found "two".', 4, 32), + $this->badValueWithMessage('Field "multipleReqs" argument "req1" requires type Int!, found "one".', 4, 45), ] ); } @@ -1081,7 +1074,7 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue('Int!', '"one"', 4, 32), + $this->badValueWithMessage('Field "multipleReqs" argument "req1" requires type Int!, found "one".', 4, 32), ] ); } @@ -1103,7 +1096,7 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue('Int!', 'null', 4, 32), + $this->badValueWithMessage('Field "multipleReqs" argument "req1" requires type Int!, found null.', 4, 32), ] ); } @@ -1277,7 +1270,7 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue('String', '2', 5, 40), + $this->badValueWithMessage('Field "complexArgField" argument "complexArg" requires type String, found 2.', 5, 40), ] ); } @@ -1340,19 +1333,13 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue( - 'Invalid', - '123', - 3, - 27, - 'Invalid scalar is always invalid: 123' - ), + $this->badValueWithMessage('Field "invalidArg" argument "arg" requires type Invalid, found 123; Invalid scalar is always invalid: 123', 3, 27), ] ); self::assertEquals( - 'Invalid scalar is always invalid: 123', - $errors[0]->getPrevious()->getMessage() + 'Field "invalidArg" argument "arg" requires type Invalid, found 123; Invalid scalar is always invalid: 123', + $errors[0]->getMessage() ); } @@ -1411,8 +1398,8 @@ class ValuesOfCorrectTypeTest extends ValidatorTestCase } ', [ - $this->badValue('Boolean!', '"yes"', 3, 28), - $this->badValue('Boolean!', 'ENUM', 4, 28), + $this->badValueWithMessage('Field "dog" argument "if" requires type Boolean!, found "yes".', 3, 28), + $this->badValueWithMessage('Field "name" argument "if" requires type Boolean!, found ENUM.', 4, 28), ] ); }