Fixes #12612 - Use Compression classes for client decoding. #12613

sbordet · 2024-12-05T12:38:07Z

Now using Compression classes for client response decoding.

Removed oej.client.GZIPContentDecoder, now using only oeh.http.GZIPContentDecoder where necessary.

sbordet · 2024-12-05T12:40:36Z

TODO

Merge Supporting Compression discovery with ServiceLoader. #12611
Make sure by default at least gzip is present for the client
Update documentation
Update migration guide due to client API changes

Now using Compression classes for client response decoding. Removed oej.client.GZIPContentDecoder, now using only oeh.http.GZIPContentDecoder where necessary. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

…pression-common. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Updated tests that required client jars in a web application. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

… installing CompressionHandler. Deprecated gzip.mod in favor of compression-gzip.mod. Renamed `CompressionHandler.registerCompression()` because there are many other methods called `put*` in the class. Fixed `jetty-home/pom.xml` to generate the correct jetty-home. Added distribution tests. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

…ing. * Fixed GzipEncoderSink, to finish deflating when last=true. * Enhanced logic in CompressionResponse, to skip by status code, content-type, content-encoding and empty content. * CompressionResponse does not need to implement Callback. * Added tests for CompressionHandler in ee11. * Code cleanups. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

...jetty-compression-gzip/src/main/java/org/eclipse/jetty/compression/gzip/GzipCompression.java

...ompression-server/src/main/java/org/eclipse/jetty/compression/server/CompressionHandler.java

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

...ompression-server/src/main/java/org/eclipse/jetty/compression/server/CompressionHandler.java

gregw · 2024-12-08T23:36:29Z

...istribution-common/src/test/java/org/eclipse/jetty/tests/distribution/DistributionTests.java

+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = {"brotli", "gzip", "zstandard"})


Need to test at least some combinations of multiple algorithms

...istribution-common/src/test/java/org/eclipse/jetty/tests/distribution/DistributionTests.java

documentation/jetty/modules/programming-guide/pages/client/http.adoc

...jetty-compression/jetty-compression-server/src/main/config/modules/compression-zstandard.mod

...core/jetty-compression/jetty-compression-server/src/main/config/modules/compression-gzip.mod

...re/jetty-compression/jetty-compression-server/src/main/config/modules/compression-brotli.mod

jetty-core/jetty-client/src/main/java/module-info.java

...jetty-compression/jetty-compression-server/src/main/config/modules/compression-zstandard.mod

Implemented usage of minCompressSize. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Updated test after implementing minCompressSize. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

gregw

a couple of niggles

documentation/jetty/modules/programming-guide/pages/migration/12.0-to-12.1.adoc

gregw

a couple of niggles left, but I'd be OK to approve if we want to build a release.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

lorban · 2024-12-10T16:43:18Z

...sion/jetty-compression-common/src/main/java/org/eclipse/jetty/compression/DecoderSource.java


-public abstract class DecoderSource implements Content.Source
+public abstract class DecoderSource extends ContentSourceTransformer


Can't this class now go away since it doesn't do anything anymore?

I think it's fine if it remains, in case it needs something more specific to compression.

jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpReceiver.java

...tty-client/src/test/java/org/eclipse/jetty/client/HttpClientContentDecoderFactoriesTest.java

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ContentSourceTransformer.java

lorban · 2024-12-12T12:08:22Z

...-ee10/jetty-ee10-proxy/src/main/java/org/eclipse/jetty/ee10/proxy/AsyncMiddleManServlet.java

@@ -777,7 +777,7 @@ public static class GZIPContentTransformer implements ContentTransformer

        private final List<ByteBuffer> buffers = new ArrayList<>(2);
        private final ContentTransformer transformer;
-        private final ContentDecoder decoder;
+        private final GZIPContentDecoder decoder;


You should set the field type as GZIPDecoder to make it explicit that using this subclass is mandatory.

It is an internal implementation detail, like declaring Map but then using HashMap.
The key point here is that we could get rid of the jetty-client's GZIPContentDecoder and we can now use jetty-https version without problems.

lorban · 2024-12-12T12:08:56Z

...-ee11/jetty-ee11-proxy/src/main/java/org/eclipse/jetty/ee11/proxy/AsyncMiddleManServlet.java

@@ -777,7 +777,7 @@ public static class GZIPContentTransformer implements ContentTransformer

        private final List<ByteBuffer> buffers = new ArrayList<>(2);
        private final ContentTransformer transformer;
-        private final ContentDecoder decoder;
+        private final GZIPContentDecoder decoder;


Type should be GZIPDecoder.

lorban · 2024-12-12T12:09:21Z

jetty-ee9/jetty-ee9-proxy/src/main/java/org/eclipse/jetty/ee9/proxy/AsyncMiddleManServlet.java

@@ -777,7 +777,7 @@ public static class GZIPContentTransformer implements ContentTransformer

        private final List<ByteBuffer> buffers = new ArrayList<>(2);
        private final ContentTransformer transformer;
-        private final ContentDecoder decoder;
+        private final GZIPContentDecoder decoder;


Type should be GZIPDecoder.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

lorban

One last small thing you forgot in a test, otherwise LGTM.

...tty-client/src/test/java/org/eclipse/jetty/client/HttpClientContentDecoderFactoriesTest.java

gregw

CI failures look real.
Just a few niggles otherwise.

documentation/jetty/modules/programming-guide/pages/migration/12.0-to-12.1.adoc

gregw · 2024-12-12T21:02:45Z

...istribution-common/src/test/java/org/eclipse/jetty/tests/distribution/DistributionTests.java

+            case "brotli" -> "br";
+            case "gzip" -> "gzip";
+            case "zstandard" -> "zstd";


Suggested change

case "brotli" -> "br";

case "gzip" -> "gzip";

case "zstandard" -> "zstd";

case "brotli" -> "br";

case "gzip" -> "gzip";

case "zstandard" -> "zstd";

case "all" -> "br,compression-gzip,compression-zstd";

@gregw I'm not sure what you exactly mean here.
You mean for "all" to have a Content-Encoding with just br as a valid value, and 2 other invalid values?
Or you meant all 3 valid values, so br, gzip, zstd?

Also, I just read RFC 9110, and it is way more complicated to support the right encoding than we can write in a distribution test, so I think it's best to write more complicated cases in non-distribution tests.

I just think we need at least one distribution test that enables all the compression at once. I'm OK if the test itself just sends a single algorithm in its accept header. We just need to check the modules and xmls do not clash somehow.

I do mean all 3 valid values, I was just hacking in the "compression-", but a more elegant solution is possible

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

…ressors'. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

gregw · 2024-12-16T04:54:52Z

...-core/jetty-compression/jetty-compression-server/src/main/config/modules/compression-all.mod

+compression-brotli
+compression-gzip
+compression-zstandard


gregw · 2024-12-16T04:55:28Z

...istribution-common/src/test/java/org/eclipse/jetty/tests/distribution/DistributionTests.java

+            case "all" -> "br;q=0.5, gzip;q=1, zstd;q=0.1";
+            default -> throw new IllegalArgumentException();
+        };
+
+        String expected = switch (compressionName)
+        {
+            case "brotli" -> "br";
+            case "gzip" -> "gzip";
+            case "zstandard" -> "zstd";
+            case "all" -> "gzip";


Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

sbordet requested review from gregw, joakime and lorban December 5, 2024 12:38

sbordet linked an issue Dec 5, 2024 that may be closed by this pull request

Use Compression classes for client decoding #12612

Closed

sbordet force-pushed the fix/jetty-12.1.x/12612/client-compressors branch from d178751 to 4b56e24 Compare December 5, 2024 17:35

sbordet added 4 commits December 6, 2024 11:35

Fixes #12612 - Use Compression classes for client decoding.

f53dd63

Now using Compression classes for client response decoding. Removed oej.client.GZIPContentDecoder, now using only oeh.http.GZIPContentDecoder where necessary. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Updated OSGi dependencies, now that jetty-client depends on jetty-com…

7a06103

…pression-common. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Now using ServiceLoader to load the Compression implementations.

f6ce763

Updated tests that required client jars in a web application. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

sbordet force-pushed the fix/jetty-12.1.x/12612/client-compressors branch from 72ee178 to 8abb27d Compare December 6, 2024 10:38

sbordet mentioned this pull request Dec 6, 2024

Update jetty-compression for OSGi #12618

Closed

Fixes #12618 - Update jetty-compression for OSGi.

4463087

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

joakime reviewed Dec 6, 2024

View reviewed changes

...jetty-compression-gzip/src/main/java/org/eclipse/jetty/compression/gzip/GzipCompression.java Outdated Show resolved Hide resolved

joakime reviewed Dec 6, 2024

View reviewed changes

...ompression-server/src/main/java/org/eclipse/jetty/compression/server/CompressionHandler.java Show resolved Hide resolved

sbordet added 2 commits December 7, 2024 00:34

Fixed client.mod dependencies.

a1ed64d

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Updated test dependencies.

f7806b8

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

sbordet added build-no-cache build-all-tests labels Dec 7, 2024

Updated documentation and migration guide.

4fb2116

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

sbordet requested a review from joakime December 8, 2024 15:01

gregw requested changes Dec 9, 2024

View reviewed changes

lorban reviewed Dec 9, 2024

View reviewed changes

...jetty-compression/jetty-compression-server/src/main/config/modules/compression-zstandard.mod Outdated Show resolved Hide resolved

sbordet added 2 commits December 9, 2024 21:34

Updates from reviews.

8e2b8ac

Implemented usage of minCompressSize. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Fixed typo.

f2eb965

Updated test after implementing minCompressSize. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

sbordet requested a review from gregw December 9, 2024 20:55

Fixed typos.

601b940

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

gregw approved these changes Dec 9, 2024

View reviewed changes

documentation/jetty/modules/programming-guide/pages/migration/12.0-to-12.1.adoc Outdated Show resolved Hide resolved

documentation/jetty/modules/programming-guide/pages/migration/12.0-to-12.1.adoc Outdated Show resolved Hide resolved

gregw requested changes Dec 9, 2024

View reviewed changes

Updates from review.

c55a3d4

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

sbordet requested review from gregw and lorban December 9, 2024 23:08

Updated test after implementing minCompressSize.

e3cf38f

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

lorban requested changes Dec 12, 2024

View reviewed changes

Updates from review.

72bb0b1

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

sbordet requested a review from lorban December 12, 2024 16:24

lorban requested changes Dec 12, 2024

View reviewed changes

...tty-client/src/test/java/org/eclipse/jetty/client/HttpClientContentDecoderFactoriesTest.java Outdated Show resolved Hide resolved

gregw approved these changes Dec 12, 2024

View reviewed changes

sbordet added 2 commits December 13, 2024 12:22

Updates from review.

cb2c159

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Fixed ContentSourceTransformer.

1c32a08

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

sbordet requested review from lorban and gregw December 13, 2024 11:52

sbordet added 3 commits December 13, 2024 13:37

Merged branch 'jetty-12.1.x' into 'fix/jetty-12.1.x/12612/client-comp…

78bfe48

…ressors'. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Implemented support for quality values in Accept-Encoding.

fc23160

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Added module compression-all and correspondent distribution test.

50067d7

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

gregw approved these changes Dec 16, 2024

View reviewed changes

sbordet requested a review from gregw December 16, 2024 07:35

lorban approved these changes Dec 16, 2024

View reviewed changes

Added module documentation.

9660e12

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

gregw approved these changes Dec 16, 2024

View reviewed changes

sbordet merged commit fc4ad94 into jetty-12.1.x Dec 16, 2024
10 checks passed

sbordet deleted the fix/jetty-12.1.x/12612/client-compressors branch December 16, 2024 21:34

sbordet mentioned this pull request Dec 16, 2024

Use Compression classes for client decoding #12612

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #12612 - Use Compression classes for client decoding. #12613

Fixes #12612 - Use Compression classes for client decoding. #12613

sbordet commented Dec 5, 2024

sbordet commented Dec 5, 2024 •

edited

Loading

gregw Dec 8, 2024

gregw left a comment

gregw left a comment

lorban Dec 10, 2024

sbordet Dec 12, 2024

lorban Dec 12, 2024

sbordet Dec 12, 2024

lorban Dec 12, 2024

lorban Dec 12, 2024

lorban left a comment

gregw left a comment

gregw Dec 12, 2024

sbordet Dec 13, 2024

gregw Dec 14, 2024

gregw Dec 16, 2024

gregw Dec 16, 2024


		public abstract class DecoderSource implements Content.Source
		public abstract class DecoderSource extends ContentSourceTransformer

Fixes #12612 - Use Compression classes for client decoding. #12613

Fixes #12612 - Use Compression classes for client decoding. #12613

Conversation

sbordet commented Dec 5, 2024

sbordet commented Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

gregw left a comment

Choose a reason for hiding this comment

gregw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorban left a comment

Choose a reason for hiding this comment

gregw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbordet commented Dec 5, 2024 •

edited

Loading