HTTPCLIENT-1843 Plug Commons-Compress into HttpClient’s automatic #651

arturobernalg · 2025-06-17T16:59:00Z

             content-decoding (optional)

New ContentDecoderRegistry discovers extra codecs (br, zstd, xz, lz4, …) via Commons-Compress when that jar is on the class-path; otherwise falls back to the built-ins (gzip, deflate) only.
No hard dependency added—projects that need the extra algorithms just add commons-compress (and helper jars like google-brotli, zstd-jni, xz-java) to their pom and HttpClient uses them automatically.

arturobernalg · 2025-06-17T17:00:07Z

@ok2c @garydgregory — could you please review this PR when you have a moment? I’ve closed the earlier one because I opted for a cleaner approach here.
Thank you!

ok2c

@arturobernalg It is much better and can basically be merged as it, but it can be made better still.

CommonsCompressDecoderFactory uses reflection. I does not have to. It is perfectly OK to have Commons Compress as a optional compile dependency. what is important that there is no direct dependency on Commons Compress in ContentDecoderRegistry, What #commonsCompressPresent in ContentDecoderRegistry does perfectly fine and sufficient. CommonsCompressDecoderFactory import Commons Compress classes directly.
REGISTRY in ContentDecoderRegistry should be unmodifiable. It is only a matter of tweaking #build method a little. This would make #snapshot unnecessary. Better yet you should probably use org.apache.hc.core5.http.config.Registry instead of a plan Map

garydgregory

TY @arturobernalg !

I agree with @ok2c comments, especially regarding reflection. I've made a few low-level comments throughout.

garydgregory · 2025-06-18T12:25:27Z

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

+     * Returns a <em>defensive copy</em> of the internal registry
+     * (key = canonical encoding token, value = {@link InputStreamFactory}).
+     */
+    public static Map<String, InputStreamFactory> snapshot() {


snapshot() is a bad method name here IMO, rename to getRegistry().
Saying this is a defensive copy is misleading because InputStreamFactory makes no thread-safety guarantees. I would say this is a "shallow copy" which is technically correct and doesn't imply intent of attack or defense for call sites.

garydgregory · 2025-06-18T12:26:38Z

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

+                    false,
+                    ContentDecoderRegistry.class.getClassLoader());
+            return true;
+        } catch (final ClassNotFoundException e) {


What about LinkageError?

garydgregory · 2025-06-18T12:28:19Z

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

+
+        final List<String> list = new ArrayList<>(4);
+        list.add("gzip");
+        list.add("x-gzip");


Maybe a later PR could gather all these magic strings ("x-gzip" and so on) into a class (existing or new).

garydgregory · 2025-06-18T12:29:22Z

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

+
+        /* 2. Optional Commons-Compress decoders (reflection, safe) */
+        if (commonsCompressPresent()) {
+            addCommons(m, "br");


See my other comment about magic strings, for a later PR.

garydgregory · 2025-06-18T12:30:41Z

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

+            Class.forName(cn, false,
+                    CommonsCompressDecoderFactory.class.getClassLoader());
+            return true;
+        } catch (final ClassNotFoundException e) {


What about LinkageError?

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

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

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

arturobernalg · 2025-06-18T17:15:43Z

Please @ok2c @garydgregory do another pass.

garydgregory

@arturobernalg
Look good. I
have some
comment
scattered

also, why
     not use longer
     lines?
         I'm not a fan
         of the
         newspaper format

;-)

garydgregory · 2025-06-18T18:14:40Z

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

+                    false,
+                    ContentDecoderRegistry.class.getClassLoader());
+            return true;
+        } catch (final ClassNotFoundException e) {


garydgregory · 2025-06-18T18:17:50Z

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

+        }
+    }
+
+    private enum Probe {


This feels a bit redundant with the new ContentCoding enum. What do you think about moving the "requires this type name" aspect (a.k.a. "probe class") of Probe to ContentCoding and turning this "Probe" into a Set<ContentCoding>?

garydgregory · 2025-06-18T18:20:36Z

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

+
+        final String enc, probeClass;
+
+        Probe(final String enc, final String probeClass) {


probeClass is misleading to me because it's a name, not an actual class. I think "requiredTypeName" or "requiredClassName" would be a clearer name because "probe" doesn't really tell you anything (to me).

garydgregory · 2025-06-18T18:23:10Z

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

+            this.probeClass = probeClass;
+        }
+
+        static String helperFor(final String enc) {


I would stay away from cryptic names like "enc", even in private code, if it's an "encoding", the call it that ;-)

garydgregory · 2025-06-18T18:25:14Z

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

+
+/**
+ * {@link InputStreamFactory} backed by
+ * <a href="https://commons.apache.org/proper/commons-compress/">Apache&nbsp;Commons Compress</a>.


If there's a non-breaking space between words 1 and 2, why not between words 2 and 3? I would just not bother since it might cause odd rendering on small screens like phones and tablets.

garydgregory · 2025-06-18T18:27:02Z

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

+ * {@link InputStreamFactory} backed by
+ * <a href="https://commons.apache.org/proper/commons-compress/">Apache&nbsp;Commons Compress</a>.
+ * <p>
+ * The class is compiled against Commons Compress but lives behind an <i>optional</i>


I would not use <i> in Javadocs, that's up to the rederer to decide what style to apply, which is why Javadoc supports <em> for ephasis which is usually translated to italics and <strong> which is usually translated to bold.

garydgregory · 2025-06-18T18:28:23Z

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

+ * </p>
+ *
+ * <h4>Run-time guards</h4>
+ * Some encodings (e.g.&nbsp;{@code br}, {@code zstd}, {@code xz/lzma})


In general, I stay away from Latin abbreviations, I find it doesn't make the text clearer to read. No need for non-breaking space IMO.

garydgregory · 2025-06-18T18:29:48Z

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

+ * <a href="https://commons.apache.org/proper/commons-compress/">Apache&nbsp;Commons Compress</a>.
+ * <p>
+ * The class is compiled against Commons Compress but lives behind an <i>optional</i>
+ * dependency.  At run-time it will only be loaded when the library is present,


You don't need two spaces after a .; we're not using typewriters anymore and Javadoc is not rendered in mono-space ;-)

please @garydgregory do another pass

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

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

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

content-decoding (optional) * New ContentDecoderRegistry discovers extra codecs (br, zstd, xz, lz4, …) via Commons-Compress when that jar is on the class-path; otherwise falls back to the built-ins (gzip, deflate) only. * No hard dependency added—projects that need the extra algorithms just add `commons-compress` (and helper jars like google-brotli, zstd-jni, xz-java) to their pom and HttpClient uses them automatically.

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

garydgregory

LGMT

arturobernalg requested a review from ok2c June 17, 2025 16:59

ok2c reviewed Jun 18, 2025

View reviewed changes

garydgregory requested changes Jun 18, 2025

View reviewed changes

arturobernalg force-pushed the HTTPCLIENT-1843-5 branch 2 times, most recently from 95f58c5 to 7ea87d5 Compare June 18, 2025 17:11

arturobernalg requested review from garydgregory and ok2c June 18, 2025 17:15

garydgregory reviewed Jun 18, 2025

View reviewed changes

arturobernalg force-pushed the HTTPCLIENT-1843-5 branch from 404fb79 to 5889705 Compare June 19, 2025 07:39

arturobernalg requested a review from garydgregory June 19, 2025 07:42

ok2c reviewed Jun 19, 2025

View reviewed changes

arturobernalg force-pushed the HTTPCLIENT-1843-5 branch from fff3ab4 to f566906 Compare June 19, 2025 08:52

arturobernalg requested a review from ok2c June 19, 2025 08:56

ok2c reviewed Jun 19, 2025

View reviewed changes

httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/ContentCompressionExec.java Show resolved Hide resolved

ok2c approved these changes Jun 19, 2025

View reviewed changes

garydgregory approved these changes Jun 19, 2025

View reviewed changes

arturobernalg merged commit d1eb5b8 into apache:master Jun 19, 2025
22 of 24 checks passed


		final String enc, probeClass;

		Probe(final String enc, final String probeClass) {

HTTPCLIENT-1843 Plug Commons-Compress into HttpClient’s automatic #651

HTTPCLIENT-1843 Plug Commons-Compress into HttpClient’s automatic #651

Uh oh!

Conversation

arturobernalg commented Jun 17, 2025

Uh oh!

arturobernalg commented Jun 17, 2025

Uh oh!

ok2c left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garydgregory left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arturobernalg commented Jun 18, 2025

Uh oh!

garydgregory left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

garydgregory left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ok2c left a comment •

edited

Loading