Skip to content

Adds support for optional request body Fixes gh-126 #263

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.

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

Closed
wants to merge 7 commits into from
Closed

Adds support for optional request body Fixes gh-126 #263

wants to merge 7 commits into from

Conversation

matt62king
Copy link
Contributor

Adds support for @RequestBody(required = false)

When request body is optional and null, encoder will add an empty body by default.

Fixes gh-126

@codecov
Copy link

codecov bot commented Dec 12, 2019

Codecov Report

Merging #263 into master will decrease coverage by 1.93%.
The diff coverage is 85.71%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #263      +/-   ##
============================================
- Coverage     79.09%   77.15%   -1.94%     
- Complexity      351      373      +22     
============================================
  Files            42       51       +9     
  Lines          1320     1497     +177     
  Branches        202      219      +17     
============================================
+ Hits           1044     1155     +111     
- Misses          190      246      +56     
- Partials         86       96      +10     
Impacted Files Coverage Δ Complexity Δ
...amework/cloud/openfeign/support/SpringEncoder.java 78.33% <50.00%> (-11.95%) 12.00 <0.00> (-6.00)
...eign/annotation/RequestBodyParameterProcessor.java 90.90% <90.90%> (ø) 5.00 <5.00> (?)
...ork/cloud/openfeign/support/SpringMvcContract.java 82.18% <100.00%> (-6.65%) 47.00 <0.00> (-9.00)
...ringframework/cloud/openfeign/DefaultTargeter.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-2.00%)
...ork/cloud/openfeign/support/PageJacksonModule.java 37.50% <0.00%> (-16.67%) 4.00% <0.00%> (ø%)
...mework/cloud/openfeign/FeignAutoConfiguration.java 88.88% <0.00%> (-3.42%) 3.00% <0.00%> (ø%)
...mework/cloud/openfeign/FeignClientFactoryBean.java 67.64% <0.00%> (-1.51%) 46.00% <0.00%> (-3.00%)
...amework/cloud/openfeign/FeignClientProperties.java 57.97% <0.00%> (-0.94%) 7.00% <0.00%> (ø%)
... and 22 more

@@ -147,6 +150,11 @@ else if (messageConverter instanceof ProtobufHttpMessageConverter
}
throw new EncodeException(message);
}
else {
if (OPTIONAL_REQUEST_BODY.equals(request.bodyTemplate())) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way to do this without a fake template body?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not one that I can see at the request level.

I think it could be done ate the Client level, not sure if we would want to go that broad for this.

Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek Apr 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The empty template is sort of a workaround, indeed. If that was the only option, I would probably still like to go with this, however, this PR was closed for lack of tests and feedback and not because the functionality proposal was rejected. If that gets merged, we could then reuse it on Contract level, without the workaround. @matt62king let me know if you would like to try amending that OpenFeign PR and getting it merged? If not, I can work on it with the OpenFeign team.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@OlgaMaciaszek I don't think I will be working the OpenFeign PR. I have not worked with that project before.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok @matt62king . Will try to submit sth like that to them.

@OlgaMaciaszek
Copy link
Collaborator

Have created another issue in OpenFeign: OpenFeign/feign#1383.

@matt62king matt62king closed this by deleting the head repository Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optional request body should be supported by feign
4 participants