[HTTPCLIENT-1843] - Delegate compression handling to Apache Commons Compress #580

arturobernalg · 2024-09-19T20:08:50Z

This PR addresses HTTPCLIENT-1843, allowing Apache HttpClient to delegate compression handling to Apache Commons Compress. The goal is to provide broader support for various compression algorithms while reducing custom implementation and maintenance.

garydgregory

1.27.1 is the current version of Commons Compress.

arturobernalg · 2024-09-19T20:41:53Z

1.27.1

changed.

httpclient5/src/main/java/org/apache/hc/client5/http/entity/CompressingEntity.java

garydgregory · 2024-09-20T13:25:55Z

httpclient5/src/main/java/org/apache/hc/client5/http/entity/CompressorFactory.java

+
+    private static final Map<String, String> COMPRESSION_ALIASES;
+    static {
+        final Map<String, String> aliases = new HashMap<>();


@arturobernalg
Why just those stream types?
How about using:

org.apache.commons.compress.compressors.CompressorStreamProvider.getInputStreamCompressorNames()

org.apache.commons.compress.compressors.CompressorStreamProvider.getOutputStreamCompressorNames()

Should I add an API to return org.apache.commons.compress.compressors.CompressorStreamFactory.ALL_NAMES?

httpclient5/src/main/java/org/apache/hc/client5/http/entity/CompressorFactory.java

httpclient5/src/main/java/org/apache/hc/client5/http/entity/DecompressEntity.java

garydgregory · 2024-09-20T13:40:42Z

httpclient5/src/main/java/org/apache/hc/client5/http/entity/DecompressEntity.java

+    public InputStream getContent() throws IOException {
+        if (super.isStreaming()) {
+            if (content == null) {
+                content = getDecompressingStream();


If we do not lock, we should document this class as not thread-safe.

httpclient5/src/main/java/org/apache/hc/client5/http/entity/DecompressEntity.java

httpclient5/src/main/java/org/apache/hc/client5/http/entity/DeflateInputStream.java

httpclient5/src/main/java/org/apache/hc/client5/http/entity/DeflateInputStreamFactory.java

httpclient5/src/main/java/org/apache/hc/client5/http/entity/DeflateDecompressingEntity.java

httpclient5/src/main/java/org/apache/hc/client5/http/entity/LazyDecompressInputStream.java

arturobernalg · 2024-09-20T20:29:04Z

HI @garydgregory
Please do another pass.
TY

garydgregory

@arturobernalg
Thank you for your update.

I have comments embedded throughout.
There is some lazy-initialization code here and there, this should be called out in the class Javadoc making the class thread-unsafe, we have an annotation for that: org.apache.hc.core5.annotation.ThreadingBehavior.

...ng/src/test/java/org/apache/hc/client5/testing/extension/sync/StandardTestClientBuilder.java

...t5-testing/src/test/java/org/apache/hc/client5/testing/extension/sync/TestClientBuilder.java

...-testing/src/test/java/org/apache/hc/client5/testing/extension/sync/TestClientResources.java

...t5-testing/src/test/java/org/apache/hc/client5/testing/sync/AbstractIntegrationTestBase.java

httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/HttpClientBuilder.java

httpclient5/src/test/java/org/apache/hc/client5/http/entity/TestEntityBuilder.java

garydgregory · 2024-09-20T21:26:45Z

httpclient5/src/test/java/org/apache/hc/client5/http/entity/TestGZip.java

-            final GzipDecompressingEntity gunzipe = new GzipDecompressingEntity(out);
-            Assertions.assertEquals("some kind of text", EntityUtils.toString(gunzipe, StandardCharsets.US_ASCII));
-        }
+


Remove extra blank lines, the comments suffice.

arturobernalg · 2024-09-21T07:10:54Z

Thank you for your update.

* I have comments embedded throughout.

* There is some lazy-initialization code here and there, this should be called out in the class Javadoc making the class thread-unsafe, we have an annotation for that: `org.apache.hc.core5.annotation.ThreadingBehavior`.

HI @garydgregory
I believe I’ve covered all the changes you requested.
TY

garydgregory · 2024-10-21T12:38:55Z

httpclient5/src/main/java/org/apache/hc/client5/http/entity/DecompressEntity.java

+ * @since 5.5
+ */
+@Contract(threading = ThreadingBehavior.UNSAFE)
+public class DecompressEntity extends HttpEntityWrapper {


The class name is mismatched with the "CompressingEntity" name, either they should both be "...ing" names, or not.

garydgregory

Hi @arturobernalg

I git-hub resolved most of the comment I wrote. I think we should consistently name ALL our new classes either with or without the "ing" word part, either [Compress|Decompress]Entity or [Compressing|Decompressing]Entity. I think "ing" is better and in keeping with our existing other HttpEntity implementations like IncomingHttpEntity and DecompressingEntity (conflicting).
I also think that all this new compression/decompression code should live in its own package, maybe org.apache.hc.client5.http.entity.compress.

arturobernalg · 2024-10-21T19:14:12Z

Hi @arturobernalg

I git-hub resolved most of the comment I wrote. I think we should consistently name ALL our new classes either with or without the "ing" word part, either [Compress|Decompress]Entity or [Compressing|Decompressing]Entity. I think "ing" is better and in keeping with our existing other HttpEntity implementations like IncomingHttpEntity and DecompressingEntity (conflicting). I also think that all this new compression/decompression code should live in its own package, maybe org.apache.hc.client5.http.entity.compress.

Please @garydgregory take another look.

garydgregory

Hi @arturobernalg
Thank you for the updates! 😄
I added more comments throughout.
We're very close I think 😄
TY!

garydgregory · 2024-10-22T11:33:04Z

httpclient5/src/main/java/org/apache/hc/client5/http/entity/compress/CompressingFactory.java

+     */
+    public String getFormattedName(final String name) {
+        if (name == null || name.isEmpty()) {
+            LOG.warn("Compression name is null or empty");


This API should not do any logging, especially at such a high level. This is just a string-to-string conversion API, it has no business rasing warnings IMO.

garydgregory · 2024-10-22T11:35:37Z

httpclient5/src/main/java/org/apache/hc/client5/http/entity/compress/CompressingFactory.java

+        Args.notNull(entity, "Entity");
+        Args.notNull(contentEncoding, "Content Encoding");
+        if (!isSupported(contentEncoding, false)) {
+            LOG.warn("Unsupported decompression type: {}", contentEncoding);


This class should not do any logging IMO, especially warnings, it should throw an illegal argument exception or let the call site deal with a null result.

garydgregory · 2024-10-22T11:38:15Z

httpclient5/src/main/java/org/apache/hc/client5/http/entity/compress/CompressingFactory.java

+    private Set<String> fetchAvailableInputProviders() {
+        final Set<String> inputNames = compressorStreamFactory.getInputStreamCompressorNames();
+        return inputNames.stream()
+                .map(String::toLowerCase)


This might have some odd results depending on the locale context (https://garygregory.wordpress.com/2015/11/03/java-lowercase-conversion-turkey/) if you do not use a Locale like the PR does above using ROOT.

garydgregory · 2024-10-22T11:38:26Z

httpclient5/src/main/java/org/apache/hc/client5/http/entity/compress/CompressingFactory.java

+    private Set<String> fetchAvailableOutputProviders() {
+        final Set<String> outputNames = compressorStreamFactory.getOutputStreamCompressorNames();
+        return outputNames.stream()
+                .map(String::toLowerCase)


garydgregory · 2024-10-22T11:39:30Z

httpclient5/src/main/java/org/apache/hc/client5/http/entity/compress/CompressingFactory.java

+     *
+     * @return a set of available output stream compression providers in lowercase.
+     */
+    private Set<String> fetchAvailableOutputProviders() {


The "fetch" method name prefix is unusual and surprising to me, I would follow the "get" naming convention.

garydgregory · 2024-10-22T11:46:02Z

httpclient5/src/main/java/org/apache/hc/client5/http/entity/DecompressingEntity.java

@@ -38,7 +38,9 @@
 * Common base class for decompressing {@link HttpEntity} implementations.
 *
 * @since 4.4
+ * @deprecated


The Javadoc tag is missing its comment to redirect reader.

garydgregory · 2024-10-22T11:52:17Z

httpclient5/src/main/java/org/apache/hc/client5/http/entity/compress/CompressingEntity.java

+        try {
+            compressorStream = CompressingFactory.INSTANCE.getCompressorOutputStream(contentEncoding, outStream);
+        } catch (final CompressorException e) {
+            throw new IOException("Error initializing decompression stream", e);


Don't you mean "Error initializing compression stream"?

garydgregory · 2024-10-22T11:53:33Z

httpclient5/src/main/java/org/apache/hc/client5/http/entity/compress/CompressingEntity.java

+            // Write compressed data
+            super.writeTo(compressorStream);
+            // Close the compressor stream after writing
+            compressorStream.close();


Could this method be rewritten to use a try-with-resources block?

garydgregory · 2024-10-22T11:56:38Z

httpclient5/src/main/java/org/apache/hc/client5/http/entity/compress/CompressingFactory.java

+    }
+
+    /**
+     * Returns the formatted name of the provided compression format.


I don't understand what this Javadoc means. What's a "formatted" name? Maybe the API is misnamed? Is the job of the method to map from a content type value to a Commons Compress key? Is it something else? I think the comment should at least make it clear, which should guide us to a better API name.

garydgregory · 2024-10-22T12:10:13Z

httpclient5/src/main/java/org/apache/hc/client5/http/entity/compress/CompressingFactory.java

+        final String formattedName = getFormattedName(name);
+        return isSupported(formattedName, false)
+                ? createCompressorInputStream(formattedName, inputStream, noWrap)
+                : inputStream;


The Javadoc and implementation don't match:

Javadoc states decompression is performed

The code doesn't decompress if the format isn't supported.

Either the Javadoc needs updating, or, more likely, we should throw an exception indicating the operation is not supported for that format type name.

I must say I find the API and Javadoc confusing, we are creating a "Compressor" input stream in order to "decompress" with a "compression" format! What?

Maybe calling the API getDecompressorInputStream would help because it "decompresses". I expect a "Compressor" to compress, not the other way around.

Maybe using the term "format type name" instead "compression format" would remove the need for the reader to constantly parse "compress" vs. "decompress" in the text.

Maybe only using one term "decompress" or "compress" for one method (Javadoc and code) would alleviate the issue.

Thanks, @garydgregory . Agreed—the Javadoc and implementation don’t match. We’ll:

Throw an exception on unsupported formats instead of returning the original stream, aligning with Javadoc expectations.

Rename to getDecompressorInputStream to reflect decompression.

Standardize to “format type” instead of “compression format” to clarify.

These changes should make both the API and docs clearer. Thanks for flagging this.

HI @garydgregory
Can you do another pass??
Thank you.

…ompress * Integrated Apache Commons Compress into CompressorFactory to handle compression and decompression of HTTP entities using supported algorithms (gzip, deflate, etc.).

garydgregory

Hi @arturobernalg

TY for the work 👍
Minor: For the Javadoc since tags, I think the next feature release will be 5.5 since the current release is 5.4.1 and the git master POM already has 5.5 in it.
Side note: In Commons Compress, CompressorException usually wraps an IOException, so it's too bad that we wrap it in another IOException here ;-) Since compression and decompression is IO, I'll see about making CompressorException extend IOException.

…se 'ing' form (e.g., CompressingEntity) to align with existing conventions like DecompressingEntity. Moved compression/decompression classes to org.apache.hc.client5.http.entity.compress package

arturobernalg · 2024-11-15T20:41:00Z

Hi @arturobernalg

* TY for the work 👍

* Minor: For the Javadoc `since` tags, I think the next feature release will be `5.5` since the current release is `5.4.1` and the git master POM already has 5.5 in it.

* Side note: In Commons Compress, `CompressorException` usually wraps an `IOException`, so it's too bad that we wrap it in another IOException here ;-) Since compression and decompression is IO, I'll see about making CompressorException extend IOException.

Hi @garydgregory ,

Thanks for the feedback!. I've updated all the @since tags to reflect version 5.5 as suggested.  Regarding the `CompressorException` wrapping `IOException`, I’ll wait for updates on whether it will extend IOException in Commons Compress. This will determine the best course of action here.

Let me know if there’s anything else to address.

garydgregory · 2024-11-16T02:06:45Z

Hi @arturobernalg,

What do you think about apache/commons-compress#605

arturobernalg · 2024-11-16T12:58:48Z

Hi @arturobernalg,

What do you think about apache/commons-compress#605

Hi @garydgregory
The changes make sense. Thank you.

garydgregory · 2024-11-16T13:59:30Z

@arturobernalg

Thank you for the review on apache/commons-compress#605, I merged that PR.

arturobernalg · 2024-11-18T07:46:04Z

@arturobernalg

Thank you for the review on apache/commons-compress#605, I merged that PR.

All right. Let me know if we need to do another change.

arturobernalg requested a review from garydgregory September 19, 2024 20:08

garydgregory requested changes Sep 19, 2024

View reviewed changes

arturobernalg force-pushed the HTTPCLIENT-1843-5 branch from a5a5890 to 48de43f Compare September 19, 2024 20:41

arturobernalg requested a review from garydgregory September 19, 2024 20:41