Support different custom configurations for multiple feign clients with the same name

[menelaosbgr]

I have an issue that is related with: spring-cloud/spring-cloud-netflix#1211

We have a service accepts multi-part upload requests for one method.
We had another method that was accepting a JSON object ( request body).

@FeignClient(name = "ab-document-store", fallback = DocumentStoreFallback.class, configuration = MultiPartSupportConfiguration.class)
public interface DocumentStoreClient {

	@Headers("Content-Type:multipart/form-data")
	@RequestMapping(path = "/document-store/v1/upload", method = RequestMethod.POST) // , consumes = MediaType.MULTIPART_FORM_DATA_VALUE, produces = MediaType.MULTIPART_FORM_DATA_VALUE)
	DocumentMetaData uploadDocument(@RequestPart("files") @Param("files") MultipartFile files);

//other methods here
		
	@RequestMapping(path = "/document-store/v1/save/as/zip")
	DocumentMetaData saveDocumentsZip(@RequestBody ZipFileModel zipFileModel);

For multi-part to work, we were using a configuration class and an interceptor:

public class MultiPartSupportConfiguration {

	@Autowired
	private ObjectFactory<HttpMessageConverters> messageConverters;

	@Bean
	public Encoder feignFormEncoder() {
		return new MultipartFormEncoder(new SpringEncoder(messageConverters));
	}

	@Bean
	public Logger.Level feignLoggerLevel() {
		return Logger.Level.HEADERS;
	}
}

public class MultipartFormEncoder extends SpringFormEncoder {

	public MultipartFormEncoder(Encoder delegate) {
		super(delegate);
	}

	@Override
	public void encode(Object object, Type bodyType, RequestTemplate template) throws EncodeException {
		if(bodyType.equals(MultipartFile.class)) {
			template.header("Content-Type", "multipart/form-data");
			super.encode(object, bodyType, template);
			return;
		}
		super.encode(object, bodyType, template);
		
	}
}

But, we we are seeing a problem. Spring ( or feign) is forcing requests to go through "ReflectiveFeign$BuildFormEncodedTemplateFromArgs".
see: https://github.com/OpenFeign/feign/blob/master/core/src/main/java/feign/ReflectiveFeign.java#L352 ).

This is removing all variables except request params ( formVariables).

So I tried to split my problem into two Feign clients with two different configuration ( for the same micro-service). And obviously this did not work. Is there a solution using the spring-cloud feign solution?

Being able to do by-method configuration would work. But also being able to make two feign different feign clients for the same service. The main architect does not want us to use openfeign in our custom starter.

Our current work-around was to limit all our input into form compatible variables (puke) .

@FeignClient(name = "ab-document-store", fallback = DocumentStoreFallback.class, configuration = MultiPartSupportConfiguration.class)
public interface DocumentStoreClient {

	@RequestMapping(path = "/document-store/v1/upload", method = RequestMethod.POST) // , consumes = MediaType.MULTIPART_FORM_DATA_VALUE, produces = MediaType.MULTIPART_FORM_DATA_VALUE)
	DocumentMetaData uploadDocument(@RequestPart("files") @Param("files") MultipartFile files);
	
	@RequestMapping(path = "/document-store/v1/save/as/zip" , method = RequestMethod.POST)
	DocumentMetaData saveDocumentsZip(@RequestParam(value="filePaths[]") String[] filePaths,
									  @RequestParam(value="fileIds[]") String[] fileIds,
									  @RequestParam(value="resultFileName") String resultFileName);
}

[ryanjbaxter]

Do you just want the MultiPartSupportConfiguration to apply to the uploadDocument method in your Feign client?

[menelaosbgr]

@ryanjbaxter Yes! Only the uploadDocuments. I want the other methods to be non multiPart.

[ryanjbaxter]

Right now the only method we have to do that is what is described in spring-cloud/spring-cloud-netflix#1211.

[menelaosbgr]

1. @ryanjbaxter Are you referring to different RequestInterceptors? So your post @ spring-cloud/spring-cloud-netflix#1211 (comment) ?

Could I put a SpringFormEncoder in the spot of a Interceptor?

2. Or are you referring to using a feignBuilder? Is this the only solution?
   Should I just go and use the feignBuilder directly as you did in the example @ spring-cloud/spring-cloud-netflix#1211 (comment) ??

[ryanjbaxter]

Yes 2. This is the only solution I can think of.

[boristhuy]

@ryanjbaxter Would you still consider the possibility of having multiple clients with the same name (by which I mean serviceId?)

If "undepreciating" serviceId attribute in @FeignClient is not an option, the url attribute could be used instead to decide whether it should be a load balanced feign client or not. For example, by using the lb:// prefix like it is done in some other Spring Cloud modules (e.g. Spring Cloud Gateway).

This would allow us to create different Feign clients for the same service, and keep the name attribute as the unique identifier of a Feign client.

[ryanjbaxter]

@boristhuy we are open to other solutions, if you want to submit a PR to make it possible that would be welcomed.

[Ph0ndragX]

I implemented fix with serviceId propsed by @boristhuy. Now, it is possible to declare multiple feign clients with different names, but the same serviceId. When serviceId is present then it will be used by ribbon for load balancing, instead of default name/value. This lets us define feign clients with different configurations, but still with the same service id.

[mattwmj]

+1 for feign client with same serviceId but different names. That gives more flexibility to organize services.

[spencergibb]

name is the key into the child application context unique to the feign client and the service id if url is not used.
url is optional and it is the switch to not use loadbalancing.

What could be added is a name for the child application context that isn't the service id, something like contextId.

[Ph0ndragX]

Ok, so the problem isn't that we cannot define feign clients with different names, since the name by default is the classname of the annotated class and the alias can be overriden by qualifier element of the FeignClient annotation. What we cannot do is to override the name of the feign client configuration bean that is registered along the feign client because it uses name of the feign client as its name.

So we could add another element like contextId that would be used as an alias for feign client and name for the feign client configuration bean. Other possibility is that we would use that qualifier also as the name of the feign client configuration bean.

I guess the second option could still break some use cases, namely if someone was using qualifier element and expecting the name of the feign client configuration bean to be equal to the FeignClient name.

[spencergibb]

I'd rather not reuse anything. Based on feedback from the team, we'll review a PR, but it would have to convince us that the cost of maintaining the new functionality outweighs the few lines of code to manaually create a feign client as documented.

[Ph0ndragX]

So that #92 would be probably the simplest possible fix without neither reusing any attributes, nor creating new ones.

[spencergibb]

except that it breaks tests.

[Ph0ndragX]

That is because it still changes the name of the feign client configuration bean and that is something that I guess we cannot avoid in order to fix this and similar issues. I can fix tests by changing the configuration bean names in them. But if this solution is not appropriate I don't see any other possible options that would not break any use cases and would not clutter api with additional attributes that would have to be maintained. In which case we could probably close this issue with the known workarond of creating Fegin clients manually as documented.

Edit:
I guess for referenced pull request to work there is at least one other place where that feign configuration name must be taken into account, so it might not be that simple change but still solution would be without reusing any attributes.

[spencergibb]

I suggested a new contextId attribute.

[Ph0ndragX]

I commited version with contextId