Skip to content

Fix overly eager validation of repeatable directive usage #848

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 23, 2021
Merged

Fix overly eager validation of repeatable directive usage #848

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
38d6b4b
Fix overly eager validation of repeatable directive usage
spawnia Apr 23, 2021
2632960
changelog
spawnia Apr 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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