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

graphql java 22 update #1967

Merged
merged 5 commits into from
May 6, 2024
Merged

graphql java 22 update #1967

merged 5 commits into from
May 6, 2024

Conversation

samuelAndalon
Copy link
Contributor

@samuelAndalon samuelAndalon commented Apr 25, 2024

📝 Description

waiting for an official release of this PR graphql-java/graphql-java#3571

UPDATE:
next graphql-java update will be on July,
https://github.com/graphql-java/graphql-java/milestone/69

they helped us releasing from this branch
graphql-java/graphql-java#3571

which will unblock us.

https://github.com/graphql-java/graphql-java/releases/tag/v22.0

Copy link

github-actions bot commented Apr 25, 2024

Apollo Federation Subgraph Compatibility Results

Federation 1 Support Federation 2 Support
_service🟢
@key (single)🟢
@key (multi)🟢
@key (composite)🟢
repeatable @key🟢
@requires🟢
@provides🟢
federated tracing🟢
@link🟢
@shareable🟢
@tag🟢
@override🟢
@inaccessible🟢
@composeDirective🟢
@interfaceObject🟢

Learn more:

@dariuszkuc dariuszkuc added the changes: major Changes require a major version label May 6, 2024
@@ -71,7 +71,7 @@ type Query @extends

type Review {
body: String!
content: String @deprecated(reason : "no longer supported, replace with use Review.body instead")
content: String
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the change in schema?

Copy link
Contributor Author

@samuelAndalon samuelAndalon May 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the change in schema?

https://github.com/apollographql/federation-jvm/blob/main/graphql-java-support/src/main/java/com/apollographql/federation/graphqljava/printer/ServiceSDLPrinter.java#L33

the @deprecated directive is hidden from the ServiceSDLPrinter

i don't know why with the previous graphql-java version the test was passing though.

@@ -31,4 +31,4 @@ details on the supported configuration properties.
| graphql.subscriptions.keepAliveInterval | **Deprecated**. Keep the websocket alive and send a message to the client every interval in ms. Defaults to not sending messages | null |
| graphql.subscriptions.protocol | WebSocket based subscription protocol. Supported protocols: APOLLO_SUBSCRIPTIONS_WS / GRAPHQL_WS | GRAPHQL_WS |
| graphql.batching.enabled | Boolean flag indicating whether to enable custom dataloader instrumentations for 1 or more GraphQL Operations | false |
| graphql.batching.strategy | Configure which custom dataloader instrumentation will be used (LEVEL_DISPATCHED or SYNC_EXHAUSTION) | LEVEL_DISPATCHED |
| graphql.batching.strategy | Configure which custom dataloader instrumentation will be used | SYNC_EXHAUSTION |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we only have single data loader batching strategy do we really need this config?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would like to keep the door open for future dispatch mechanisms, SYNC_EXHAUSTION works but comes with a price, needs to hold state of each field, and memory usage could be a concern, in the future we could try to have a different dispatch mechanism that will use less memory maybe.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The keyword is "might" -> Unless there is something already in the works/roadmap I am unsure if there is ever going to be one so I'd just drop it and reintroduce it later on if needed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I find graphql.batching.enabled property confusing -> we always support batching so this only applies to dataloader config (as per comments). Wondering whether this should be deprecated in favor of something like graphql.loader.batching.strategy = SYNC_EXHAUSTION | NONE (NONE would imply current graphql.batching.enabled = false)

Copy link
Contributor Author

@samuelAndalon samuelAndalon May 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we always support batching

we added graphql.batching.enabled in order to opt out from the built-in "by level" graphql-java dataloader instrumentation.

https://github.com/ExpediaGroup/graphql-kotlin/blob/master/servers/graphql-kotlin-spring-server/src/main/kotlin/com/expediagroup/graphql/server/spring/GraphQLSchemaConfiguration.kt#L90

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah we added it originally for the custom data loaders -> just wondering whether we should revisit this property naming for the next major

Copy link
Collaborator

@dariuszkuc dariuszkuc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Data loader config properties can be fixed/dropped later if needed.

@samuelAndalon samuelAndalon merged commit bce5bc4 into master May 6, 2024
13 checks passed
@samuelAndalon samuelAndalon deleted the chore/graphql-java-update branch May 6, 2024 22:26
dariuszkuc pushed a commit that referenced this pull request Jun 20, 2024
### 📝 Description

Follow up to #1967 to
use an official graphql java release.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes: major Changes require a major version
Development

Successfully merging this pull request may close these issues.

2 participants