feat!: allow registering multiple encoders in Feign

[yvasyliev]

Co-authored-by: trumpetinc 6618744+trumpetinc@users.noreply.github.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> 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.

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.

• Encoder.of(Encoder...) / Encoder.of(List<Encoder>) — factory method that creates a DelegatingEncoder, which tries each delegate's encode() in order and uses the first one that returns true. Throws EncodeException if no encoder succeeds.

• DelegatingEncoder — a composite encoder that delegates to a list of encoders, using the first one whose encode() returns true.

• Util.isJsonContentType(RequestTemplate) / Util.isXmlContentType(RequestTemplate) — utility methods that check the Content-Type header against JSON (application/json, application/ld+json, etc.) and XML (text/xml, application/xml, etc.) patterns.

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	Content-Type check via Util.isJsonContentType(template)
All XML encoders	Content-Type check via Util.isXmlContentType(template)

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(Encoder.of(
        new FormEncoder(),          // handles multipart/form-urlencoded by Content-Type
        new JacksonEncoder(),       // auto-detects JSON via Content-Type (implements JsonEncoder)
        new JAXBEncoder(factory)    // auto-detects XML via Content-Type
    ))
    .target(MyApi.class, "https://api.example.com");

Breaking Changes

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

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 does not handle the request.
2. Single encoder usage is unchanged — Feign.builder().encoder(new JacksonEncoder()) still works.

Files Changed

• New: api/src/main/java/feign/codec/DelegatingEncoder.java
• Modified: Encoder.java, Util.java, Feign.java, AsyncFeign.java, HystrixFeign.java, VertxFeign.java, DefaultEncoder.java, FormEncoder.java, GraphqlEncoder.java, MeteredEncoder.java (×3), JAXBEncoder.java (×2), SOAPEncoder.java (×2), JacksonEncoder.java, Jackson3Encoder.java, GsonEncoder.java, MoshiEncoder.java, Fastjson2Encoder.java, JacksonJrEncoder.java, JacksonJaxbJsonEncoder.java
• Tests: DelegatingEncoderTest.java, all JSON/XML encoder test files, and various integration tests
• Documentation: MIGRATION-v14.md, CHANGELOG.md

[velo]

I think this could be accomplished by a single encoder that accepts multiple encoders.

The blast radius of the chance would also be much smaller

[yvasyliev]

@velo I agree. I'm just wondering what would be better from user perspective:

var client = Feign.builder()
    .encoder(Encoder.of(new Encoder1(), new Encoder2()))
    .target(Client.class, "https://example.com");

// or

var client = Feign.builder()
    .encoders(new Encoder1(), new Encoder2())
    .target(Client.class, "https://example.com");

[yvasyliev]

The build fails because https://nghttp2.org/httpbin/* endpoints are down (affects feign.http2client.test.Http2ClientTest). A follow-up issue is opened - #3483

[trumpetinc]

@yvasyliev can you please take a look at the changes required to have a separate canEncode() method vs changing the return type to boolean? I think you will find that the Encoder implementations are going to wind up with a bunch of duplicate code...

Take DefaultEncoder as an example.

If we change the return type of encode(), then the implementation is this:

public class DefaultEncoder implements Encoder {

  @Override
  public boolean encode(Object object, Type bodyType, RequestTemplate template) {
    if (bodyType == String.class) {
      template.body(Request.Body.of(object.toString()));
    } else if (bodyType == byte[].class) {
      template.body(Request.Body.of((byte[]) object));
    } else if (object instanceof File) {
      template.body(new Request.PathBody(((File) object).toPath()));
    } else if (object instanceof Path) {
      template.body(new Request.PathBody((Path) object));
    } else if (object instanceof InputStream) {
      template.body(new Request.InputStreamBody((InputStream) object));
    } else if (object instanceof Request.Body) {
      template.body((Request.Body) object);
    } else {
       return false;
    }
  }
}

If we have to have a separate method, then we wind up with this:

public class DefaultEncoder implements Encoder {

  @Override
  public void encode(Object object, Type bodyType, RequestTemplate template) {
    if (bodyType == String.class) {
      template.body(Request.Body.of(object.toString()));
    } else if (bodyType == byte[].class) {
      template.body(Request.Body.of((byte[]) object));
    } else if (object instanceof File) {
      template.body(new Request.PathBody(((File) object).toPath()));
    } else if (object instanceof Path) {
      template.body(new Request.PathBody((Path) object));
    } else if (object instanceof InputStream) {
      template.body(new Request.InputStreamBody((InputStream) object));
    } else if (object instanceof Request.Body) {
      template.body((Request.Body) object);
    } 
  }

  @Override
  public boolean canEncode(Object object, Type bodyType, RequestTemplate template) {
    if (bodyType == String.class) {
      return true;
    } else if (bodyType == byte[].class) {
      return true;
    } else if (object instanceof File) {
      return true;
    } else if (object instanceof Path) {
      return true;
    } else if (object instanceof InputStream) {
      return true;
    } else if (object instanceof Request.Body) {
      return true;
    } 

    return false;
  }
}

That's a ton of duplication.

I understand the reason for the separate method (so you can use an interface with default method as a mix-in), but is it worth it? I just feel like adding the check to the top of the handful of effected encoders is simpler (and you are already having to modify those encoders to add the marker interface)...

If we want to have common "this is a JSON content-type" functionality, then let's put that in a utility class and add a check for it at the top of the various json encoders' boolean encode() method...

(or use the PredicateEncoder design - but I can see why we might prefer to have the json and xml content types automatically handled).

[trumpetinc]

I still think that the PredicateEncoder design is appropriate for scenarios where we want to tune encoders based on content-type headers, etc...

[trumpetinc]

@velo I agree. I'm just wondering what would be better from user perspective:

var client = Feign.builder()
    .encoder(Encoder.of(new Encoder1(), new Encoder2()))
    .target(Client.class, "https://example.com");

// or

var client = Feign.builder()
    .encoders(new Encoder1(), new Encoder2())
    .target(Client.class, "https://example.com");

I think that we could go with the top option for now if it makes it easier to get this change accepted.

But it would be very straightforward to make the current configbuilder.encoder() method be vararg as a syntactic sugar improvement. All it would do would be to check the length of the vararg, and if >1 it would implicitly do the Encoder.of(...) call. Internally, Feign will continue to only use a single encoder.

This would be source (but not binary) compatible - anyone currently passing a single argument would get the exact same experience.

[trumpetinc]

I think if we replace encoder(Encoder) with encoder(Encoder...) and encoder(List<Encoder>) that we can eliminate the encoders() method.

This will drastically reduce the impact on all the test classes, etc...

[yvasyliev]

If we change the return type of encode(), then the implementation is this:

I believe a valid implementation would look like this:

public class DefaultEncoder implements Encoder {

    @Override
    public boolean encode(Object object, Type bodyType, RequestTemplate template) {
        if (bodyType == String.class) {
            template.body(Request.Body.of(object.toString()));
            return true;
        } else if (bodyType == byte[].class) {
            template.body(Request.Body.of((byte[]) object));
            return true;
        } else if (object instanceof File) {
            template.body(new Request.PathBody(((File) object).toPath()));
            return true;
        } else if (object instanceof Path) {
            template.body(new Request.PathBody((Path) object));
            return true;
        } else if (object instanceof InputStream) {
            template.body(new Request.InputStreamBody((InputStream) object));
            return true;
        } else if (object instanceof Request.Body) {
            template.body((Request.Body) object);
            return true;
        } else {
            return false;
        }
    }
}

That's a ton of duplication.

Is it relevant for DefaultEncoder only? Ideally, a clean encoder would only need to support a single body type or
Content-Type...

If we want to have common "this is a JSON content-type" functionality, then let's put that in a utility class and add
a check for it at the top of the various json encoders' boolean encode() method...

Won't we end up with duplicated if (Util.isJsonContent(template)) { checks across multiple encoders?

---

I decided to give the abstract Encoder#canEncode method a shot for the following reasons:

1. Doesn't mess with return statements in the Encode#encode method and keep existing encoding logic the same (swapping
   throws new EncodeException for return false - is a business logic change with potentially unexpected
   repercussions)
2. Having the Encoder#encode method return a boolean will lead to modifying the tests anyway (we have to add
   return statements to all lambdas across the code)
3. If we compare the existing HTTP libraries:
   • Retrofit's Converter.Factory returns null for unsupported types. It would make perfect sense for Feign to
     treat null as an "I don't support it" reply if Feign's Encoder#encode method had a return type in the first
     place. Since Feign's Encoder is originally designed to look like java.util.function.Consumer, trying to add a
     return type now is like fitting a square peg in a round hole. I would consider making Encoder#encode method
     return Request.Body instead of boolean then.
   • On the other hand, Spring's HttpMessageConverter interface has both write and canWrite abstract methods. I
     tend to think that Feign's existing Encoder is @Functional by accidental design gap rather than weighted
     choice (it doesn't even have the @Functional annotation!).

---

My bet is that the HttpMessageConverter approach fits better for Feign now. Please challenge my arguments! 😊

[trumpetinc]

This is splitting hairs, but here is how I would implement that logic to reduce lines of code:

public class DefaultEncoder implements Encoder {

  @Override
  public boolean encode(Object object, Type bodyType, RequestTemplate template) {
    if (bodyType == String.class) {
      template.body(Request.Body.of(object.toString()));
    } else if (bodyType == byte[].class) {
      template.body(Request.Body.of((byte[]) object));
    } else if (object instanceof File) {
      template.body(new Request.PathBody(((File) object).toPath()));
    } else if (object instanceof Path) {
      template.body(new Request.PathBody((Path) object));
    } else if (object instanceof InputStream) {
      template.body(new Request.InputStreamBody((InputStream) object));
    } else if (object instanceof Request.Body) {
      template.body((Request.Body) object);
    } else {
       return false;
    }

    return true;
  }
}

[trumpetinc]

Is it relevant for DefaultEncoder only?

I think that we are talking doubling the lines of code (and implementing the same logic in two methods instead of one). So no, I think this is relevant across the board. Spreading logic across two methods dramatically increases the chances of mistakes (and these are the types of nasty mistakes that will manifest as weird corner cases). Plus this will make migrating existing code that much more complicated.

Fundamentally, though, there is no significant upside to doing this.

The default method/mix-in interface approach is clever - but it isn't doing very much. Instead, we could break the content type check out into a static isContentTypeJson() utility method, then call that utility method at the top of the 4 JSON encoders. This is just as many lines of code as adding the JsonEncoder interface to each of the 4 JSON encoders.

Like this:

  public static class ContentTypeUtils{
    public static boolean isRequestJson(RequestTemplate t){ ... }
    public static boolean isRequestXml(RequestTemplate t){ ... }
  }

And Jackson encoding would look like this:

public class JacksonEncoder implements Encoder {
  // set up objectMapper

  @Override
  public boolean encode(Object object, Type bodyType, RequestTemplate template) {
    if(!ContentTypeUtils.isRequestJson(template)) return false;

    try {
      JavaType javaType = mapper.getTypeFactory().constructType(bodyType);
      template.body(Request.Body.of(mapper.writerFor(javaType).writeValueAsBytes(object)));
    } catch (JsonProcessingException e) {
      throw new EncodeException(e.getMessage(), e); // note that we actually do want to throw here b/c something bad happened
    }

    return true;
  }
}

So for the json and xml cases, we are talking about whether we want to add 'JsonEncoder' to the implements line, or whether we just add one line to the top of the encode() method. I just don't think the clutter is worth it in all of the other encoder types. It just seems like it is better for our json and xml encoders to have one extra line of code than force all the other encoders to double their complexity.

Unit testing the utility class is more straightforward, etc...

---

Won't we end up with duplicated if (Util.isJsonContent(template)) { checks across multiple encoders?

Yep - just like we have duplicated 'implements JsonEncoder' in multiple encoders. I feel that line of code is trivial to add where it is needed. I just don't think the default method is helping us.

I do acknowledge that that if there a ton of existing encoders that already implement JsonEncoder, then having the extra method would theoretically make these encoders so they work as-is without making changes to them.

So it's a tradeoff:

Do we:

1. Make it so existing implementations of JsonEncoder don't have to be migrated, or
2. Make it so all Encoders are a lot easier to write and maintain

I am encouraging that we pick #2 because it improves the experience for ALL encoders.

Quasi-related: Are we clear on why JsonEncoder even exists? I can't figure out any real-world reason for that interface to even be there.

[trumpetinc]

Having the Encoder#encode method return a boolean will lead to modifying the tests anyway (we have to add
return statements to all lambdas across the code)

Since Feign's Encoder is originally designed to look like java.util.function.Consumer, trying to add a
return type now is like fitting a square peg in a round hole. I would consider making Encoder#encode method
return Request.Body instead of boolean then.

This is a good point that I hadn't considered. You've worked the test code base much more than me - how bad is it? Would it make the unit tests an absolute nightmare to overhaul? If so, then that is an additional item that needs to be accounted for in the tradeoff analysis.

PS - the reason you can't return Request.Body is that the encode() method is expected to be able to make changes to the RequestTemplate (adding headers, etc...). Encoding isn't just about creating a body.

So returning true/false is what makes sense to me. It indicates whether the method actually modified the RequestTemplate it was passed.

[trumpetinc]

Oh - and at the end of the day, the decision on this doesn't impact my excitement about the direction.

I'm just providing architectural feedback based on a long, long time of designing and working with these sorts of systems. Any time related business logic is implemented in multiple places, it increases the risk of really nasty corner case bugs. I have lost many, many weeks in my career diagnosing these sorts of bugs, so I am very resistant :-)

[yvasyliev]

You raise a fair point about duplication — I want to explore whether it's actually the architecture or just
DefaultEncoder that's the problem here.

I'm not convinced that if (!ContentTypeUtils.isRequestJson(template)) { at the top of encode() is cleaner, more
error-proof, or safer than adding implements JsonEncoder to the class declaration. While the industry standard is
to keep methods simple, the alternative approach inlines extra dispatch logic into the encode method itself.

DefaultEncoder is a bit of an outlier as an encoder example. It has 7 branches, which can only be justified by
the fact that each branch is a single-line encoding operation. A clean "default encoder" would look like
this:

public class StringEncoder implements Encoder {
    @Override
    public boolean canEncode(Object object, Type bodyType, RequestTemplate template) {
        return bodyType == String.class;
    }

    @Override
    public void encode(Object object, Type bodyType, RequestTemplate template) {
        template.body(Request.Body.of(object.toString()));
    }
}

public class ByteArrayEncoder implements Encoder {
    @Override
    public boolean canEncode(Object object, Type bodyType, RequestTemplate template) {
        return bodyType == byte[].class;
    }

    @Override
    public void encode(Object object, Type bodyType, RequestTemplate template) {
        template.body(Request.Body.of((byte[]) object));
    }
}

public class FileEncoder implements Encoder {
    @Override
    public boolean canEncode(Object object, Type bodyType, RequestTemplate template) {
        return object instanceof File;
    }

    @Override
    public void encode(Object object, Type bodyType, RequestTemplate template) {
        template.body(new Request.PathBody(((File) object).toPath()));
    }
}

public class PathEncoder implements Encoder {
    @Override
    public boolean canEncode(Object object, Type bodyType, RequestTemplate template) {
        return object instanceof Path;
    }

    @Override
    public void encode(Object object, Type bodyType, RequestTemplate template) {
        template.body(new Request.PathBody((Path) object));
    }
}

public class InputStreamEncoder implements Encoder {
    @Override
    public boolean canEncode(Object object, Type bodyType, RequestTemplate template) {
        return object instanceof InputStream;
    }

    @Override
    public void encode(Object object, Type bodyType, RequestTemplate template) {
        template.body(new Request.InputStreamBody((InputStream) object));
    }
}

public class RequestBodyEncoder implements Encoder {
    @Override
    public boolean canEncode(Object object, Type bodyType, RequestTemplate template) {
        return object instanceof Request.Body;
    }

    @Override
    public void encode(Object object, Type bodyType, RequestTemplate template) {
        template.body((Request.Body) object);
    }
}

Do any of the encoders above have code duplication? Are any of them error-prone? I'd argue that we shouldn't
bend the architecture just to make DefaultEncoder's 7 if-else statements look good. Instead, shouldn't we
encourage users to write clean, single-purpose encoders like these?

[trumpetinc]

I really appreciate this discussion. I often work on instinct, and the back and forth is encouraging me to really think hard about why I am uncomfortable with the canEncode() approach. I will try to lay my concrete reasons here in several sections.

---

Semantic change to encode() - not just a method signature change

The fundamental issue is that we are not just changing the API signature - we are literally changing the semantics of the encode() method.

• Prior to this change, EncodeException could mean that the encoder doesn't handle the request OR that there was a failure during encoding that the Encoder should have been able to handle.
• After the change, EncodeException must only be thrown if there was a failure during encoding that the Encoder should have been able to handle.

Unfortunately, it is not possible to bridge a semantic change like this with a default method. The encoder's encode() logic must be changed so it does not throw an exception when it can't handle the request. So it is really important that we break existing code and force a rewrite of every encoder.

This may sound like a small thing, but it is actually quite big.

Right now, you are assuming that checking the content-type header is sufficient to determine whether a given existing JsonEncoder or XmlEncoder can handle the encode. That might be true - but it also might not be true - we really have no way of knowing without actually reviewing the code of every JsonEncoder. Maybe there is some Json handler that explicitly doesn't handle certain types of situations?

Also, Encoders can be written by 3rd parties, so we can't be the ones to check to make sure that the semantic change is fully realized by the default method. The default method approach could result in us calling encode when we shouldn't (and getting an EncodeException instead of being able to continue walking the encoder chain). We have now introduced a breaking change that is only detected at runtime.

This is my biggest concern.

---

Are Existing Encoders Really Impacted That Much By the Extra Method?

I agree that the "normal" encoder examples you provided aren't that big of a deal (it does increase the LoCs a bit, though). Here are some examples where having the separate method is awkward:

FormEncoder (I know the point of all of our work is to deprecate that, but it's a good example of why we need to be super careful)
SpringEncoder
PageableSpringEncoder

PredicateEncoder is also going to be more awkward - instead of just calling the delegate, now we have to check both our predicate and whether the delegate can encode.

The new DelegatingEncoder is also awkward (though not as bad as the ones above)

---

Interface Default Method Antipattern

Using default methods in this way feels like a code smell. I admit that it is not 100% certain for this specific case, but documented anti-patterns exist for a reason, and we should have very, very good reasons for using an anti-pattern. I'm not closed minded on using antipatterns where it makes sense - but I'm also not yet convinced that I've seen big enough reasons to use an antipattern in this case.

On the other side of the argument, we could say that the default method is there to bridge an API as it has evolved so we get reasonable default behavior for all of the existing implementations. And I would be ok with that if we were adding the default method to the Encoder interface (not possible, of course, b/c this is a semantic change, not something that a default method can handle).

But even if there weren't semantic differences, it is awkward that we are evolving the Encoder interface, but adding the default method to the JsonEncoder sub-interface. I get that JsonEncoder extends Encoder - but it just feels wrong. Especially when there is an easy way to do this that isn't opaque.

My primary objection on this is that using interface default methods in this way feels very opaque. It is not at all obvious looking at the code that there is special handling going on for the content-type. If we were using abstract base classes, then I wouldn't object - but interfaces (and especially what has traditionally been a marker interface - no behavior) really feel non-obvious.

---

Conclusion

I think that recognizing there there is a significant semantic change is a very good reason to not use a default method to try to bridge this change. We must force compile time failures to ensure that each encoder migrates to the new semantics.

If that is the case, then I think the discussion becomes purely about whether having two methods is better than one method, and I definitely think that one method is better - but I could be convinced otherwise if the amount of code required to add a return type would be excessive (I think you said that there are a lot of lambdas in the unit tests??). If that does turn out to be the case, I would argue that introducing another exception type would still be better than a second method (but I'm not super crazy about that either - exceptions for standard control flow are ugly).

Please let me know if it would be helpful for me to dive deeper into the semantic changes being made. This is subtle - but super important.

[yvasyliev]

@trumpetinc Encode#canEncode method is removed and Encode#encode method now returns boolean. Could you please check and let me know if you have any doubts? Thank you!

[trumpetinc]

One thing that I thought of while doing the code review:

Users may be relying on the various json and xml encoders "just working" without having to set the content-type header... Or maybe they are intentionally setting a different content-type header for their specific endpoint?

It is unfortunate that the encoders are created using constructors (if they were created using static factory methods we could use composition and wrap with a PredicateEncoder).

I'm not sure what the real world risk on this is - but it could impact regular users (not just custom Encoder creators), so I think worth a quick discussion.

It's been a long time since I've seen a REST endpoint that didn't work without the content-type set - so it seems unlikely that this would impact many existing users (if any).

If we wanted to eliminate this, we could NOT do the content type check in the various json and xml encoders, but rather wrap them in a PredicateEncoder when needed. That would ensure complete backwards compatibility at the cost of more difficult DelegateExtractor setup.

[trumpetinc]

This is an excellent PR in my opinion. My only feedback were small things on javadocs - very well done.

[trumpetinc]

Suggested wording:

@return {@code true} if the encoder is able to handle the encode request, {@code false} if the encoder cannot encode the request
@throws EncodeException if this encoder should be able to encode the request but encoding fails

[trumpetinc]

Should we refer to the original form encoding as "legacy form encoding" maybe?

I'm a little concerned that it could be confusing to users if we don't distinguish between the two form encoding options.

[trumpetinc]

Consider renaming to encodeOrThrow

[trumpetinc]

spelling - change 'util' to 'until' (add an 'n'); change 'OpenFiegn' to 'OpenFeign' (reverse i and e)

[trumpetinc]

I think we probably need to have something along these lines:

Built in JSON and XML encoders require Content-Type header to be set:
If you are using json or xml encoding without setting the content-type header (either via annotations or a Feign request listener), you must add an appropriate content-type header (application/json, text/xml, application/xml).

Note that Spring Feign sets the content-type header to application/json by default, so this note does not apply to Spring Feign users).

[trumpetinc]

PS - this is a big enough impact that we may want to have it be a separately numbered migration line (i.e. #15).

Most of the other changes here do not directly impact normal users - but this change does.

[trumpetinc]

Do we have to do anything with SpringEncoder or PageableSpringEncoder? Those are the only 2 Encoders that I'm seeing in the code base that aren't yet covered by this PR...