Disallow "true", "false", and "null" as enum values

[Yogu]

The specification disallows these in the SDL grammar definition, and using one of these enum values would result in a schema that would throw validation errors on creation.

Fixes #3221.

I also changed the type of EnumValueDefinitionNode.name from NameNode to EnumValueNode, so this is definitely breaking. We might even want to rename the name property to value, but that would be even more breaking.

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Jan Melcher (2e12fc8)

[IvanGoncharov]

@Yogu Thanks for the change 👍

Can you please make it more in line with what we already have for restricting fragment names:

graphql-js/src/language/parser.ts

Lines 498 to 506 in 6a5f51f

	/**
	* FragmentName : Name but not `on`
	*/
	parseFragmentName(): NameNode {
	if (this._lexer.token.value === 'on') {
	throw this.unexpected();
	}
	return this.parseName();
	}

No need for custom AST node type just apple restriction during parsing.

But please keep your custom error message, since it improves DX a lot.

[Yogu]

@IvanGoncharov

No need for custom AST node type just apple restriction during parsing.

It isn't a "custom" AST node type. EnumValueNode already existed and that's the name of the non-terminal used in the spec at the respective location. If we were to start fresh, it would probably be best to use EnumValueNode and rename name to value. Do you want to avoid the breaking change by continue using NameNode as the type for EnumValueDefinitionNode? I wouldn't have a problem with that, just checking that I understood you correctly.

[Yogu]

renamed this to parseEnumValueName() because it's no longer parsing into an EnumValue. but still having it in its own function keeps things clean.

[IvanGoncharov]

It isn't a "custom" AST node type. EnumValueNode already existed and that's the name of the non-terminal used in the spec at the respective location.

I checked the spec and you are completely right 👍

If we were to start fresh, it would probably be best to use EnumValueNode and rename name to value.

I think it's would be surprising behaviors, e.g. you wrote your visitor for EnumValue so most devs including me will assume you would be triggered only on literal enum value (inside arguments or inside default values).
Vise versa if you register a visitor on Name you expect to be called on all names.
I think using value inside literals and name inside definition is pretty intuitive.

But I agree that this creates a mismatch with the spec.
However, I'm not sure how to address it in spec.

[IvanGoncharov]

BTW. Adding a similar message to fragment name parsing is welcomed if you want to work on such PR.