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.

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

FeignException with content as separate field #642

Closed
VaverDariush opened this issue Feb 14, 2018 · 10 comments · Fixed by #769
Closed

FeignException with content as separate field #642

VaverDariush opened this issue Feb 14, 2018 · 10 comments · Fixed by #769
Labels
enhancement For recommending new capabilities help wanted Issues we need help with tackling

Comments

@VaverDariush
Copy link

FeignException includes returned content in message. While this is mostly OK (unless the returned content is very long), the exception could also provide a separate field to obtain the content itself for further parsing instead of having to parse the message for it.

@VaverDariush VaverDariush changed the title Exception with content as separate field FeignException with content as separate field Feb 14, 2018
@kdavisk6
Copy link
Member

kdavisk6 commented Mar 8, 2018

@scythepl this is an enhancement. Let's mark it as such.

@fdirlikli
Copy link

fdirlikli commented Mar 8, 2018

@scythepl, are you talking about parsing the response body from content: part of the FeignException message? If so this makes sense because passing the response body and the status upstream is a common and valid use case. Someone consuming the FeignClient shouldn't be handling parsing the feign specific message content. As @kdavisk6 states we can mark this as an enhancement and implement in an upcoming release.

@VaverDariush
Copy link
Author

@orwashere yes, that's exactly what I was talking about. Sorry if it wasn't clear. I can't really find an option to mark this issue in any way, maybe it's a permissions thing? I'm not a fluent github issue tracker user, I'm sorry.
The issue is more an enhancement than bug, though I daresay that obligatory inclusion of response content in logs is not a good thing, in some cases it will just be a bad practice (like large messages), in others a security risk (custom backend authentication messages).
Furthermore, getting body by parsing e.getMessage() string doesn't seem like a good practice either.

To sum up, here's what I think would be good:

  • include response as a separate field in FeignException or a dedicated subclass of it
  • reconsider putting response body in exception.getMessage()

@kdavisk6
Copy link
Member

kdavisk6 commented Mar 9, 2018

@scythepl If you feel particularly jazzed about this, why not attempt a PR? Checkout HACKING and experiment.

@kdavisk6 kdavisk6 added enhancement For recommending new capabilities help wanted Issues we need help with tackling labels Jul 26, 2018
@finke-ba
Copy link
Contributor

Hello, @kdavisk6 !
I'm very interested in this issue.
I see that the work on the pull request stopped for some reason. Can I somehow help with this?

@kdavisk6
Copy link
Member

Of course. Please read Contributing and Hacking for information on how to contribute.

@finke-ba
Copy link
Contributor

Ok. @kdavisk6, what should I do - continue with #664(I could create my own PR based on this PR with fixed review comments) or create fully new PR?

@finke-ba
Copy link
Contributor

Hello @kdavisk6 !
I finished with PR #769.
I'm looking forward to questions and comments.

@Ravis22
Copy link

Ravis22 commented Jul 12, 2019

any idea which version of spring-cloud-openfiegn includes this fix
or how can I include this as separate dependency without impacting spring-cloud-openfeign

@kdavisk6
Copy link
Member

This change was merged and release over a year ago. Using the most recent version of Spring Cloud should include this update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement For recommending new capabilities help wanted Issues we need help with tackling
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants