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 Non blocking support #25194

Merged
merged 1 commit into from May 5, 2022
Merged

GraphQL Non blocking support #25194

merged 1 commit into from May 5, 2022

Conversation

phillip-kruger
Copy link
Member

@phillip-kruger phillip-kruger commented Apr 27, 2022

This PR pulls in a new release from SmallRye (https://github.com/smallrye/smallrye-graphql/releases/tag/1.5.0) and adds supports for Non blocking operations. The same rule applies as with RESTEasy Reactive.

  • By default, normal response objects will execute blocking, except if marked with @NonBlocking
  • By default, Uni and CompletionStage response objects will execute non-blocking, except if marked with @Blocking

Breaking change: The ExecutionService has changed to not respond with a result, but rather a writer needs to be passed in. a default JsonObjectResponseWriter exist to get the same result as before.

Because this is a major new feature, non-blocking can be turned off using a config: quarkus.smallrye-graphql.nonblocking.enabled=false. By default it will be enabled

Signed-off-by: Phillip Kruger phillip.kruger@gmail.com

@phillip-kruger
Copy link
Member Author

phillip-kruger commented Apr 27, 2022

@geoand - There are small changes on how the locale resolver work ( in hibernate-validator) for GraphQL (something that came in a few weeks ago) - I moved the resolver to the graphql extension and are now getting the locale from the graphql context. This helps to make sure the locale are propagated.

@geoand geoand added the area/hibernate-validator Hibernate Validator label Apr 27, 2022
@geoand
Copy link
Contributor

geoand commented Apr 27, 2022

Understood, thanks.

@jmartisk
Copy link
Contributor

jmartisk commented Apr 27, 2022

This currently makes the hibernate-validator a required dependency of the smallrye-graphql extension. To make it properly optional, we will probably need to:

  • Change the dependencies in smallrye-graphql extension's poms to be <optional>true</optional>. I think we should also switch from depending on hibernate-validator directly to depending on io.quarkus:quarkus-hibernate-validator. Note that the deployment pom already has a dependency on it, it's in the test scope! We need it to be optional, but present for testing. It seems that an optional dependency is always present during tests.
  • In SmallRyeGraphQLProcessor#additionalBean we need to detect whether the hibernate-validator extension is present before we produce SmallRyeGraphQLLocaleResolver as a bean. If we produce it when the extension is not there, it will blow up. This means we will probably need to add a Capability for the hibernate-validator extension, to be able to do this check.

@phillip-kruger
Copy link
Member Author

Thanks @jmartisk - Agree. I can make those changes now in this PR.

@quarkus-bot

This comment has been minimized.

@phillip-kruger
Copy link
Member Author

@jmartisk - I made all the changes as you requested. I think it's all fine now. MP TCK is passing on my localhost. Please review :)

@quarkus-bot

This comment has been minimized.

@mschorsch
Copy link

Any chance that this will be part of 2.9?

@quarkus-bot

This comment has been minimized.

@phillip-kruger
Copy link
Member Author

Any chance that this will be part of 2.9?

Not sure at this stage. Waiting for a green CI and PR approval. Might only be the next version, or allowing it here with a config option to enable it but default back to prev version

@phillip-kruger
Copy link
Member Author

I have now added quarkus.smallrye-graphql.nonblocking.enabled=true that by default will be false. This allows a more soft release, so by default there is no change, but when enabled this experimental feature kicks in.

@quarkus-bot

This comment has been minimized.

@cescoffier
Copy link
Member

The CI failures look related.

Signed-off-by: Phillip Kruger <phillip.kruger@gmail.com>
@phillip-kruger
Copy link
Member Author

@mschorsch - we missed the 2.9 release. Look out for the CR of 2.10

@mschorsch
Copy link

OK. Thanks.

@phillip-kruger
Copy link
Member Author

@jmartisk @cescoffier - CI is green. Please review when you have a chance :) Thanks

@gsmet
Copy link
Member

gsmet commented May 4, 2022

@jmartisk @cescoffier FWIW, this is in the way of my Jakarta work so if we can get it in soon, that would be awesome.

@phillip-kruger
Copy link
Member Author

I have now added quarkus.smallrye-graphql.nonblocking.enabled=true that by default will be false. This allows a more soft release, so by default there is no change, but when enabled this experimental feature kicks in.

This is now the other way around : non-blocking can be turned off using a config: quarkus.smallrye-graphql.nonblocking.enabled=false. By default it will be enabled

Copy link
Contributor

@jmartisk jmartisk left a comment

Choose a reason for hiding this comment

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

LGTM, very nice work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants