Skip to content

Commit

Permalink
Fix overly eager validation of repeatable directive usage (#848)
Browse files Browse the repository at this point in the history
  • Loading branch information
@spawnia
spawnia authored Apr 23, 2021
1 parent cb54f64 commit bcbb692
Show file tree
Hide file tree
Showing 3 changed files with 75 additions and 37 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@

#### Unreleased

#### 14.6.2

Fix:
- Fix overly eager validation of repeatable directive usage

#### 14.6.1

Fix:
Expand Down
21 changes: 14 additions & 7 deletions src/Type/SchemaValidationContext.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -372,8 +372,9 @@ public function validateTypes() : void
*/
private function validateDirectivesAtLocation($directives, string $location)
{
$directivesNamed = [];
$schema = $this->schema;
/** @var array<string, array<int, DirectiveNode>> $potentiallyDuplicateDirectives */
$potentiallyDuplicateDirectives = [];
$schema = $this->schema;
foreach ($directives as $directive) {
$directiveName = $directive->name->value;

Expand All @@ -386,6 +387,7 @@ private function validateDirectivesAtLocation($directives, string $location)
);
continue;
}

$includes = Utils::some(
$schemaDirective->locations,
static function ($schemaLocation) use ($location) : bool {
Expand All @@ -402,17 +404,22 @@ static function ($schemaLocation) use ($location) : bool {
);
}

$existingNodes = $directivesNamed[$directiveName] ?? [];
$existingNodes[] = $directive;
$directivesNamed[$directiveName] = $existingNodes;
if ($schemaDirective->isRepeatable) {
continue;
}

$existingNodes = $potentiallyDuplicateDirectives[$directiveName] ?? [];
$existingNodes[] = $directive;
$potentiallyDuplicateDirectives[$directiveName] = $existingNodes;
}
foreach ($directivesNamed as $directiveName => $directiveList) {

foreach ($potentiallyDuplicateDirectives as $directiveName => $directiveList) {
if (count($directiveList) <= 1) {
continue;
}

$this->reportError(
sprintf('Directive @%s used twice at the same location.', $directiveName),
sprintf('Non-repeatable directive @%s used more than once at the same location.', $directiveName),
$directiveList
);
}
Expand Down
86 changes: 56 additions & 30 deletions tests/Type/ValidationTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3078,19 +3078,19 @@ public function testRejectsASchemaWithSameSchemaDirectiveUsedTwice()
type Query implements SomeInterface @object @object {
test(arg: SomeInput @argument_definition @argument_definition): String
}
interface SomeInterface @interface @interface {
test: String @field_definition @field_definition
}
union SomeUnion @union @union = Query
scalar SomeScalar @scalar @scalar
enum SomeEnum @enum @enum {
SOME_VALUE @enum_value @enum_value
}
input SomeInput @input_object @input_object {
some_input_field: String @input_field_definition @input_field_definition
}
Expand All @@ -3099,43 +3099,68 @@ enum SomeEnum @enum @enum {
$schema->validate(),
[
[
'message' => 'Directive @schema used twice at the same location.',
'message' => 'Non-repeatable directive @schema used more than once at the same location.',
'locations' => [[ 'line' => 14, 'column' => 18 ], [ 'line' => 14, 'column' => 26 ]],
],[
'message' => 'Directive @argument_definition used twice at the same location.',
],
[
'message' => 'Non-repeatable directive @argument_definition used more than once at the same location.',
'locations' => [[ 'line' => 19, 'column' => 33 ], [ 'line' => 19, 'column' => 54 ]],
],[
'message' => 'Directive @object used twice at the same location.',
],
[
'message' => 'Non-repeatable directive @object used more than once at the same location.',
'locations' => [[ 'line' => 18, 'column' => 47 ], [ 'line' => 18, 'column' => 55 ]],
],[
'message' => 'Directive @field_definition used twice at the same location.',
],
[
'message' => 'Non-repeatable directive @field_definition used more than once at the same location.',
'locations' => [[ 'line' => 23, 'column' => 26 ], [ 'line' => 23, 'column' => 44 ]],
],[
'message' => 'Directive @interface used twice at the same location.',
],
[
'message' => 'Non-repeatable directive @interface used more than once at the same location.',
'locations' => [[ 'line' => 22, 'column' => 35 ], [ 'line' => 22, 'column' => 46 ]],
],[
'message' => 'Directive @input_field_definition used twice at the same location.',
],
[
'message' => 'Non-repeatable directive @input_field_definition used more than once at the same location.',
'locations' => [[ 'line' => 35, 'column' => 38 ], [ 'line' => 35, 'column' => 62 ]],
],[
'message' => 'Directive @input_object used twice at the same location.',
],
[
'message' => 'Non-repeatable directive @input_object used more than once at the same location.',
'locations' => [[ 'line' => 34, 'column' => 27 ], [ 'line' => 34, 'column' => 41 ]],
],[
'message' => 'Directive @union used twice at the same location.',
],
[
'message' => 'Non-repeatable directive @union used more than once at the same location.',
'locations' => [[ 'line' => 26, 'column' => 27 ], [ 'line' => 26, 'column' => 34 ]],
],[
'message' => 'Directive @scalar used twice at the same location.',
],
[
'message' => 'Non-repeatable directive @scalar used more than once at the same location.',
'locations' => [[ 'line' => 28, 'column' => 29 ], [ 'line' => 28, 'column' => 37 ]],
],[
'message' => 'Directive @enum_value used twice at the same location.',
],
[
'message' => 'Non-repeatable directive @enum_value used more than once at the same location.',
'locations' => [[ 'line' => 31, 'column' => 24 ], [ 'line' => 31, 'column' => 36 ]],
],[
'message' => 'Directive @enum used twice at the same location.',
],
[
'message' => 'Non-repeatable directive @enum used more than once at the same location.',
'locations' => [[ 'line' => 30, 'column' => 25 ], [ 'line' => 30, 'column' => 31 ]],
],
]
);
}

public function testAllowsRepeatableDirectivesMultipleTimesAtTheSameLocation() : void
{
$schema = BuildSchema::build('
directive @repeatable repeatable on OBJECT
type Query @repeatable @repeatable {
foo: ID
}
', null, ['assumeValid' => true]);
$this->assertMatchesValidationMessage(
$schema->validate(),
[]
);
}

/**
* @see it('rejects a Schema with directive used again in extension')
*/
Expand All @@ -3157,10 +3182,11 @@ public function testRejectsASchemaWithSameDefinitionDirectiveUsedTwice()

$this->assertMatchesValidationMessage(
$extendedSchema->validate(),
[[
'message' => 'Directive @testA used twice at the same location.',
'locations' => [[ 'line' => 4, 'column' => 22 ], [ 'line' => 2, 'column' => 29 ]],
],
[
[
'message' => 'Non-repeatable directive @testA used more than once at the same location.',
'locations' => [[ 'line' => 4, 'column' => 22 ], [ 'line' => 2, 'column' => 29 ]],
],
]
);
}
Expand Down

0 comments on commit bcbb692

Please sign in to comment.