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

Added gzip compression feature for SmallRye-GraphQL, added tests #35767

Merged
merged 1 commit into from
Sep 8, 2023

Conversation

mskacelik
Copy link
Contributor

/cc @jmartisk @mkouba
fixes: #21642

The one thing I am not sure about is this line of code: https://github.com/mskacelik/quarkus/blob/110e460ca5f5e468e9ce3811daa79620cc2576ee/extensions/smallrye-graphql/runtime/src/main/java/io/quarkus/smallrye/graphql/runtime/SmallRyeGraphQLCompressionHandler.java#L17

Here I use addHeadersEndHandler instend of addEndHandler. The reason is because SmallRyeGraphQLAbstractHandler rewrites the endHandler for the ExecutionHandler.

@mkouba
Copy link
Contributor

mkouba commented Sep 6, 2023

The one thing I am not sure about is this line of code: https://github.com/mskacelik/quarkus/blob/110e460ca5f5e468e9ce3811daa79620cc2576ee/extensions/smallrye-graphql/runtime/src/main/java/io/quarkus/smallrye/graphql/runtime/SmallRyeGraphQLCompressionHandler.java#L17

Here I use addHeadersEndHandler instend of addEndHandler. The reason is because SmallRyeGraphQLAbstractHandler rewrites the endHandler for the ExecutionHandler.

I wonder if it would make more sense to change the logic in the SmallRyeGraphQLAbstractHandler.handle() method to use RoutingContext.addEndHandler() instead 🤷 .

@mskacelik
Copy link
Contributor Author

I wonder if it would make more sense to change the logic in the SmallRyeGraphQLAbstractHandler.handle() method to use RoutingContext.addEndHandler() instead 🤷 .

I tried it once, but unfortunately it had no effect. But I'll try it again, with a little more in-depth look into it.

@quarkus-bot
Copy link

quarkus-bot bot commented Sep 6, 2023

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@mskacelik
Copy link
Contributor Author

Ok, I have tried it again and it once again had no effect. Funny enough, while I commented out the ctx.response()... lines of code in the SmallRyeGraphQLAbstractHandler:

//        ctx.response()
//                .endHandler(currentManagedContextTerminationHandler)
//                .exceptionHandler(currentManagedContextExceptionHandler)
//                .closeHandler(currentManagedContextTerminationHandler);

And at the same time having the context.addEndHandler in the SmallRyeGraphQLCompressionHandler did not fix the issue. So it is probably not the cause of this problem. 🤔

@mkouba
Copy link
Contributor

mkouba commented Sep 7, 2023

FTR the addEndHandler() was probably a bad idea; it shouldn't be used in this case. There's a proposal to fix the support for some extensions: #35804. Let's wait until that fix is approved.

@geoand
Copy link
Contributor

geoand commented Sep 8, 2023

FYI, #35804 is now in

@jmartisk jmartisk merged commit 047ecb1 into quarkusio:main Sep 8, 2023
50 checks passed
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Sep 8, 2023
@quarkus-bot quarkus-bot bot added this to the 3.5 - main milestone Sep 8, 2023
@mskacelik mskacelik deleted the compression-SRGQL branch September 8, 2023 11:49
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.

Support for GZIP in GraphQL Services
4 participants