From 0c329821715fc82f08d7443dc474555c667a5078 Mon Sep 17 00:00:00 2001 From: Daniel Tschinder Date: Sun, 11 Feb 2018 13:15:51 +0100 Subject: [PATCH] Fix KnownDirectives validator to support all directives --- src/Validator/DocumentValidator.php | 5 +- src/Validator/Rules/KnownDirectives.php | 23 +++- tests/Language/VisitorTest.php | 4 +- .../Validator/ArgumentsOfCorrectTypeTest.php | 2 +- tests/Validator/KnownDirectivesTest.php | 101 +++++++++++++++++- .../OverlappingFieldsCanBeMergedTest.php | 32 +++--- tests/Validator/QuerySecuritySchema.php | 2 +- tests/Validator/TestCase.php | 101 +++++++++++++++--- tests/Validator/ValidationTest.php | 2 +- 9 files changed, 221 insertions(+), 51 deletions(-) diff --git a/src/Validator/DocumentValidator.php b/src/Validator/DocumentValidator.php index 1a1e83c..1750c84 100644 --- a/src/Validator/DocumentValidator.php +++ b/src/Validator/DocumentValidator.php @@ -266,9 +266,9 @@ class DocumentValidator } } return $errors; - } else { - return static::isValidLiteralValue($itemType, $valueNode); } + + return static::isValidLiteralValue($itemType, $valueNode); } // Input objects check each defined field and look for undefined fields. @@ -278,6 +278,7 @@ class DocumentValidator } $fields = $type->getFields(); + $errors = []; // Ensure every provided field is defined. diff --git a/src/Validator/Rules/KnownDirectives.php b/src/Validator/Rules/KnownDirectives.php index 3593f62..3043cb6 100644 --- a/src/Validator/Rules/KnownDirectives.php +++ b/src/Validator/Rules/KnownDirectives.php @@ -1,10 +1,9 @@ getLocationForAppliedNode($appliedTo); + $candidateLocation = $this->getDirectiveLocationForASTPath($ancestors); if (!$candidateLocation) { $context->reportError(new Error( @@ -58,8 +56,9 @@ class KnownDirectives extends AbstractValidationRule ]; } - private function getLocationForAppliedNode(Node $appliedTo) + private function getDirectiveLocationForASTPath(array $ancestors) { + $appliedTo = $ancestors[count($ancestors) - 1]; switch ($appliedTo->kind) { case NodeKind::OPERATION_DEFINITION: switch ($appliedTo->operation) { @@ -72,6 +71,20 @@ class KnownDirectives extends AbstractValidationRule case NodeKind::FRAGMENT_SPREAD: return DirectiveLocation::FRAGMENT_SPREAD; case NodeKind::INLINE_FRAGMENT: return DirectiveLocation::INLINE_FRAGMENT; case NodeKind::FRAGMENT_DEFINITION: return DirectiveLocation::FRAGMENT_DEFINITION; + case NodeKind::SCHEMA_DEFINITION: return DirectiveLocation::SCHEMA; + case NodeKind::SCALAR_TYPE_DEFINITION: return DirectiveLocation::SCALAR; + case NodeKind::OBJECT_TYPE_DEFINITION: return DirectiveLocation::OBJECT; + case NodeKind::FIELD_DEFINITION: return DirectiveLocation::FIELD_DEFINITION; + case NodeKind::INTERFACE_TYPE_DEFINITION: return DirectiveLocation::IFACE; + case NodeKind::UNION_TYPE_DEFINITION: return DirectiveLocation::UNION; + case NodeKind::ENUM_TYPE_DEFINITION: return DirectiveLocation::ENUM; + case NodeKind::ENUM_VALUE_DEFINITION: return DirectiveLocation::ENUM_VALUE; + case NodeKind::INPUT_OBJECT_TYPE_DEFINITION: return DirectiveLocation::INPUT_OBJECT; + case NodeKind::INPUT_VALUE_DEFINITION: + $parentNode = $ancestors[count($ancestors) - 3]; + return $parentNode instanceof InputObjectTypeDefinitionNode + ? DirectiveLocation::INPUT_FIELD_DEFINITION + : DirectiveLocation::ARGUMENT_DEFINITION; } } } diff --git a/tests/Language/VisitorTest.php b/tests/Language/VisitorTest.php index c65141e..5cfd0b1 100644 --- a/tests/Language/VisitorTest.php +++ b/tests/Language/VisitorTest.php @@ -1131,7 +1131,7 @@ class VisitorTest extends \PHPUnit_Framework_TestCase { $visited = []; - $typeInfo = new TypeInfo(TestCase::getDefaultSchema()); + $typeInfo = new TypeInfo(TestCase::getTestSchema()); $ast = Parser::parse('{ human(id: 4) { name, pets { ... { name } }, unknown } }'); Visitor::visit($ast, Visitor::visitWithTypeInfo($typeInfo, [ @@ -1213,7 +1213,7 @@ class VisitorTest extends \PHPUnit_Framework_TestCase public function testMaintainsTypeInfoDuringEdit() { $visited = []; - $typeInfo = new TypeInfo(TestCase::getDefaultSchema()); + $typeInfo = new TypeInfo(TestCase::getTestSchema()); $ast = Parser::parse( '{ human(id: 4) { name, pets }, alien }' diff --git a/tests/Validator/ArgumentsOfCorrectTypeTest.php b/tests/Validator/ArgumentsOfCorrectTypeTest.php index 5aefb8c..1f1c9df 100644 --- a/tests/Validator/ArgumentsOfCorrectTypeTest.php +++ b/tests/Validator/ArgumentsOfCorrectTypeTest.php @@ -615,7 +615,7 @@ class ArgumentsOfCorrectTypeTest extends TestCase $this->expectPassesRule(new ArgumentsOfCorrectType(), ' { complicatedArgs { - stringListArgField(stringListArg: ["one", "two"]) + stringListArgField(stringListArg: ["one", null, "two"]) } } '); diff --git a/tests/Validator/KnownDirectivesTest.php b/tests/Validator/KnownDirectivesTest.php index 50fe215..d581d1c 100644 --- a/tests/Validator/KnownDirectivesTest.php +++ b/tests/Validator/KnownDirectivesTest.php @@ -89,12 +89,16 @@ class KnownDirectivesTest extends TestCase public function testWithWellPlacedDirectives() { $this->expectPassesRule(new KnownDirectives, ' - query Foo { + query Foo @onQuery { name @include(if: true) ...Frag @include(if: true) skippedField @skip(if: true) ...SkippedFrag @skip(if: true) } + + mutation Bar @onMutation { + someField + } '); } @@ -105,16 +109,103 @@ class KnownDirectivesTest extends TestCase { $this->expectFailsRule(new KnownDirectives, ' query Foo @include(if: true) { - name @operationOnly - ...Frag @operationOnly + name @onQuery + ...Frag @onQuery + } + + mutation Bar @onQuery { + someField } ', [ $this->misplacedDirective('include', 'QUERY', 2, 17), - $this->misplacedDirective('operationOnly', 'FIELD', 3, 14), - $this->misplacedDirective('operationOnly', 'FRAGMENT_SPREAD', 4, 17), + $this->misplacedDirective('onQuery', 'FIELD', 3, 14), + $this->misplacedDirective('onQuery', 'FRAGMENT_SPREAD', 4, 17), + $this->misplacedDirective('onQuery', 'MUTATION', 7, 20), ]); } + // within schema language + + /** + * @it with well placed directives + */ + public function testWSLWithWellPlacedDirectives() + { + $this->expectPassesRule(new KnownDirectives, ' + type MyObj implements MyInterface @onObject { + myField(myArg: Int @onArgumentDefinition): String @onFieldDefinition + } + + scalar MyScalar @onScalar + + interface MyInterface @onInterface { + myField(myArg: Int @onArgumentDefinition): String @onFieldDefinition + } + + union MyUnion @onUnion = MyObj | Other + + enum MyEnum @onEnum { + MY_VALUE @onEnumValue + } + + input MyInput @onInputObject { + myField: Int @onInputFieldDefinition + } + + schema @onSchema { + query: MyQuery + } + '); + } + + /** + * @it with misplaced directives + */ + public function testWSLWithMisplacedDirectives() + { + $this->expectFailsRule(new KnownDirectives, ' + type MyObj implements MyInterface @onInterface { + myField(myArg: Int @onInputFieldDefinition): String @onInputFieldDefinition + } + + scalar MyScalar @onEnum + + interface MyInterface @onObject { + myField(myArg: Int @onInputFieldDefinition): String @onInputFieldDefinition + } + + union MyUnion @onEnumValue = MyObj | Other + + enum MyEnum @onScalar { + MY_VALUE @onUnion + } + + input MyInput @onEnum { + myField: Int @onArgumentDefinition + } + + schema @onObject { + query: MyQuery + } + ', + [ + $this->misplacedDirective('onInterface', 'OBJECT', 2, 43), + $this->misplacedDirective('onInputFieldDefinition', 'ARGUMENT_DEFINITION', 3, 30), + $this->misplacedDirective('onInputFieldDefinition', 'FIELD_DEFINITION', 3, 63), + $this->misplacedDirective('onEnum', 'SCALAR', 6, 25), + $this->misplacedDirective('onObject', 'INTERFACE', 8, 31), + $this->misplacedDirective('onInputFieldDefinition', 'ARGUMENT_DEFINITION', 9, 30), + $this->misplacedDirective('onInputFieldDefinition', 'FIELD_DEFINITION', 9, 63), + $this->misplacedDirective('onEnumValue', 'UNION', 12, 23), + $this->misplacedDirective('onScalar', 'ENUM', 14, 21), + $this->misplacedDirective('onUnion', 'ENUM_VALUE', 15, 20), + $this->misplacedDirective('onEnum', 'INPUT_OBJECT', 18, 23), + $this->misplacedDirective('onArgumentDefinition', 'INPUT_FIELD_DEFINITION', 19, 24), + $this->misplacedDirective('onObject', 'SCHEMA', 22, 16), + ] + ); + } + private function unknownDirective($directiveName, $line, $column) { return FormattedError::create( diff --git a/tests/Validator/OverlappingFieldsCanBeMergedTest.php b/tests/Validator/OverlappingFieldsCanBeMergedTest.php index c9900e7..4c65960 100644 --- a/tests/Validator/OverlappingFieldsCanBeMergedTest.php +++ b/tests/Validator/OverlappingFieldsCanBeMergedTest.php @@ -2,13 +2,11 @@ namespace GraphQL\Tests\Validator; use GraphQL\Error\FormattedError; -use GraphQL\Language\Source; use GraphQL\Language\SourceLocation; -use GraphQL\Schema; +use GraphQL\Type\Schema; use GraphQL\Type\Definition\InterfaceType; use GraphQL\Type\Definition\ObjectType; use GraphQL\Type\Definition\Type; -use GraphQL\Type\Definition\UnionType; use GraphQL\Validator\Rules\OverlappingFieldsCanBeMerged; class OverlappingFieldsCanBeMergedTest extends TestCase @@ -445,7 +443,7 @@ class OverlappingFieldsCanBeMergedTest extends TestCase // type IntBox and the interface type NonNullStringBox1. While that // condition does not exist in the current schema, the schema could // expand in the future to allow this. Thus it is invalid. - $this->expectFailsRuleWithSchema($this->getTestSchema(), new OverlappingFieldsCanBeMerged, ' + $this->expectFailsRuleWithSchema($this->getSchema(), new OverlappingFieldsCanBeMerged, ' { someBox { ...on IntBox { @@ -476,7 +474,7 @@ class OverlappingFieldsCanBeMergedTest extends TestCase // In this case `deepBox` returns `SomeBox` in the first usage, and // `StringBox` in the second usage. These return types are not the same! // however this is valid because the return *shapes* are compatible. - $this->expectPassesRuleWithSchema($this->getTestSchema(), new OverlappingFieldsCanBeMerged, ' + $this->expectPassesRuleWithSchema($this->getSchema(), new OverlappingFieldsCanBeMerged, ' { someBox { ... on SomeBox { @@ -499,7 +497,7 @@ class OverlappingFieldsCanBeMergedTest extends TestCase */ public function testDisallowsDifferingReturnTypesDespiteNoOverlap() { - $this->expectFailsRuleWithSchema($this->getTestSchema(), new OverlappingFieldsCanBeMerged, ' + $this->expectFailsRuleWithSchema($this->getSchema(), new OverlappingFieldsCanBeMerged, ' { someBox { ... on IntBox { @@ -527,7 +525,7 @@ class OverlappingFieldsCanBeMergedTest extends TestCase */ public function testDisallowsDifferingReturnTypeNullabilityDespiteNoOverlap() { - $this->expectFailsRuleWithSchema($this->getTestSchema(), new OverlappingFieldsCanBeMerged, ' + $this->expectFailsRuleWithSchema($this->getSchema(), new OverlappingFieldsCanBeMerged, ' { someBox { ... on NonNullStringBox1 { @@ -555,7 +553,7 @@ class OverlappingFieldsCanBeMergedTest extends TestCase */ public function testDisallowsDifferingReturnTypeListDespiteNoOverlap() { - $this->expectFailsRuleWithSchema($this->getTestSchema(), new OverlappingFieldsCanBeMerged, ' + $this->expectFailsRuleWithSchema($this->getSchema(), new OverlappingFieldsCanBeMerged, ' { someBox { ... on IntBox { @@ -582,7 +580,7 @@ class OverlappingFieldsCanBeMergedTest extends TestCase ]); - $this->expectFailsRuleWithSchema($this->getTestSchema(), new OverlappingFieldsCanBeMerged, ' + $this->expectFailsRuleWithSchema($this->getSchema(), new OverlappingFieldsCanBeMerged, ' { someBox { ... on IntBox { @@ -611,7 +609,7 @@ class OverlappingFieldsCanBeMergedTest extends TestCase public function testDisallowsDifferingSubfields() { - $this->expectFailsRuleWithSchema($this->getTestSchema(), new OverlappingFieldsCanBeMerged, ' + $this->expectFailsRuleWithSchema($this->getSchema(), new OverlappingFieldsCanBeMerged, ' { someBox { ... on IntBox { @@ -645,7 +643,7 @@ class OverlappingFieldsCanBeMergedTest extends TestCase */ public function testDisallowsDifferingDeepReturnTypesDespiteNoOverlap() { - $this->expectFailsRuleWithSchema($this->getTestSchema(), new OverlappingFieldsCanBeMerged, ' + $this->expectFailsRuleWithSchema($this->getSchema(), new OverlappingFieldsCanBeMerged, ' { someBox { ... on IntBox { @@ -681,7 +679,7 @@ class OverlappingFieldsCanBeMergedTest extends TestCase */ public function testAllowsNonConflictingOverlapingTypes() { - $this->expectPassesRuleWithSchema($this->getTestSchema(), new OverlappingFieldsCanBeMerged, ' + $this->expectPassesRuleWithSchema($this->getSchema(), new OverlappingFieldsCanBeMerged, ' { someBox { ... on IntBox { @@ -700,7 +698,7 @@ class OverlappingFieldsCanBeMergedTest extends TestCase */ public function testSameWrappedScalarReturnTypes() { - $this->expectPassesRuleWithSchema($this->getTestSchema(), new OverlappingFieldsCanBeMerged, ' + $this->expectPassesRuleWithSchema($this->getSchema(), new OverlappingFieldsCanBeMerged, ' { someBox { ...on NonNullStringBox1 { @@ -719,7 +717,7 @@ class OverlappingFieldsCanBeMergedTest extends TestCase */ public function testAllowsInlineTypelessFragments() { - $this->expectPassesRuleWithSchema($this->getTestSchema(), new OverlappingFieldsCanBeMerged, ' + $this->expectPassesRuleWithSchema($this->getSchema(), new OverlappingFieldsCanBeMerged, ' { a ... { @@ -734,7 +732,7 @@ class OverlappingFieldsCanBeMergedTest extends TestCase */ public function testComparesDeepTypesIncludingList() { - $this->expectFailsRuleWithSchema($this->getTestSchema(), new OverlappingFieldsCanBeMerged, ' + $this->expectFailsRuleWithSchema($this->getSchema(), new OverlappingFieldsCanBeMerged, ' { connection { ...edgeID @@ -773,7 +771,7 @@ class OverlappingFieldsCanBeMergedTest extends TestCase */ public function testIgnoresUnknownTypes() { - $this->expectPassesRuleWithSchema($this->getTestSchema(), new OverlappingFieldsCanBeMerged, ' + $this->expectPassesRuleWithSchema($this->getSchema(), new OverlappingFieldsCanBeMerged, ' { someBox { ...on UnknownType { @@ -787,7 +785,7 @@ class OverlappingFieldsCanBeMergedTest extends TestCase '); } - private function getTestSchema() + private function getSchema() { $StringBox = null; $IntBox = null; diff --git a/tests/Validator/QuerySecuritySchema.php b/tests/Validator/QuerySecuritySchema.php index 771af77..024990a 100644 --- a/tests/Validator/QuerySecuritySchema.php +++ b/tests/Validator/QuerySecuritySchema.php @@ -1,7 +1,7 @@ ['type' => $CatOrDog], 'dogOrHuman' => ['type' => $DogOrHuman], 'humanOrAlien' => ['type' => $HumanOrAlien], - 'complicatedArgs' => ['type' => $ComplicatedArgs] + 'complicatedArgs' => ['type' => $ComplicatedArgs], ] ]); - $defaultSchema = new Schema([ + $testSchema = new Schema([ 'query' => $queryRoot, - 'directives' => array_merge(GraphQL::getInternalDirectives(), [ + 'directives' => [ + Directive::includeDirective(), + Directive::skipDirective(), new Directive([ - 'name' => 'operationOnly', - 'locations' => [ 'QUERY' ], - ]) - ]) + 'name' => 'onQuery', + 'locations' => ['QUERY'], + ]), + new Directive([ + 'name' => 'onMutation', + 'locations' => ['MUTATION'], + ]), + new Directive([ + 'name' => 'onSubscription', + 'locations' => ['SUBSCRIPTION'], + ]), + new Directive([ + 'name' => 'onField', + 'locations' => ['FIELD'], + ]), + new Directive([ + 'name' => 'onFragmentDefinition', + 'locations' => ['FRAGMENT_DEFINITION'], + ]), + new Directive([ + 'name' => 'onFragmentSpread', + 'locations' => ['FRAGMENT_SPREAD'], + ]), + new Directive([ + 'name' => 'onInlineFragment', + 'locations' => ['INLINE_FRAGMENT'], + ]), + new Directive([ + 'name' => 'onSchema', + 'locations' => ['SCHEMA'], + ]), + new Directive([ + 'name' => 'onScalar', + 'locations' => ['SCALAR'], + ]), + new Directive([ + 'name' => 'onObject', + 'locations' => ['OBJECT'], + ]), + new Directive([ + 'name' => 'onFieldDefinition', + 'locations' => ['FIELD_DEFINITION'], + ]), + new Directive([ + 'name' => 'onArgumentDefinition', + 'locations' => ['ARGUMENT_DEFINITION'], + ]), + new Directive([ + 'name' => 'onInterface', + 'locations' => ['INTERFACE'], + ]), + new Directive([ + 'name' => 'onUnion', + 'locations' => ['UNION'], + ]), + new Directive([ + 'name' => 'onEnum', + 'locations' => ['ENUM'], + ]), + new Directive([ + 'name' => 'onEnumValue', + 'locations' => ['ENUM_VALUE'], + ]), + new Directive([ + 'name' => 'onInputObject', + 'locations' => ['INPUT_OBJECT'], + ]), + new Directive([ + 'name' => 'onInputFieldDefinition', + 'locations' => ['INPUT_FIELD_DEFINITION'], + ]), + ], ]); - return $defaultSchema; + return $testSchema; } function expectValid($schema, $rules, $queryString) @@ -313,12 +380,12 @@ abstract class TestCase extends \PHPUnit_Framework_TestCase function expectPassesRule($rule, $queryString) { - $this->expectValid($this->getDefaultSchema(), [$rule], $queryString); + $this->expectValid($this->getTestSchema(), [$rule], $queryString); } function expectFailsRule($rule, $queryString, $errors) { - return $this->expectInvalid($this->getDefaultSchema(), [$rule], $queryString, $errors); + return $this->expectInvalid($this->getTestSchema(), [$rule], $queryString, $errors); } function expectPassesRuleWithSchema($schema, $rule, $queryString) @@ -333,11 +400,11 @@ abstract class TestCase extends \PHPUnit_Framework_TestCase function expectPassesCompleteValidation($queryString) { - $this->expectValid($this->getDefaultSchema(), DocumentValidator::allRules(), $queryString); + $this->expectValid($this->getTestSchema(), DocumentValidator::allRules(), $queryString); } function expectFailsCompleteValidation($queryString, $errors) { - $this->expectInvalid($this->getDefaultSchema(), DocumentValidator::allRules(), $queryString, $errors); + $this->expectInvalid($this->getTestSchema(), DocumentValidator::allRules(), $queryString, $errors); } } diff --git a/tests/Validator/ValidationTest.php b/tests/Validator/ValidationTest.php index d5fd855..a105bb2 100644 --- a/tests/Validator/ValidationTest.php +++ b/tests/Validator/ValidationTest.php @@ -45,6 +45,6 @@ class ValidationTest extends TestCase 'locations' => [ ['line' => 1, 'column' => 2] ] ]; $this->expectFailsCompleteValidation($query, [$expectedError]); - $this->expectValid($this->getDefaultSchema(), [], $query); + $this->expectValid($this->getTestSchema(), [], $query); } }