feat(validation): reject directive definition cycles

[yaacovCR]

Adds a new NoDirectiveDefinitionCyclesRule SDL validation rule that detects cycles in the graph of directives used within directive definitions.

What is a directive definition cycle?

A cycle arises when following the references within directive definitions leads back to the same directive. Two kinds of references are tracked:

1. Directive argument types — if @foo takes an argument of type InputObject, and InputObject has a field with directive @foo, that forms a cycle: @foo → InputObject → @foo.
2. Directives on directive definitions — when the experimental experimentalDirectivesOnDirectiveDefinitions parser option is enabled, a directive applied directly to a directive definition or extension is also a reference. If @a is annotated with @b and @b is annotated with @a, that is a cycle: @a → @b → @a.

Cycles make it impossible to build a well-defined schema; the spec already forbids them, and experimental directive extensions do the same.

Key design question: should we consider existing schema applied directives?

In this initial attempt, when validating an SDL extension document against an existing schema, only references that appear in the document being validated are considered to close a cycle. Applied directives already persisted in the base schema are not counted as cycle-forming edges.

This means you can have a pre-existing schema like:

directive @a @b on DIRECTIVE_DEFINITION
directive @b on DIRECTIVE_DEFINITION

…where @a already carries @b in the stored schema, and then in your extension document write:

extend directive @b @a

This will not be reported as a cycle (@a → @b → @a), because the @b edge on @a comes from the original schema, not from the document under validation. The rule only errors when at least one edge in the detected cycle is introduced by the new document. This is because applied directives, while accessible via the stored definitions/extensions, do not actually form part of the existing schema object => at least, per the specification.

Is this the desired behavior? @BoD @graphql/tsc

[vercel]

@yaacovCR is attempting to deploy a commit to the The GraphQL Foundation Team on Vercel.

A member of the Team first needs to authorize it.

[BoD]

Thank you for looking into this and implementing the validation @yaacovCR!

About extensions: to me, and generally speaking, it would be surprising if a declaration that is invalid in the context of a schema could still be made valid via extensions. Unless this can already happen otherwise in the current spec, I would be in favor of avoiding that. Meaning: IMO it would make sense for the validation to also detect cycles introduced by extensions.

I you agree to that, do you also think that the current wording in the proposed spec ("Any directives provided must not contain the use of a Directive which references the previous directive directly") should be made more explicit to avoid an ambiguity here?

[yaacovCR]

I’m not sure we actually need a change in wording, after reconsidering for a little bit. Although applied directors currently don’t exist, that could change in the future, and we need to preserve the option for directives or a subset of them to be preserved and therefore not cyclical. Will make changes!