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

no more HTTP compression for resteasy-classic endpoints #26112

Closed
tnmtechnologies opened this issue Jun 14, 2022 · 13 comments
Closed

no more HTTP compression for resteasy-classic endpoints #26112

tnmtechnologies opened this issue Jun 14, 2022 · 13 comments
Labels
area/resteasy-classic kind/bug Something isn't working

Comments

@tnmtechnologies
Copy link
Contributor

Describe the bug

Our REST enpoints are based on resteasy classic mode.
For quarkus 2.8.2.Final, we set quarkus.http.enable-compression=true and responses are well compressed.

After upgrading to quarkus 2.9.0.Final, responses are no more compressed.
Behaviour of quarkus.http.enable-compression property has been changed in quarkus 2.9.0.Final.

Expected behavior

Be able to serve compressed responses in resteasy classic mode

Actual behavior

For quarkus 2.9.0.Final and for resteasy classic mode, responses are no more compressed when property quarkus.http.enable-compression=true.

How to Reproduce?

Having a REST service with

  1. Quarkus 2.9.0.Final
  2. resteasy classic mode
  3. quarkus.http.enable-compression=true
    and serving a response with for instance a application/json or application/xml content type

Output of uname -a or ver

No response

Output of java -version

No response

GraalVM version (if different from Java)

No response

Quarkus version or git rev

No response

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

In #24558 (comment), we can read:
Note that it's a breaking change in the sense that previously quarkus.http.enable-compression=true enabled compression for every HTTP response which is not the case anymore. The configuration property is effectively ignored for reasteasy-classic endpoints and routes registered manually.

@geoand
Copy link
Contributor

geoand commented Jun 14, 2022

cc @mkouba

@mkouba
Copy link
Contributor

mkouba commented Jun 14, 2022

Yes, it's a breaking change as documented in the migration guide. We believe that the original behavior did not make a lot of sense because it enabled compression for every HTTP response. You should annotate your resteasy classic resources with @org.jboss.resteasy.annotations.GZIP to enable compression. Note that static resources would be compressed if quarkus.http.enable-compression=true and the content type matches a value from quarkus.http.compress-media-types.

@gsmet
Copy link
Member

gsmet commented Jun 14, 2022

@mkouba hmm, the migration guide says nothing about RESTEasy Classic or did I miss something?

Also, can you compress all your JSON REST resources served by RESTEasy Classic/Reactive by using quarkus.http.compress-media-types?

@mkouba
Copy link
Contributor

mkouba commented Jun 14, 2022

hmm, the migration guide says nothing about RESTEasy Classic or did I miss something?

No, it doesn't and quarkus.http.enable-compression did not mention RESTEasy classic either. It was just an all or nothing switch.

Also, can you compress all your JSON REST resources served by RESTEasy Classic/Reactive by using quarkus.http.compress-media-types?

For RESTEasy Reactive the response is compressed if the Content-Type header is set and the value is listed in quarkus.http.compress-media-types. I'm not sure about RESTEasy though...

@gsmet
Copy link
Member

gsmet commented Jun 14, 2022 via email

@mkouba
Copy link
Contributor

mkouba commented Jun 14, 2022

FTR this is our RESTEasy Classic documentation about GZip Support (we added a note that quarkus.http.enable-compression has no effect; previously the property was not mentioned at all):
https://quarkus.io/guides/resteasy#gzip-support

And we should probably check if quarkus.http.compress-media-types works
with RESTEasy Classic...

I don't think that it does because we add Content-Encoding: identity" if quarkus.http.enable-compression=true and then remove the header when needed.

as adding @GZIP to every REST resource will be a major pain.

It is. On the other hand, the previous behavior of quarkus.http.enable-compression=true was IMO quite unreasonable. Also it should be fairly straightforward to implement a ContainerResponseFilter that simply removes the Content-Encoding: identity" header.

@gsmet
Copy link
Member

gsmet commented Jun 15, 2022

I think we really need to test this and I also think we should make the media types configuration work with RESTEasy Classic i.e. have the ability to say: all my JSON content will be gzipped and that would include RESTEasy Classic endpoints returning JSON.

TBH I was under the impression that it was done this way. We also need something in the migration guide as the behavior has changed (even if it wasn't documented, we were telling people to use this property when asked).

@mkouba
Copy link
Contributor

mkouba commented Jun 16, 2022

all my JSON content will be gzipped and that would include RESTEasy Classic endpoints returning JSON

In theory, we could add a ContainerResponseFilter that would implement the same functionality as we have for resteasy reactive and reactive routes.

TBH I was under the impression that it was done this way.

It wasn't ;-). The PR description was very clear that quarkus.http.enable-compression is ignored for reasteasy-classic endpoints and routes registered manually.

@tnmtechnologies
Copy link
Contributor Author

tnmtechnologies commented Jun 16, 2022

Thanks for looking into this issue.

Based on your comments, I have done some tests from the quarkus-quickstarts/getting-started project in this branch.
I added an endpoint that serves a response with a 1024 bytes entity

The file TESTSSUMMARY.md contains the test results I did (please open as raw).

Table of the test results:

quarkus version resteasy dependency quarkus.http.enable-compression quarkus.resteasy.gzip.enabled with org.jboss.resteasy.annotations.GZIP endpoint Accept-Encoding Content-Encoding
2.9.2.Final quarkus-resteasy-reactive true /entity/1024 gzip, deflate, br gzip
/q/metrics gzip, deflate, br quarkus.http.compress-media-types=text/plain has no effect
quarkus-resteasy-jsonb true /entity/1024 gzip, deflate, br
/q/metrics gzip, deflate, br
true /entity/1024 gzip, deflate, br gzip
/q/metrics gzip, deflate, br
2.8.2.Final quarkus-resteasy-reactive true /entity/1024 gzip, deflate, br gzip
/q/metrics gzip, deflate, br gzip
quarkus-resteasy-jsonb true /entity/1024 gzip, deflate, br gzip
/q/metrics gzip, deflate, br gzip

Summary

In quarkus 2.9.2.Final, resteasy classic endpoint only serves a compressed response when quarkus.resteasy.gzip.enabled=true and @org.jboss.resteasy.annotationsGZIP annotation is added to the resource.
I also observed that /q/metrics endpoint (quarkus-smallrye-metrics) response is no more compressed.
I did not test heath check endpoints.

@mkouba
Copy link
Contributor

mkouba commented Jun 17, 2022

@tnmtechnologies Thanks for the tests! Yes, the results are expected. I believe that quarkus-smallrye-metrics register a custom route but I'm not sure.

@gsmet
Copy link
Member

gsmet commented Jul 4, 2022

In theory, we could add a ContainerResponseFilter that would implement the same functionality as we have for resteasy reactive and reactive routes.

Well, I think we will probably have to do that. I'm not looking forward to explaining to people that they need to add the @GZip annotation to all their RESTEasy Classic resources.

In any case, we need to make things clear there for RESTEasy Classic: https://github.com/quarkusio/quarkus/wiki/Migration-Guide-2.9#http-compression . Right now, people have no idea that the behavior has changed and they might miss it (until they get their bandwidth bill).

Also, probably a good idea to understand what's happening with metrics and why it worked before and doesn't work anymore.

mkouba added a commit to mkouba/quarkus that referenced this issue Feb 27, 2023
- honor the quarkus.http.compress-media-types in Undertow Servlet and
RESTEasy Classsic extensions
- fixes quarkusio#31415 and quarkusio#26112
mkouba added a commit to mkouba/quarkus that referenced this issue Feb 28, 2023
- honor the quarkus.http.compress-media-types in Undertow Servlet and
RESTEasy Classsic extensions
- fixes quarkusio#31415 and quarkusio#26112
mkouba added a commit to mkouba/quarkus that referenced this issue Mar 1, 2023
- honor the quarkus.http.compress-media-types in Undertow Servlet and
RESTEasy Classsic extensions
- fixes quarkusio#31415 and quarkusio#26112
@geoand
Copy link
Contributor

geoand commented Mar 20, 2023

@mkouba this has been fixed, no?

@mkouba
Copy link
Contributor

mkouba commented Mar 20, 2023

@mkouba this has been fixed, no?

Yes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/resteasy-classic kind/bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants