feat: add @Part annotation for multipart form-data request contracts

[yvasyliev]

Co-authored-by: trumpetinc 6618744+trumpetinc@users.noreply.github.com

Summary

Another PR in the series related to #2734. Delivers the full multipart request API for Feign.

This PR introduces first-class, declarative support for multipart/form-data requests in Feign through a new @Part parameter annotation, backed by a dedicated MultipartFormEncoder that serializes the parsed parts into a streaming-capable multipart/form-data body.

This change is fully additive and non-breaking. Existing users and contracts will experience zero impact, as it introduces entirely new resolution pathways without altering standard form-encoded or single-body behaviors.

---

Usage Example

public interface FileUploadClient {
    @RequestLine("POST /api/v1/upload")
    // Content-Type: multipart/form-data can be omitted
    void uploadForm(
        // Shorthand notation; sufficient for 90% scenarios;
        // filename & Content-Type to be discovered & appended by a dedicated Encoder
        @Part("file")
        Path file,

        // Explicit single header
        @Part("Content-Disposition: form-data; name=\"metadata\"")
        byte[] metadata,

        // Multi-header declaration with param expansion
        @Part({
          "Content-Disposition: form-data; name=\"kv\"",
          "Content-Type: {customPartContentType}"
        })
        String customPart,

        @Param("customPartContentType")
        String customPartContentType,

        // Exploded collection part
        @Part(value = "tags", explode = true)
        List<String> tags
    );
}

---

API Design & Naming Rationale

During background design discussions, the API contract and naming conventions were carefully weighed to ensure maximum flexibility and architectural consistency with Feign's existing codebase:

1. The @Part Annotation

• Why @Part instead of @Multipart or @FormData? The term multipart is fundamentally coupled to the nature of the entire HTTP request rather than an individual parameter. Conversely, form data is specific to a single header type (Content-Disposition). We needed a name that describes the entire standalone part entity (comprising its own distinct headers, metadata, and body payload). Hence, @Part is the most precise candidate.
• Property Flexibility (value & headers):
  The value attribute acts as a concise alias for headers. This elegant symmetry allows developers to seamlessly scale from a simple shorthand string (@Part("partName")) to a full raw header line or a multi-line array of distinct headers.
• The explode Flag:
  This property mirrors Feign's existing feign.CollectionFormat#EXPLODED configuration paradigm. It defines whether a collection or array should be unrolled into repeated separate parts on the wire.
• Rejection of explicit name / filename attributes:
  An alternative design considered introducing dedicated name and filename properties directly onto the annotation. This was rejected because it introduces unneeded implementation complexity while arbitrarily limiting the user from injecting custom headers into individual parts.

2. MultipartFormData & PartData

• MultipartFormData: Named to maintain a clear conceptual bridge to the legacy Map<String, Object> formData instances heavily utilized throughout Feign's core engine.
• PartData: Extends the semantic pattern established by MultipartFormData, providing an isolated, intuitive representation of a specific runtime part payload.

---

Key Changes

1. Declarative API & Metadata

• @Part Annotation: Defines the annotation interface with value, headers, and explode behaviors.
• PartMetadata & PartData: Distinguishes compile-time contract definition metadata (PartMetadata) from runtime invocation state (PartData).
• MultipartFormData: Holds the aggregated collection of runtime parts alongside standard invocation variables, ready to be passed downstream to the encoding layer.

2. Contract Integration & Validation

• DefaultContract Parsing: Inspects parameters for @Part. If a single shorthand value lacking a colon (:) is provided, it automatically expands it into a standard Content-Disposition: form-data; name="..." structure.
• Validation Defenses: Explicitly blocks invalid contract combinations (e.g., throwing an IllegalStateException if a contract attempts to mix an unannotated body parameter with @Part parameters, or if a user provides both value and headers attributes simultaneously).

3. Execution Factory Pathway

• RequestTemplateFactoryResolver: Routes routing logic through a new BuildMultipartTemplateFromArgs factory whenever @Part structures are registered on a method contract.

4. Encoder Implementation

• MultipartFormEncoder: A new Encoder that consumes MultipartFormData or MultipartFormBody objects and serializes them into a standards-compliant multipart/form-data request body with a generated boundary. Delegates to a configurable chain of Encoder instances for individual part body encoding.
• MultipartFormBody: A streaming Request.Body implementation that writes a complete multipart message (boundary-delimited headers + body for each part, followed by the closing boundary).
• PartBody: A Request.Body representing a single multipart part — headers and an optional body payload.
• PartBodyFactory: Iterates a configurable chain of Encoder instances to encode each PartData into a PartBody, collecting suppressed exceptions for diagnostics.
• ContentDisposition: Parses and generates Content-Disposition headers with proper filename and filename* (RFC 5987) encoding.
• Rfc5987Util: Implements RFC 5987 parameter value encoding for non-ASCII filenames in Content-Disposition headers.

5. Spring Integration

• MultipartFileEncoder: Extends MultipartFormEncoder to directly accept Spring's MultipartFile objects, automatically generating part headers (including Content-Disposition with filename) and streaming the file content through MultipartFileBody.
• PartHeadersFactory: Creates multipart part headers from MultipartFile metadata.

---

Safety & Validation Rules Matrix

Scenario	Contract Engine Behavior
@Part("fieldName")	Translates to Content-Disposition: form-data; name="fieldName"
Mixed @Part and unannotated body param	Throws IllegalStateException (Body conflict)
Missing both value and headers on @Part	Throws IllegalStateException
Specifying both value and headers on @Part	Throws IllegalStateException

---

Test Coverage

• DefaultContractMultipartTest: Thoroughly asserts metadata generation correctness, shorthand header translation, error-state rejection, explicit generic type resolution, and multiple part handling.
• BuildMultipartTemplateFromArgsTest: Confirms runtime template resolution factory behavior, ensuring variables map reliably and standard runtime execution faults transfer elegantly into Feign EncodeException types.
• StreamingMultipartFormTest: End-to-end integration test covering the full pipeline — from @Part-annotated interface through MultipartFormEncoder — including file uploads, byte arrays, strings, exploded collections, custom headers, non-ASCII filenames (RFC 5987), and empty/null part values.
• ContentDispositionTest: Verifies Content-Disposition header parsing and generation, including filename encoding edge cases.
• Rfc5987UtilTest: Validates RFC 5987-compliant percent-encoding of parameter values.
• MultipartFileEncoderTest: Confirms Spring MultipartFile encoding via MultipartFileEncoder, including single files, collections, and fallback to the delegate encoder for non-MultipartFile types.

[trumpetinc]

@yvasyliev nice!

I would like to encourage that we have a clear model of the filename handling - i.e. we get the Encoder part of the puzzle also completely figured out as part of this PR.

Specifically, I'm still not crystal clear on how the Encoder will handle putting the filename in the content disposition header without having multipart specific File and Path encoders.

I had originally suggested using a bean expression syntax to address this - is that still on the table, or are you thinking of something different?

[trumpetinc]

@yvasyliev one big suggestion to consider:

I think the current PR may be pushing too much responsibility to the Encoder...

I think the place to handle sub-part encoding is in BuildMultipartTemplateFromArgs itself. Same with the unwrapping/unrolling/exploding collections.

Here's what that would look like (I haven't compiled this, so there may be some type-os):

  static class BuildMultipartTemplateFromArgs extends BuildTemplateByResolvingArgs {
    private final Encoder encoder;

    BuildMultipartTemplateFromArgs(
        MethodMetadata metadata,
        Encoder encoder,
        QueryMapEncoder queryMapEncoder,
        Target<?> target) {
      super(metadata, queryMapEncoder, target);
      this.encoder = encoder;
    }

// everything above this is the same as in your code

    @Override
    protected RequestTemplate resolve(
        Object[] argv, RequestTemplate mutable, Map<String, Object> variables) {
      List<PartData> parts =
          metadata.partMetadata().entrySet().stream()
              // not shown, but I think this is where you should call the expander...
              .map(
                  entry -> {
                    PartMetadata partMeta = entry.getValue();

                    // construct a separate RequestTemplate for the part, resolve variables, and encode the body
                    RequestTemplate partRequestTemplate = new RequestTemplate(); 
                    partRequestTemplate.headers(partMeta.headers());
                    partRequestTemplate = partRequestTemplate.resolve(variables);
                    encoder.encode( argv[entry.getKey()], partMeta.type(), partRequestTemplate); // note that we have access to the configured Feign encoder here

                   // now create a PartData from the info in the part's RequestTemplate
                    return new PartData(partRequestTemplate.getHeaders(), partRequestTemplate.getBody())
                  })
              .collect(Collectors.toList());

// everything below this is the same as your code

      MultipartFormData formData = new MultipartFormData(parts);

      try {
        encoder.encode(formData, MultipartFormData.class, mutable);
      } catch (EncodeException e) {
        throw e;
      } catch (RuntimeException e) {
        throw new EncodeException(e.getMessage(), e);
      }

      return super.resolve(argv, mutable, variables);
    }
  }

Basically, BuildMultipartTemplateFromArgs would produce the exact MultiPartFormData object that a user would create manually if they weren't using annotations. This keeps the architecture nice and encapsulated.

The multipart encoder then becomes very simple - it just adds the separator header to the request template, then it creates a Body that adds the separator and part headers, plus calls each part Body to emit its data to the output beneath each part header.

If we have good separation of concerns, the user should be able to register a MultiPartFormDataEncoder in Feign config then do either of these and get the same behavior:

// option 1
public interface FileUploadClient {
    @RequestLine("POST /api/v1/upload")
    void uploadForm(
        // Shorthand notation; sufficient for 90% scenarios;
        // filename & Content-Type to be discovered & appended by a dedicated Encoder
        @Part("file")
        Path file
   );

//

uploadClient.uploadForm(myPath);

or

// option 2
public interface FileUploadClient {
    @RequestLine("POST /api/v1/upload")
    void uploadForm(
        MultiPartFormData multiPartFormData
   );

//

uploadClient.uploadForm(
  MultiPartFormData.builder()
    .part(MultiPartForm
    .body(
        PartData.builder()
        .header("Content-Type: application/xml") // or whatever
        .header("Content-Disposition: form-part; name="file"; filename="aaaaa.xml")
        .body(new Request.PathBody(myPath)
        .build()
    )
    .build()
);

If the user can do either of those with the same result (and not having to jump through hoops in the back-end code to make it happen), then we will have really good encapsulation.

[yvasyliev]

So, BuildMultipartTemplateFromArgs delegates parts encoding to the Encoder, packs them into a MultipartFormData, and delegates the MultipartFormData to the same encoder again. Doesn't this flow violate the single responsibility principle? I always thought that the body transformation/expansion/explosion is the Encoder's jurisdiction.

Like the legacy Map<String, Object> formData, MultipartFormData is an intermediate state between raw body metadata and the transfer-ready Request.Body. In this POC, I don't expect MultipartFormData to be available for regular users. Instead, I expect the Encoder to map the MultipartFormData (and its parts) to a MultipartFormBody. And the MultipartFormBody is something we can expose to the regular users.

This way, your second option becomes:

// option 2
public interface FileUploadClient {
    @RequestLine("POST /api/v1/upload")
    void uploadForm(MultipartFormBody body);
}

//

uploadClient.uploadForm(new MultipartFormBody(
    "my-own-boundary",
    List.of(new PartBody(
        Map.of(
            "Content-Disposition", List.of("form-data; name=\"file\"; filename=\"aaaaa.xml\""),
            "Content-Type", List.of("application/xml")
        ),
        new Request.PathBody(myPath)
    ))
));

Does it work for you?

[trumpetinc]

ok - I see that I may have misinterpreted the MultiPartFormData class. I don't see a MultiPartFormBody class in the PR - maybe this is why I got confused?

As for concerns, here's how I look at it:

The Contract is concerned with reading the interface method signatures and building a meta object.

The RequestTemplateFactoryResolver is about combining the actual arguments of the method call (which can contain a Java object that represents the body) and the method meta to construct a resolved RequestTemplate. The construction of a RequestTemplate requires an Encoder to convert the Java object argument to a Body in the RequestTemplate (recognizing that this encoding may require some reading and/or setting of the request headers).

My strong opinion is that Encoders are concerned only with converting a Java object into the body+headers required for the request to be sent. When we make this distinction, very good things happen to the architecture (including your original very important observation that encoders should be usable for multi-part section encoding). When the encoders try to be responsible for more than that one concern, all sorts of dependencies and limitations show up.

With multi-part, the parallel between the sub-part and Request is very, very strong, and I think we can take advantage of that to really simplify the coding and make things more intuitive for users.

I am pretty confident about my description of the Encoder's (limited) concern because of the following:

1. If we try to put responsibility of creating the part bodies into the MultiPartFormEncoder, we immediately have the problem that MultiPartFormEncoder doesn't have access to the Feign configured Encoder. This is why the current multipart handler has effectively recreated the entire encoder functionality. I think this is also why your previous proof of concept had explicit encoder registration with the MultiPartFormEncoder itself. We could "solve" this by putting the Feign configured Encoder into the MultiPartFormData object so it will be available to the form encoder, but this is getting really messy. We were basically fighting against the natural responsibilities of the Feign architecture.
2. Collection expansion already happens in the RequestTemplateFactoryResolver. So the original Feign implementation already acknowledges where that behavior belongs.
3. Resolving header templates and bodies is already done in RequestTemplateFactoryResolver (it's actually the name of the class!). In the PoC PR, we are having to package all of the information from RequestTemplateFactoryResolver into MultiPartFormData just so MultiPartFormEncoder can do the work. It is much cleaner to just do that processing right there in RequestTemplateFactoryResolver.

One thing that is certain: Encoders are designed to work with RequestTemplate. So if we want to re-use Encoders (and I think that we really, really do), then it makes a lot of sense to use RequestTemplate and resolve it from within the RequestTemplateFactoryResolver.

Unfortunately, I don't think that the MultiPartFormData class as written is very helpful - you can see from my BuildMultipartTemplateFromArgs sample code I posted earlier that we have all the information we need to construct a fully defined PartData with all headers and the body. Capturing the variables, header templates, etc... in the MultiPartFormData then passing that to the MultiPartFormEncoder doesn't seem to add much. Much cleaner to just create a MultiPartFormBody (which is really just a collection of PartDatas, or maybe we should call them PartRequests ??).

Fundamentally, I see many reasons to do this request template resolution in RequestTemplateFactoryResolver, but I can't see much benefit to trying to do any of that in the encoder.

For thoroughness, here is what I think the MultiPartFormEncoder would look like - very simple, focused only on encoding a well defined MultiPartFormBody:

public   void encode(Object object, Type bodyType, RequestTemplate template) throws EncodeException{
  // not shown - make sure bodyType is MultiPartFormBody
  final MultiPartFormBody formBody = (MultiPartFormBody)object;
  final String boundary = // generate random - don't make the user provide it

  template.headers()....  // set the content-type header including boundary

  // our Body implementation becomes super straightforward
  template.body().writeTo(os -> {
    formBody.parts().forEach(part -> {
       emitBoundary(os, boundary);
       part.headers().entrySet().forEach(entry -> emitHeader(os, entry.key(), entry.value()));
       emitNewLine(os);
       part.body().writeTo(os);
    }

   // add closing boundary (I'm pretty sure that is required by the spec, but I haven't double checked)
   emitBoundary(boundary);

  };

}

private void emitBoundary(OutputStream os, String boundary) {...}
private void emitHeader(OutputStream os, String headerName, Collection<String> headerVals ) {...}
private void emitNewline(OutputStream os) {...}

Note that this encoder does not need access to any other encoders - because the part headers and body have already been resolved.

Does that make sense?

[yvasyliev]

@trumpetinc I have completed the POC with a MultipartFormEncoder using the initial design. Could you please take a look?

I assume that constructing RequestTemplate for each part in the BuildMultipartTemplateFromArgs directly implies having PartDataFlattener, PartDataTemplateFactory, ContentDisposition & Rfc5987Util in the feign-api module. I feel like it's quite a lot of responsibilities for an API module. What do you think?

[trumpetinc]

@yvasyliev apologies for the delayed response - was traveling.

I'm digging into your question about the PartDataFlattener, etc... utility classes being in core... This is a fair question and important for us to consider. The question is this: Are we willing to do something very ugly with the architecture to preserve the module separation for multi-part handling? Because trying to do everything in the encoding layer is going to do some very ugly things.

I think that once we have decided that multi-part has to be handled in core, then there is very little reason to package some pieces of it out into separate modules. The ideal, of course, would be if RequestTemplateFactoryResolver used some sort of SPI discovery to register factories, but Feign isn't designed around this, so there will be some pollution of the dependency graph.

Looking at the utility classes you asked about, there are two categories:

Category 1 - PartDataFlattener, PartDataTemplateFactory, and ContentDisposition - these are truly internal to the BuildMultipartTemplateFromArgs class. I see no problems with these being inner static classes.

Category 2 - Rfc5987Util - this is general purpose functionality that should probably be available across all of Feign.

If we want to keep the core api file hierarchy clean, what do you think about having a dedicated MultipartRequestTemplateFactory.java file, then put everything related inside that file as private static classes?

[trumpetinc]

I have a few thoughts on the latest MultipartFormEncoder...

First off, I want to make clear that the approach does work. I just think that it requires users to do some very awkward configuration (I'll give concrete example of this down below).

I have a couple of comments specific to the approach itself (the code as written is great - all of my comments are about the approach itself):

1. Specific to the PartBodyFactory class, the code is relying on the encoder not throwing an EncodeException if it is able to encode a partData. This behavior is not part of the Encoder API specification - Are we assuming that if a call to Encoder.encode() doesn't throw that it absolutely handled the encoding? Is that a safe assumption? If yes, then that opens up some possibilities. Let me know.

2. My biggest comment is to ask you to think through what the user is going to have to do to set up the encoders in Feign config...

With this latest approach, the user has to think about all sorts of things to enable multipart handling (including combining encoders together to create a PartBodyFactory).

So let's assume that I want to use the behavior of DefaultEncoder, plus a FileEncoder (b/c that isn't part of DefaultEncoder), plus JacksonEncoder for everything else - and I want that to work with Multipart bodies (as well as regular requests). The config code is going to be very messy. To even make this work, I'm going to have to introduce something like the MultiEncoder concept I've talked about previously (if I'm wrong on this, definitely let me know).

I'll point out that it isn't even possible to easily set up the encoder chain that I described here with current Feign classes, so let's assume that I also have created a MultiEncoder class that loops a list of encoders until it finds one that doesn't throw (assuming that is even a reliable way of determining whether a given encoder actually encoded or not):

  Encoder baselineEncoder = MultiEncoder.of(new DefaultEncoder()).then(new FileEncoder()).then(new JacksonEncoder());
  Encoder multipartEncoder =  MultipartFormEncoder.builder()
                    .partBodyFactory(
                        new PartBodyFactory(
                            List.of(
                                new DefaultEncoder(),
                                new FileEncoder(),
                                new JacksonEncoder() )))
                    .delegate(baselineEncoder)
                    .build())
Feign.builder().encoder(multipartEncoder)
   ...

So yes, it is doable.

But it's nowhere near as clean as the following (which is what would be possible if we do the work in a MultipartRequestTemplateFactory class):

  Feign.builder().encoder(MultiEncoder.of(new DefaultEncoder()).then(new FileEncoder()).then(new JacksonEncoder(), new MultipartEncoder()))
   ...

I know that I would prefer to write the second example :-)

The one downside is that if the user truly wants completely different behavior for the multipart handling than for other requests (e.g. if they want to use a different encoder based on a content-type predicate for multipart vs other Feign methods), they will have to create a separate Feign config for multipart. But I think this scenario is very, very unusual.

I'm looking forward to hearing your responses!

[yvasyliev]

I haven't had much free time lately either. 😊 Are you willing to get your hands dirty and submit a change by yourself?

Regarding your comments:

I'm digging into your question about the PartDataFlattener, etc... utility classes being in core... This is a fair question and important for us to consider. The question is this: Are we willing to do something very ugly with the architecture to preserve the module separation for multi-part handling? Because trying to do everything in the encoding layer is going to do some very ugly things.

The utility classes were originally private methods in my code. To make the code clean, I moved the private methods to package-private utility classes. I find the utility classes ugly by themselves regardless of whether we put them in the api or form module. And I wouldn't expose either of them publicly, and I'm now thinking that PartDataTemplateFactory must be package-private too.

If we forget about Feign and this PR for a moment, Part generally belongs to MultiPartForm. It means that in a perfect world, an imaginary multipart form processor would delegate request metadata to a MultipartFormFactory and the MultipartFormFactory would delegate next to a PartFactory.

The way I see it, the MultipartFormEncoder from this POC is the MultipartFormFactory, and PartBodyFactory is the PartFactory.

Additionally, there's a logical problem with the suggested approach: BuildMultipartTemplateFromArgs encodes parts first and passes the already encoded (!) parts back to the same encoder, this time, wrapped into the MultipartFormData. Why not replace encoder.encode(formData, MultipartFormData.class, mutable) with the mutable.body(formData) then?

Thus, the reason I'd prefer to keep the utility classes in the form module is to avoid api module pollution and avoid breaking the single responsibility principle in the BuildMultipartTemplateFromArgs.

---

1. Specific to the PartBodyFactory class, the code is relying on the encoder not throwing an EncodeException if it is able to encode a partData. This behavior is not part of the Encoder API specification - Are we assuming that if a call to Encoder.encode() doesn't throw that it absolutely handled the encoding? Is that a safe assumption? If yes, then that opens up some possibilities. Let me know.

We have to assume it unless we want to introduce breaking changes in the Encoder interface (remember Encoder#supports method from an old PR? 😊). feign.form.multipart.ConditionalEncoder is there to support the assumption. We can additionally enforce it by migrating PartBodyFactory#partBodyEncoders from List<Encoder> to List<ConditionalEncoder>, but it would be a bit too restrictive imo.

---

2. My biggest comment is to ask you to think through what the user is going to have to do to set up the encoders in Feign config

The scope of this change is multipart form encoding only. I see your arguments for the MultiEncoder, but isn't it outside the #2734 context? If we're talking about parts encoding only, PartBodyFactory virtually duplicates the functionality of the MultiEncoder. If it helps, I see the possibility for hiding PartBodyFactory from the user eyes behind the constructor/builder so that

MultipartFormEncoder.builder()
    .partBodyFactory(
        new PartBodyFactory(
            List.of(
                new Encoder1(),
                new Encoder2(),
                new Encoder3() )))
    .build()

becomes

MultipartFormEncoder.builder()
    .partBodyEncoders(
        List.of(
            new Encoder1(),
            new Encoder2(),
            new Encoder3() ))
    .build()

For List, the order matters by definition. In this POC, Encoder1 and Encoder2 can either implement ConditionalEncoder or be wrapped into ConditionalEncoder to make the chaining work.

---

Hey @velo, we don't seem to see eye to eye on the multipart form encoding architecture. Do you have any thoughts/recommendations? I'm happy to proceed with whichever approach has stronger consensus.

[trumpetinc]

@yvasyliev yes - I can definitely get my hands dirty :-). I have time Friday that I can focus on this.

We have to assume it unless we want to introduce breaking changes in the Encoder interface (remember Encoder#supports method from an old PR? 😊)

Yes, I definitely remember it! My proposed "solution" for this is to have encode() return a boolean indicating success or failure. But if we don't want to do that, then I guess an even easier fix will be to change the javadoc to indicate "If this encoder is unable to encode the result, it must throw an EncodeException. Successful return from this method indicates that encoding was performed."

---

The scope of this change is multipart form encoding only.

I respect that - however, I think that it is important to think about the big picture and not duplicate code. If we see a need (and I truly do) to handle a chain-of-responsibility for encoders, then it would be unfortunate to solve that problem locally (specific to this PR), then find that our local solution isn't compatible with some future global solution.

There are some fundamental issues with how Feign handles encoder registration, and if fixing those is surgical, non-breaking, and creates an elegant solution for the current PR, then I think we should seriously consider it.

[trumpetinc]

I think that it is helpful to lay out real-world examples where Multipart handling is a part of a bigger Feign usage. This is where the awkwardness of the current approach becomes visible. My opinion is that architecture is super important, but not if it gets in the way of the API being intuitive.

To make this clear, I started thinking about what kind of migration tutorial documentation we would need to create to encourage people to move to the new approach.

Let's look at the user experience using the existing multipart handling:

// Existing Feign functionality
MyClient client = Feign.builder()
    .encoder(new FormEncoder(new JacksonEncoder())) // Wraps standard encoder
    .decoder(new JacksonDecoder())
    .target(MyClient.class, "https://example.com");

now consider how this would have to change for the current PR approach (PS - I'm replacing the List.of() call with vararg method to clean things up a little bit further):

// PR approach
MyClient client = Feign.builder()
    .encoder(MultipartFormEncoder.partBodyEncoders(new FileEncoder()).delegate(new JacksonEncoder()).build())
    .decoder(new JacksonDecoder())
    .target(MyClient.class, "https://example.com");

Yes - it works, but now the user has to think about partbody encoders vs regular encoders, they need to understand delegates - and almost certainly will wind up having to duplicate creation of encoders. For a complex scenario with multiple encoders, it becomes awkward.

But - and this is really important: the above example doesn't even support streaming upload of File that aren't multipart. To support direct File upload streaming, we have to somehow get an encoder that can handle multiple types - so something like DefaultEncoder - but now we need to add FileEncoder to that.

Yes - we could always add more things to DefaultEncoder - but that is really getting away from the Feign design intent.

With the approach I am proposing, this becomes as follows (this assumes we make encoder() accept vararg in the Feign config builder):

// My approach (encoders as simple plugins)
MyClient client = Feign.builder()
    .encoder(new MultipartFormEncoder(), new FileEncoder(), new JacksonEncoder() )
    .decoder(new JacksonDecoder())
    .target(MyClient.class, "https://example.com");

The user doesn't have to understand anything about MultipartFormEncoder other than the fact that they want to support multipart, file, and JSON encoded endpoints. It becomes much more like a plugin based architecture, and I think that is desirable.

[trumpetinc]

Why not replace encoder.encode(formData, MultipartFormData.class, mutable) with the mutable.body(formData) then?

Sorry - I missed this question.

The answer is because the user may want different encoding strategies for MultipartForm. We could, for example, have another encoder implementation that does URL query parameter encoding. I just don't see any reason to take the decision away from the Feign system-wide encoder(s).

The general approach I'm proposing is exactly what we would do if we manually constructed a MultiPartForm without the annotations:

1. Encode the body for each part (delegating to the Feign encoder chain)
2. Package those bodies up into a multipart
3. Encode the multipart (delegating to the Feign encoder chain)

[trumpetinc]

@yvasyliev I may have found a way to get the best of both worlds...

What if we added the encoder to the MultipartFormData constructor and pass it in from BuildMultipartTemplateFromArgs? Then the MultpartFormEncoder can construct a PartBodyFactory on the fly (or pass encoder to the PartBodyFactory.create() method) as it encodes each part.

If we do this, then the multipart encoder will use the Feign-wide encoder, but still keep all those utility classes in the form module.

[yvasyliev]

Hey @trumpetinc, I've spent time re-reading our discussion and researching how other libraries (Spring HTTP Interface,
Retrofit, Micronaut) handle multipart. I now think your approach is the right one 😅. A few things brought me around:

1. All the other libraries have some form of canEncode() helper method — Spring's HttpMessageConverter.canWrite(),
   Retrofit's Converter.Factory returning null for unsupported types, Micronaut's MediaTypeCodec. A user
   configures a list of encoders once, and they are reused all across the code. The approach from this POC,
   MultipartFormEncoder.builder().delegate(...).partBodyEncoders(...).build(), will certainly fall behind other
   libraries in terms of user experience.

2. It seems that we bumped into Feign's long-standing architecture gap where the Encoder interface doesn't have
   canEncode helper method. ConditionalEncoder from this POC is actually a workaround rather than a proper
   implementation. The problem is that EncodeException is ambiguous — it could mean "I don't support this type" or
   "something genuinely broke during encoding." Using exceptions as a "can you handle this?" signal conflates these two
   cases. Encoder#supports method from the old PR wasn't a bad idea after all. 😅

3. As for implementation: rather than introducing a new MultiEncoder type, I'd prefer an Encoder.of(List<Encoder>) /
   Encoder.of(Encoder...) factory on the Encoder interface itself, backed by a
   default boolean canEncode(Object, Type, RequestTemplate) method with a default return true for backward
   compatibility. The user would write
   Feign.builder().encoder(Encoder.of(jacksonEncoder, defaultEncoder, multipartEncoder)).target(...).

4. If we now operate with a single list of encoders, it makes sense to move the parts encoding from PartBodyFactory to
   BuildMultipartTemplateFromArgs.

What concerns me is that introducing canEncode() touches nearly every part of Feign — far beyond the scope of the "add
body streaming feature" topic. I believe it was the main reason why the old Encoder#supports PR was reverted.
Realistically, this needs its own issue and its own PR, independent of #2734. But it is a prerequisite for a clean
multipart implementation: without it, moving part encoding into BuildMultipartTemplateFromArgs isn't possible, because
the factory wouldn't know which encoder to delegate to for each part.

Bottom line: I think we should tackle this in two steps — first a focused PR to add Encoder.canEncode() +
Encoder.of(), then a multipart PR on top of it. Does that sequence make sense to you?

Just an FYI: I'm itching to start coding this. Let me know if you want to take the lead on the implementation, otherwise
I'll start drafting something... 😊

PS: the DefaultEncoder now handles file bodies, so no real FileEncoder is required (I saw you were referencing to FileEncoder across several comments 😊)

[trumpetinc]

@yvasyliev please see my proposal for allowing BuildMultipartTemplateFromArgs to push the Feign encoder into MultipartFormData.

I think if we do this that we get the best of both worlds...

[trumpetinc]

As for fixing the Encoder chaining, my proposal is that instead of adding another method to Encoder, we change the return type of encode() to be a boolean,.

If that returns true, then encoding was handled. If it returns false, then the encoder did not support the encoding request. If an EncodeException is thrown, it means that the encoder could have handled the request, but something bad happened that the user needs to know about.

I think there are a lot of advantages to doing it this with vs adding another method:

1. Encoder continues to be a functional interface
2. Inside the Encoder, the encode() method is going to need to check to make sure it can actually encode the request anyway - having the separate method is awkward.
3. It maintains source compatibility (but not byte code compatibility) of the encode() method
4. It forces proper implementing of any existing Encoder implementations. If we try to rely on a default method implementation, then we are going to continue to rely on the throwing of exceptions, and that is going to really muddy the waters.

The reality is that we are already making a breaking change with 14.X, so I think this is a pretty minor change to users.

I do acknowledge that we'll need to rewrite all of the existing encoders so they return false instead of throwing, but this seems like a pretty reasonable change.

[trumpetinc]

I'd prefer an Encoder.of(List) / Encoder.of(Encoder...) factory on the Encoder interface itself

I completely support this - it is much cleaner.

[trumpetinc]

Bottom line: I think we should tackle this in two steps — first a focused PR to add Encoder.canEncode() +
Encoder.of(), then a multipart PR on top of it. Does that sequence make sense to you?

Just an FYI: I'm itching to start coding this. Let me know if you want to take the lead on the implementation, otherwise
I'll start drafting something... 😊

I agree with the two PRs as you describe. It will be VERY nice to fix the encoder chaining issue!

I do want to get the canEncode() vs boolean encode() decision nailed down. At the end of the day, either way is going to be OK - so I acknowledge that I may be overly picky with trying to make it elegant :-) - and it may be better to have it be less elegant and not force us to touch the existing Encoders...

** EDIT: ** I have changed my mind. I think that using a separate method with default implementation is a problem. We don't want to have the multi-encoder catch EncodeException at all. If any encoder throws an EncodeException, it should halt the encoding sequence so the user will know what the problem is. If we use the default method approach, then I think the multi-extractor is going to have to still catch EncodeException (to handle the legacy cases), which undermines the entire purpose of having the other method!

I would say that you should proceed with the PR you proposed (I won't be able to work on any of this until tomorrow). I am at my desk right now, so I'll see any responses and reply quickly (and you have my direct email address if you want to move quicker).

[yvasyliev]

** EDIT: ** I have changed my mind. I think that using a separate method with default implementation is a problem. We don't want to have the multi-encoder catch EncodeException at all. If any encoder throws an EncodeException, it should halt the encoding sequence so the user will know what the problem is. If we use the default method approach, then I think the multi-extractor is going to have to still catch EncodeException (to handle the legacy cases), which undermines the entire purpose of having the other method!

So, would it be a problem to do something like this?

@RequiredArgsConstructor
class DelegatingEncoder implements Encoder {
    @NotNull private final List<Encoder> delegates;

    @Override
    public void encode(
        Object object,
        Type bodyType,
        RequestTemplate template
    ) throws EncodeException {
        for (Encoder delegate : delegates) {
            if (delegate.canEncode(object, bodyType, template)) {
                delegate.encode(object, bodyType, template);

                return;
            }
        }
        
        throw new EncodeException("Not encoded");
    }

    @Override
    public boolean canEncode(
        Object object,
        Type bodyType,
        RequestTemplate template
    ) {
        return delegates.stream()
            .anyMatch(delegate -> delegate.canEncode(
                object,
                bodyType,
                template
            ));
    }
}

---

It's late evening for me. I won't start implementing anything till tomorrow either 😅

[trumpetinc]

@yvasyliev The problem is going to be caused by the default method implementation in Encoder:

default boolean canEncode(Object, Type, RequestTemplate) { return true; }

The result of this is that every "old" Encoder is going to get it's encode() method called, which will then throw EncodeException. So the first old Encoder that you hit that can't handle the request will cause the entire encode operation to fail, even though there are probably other encoders later in the chain that can handle the request.

And that failure is a runtime failure which will be nasty to detect (a user could have some Feign endpoints that work fine, then others that would fail, all depending on the order of the encoders in the chain).

I really think it is much better to force a compile-time error.

[trumpetinc]

@yvasyliev I really think we are close to having this completely done! Because you will be awake earlier than I will, please go ahead and run with the two PRs - you have earned a huge amount of appreciation from the community for all your work on this.

Summarizing the open discussion items (I think this is all that is left):

1. Consider whether it makes sense to a) have BuildMultipartTemplateFromArgs push the Feign encoder into MultipartFormData and have the encoder crate the MultiPartForm. Or b) have BuildMultipartTemplateFromArgs be responsible for constructing the MultiPartForm itself. I'm good with either - (a) would allow a little more code to be pushed to the form module.
2. Make a decision about whether to return boolean from encode(), or to have a default method.

PS: What do you think about changing the Feign config .encoder(Encoder) to be .encoder(...Encoder) - that would be source compatible, but a binary change - not sure if it's worth messing with? This would just be "nice to have". One reason to not do that right now is that it raises the question of why .decoder() doesn't support multiple decoders. I think that is a problem for another day!