smallrye-graphql icon indicating copy to clipboard operation
smallrye-graphql copied to clipboard

fix #1689: properly add/remove federation directives

Open t1 opened this issue 2 years ago • 2 comments

t1 avatar Jan 08 '23 16:01 t1

Right we could potentially do this logic in SmallRye, but I don't see where you scan the index to decide whether Federation should be enabled or not. We need the schema-builder module (which is executed at build time) to decide whether federation should be enabled, and then somehow pass that info the runtime, because that is where we call Federation.transform (or not). This way you have it now, if I understand it correctly, calling Federation.transform would be based only on configuration, no auto detection (unless we do some extra stuff for that in Quarkus - which would defeat the purpose of this PR, no?)

How I imagine it could work:

build/deployment time:

  • In Quarkus, the SmallRyeGraphQLProcessor looks at the value of quarkus.smallrye-graphql.federation.enabled
    • if it's true, set smallrye.graphql.federation.enabled=true
    • if it's false, set smallrye.graphql.federation.enabled=false
    • if it's unspecified, don't set anything
  • (In WildFly, the first step is skipped, we just execute the SchemaBuilder right away)
  • Then, we call the SchemaBuilder
  • The SchemaBuilder will:
    • if smallrye.graphql.federation.enabled=true, always add federation directive types to the schema
    • if smallrye.graphql.federation.enabled=false, never add federation directive types to the schema
    • if smallrye.graphql.federation.enabled is unspecified, check the index whether there is at least one federation annotation in the app. If yes, add all federation directive types into the schema, AND set smallrye.graphql.federation.enabled to true or false accordingly
  • In Quarkus, if SmallRyeGraphQLProcessor sees that the smallrye.graphql.federation.enabled was set to true by SmallRye, it produces it again as a SystemPropertyBuildItem to make sure that the value is persisted at runtime. In WildFly, this is not necessary.

runtime:

  • The Bootstrap class calls Federation.transform or not based on the value of smallrye.graphql.federation.enabled. It doesn't have to care about annotations anymore at all, because if federation is disabled, then the directive types are not in the schema.

This, I think would be the way with the least amount of logic needed to be handled by WildFly/Quarkus (about 4 lines of code in Quarkus, and zero in WildFly), and with the least stuff that needs to be done during runtime. Does this make sense?

jmartisk avatar Jan 10 '23 08:01 jmartisk

(I also don't like that here you're always adding the directive types into the io.smallrye.graphql.schema.model.Schema to only later maybe skip them when transforming it to graphql.schema.GraphQLSchema at runtime - it would be more efficient to skip them right away at build time)

jmartisk avatar Jan 10 '23 08:01 jmartisk