Multi encoder

[trumpetinc]

Continuing work started in: #3476

Co-authored-by: trumpetinc 6618744+trumpetinc@users.noreply.github.com and yvasyliev ye.vasyliev@gmail.com

Note

Target branch: 14.x

Important

Please squash and merge this PR if it's approved.

Important

Please add Co-authored-by: trumpetinc <6618744+trumpetinc@users.noreply.github.com> and yvasyliev <ye.vasyliev@gmail.com> footer to the commit message if this PR is approved.

Summary

This PR introduces multi-encoder support in Feign, allowing users to register multiple encoders that are automatically selected at request time. The Encoder.encode() method now returns boolean (true if the encoder handled the object, false otherwise), and a new Encoder.of() factory combines encoders into a DelegatingEncoder. EncoderException is now thrown only when there are true encoding exceptions.

Motivation

Previously, Feign only supported a single Encoder registered via Feign.builder().encoder(...). For APIs that mix different content types (JSON, XML, form data, etc.), users had to manually wrap encoders (e.g., new FormEncoder(new JacksonEncoder())). This was brittle and didn't scale well.

Changes

New API

• Encoder.encode() now returns boolean - return true when encoding succeeds, false if the encoder does not handle the request. This replaces the need for a separate canEncode() method. Note that this is a semantic change of the encode() method - previously, an EncodeException was thrown in two scenarios: the encoder could not handle the encoding request and when there was a failure in handling the encoding request. Now, a return type of 'false' is used to indicate that the encoder could not handle the encoding request. EncodeException throw is reserved for true failures to encode.

• MultiEncoder - a composite encoder that delegates to a list of encoders, using the first one whose encode() returns true. If none of the delegates can handle the request, MultiEncoder.encode() returns false.

• PredicateEncoder - Conditionally delegates to a delegate encoder if the provided EnocerPredicate passes. Useful for conditionally processing a given encoder (for example, if the user wishes to limit JsonEncoder based on a content-type header value)

Encoder Updates

All built-in encoders now return boolean from encode() and gate on Content-Type where applicable:

Encoder	Strategy
DefaultEncoder	Type-based: accepts String, byte[], File, Path, InputStream, Request.Body, and null
FormEncoder	Content-Type-based: accepts multipart/form-data and application/x-www-form-urlencoded
GraphqlEncoder	Metadata-based: accepts requests matched by GraphqlContract; delegates to wrapped encoder otherwise
MeteredEncoder (all variants)	Delegates to the wrapped encoder
All JSON encoders	Any encode request
All XML encoders	Any encode request

Example Usage

Before:

// Only one encoder — had to manually wrap
Feign.builder()
    .encoder(new FormEncoder(new JacksonEncoder()))
    .target(MyApi.class, "https://api.example.com");

After:

Feign.builder()
    .encoder(MultiEncoder.of(
        new FormEncoder(),          // handles multipart/form-urlencoded by Content-Type
        new JacksonEncoder()       // handles JSON encoding
    ))
    .target(MyApi.class, "https://api.example.com");

Breaking Changes

Breaking changes are documented in MIGRATION-v14.md (sections 14–17):

1. Encoder.encode() now returns boolean instead of void. All custom Encoder implementations must update the return type and return true on success or false when the encoder can not handle the request (instead of throwing EncodeException). EncodeException should still be thrown for encoding failures.
2. Single encoder usage is unchanged — Feign.builder().encoder(new JacksonEncoder()) still works.

[trumpetinc]

@yvasyliev I have finished the refactor. I hope the surgery wasn't too bad. Biggest change was renaming DelegateEncoder to MultiEncoder and moving that to the core module.

Please provide feedback when you get a chance!

Also, if you are ok with these changes, it would probably be a good idea to withdraw #3476 so the maintainers don't get confused.

As a side note, I'm not completely sure where the core/api split is headed, but I would normally expect api to have only interfaces and exceptions (not all the concrete classes they have in the feign package). If we do wind up wanting to adjust BaseBuilder.encode() to support multiple encoders (instead of the user having to call builder.encoder(MultiEncoder.of(...)), this could be an issue (but I would make the argument that BaseBuilder, etc...) shouldn't be in the api module. This is a problem for another day.

[velo]

I maintain my comment on #3476 (comment)

Regarding the format this should take, I think the MultiEncoder needs a predicate and encore

Feign.builder()
  .encoder(
     MultiEncoder.add(request -> request.contentType() == json, new JacksonEncoder())
                          .add(request -> request.lenght() == 0, new EmptyEncoder())
                          .add(request -> request.type() instanceof Binary, new BinaryEncoder())
...
  )
  ...

[velo]

I don't think we can mix static and non-static add methods... if that is the case,

builder() + build()

or

MultiEncoder.default(new StringEncoder).add(...).add(...).add(...)

with each add returning a new immutable instance

[velo]

that could target main, and it would be amazing to have decoder following the same principle

[yvasyliev]

Thanks @trumpetinc for driving this change! I only have one question - is it required to bump maven dependencies across multiple modules in the scope of this PR?

All my review comments are really minor, so feel free to ignore them. I still think it would be cleaner for Encoder to have separate canEncode and encode method because that's how I prefer to structure code, but I don't have any architectural reasoning to push for it. It would be great to get this PR merged as-is, though!

[yvasyliev]

Why not leverage the full strength of MockitoExtension? 😊

Besides, it's better to have each test method self-contained, meaning that object, bodyType & requestTemplate must be local variables. You never know when we will need to add a test case with non-default method arguments (e.g., nulls)

@ExtendWith(MockitoExtension.class)
class PredicateEncoderTest {

  @Mock private Encoder delegate;
  @Mock private EncoderPredicate encoderPredicate
  @InjectMocks private PredicateEncoder predicateEncoder;

  @Test
  void shouldEncode() {
    var object = "Hello, World!";
    var bodyType = String.class;
    var requestTemplate = mock(RequestTemplate.class);

    when(encoderPredicate.test(object, bodyType, requestTemplate)).thenReturn(true);
    when(delegate.encode(object, bodyType, requestTemplate)).thenReturn(true);

    assertThat(predicateEncoder.encode(object, bodyType, requestTemplate)).isTrue();

    verify(delegate).encode(object, bodyType, requestTemplate);
  }

  @Test
  void shouldNotEncode() {
    var object = "Hello, World!";
    var bodyType = String.class;
    var requestTemplate = mock(RequestTemplate.class);

    assertThat(predicateEncoder.encode(object, bodyType, requestTemplate)).isFalse();

    verifyNoInteractions(delegate);
  }
}

[trumpetinc]

That works for me - I also added a when()....thenReturn(false) to the encoderPredicate to make it clear that we are explicitly returning false instead of relying on default mock behavior).

I definitely learned something from your example - thank you.

[yvasyliev]

that could target main, and it would be amazing to have decoder following the same principle

It's a breaking change @velo

[velo]

It's a breaking change @velo

it doesn't have to be... why must be a breaking change?

[trumpetinc]

I maintain my comment on #3476 (comment)

Regarding the format this should take, I think the MultiEncoder needs a predicate and encore

Feign.builder()
  .encoder(
     MultiEncoder.add(request -> request.contentType() == json, new JacksonEncoder())
                          .add(request -> request.lenght() == 0, new EmptyEncoder())
                          .add(request -> request.type() instanceof Binary, new BinaryEncoder())
...
  )
  ...

@velo can you help me understand the advantages of this over having a PredicateEncoder that wraps other encoders? I feel like PredicateEncoder (included in this PR) is a better separation of concerns than making the multi-encoder itself hard-coded with predicate awareness.

I'm also pretty sure that predicates will need to handle 3 arguments (same as the encode() method). Request does not have type(), contentType() or length() members.

We could certainly have static accessors for common types of predicates to make it easier (for example, I think a PredicateEncoder.forJsonContentType(delegate) makes sense):

Feign.builder()
  .encoder(MultiEncoder.of( PredicateEncoder.forJsonContentType(new JacksoneEncoder()) ()

or, if we are ok with expanding the arguments of BaseBuilder.encoder to accept a vararg, then it becomes:

Feign.builder()
  .encoder(
          PredicateEncoder.forJsonContentType(new JacksoneEncoder()),
          PredicateEncoder.forEmptyBody(new EmptyEncoder()),
          new DefaultEncoder()
  )
  ...

[trumpetinc]

I don't think we can mix static and non-static add methods... if that is the case,

builder() + build()

or

MultiEncoder.default(new StringEncoder).add(...).add(...).add(...)

with each add returning a new immutable instance

@velo can you please expand on this feedback? I wasn't intending to have add methods at all...

I think it is pretty unlikely that a user will want to add encoders to an existing MultiEncoder (they can always wrap a MultiEncoder in another MultiEncoder if that becomes necessary). So a single static factory method with a vararg makes for really clean code.

Also, there is no concept of a 'default' encoder. The encoders are processed linearly until one is found that can handle the request.

In the unusual case where someone wants to compose an existing MultiEncoder with another encoder, it would look like this:

Encoder originalEncoder = MultiEncoder.of(new DefaultEncoder(), new EmptyBodyEncoder());

... at some point later ...

Encoder finalEncoder = MultiEncoder.of(originalEncoder, new JacksonEncoder());

I think that this approach allows us to work with the Encoder interface as much as possible (Liskov Substitution Principle).

[trumpetinc]

@velo one thing that I wanted to ask about: Can you give some history on the core->api module split?

There is a ton of code in the api module that was moved from the core module (and it is not really 'api-like'). The Feign builder interface, for example, is now in api. This means that if we want to do anything in the builder, we are forced to move more core classes into the api module.

Specific example:

I would really like to change the BaseBuilder.encoder() method signature from:

  .encoder(Encoder encoder)

to

  .encoder(Encoder... encoders)

The builder would wrap the varargs in a MultiEncoder behind the scenes.

But this isn't currently possible without moving MultiEncoder from core to api.

It just feels very weird to me to have an api module that still has a ton of core functionality code in it... So I feel like I must be missing something about the intent of api vs core.

Thx

[trumpetinc]

It's a breaking change @velo

it doesn't have to be... why must be a breaking change?

The semantics of the Encoder interface must change to support this. The current encode() method uses a thrown EncodeException for two scenarios: a) Encoder cannot handle the request; and b) An error happened during encoding that the Encoder should have been able to handle. We need to separate these two code flows, which means a semantic change.

A change in semantics of a public API, unfortunately, absolutely mandates a breaking change. In this case, I have decided to force implementers to respond to the change in semantics by changing the method signature of encode() (it now returns a boolean instead of void). This achieves two things:

1. we clearly split the semantic change (return false if encoding not supported, continue throwing EncodeException if there is an actual failure during encoding) - this is necessary to properly implement encoder chaining like we are in MultiExtractor.
2. We force interface implementers to rewrite - this forces them to actually read about the breaking change and adjust their code to the new semantics.

It seems like such a small thing, but semantic changes of public APIs are really tricky - so super important that they be very obviously breaking changes.

[trumpetinc]

that could target main, and it would be amazing to have decoder following the same principle

@velo I actually looked at Decoder yesterday. It will probably be less impactful than on the encoder side, but I would prefer to use the same syntax for configuring encoders and decoders, so I think it is worth consideration.

The question becomes: "How do we differentiate between the 'decoder does not handle this return type' case vs the 'decoding error was encountered' case".

Here are my initial thoughts (if this looks promising, we should probably break this out into a separate PR/issue):

decode() has a return type. We could have decode() return a sentinel value (Decoder.NOT_SUPPORTED) to indicate that the type isn't supported.

One issue I see here is that this will need to be a semantic change, but we don't have a clean way of forcing implementers to rewrite their code. This could result in really ugly runtime failures after a user migrates. I'm not sure what to do about that - normally this would be done by changing the method signature in a breaking way, but it's not obvious how to do that here.

Do you have any thoughts?

Once we have the semantic issue figured out, creating a MultiDecoder and PredicateDecoder will be simple.

[trumpetinc]

@yvasyliev @velo one question: I was trying to keep the scope of this PR small (as small as possible, anyway), so I did not include any of the methods/classes related to creating EncoderPredicate of various types. My intent is to add those in a future PR - is that OK, or would it be better to include those now?

Basically, I really, really want to get this PR accepted and merged so other dependent PRs (forms overhaul for example) can proceed.

[yvasyliev]

that could target main, and it would be amazing to have decoder following the same principle

@velo I actually looked at Decoder yesterday. It will probably be less impactful than on the encoder side, but I would prefer to use the same syntax for configuring encoders and decoders, so I think it is worth consideration.

The question becomes: "How do we differentiate between the 'decoder does not handle this return type' case vs the 'decoding error was encountered' case".

Here are my initial thoughts (if this looks promising, we should probably break this out into a separate PR/issue):

decode() has a return type. We could have decode() return a sentinel value (Decoder.NOT_SUPPORTED) to indicate that the type isn't supported.

😊 I apologize for moving conversations in circles. May I ask you to revisit the Encoder#canEncode approach at this stage?

There's a broken single responsibility principle in Feign's Encoder. By providing direct access to the RequestTemplate, the Encoder acts less like a body encoder and more like a request interceptor, despite having a dedicated RequestInterceptor interface in the library.

If it were a pure encoder, the encode method would have either a non-void return type (Request.Body encode(Object object, Type bodyType)) or direct access to the OutputStream (void encode(Object object, Type bodyType, OutputStream out)), effectively disabling the mutation of an underlying RequestTemplate. And it's okay to add another Map<String, List<String>> method parameter for reading request headers in both scenarios.

Just a side note

If we need to reduce the number of method parameters, we can pack everything into a container: Request.Body encode(RequestContext ctx)

To be explicit, I don't mind having a mutating Encoder, but I want to address the missing symmetry between the Encoder and the Decoder, which I hadn't considered previously, but now see as a strong argument for Encoder#canEncode.

The only sentinel value I'm aware of in Java is null. Any Util.NOT_SUPPORTED value would be Java-unfriendly and confusing to users. Even true/false return values are not that intuitive despite being adopted by some JDK APIs. This suggests that it would be nice if the encode method had a return type like Request.Body. If the Encoder#encode method had a return type, (a) a null value could indicate an unsupported type and (b) the logic could be the same for Decoder#decode.

The alternative is to have explicit canEncode and canDecode methods in the Encoder and Decoder interfaces, respectively. The benefit is obvious: we can keep all existing encode/decode implementations as-is while requiring users to define canEncode and canDecode explicitly - without the need to read Javadocs to understand why encode now needs to return a boolean or what a hypothetical Util.NOT_SUPPORTED even means. And I'm quite confident it's a good practice to separate business (encoding/decoding) logic from validation logic.

[velo]

> Feign.builder()
>   .encoder(
>           PredicateEncoder.forJsonContentType(new JacksoneEncoder()),
>           PredicateEncoder.forEmptyBody(new EmptyEncoder()),
>           new DefaultEncoder()
>   )
>   ...

This syntax looks nice.... but would need to be the other way around with DefaultEncoder first.

public Builder encoder(Encoder defaultEncoder, PredicatedEncoder...encoders) {
  return this.encoder(MultiEncoder.of( defaultEncoder, encoders );
}

major gain in terms of functionality, and don't need to break compatibility

love the recommended Predicates, just make sure PredicatedEncoder is a functionaly type so people can easily expand on this

I still see value on a easy syntax to provide PredicatedEncoder/Encoder pairs to build a MultiEncoder/Decoder

There is a lot happening, I can't keep up

[trumpetinc]

@velo

Do see an issue (backwards compatibility, etc...) with just changing the Builder.encoder(Encoder) method to be Builder.encoder(Encoder...) (just change the argument to vararg) ?

This change is source compatible with previous code.

This change is not compile time compatible, but we are already making breaking changes because of the semantic change, so breaking changes are absolutely mandatory - no way around that. So unless I am missing something, making a source compatible change should be fine to do.

I also do not want to assume that users will want to always add PredicateEncoder as their additional encoders - that class is very useful for some scenarios, but certainly not all.

[trumpetinc]

@yvasyliev I am fine with discussing this until we get it right. I have had a very long day today and tomorrow, so it will be a couple of days before I can respond. Apologies for the delay!

[trumpetinc]

@yvasyliev finally had a chance to read your response ( #3485 (comment) ) and give it some careful thought.

Attempting to find symmetry between Encoder and Decoder

For background, during one of my earlier attempts at adding streaming support to Feign, I actually did a lot of work attempting to unify the concepts of a request body and response body. The understanding I gained from that effort is informing my opinions on this (including the fact that trying to unify request and response bodies was a terrible idea!).

Here are my opinions:

1. I completely agree that Encoder is fundamentally a request interceptor that has a strict stage in the request processing lifecycle. In addition, there is an implied contract that only a single Encoder should operate on a given request flow (this was not part of the original Feign lifecycle b/c only one encoder was allowed).
2. I would argue that the response decoding phase has nothing in common with the encoding phase. It might be tempting to view them as mirror images, and that their designs should reflect that. But when you really get into the specifics, you will find that they have almost nothing in common except having names ending in 'coder'.

Decoder produces a Java object (return type of the service interface method), and that's pretty much it.

Encoder is a completely different animal. It is not just about taking a Java object and converting it into a stream of bytes that are sent to the server. Having the ability to transform the RequestTemplate is fundamental to the Feign encoding lifecycle. In fact, it is helpful to think about RequestTemplate as being a builder interface (mutable), and Encoder.encode() is intended to make changes to that builder.

Based on what I've seen in the architecture and code, the Feign designers almost certainly intended for the Encoder to be able to manipulate the request body and the request headers and other aspects of the RequestTemplate as needed. I probably wouldn't have designed it this way myself - but there are advantages to the current approach.

I am not completely opposed to attempting to change this (i.e. make the encoder so it focuses only on the bodies) - but I think this would be a massive, massive challenge with all sorts of unexpected fallout.

How to indicate that a Decoder did not work

(I'm sure you guys are sick of hearing me talk about semantic differences! But I'm really serious that needs to be taken into account when we make breaking changes... I've lost months and a ton of money when vendors made semantic changes that didn't force compile time errors, so I am coming at this from a place of a lot of painful experience).

I see six approaches (I realize I'm partially restating what yvasyliev wrote - but I want to summarize all options) for dealing with this semantic change:

1. introduce a DecodeResult return type - this would force Decoder implementers to change. However, it would NOT force errors on any code that calls decode(). This may not be an important consideration because all code that called Decoder.decode() is feign core code, and we can find instances and fix them. But I want to point out that the method signature from the callers perspective won't change. They are expecting an Object. They will get an Object. This will cause downstream failures when Feign attempts to case to the return type of the method - but that is a runtime error, not compile time.
2. Introduce a checked new DecodeNotSupportedException (important that this NOT derive from FeignException, though). This would cause compile time errors for callers.
3. Sentinel value (and it can't be null because there are scenarios where a decode operation produces a null)
4. Add a separate canDecode(Type, Response) method to the interface
5. Change return type to Optional
6. Change the decode() method name or argument list (and return a DecodeResult)

Each of the above has pros/cons:

• DecodeResult - Pro: Compile time errors for implementers. Con: runtime errors for callers.
• DecodeNotSupportedException - Pro: Compile time errors for implements and callers. Con: Exception for control flow - yuck.
• Sentinel value - Pro: None. Con: Runtime errors for implementers and callers
• canDecode(Type, Response) - Pro: Compile time errors for implementers. Con: Runtime errors for callers.
• Optional return type - Pro: Compile time errors for implementers. Con: Runtime errors for callers.
• Change method name/arguments - Pro: Compile time errors for implements and callers. Con: seems like it would be weird to rename such an obvious method name...

I think that narrowing this list down will require determining whether we care about "Runtime errors for callers". If this is something we have to avoid at all costs, then the exception and change method name may be the only ways to do this.

Personally, I think accepting the risk of runtime exceptions for callers is an acceptable compromise. These methods truly have no value except by Feign core (maybe in some unit test scenarios, but runtime errors are fine in a test situation).

A comment about separation of concerns

I think that a lot of your view on separation of concerns is being driven by the idea that Encoder's concern is to take a request body argument from the method call and convert it into a request Body object.

I do not view that as Encoder's concern. I view that Encoder's concern is "Manipulate a RequestTemplate to ensure that it is ready to be sent to the server". In that context, the boolean return type is not a matter of "can the encoder handle the request?", it is "did the encoder actually handle the request?"

This is a subtle thing, but is why I believe that canEncode() isn't appropriate.

Fundamentally, in a chain of responsibility design, the goal is to keep trying until you have success - then stop.

I could be convinced of the need for a canEncode method - but only if there were use cases where that method would be called by itself. In the context of the Feign request processing cycle, every single call to canEncode() would be paired with a subsequent call to encode(). This is a clear sign that the purpose of the returned boolean is not to provide information about whether an Encoder can encode (along with weird side effects of actually doing the encoding!). The purpose of the returned boolean is to inform the caller that the RequestTemplate was actually updated - thus telling the chain of responsibility that it's work is done.

I hope that makes sense. And for sure, if you have an example of where Encoder.canEncode() would be called standalone for a reason that is truly answering the question ("Can you encode this request?"), I am very happy to consider it. I just don't see it in a chain of responsibility design like we have here.

Maybe have an EncodeResult?

If we wanted to preserve symmetry between Encoder.encode() and Decoder.decode(), we could go with having them return an EncodeResult / DecodeResult. The EncodeResult would pretty much just have an encodingWasPerformed boolean - so not much advantage - but it would allow for some cosmetic symmetry between the calls.

Side Note: EncodeResult can not include a summary of the changes made to the RequestTemplate, unless we are going to try to make the RequestTemplate passed to the encode() method immutable, then introduce a RequestTemplateMutator that the encode() method returns and is then applied to the RequestTemplate. This just feels like a huge undertaking for pretty minimal gains.

Would a simple EncodeResult like I'm describing help with "it's not symmetrical" feeling a bit? One thing that I like this approach is that the method names would make really clear what was intended. Returning 'true' or 'false' is not nearly as informative as returning EncodeResult.encodingSuccessful() vs EncodeResult.encodingNotSupported().

[velo]

1. we clearly split the semantic change (return false if encoding not supported, continue throwing EncodeException if there is an actual failure during encoding) - this is necessary to properly implement encoder chaining like we are in MultiExtractor.

we can control support in the predicate, so encoder stays as is.

[trumpetinc]

The current Encoder.encode() method uses a thrown EncodeException to indicate TWO outcomes: a) Encoder does not support encoding of the request; and b) an error happened during encoding.

This means that callers have no way of knowing whether an exception thrown by encode() means failure, or just that the Encoder doesn't support the operation.

It is not possible to construct a MultiEncoder with that ambiguity. If you feel there is, we will need to see some code showing how it would work.

I think that we need to get clear on this before we have any discussion about adding this to a BaseBuilder.encode() method.