Add support for streaming on requests (and add better handling for streaming in responses)

Related issue - my apologies if this is a double post - I wasn't sure how to make sure the discussion was being viewed by the appropriate people: #1243

Problem

Feign currently uses a byte[] when processing the request body. This does not work for transfers of large request bodies (for example, uploading large files).

Requested solution

Make request bodies be designed around streams.

Status/Findings

I just finished up creating a bunch of shims to make streaming requests work in Feign, and would like to see if ya'll would be open to having a discussion about how to make this part of the main library. This works, but it is ugly.

Doing this properly will require a breaking change, so I am hesitant to create a pull request without some good discussion about the best way to do it, where we can and can't break API, etc...

Possible Approach

Based on the workaround implementation I created, I believe that the general approach will be as follows:

First, the existing Response.Body class is perfectly suitable for handing both Request and Responses. It actually looks like Response.Body was designed with this in mind. I think that refactoring this into a standalone Body interface, with concrete implementations for ByteArrayBody, InputStreamBody, NoContentBody, and possibly a FileContentBody. I also suggest removing calls specific to encoding, etc... from this class. I would like to get rid of null checks on the body parameter and just use a no-op NoContentBody if possible.

Next, there will be some surgical changes to Client.Default (for requests, we need to write a stream instead of a byte[])

There are some questions about calling close() (or not calling close() ) at the end of the response.

Another big impact area is going to be in logger implementations (which need to either not do body logging on streamed bodies, or need to use a different approach that monitors the stream but does not actually read the stream as part of the logging call).

And I think there should be some discussion about supporting InputStream as a body parameter type, and possibly OutputStream as a client method response parameter type. Maybe these could be handled with specialized encoder/decoder implementations, but it's worth discussion about whether these should be part of the core library. Might also discuss a File type as a body parameter type (this would allow setting content-length header and make the body retryable, which would worthwhile.

Finally, all of my work has been on the synchronous implementations - so evaluation with respect to Async will be needed.

Conclusion

All in all, while the design discussion will be important, I am confident that it is possible to make this change without massive amounts of surgery. And I think this would be a huge enhancement to Feign. This is a huge gap with Jax-RS that would be good to close.

Is there interest in pursuing this? I'm happy to do the grunt work on the pull request, but I think this is a big enough breaking change that there should be discussion to get the design right before I start slinging code.

Thanks for taking the time to write this up. You are on the right track by understanding that our preference is to keep the core Feign objects simple and rely on specialized Encoder and Decoder implementations to deal with these sorts of scenarios. We've learned that trying to include this type of capabilities into core Feign result in best intentions gone wrong.

With that said, including OutputStream and InputStream specific implementations would be a welcome addition. Please feel free to submit a Pull Request with your ideas for our review.

ok - I will put together a PR over the next week or so, then we can have what I'm sure will be a lively discussion about some of the tradeoffs.

It would be great if Feign could provide an elegant way to support streaming large files.

Our organization has hundreds of applications using Feign clients to upload files to a central file service. While most files are relatively small (less than 20MB), there are occasional cases where files can reach sizes up to 2GB. Unfortunately, this causes our services to experience frequent OutOfMemory (OOM) errors due to Feign loading the entire file into memory.

To address this issue, I had to resort to a temporary and admittedly ugly hack. Specifically, I recompiled the FeignInvocationHandler class to handle large file uploads with a custom workaround. Here's an excerpt of the adjusted invoke() method:

@Override
public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
    // ...
    if (method.getDeclaringClass() == MyBigFileFeignClient.class &&
            method.getName().equals("uploadBigFile")) {
        return MyUglyTempWorkAround.streamingLargeFile(method, args);
    }

    return dispatch.get(method).invoke(args);
}

The class MyUglyTempWorkAround will use OkHttpClient directly to stream large file, full of hard codes.

I fully understand this is not ideal, but given the lack of native support for streaming large files in Feign, I had no other choice.

I hope the Feign team can consider adding native support for streaming large files in the future. This would not only address issues like ours but also make Feign more robust for handling various HTTP use cases.

Thank you for your consideration.

Initial pull request has been submitted ( #2750 ) - I am eager to get feedback and comments!

I have played around with this approach with different Encoder implementations, and I think that I need to do another design iteration. Requests should be writing to an outputstream, not providing an inputstream.

I'll do another iteration sometime this week.

@kdavisk6

@kdavisk6

I'm on my 3rd iteration - I have streaming working, but I'm not happy about some of the limitations inherent to my earlier designs. I think that I now understand the problem space well enough to propose a solid solution

I am at a point where I think some discussion is needed so I can understand design intent/etc...

Streaming Requests Design Overview

Many streaming Encoder implementations are going to be designed to write to an OutputStream. Case in point, the Multi-part MIME encoder will stream to an outputstream. The Jackson encoder would also be naturally implemented this way (current behavior is to write to a byte[], but it could just a easily write to an output stream). I also intend to support encoders writing to a Writer interface, which would encapsulate charset encoding for unicode output.

The approach that I am planning on pursuing for adding support for streaming requests is to introduce the concept of an OutputStreamSender for the RequestTemplate (and Request). This will have a single method ( send(OutputStream) ). For any unicode based bodies, we will have a convenience method that will take the unicode form of the request body and write it using the charset specified in the content-type header.

The net effect of this will be that instead of providing a byte[] body to the request, we will ultimately provide an OutputStreamSender. Encoders are ultimately going to provide an appropriate OutputStreamWriter, and for backwards compatibility/simplicity we will have convenience methods on RequestTemplate that accept byte[], String, etc... and construct a suitable OutputStreamSender.

I have 6 areas that I could use some clarification/guidance on:

1. Charset encoding in RequestTemplate (and to a lesser degree, in Request)

Right now, the only place that the charset encoding can be set different than the default (UTF_8) appears to be via the RequestTemplate#bodyTemplate() method. And that method is called by 2 unit tests, and nowhere else in the Feign project.

It seems to me that the correct place to assign charset encoding is via the header template (Content-Type, if charset is specified, otherwise utf-8, unless that was maybe a different implied encoding for older versions of HTTP). Everything else with the request really should be handled via unicode (if charset is important) or binary (if charset is not important).

So I would expect that we would see a bodyTemplate(String) method, but not a bodyTemplate(String, Charset) method. We would then capture the string, and encode it using the charset from the content-type header as the request is sent.

Basically, I am suggesting that the work to capture a byte[] for the request should be deferred to as late as possible, which means having RequestTemplate and Request be entirely agnostic of the charset encoding. Instead, the charset from the content-type header will be used to encode any unicode content as it is sent to the server.

My question: Is anyone going to object to a hard-deprecation on RequestTemlate#bodyTemplate(String, Charset) (i.e. where we basically ignore the Charset parameter in that method in preference to whatever charset is set in the Content-Type request header template) ? I don't really want to get into tweaking the content-type header based on this method argument...

2. Request Logging

Streaming request bodies are not always re-playable, so we need to be a bit careful with how logging of the body is done. There is a similar concern in the Request#toString() method.

There are two approaches that I can think of:

a) Specify a string representation of the request body in the RequestTemplate (when we set the body, we also set a string representation). For non-streaming encoders, the body content and string representation will be the same (though there is an argument to be made for setting the string representation of a binary request body to something like "-- Binary Content (xxx bytes) --" or "-- Binary Content (unknown length) --" instead of attempting to encode the byte stream as text).

b) Change how request body logging is done by logging the bytes as they are sent to the server. Basically, we would instrument the ouputstream that the encoders will ultimately be writing to. This would allow us to absolutely log the exact data sent to the server. The downside of this approach is that the actual logging of the request body wouldn't happen in the Logger#logRequest() method. This would force us to make Request be mutable, which I'm not very happy about. I also think that most times with a streaming request, logging the actual body content isn't desirable anyway.

My recommendation is to pursue approach (a). This will provide identical logging/toString behavior to the current implementation, and should be completely compatible with existing Encoder implementations.

My question: Does anyone see a problem with (a) as the approach to request logging?

3. FeignException body handling

This one is a bit awkward. FeignException currently has a field for the response body. But there is a static factory for creating FeignExceptions during request processing that looks like it mixes up the request and response bodies: FeignException#errorReading(Request, Response, IOException). This sets the FeignException response body to the request.body() value.

Similarly, in errorReading, the headers are set to the request headers instead of the response headers.

I'm honestly not sure what to make of this one.

My question: Can someone provide me with an explanation of why the request body is captured here instead of the response body?

**4. Response Logging##

Streaming responses may only be readable a single time (not resettable). There are a couple of options I see for handling logging.

a) Use a buffered input stream to support logging of up to the first X bytes of the stream. We then reset the stream for decoder body processing (basically, any call to Response.Body#asReader or #asInputStream will reset the stream as long as not too many bytes have been read).

b) We instrument the inputstream so it writes to the log output as the stream is processed. This has similar advantages and concerns as I described in the request logging section above.

My plan is to use approach (a). I have done this in an earlier iteration of my design, and it works well.

My question: Does anyone see a problem with (a) as the approach to response logging? Are we ok with not logging the entire response body if it exceeds some threshold?

##5. Charset in response bodies##

I believe that encoding for response bodies should always come from the response headers. Right now, there are two Response.Body#asReader() methods that ignore the headers. This puts a requirement onto the decoder to explicitly set the charset, which feels awkward and potentially error prone. If the content-type says the body encoding is XYZ, then we really need to be interpreting the body using that encoding, right?

This one isn't a show stopper for me, but it's odd enough that I felt it was worth asking about.

My question: Am I missing something here? Are there normal usage scenarios where it is desirable to interpret the response body using a different charset than specified in the headers?

##6. Use of nulls for body##

There are a lot of null checks happening related to request and response bodies. What I would like to suggest is that null body not be allowed. We can have an empty body (request will have a header of content-length=0), but not null.

My question: Am I going to step on toes if I try to do something about this? I am going to be reworking the bodies in requests anyway, so it seems like maybe the time to adjust this is now.

I am going to continue pursuing my 3rd iteration using my preferred approaches - please get back to me with any feedback!

I have submitted a PR with a branch of my 3rd design iteration. Branch currently covers streaming responses: #2754

I will work on streaming requests next.

Specifically, I recompiled the FeignInvocationHandler class to handle large file uploads with a custom workaround. Here's an excerpt of the adjusted invoke() method:
@Override
public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
    // ...
    if (method.getDeclaringClass() == MyBigFileFeignClient.class &&
            method.getName().equals("uploadBigFile")) {
        return MyUglyTempWorkAround.streamingLargeFile(method, args);
    }

    return dispatch.get(method).invoke(args);
}
The class MyUglyTempWorkAround will use OkHttpClient directly to stream large file, full of hard codes.

Ooh, that's dirty. I like it! 👍

Do you mind sharing some of your MyUglyTempWorkAround class, too?

Specifically, I recompiled the FeignInvocationHandler class to handle large file uploads with a custom workaround. Here's an excerpt of the adjusted invoke() method:
@Override
public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
    // ...
    if (method.getDeclaringClass() == MyBigFileFeignClient.class &&
            method.getName().equals("uploadBigFile")) {
        return MyUglyTempWorkAround.streamingLargeFile(method, args);
    }

    return dispatch.get(method).invoke(args);
}
The class MyUglyTempWorkAround will use OkHttpClient directly to stream large file, full of hard codes.
Ooh, that's dirty. I like it! 👍

Do you mind sharing some of your MyUglyTempWorkAround class, too?

Actually i already deleted the work around.

The MyUglyTempWorkAround is to iterate the "Object[] args", to find the MultipartFile type argument, and then use OkHttpClient to relay data from MultipartFile.getInputStream to okhttp3.RequestBody.

Since this feature requires a breaking change anyway, would you be open to redesigning the whole request body definition?

I'm thinking about something like this (rough draft):

Change feign.Request.Body class to an interface:

public interface Body<T> {
    T getData();
}

Create implementations:

@RequiredArgsConstructor
@Getter
public class FormUrlEncoded implements Body<Map<String, String>> {
    private final Map<String, String> data; // not URL-encoded
}

@RequiredArgsConstructor
@Getter
public class MultipartFormData implements Body<List<Part>> {
    private final List<Part> data;
    
    public interface Part<T> {
        String getName();
        T getValue();
    }
    
    public interface FilePart<T> extends Part<T> {
        String getFilename();
        String getContentType();
    }
    
    @RequiredArgsConstructor
    @Getter
    public static class StringPart implements Part<String> {
        private final String name;
        private final String value;
    }
    
    @RequiredArgsConstructor
    @Getter
    public static class DiskFilePart implements FilePart<File> {
        private final String name;
        private final File value;
        private final String filename;
        private final String contentType;
    }
    
    @RequiredArgsConstructor
    @Getter
    public static class StreamFilePart implements FilePart<InputStream> {
        private final String name;
        private final InputStream value;
        private final String filename;
        private final String contentType;
    }
    
    @RequiredArgsConstructor
    @Getter
    public static class ByteArrayFilePart implements FilePart<byte[]> {
        private final String name;
        private final byte[] value;
        private final String filename;
        private final String contentType;
    }
}

Then each feign.Client implementation can handle request Body according to the impl class and the underlying HTTP client capabilities.
feign-form becomes a part of feign-core (as it should've been in the first place - imo).
Encoders, loggers, and retry logic would all be adjusted accordingly.

@yvasyliev

@yvasyliev After working on this concept for several iterations (I've actually created and rejected 3 different approaches), I have come to understand that the fundamental issue is about matching the data transfer direction to the use case.

For example, in a response body, we are reading data from the http client. The http client is reading the data from the network interface. This is inherently an InputStream operation.

But in in a request body, we are trying to write data to the http client. The http client is writing the data to a network interface. This is inherently an OutputStream operation.

Today, OpenFeign is trying to accept an InputStream for request body content that it is then having to turn around and write to the http client (and funny enough, most http clients are built around an OutputStream - which makes sense when you think about what they are doing).

So there is an inherent mismatch - Feign has an InputStream. The actual operation that is being performed is an OutputStream activity. While it is certainly possible to stream in InputStream into an OutputStream using transferTo, that really isn't something that the Feign core should be doing.

So what I'm proposing (and the reason I think that this PR needs to be split up and reconsidered) is that we change it so Feign receives a request body that knows how to write to an OutputStream. Something like this:

interface RequestBody{
  public void writeToOutput(OutputStream os);
}

(as a side benefit, because this is a single method, it is easy to use as a functional interface.)

We can then easily create different body types we want. If we are dealing with an InputStream, then we can stream that to the Output. But if we are dealing with other formats (like the MultiPart you described), it can operate cleanly. There are a number of really good MultiPart libraries out there - and they all write to OutputStreams. Plugging those into Feign will be trivial once we make the adjustment I describe above.

For example, assume a Feign client interface method definition:

public interface MyApi{
  @RequestLine("POST /my-endpoint")
  void sendContentToServer(RequestBody body);
}

we could then call this using:

MyApi myApi = ...;

try(FileInputStream is = new FileInputStream(...) ){
  myApi.sendContentToServer(os -> is.transferTo(os));
}

That is very clean. And if we want to have a Feign add-on library that adds some nice handlers to encapsulate it further (including Reader, InputStream, MultiPart, etc...), then that's easy to do - without anything additional in Feign core.

The key is that core functionality of Feign doesn't have to have a lot of flavors, etc... Core should be providing the absolute baseline of functionality that plugins can build on top of.

Does that make sense?

Another thing that I want to clarify - while this will be a breaking change, I still want the breaks to be as isolated as possible - and I think we can do this in a way that will really only impact the various http client implementations. Ideally, regular users of Feign won't be impacted by the change at all.

@trumpetinc

I decided to contribute to this proposal since #2754 is outdated and @trumpetinc don't have enough capacity to complete the change.

This feature request requires quite a lot of changes, and find it better to complete it in (around) three steps:

[feign-core] Make Request.Body streaming-ready.
[feign-form] Make MultipartFormContentProcessor accept InputStream values for streaming.
[feign-core] Make response body streamable.

I submitted #3360 to implement the first step. If it's approved, I can procced with the second step.

My plan for step 2 is to create class MultipartBody implements Body in the feign-form module so that it implements both writeTo(OutputStream) method for streaming & toString() for logging. I'll might need to create helper FormDataPart class(es) so that MultipartBody will store a List<FormDataPart> representing each multipart body parameter.

@yvasyliev

@yvasyliev the new PR looks really good - that is a very extensive overhaul - I am impressed, and grateful.

I also agree with the phases you are proposing (I would suggest a phase 2.5 that adds support for OutputStream and Writer for endpoints that send large bodies without Multipart wrapping (many REST services do not use multipart). This will be very simple once phase 1 is complete.

@PARAM

Thank you - your answer really emphasizes something I've been leaning towards as a deeper issue:

I have always found the current approach in Feign of sending a magic Map<String, Object> to the encoder be pretty clunky. Even the determination of whether an argument is multipart related is weird (see PathVariableParameterProcessor.processArgument() - it literally says “if the argument isn’t referred to in the URL, queries or headers, then just assume it is a multipart form value”. This is really not a good design. And I believe that continuing to try to solve inside that original deign decision is making things very awkward.

So I have an alternative proposal - this is not completely thought through, but I wanted to get your initial reaction.

Baseline

As background, I think it is important to recognize that the multi-part form encoding standard basically treats each part as a mini-html document (you know all this - I'm just laying it out to build my proposal). Each part has it's own headers and it's own body. In essence, each part of a multi-part post is itself a lightweight RequestTemplate (absent the 'path' - though I could make an argument that the 'path' in this case would be the part name!).

We can easily introduce a MultipartForm entity and create an encoder that supports it:

public class MultipartForm{
  List<MiltipartPart> parts;
}

public class MultipartPart{
  Body body;
  Map<String, HeaderTemplate> headers; // am I the only one that wishes Feign had done this with a dedicated HttpHeaders class??
}

and the Feign interface class would be:

@RequestLine("POST /files")
public UploadResponse uploadAFile(MultipartForm multipart);

This would require users to build their own MultipartForm instance, but we want to make this more Feign-like (annotation driven syntactic sugar). I think the majority of the work here is about annotation processing required to automatically build a MultipartForm instance.

Here's what it would look like if users had to build the MultipartForm instance themselves (I'm using a default method in the Feign interface):

@RequestLine("POST /files")
public UploadResponse uploadAFile(MultipartForm form);

default public UploadResponse uploadAFile(File file1){
   return this.uploadAFile(MultipartForm.builder()
      .part("file1")
        .withHeader("Content-Type: application/xml")
        .withHeader("Content-Disposition: form-data; filename=\" + file1.getName() + "\"")
        .andBody(new Request.PathBody(file1.toPath()))
      // we could add more parts here if desired
      .build()
   );
}

The above is not terrible, but could improved by making it more Feign-like.

Syntactic Sugar via Annotations

Here are the requirements as I understand them for the syntactic sugar:

Parts need to be named
Parts need to be converted from object into Body
Automatic unwrapping of collections and arrays of objects (that will then be encoded to obtain the headers and Body) into multiple parts (presumably all having the same name)

Note that this can’t be for all collections and arrays because maybe the user wants to actually encode the collection (i.e. a part that encodes an array of strings into json)
A mechanism for users to optionally specify the type of encoding to be done for a given argument (e.g. “Use JacksonEncoder for this argument; User XMLEncoder for this other argument”)

The fundamental problem that we keep running into is that there isn't an elegant, intuitive way to specify headers+body in the @PARAM annotation. So far, the majority of our logic in the Encoders is about working around this problem and trying to figure out what the user intended to have happen so we can create the MultipartForm instance.

Proposal

My proposal is that we introduce a new annotation (@Multipart) that does what we actually want - instead of trying to force the current annotation to work. I believe that the end result will be far more intuitive to users.

Feign Architecture Note:

Note that one ramification of this is that Encoder should not be attempting to process arguments. Our MultipartFormEncoder should just be encoding a MultipartForm instance.

The process of converting annotations into a MultipartForm instance happens much earlier in the Feign processing lifecycle - specifically in the RequestTemplate.Factory.create() method (see RequestTemplateFactoryResolver.resolve() for where the selection of the appropriate factory for a given method happens).

Proposed annotation:

@Multipart(name=”myname”, headers={“aaa: bbb”, “ccc: ddd”}, unwrap=true)

applied to a method parameter that contains the value that will be the body of the multipart form part
headers should be templates that allow resolving of any @PARAM arguments.
unwrap - converts collection/array to separate MultipartParts (I think we want default on this to be true)

Note: What I really want to do is have an encodeWith=JacksonEncoder.class parameter. But the chain-of-responsibility pattern makes this almost impossible to do without very ugly hacks. I'll discuss this more in a second.

The problem with chain-of-responsibility design for Encoders

Registering multipart-specific encoders with this new approach may not be practical. The problem is that RequestTemplateFactoryResolver (which is where the annotation processing happens) has a single Encoder. This is where the chain-of-responsibility pattern causes a lot of difficulty.

So I think that we are going to have to use the Encoder that is configured for Feign. In the end, I think this will result in a cleaner, more intuitive experience for the user (they only set up encoders once), but it is going to require careful design to implement in a way that doesn't make the user code messy.

I think using your original predicate/content-type approach will work - I just think we need to stick with the chain of responsibility design for Encoders (instead of registering multipart-specific encoders).

Something like this:

public class ConditionalEncoder{
  Encoder target; // called if predicate matches
  Encoder delegate; // called if predicate does not match
  EncoderPredicate predicate;
  
  void encode(Object object, Type bodyType, RequestTemplate template) throws EncodeException{
    if (predicate.test(object, bodyType, template)){
      target.encode(object, bodyType, template);
    } else {
      delegate.encode(object, bodyType, template);
    }
  }

}

This will result in a lot of nested constructors, so a fluent interface will probably be helpful:

EncoderChain{
  - EncoderChain then(encoder) ...
  - EncoderChain thenForContentType(contentType, encoder) ...
  - EncoderChain thenForPredicate(predicate, encoder) ...
}

The user's Feign setup code would then look like this:

  Feign.builder()
  .encoder(
    new EncoderChain()
    .thenForContentType('application/xml', new XmlEncoder())
    .then(new FileEncoder())
    .then(new PathEncoder())
    .then(new InputStreamEncoder())
    .then(new MultiPartFormEncoder())
    .then(new JacksonEncoder())
  )
  ...

and the Feign interface would be like this:

@RequestLine("POST /files")
public UploadResponse uploadAFile(
  @Multipart(name="file1", 
                     headers={"Content-Type: application/xml") File file1);

public UploadResponse uploadAStream(
  @Multipart(name="file1", 
                     headers={"Content-Type: application/xml", "Content-Disposition: form-data; filename=\"{filename}\""}) InputStream file1Stream, String filename);

Problems with the above approach

For the uploadAFile example above to work, we have to figure out some way to set the Content-Disposition filename header value in the RequestTemplate. Making this more complicated, there are other fields of the File or Path that we might want to push into header values or even other multipart form parts (file modified/created date, for example) - so this is not as simple has just handling the filename extraction. The most obvious solution I can think of would be to support bean expansion of expressions in the templates (for example `{file1.name}') - that could then be used anywhere in the headers.

To Be Figured Out

RequestInterceptor is extremely useful for applying headers - I have not thought through what the analog for multipart would be. Possibly just wrap new MultipartEncoder() with our own pre-handler that adjusts the MultiPart instance headers as desired.

Wow, that's a great plan! I like the idea of resolving part's headers template.

I have always found the current approach in Feign of sending a magic Map<String, Object> to the encoder be pretty clunky. Even the determination of whether an argument is multipart related is weird (see PathVariableParameterProcessor.processArgument() - it literally says “if the argument isn’t referred to in the URL, queries or headers, then just assume it is a multipart form value”. This is really not a good design. And I believe that continuing to try to solve inside that original deign decision is making things very awkward.

Do you wish to modify that logic for regular body encoding & application/x-www-form-urlencoded bodies too?

Proposed annotation:

@Multipart(name=”myname”, headers={“aaa: bbb”, “ccc: ddd”}, unwrap=true)

It looks like the @Multipart annotation combines @Param, @Headers & @RequestLine functionality. At the moment, I'm clueless regarding implementation complexity, but I would consider something like this too:

public interface Client {
    @RequestLine(
        value = "POST /",

        // alternative to unwrap=true
        collectionFormat = CollectionFormat.EXPLODED
    )
    @Headers("Content-Type: multipart/form-data")
    void sendFile(
        @Param("file")
        @Headers("Content-Disposition: form-data; filename=\"{filename}\"")
        @Headers("Content-Type: application/xml")
        InputStream file,
        
        @Param("filename")
        String filename
    );
}

The user experience for urlencoded bodies is pretty nice, so I would not want to touch that. The internal implementation is a bit awkward, but it works. So I don't think any change is needed there.

The multipart handling really doesn't work well, and the lack of streaming support means that we need to change it anyway. So I think it is worth the work to change.

Can you please explain what you meant about "Do you wish to modify that logic for regular body encoding" ? I don't see anything that would need to change for regular body encoding so I think I may be missing something.

For the annotation design, what you are asking about is possible. Java 8 (which Feign requires) added ability to repeat an annotation like this.

Re-using the existing headers annotation might make it easier to reuse template processing. And it is intuitive for the user. I like that.

I still think that we should use @multipart (vs re-using @part ). This clearly indicates the users intent (plus allows us to have additional attributes to control the multipart behavior if needed).

As a general observation, what we are doing here is making a mini requesttemplate for each multipart argument. We may even be able to code it that way and avoid a lot of DRY.

What other CollectionFormats do you expect to see? Or is this just a future proofing approach? I'm fine with using an enum instead of boolean.

Oh and you are suggesting we add collectionFormat to the RequestLine annotation... My general rule is to never solve a local problem with a global solution. If this would be specific to multipart requests, then we need to keep the attribute in the multipart annotation. This is one reason I want to use @multipart for this.

feign.CollectionFormat is an existing enum; it's used for encoding Collection types in both query parameters & application/x-www-form-urlencoded requests. And in both cases, it's applied globally per request, so I thought we could do the same for the multipart requests. But on the second thought, I don't see a good CollectionFormat alternative for unwrap=false scenario. We have to either introduce CollectionFormat.NONE constant or all agree that CollectionFormat.EXPLODED -> unwrap=true & CollectionFormat.<anything_else> -> unwrap=false.

I just found another doubt:

public interface Client {
    @RequestLine("POST /")
    @Headers("Content-Type: multipart/form-data")
    void sendFile(
        @Multipart(
            name = "file", 
            headers = {
                "Content-Disposition: form-data; filename=\"{filename}\"",
                "Content-Type: application/xml"
            },
            unwrap = true
        )
        InputStream file,
        
        @Param("filename")
        String filename
    );
}

The name parameter is mandatory for the Content-Disposition header. Omitting the name value in the Content-Disposition header declaration feels unnatural imo. Yet I can't find a good alternative at the moment.

@PARAM

Re-using the existing CollectionFormat enum would be very good. I will need to take a look at how the enum is implemented (i.e. is it a simpler marker enum, or does the enum actually have associated behavior like a Strategy?). If we don't want to have a NONE instance, then a null value behavior (i.e. default) would probably be fine.
I agree about the content disposition header + name... we could certainly do something like this:

        @Multipart(
            headers = {
                "Content-Disposition: form-data; name=\"file\" filename=\"{filename}\"",
                "Content-Type: application/xml"
            }
        )
        InputStream file,

This would be strictly correct, but it starts putting a lot of responsibility on the user - and I am not happy about that.

One possible approach would be to add filename as an optional setting to the @Multpart annotation

@RequestLine(...)
public UploadResult upload(@Multipart(name="myFile", filename="{fileName}") Path myFile);

@RequestLine(...)
public UploadResult upload(@Multipart(name="myFile", filename="{fileName}") InputStream myStream, @Param("fileName") String fileName);

In the above, 'fileName' would either pull from any @PARAM annotation that is named 'fileName' - and if not, it will be evaluated as a Bean getter against myFile. This allows us to set the filename in the header automatically, even in cases where the argument is a collection that needs to be unwrapped.

How is that feeling?

I spent some time thinking about Encoder today and wanted to share. We can solve the multipart encoding requirements from a technical perspective - and the annotation/interface side of things will be very clean and elegant - but the aesthetics of the Feign configuration become horrible.

Here's what I think is going on:

Thoughts on Encoder

The current Encoder interface has no way of indicating to the caller whether it actually encoded the body or not. When multiple encoders are involved, we have the unfortunate situation of having to use a delegate provided to the encoder constructor.

For 'end of chain' encoders like 'JacksonEncoder' this isn't a big deal. But once you have an API that requires multiple encoders, actually configuring the encoder chain becomes very ugly.

For example:

Encoder enc = new Encoder1(new Encoder2(new Encoder3(new JacksonEncoder())));

I feel like I am coding in Lisp - I can't even keep track of the number of closing parenthesis required! And if we introduce a predicate based conditional encoder, this is going to explode in ugliness.

Fundamentally, the long ago decision to use a quick constructor delegate probably made sense when there were only one or two types of Encoders - but things have gotten complex over the years, and this decision is no longer simple.

We can solve the multi-part encoding requirements from a technical perspective - and the annotation/interface side of things will be very clean and elegant - but the aesthetics of the Feign configuration become horrible.

What we really need is a proper (easily configured) chain of responsibility where processing stops at the first element of the chain that has processed a request.

Possible solutions

The most obvious way to make a chain of Encoders (that is easy to configure) is using a MultiEncoder implementation that has a list of registered Encoders, and it loops them in order until it finds one that can handle the request. Unfortunately, this requires us to be able to tell whether a given Encoder actually handled the request. Today, there is no clean mechanism for this.

I have kicked around the following ideas of how to determine if a given Encoder actually handled the request or not:

We could check the RequestTemplate body and see if it is set after an encoder runs. But this doesn't address the fact of all of these delegate encoders.
We could catch EncodeException as an indicator that the encoder was not appropriate for the input. But this doesn't address the fact that EncodeException could easily indicate a real problem that needs to be thrown up the call stack.

Neither of these will work.

I think we may be looking at another breaking change

This makes me think that it might be necessary to fix the Encoder chaining issue once and for all and get rid of the delegate pattern entirely. With the Request.Body change, we are already making a breaking change that will impact anyone with their own Encoder implementation. I don't take this lightly (I absolutely hate making breaking changes) - but if we are going to ever make this sort of change, doing it while we are making other breaking changes that impact Encoder seems to make sense.

Proposed breaking change

I am proposing that we change the Encoder.encode() method signature so it returns a boolean to indicate whether it handled the request or not. And we adjust all current Encoder implementations so in cases where the encoder can't handle the input, it returns false instead of throwing EncodeException.

The RequestTemplateFactoryResolver.BuildTemplateByResolvingArgs and BuildTemplateByResolvingArgs would be adjusted to throw their own EncodeException if the Encoder.encode() method returns false.

What I'm not suggesting

Note that I'm not proposing we add multiple encoder registration to Feign config - I still think we can handle that using a MultiEncoder implementation.

But without having the encoder return true or false, I can't see any way to not wind up with the configuration being incredibly ugly.

Do you see a different/better approach? If we don't want to propose another breaking change, we can make this work by just having ugly Feign configuration. But I know every time I use it, I'll be asking "what in the heck were they thinking?" :-)

Submitted #3440 to revert all changes from #3414.

I’m now thinking that we’d better enforce the complete Content-Disposition header declaration, including the name parameter. Otherwise, trying to parse a half-baked Content-Disposition header will be an implementation nightmare. Feign doesn’t have a dedicated HTTP header parser. We’ll have to create one if we want to insert the name parameter into Content-Disposition. Apart from that, what should we do if the header already has a name parameter? Implementing an HTTP header parser is like creating a library within a library, considering the complexity of RFC 7578.

public interface Client {
    @RequestLine("POST /")
    void sendFile(
        @Multipart(
            name = "file", // mandatory
            headers = {
                // Should we replace or keep "another-name"?
                //
                // Replacing it is very complicated. header.split(";") won't work because
                // this is also a valid header:
                //
                // Content-Disposition: form-data; name="another;name"; filename="{filename}"
                "Content-Disposition: form-data; name=\"another-name\"; filename=\"{filename}\"",
                "Content-Type: application/xml"
            },
            unwrap = true
        )
        InputStream file,

        @Param("filename")
        String filename
    );
}

Having a new @Multipart annotation (BTW, I prefer @Part over @Multipart) in the feign-core module implies that we’ll modify feign.core.DefaultContract. This means that half of multipart form handling will already be baked into the feign-core module. I don’t feel that exposing the @Multipart annotation and its handling to users makes much sense without the complete feign-form module. As I already said in this thread, I think feign-form should’ve been part of feign-core in the first place. But considering our current reality, I would stick to, or at least try, reusing the existing @Param/@Headers annotations for describing parts rather than introducing a new @Multipart annotation.

I don’t insist on any of the points above; I just want you to take them into consideration.

Thanks for continuing to work through the design here. The compatibility boundary I need to hold for the current line is source/binary compatibility: existing Encoder, Request, RequestTemplate, logging behavior, and null-body behavior all need to keep working for existing users. So we myst target 14.x.

If request streaming requires changing Encoder.encode(...), replacing existing body abstractions, or changing current annotation semantics, that part belongs in a major-release design rather than a minor. For a near-term PR, please keep the shape additive and opt-in: new body/part/multipart APIs or helpers, no behavior change for existing clients, and tests that cover client implementations, logging, retry/non-repeatable streams, and multipart edge cases.

A focused PR that demonstrates that compatible path is welcome. If you would rather sponsor or commission the design/implementation work, I am available through GitHub Sponsors: https://github.com/sponsors/velo.

MultiEncoder implementation that has a list of registered Encoders

more like a Map content-type: enconders (or Accept: encoders I can never say it correctly without looking)

similar for decoder....

So we create feign client with multiple encoders, and each method can be precise about which encoder using by settings content-type

At decode time, we do the same, but based on the server response...

@velo

I’m now thinking that we’d better enforce the complete Content-Disposition header declaration

I am completely in agreement on this. Using template strings with the bean getter syntax solves the problem and is much more friendly for the user.

This is why I proposed this:

@RequestLine(...)
public UploadResult upload(@Multipart(name="myFile", filename="{fileName}") Path myFile);

@RequestLine(...)
public UploadResult upload(@Multipart(name="myFile", filename="{fileName}") InputStream myStream, @Param("fileName") String fileName);

*Note that in the first example, the {fileName} syntax ultimately resolves to a call go myFile.getFileName(). This completely side-steps the problem of having the encoder (or dedicated part handler, or whatever) being responsible for surfacing the filename somehow.

@velo please note that we are already in the middle of a breaking change (major release RC) for streaming, so if we do anything with changing the method signature of encode(), we are talking about doing that in the context of the current RC (including additional notes in the migration documentation, etc...). Let's see how the discussion plays out, though - I'm not entirely convinced that adding a return type to encode() is the right path - just throwing ideas around.

re: the idea of registering encoders using content-type: This would certainly allow us to do conditional encoding (though I prefer @yvasyliev approach of using a predicate better than hard coding to content type).

That said, I believe that this place a pretty big burden on the user when they configure Feign - but let's try it out:

  Extractor myExtractor = MultiExtractor
   .forContentType("application/xml", new XmlEncoder())
   .forContentType("application/octet-stream", new InputStreamEncoder())
   .forContentType("application/vnd.openxmlformats-officedocument.spreadsheetml.sheet", new FileEncoder())
   .forContentType("application/vnd.openxmlformats-officedocument.wordprocessingml.document", new FileEncoder())
   ... // more lines for every type of content type we may encounter
   .withDefault(new JacksonEncoder())

You'll see that every flavor of file that we might upload now needs to have a registration. We could certainly address some of this with more clever predicates that look for all the content types that could possible imply encoding of File, Path or InputStream - but I don't think any of us want to maintain that list - and I'm sure the user doesn't want to do that either!

Plus (and I think this is a big deal), I think that the above would create serious obstacles to having the content-type header being determined/set automatically by some encoders.

We also have encoders that are already designed around the constructor based delegate pattern - those will definitely not work nicely with the above - so we are going to have to change the behavior of those (which is going to break the constructor method signatures).

Long and short: I have no issue with predicate based encoders (including content type like my example above) - I just do not think that they can solve the problem. without making Feign configuration just as painful as the nested constructor/delegate approach that I'm trying to solve.

@headers

Having a new @multipart annotation (BTW, I prefer @part over @multipart) in the feign-core module implies that we’ll modify feign.core.DefaultContract. This means that half of multipart form handling will already be baked into the feign-core module. I don’t feel that exposing the @multipart annotation and its handling to users makes much sense without the complete feign-form module. As I already said in this thread, I think feign-form should’ve been part of feign-core in the first place. But considering our current reality, I would stick to, or at least try, reusing the existing @Param/@headers annotations for describing parts rather than introducing a new @multipart annotation.

These are good points.

The alternative argument is that we already have multipart handling in feign-core - it is just very limited and is forcing non-optimal compromises downstream in the architecture:

DefaultContract line 118 pushes form-specific parameters into the method metadata
Also see RequestTemplateFactoryResolver.BuildFormEncodedTemplateFromArgs

I was originally opposed to having multi-part handling in feign-core - but your discussion around this makes sense. And once I realized that multi-part handling was already in feign-core (and the reasons why), I have changed my stance. If we could achieve the same general result without doing things in feign-core, then I would say "absolutely we should not mess with feign-core". But we've explored a bunch of ways to address this purely in the encoding layer, and every one of them results in a poor user experience. If all we are going to continue to be limited by the current approach, then we are probably better off just rewriting the existing multipart code to stream (and not use encoders at all).

I think that your proposal of using encoders for multi-part is very compelling, and I fully support this - even if it means that we slightly increase the footprint of the breaking changes we made to support streaming.

The reality is that the correct way to do this requires working in the RequestTemplate resolution (which means feign-core). Trying to do this in the encoding layer creates all sorts of very messy compromises that impact the user experience. Whatever we do here, it must be neutral or better for the user.

I am fine with @Part vs @Multipart

I actually used @Part originally in my design but changed it because @Part and @Param are very similar to each other, and it would be easy for a user to confuse the two.

Another option would be @FormData (this maps more closely to the nomenclature of the section's content-disposition).

Let me know your thoughts - names matter a lot with these types of libraries, so I think it is productive to debate and explore different options.

I do not think that re-using @PARAM is a good idea. That annotation very clearly indicates "request parameters", which means URL encoded parameters (either in the request URI, or in a URL encoded body). I also think that we are going to need additional values (like filename) that aren't in @PARAM - and I don't want to add multi-part specific parameters to a general purpose annotation.

I am very ok with using @Headers on the method parameter instead of having it be a headers={...} value in the @Multipart annotation. Either way is fine, and I think either way will be intuitive to users.

From an implementation perspective, I don't think there will be much benefit to using @Headers vs @Part(headers={...}). I would suggest that we start with headers={...} - it will make things a little simpler up front and allow us to fully understand the details like template resolution. Then we can switch to @headers if we feel that is desirable.

Created a POC for a new @Part annotation based on the latest discussion via #3450. It declares the contract only; a dedicated Encoder is yet to be implemented. The PR is in draft state to avoid unnecessary merge. I changed some naming during the implementation, and I tried to collect the motivation in the PR description.

Based on discussion from the #3450, a new #3476 PR is opened to allow registering multiple encoders in Feign. This is a prerequisite for multipart bodies streaming feature.

Sunbelt Computer Software

PL/B Language Development and Support

Navigation Menu

Repository navigation

Add support for streaming on requests (and add better handling for streaming in responses) #2734

Problem

Requested solution

Status/Findings

Possible Approach

Conclusion

25 remaining items

Baseline

Syntactic Sugar via Annotations

Proposal

Feign Architecture Note:

Proposed annotation:

The problem with chain-of-responsibility design for Encoders

Problems with the above approach

To Be Figured Out

Proposed annotation:

Thoughts on Encoder

Possible solutions

I think we may be looking at another breaking change

Proposed breaking change

What I'm not suggesting

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Sunbelt Computer Software

PL/B Language Development and Support

Add support for streaming on requests (and add better handling for streaming in responses) #2734

Description

Problem

Requested solution

Status/Findings

Possible Approach

Conclusion

Activity

kdavisk6 commented on Jan 20, 2025

trumpetinc commented on Jan 20, 2025

tdsnxgong-rick commented on Jan 22, 2025

trumpetinc commented on Jan 31, 2025

trumpetinc commented on Feb 1, 2025

trumpetinc commented on Feb 3, 2025

trumpetinc commented on Feb 4, 2025

sdoeringNew commented on May 26, 2025

tdsnxgong-rick commented on May 30, 2025

yvasyliev commented on Apr 18, 2026

trumpetinc commented on Apr 18, 2026

yvasyliev commented on May 18, 2026

trumpetinc commented on May 18, 2026

25 remaining items

trumpetinc commented on Jun 21, 2026

Baseline

Syntactic Sugar via Annotations

Proposal

Feign Architecture Note:

Proposed annotation:

The problem with chain-of-responsibility design for Encoders

Problems with the above approach

To Be Figured Out

yvasyliev commented on Jun 22, 2026

Proposed annotation:

trumpetinc commented on Jun 22, 2026

yvasyliev commented on Jun 22, 2026

trumpetinc commented on Jun 22, 2026

trumpetinc commented on Jun 22, 2026

Thoughts on Encoder

Possible solutions

I think we may be looking at another breaking change

Proposed breaking change

What I'm not suggesting

yvasyliev commented on Jun 23, 2026

velo commented on Jun 23, 2026

velo commented on Jun 23, 2026

trumpetinc commented on Jun 23, 2026

trumpetinc commented on Jun 23, 2026

yvasyliev commented on Jun 24, 2026

yvasyliev commented on Jul 15, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions