Streaming decoder support2

[trumpetinc]

Adds support for streaming decode for Feign RequestLine methods that return InputStream or Reader.

To use:

  interface LargeStreamTestInterface {

    @RequestLine("GET /")
    InputStream getLargeStream();

    @RequestLine("GET /")
    Reader getLargeReader();
  }

public void test(){
  try(InputStream is = myLargeStreamTestInterface.getLargeStream()){
     // process the is
  }
}

Changes

1. InvocationContext now leaves the response stream open if the return type from the decoder implements Closeable
2. New InputStreamAndReaderDecoder class that can be registered with the Feign builder.decoder() method. Supports passing the decode request to a delegate if the method return type is not InputStream or Reader. If the return type is Reader, the charset of the response Content-Type header is used. If no charset is specified in the header, UTF-8 is assumed.
3. New ContentTypeParser utility method for obtaining information from the Content-Type header

[trumpetinc]

Fixing problems with initial git branch ( #3494 )

[trumpetinc]

@velo let's try this - I have no idea what I messed up with the git branching on the earlier PR. This PR is as clean as I can make it. I branched from the latest 14.x and made the required changes.

CircleCI is already complaining about build problems (certainly nothing to do with this PR itself??):

wget: Failed to fetch https://downloads.apache.org/maven/mvnd/1.0.2/maven-mvnd-1.0.2-linux-amd64.zip

But at least security snyk checked out this time.

[trumpetinc]

@velo this is ready for review. I implemented it as a separate decoder class - if you prefer that this functionality be implemented in DefaultDecoder instead, we can do that - but I'll need you to merge the latest PredicatedDecoder changes into the 14.x branch (DefaultDecoder in 14.x isn't a PredicatedDecoder).

[velo]

Thanks for this — streaming decoder support is a real gap. Two structural issues before this can merge, both of which also shrink the diff a lot once fixed:

1. InvocationContext.proceed() infers "don't close the response" from whether the declared return type is Closeable, on the single shared decode path every Feign call goes through. This duplicates a mechanism that already exists: BaseBuilder.doNotCloseAfterDecode(), explicitly documented for "lazy-evaluated constructs," where the custom decoder is responsible for closing. As written, any existing method that returns any Closeable-implementing type through any decoder (not just the new one) will silently stop having its response closed — a connection-leak risk with no opt-in and no test guarding it. It's also inconsistent with itself: wrap the same return type in TypedResponse<InputStream> and rawType is no longer Closeable, so the response gets closed anyway, breaking the exact case this was added for. Please drop the Closeable/noClose addition entirely and have streaming callers use the existing doNotCloseAfterDecode() flag, with the new decoder taking ownership of closing as the existing contract already requires of custom decoders.

2. ContentTypeParser.parseContentTypeHeader reimplements Response.charset() (same split-on-;-then-= logic, even the same contentTypeParmeters typo) as new public API (ContentTypeResult, with an unused getContentType()), and ships with a // TODO admitting it doesn't implement the full parser spec, with no dedicated test. StringDecoder already reuses Response.charset() for this. Please drop ContentTypeParser/ContentTypeResult and call response.charset() directly in the Reader branch. If quoted-charset support is a real gap, that's worth fixing in Response.charset() itself rather than a second parallel implementation.

Smaller cleanup while you're in there:

• The blank-line-only change in DefaultDecoder.java looks like unintentional diff noise.
• The .mvn/wrapper/maven-wrapper.properties mvnd URL fix looks like an unrelated fix that landed on this branch — worth splitting into its own PR so this one's diff stays reviewable on its own terms.
• InputStreamAndReaderDecoderTest starts MockWebServer instances but doesn't stop them — worth adding @AfterEach cleanup.

Happy to take another look once the two structural points are addressed — the feature itself (an InputStream/Reader-capable decoder) is worth having, it just needs to lean on the existing extension points instead of adding parallel ones.

[trumpetinc]

@velo good feedback, thank you.

I could use your guidance on one of your requests: doNotCloseAfterDecode is a FeignBuilder method. I do not see how we can make a single decoder set that value. And given the move to MultiDecoder, I am certain that we don't want to force the user to have to create a separate Feign instance for streaming decoders vs others.

Potential approaches

1. Add a doNotCloseAfterDecode setting in Response (which would then pass to Request and be checked/honored in the InvocationContext). And the default value of that would pull from the Feign builder setting (maintaining backwards compatibility).

Concern with that is that Response is currently immutable. So we are talking about making it mutable - AND requiring a side-effect from the Decoder. Not great.

2. Make Decoder return a DecodeResult object that encapsulates the decoded Object, along with an autoClose argument.

Downside of this is that we have to change the method signature of Decoder. And because DecodeResult is itself an Object, we have to be a little careful to ensure there are no code paths that don't properly process the DecodeResult. I'm not overly concerned with this downside, but it does need to be considered.

3. Add a shouldAutoclose() method to the Decoder interface (with default method returning false).

Downside here is that this sort of approach has a way of really cluttering an interface spec. Over time, we wind up adding a bunch of boolean methods to an interface to control different aspects of the processing. Wearing my architecture hat, I am not crazy about this option - even though it is probably the least disruptive.

4. Add a getDecoderHints() method to the Decoder interface (with default method returning a default Hints instance). For now, DecoderHints would have a single shouldAutoClose() property, but we could add others without further cluttering the interface.

Preference

My preference would be option 2 - it is a better design overall. But it absolutely makes this a v14 change (breaking). So you may prefer option 3 or 4.

How you would like me to pursue this?

[trumpetinc]

@velo can you please get the change from master:/feign/.mvn/wrapper/maven-wrapper.properties merged into OpenFeign:14.x? That will be necessary for CI to run on this PR. Thanks.

[trumpetinc]

1. ContentType parser has been removed (I still think that centralizing parsing of content-type headers is eventually desirable, but that doesn't need to be done here).
2. DefaultDecoder blank line has been reverted - thanks for catching that
3. MockWebServer is now started and stopped using a try-with-resources block in each test

I have reverted the change to the maven config file - as soon as the mvn config change is merged from master to 14.x, the CI build will work again.