Skip to content

Commit d4620e1

Browse files
vhenzlvhenzl
authored
Extract check for unique directive names into separate rule (#1109)
1 parent 7a33c3d commit d4620e1

File tree

7 files changed

+228
-35
lines changed

7 files changed

+228
-35
lines changed

‎src/Utils/SchemaExtender.php‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -104,12 +104,6 @@ public static function extend(
104104
static::assertTypeMatchesExtension($existingType, $def);
105105
static::$typeExtensionsMap[$extendedTypeName][] = $def;
106106
} elseif ($def instanceof DirectiveDefinitionNode) {
107-
$directiveName = $def->name->value;
108-
$existingDirective = $schema->getDirective($directiveName);
109-
if ($existingDirective !== null) {
110-
throw new Error('Directive "' . $directiveName . '" already exists in the schema. It cannot be redefined.', [$def]);
111-
}
112-
113107
$directiveDefinitions[] = $def;
114108
}
115109
}

‎src/Validator/DocumentValidator.php‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
use GraphQL\Validator\Rules\ScalarLeafs;
3535
use GraphQL\Validator\Rules\SingleFieldSubscription;
3636
use GraphQL\Validator\Rules\UniqueArgumentNames;
37+
use GraphQL\Validator\Rules\UniqueDirectiveNames;
3738
use GraphQL\Validator\Rules\UniqueDirectivesPerLocation;
3839
use GraphQL\Validator\Rules\UniqueEnumValueNames;
3940
use GraphQL\Validator\Rules\UniqueFragmentNames;
@@ -46,6 +47,7 @@
4647
use GraphQL\Validator\Rules\ValuesOfCorrectType;
4748
use GraphQL\Validator\Rules\VariablesAreInputTypes;
4849
use GraphQL\Validator\Rules\VariablesInAllowedPosition;
50+
use function implode;
4951

5052
/**
5153
* Implements the "Validation" section of the spec.
@@ -203,6 +205,7 @@ public static function sdlRules(): array
203205
LoneSchemaDefinition::class => new LoneSchemaDefinition(),
204206
UniqueOperationTypes::class => new UniqueOperationTypes(),
205207
UniqueTypeNames::class => new UniqueTypeNames(),
208+
UniqueDirectiveNames::class => new UniqueDirectiveNames(),
206209
KnownTypeNames::class => new KnownTypeNames(),
207210
KnownDirectives::class => new KnownDirectives(),
208211
KnownArgumentNamesOnDirectives::class => new KnownArgumentNamesOnDirectives(),
@@ -302,11 +305,11 @@ public static function assertValidSDLExtension(DocumentNode $documentAST, Schema
302305
*/
303306
private static function combineErrorMessages(array $errors): string
304307
{
305-
$str = '';
308+
$messages = [];
306309
foreach ($errors as $error) {
307-
$str .= $error->getMessage() . "\n\n";
310+
$messages[] = $error->getMessage();
308311
}
309312

310-
return $str;
313+
return implode("\n\n", $messages);
311314
}
312315
}

‎src/Validator/Rules/UniqueDirectiveNames.php‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
<?php declare(strict_types=1);
2+
3+
namespace GraphQL\Validator\Rules;
4+
5+
use GraphQL\Error\Error;
6+
use GraphQL\Language\AST\NameNode;
7+
use GraphQL\Language\AST\NodeKind;
8+
use GraphQL\Language\Visitor;
9+
use GraphQL\Language\VisitorOperation;
10+
use GraphQL\Validator\SDLValidationContext;
11+
12+
/**
13+
* Unique directive names.
14+
*
15+
* A GraphQL document is only valid if all defined directives have unique names.
16+
*/
17+
class UniqueDirectiveNames extends ValidationRule
18+
{
19+
public function getSDLVisitor(SDLValidationContext $context): array
20+
{
21+
$schema = $context->getSchema();
22+
23+
/** @var array<string, NameNode> $knownDirectiveNames */
24+
$knownDirectiveNames = [];
25+
26+
return [
27+
NodeKind::DIRECTIVE_DEFINITION => static function ($node) use ($context, $schema, &$knownDirectiveNames): ?VisitorOperation {
28+
$directiveName = $node->name->value;
29+
30+
if ($schema !== null && $schema->getDirective($directiveName) !== null) {
31+
$context->reportError(
32+
new Error(
33+
'Directive "@' . $directiveName . '" already exists in the schema. It cannot be redefined.',
34+
$node->name,
35+
),
36+
);
37+
38+
return null;
39+
}
40+
41+
if (isset($knownDirectiveNames[$directiveName])) {
42+
$context->reportError(
43+
new Error(
44+
'There can be only one directive named "@' . $directiveName . '".',
45+
[
46+
$knownDirectiveNames[$directiveName],
47+
$node->name,
48+
]
49+
),
50+
);
51+
} else {
52+
$knownDirectiveNames[$directiveName] = $node->name;
53+
}
54+
55+
return Visitor::skipNode();
56+
},
57+
];
58+
}
59+
}

‎tests/Type/ValidationTest.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2941,7 +2941,7 @@ public function testRejectsASchemaWithDirectiveDefinedMultipleTimes(): void
29412941
29422942
directive @testA on SCHEMA
29432943
directive @testA on SCHEMA
2944-
');
2944+
', null, ['assumeValidSDL' => true]);
29452945
$this->assertMatchesValidationMessage(
29462946
$schema->validate(),
29472947
[

‎tests/Utils/SchemaExtenderLegacyTest.php‎

Lines changed: 0 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@
2828
* Their counterparts have been removed from `extendSchema-test.js` and moved elsewhere,
2929
* but these changes to `graphql-js` haven't been reflected in `graphql-php` yet.
3030
* TODO align with:
31-
* - https://github.com/graphql/graphql-js/commit/c1745228b2ae5ec89b8de36ea766d544607e21ea
3231
* - https://github.com/graphql/graphql-js/commit/e6a3f08cc92594f68a6e61d3d4b46a6d279f845e.
3332
*/
3433
class SchemaExtenderLegacyTest extends TestCase
@@ -189,29 +188,6 @@ protected function extendTestSchema(string $sdl, array $options = []): Schema
189188
return $extendedSchema;
190189
}
191190

192-
// Extract check for unique directive names into separate rule
193-
194-
/**
195-
* @see it('does not allow replacing a custom directive')
196-
*/
197-
public function testDoesNotAllowReplacingACustomDirective(): void
198-
{
199-
$extendedSchema = $this->extendTestSchema('
200-
directive @meow(if: Boolean!) on FIELD | FRAGMENT_SPREAD
201-
');
202-
203-
$replacementAST = Parser::parse('
204-
directive @meow(if: Boolean!) on FIELD | QUERY
205-
');
206-
207-
try {
208-
SchemaExtender::extend($extendedSchema, $replacementAST);
209-
self::fail();
210-
} catch (Error $error) {
211-
self::assertEquals('Directive "meow" already exists in the schema. It cannot be redefined.', $error->getMessage());
212-
}
213-
}
214-
215191
/**
216192
* @see it('does not allow replacing an existing field')
217193
*/

‎tests/Utils/SchemaExtenderTest.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1390,7 +1390,7 @@ public function testDoesNotAllowReplacingADefaultDirective(): void
13901390
$this->extendTestSchema($sdl);
13911391
self::fail();
13921392
} catch (Error $error) {
1393-
self::assertSame('Directive "include" already exists in the schema. It cannot be redefined.', $error->getMessage());
1393+
self::assertSame('Directive "@include" already exists in the schema. It cannot be redefined.', $error->getMessage());
13941394
}
13951395
}
13961396

‎tests/Validator/UniqueDirectiveNamesTest.php‎

Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,161 @@
1+
<?php declare(strict_types=1);
2+
3+
namespace GraphQL\Tests\Validator;
4+
5+
use GraphQL\Type\Schema;
6+
use GraphQL\Utils\BuildSchema;
7+
use GraphQL\Validator\Rules\UniqueDirectiveNames;
8+
9+
final class UniqueDirectiveNamesTest extends ValidatorTestCase
10+
{
11+
/**
12+
* @param array<int, array<string, mixed>> $errors
13+
*/
14+
private function expectSDLErrors(string $sdlString, ?Schema $schema, array $errors): void
15+
{
16+
$this->expectSDLErrorsFromRule(new UniqueDirectiveNames(), $sdlString, $schema, $errors);
17+
}
18+
19+
/**
20+
* @see describe('Validate: Unique directive names')
21+
* @see it('no directive')
22+
*/
23+
public function testNoDirective(): void
24+
{
25+
$this->expectValidSDL(
26+
new UniqueDirectiveNames(),
27+
'
28+
type Foo
29+
'
30+
);
31+
}
32+
33+
/**
34+
* @see it('one directive')
35+
*/
36+
public function testOneDirective(): void
37+
{
38+
$this->expectValidSDL(
39+
new UniqueDirectiveNames(),
40+
'
41+
directive @foo on SCHEMA
42+
'
43+
);
44+
}
45+
46+
/**
47+
* @see it('many directives')
48+
*/
49+
public function testManyDirectives(): void
50+
{
51+
$this->expectValidSDL(
52+
new UniqueDirectiveNames(),
53+
'
54+
directive @foo on SCHEMA
55+
directive @bar on SCHEMA
56+
directive @baz on SCHEMA
57+
'
58+
);
59+
}
60+
61+
/**
62+
* @see it('directive and non-directive definitions named the same')
63+
*/
64+
public function testDirectiveAndNonDirectiveDefinitionsNamedTheSame(): void
65+
{
66+
$this->expectValidSDL(
67+
new UniqueDirectiveNames(),
68+
'
69+
query foo { __typename }
70+
fragment foo on foo { __typename }
71+
type foo
72+
73+
directive @foo on SCHEMA
74+
'
75+
);
76+
}
77+
78+
/**
79+
* @see it('directives named the same')
80+
*/
81+
public function testDirectivesNamedTheSame(): void
82+
{
83+
$this->expectSDLErrors(
84+
'
85+
directive @foo on SCHEMA
86+
87+
directive @foo on SCHEMA
88+
',
89+
null,
90+
[
91+
[
92+
'message' => 'There can be only one directive named "@foo".',
93+
'locations' => [
94+
['line' => 2, 'column' => 18],
95+
['line' => 4, 'column' => 18],
96+
],
97+
],
98+
],
99+
);
100+
}
101+
102+
/**
103+
* @see it('adding new directive to existing schema')
104+
*/
105+
public function testAddingNewDirectiveToExistingSchema(): void
106+
{
107+
$schema = BuildSchema::build('directive @foo on SCHEMA');
108+
109+
$this->expectValidSDL(new UniqueDirectiveNames(), 'directive @bar on SCHEMA', $schema);
110+
}
111+
112+
/**
113+
* @see it('adding new directive with standard name to existing schema')
114+
*/
115+
public function testAddingNewDirectiveWithStandardNameToExistingSchema(): void
116+
{
117+
$schema = BuildSchema::build('type foo');
118+
119+
$this->expectSDLErrors(
120+
'directive @skip on SCHEMA',
121+
$schema,
122+
[
123+
[
124+
'message' => 'Directive "@skip" already exists in the schema. It cannot be redefined.',
125+
'locations' => [
126+
['line' => 1, 'column' => 12],
127+
],
128+
],
129+
],
130+
);
131+
}
132+
133+
/**
134+
* @see it('adding new directive to existing schema with same-named type')
135+
*/
136+
public function testAddingNewDirectiveToExistingSchemaWithSameNamedType(): void
137+
{
138+
$schema = BuildSchema::build('type foo');
139+
140+
$this->expectValidSDL(new UniqueDirectiveNames(), 'directive @foo on SCHEMA', $schema);
141+
}
142+
143+
/**
144+
* @see it('adding conflicting directives to existing schema')
145+
*/
146+
public function testAddingConflictingDirectivesToExistingSchema(): void
147+
{
148+
$schema = BuildSchema::build('directive @foo on SCHEMA');
149+
150+
$this->expectSDLErrors(
151+
'directive @foo on SCHEMA',
152+
$schema,
153+
[
154+
[
155+
'message' => 'Directive "@foo" already exists in the schema. It cannot be redefined.',
156+
'locations' => [['line' => 1, 'column' => 12]],
157+
],
158+
],
159+
);
160+
}
161+
}

0 commit comments

Comments
 (0)