[HTTPCLIENT-1843] - Delegate compression handling to Apache Commons Compress by arturobernalg · Pull Request #580 · apache/httpcomponents-client

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.

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

+
+    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?

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

+    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.

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.

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

-            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

+ * @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

+     */
+    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

+        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

+    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

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


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

+     *
+     * @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

 * 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

+        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

+            // 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

+    }
+
+    /**
+     * 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

+        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.

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.

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 · 2025-03-17T17:56:04Z

Hi @garydgregory , do I need to do anything else for PR #580? Any timing to validate it? Thanks!

arturobernalg · 2025-05-02T17:05:22Z

@ok2c would you mind to validate this PR.

ok2c · 2025-05-03T10:56:37Z

@ok2c would you mind to validate this PR.

@arturobernalg Sure. Will do.

ok2c

@arturobernalg Unfortunately CompressingFactory in its present form does not work if commons-compress is not present on the classpath, but it should be

Exception in thread "main" java.lang.NoClassDefFoundError: org/apache/commons/compress/compressors/deflate/DeflateCompressorInputStream
	at org.apache.hc.client5.http.impl.classic.ContentCompressionExec.<init>(ContentCompressionExec.java:90)
	at org.apache.hc.client5.http.impl.classic.ContentCompressionExec.<init>(ContentCompressionExec.java:106)
	at org.apache.hc.client5.http.impl.classic.HttpClientBuilder.build(HttpClientBuilder.java:1048)
	at org.apache.hc.client5.http.impl.classic.HttpClients.createDefault(HttpClients.java:57)
	at org.example.App.main(App.java:16)
Caused by: java.lang.ClassNotFoundException: org.apache.commons.compress.compressors.deflate.DeflateCompressorInputStream
	at java.net.URLClassLoader.findClass(URLClassLoader.java:387)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:418)
	at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:352)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:351)
	... 5 more

Your code must use reflection to see if commons-compress is present before trying to call any of its functions and it must not fail if it is not present.

There is still a lot of work that needs to be done here.

ok2c · 2025-05-04T14:51:10Z

    </dependency>
+    <dependency>
+      <groupId>org.apache.commons</groupId>
+      <artifactId>commons-compress</artifactId>


@arturobernalg This is wrong. commos-compress must be optional

ok2c · 2025-05-04T14:54:47Z

+        <groupId>org.apache.commons</groupId>
+        <artifactId>commons-compress</artifactId>
+        <version>${commpress.version}</version>
+        <optional>true</optional>


@arturobernalg I do not think this has any effect. Anyway, dependencyManagement section is the wrong place to declare artifact's optionality. This needs to be declared in the module's dependecies section, not here.

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