graphql-java-support: Remove federation directive and type definitions from SDL and use stricter SchemaGenerator checks

[sachindshinde]

Problem

There are two general ways users can provide schemas to federation-jvm:

1. They can provide a TypeDefinitionRegistry and RuntimeWiring, which we pass through SchemaGenerator to generate a GraphQLSchema. This is the usual route if you have SDL files/text.
2. They can directly provide a GraphQLSchema. This is the route to use if they create their schema programmatically via GraphQLSchema.newSchema().... They can also run SchemaGenerator themselves on SDL files/text, and give us the resulting GraphQLSchema.

We print out their GraphQLSchema as federation SDL (that is, the SDL that needs to be returned by query { _service { sdl } }) using SchemaPrinter, not including any standard directive definitions (@deprecated, @include, @skip).

There are two general problems here:

1. Federation SDL isn't valid GraphQL SDL, as per federation spec. The more concrete issue is that we can't print out directive definitions for federation directives (e.g. @key) in federation SDL without causing gateway composition to error. And currently, we don't check for or remove such federation directive definitions from user-provided schemas.

   This means that users attempting to include federation directive definitions in their provided TypeDefinitionRegistry (or their provided GraphQLSchema) will see federation-jvm happily accept their schema and they'll be able to startup their GraphQL server just fine. But they'll run into an error when they talk to gateway, and they might not be able to guess so easily that it's an issue with federation-jvm not presenting federation-compliant SDL to gateway.

2. We run SchemaGenerator with SchemaGenerator.Options.enforceSchemaDirectives(false), which means that we don't check whether they use their directives properly at all. They may have used directives in invalid locations, or used directives with invalid names or invalid arguments, but we wouldn't catch those mistakes at all because of those settings. As far as I can tell, the only reason we do this is because enabling the checks would mean we have to add federation directive definitions into the TypeDefinitionRegistry, which (as mentioned earlier) would cause gateway composition to error.

   The user's only recourse is to validate their own schema separately while including the federation directive definitions, and pass federation-jvm a copy of their schema with the federation directive definitions removed. It's not particularly obvious that a user should need to do this, nor is it particularly simple to do for someone who's just starting out with graphql-java.

Solution

This PR primarily does two things:

1. It changes SchemaTransformer.sdl() to remove federation directive definitions, along with the _FieldSet scalar definition if it is unused by other types federation directive definitions and federation type definitions. It also makes SchemaTransformer.sdl() public, which is useful for users of managed federation who wish to generate partial schema files for apollo service:push/apollo service:check (in particular because such schema files need to be federation SDL, and not standard GraphQL SDL).

2. It changes Federation.transform() to add federation directive definitions to any provided TypeDefinitionRegistrys that don't already define such directives, and changes our SchemaGenerator invocation to use SchemaGenerator.Options.enforceSchemaDirectives(true) to properly verify directive usages.

Reviewer Notes

So initially I tried to just remove the directive definitions by removing them from GraphQLSchema.directives, and use graphql.schema.SchemaTransformer.transformSchema() to change directive usages to not reference federation type definitions. The first part of this worked, but the second part of this did not, due to a bug in graphql-java's traverser and (possibly) due to bugs in their zipper-handling in transformSchema() (I say possibly because it might just be I've misunderstood the algorithm). It would work for the simple schemas in our tests, but not for more complex real-life examples (e.g. the schema in Apollo's infra), where transformSchema() would throw NPEs.

I've opted to implement both directive definition and type definition hiding in FederationSdlPrinter instead. This has the advantage of being much cleaner and easier to reason about than using transformSchema(), and also allows us to apply this PR to graphql-java v13 (note transformSchema() is a new v14 feature).

This has the disadvantage of forcing us to maintain this printer and updating it when we change graphql-java versions, but until

• graphql-java releases the braces fix, and
• graphql-java's SchemaPrinter allows native definition filtering, or federation spec changes to allow federation definitions, or transformSchema() has its bugs fixed

it's the simplest option here.

[pcarrier]

The first part of this worked, but the second part of this did not, due to a bug in graphql-java's traverser and (possibly) due to bugs in their zipper-handling in transformSchema() (I say possibly because it might just be I've misunderstood the algorithm). It would work for the simple schemas in our tests, but not for more complex real-life examples (e.g. the schema in Apollo's infra), where transformSchema() would throw NPEs.

Follow-up item: Could we throw a dump of our kotlin SDL in this repo as a fixture for tests?

[pcarrier]

👍 Super happy we took the hit of maintaining our printer, smoothes out thorny problems.