Replace the old InputStreamFactory registries with the new Decoder / Encoder pipeline #660

arturobernalg · 2025-06-25T13:11:28Z

This change swaps out the legacy compression infrastructure for the new decoder / encoder model introduced in 5.6. The codebase now routes all (de)compression through ContentCodecRegistry, uses functional interfaces instead of InputStreamFactory, and updates tests and docs accordingly while preserving existing public classes as deprecated shims. Everything builds and the full test suite passes.

garydgregory · 2025-06-25T17:45:20Z

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

 * {@link org.apache.hc.core5.http.io.entity.HttpEntityWrapper} responsible for
 * handling br Content Coded responses.
 *
+ * @deprecated Use Decoder API


I think it best to use '@link' in '@deprecated' comments. It makes it much easier to follow the bouncing ball 😉

garydgregory · 2025-06-25T17:46:31Z

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

 /**
 * Factory for decorated {@link InputStream}s.
- *
+ * @deprecated


The comment with an '@link' is missing, which happens in many places in the PR.

garydgregory · 2025-06-25T17:53:03Z

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

+ * @since 5.6
+ */
+@FunctionalInterface
+public interface Encoder {


We could reuse or extend UnaryOperator instead of inventing our own.

Prefer a dedicated Encoder over UnaryOperator – the name conveys intent and leaves room for HTTP-specific defaults later. Keeps symmetry with Decoder and avoids leaking java.util.function into the public API.

garydgregory · 2025-06-25T17:58:45Z

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

+    /**
+     * Custom decoders keyed by {@link ContentCoding}.
+     *
+     * @since 5.6


We don't need '@SInCE' tags for private code.

arturobernalg · 2025-06-26T09:10:59Z

Added a terse Javadoc note that points users to the new Decoder/Encoder API.

ok2c · 2025-06-28T10:09:51Z

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

+ * @since 5.6
+ */
+@FunctionalInterface
+public interface Decoder {


@arturobernalg Should not Decoder be working the same was as Encoder and unwrap HttpEntity?

@ok2c Decoder only swaps the input stream—DecompressingEntity already wraps the HttpEntity—so returning an InputStream keeps the API minimal and avoids duplicating header-handling logic.

@arturobernalg to me it is more important for the API to be logically consistent than saving a few lines of header handling code that will likely get optimized out by the compiler. In my opinion The Encoder / Decoder interfaces should work either with InputStream / OutputStream or with HttpEntity, but not with a mix. Feel free to leave it as is and merge the change-set but I will likely try to normalize the APIs later.

@ok2c please do another pass.

ok2c · 2025-06-29T09:12:49Z

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

+    /**
+     * Returns the {@link Codec} (or {@code null}).
+     */
+    public static Codec codec(final ContentCoding coding) {


@arturobernalg Here we have a scope violation: a public method exposing a package private class. Either make the method package private or make the class public. It is internal anyway.

ok2c · 2025-06-29T09:18:19Z

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

+ * @since 5.6
+ */
+@FunctionalInterface
+public interface Decoder {


@arturobernalg to me it is more important for the API to be logically consistent than saving a few lines of header handling code that will likely get optimized out by the compiler. In my opinion The Encoder / Decoder interfaces should work either with InputStream / OutputStream or with HttpEntity, but not with a mix. Feel free to leave it as is and merge the change-set but I will likely try to normalize the APIs later.

ok2c · 2025-07-04T15:47:56Z

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

+     * @throws IOException if the decoding entity cannot be created or an
+     *                     underlying I/O error occurs
+     */
+    HttpEntity wrap(HttpEntity src) throws IOException;


@arturobernalg I think this method does not need to throw IOException. At least it does not appear to be thrown anywhere in the existing code.

I also agree with @garydgregory here. I think we could just use standard Function here, but this is a matter of taste so I will not object.

Looks good otherwise.

HI @ok2c please do another pass. Now I'm using java functions instead.

HI @ok2c please do another pass. Now I'm using java functions instead.

@arturobernalg Oh, man. I meant something different. I think IOFunction was fine. It makes sense to have a function that throws IOException. What I meant the Decoder and Encoder could be just UnaryOperator or Function as suggested by @garydgregory

ok2c

@arturobernalg I would prefer to have IOFunction restored and Encoder and Decoder replaced but this is more of a matter of taste. You can keep them if you strongly prefer it

arturobernalg · 2025-07-07T11:37:08Z

@arturobernalg I would prefer to have IOFunction restored and Encoder and Decoder replaced but this is more of a matter of taste. You can keep them if you strongly prefer it

@ok2c i just

@arturobernalg I would prefer to have IOFunction restored and Encoder and Decoder replaced but this is more of a matter of taste. You can keep them if you strongly prefer it

HI @ok2c i just implement the UnaryOperator instead. please do another pass.

arturobernalg · 2025-07-10T13:25:02Z

If I hear no complaints or no feedback within 1 day I will merge the PR as is.

rhernandez35 · 2025-07-10T14:52:28Z

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

+
+import java.io.IOException;
+
+import org.conscrypt.Internal;


This is the wrong @Internal import. You want org.apache.hc.core5.annotation.Internal.

rhernandez35 · 2025-07-10T14:57:02Z

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

+            return decoder.apply(super.getContent());
+        }
+
+        InputStream local = cached;


Do we actually need DCL here? writeTo isn't thread-safe or idempotent, so under what scenarios do we expect getContent() to be called from multiple threads?

DCL is a cheap guard against accidental multi-thread reuse of the same entity instance, ensuring we don’t create two competing decoder streams.

…Encoder pipeline. Adds ContentCodecRegistry, rewrites DecompressingEntity, updates HttpClient internals and tests, deprecates legacy helpers, and keeps backward compatibility.

…ic Encoder/Decoder API, introduce IOFunction and ContentCodecRegistry, update DecompressingEntity and all tests accordingly.

ok2c · 2025-07-15T07:28:47Z

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

    private Lookup<AuthSchemeFactory> authSchemeRegistry;
    private Lookup<CookieSpecFactory> cookieSpecRegistry;
+    @Deprecated
    private LinkedHashMap<String, InputStreamFactory> contentDecoderMap;


@arturobernalg I found an issue with this change-set that is easily fixable.

At the moment the contentDecoderMap variable is essentially write-only. There is a non-deprecated setter for it but it is not being read anywhere. That is wrong.

What needs to be done is this: contentDecoderMap must be removed, the #setContentDecoderRegistry setter must be changed to populate contentDecoder through an adaptor (see DefaultHttpClientConnectionOperator for an example ) and the #setContentDecoderRegistry must be deprecated.

contentDecoder also sounds like a rather bad choice of a name for what is essentially a map of objects

@arturobernalg I found an issue with this change-set that is easily fixable.

At the moment the contentDecoderMap variable is essentially write-only. There is a non-deprecated setter for it but it is not being read anywhere. That is wrong.

What needs to be done is this: contentDecoderMap must be removed, the #setContentDecoderRegistry setter must be changed to populate contentDecoder through an adaptor (see DefaultHttpClientConnectionOperator for an example ) and the #setContentDecoderRegistry must be deprecated.

contentDecoder also sounds like a rather bad choice of a name for what is essentially a map of objects

@ok2c please take a look fd30e92

arturobernalg requested a review from ok2c June 25, 2025 13:11

garydgregory reviewed Jun 25, 2025

View reviewed changes

arturobernalg force-pushed the content-codec-registry branch 2 times, most recently from 3915567 to d0ed410 Compare June 26, 2025 09:10

arturobernalg requested a review from garydgregory June 27, 2025 12:09

ok2c reviewed Jun 28, 2025

View reviewed changes

arturobernalg requested a review from ok2c June 28, 2025 16:50

ok2c reviewed Jun 29, 2025

View reviewed changes

ok2c reviewed Jul 4, 2025

View reviewed changes

arturobernalg requested a review from ok2c July 5, 2025 12:24

arturobernalg force-pushed the content-codec-registry branch from c591d1e to 24ac6ca Compare July 5, 2025 12:59

ok2c approved these changes Jul 5, 2025

View reviewed changes

arturobernalg force-pushed the content-codec-registry branch 2 times, most recently from 8c31bf1 to 474c669 Compare July 7, 2025 11:32

arturobernalg requested a review from ok2c July 7, 2025 11:37

ok2c approved these changes Jul 8, 2025

View reviewed changes

rhernandez35 reviewed Jul 10, 2025

View reviewed changes

arturobernalg force-pushed the content-codec-registry branch from d1a6b23 to 2987731 Compare July 10, 2025 16:32

arturobernalg added 3 commits July 14, 2025 12:55

Replace the old InputStreamFactory registries with the new Decoder / …

5e58282

…Encoder pipeline. Adds ContentCodecRegistry, rewrites DecompressingEntity, updates HttpClient internals and tests, deprecates legacy helpers, and keeps backward compatibility.

Unify content-coding support: replace InputStreamFactory with symmetr…

3ff70de

…ic Encoder/Decoder API, introduce IOFunction and ContentCodecRegistry, update DecompressingEntity and all tests accordingly.

Use UnaryOperator instead

d4cc112

arturobernalg force-pushed the content-codec-registry branch from 2987731 to d4cc112 Compare July 14, 2025 10:56

arturobernalg merged commit 9099978 into apache:master Jul 14, 2025
10 checks passed

ok2c reviewed Jul 15, 2025

View reviewed changes

reta mentioned this pull request Jan 3, 2026

Bump Apache HttpClient5 to 5.6 and Apache HttpCore5 to 5.4 opensearch-project/OpenSearch#20358

Merged

3 tasks


		import java.io.IOException;

		import org.conscrypt.Internal;

Replace the old InputStreamFactory registries with the new Decoder / Encoder pipeline #660

Replace the old InputStreamFactory registries with the new Decoder / Encoder pipeline #660

Uh oh!

Conversation

arturobernalg commented Jun 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garydgregory Jun 25, 2025 • 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!

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!

arturobernalg commented Jun 26, 2025

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!

ok2c left a comment

Choose a reason for hiding this comment

Uh oh!

arturobernalg commented Jul 7, 2025

Uh oh!

arturobernalg commented Jul 10, 2025

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

garydgregory Jun 25, 2025 •

edited

Loading