Port multi-encoder to master using predicates

[velo]

Ports the multi-encoder feature from #3485 to master.

Credit: this is @yvasyliev's and @trumpetinc's work — commits carry their authorship and co-author trailers.

Usage

Most first-party encoders now declare what they can handle, so they can simply be listed:

Feign.builder().encoder(new DefaultEncoder(), new GsonEncoder(), new JAXBEncoder())

For an encoder that does not declare itself — including one you do not control — pair it with a predicate:

MultiEncoder.builder(new DefaultEncoder())
    .add(new GsonEncoder())                                  // self-declaring
    .add(EncoderPredicate.xmlContentType(), someXmlEncoder)  // paired
    .add((object, bodyType, template) -> bodyType == byte[].class, binary)
    .build()

The first argument is the default encoder, consulted last. Delegates are tried in the order added.

Why a port instead of a cherry-pick

#3485 targets 14.x and could not come across as-is:

1. Its foundation is Encoder#encode changing void -> boolean, which is source and binary breaking for every third-party Encoder. Legitimate on 14.x, not on 13.14.
2. Every file lives under the api/ module and the feign.core.codec package, neither of which exists on master.

A predicate carries the "can you handle this?" decision instead, so Encoder is untouched and every existing encoder keeps working.

Design

Settled over #3485 (comment) and #3485 (comment). Four shapes were prototyped as compiling Java before picking this one; notes in the "Alternatives rejected" section below.

• EncoderPredicate — @FunctionalInterface, boolean canEncode(Object, Type, RequestTemplate), with jsonContentType(), xmlContentType(), contentType(mediaType), emptyBody(), bodyType(type), formEncoded() and and/or/negate
• PredicatedEncoder extends Encoder — @FunctionalInterface; canEncode is a default returning true, so encode() stays the single abstract method
• MultiEncoder.builder(defaultEncoder) — .add(PredicatedEncoder) and .add(EncoderPredicate, Encoder)
• BaseBuilder.encoder(Encoder defaultEncoder, PredicatedEncoder... encoders)
• Util.isJsonContentType / isXmlContentType / hasContentType

The whole surface is @Experimental while the shape settles — a MultiDecoder counterpart would likely influence it.

Encoders granted PredicatedEncoder

All keep implements Encoder explicitly.

• JSON (Util.isJsonContentType): Gson, Jackson, Jackson 3, Jackson Jr, Jackson JAXB, Moshi, Fastjson2, JSON-java
• XML (Util.isXmlContentType): JAXB, JAXB Jakarta, SOAP, SOAP Jakarta
• Delegating (forwards canEncode to the wrapped encoder): the three MeteredEncoders

Deliberately left alone: DefaultEncoder (it is the fallback), FormEncoder/SpringFormEncoder (they already have their own delegate-fallback logic), and GraphqlEncoder (it rewrites the body to MAP_STRING_WILDCARD before delegating, so forwarding its delegate's predicate against the original bodyType would be wrong).

Capabilities interaction

Verified by test (MultiEncoderCapabilityTest), not by assertion:

• Capability.enrich(Encoder) wraps the MultiEncoder as one unit; routing is unaffected. Capabilities never reach the delegates, so nothing is erased.
• A wrapper that does not forward canEncode claims every request, since the default accepts everything. That is why the metrics modules forward it, and it is documented on PredicatedEncoder and in the README.
• Granularity note: micrometer now reports one feign.Encoder timer for the composite rather than per format.

Alternatives rejected

• Capability mixin (implements Encoder, PredicatedEncoder where MultiEncoder takes plain Encoders and asks via instanceof): an encoder that has not opted in accepts everything and silently becomes a greedy catch-all, making the default encoder unreachable. Ordering would decide correctness with no signal at the call site.
• Subtype only, no pairing: opting in means changing the encoder's declared type, which is impossible for a third-party encoder.
• Wrapper only (the first draft of this PR): cannot express "encoder declares its own capability"; everything has to be wrapped at the call site.

Deviations from #3485

• PredicatingEncoder -> PredicatedEncoder, and the predicate method is canEncode rather than test.
• The upstream content-type check does an exact-match headers().getOrDefault("Content-Type", ...) lookup. Since avoid duplicate Content-Length header in DefaultClient #3451 was the same bug one header field over, the header name is now matched case-insensitively and the regexes are compiled Pattern constants. Covered by a test.

Docs

• README gains a "Multiple encoders" section under Encoders, flagged experimental, covering both call styles, the ordering rule, how to declare your own encoder, and the forward-canEncode contract for wrappers.
• The feature mindmap gains a "Multi encoder (predicate based, experimental)" node.

Verification

• core plus all 15 touched modules, plus graphql/form/form-spring: green
• Whole-reactor test-compile: only feign-jaxrs3 fails, identically on plain master (javax.ws.rs.client.ClientBuilder not found) — pre-existing and unrelated
• git-code-format-maven-plugin:validate-code-format passes
• Java 8 bytecode level respected in main sources

Follow-ups (not in this PR)

• MultiDecoder counterpart — the decode side was never covered by Multi encoder #3485.
• Capability.invoke calls method.invoke without setAccessible(true), so a capability declared as a private class fails with Unable to enrich .... Pre-existing, unrelated, found while writing the capability test.

[velo]

@yvasyliev @trumpetinc — this is your multi-encoder work from #3485 ported to master, and I'd really like your eyes on it before it goes in. GitHub won't let me add you as formal reviewers (neither of you is a repo collaborator, so the API refuses), hence the mention.

The commits carry your authorship and co-author trailers, so the credit stays with you.

What had to change, and why. #3485 is built on Encoder#encode returning boolean, which is source and binary breaking for every third-party Encoder. That's fine on 14.x but can't land on 13.14. The other blocker was purely structural: everything in #3485 lives under the api/ module and the feign.core.codec package, and neither exists on master. So a predicate carries the "can you handle this?" decision instead, and Encoder is left completely untouched.

Where it landed, after the direction in #3485 (comment) and #3485 (comment):

// encoders that declare what they handle can just be listed
Feign.builder().encoder(new DefaultEncoder(), new GsonEncoder(), new JAXBEncoder())

// anything that doesn't declare itself gets paired with a predicate
MultiEncoder.builder(new DefaultEncoder())
    .add(new GsonEncoder())
    .add(EncoderPredicate.xmlContentType(), someXmlEncoder)
    .add((object, bodyType, template) -> bodyType == byte[].class, binary)
    .build()

PredicatedEncoder extends Encoder with canEncode as a default method, so encode() stays the only abstract method and it's still a real @FunctionalInterface. The first-party JSON and XML encoders now declare themselves.

Specifically useful to hear from you:

• @yvasyliev — your original DelegatingEncoder used canEncode as an abstract method on Encoder before the boolean refactor replaced it. This is close to that first design, with canEncode moved onto a sub-interface as a default so nothing breaks. Does that land where you were originally headed, or did the boolean version solve something this loses?
• @trumpetinc — your PredicatingEncoder wrapper became PredicatedEncoder as a functional interface encoders implement directly, and the pairing you had at the call site moved into MultiEncoder.Builder.add(predicate, encoder). I think that keeps the separation of concerns you argued for while also covering encoders you don't control. Fair reading?
• Anyone's view on the ordering rule: delegates are consulted in registration order, so Content-Type: application/json with a null body is claimed by a JSON encoder before EncoderPredicate.emptyBody() gets a look. Documented, but I'm open to something less footgun-y.

One thing worth flagging: a wrapper that doesn't forward canEncode claims every request, because the default accepts everything. That's why the metrics modules' MeteredEncoder now forwards, and there's a test pinning it. If you can think of other wrapping encoders that need the same treatment, I'd like to catch them now.

The whole surface is @Experimental for the moment, so there's room to move before it's frozen. MultiDecoder is the obvious follow-up — #3485 never covered the decode side.

[velo]

BTW, if you want to re-open this PR with you as author, I can close it and leave to you as author

[trumpetinc]

@velo @yvasyliev

Quick Summary of my understanding
So with this approach, MultiEncoder only works with PredicateEncoders (plus a single legacy encoder). And the predicate needs to take into account the internal capabilities of the wrapped encoder.

Then if we want the Encoder to "just work" without having to add a custom predicate for it, then that maker of the custom Encoder needs to implement PredicateEncoder and provide a predicate.

ok - I see what you are doing. It effectively adds the canEncode() method to any "modern" Encoder that wants to partake in the MultiEncoder paradigm. And it forces a semantic change based on participating in the new MultiExtractor mechanism instead of forcing everyone to adopt the change.

Any existing legacy Encoder won't be able to be part of MultiEncoder until it is updated.

I like this approach.

Question/Suggestion 1: Why not back-fill this for DefaultEncoder and say that MultiEncoder doesn't support a default encoder at all?

Question/Suggestion 2

I am a little concerned about the semantics of the overloaded BaseBuilder.encoder() method. When users see:

builder.encode(enc1, enc2, enc3)

they are going to expect that enc1 will be tried first, then enc2, then enc3. Not enc2, enc3, enc1.

Having the "default" encoder be first is really non-obvious.

I realize this will force a different method name on builder (like builder.encoders(PredicatedEncoder enc1, PredicatedEncoder enc2, PredicatedEncoder enc3) with an 's' at the end?), but maybe that is a better tradeoff than having the argument order be non-obvious?

I'm thinking that removing the concept of a default encoder from MultiEncoder will simplify things considerably, at a pretty low cost to existing users. Basically, existing users with existing non-predicate encoder continue using the existing encoder() method. Users who want to take advantage of chaining use encoders(). And encoder() can eventually be deprecated.

I look forward to hearing your thoughts.

[trumpetinc]

@velo

your PredicatingEncoder wrapper became PredicatedEncoder as a functional interface encoders implement directly, and the pairing you had at the call site moved into MultiEncoder.Builder.add(predicate, encoder). I think that keeps the separation of concerns you argued for while also covering encoders you don't control. Fair reading?

Yes - this addresses it cleanly, especially because PredicatedEncoder implements Encoder. You have basically inverted the problem in a very elegant way.

I believe that with your approach it would be possible to stack PredicatedEncoders right? So if someone wanted to be more selected with a GsonEncoder than the default GsonEncoder predicate, they could do that?

[trumpetinc]

@velo

your original DelegatingEncoder used canEncode as an abstract method on Encoder before the boolean refactor replaced it. This is close to that first design, with canEncode moved onto a sub-interface as a default so nothing breaks. Does that land where you were originally headed, or did the boolean version solve something this loses?

Removal of the default method (eventually replacing it with a returned boolean) was needed because it forces implementers to account for the change in semantics. You address this by forcing implementers to add implements PredicateEncoder to their code in order to participate in the new multi-encoder capability.

To be clear, the approach in this PR does address the semantic change issue - but I still recommend not having default on canEncode(). Here's why:

There is a reasonable discussion about how much we want to protect users from themselves, but if someone has a custom Encoder implementation, and they just slap PredicateEncoder on the implements line of the class, it is very unlikely to produce good results.

I really think that each PredicateEncoder needs to decide the exact criteria under which it could return true from this method. For JsonEncoder, not a problem - it can encode anything, so canEncode will always return true. But that is not the general case.

I think a little extra code on the built-in PredicateEncoder implementations is worth it to force Encoder implementors to explicitly declare their predicate.

I won't object if we leave it as default (because you have forced the developer to actually make changes to their code) - but I think it's still better to not give them a default that could shoot them in the foot.

[trumpetinc]

Anyone's view on the ordering rule: delegates are consulted in registration order, so Content-Type: application/json with a null body is claimed by a JSON encoder before EncoderPredicate.emptyBody() gets a look. Documented, but I'm open to something less footgun-y.

I think it should be driven by the order of the encoders. If we remove the concept of a default encoder from MultiEncoder (per my discussion above), then this becomes simple for the user to understand. If they want the DefaultEncoder to win, they should put it first.

Let's just make DefaultEncoder so it is a PredicateEncoder.

[trumpetinc]

One thing worth flagging: a wrapper that doesn't forward canEncode claims every request, because the default accepts everything. That's why the metrics modules' MeteredEncoder now forwards, and there's a test pinning it. If you can think of other wrapping encoders that need the same treatment, I'd like to catch them now.

I reviewed all of the encoders that we had to touch in https://github.com/OpenFeign/feign/pull/3485/changes and the only ones that I found that you didn't cover already were the form encoders.

FormEncoder and SpringFormEncoder currently wrap a delegate. We will eventually want to see people move to the new multipart encoding that @yvasyliev is proposing, so it may not be worth back-porting - but I don't think it would be difficult to make those a PredicatedEncoder.

The challenge is that for backwards compatibility we are going to have to continue allowing the delegate to be passed in on the constructor - but the FormEncoder doesn't know the predicate characteristics of the delegate, which means that it will be unable to accurately implement canEncode() that covers the delegate.

FormEncoder could check the type of it's delegate and pass canEncode() if possible, but if the user passes a custom Encoder (non-predicated) as delegate we are stuck.

---

Maybe the way to handle this is to intentionally not make FormEncoder predicated (like you have done). Then add a FormEncoder.createPredicatedFormEncoder() method (disallowing delegate, and not passing new DefaultEncoder() as a default delegate) to construct a new FormEncoder that is wrapped by PredicatedEncoder so form users can use the new multi-encoder functionality?

  public static PredicatedEncoder createPredicatedFormEncoder(){
    return new PredicatedEncoder(formContentTypePredicate, new FormEncoder(null)); // don't have source code on hand, so I don't know the details of the predicate that will be required for form - I think it checks for Map<String, String> body type plus a header?
  }

and handle the null delegate gracefully...

[velo]

@trumpetinc — all of this landed, thanks. Point by point:

The default encoder is gone. You were right that encoder(enc1, enc2, enc3) trying enc1 last is non-obvious, so there is no default slot any more. MultiEncoder consults its encoders in registration order and that is the whole rule. The builder method is encoders(...), plural, as you suggested; encoder(Encoder) is untouched and not deprecated.

Feign.builder()
    .encoders(
        new JacksonEncoder(),
        new JAXBEncoder(),
        PredicatedEncoder.of(EncoderPredicate.any(), new DefaultEncoder())); // the default, last

A default is now just an encoder with a yes-man predicate. EncoderPredicate.any() matches everything, so the fallback is registered like anything else — last, and visibly so. I did not make DefaultEncoder a PredicatedEncoder: an encoder that silently claims every request is exactly the thing that bites people when it ends up in the wrong position, and PredicatedEncoder.of(EncoderPredicate.any(), ...) says out loud what is happening at the call site.

Nothing matching is an error, not a silent fallback. Predicates describe themselves, so the failure can say what it considered:

Unable to encode java.lang.String (Content-Type: text/plain) for POST /orders. Encoders tried, in order:
  - JacksonEncoder
  - JAXBEncoder when Content-Type is XML
Add an encoder guarded by EncoderPredicate.any() last to act as a default.

EncoderPredicate.jsonContentType(), contentType(...), bodyType(...) etc. all carry a description, and/or/negate compose them, and EncoderPredicate.describedAs("it is Tuesday", lambda) names your own.

canEncode has no default any more. You talked me round: an encoder that says nothing would claim everything, and implements PredicatedEncoder on an existing class should not compile into a lie. PredicatedEncoder is therefore no longer a @FunctionalInterface. Lambdas did not go away though — EncoderPredicate is the functional interface, and PredicatedEncoder.of(predicate, encoder) is how a lambda gets attached to an encoder you do not own:

PredicatedEncoder.of((object, bodyType, template) -> bodyType == byte[].class, binaryEncoder);

Stacking works, in both directions. of replaces whatever the encoder declares (so you can point Gson at a vendor content type it would otherwise refuse), narrowing keeps the encoder's own declaration and ANDs yours onto it:

PredicatedEncoder.narrowing(
    EncoderPredicate.contentType("application/vnd.acme+json"), new GsonEncoder());
// canEncode == (Content-Type is application/vnd.acme+json and GsonEncoder accepts it)

The form encoders got your factory. FormEncoder.createPredicatedFormEncoder() and SpringFormEncoder.createPredicatedFormEncoder() return a delegate-free form encoder guarded by FormEncoder.formRequests() — a form or multipart Content-Type carrying a map or a user pojo:

Feign.builder()
    .encoders(FormEncoder.createPredicatedFormEncoder(), new JacksonEncoder());

The constructors still take a delegate for backwards compatibility, and new FormEncoder(null) is handled: anything the form encoder cannot encode itself fails with an EncodeException naming the body type, instead of NPE-ing on a missing delegate. The existing FormEncoder/SpringFormEncoder stay plain Encoders, for the reason you gave — they cannot honestly declare a delegate's applicability.

On the ordering footgun (Content-Type: application/json with a null body being claimed by JSON before emptyBody()): with the default slot gone, order is the only rule, so put emptyBody() first if that is what you want. That felt like the smallest thing to explain.

Same treatment is going onto the decode side in #3528.

[trumpetinc]

@velo "footgun" is my new favorite phrase - I'll be using that in my design sessions - thank you for the laugh.

Some things to consider:

1. I still think DefaultEncoder could/should be a PredicateEncoder - the predicate would be:

    return (bodyType == String.class || bodyType == byte[].class);

2. Observation: DefaultEncoder could easily be implemented as a MultiEncoder (not saying it should be, just pointing out that composition is now possible).

3. I think it is a good idea to make it possible to add a MultiEncoder to another MultiEncoder. In my code review (which I'll submit in a second), I recommend having MultiEncoder itself be a PredicatedEncoder. This opens up the possibility of libraries contributing sets of encoders.

   For example, if we wanted to make the upcoming StreamingEncoders modular (instead of adding streaming to DefaultEncoders), we could have a StreamingFeign.encoders() convenience method that would return a MultiEncoder with all of the streaming encoders. The user could then added streaming support to the builder.encoders(...) method without having to add each individual streaming encoder.

[trumpetinc]

Instead of reporting only the content-type header, I suggest enumerating all of the headers in the template. This will provide more useful info for users who specify additional request headers via annotations, etc...

[trumpetinc]

I do not think that adding encoders with predicate .any() should be general guidance. It could also be confusing for users who are using builder.encoders() as the concept of predicates is going to be a hidden implementation detail for most users.

Suggested wording "Add an encoder that supports processing of this request".

[trumpetinc]

suggest throwing exception if encoders is empty

[trumpetinc]

Suggest making MultiEncoder implement PredicatedEncoder.

This is easy to implement and opens up all sorts of possibilities for composition.

Here's the predicate:

   return encoders.stream().anyMatch(e -> e.canEncode(object, bodyType, template));

[trumpetinc]

I'm trying to wrap my head around the need for PairedEncoder.narrow() - couldn't we achieve the same thing with multiEncoderBuilder.add(newPredicate, originalPredicatedEncoder) ? The newPredicate will be evaluated first, then if it passes, the predicate in the originalPredicateEncoder would be evaluated.

I think this is effectively the EncoderPredicate.and() that is happening in PairdEncoder.narrow() ?

[trumpetinc]

I retract my comment on this. narrow() allows users to add a non-predicate encoder to the builder.encoders() method without having to mess with MultiEncoder.builder(). That makes sense. It also allows tweaking the predicate without having to use MultiEncoder.builder() directly.

[trumpetinc]

The default toString method includes the class name pretty cleanly - am I missing a corner case where encoder.toString() would be insufficient on it's own?

Basically, is the reflection here worth it?

[trumpetinc]

I retract my comment on this. Making it easy for users to troubleshoot their configuration issues is good - and a little effort making the messages easier to read is worth it.

[trumpetinc]

FYI - I have added a more robust content type header parser to one of the other PRs I have outstanding ( https://github.com/OpenFeign/feign/pull/3494/changes#diff-3d5ee4752b168285974eb09fc4782f489edeadba936b5c71dc59ff6a043d779d )

There is a lot of one-off code in Feign related to parsing the content type header, would be good to centralize that!

[trumpetinc]

minor thing, but this comment is a little hard to read...

Suggest adding 'Creates a predicate for' to the beginning:

"Creates a predicate for requests a delegate-less form encoder can handle"

[trumpetinc]

At some point, it may be worth deprecating and calling out that the preferred way to specify downstream encoders is to use builder.encoders() instead of the delegate. I get why we wouldn't want to do that while things are @experimental.

[velo]

@trumpetinc — thanks for both retractions; our comments crossed in flight.

We landed in the same place on narrow() from different directions, and yours is the better framing: #3534 adds Builder.narrow(predicate, encoder) precisely so a non-predicated encoder — or a tweaked predicate on one that does declare itself — can go through encoders(...) without dropping down to MultiEncoder.builder(). And describe() stays reflective for the reason you gave; without it a third-party encoder with no toString renders as feign.gson.GsonEncoder@1f2a3b4 in the failure list, which is exactly the troubleshooting experience we are trying to buy.

That leaves the header question as the only genuinely open item from your review. #3534 reports Content-Type and Accept rather than the whole map, because EncodeException messages reach logs and Authorization lives in that map; the rest of the detail rides on the predicate descriptions, which are printed next to the encoder they guard. If you think that trade is wrong, argue me out of it over there.

Also: I have sent you a repo invite (triage), so you can be a formal reviewer instead of an @-mention — you are already requested on #3534, which takes effect once you accept.

[trumpetinc]

Good point about not wanting the Authorization header to land in the logs... (I really wish the original Feign implementation had used a dedicated class to specify headers instead of passing a Map around - it would have made sanitizing the headers super simple)...

Quick discussion about reporting the headers:

I think it's also worth pointing out that because of where Encoder operates in the lifecycle, most uses will not have Authorization header in the template (most users will add a request listener to insert that auth header). But there are some cases where an explicit @Header might have been added, and we do need to account for that.

My counter-argument for including all headers (possibly with masking or eliminating the auth header) is that for complex use cases, users may be using custom headers to drive encoding behavior. Not having that information could make debugging more challenging.

My counter-counter argument is that if someone is doing that level of complexity, they an set breakpoints and debug it.

So at this point, I think the current approach is going to cover 99.9% of scenarios. And if more complex error reporting becomes a need, it can be handled in a future non-breaking PR. The fact that adding more headers later is non-breaking indicates that going with "least risky" is the correct course of action.

So I support your decision to leave that as-is.