Skip to content

Optional request body should be supported by feign #126

New issue
New issue
Open
Open
Optional request body should be supported by feign#126
Labels
enhancementNew feature or requesthelp wantedExtra attention is needed
@dimw

Description

@dimw
dimw
opened on 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

Activity

daniellavoie

daniellavoie commented on May 29, 2016

@daniellavoie
daniellavoie
on May 29, 2016
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

dimw commented on May 30, 2016

@dimw
dimw
on May 30, 2016
Author

@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

daniellavoie commented on May 30, 2016

@daniellavoie
daniellavoie
on May 30, 2016
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 ;)

spencergibb
closed this as completedon Jun 3, 2016
dimw

dimw commented on Jun 5, 2016

@dimw
dimw
on Jun 5, 2016
Author

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

spencergibb

spencergibb commented on Jun 8, 2016

@spencergibb
spencergibb
on Jun 8, 2016
Member

Sorry about that, it was a mistake.

spencergibb
reopened this on Jun 8, 2016
spencergibb
changed the title [-]Optional request body should be supported[/-] [+]Optional request body should be supported by feign[/+] on Oct 5, 2016
realdammy

realdammy commented on May 11, 2017

@realdammy
realdammy
on May 11, 2017

Hello @spencergibb, any update on this issues?

ryanjbaxter

ryanjbaxter commented on May 11, 2017

@ryanjbaxter
ryanjbaxter
on May 11, 2017
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

dyc87112 commented on Aug 17, 2017

@dyc87112
dyc87112
on Aug 17, 2017

@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

sdhery commented on Nov 25, 2017

@sdhery
sdhery
on Nov 25, 2017

Already have a solution?

dyc87112

dyc87112 commented on Nov 27, 2017

@dyc87112
dyc87112
on Nov 27, 2017

@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
transferred this issue fromspring-cloud/spring-cloud-netflixon Jan 31, 2019
spencergibb
added
enhancementNew feature or request
help wantedExtra attention is needed
on Jan 31, 2019
matt62king
mentioned this on Dec 12, 2019
OlgaMaciaszek
mentioned this on May 12, 2020
OlgaMaciaszek
mentioned this on Apr 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or requesthelp wantedExtra attention is needed

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      Participants

      @ryanjbaxter@spencergibb@dimw@dyc87112@sdhery

      Issue actions
        Optional request body should be supported by feign · Issue #126 · spring-cloud/spring-cloud-openfeign