-
Notifications
You must be signed in to change notification settings - Fork 65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
graphql-java-support: Remove federation directive and type definitions from SDL and use stricter SchemaGenerator checks #53
Conversation
7d76019
to
5860556
Compare
5860556
to
b566d92
Compare
b566d92
to
39c3136
Compare
2c50ade
to
467c19f
Compare
graphql-java-support/src/main/java/com/apollographql/federation/graphqljava/Federation.java
Outdated
Show resolved
Hide resolved
Follow-up item: Could we throw a dump of our |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Super happy we took the hit of maintaining our printer, smoothes out thorny problems.
…e definitions as required by federation spec
467c19f
to
1ac5a36
Compare
Problem
There are two general ways users can provide schemas to
federation-jvm
:TypeDefinitionRegistry
andRuntimeWiring
, which we pass throughSchemaGenerator
to generate aGraphQLSchema
. This is the usual route if you have SDL files/text.GraphQLSchema
. This is the route to use if they create their schema programmatically viaGraphQLSchema.newSchema()...
. They can also runSchemaGenerator
themselves on SDL files/text, and give us the resultingGraphQLSchema
.We print out their
GraphQLSchema
as federation SDL (that is, the SDL that needs to be returned byquery { _service { sdl } }
) usingSchemaPrinter
, not including any standard directive definitions (@deprecated
,@include
,@skip
).There are two general problems here:
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 providedGraphQLSchema
) will seefederation-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 withfederation-jvm
not presenting federation-compliant SDL to gateway.We run
SchemaGenerator
withSchemaGenerator.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 theTypeDefinitionRegistry
, 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 withgraphql-java
.Solution
This PR primarily does two things:
It changes
SchemaTransformer.sdl()
to removefederation directive definitions, along with thefederation directive definitions and federation type definitions. It also makes_FieldSet
scalar definition if it is unused by other typesSchemaTransformer.sdl()
public, which is useful for users of managed federation who wish to generate partial schema files forapollo service:push
/apollo service:check
(in particular because such schema files need to be federation SDL, and not standard GraphQL SDL).It changes
Federation.transform()
to add federation directive definitions to any providedTypeDefinitionRegistry
s that don't already define such directives, and changes ourSchemaGenerator
invocation to useSchemaGenerator.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 usegraphql.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 ingraphql-java
's traverser and (possibly) due to bugs in their zipper-handling intransformSchema()
(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), wheretransformSchema()
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 usingtransformSchema()
, and also allows us to apply this PR to graphql-java v13 (notetransformSchema()
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 untilgraphql-java
releases the braces fix, andgraphql-java
'sSchemaPrinter
allows native definition filtering, or federation spec changes to allow federation definitions, ortransformSchema()
has its bugs fixedit's the simplest option here.