Skip to content

Run directive wirings on the assembled schema - #895

Open
oryan-block wants to merge 4 commits into
masterfrom
bugfix/875
Open

oryan-block wants to merge 4 commits into
masterfrom
bugfix/875

Conversation

@oryan-block

Copy link
Copy Markdown
Collaborator

Fixes #875

Checklist

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

Description

A SchemaDirectiveWiring on a recursive input field got a GraphQLTypeReference as the field's type instead of the input object:

directive @check on INPUT_FIELD_DEFINITION

type Query {
    trees(filter: Tree): Int
}

input Tree {
    and: [Tree!] @check
    name: String @check
}

So GraphQLTypeUtil.unwrapAll(element.type) in onInputObjectField threw ClassCastException: GraphQLTypeReference cannot be cast to GraphQLUnmodifiedType and the schema didn't build. Types are immutable, so one field of a cycle has to stay a reference while the types are built, and we ran the wirings right then.

graphql-java runs them only once the schema is assembled, with SchemaTransformer, so its wirings never see references. This does the same: the create* methods in SchemaParser no longer wire anything, and parseSchemaObjects builds the schema, runs the wirings over it (DirectiveWiringHelper#wireSchema) and copies the wired types back into SchemaObjects. Only when a wiring is registered, without one nothing changes.

SchemaTransformer can only rebuild types whose cycles go through a type reference, so a couple of things had to follow:

  • Directive definitions now point at input objects by name, like they already did for enums. Otherwise input Filter @described @example(value: { name: "a" }) with directive @example(value: Filter) contains itself through its own applied directive, and a wiring changing Filter failed with "All types within a GraphQL schema must have unique names". It also means the two-step SchemaDirectives build is gone, it only existed so wirings saw real input objects in directive arguments.
  • When a wiring changes a type, its fields point at the schema's object, interface, union and input types by name again, including inside types the wiring builds in code. Otherwise a wiring doing it.type(GraphQLNonNull.nonNull(element.type)) on friend: Person in type Person fails with "This schema is not forming an Directed Acyclic Graph". graphql-java's own SchemaGenerator fails on that one, but it worked here before, so I kept it working.
  • Types without an SDL definition (introspection, types a wiring builds in code) aren't wired, same as graphql-java.
  • The unmapped input field check now runs on the wired input objects, so fields a wiring removes aren't reported.

Release notes material, these only apply when a wiring is registered unless said otherwise:

  • Wirings see the real types everywhere, not just on recursive inputs. Before, every output field pointing at an object, interface, union or enum, and every input field of an enum type, was a GraphQLTypeReference too.
  • Wirings run in graphql-java's traversal order instead of input objects, interfaces, objects, unions, enums. A field or argument wiring now sees the input objects before they're wired.
  • The schema is built and validated before the wirings run, so an invalid schema throws InvalidSchemaException from parseSchemaObjects() and a wiring can't make an invalid schema valid anymore.
  • Building is slower with wirings, since the schema is built twice plus the transform. In a reviewer's probe with synthetic, densely connected schemas, 150 types went from ~12ms to ~40ms and 1500 types from ~0.1s to ~1.5s. edealer registers no SchemaDirectiveWiring so it's not affected.
  • Without wirings too: input object argument types of directive definitions and applied directives are type references in SchemaObjects until the schema is built. toSchema() resolves them, so this only matters if you read parseSchemaObjects() directly.
  • The unmapped input field check reports after the other schema errors now, since it runs at the end.

Tests: the #843 test in SchemaParserTest asserted that the recursive field stays a type reference, now it's the input object. New cases in DirectiveTest for the directive cycle, for wirings changing self-referencing field types (output, input and through a union) and building a type in code, and one in UnmappedInputFieldsTest for a field removed by a wiring. Each of them failed on an earlier version of this branch, and the directive cycle one passes on master.

mvn clean verify passes on JDK 25 and on JDK 11 against graphql-java 25.1. Also checked through the public API with the issue's schema: master fails with the ClassCastException, this branch builds and trees returns 4. A second schema with all the shapes above builds on master and gives an identical printed schema and results here, and it also builds and runs on graphql-java 25.1. The existing directives probe scenario is identical too.

…cycles can be rebuilt

Also skip types without a definition when wiring, and check unmapped input fields on the wired input objects.
…change self-referencing field types

Also drop the input objects parameter of determineType, which can no longer match.
@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.

Directive wirings see a type reference on recursive input fields

1 participant