Skip to content
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

fix #1689: properly add/remove federation directives #1690

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

t1
Copy link
Collaborator

@t1 t1 commented Jan 8, 2023

No description provided.

t1 and others added 10 commits January 4, 2023 13:28
* maven-cache the github setup-java action

* fix smallrye#1670: make `@Key` and `@Provides` repeatable and allow directives to be repeatable. Update the federation annotation documentation and fix some deprecations.

* smallrye#1670: make `@Key` and `@Provides` repeatable and allow directives to be repeatable. Update the federation annotation documentation and fix some deprecations. (squashed)

* smallrye#1670: update documentation
@t1 t1 requested a review from jmartisk January 8, 2023 16:10
@jmartisk
Copy link
Member

jmartisk commented Jan 10, 2023

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
Copy link
Member

(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)

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.

4 participants