From 36b1124d90f12d68fe24b05983f41c2fe737f33e Mon Sep 17 00:00:00 2001 From: Yury Date: Wed, 24 Oct 2018 23:51:27 +0300 Subject: [PATCH 1/3] Error handling improvements --- src/Error/Error.php | 12 ++-- src/Utils/TypeInfo.php | 1 + src/Validator/Rules/ValuesOfCorrectType.php | 19 +++++- tests/Type/EnumTypeTest.php | 2 +- tests/Validator/ValidationTest.php | 2 +- tests/Validator/ValuesOfCorrectTypeTest.php | 67 ++++++++++----------- 6 files changed, 61 insertions(+), 42 deletions(-) diff --git a/src/Error/Error.php b/src/Error/Error.php index 254d395..04112a8 100644 --- a/src/Error/Error.php +++ b/src/Error/Error.php @@ -182,12 +182,16 @@ class Error extends \Exception implements \JsonSerializable, ClientAware if ($previous instanceof ClientAware) { $this->isClientSafe = $previous->isClientSafe(); $this->category = $previous->getCategory() ?: static::CATEGORY_INTERNAL; - } else if ($previous) { - $this->isClientSafe = false; - $this->category = static::CATEGORY_INTERNAL; } else { $this->isClientSafe = true; - $this->category = static::CATEGORY_GRAPHQL; + while ($previous) { + if (!preg_match('#/webonyx/graphql-php/#u', $previous->getFile())) { + $this->isClientSafe = false; + break; + } + $previous = $previous->getPrevious(); + } + $this->category = $this->isClientSafe ? self::CATEGORY_GRAPHQL : self::CATEGORY_INTERNAL; } } diff --git a/src/Utils/TypeInfo.php b/src/Utils/TypeInfo.php index a211c5a..8553f4e 100644 --- a/src/Utils/TypeInfo.php +++ b/src/Utils/TypeInfo.php @@ -20,6 +20,7 @@ use GraphQL\Type\Definition\InputType; use GraphQL\Type\Definition\InterfaceType; use GraphQL\Type\Definition\ListOfType; use GraphQL\Type\Definition\ObjectType; +use GraphQL\Type\Definition\OutputType; use GraphQL\Type\Definition\Type; use GraphQL\Type\Definition\UnionType; use GraphQL\Type\Definition\WrappingType; diff --git a/src/Validator/Rules/ValuesOfCorrectType.php b/src/Validator/Rules/ValuesOfCorrectType.php index a64d034..68b2c7d 100644 --- a/src/Validator/Rules/ValuesOfCorrectType.php +++ b/src/Validator/Rules/ValuesOfCorrectType.php @@ -4,6 +4,7 @@ namespace GraphQL\Validator\Rules; 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; @@ -15,6 +16,7 @@ use GraphQL\Language\AST\StringValueNode; use GraphQL\Language\AST\ValueNode; use GraphQL\Language\Printer; use GraphQL\Language\Visitor; +use GraphQL\Type\Definition\FieldArgument; use GraphQL\Type\Definition\EnumType; use GraphQL\Type\Definition\EnumValueDefinition; use GraphQL\Type\Definition\InputObjectType; @@ -33,8 +35,15 @@ use GraphQL\Validator\ValidationContext; */ class ValuesOfCorrectType extends AbstractValidationRule { - static function badValueMessage($typeName, $valueName, $message = null) + private static $fieldName; + + static function badValueMessage($typeName, $valueName, $message = null, $context = null) { + if ($context && ($arg = $context->getArgument()) && $arg instanceof FieldArgument) { + $fieldName = self::$fieldName; + $argName = $arg->name; + return "Field \"{$fieldName}\" argument \"{$argName}\" requires type {$typeName}, found {$valueName}" . ($message ? "; ${message}" : '.'); + } return "Expected type {$typeName}, found {$valueName}" . ($message ? "; ${message}" : '.'); } @@ -56,6 +65,11 @@ class ValuesOfCorrectType extends AbstractValidationRule public function getVisitor(ValidationContext $context) { return [ + NodeKind::FIELD => [ + 'enter' => function (FieldNode $node) { + self::$fieldName = $node->name->value; + } + ], NodeKind::NULL => function(NullValueNode $node) use ($context) { $type = $context->getInputType(); if ($type instanceof NonNull) { @@ -183,7 +197,8 @@ class ValuesOfCorrectType extends AbstractValidationRule self::badValueMessage( (string) $locationType, Printer::doPrint($node), - $error->getMessage() + $error->getMessage(), + $context ), $node, null, diff --git a/tests/Type/EnumTypeTest.php b/tests/Type/EnumTypeTest.php index 3dbc0d9..4a53e5e 100644 --- a/tests/Type/EnumTypeTest.php +++ b/tests/Type/EnumTypeTest.php @@ -292,7 +292,7 @@ class EnumTypeTest extends \PHPUnit_Framework_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 51c7dbf..481913d 100644 --- a/tests/Validator/ValidationTest.php +++ b/tests/Validator/ValidationTest.php @@ -36,7 +36,7 @@ class ValidationTest extends TestCase '; $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 7240f99..80d6d39 100644 --- a/tests/Validator/ValuesOfCorrectTypeTest.php +++ b/tests/Validator/ValuesOfCorrectTypeTest.php @@ -19,6 +19,11 @@ class ValuesOfCorrectTypeTest extends TestCase ); } + private function badValue2($error, $line, $column) + { + return FormattedError::create($error, [new SourceLocation($line, $column)]); + } + private function requiredField($typeName, $fieldName, $fieldTypeName, $line, $column) { return FormattedError::create( ValuesOfCorrectType::requiredFieldMessage( @@ -231,7 +236,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue('String', '1', 4, 39) + $this->badValue2("Field \"stringArgField\" argument \"stringArg\" requires type String, found 1.", 4, 39) ]); } @@ -247,7 +252,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue('String', '1.0', 4, 39) + $this->badValue2("Field \"stringArgField\" argument \"stringArg\" requires type String, found 1.0.", 4, 39) ]); } @@ -263,7 +268,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue('String', 'true', 4, 39) + $this->badValue2("Field \"stringArgField\" argument \"stringArg\" requires type String, found true.", 4, 39) ]); } @@ -279,7 +284,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue('String', 'BAR', 4, 39) + $this->badValue2("Field \"stringArgField\" argument \"stringArg\" requires type String, found BAR.", 4, 39) ]); } @@ -297,7 +302,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue('Int', '"3"', 4, 33) + $this->badValue2("Field \"intArgField\" argument \"intArg\" requires type Int, found \"3\".", 4, 33) ]); } @@ -313,7 +318,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue('Int', '829384293849283498239482938', 4, 33) + $this->badValue2("Field \"intArgField\" argument \"intArg\" requires type Int, found 829384293849283498239482938.", 4, 33) ]); } @@ -329,7 +334,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue('Int', 'FOO', 4, 33) + $this->badValue2("Field \"intArgField\" argument \"intArg\" requires type Int, found FOO.", 4, 33) ]); } @@ -345,7 +350,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue('Int', '3.0', 4, 33) + $this->badValue2("Field \"intArgField\" argument \"intArg\" requires type Int, found 3.0.", 4, 33) ]); } @@ -361,7 +366,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue('Int', '3.333', 4, 33) + $this->badValue2("Field \"intArgField\" argument \"intArg\" requires type Int, found 3.333.", 4, 33) ]); } @@ -379,7 +384,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue('Float', '"3.333"', 4, 37) + $this->badValue2("Field \"floatArgField\" argument \"floatArg\" requires type Float, found \"3.333\".", 4, 37) ]); } @@ -395,7 +400,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue('Float', 'true', 4, 37) + $this->badValue2("Field \"floatArgField\" argument \"floatArg\" requires type Float, found true.", 4, 37) ]); } @@ -411,7 +416,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue('Float', 'FOO', 4, 37) + $this->badValue2("Field \"floatArgField\" argument \"floatArg\" requires type Float, found FOO.", 4, 37) ]); } @@ -429,7 +434,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue('Boolean', '2', 4, 41) + $this->badValue2("Field \"booleanArgField\" argument \"booleanArg\" requires type Boolean, found 2.", 4, 41) ]); } @@ -445,7 +450,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue('Boolean', '1.0', 4, 41) + $this->badValue2("Field \"booleanArgField\" argument \"booleanArg\" requires type Boolean, found 1.0.", 4, 41) ]); } @@ -461,7 +466,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue('Boolean', '"true"', 4, 41) + $this->badValue2("Field \"booleanArgField\" argument \"booleanArg\" requires type Boolean, found \"true\".", 4, 41) ]); } @@ -477,7 +482,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue('Boolean', 'TRUE', 4, 41) + $this->badValue2("Field \"booleanArgField\" argument \"booleanArg\" requires type Boolean, found TRUE.", 4, 41) ]); } @@ -495,7 +500,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue('ID', '1.0', 4, 31) + $this->badValue2("Field \"idArgField\" argument \"idArg\" requires type ID, found 1.0.", 4, 31) ]); } @@ -511,7 +516,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue('ID', 'true', 4, 31) + $this->badValue2("Field \"idArgField\" argument \"idArg\" requires type ID, found true.", 4, 31) ]); } @@ -527,7 +532,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue('ID', 'SOMETHING', 4, 31) + $this->badValue2("Field \"idArgField\" argument \"idArg\" requires type ID, found SOMETHING.", 4, 31) ]); } @@ -713,7 +718,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue('String', '2', 4, 55), + $this->badValue2("Field \"stringListArgField\" argument \"stringListArg\" requires type String, found 2.", 4, 55), ]); } @@ -729,7 +734,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue('[String]', '1', 4, 47), + $this->badValue2("Field \"stringListArgField\" argument \"stringListArg\" requires type [String], found 1.", 4, 47), ]); } @@ -889,8 +894,8 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue('Int!', '"two"', 4, 32), - $this->badValue('Int!', '"one"', 4, 45), + $this->badValue2("Field \"multipleReqs\" argument \"req2\" requires type Int!, found \"two\".", 4, 32), + $this->badValue2("Field \"multipleReqs\" argument \"req1\" requires type Int!, found \"one\".", 4, 45), ]); } @@ -906,7 +911,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue('Int!', '"one"', 4, 32), + $this->badValue2("Field \"multipleReqs\" argument \"req1\" requires type Int!, found \"one\".", 4, 32), ]); } @@ -1058,7 +1063,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue('String', '2', 5, 40), + $this->badValue2("Field \"complexArgField\" argument \"complexArg\" requires type String, found 2.", 5, 40), ]); } @@ -1102,13 +1107,7 @@ class ValuesOfCorrectTypeTest extends TestCase invalidArg(arg: 123) } ', [ - $this->badValue( - 'Invalid', - '123', - 3, - 27, - 'Invalid scalar is always invalid: 123' - ), + $this->badValue2("Field \"invalidArg\" argument \"arg\" requires type Invalid, found 123; Invalid scalar is always invalid: 123", 3, 27), ]); $this->assertEquals( @@ -1163,8 +1162,8 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue('Boolean!', '"yes"', 3, 28), - $this->badValue('Boolean!', 'ENUM', 4, 28), + $this->badValue2("Field \"dog\" argument \"if\" requires type Boolean!, found \"yes\".", 3, 28), + $this->badValue2("Field \"name\" argument \"if\" requires type Boolean!, found ENUM.", 4, 28), ]); } From c4ee28f2f1da3a653153d422edef86e7adb0781d Mon Sep 17 00:00:00 2001 From: Yury Date: Mon, 29 Oct 2018 14:51:12 +0300 Subject: [PATCH 2/3] Changed the method of errors categorization --- src/Error/Error.php | 12 +-- src/Validator/Rules/ValuesOfCorrectType.php | 37 ++++----- tests/Type/EnumTypeTest.php | 8 +- tests/Validator/ValuesOfCorrectTypeTest.php | 90 +++++++++------------ 4 files changed, 64 insertions(+), 83 deletions(-) diff --git a/src/Error/Error.php b/src/Error/Error.php index 04112a8..254d395 100644 --- a/src/Error/Error.php +++ b/src/Error/Error.php @@ -182,16 +182,12 @@ class Error extends \Exception implements \JsonSerializable, ClientAware if ($previous instanceof ClientAware) { $this->isClientSafe = $previous->isClientSafe(); $this->category = $previous->getCategory() ?: static::CATEGORY_INTERNAL; + } else if ($previous) { + $this->isClientSafe = false; + $this->category = static::CATEGORY_INTERNAL; } else { $this->isClientSafe = true; - while ($previous) { - if (!preg_match('#/webonyx/graphql-php/#u', $previous->getFile())) { - $this->isClientSafe = false; - break; - } - $previous = $previous->getPrevious(); - } - $this->category = $this->isClientSafe ? self::CATEGORY_GRAPHQL : self::CATEGORY_INTERNAL; + $this->category = static::CATEGORY_GRAPHQL; } } diff --git a/src/Validator/Rules/ValuesOfCorrectType.php b/src/Validator/Rules/ValuesOfCorrectType.php index 68b2c7d..a9666c9 100644 --- a/src/Validator/Rules/ValuesOfCorrectType.php +++ b/src/Validator/Rules/ValuesOfCorrectType.php @@ -16,9 +16,9 @@ use GraphQL\Language\AST\StringValueNode; use GraphQL\Language\AST\ValueNode; use GraphQL\Language\Printer; use GraphQL\Language\Visitor; -use GraphQL\Type\Definition\FieldArgument; 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; @@ -39,10 +39,10 @@ class ValuesOfCorrectType extends AbstractValidationRule static function badValueMessage($typeName, $valueName, $message = null, $context = null) { - if ($context && ($arg = $context->getArgument()) && $arg instanceof FieldArgument) { - $fieldName = self::$fieldName; + $fieldName = self::$fieldName; + if ($context AND $arg = $context->getArgument() AND $arg instanceof FieldArgument) { $argName = $arg->name; - return "Field \"{$fieldName}\" argument \"{$argName}\" requires type {$typeName}, found {$valueName}" . ($message ? "; ${message}" : '.'); + return "Field \"{$fieldName}\" argument \"{$argName}\" requires type {$typeName}, found {$valueName}" . ($message ? "; {$message}" : '.'); } return "Expected type {$typeName}, found {$valueName}" . ($message ? "; ${message}" : '.'); @@ -75,7 +75,7 @@ class ValuesOfCorrectType extends AbstractValidationRule if ($type instanceof NonNull) { $context->reportError( new Error( - self::badValueMessage((string) $type, Printer::doPrint($node)), + self::badValueMessage((string) $type, Printer::doPrint($node), null, $context), $node ) ); @@ -147,7 +147,8 @@ class ValuesOfCorrectType extends AbstractValidationRule self::badValueMessage( $type->name, Printer::doPrint($node), - $this->enumTypeSuggestion($type, $node) + $this->enumTypeSuggestion($type, $node), + $context ), $node ) @@ -178,7 +179,8 @@ class ValuesOfCorrectType extends AbstractValidationRule self::badValueMessage( (string) $locationType, Printer::doPrint($node), - $this->enumTypeSuggestion($type, $node) + $this->enumTypeSuggestion($type, $node), + $context ), $node ) @@ -191,7 +193,8 @@ class ValuesOfCorrectType extends AbstractValidationRule try { $type->parseLiteral($node); } catch (\Exception $error) { - // Ensure a reference to the original error is maintained. + // We should not pass $error to "previous" parameter of Error's constructor here, + // otherwise this error will be in "internal" category instead of "graphql". $context->reportError( new Error( self::badValueMessage( @@ -200,27 +203,21 @@ class ValuesOfCorrectType extends AbstractValidationRule $error->getMessage(), $context ), - $node, - null, - null, - null, - $error + $node ) ); } catch (\Throwable $error) { - // Ensure a reference to the original error is maintained. + // We should not pass $error to "previous" parameter of Error's constructor here, + // otherwise this error will be in "internal" category instead of "graphql". $context->reportError( new Error( self::badValueMessage( (string) $locationType, Printer::doPrint($node), - $error->getMessage() + $error->getMessage(), + $context ), - $node, - null, - null, - null, - $error + $node ) ); } diff --git a/tests/Type/EnumTypeTest.php b/tests/Type/EnumTypeTest.php index 4a53e5e..37c7f5e 100644 --- a/tests/Type/EnumTypeTest.php +++ b/tests/Type/EnumTypeTest.php @@ -220,7 +220,7 @@ class EnumTypeTest extends \PHPUnit_Framework_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)] ] ); @@ -235,7 +235,7 @@ class EnumTypeTest extends \PHPUnit_Framework_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)] ] ); @@ -250,7 +250,7 @@ class EnumTypeTest extends \PHPUnit_Framework_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)] ] ); @@ -280,7 +280,7 @@ class EnumTypeTest extends \PHPUnit_Framework_TestCase $this->expectFailure( '{ colorEnum(fromEnum: 1) }', null, - "Expected type Color, found 1." + "Field \"colorEnum\" argument \"fromEnum\" requires type Color, found 1." ); } diff --git a/tests/Validator/ValuesOfCorrectTypeTest.php b/tests/Validator/ValuesOfCorrectTypeTest.php index 80d6d39..a6350e4 100644 --- a/tests/Validator/ValuesOfCorrectTypeTest.php +++ b/tests/Validator/ValuesOfCorrectTypeTest.php @@ -19,9 +19,9 @@ class ValuesOfCorrectTypeTest extends TestCase ); } - private function badValue2($error, $line, $column) + private function badValueWithMessage($message, $line, $column) { - return FormattedError::create($error, [new SourceLocation($line, $column)]); + return FormattedError::create($message, [new SourceLocation($line, $column)]); } private function requiredField($typeName, $fieldName, $fieldTypeName, $line, $column) { @@ -236,7 +236,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue2("Field \"stringArgField\" argument \"stringArg\" requires type String, found 1.", 4, 39) + $this->badValueWithMessage("Field \"stringArgField\" argument \"stringArg\" requires type String, found 1.", 4, 39) ]); } @@ -252,7 +252,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue2("Field \"stringArgField\" argument \"stringArg\" requires type String, found 1.0.", 4, 39) + $this->badValueWithMessage("Field \"stringArgField\" argument \"stringArg\" requires type String, found 1.0.", 4, 39) ]); } @@ -268,7 +268,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue2("Field \"stringArgField\" argument \"stringArg\" requires type String, found true.", 4, 39) + $this->badValueWithMessage("Field \"stringArgField\" argument \"stringArg\" requires type String, found true.", 4, 39) ]); } @@ -284,7 +284,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue2("Field \"stringArgField\" argument \"stringArg\" requires type String, found BAR.", 4, 39) + $this->badValueWithMessage("Field \"stringArgField\" argument \"stringArg\" requires type String, found BAR.", 4, 39) ]); } @@ -302,7 +302,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue2("Field \"intArgField\" argument \"intArg\" requires type Int, found \"3\".", 4, 33) + $this->badValueWithMessage("Field \"intArgField\" argument \"intArg\" requires type Int, found \"3\".", 4, 33) ]); } @@ -318,7 +318,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue2("Field \"intArgField\" argument \"intArg\" requires type Int, found 829384293849283498239482938.", 4, 33) + $this->badValueWithMessage("Field \"intArgField\" argument \"intArg\" requires type Int, found 829384293849283498239482938.", 4, 33) ]); } @@ -334,7 +334,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue2("Field \"intArgField\" argument \"intArg\" requires type Int, found FOO.", 4, 33) + $this->badValueWithMessage("Field \"intArgField\" argument \"intArg\" requires type Int, found FOO.", 4, 33) ]); } @@ -350,7 +350,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue2("Field \"intArgField\" argument \"intArg\" requires type Int, found 3.0.", 4, 33) + $this->badValueWithMessage("Field \"intArgField\" argument \"intArg\" requires type Int, found 3.0.", 4, 33) ]); } @@ -366,7 +366,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue2("Field \"intArgField\" argument \"intArg\" requires type Int, found 3.333.", 4, 33) + $this->badValueWithMessage("Field \"intArgField\" argument \"intArg\" requires type Int, found 3.333.", 4, 33) ]); } @@ -384,7 +384,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue2("Field \"floatArgField\" argument \"floatArg\" requires type Float, found \"3.333\".", 4, 37) + $this->badValueWithMessage("Field \"floatArgField\" argument \"floatArg\" requires type Float, found \"3.333\".", 4, 37) ]); } @@ -400,7 +400,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue2("Field \"floatArgField\" argument \"floatArg\" requires type Float, found true.", 4, 37) + $this->badValueWithMessage("Field \"floatArgField\" argument \"floatArg\" requires type Float, found true.", 4, 37) ]); } @@ -416,7 +416,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue2("Field \"floatArgField\" argument \"floatArg\" requires type Float, found FOO.", 4, 37) + $this->badValueWithMessage("Field \"floatArgField\" argument \"floatArg\" requires type Float, found FOO.", 4, 37) ]); } @@ -434,7 +434,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue2("Field \"booleanArgField\" argument \"booleanArg\" requires type Boolean, found 2.", 4, 41) + $this->badValueWithMessage("Field \"booleanArgField\" argument \"booleanArg\" requires type Boolean, found 2.", 4, 41) ]); } @@ -450,7 +450,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue2("Field \"booleanArgField\" argument \"booleanArg\" requires type Boolean, found 1.0.", 4, 41) + $this->badValueWithMessage("Field \"booleanArgField\" argument \"booleanArg\" requires type Boolean, found 1.0.", 4, 41) ]); } @@ -466,7 +466,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue2("Field \"booleanArgField\" argument \"booleanArg\" requires type Boolean, found \"true\".", 4, 41) + $this->badValueWithMessage("Field \"booleanArgField\" argument \"booleanArg\" requires type Boolean, found \"true\".", 4, 41) ]); } @@ -482,7 +482,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue2("Field \"booleanArgField\" argument \"booleanArg\" requires type Boolean, found TRUE.", 4, 41) + $this->badValueWithMessage("Field \"booleanArgField\" argument \"booleanArg\" requires type Boolean, found TRUE.", 4, 41) ]); } @@ -500,7 +500,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue2("Field \"idArgField\" argument \"idArg\" requires type ID, found 1.0.", 4, 31) + $this->badValueWithMessage("Field \"idArgField\" argument \"idArg\" requires type ID, found 1.0.", 4, 31) ]); } @@ -516,7 +516,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue2("Field \"idArgField\" argument \"idArg\" requires type ID, found true.", 4, 31) + $this->badValueWithMessage("Field \"idArgField\" argument \"idArg\" requires type ID, found true.", 4, 31) ]); } @@ -532,7 +532,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue2("Field \"idArgField\" argument \"idArg\" requires type ID, found SOMETHING.", 4, 31) + $this->badValueWithMessage("Field \"idArgField\" argument \"idArg\" requires type ID, found SOMETHING.", 4, 31) ]); } @@ -550,7 +550,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue('DogCommand', '2', 4, 41) + $this->badValueWithMessage("Field \"doesKnowCommand\" argument \"dogCommand\" requires type DogCommand, found 2.", 4, 41) ]); } @@ -566,7 +566,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue('DogCommand', '1.0', 4, 41) + $this->badValueWithMessage("Field \"doesKnowCommand\" argument \"dogCommand\" requires type DogCommand, found 1.0.", 4, 41) ]); } @@ -582,13 +582,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $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) ]); } @@ -604,7 +598,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue('DogCommand', 'true', 4, 41) + $this->badValueWithMessage("Field \"doesKnowCommand\" argument \"dogCommand\" requires type DogCommand, found true.", 4, 41) ]); } @@ -620,7 +614,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue('DogCommand', 'JUGGLE', 4, 41) + $this->badValueWithMessage("Field \"doesKnowCommand\" argument \"dogCommand\" requires type DogCommand, found JUGGLE.", 4, 41) ]); } @@ -636,13 +630,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $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) ]); } @@ -718,7 +706,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue2("Field \"stringListArgField\" argument \"stringListArg\" requires type String, found 2.", 4, 55), + $this->badValueWithMessage("Field \"stringListArgField\" argument \"stringListArg\" requires type String, found 2.", 4, 55), ]); } @@ -734,7 +722,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue2("Field \"stringListArgField\" argument \"stringListArg\" requires type [String], found 1.", 4, 47), + $this->badValueWithMessage("Field \"stringListArgField\" argument \"stringListArg\" requires type [String], found 1.", 4, 47), ]); } @@ -894,8 +882,8 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue2("Field \"multipleReqs\" argument \"req2\" requires type Int!, found \"two\".", 4, 32), - $this->badValue2("Field \"multipleReqs\" argument \"req1\" requires type Int!, found \"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), ]); } @@ -911,7 +899,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue2("Field \"multipleReqs\" argument \"req1\" requires type Int!, found \"one\".", 4, 32), + $this->badValueWithMessage("Field \"multipleReqs\" argument \"req1\" requires type Int!, found \"one\".", 4, 32), ]); } @@ -927,7 +915,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue('Int!', 'null', 4, 32), + $this->badValueWithMessage("Field \"multipleReqs\" argument \"req1\" requires type Int!, found null.", 4, 32), ]); } @@ -1063,7 +1051,7 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue2("Field \"complexArgField\" argument \"complexArg\" requires type String, found 2.", 5, 40), + $this->badValueWithMessage("Field \"complexArgField\" argument \"complexArg\" requires type String, found 2.", 5, 40), ]); } @@ -1107,12 +1095,12 @@ class ValuesOfCorrectTypeTest extends TestCase invalidArg(arg: 123) } ', [ - $this->badValue2("Field \"invalidArg\" argument \"arg\" requires type Invalid, found 123; Invalid scalar is always invalid: 123", 3, 27), + $this->badValueWithMessage("Field \"invalidArg\" argument \"arg\" requires type Invalid, found 123; Invalid scalar is always invalid: 123", 3, 27), ]); $this->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() ); } @@ -1162,8 +1150,8 @@ class ValuesOfCorrectTypeTest extends TestCase } } ', [ - $this->badValue2("Field \"dog\" argument \"if\" requires type Boolean!, found \"yes\".", 3, 28), - $this->badValue2("Field \"name\" argument \"if\" requires type Boolean!, found 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), ]); } From 9dc41f0bf05e9d52149765e4dc20c3d78f9bd23e Mon Sep 17 00:00:00 2001 From: Yury Date: Mon, 5 Nov 2018 23:39:39 +0300 Subject: [PATCH 3/3] Fixes according to the changes request --- src/Validator/Rules/ValuesOfCorrectType.php | 76 ++++++++++++--------- 1 file changed, 45 insertions(+), 31 deletions(-) diff --git a/src/Validator/Rules/ValuesOfCorrectType.php b/src/Validator/Rules/ValuesOfCorrectType.php index a9666c9..f038412 100644 --- a/src/Validator/Rules/ValuesOfCorrectType.php +++ b/src/Validator/Rules/ValuesOfCorrectType.php @@ -35,19 +35,18 @@ use GraphQL\Validator\ValidationContext; */ class ValuesOfCorrectType extends AbstractValidationRule { - private static $fieldName; - - static function badValueMessage($typeName, $valueName, $message = null, $context = null) + static function badValueMessage($typeName, $valueName, $message = null) { - $fieldName = self::$fieldName; - if ($context AND $arg = $context->getArgument() AND $arg instanceof FieldArgument) { - $argName = $arg->name; - return "Field \"{$fieldName}\" argument \"{$argName}\" requires type {$typeName}, found {$valueName}" . ($message ? "; {$message}" : '.'); - } return "Expected type {$typeName}, found {$valueName}" . ($message ? "; ${message}" : '.'); } + static function badArgumentValueMessage($typeName, $valueName, $fieldName, $argName, $message = null) + { + return "Field \"{$fieldName}\" argument \"{$argName}\" requires type {$typeName}, found {$valueName}" . + ($message ? "; {$message}" : '.'); + } + static function requiredFieldMessage($typeName, $fieldName, $fieldTypeName) { return "Field {$typeName}.{$fieldName} of required type " . @@ -62,42 +61,53 @@ class ValuesOfCorrectType extends AbstractValidationRule ); } + private static function getBadValueMessage($typeName, $valueName, $message = null, $context = null, $fieldName = null) + { + if ($context AND $arg = $context->getArgument()) { + return self::badArgumentValueMessage($typeName, $valueName, $fieldName, $arg->name, $message); + } else { + return self::badValueMessage($typeName, $valueName, $message); + } + } + public function getVisitor(ValidationContext $context) { + $fieldName = ''; return [ NodeKind::FIELD => [ - 'enter' => function (FieldNode $node) { - self::$fieldName = $node->name->value; + 'enter' => function (FieldNode $node) use (&$fieldName) { + $fieldName = $node->name->value; } ], - NodeKind::NULL => function(NullValueNode $node) use ($context) { + NodeKind::NULL => function(NullValueNode $node) use ($context, &$fieldName) { $type = $context->getInputType(); if ($type instanceof NonNull) { $context->reportError( new Error( - self::badValueMessage((string) $type, Printer::doPrint($node), null, $context), + 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); @@ -137,32 +147,33 @@ class ValuesOfCorrectType extends AbstractValidationRule ); } }, - 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); } else if (!$type->getValue($node->value)) { $context->reportError( new Error( - self::badValueMessage( + self::getBadValueMessage( $type->name, Printer::doPrint($node), $this->enumTypeSuggestion($type, $node), - $context + $context, + $fieldName ), $node ) ); } }, - NodeKind::INT => function (IntValueNode $node) use ($context) { $this->isValidScalar($context, $node); }, - NodeKind::FLOAT => function (FloatValueNode $node) use ($context) { $this->isValidScalar($context, $node); }, - NodeKind::STRING => function (StringValueNode $node) use ($context) { $this->isValidScalar($context, $node); }, - NodeKind::BOOLEAN => function (BooleanValueNode $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, &$fieldName) { $this->isValidScalar($context, $node, $fieldName); }, + NodeKind::STRING => function (StringValueNode $node) use ($context, &$fieldName) { $this->isValidScalar($context, $node, $fieldName); }, + NodeKind::BOOLEAN => function (BooleanValueNode $node) use ($context, &$fieldName) { $this->isValidScalar($context, $node, $fieldName); }, ]; } - 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(); @@ -176,11 +187,12 @@ class ValuesOfCorrectType extends AbstractValidationRule if (!$type instanceof ScalarType) { $context->reportError( new Error( - self::badValueMessage( + self::getBadValueMessage( (string) $locationType, Printer::doPrint($node), $this->enumTypeSuggestion($type, $node), - $context + $context, + $fieldName ), $node ) @@ -197,11 +209,12 @@ class ValuesOfCorrectType extends AbstractValidationRule // otherwise this error will be in "internal" category instead of "graphql". $context->reportError( new Error( - self::badValueMessage( + self::getBadValueMessage( (string) $locationType, Printer::doPrint($node), $error->getMessage(), - $context + $context, + $fieldName ), $node ) @@ -211,11 +224,12 @@ class ValuesOfCorrectType extends AbstractValidationRule // otherwise this error will be in "internal" category instead of "graphql". $context->reportError( new Error( - self::badValueMessage( + self::getBadValueMessage( (string) $locationType, Printer::doPrint($node), $error->getMessage(), - $context + $context, + $fieldName ), $node )