8000 send empty response body for POST when no body parameter present by pnepywoda · Pull Request #271 · OpenFeign/feign · GitHub
[go: up one dir, main page]

Skip to content

send empty response body for POST when no body parameter present#271

Closed
pnepywoda wants to merge 3 commits intoOpenFeign:masterfrom
pnepywoda:empty-body
Closed

send empty response body for POST when no body parameter present#271
pnepywoda wants to merge 3 commits intoOpenFeign:masterfrom
pnepywoda:empty-body

Conversation

@pnepywoda
Copy link

@cloudbees-pull-request-builder

NetflixOSS » feign » feign-pull-requests #148 SUCCESS
This pull request looks good

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens with other clients? Will this work?

Copy link
Author

Choose a reason for hiding this comment

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

What other clients? You mean ones that don't use ReflectiveFeign? We only use the JAXRSContract so that's the targeted fix that I made here. I didn't test out other contracts.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean non-OkHttpClient, such as ribbon or apache http client.

Copy link
Author

Choose a reason for hiding this comment

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

Oh good point. My followup commit should fix it to only write an empty body for okhttp

@cloudbees-pull-request-builder

NetflixOSS » feign » feign-pull-requests #149 SUCCESS
This pull request looks good

@spencergibb
Copy link
Contributor

👍 cool, thanks!

@codefromthecrypt
Copy link
Contributor

Looks safe. Few things; one is a nit.

  • Why are we adding the encoder as a constructor param? It isn't used in
    this PR.
  • Add a test for Client.Default?
  • update CHANGES? Probably there are folks who can undo similar logic
    knowing this.
  • new byte[0] is less code format sensitive than { }

Otherwise, LGTM!

@codefromthecrypt
Copy link
Contributor

ping! I'd like to cut a release today, so if you're ready, you can be in it!

@cloudbees-pull-request-builder

NetflixOSS » feign » feign-pull-requests #150 FAILURE
Looks like there's a problem with this pull request

@pnepywoda
Copy link
Author

Huh this is strange, it says there's a conflict in CHANGELOG.md but I can't figure out why or what the merge conflict actually is. @adriancole any ideas?

@codefromthecrypt
Copy link
Contributor

you're behind upstream.. no big deal, I can rebase this on the way in. Thanks!

@codefromthecrypt codefromthecrypt added this to the 8.10.0 milestone Sep 14, 2015
@codefromthecrypt
Copy link
Contributor

in 8.10.0!

@uschi2000
Copy link

Awesome stuff, thanks Adrian & Paul!

pnepywoda added a commit to pnepywoda/feign that referenced this pull request Jan 26, 2016
pnepywoda added a commit to pnepywoda/feign that referenced this pull request Jan 26, 2016
codefromthecrypt pushed a commit that referenced this pull request Jan 27, 2016
velo pushed a commit that referenced this pull request Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

0