Skip to content

Warn about directives applied where their definition doesn't allow them - #885

Open
oryan-block wants to merge 1 commit into
masterfrom
feature/225-validate-directives
Open

oryan-block wants to merge 1 commit into
masterfrom
feature/225-validate-directives

Conversation

@oryan-block

Copy link
Copy Markdown
Collaborator

Fixes #225

Checklist

  • Pull requests follows the contribution guide
  • New or modified functionality is covered by tests

Description

Three kinds of wrong directive usage build without a word today:

directive @uppercase on OBJECT
directive @auth(role: String!) on FIELD_DEFINITION
directive @cached(ttl: Int) on FIELD_DEFINITION

type Query {
    motto: String @uppercase                     # declared on OBJECT, applied to a field
    secret: String @auth                         # required argument missing
    members: [Member!] @cached @cached(ttl: 10)  # not repeatable, applied twice
}

graphql-java's SchemaGenerator rejects all three, but tools doesn't use it. The schema validation that does run on our schema doesn't catch them either: it checks repeats only on the deprecated GraphQLDirectives, which buildDirectives already de-dupes, and it never checks locations or missing arguments.

These schemas always built, so for now they log a WARN and keep building as before. Throwing instead is #884, for 15.0.0:

Directive @uppercase is applied to field 'Query.motto', but it's declared on OBJECT, not FIELD_DEFINITION.
Directive @auth is applied to field 'Query.secret' without its required argument 'role'.
Directive @cached isn't repeatable but is applied more than once to field 'Query.members'.

The checks live in buildAppliedDirectives, which now gets the location and a name for the element (type 'Book', argument 'id' of field 'Query.book', the schema, ...). They run against the GraphQLDirective definition, so built-in directives are checked too (@deprecated on a type warns). Undeclared directives allowed by allowUndeclaredDirectives have nothing to check against and are skipped. A repeat that a type extension adds is still handled by the existing warning in directivesOf, so it isn't reported twice.

Every element used to call withAppliedDirectives(*buildAppliedDirectives(..)) followed by withDirectives(*buildDirectives(.., LOCATION)), 10 times. That's now one withDirectivesAt(directives, LOCATION, element) per site, so the location is written once. The directive definition arguments were passing OBJECT there, which is now ARGUMENT_DEFINITION. That only changes the location on the deprecated GraphQLDirectives attached to those arguments, and nothing in tools reads it. createField and createArgument get the type name for the messages.

Two test fixtures warned: DirectiveTest's @uppercase on OBJECT applied to a field, and RelayConnection.graphqls's @connection(for: String!) on FIELD. FIELD is a location for queries, not for the schema, so both now use FIELD_DEFINITION. The relay docs don't show a declaration, so I'd guess some users copied the on FIELD one from our tests. They'll start seeing the warning.

Added a DirectiveTest case with all three checks, a built-in directive and the schema, asserting the exact warnings and that the repeats are still kept. The default values test now asserts no warnings, which covers the schema, enum value, input field and argument sites. TypeExtensionTest's exact warning list didn't change, which covers the type sites and the extension repeat. I also traced every check across the whole suite: only the two fixtures above warned. mvn clean verify passes on JDK 25, on JDK 25 against graphql-java 25.1, and on JDK 11.

Also checked through the public API with a schema like the one above: master logs nothing, this branch logs the three warnings (plus @deprecated on a type and a repeated schema directive). The printed schema and the query result are identical on both, and @uppercase still uppercases through its wiring. graphql-java's own checker finds no directive problems in a large real schema (107 SDL files), so I don't expect much noise.

Release notes: schemas that misuse directives now log a WARN from graphql.kickstart.tools.SchemaParser. For @connection, declare it on FIELD_DEFINITION.

Logs a warning for a directive applied at a location its definition doesn't
list, without a required argument, or more than once when it isn't repeatable.
The schema still builds as before. Arguments of directive definitions now get
the ARGUMENT_DEFINITION location instead of OBJECT.

Fixes #225
@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

No way to validate SDL-defined directives

1 participant