add validation of directives in SDL

[leebyron]

From #1383:

It's very critical to add validation of directives in SDL since as noted here no one validates them correctly at the moment. That will lead to a situation very similar to the recent Allow interfaces to have no implementors. Since spec defines set of validation rules but no one implements them. Probably we shouldn't enforce Directives Are Unique Per Location until graphql/graphql-spec#429 resolved but all other validation should be implemented e.g. require the schema to contain applied directives, correct directive arguments, etc.

[mjmahone]

Currently working on a PR for this issue. I am going to make it more strict, for now. So we will validate all of:

• directives are not defined more than once
• directives are only used once per location in the schema
• directives used on the schema are defined
• directives are used at a valid location: can't use a SCHEMA directive on a type definition.

My plan is to add this within the validateSchema function, as we already are doing a minor bit of directive validation there.

[IvanGoncharov]

I'm also working on it for a couple of weeks and I tried a few different approaches. Currently, I'm working on creating validateSDL and an accompanying set of rules. Idea is to share the majority of code with validate function and reuse some of the existing rules. For example:

directives are only used once per location in the schema

https://github.com/graphql/graphql-js/blob/master/src/validation/rules/UniqueDirectivesPerLocation.js

A few benefits:

• Can be reused in language-services
• Can work on incomplete SDL (validateSchema will fail if you didn't define query type).
• Can add/remove rules
• Allows to move error checking from buildSchema/extendSchema into validation rules and this allows to speed up schema building from already validated SDL:

  graphql-js/src/utilities/extendSchema.js

  Lines 596 to 637 in e124ca8

  	function checkExtensionNode(type, node) {
  	switch (node.kind) {
  	case Kind.OBJECT_TYPE_EXTENSION:
  	if (!isObjectType(type)) {
  	throw new GraphQLError(
  	`Cannot extend non-object type "${type.name}".`,
  	[node],
  	);
  	}
  	break;
  	case Kind.INTERFACE_TYPE_EXTENSION:
  	if (!isInterfaceType(type)) {
  	throw new GraphQLError(
  	`Cannot extend non-interface type "${type.name}".`,
  	[node],
  	);
  	}
  	break;
  	case Kind.ENUM_TYPE_EXTENSION:
  	if (!isEnumType(type)) {
  	throw new GraphQLError(`Cannot extend non-enum type "${type.name}".`, [
  	node,
  	]);
  	}
  	break;
  	case Kind.UNION_TYPE_EXTENSION:
  	if (!isUnionType(type)) {
  	throw new GraphQLError(`Cannot extend non-union type "${type.name}".`, [
  	node,
  	]);
  	}
  	break;
  	case Kind.INPUT_OBJECT_TYPE_EXTENSION:
  	if (!isInputObjectType(type)) {
  	throw new GraphQLError(
  	`Cannot extend non-input object type "${type.name}".`,
  	[node],
  	);
  	}
  	break;
  	}
  	}

@mjmahone What do you think about this aproach?
I will try to publish WIP PR tomorrow.

[mjmahone]

@IvanGoncharov that sounds like you're further along than me. I want basically everything you described, but I am willing to take a shortcut and just extend validateSchema to get this out sooner. I've got all the validation (I think) working as-is (putting up a PR imminently).

On thing is that I really want to have us push 14.0.0 as soon as possible. I keep reaching for the "schema directive" tool and then remembering "oh right, that won't be available until we release 14." I don't think my work would be difficult to undo, though, so we could always publish a 14.1 where we switch validateSchema to be run purely on the schema AST nodes.

[IvanGoncharov]

I don't think my work would be difficult to undo, though, so we could always publish a 14.1 where we switch validateSchema to be run purely on the schema AST nodes.

validateSchema is part of public API and people using it:
https://github.com/search?q=graphql+validateSchema+-filename%3AvalidateSchema&type=Code
Also some 3rd-party implementations synchronise their codebase with graphql-js after each release, e.g. graphql-php.
It's not a super big problem but still moving functionality from one public function to another is a breaking change.

Can you wait until monday morning your time?
I will limit scope of my PR to two checks you implemented in your PR and it should take me 1-2 days together with tests.

[mjmahone]

Yup I will wait. I would much rather have validation that can be run on the SDL AST than on an already-created schema, and like that this validation would mirror what we already do with validate.

@IvanGoncharov if there's a branch I could work on top of, or if you want me to create that branch, I might be able to add more validation, or move more validation from validateSchema to individual SDL validation rules.

[IvanGoncharov]

if there's a branch I could work on top of, or if you want me to create that branch, I might be able to add more validation, or move more validation from validateSchema to individual SDL validation rules.

I'm trying to figure out if it's possible to reuse the same context for buildSchema and extendSchema since they are totally different use cases so I'm mostly figuring API for ValidationContext. I will reuse tests from your PR and I will try to push WIP PR after I have stable API to get feedback from you.

[IvanGoncharov]

@mjmahone Update: I finally figured out how to solve the last major problem.
I will open a couple of PRs implementing this feature in the next few hours.

[mjmahone]

@IvanGoncharov is there more SDL validation you think we need prior to the 14.0 release? If so, anything I can do to help push it forward? I'd like us to put up at least an RC release by Monday if possible.

[IvanGoncharov]

@mjmahone Sorry for the silence. I'm working on a PR that will add a few more rules.
I will push hard to publish it until Sunday morning your time so we can discuss the next steps.

I'd like us to put up at least an RC release by Monday if possible.

Monday seems reasonable due date for RC release 👍

[Hendrixer]

@IvanGoncharov not sure what other validations you have in mind. But validating input types on Directive args would be nice. Down to help with this if needed. Thanks for the hard work. 💯

[mjmahone]

@IvanGoncharov I think with #1463, this release is ready for at least a public RC? If so, I can put up a PR to bump the version tomorrow. I should also be able to do the upgrade across Relay's packages as well, although we might wait to actually bump the Relay version (@kassens would know that process better than me).

[IvanGoncharov]

But validating input types on Directive args would be nice.

@Hendrixer You absolutely right, we still don't validate directives fully since this rule doesn't support SDL:
https://github.com/graphql/graphql-js/blob/master/src/validation/rules/ValuesOfCorrectType.js

But I think it less important and we could add this support later minor release.
Since breaking change is not about fully validating directives but about forcing developers to actually define them.
At the moment we validating

• known directives in the correct locations
• known arguments
• presence of all required args
• forbid to duplicate directives on the same location

Down to help with this if needed.

It would be great if you try current master:
https://github.com/graphql/graphql-js#want-to-ride-the-bleeding-edge
And provide feedback on it.
Here is an example of how directives could be specified:

graphql-js/src/utilities/__tests__/extendSchema-test.js

Lines 1292 to 1303 in 4631744

	let schema = new GraphQLSchema({
	directives: [FooDirective],
	types: [FooType],
	});
	expect(schema.getQueryType()).to.equal(undefined);
	const ast = parse(`
	schema @foo {
	query: Foo
	}
	`);
	schema = extendSchema(schema, ast);

@mjmahone In the last few weeks, I also discover a couple of pretty small issues that technically a breaking change. They all about removing a few obscure features so I will try to make PRs in a next few hours.

[eurostar-fennec-cooper]

Is this feature still being considered? I ask because it's mentioned in ardatan/graphql-tools#920. It's hard to trust/rely on directives when a graphql server can be started without directive functions and the directives in the SDL get skipped over/ignored.

[yaacovCR]

I believe directive usage is validated now. Your issue over at graphql-tools ardatan/graphql-tools#920 (comment) suggests that you have a slightly different ask, see my response over there

[yaacovCR]

Well, validated in a sense that the directives have to exist, I don't know if the arguments are validated / coerced yet...

[eurostar-fennec-cooper]

Thanks for the help, all I want is to ensure that the directive functions exist, although yes I believe that the original author of the issue on graphql-tools wants to verify that the implementation is also correct.

I've been trying this with 15.3.0 and 14.5.8 and finding that the directive functions don't have to exist, but I'll give it another go.

[yuchenshi]

Any updates on validating the argument types?

https://spec.graphql.org/October2021/#sec-Values-of-Correct-Type seems to suggest that all literals should be validated, not just in executable. Shall we extend ValuesOfCorrectTypeRule.ts to SDL?