diff --git a/CHANGELOG.md b/CHANGELOG.md index bed767cae..917d5db86 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,7 @@ You can find and compare releases at the [GitHub release page](https://github.co - Expose structured enumeration of directive locations - Add `AST::concatAST()` utility - Allow lazy input object fields +- Add validation rule `UniqueEnumValueNames` ### Optimized diff --git a/src/Language/VisitorOperation.php b/src/Language/VisitorOperation.php index 231626166..20dc8337e 100644 --- a/src/Language/VisitorOperation.php +++ b/src/Language/VisitorOperation.php @@ -6,12 +6,9 @@ class VisitorOperation { - /** @var bool */ - public $doBreak; + public bool $doBreak = false; - /** @var bool */ - public $doContinue; + public bool $doContinue = false; - /** @var bool */ - public $removeNode; + public bool $removeNode = false; } diff --git a/src/Type/SchemaValidationContext.php b/src/Type/SchemaValidationContext.php index b3737bd6d..3f415d7dc 100644 --- a/src/Type/SchemaValidationContext.php +++ b/src/Type/SchemaValidationContext.php @@ -9,7 +9,6 @@ use GraphQL\Language\AST\DirectiveNode; use GraphQL\Language\AST\EnumTypeDefinitionNode; use GraphQL\Language\AST\EnumTypeExtensionNode; -use GraphQL\Language\AST\EnumValueDefinitionNode; use GraphQL\Language\AST\FieldDefinitionNode; use GraphQL\Language\AST\InputObjectTypeDefinitionNode; use GraphQL\Language\AST\InputObjectTypeExtensionNode; @@ -958,15 +957,6 @@ private function validateEnumValues(EnumType $enumType): void foreach ($enumValues as $enumValue) { $valueName = $enumValue->name; - // Ensure no duplicates - $allNodes = $this->getEnumValueNodes($enumType, $valueName); - if (count($allNodes) > 1) { - $this->reportError( - sprintf('Enum type %s can include value %s only once.', $enumType->name, $valueName), - $allNodes - ); - } - // Ensure valid name. $this->validateName($enumValue); if ($valueName === 'true' || $valueName === 'false' || $valueName === 'null') { @@ -988,29 +978,6 @@ private function validateEnumValues(EnumType $enumType): void } } - /** - * @return array - */ - private function getEnumValueNodes(EnumType $enum, string $valueName): array - { - $allNodes = $enum->astNode !== null - ? array_merge([$enum->astNode], $enum->extensionASTNodes) - : $enum->extensionASTNodes; - - $values = []; - foreach ($allNodes as $node) { - foreach ($node->values as $value) { - if ($value->name->value !== $valueName) { - continue; - } - - $values[] = $value; - } - } - - return $values; - } - private function validateInputFields(InputObjectType $inputObj): void { $fieldMap = $inputObj->getFields(); diff --git a/src/Utils/SchemaExtender.php b/src/Utils/SchemaExtender.php index 31e9358df..4ae2953a0 100644 --- a/src/Utils/SchemaExtender.php +++ b/src/Utils/SchemaExtender.php @@ -20,7 +20,6 @@ use GraphQL\Type\Definition\CustomScalarType; use GraphQL\Type\Definition\Directive; use GraphQL\Type\Definition\EnumType; -use GraphQL\Type\Definition\EnumValueDefinition; use GraphQL\Type\Definition\FieldArgument; use GraphQL\Type\Definition\ImplementingType; use GraphQL\Type\Definition\InputObjectType; @@ -215,14 +214,9 @@ protected static function extendInputFieldMap(InputObjectType $type): array protected static function extendEnumValueMap(EnumType $type): array { $newValueMap = []; - /** @var array $oldValueMap */ - $oldValueMap = []; - foreach ($type->getValues() as $value) { - $oldValueMap[$value->name] = $value; - } - foreach ($oldValueMap as $key => $value) { - $newValueMap[$key] = [ + foreach ($type->getValues() as $value) { + $newValueMap[$value->name] = [ 'name' => $value->name, 'description' => $value->description, 'value' => $value->value, @@ -239,12 +233,7 @@ protected static function extendEnumValueMap(EnumType $type): array */ foreach (static::$typeExtensionsMap[$type->name] as $extension) { foreach ($extension->values as $value) { - $valueName = $value->name->value; - if (isset($oldValueMap[$valueName])) { - throw new Error('Enum value "' . $type->name . '.' . $valueName . '" already exists in the schema. It cannot also be defined in this type extension.', [$value]); - } - - $newValueMap[$valueName] = static::$astBuilder->buildEnumValue($value); + $newValueMap[$value->name->value] = static::$astBuilder->buildEnumValue($value); } } } diff --git a/src/Validator/DocumentValidator.php b/src/Validator/DocumentValidator.php index 80e7638bc..0f9d83d36 100644 --- a/src/Validator/DocumentValidator.php +++ b/src/Validator/DocumentValidator.php @@ -36,6 +36,7 @@ use GraphQL\Validator\Rules\SingleFieldSubscription; use GraphQL\Validator\Rules\UniqueArgumentNames; use GraphQL\Validator\Rules\UniqueDirectivesPerLocation; +use GraphQL\Validator\Rules\UniqueEnumValueNames; use GraphQL\Validator\Rules\UniqueFragmentNames; use GraphQL\Validator\Rules\UniqueInputFieldNames; use GraphQL\Validator\Rules\UniqueOperationNames; @@ -201,6 +202,7 @@ public static function sdlRules() KnownArgumentNamesOnDirectives::class => new KnownArgumentNamesOnDirectives(), UniqueDirectivesPerLocation::class => new UniqueDirectivesPerLocation(), UniqueArgumentNames::class => new UniqueArgumentNames(), + UniqueEnumValueNames::class => new UniqueEnumValueNames(), UniqueInputFieldNames::class => new UniqueInputFieldNames(), ProvidedRequiredArgumentsOnDirectives::class => new ProvidedRequiredArgumentsOnDirectives(), ]; diff --git a/src/Validator/Rules/UniqueEnumValueNames.php b/src/Validator/Rules/UniqueEnumValueNames.php new file mode 100644 index 000000000..93a03c92b --- /dev/null +++ b/src/Validator/Rules/UniqueEnumValueNames.php @@ -0,0 +1,70 @@ +> $knownValueNames */ + $knownValueNames = []; + + /** + * @param EnumTypeDefinitionNode|EnumTypeExtensionNode $enum + */ + $checkValueUniqueness = static function ($enum) use ($context, &$knownValueNames): VisitorOperation { + $typeName = $enum->name->value; + + $schema = $context->getSchema(); + $existingType = $schema !== null + ? $schema->getType($typeName) + : null; + + $valueNodes = $enum->values; + + if (! isset($knownValueNames[$typeName])) { + $knownValueNames[$typeName] = []; + } + + $valueNames = &$knownValueNames[$typeName]; + + foreach ($valueNodes as $valueDef) { + $valueNameNode = $valueDef->name; + $valueName = $valueNameNode->value; + + if ($existingType instanceof EnumType && $existingType->getValue($valueName) !== null) { + $context->reportError(new Error( + "Enum value \"${typeName}.${valueName}\" already exists in the schema. It cannot also be defined in this type extension.", + $valueNameNode + )); + } elseif (isset($valueNames[$valueName])) { + $context->reportError(new Error( + "Enum value \"${typeName}.${valueName}\" can only be defined once.", + [$valueNames[$valueName], $valueNameNode] + )); + } else { + $valueNames[$valueName] = $valueNameNode; + } + } + + return Visitor::skipNode(); + }; + + return [ + NodeKind::ENUM_TYPE_DEFINITION => $checkValueUniqueness, + NodeKind::ENUM_TYPE_EXTENSION => $checkValueUniqueness, + ]; + } +} diff --git a/tests/Type/ValidationTest.php b/tests/Type/ValidationTest.php index 501c59e2c..9441639b7 100644 --- a/tests/Type/ValidationTest.php +++ b/tests/Type/ValidationTest.php @@ -1093,32 +1093,6 @@ enum SomeEnum ); } - /** - * @see it('rejects an Enum type with duplicate values') - */ - public function testRejectsAnEnumTypeWithDuplicateValues(): void - { - $schema = BuildSchema::build(' - type Query { - field: SomeEnum - } - - enum SomeEnum { - SOME_VALUE - SOME_VALUE - } - '); - $this->assertMatchesValidationMessage( - $schema->validate(), - [ - [ - 'message' => 'Enum type SomeEnum can include value SOME_VALUE only once.', - 'locations' => [['line' => 7, 'column' => 9], ['line' => 8, 'column' => 9]], - ], - ] - ); - } - public function testDoesNotAllowIsDeprecatedWithoutDeprecationReasonOnEnum(): void { $enum = new EnumType([ diff --git a/tests/Utils/SchemaExtenderTest.php b/tests/Utils/SchemaExtenderTest.php index b5587b8f3..dd70bf741 100644 --- a/tests/Utils/SchemaExtenderTest.php +++ b/tests/Utils/SchemaExtenderTest.php @@ -1392,25 +1392,6 @@ public function testDoesNotAllowReplacingADefaultDirective(): void } } - /** - * @see it('does not allow replacing an existing enum value') - */ - public function testDoesNotAllowReplacingAnExistingEnumValue(): void - { - $sdl = ' - extend enum SomeEnum { - ONE - } - '; - - try { - $this->extendTestSchema($sdl); - self::fail(); - } catch (Error $error) { - self::assertEquals('Enum value "SomeEnum.ONE" already exists in the schema. It cannot also be defined in this type extension.', $error->getMessage()); - } - } - /** * @see it('does not automatically include common root type names') */ diff --git a/tests/Validator/UniqueEnumValueNamesTest.php b/tests/Validator/UniqueEnumValueNamesTest.php new file mode 100644 index 000000000..0c87de424 --- /dev/null +++ b/tests/Validator/UniqueEnumValueNamesTest.php @@ -0,0 +1,283 @@ +expectValidSDL( + new UniqueEnumValueNames(), + ' + enum SomeEnum + ' + ); + } + + /** + * @see it('one value') + */ + public function testOneValue(): void + { + $this->expectValidSDL( + new UniqueEnumValueNames(), + ' + enum SomeEnum { + FOO + } + ' + ); + } + + /** + * @see it('multiple values') + */ + public function testMultipleValues(): void + { + $this->expectValidSDL( + new UniqueEnumValueNames(), + ' + enum SomeEnum { + FOO + BAR + } + ' + ); + } + + /** + * @see it('duplicate values inside the same enum definition') + */ + public function testDuplicateValuesInsideTheSameEnumDefinition(): void + { + $this->expectSDLErrorsFromRule( + new UniqueEnumValueNames(), + ' + enum SomeEnum { + FOO + BAR + FOO + } + ', + null, + [ + [ + 'message' => 'Enum value "SomeEnum.FOO" can only be defined once.', + 'locations' => [ + ['line' => 3, 'column' => 9], + ['line' => 5, 'column' => 9], + ], + ], + ], + ); + } + + /** + * @see it('extend enum with new value') + */ + public function testExtendEnumWithNewValue(): void + { + $this->expectValidSDL( + new UniqueEnumValueNames(), + ' + enum SomeEnum { + FOO + } + extend enum SomeEnum { + BAR + } + extend enum SomeEnum { + BAZ + } + ' + ); + } + + /** + * @see it('extend enum with duplicate value') + */ + public function testExtendEnumWithDuplicateValue(): void + { + $this->expectSDLErrorsFromRule( + new UniqueEnumValueNames(), + ' + extend enum SomeEnum { + FOO + } + enum SomeEnum { + FOO + } + ', + null, + [ + [ + 'message' => 'Enum value "SomeEnum.FOO" can only be defined once.', + 'locations' => [ + ['line' => 3, 'column' => 9], + ['line' => 6, 'column' => 9], + ], + ], + ], + ); + } + + /** + * @see it('duplicate value inside extension') + */ + public function testDuplicateValueInsideExtension(): void + { + $this->expectSDLErrorsFromRule( + new UniqueEnumValueNames(), + ' + enum SomeEnum + extend enum SomeEnum { + FOO + BAR + FOO + } + ', + null, + [ + [ + 'message' => 'Enum value "SomeEnum.FOO" can only be defined once.', + 'locations' => [ + ['line' => 4, 'column' => 9], + ['line' => 6, 'column' => 9], + ], + ], + ], + ); + } + + /** + * @see it('duplicate value inside different extensions') + */ + public function testDuplicateValueInsideDifferentExtensions(): void + { + $this->expectSDLErrorsFromRule( + new UniqueEnumValueNames(), + ' + enum SomeEnum + extend enum SomeEnum { + FOO + } + extend enum SomeEnum { + FOO + } + ', + null, + [ + [ + 'message' => 'Enum value "SomeEnum.FOO" can only be defined once.', + 'locations' => [ + ['line' => 4, 'column' => 9], + ['line' => 7, 'column' => 9], + ], + ], + ], + ); + } + + /** + * @see it('adding new value to the type inside existing schema') + */ + public function testAddingNewValueToTheTypeInsideExistingSchema(): void + { + $schema = BuildSchema::build('enum SomeEnum'); + $sdl = ' + extend enum SomeEnum { + FOO + } + '; + + $this->expectValidSDL( + new UniqueEnumValueNames(), + $sdl, + $schema, + ); + } + + /** + * @see it('adding conflicting value to existing schema twice') + */ + public function testAddingConflictingValueToExistingSchemaTwice(): void + { + $schema = BuildSchema::build(' + enum SomeEnum { + FOO + } + '); + $sdl = ' + extend enum SomeEnum { + FOO + } + extend enum SomeEnum { + FOO + } + '; + + $this->expectSDLErrorsFromRule( + new UniqueEnumValueNames(), + $sdl, + $schema, + [ + [ + 'message' => 'Enum value "SomeEnum.FOO" already exists in the schema. It cannot also be defined in this type extension.', + 'locations' => [ + ['line' => 3, 'column' => 9], + ], + ], + [ + 'message' => 'Enum value "SomeEnum.FOO" already exists in the schema. It cannot also be defined in this type extension.', + 'locations' => [ + ['line' => 6, 'column' => 9], + ], + ], + ], + ); + } + + /** + * @see it('adding enum values to existing schema twice') + */ + public function testAddingEnumValuesToExistingSchemaTwice(): void + { + $schema = BuildSchema::build(' + enum SomeEnum + '); + $sdl = ' + extend enum SomeEnum { + FOO + } + extend enum SomeEnum { + FOO + } + '; + + $this->expectSDLErrorsFromRule( + new UniqueEnumValueNames(), + $sdl, + $schema, + [ + [ + 'message' => 'Enum value "SomeEnum.FOO" can only be defined once.', + 'locations' => [ + ['line' => 3, 'column' => 9], + ['line' => 6, 'column' => 9], + ], + ], + ], + ); + } +}