Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Optional request body should be supported by feign #126

Open
dimw opened this issue May 25, 2016 · 10 comments
Open

Optional request body should be supported by feign #126

dimw opened this issue May 25, 2016 · 10 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@dimw
Copy link

dimw commented May 25, 2016

In Spring MVC it is possible to annotate body parameter as optional using @RequestBody(required = false) parameter annotation, e.g.:

@FeignClient("foo-service")
public interface FooService {
    @RequestMapping(value = "/foo", method = RequestMethod.POST)
    void doFoo(@RequestBody(required = false) FilterDto filter);
}

However, Spring-Feign contract does not consider the optionality flag which leads to an IllegalStateException on proxy bootstrapping in consumer when calling something like:

fooService.doFoo(null);
java.lang.IllegalArgumentException: Body parameter 0 was null
    at feign.Util.checkArgument(Util.java:102)
    at feign.ReflectiveFeign$BuildEncodedTemplateFromArgs.resolve(ReflectiveFeign.java:323)
    at feign.ReflectiveFeign$BuildTemplateByResolvingArgs.create(ReflectiveFeign.java:213)
    at feign.SynchronousMethodHandler.invoke(SynchronousMethodHandler.java:72)
    at feign.ReflectiveFeign$FeignInvocationHandler.invoke(ReflectiveFeign.java:103)

It is expected that null can be passed as parameter since it is optional.

Using:

  • spring-cloud-starter-feign:1.1.0.RELEASE
  • feign-core:8.16.2
@daniellavoie
Copy link
Contributor

From the stack trace you should understand that this issue is a pure Feign problem. Spring Cloud Netflix facilitate Netflix OSS component integration into Spring. I would suggest you to open an issue on the feign project.

@dimw
Copy link
Author

dimw commented May 30, 2016

@daniellavoie, thanks for your feedback. Indeed, initially I was about to open this issue on the Feign project (because of the reasons admitted by you) but stack traces should not be always interpreted literally. Basically it seems not like a "a pure Feign problem" for me. Please correct me if I am wrong but:

  1. The Spring MVC-Feign contract alongside with annotation processors (both part of this project) are responsible for proper processing of the producer interface. Thus, they should process the @RequestBody annotation and translate the required flag to Feign metadata.
  2. Feign does not know anything about Spring MVC annotations (i.e. @RequestBody) so even it can see the annotations provided on the method, it cannot (and should not) understand semantics of these.

In fact, there might be currently no way in Feign to handle optional request bodies. However, this behavior should be at least documented in this project (because of the difference to behavior anticipated in Spring MVC). Further, it might be considered to create a certain RequestBodyParameterProcessor which catches the issue one level higher before letting Feign to throw semi-reasonable exception.

@daniellavoie
Copy link
Contributor

You are right. Looks like I had a bad understanding of Spring Cloud Feign. I'll try to have a look and see if I can come up with a trick to handle an optinal parameter. If you have any inspiration, feel free to give some help ;)

@dimw
Copy link
Author

dimw commented Jun 5, 2016

@spencergibb: Do you think you can drop a line and explain why this issue has been closed without any further comment?

@spencergibb
Copy link
Member

Sorry about that, it was a mistake.

@spencergibb spencergibb reopened this Jun 8, 2016
@spencergibb spencergibb changed the title Optional request body should be supported Optional request body should be supported by feign Oct 5, 2016
@realdammy
Copy link

Hello @spencergibb, any update on this issues?

@ryanjbaxter
Copy link
Contributor

@realdammy it is labeled as an enhancement but as far as I know no one is working on it at the moment. If you want to submit a PR for it that would be more than welcome.

@dyc87112
Copy link

@ryanjbaxter I try to fix this issue. But feign always check the body, so I have to wait this PR : OpenFeign/feign#583.

@Override
protected RequestTemplate resolve(Object[] argv, RequestTemplate mutable, Map<String, Object> variables) {
  Object body = argv[metadata.bodyIndex()];
  checkArgument(body != null, "Body parameter %s was null", metadata.bodyIndex());
  ...
  return super.resolve(argv, mutable, variables);
}

If this PR can be accept. I will create another PR for here.

@dimw Thanks for your analysis.

@sdhery
Copy link

sdhery commented Nov 25, 2017

Already have a solution?

@dyc87112
Copy link

@sdhery OpenFeign/feign#583 has not been accept，so you need compile feign by yourself，then write an RequestBodyParameterProcessor implements AnnotatedParameterProcessor interface to support this issue

@spencergibb spencergibb transferred this issue from spring-cloud/spring-cloud-netflix Jan 31, 2019
@spencergibb spencergibb added enhancement New feature or request help wanted Extra attention is needed labels Jan 31, 2019
@matt62king matt62king mentioned this issue Dec 12, 2019
Closed
@OlgaMaciaszek OlgaMaciaszek mentioned this issue May 12, 2020
Closed
@OlgaMaciaszek OlgaMaciaszek mentioned this issue Apr 16, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Adds support for optional request body Fixes gh-126
7 participants
@ryanjbaxter @spencergibb @dimw @dyc87112 @sdhery @daniellavoie @realdammy