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 contains request- or response-body #920

Closed
robachmann opened this issue Mar 11, 2019 · 1 comment · Fixed by #1149
Closed

FeignException contains request- or response-body #920

robachmann opened this issue Mar 11, 2019 · 1 comment · Fixed by #1149
Labels
documentation Issues that require updates to our documentation

Comments

@robachmann
Copy link
Contributor

robachmann commented Mar 11, 2019

While working on #912 I came across an odd behaviour:

In errorStatus a FeignException is created with the response's body:

body = Util.toByteArray(response.body().asInputStream());

In errorReading a FeignException is created with the request's body (even though the response with its body would be at hand):

I think it's inconsistent and confusing for the user as he or she can't rely on the same data-source for all exceptions. On the other hand, I'm not sure whether it's intentional as there are unit-tests that check this behaviour, thus changing it could also be breaking-change for users:

assertThat(e.contentUTF8()).isEqualTo("Request body");

If we agree on returning the response-body, I'd gladly create a PR for it.

@kdavisk6
Copy link
Member

It is intentional. Feign Exception cannot depend on a Response being available when it is created or available since these exceptions can be thrown before and after a request. For example, when a request times out, a FeignException is created and there is no response. In order to provide additional context, we use the Request.

One thing we can do to clear up this potential confusion is to update the documentation and comment to code to make this intention clearer.

@kdavisk6 kdavisk6 added the documentation Issues that require updates to our documentation label Mar 17, 2019
kdavisk6 added a commit to kdavisk6/feign that referenced this issue Dec 30, 2019
Fixes OpenFeign#920

FeignException may contain the data from the response if the response is available and contains data.  However, the method `content` is ambiguious and does not reveal it's intent.  User's have expressed confusion as to if it is for the Request or the Response.

This change adds a new method `responseBody` to addresse this.  Use of content is now `@deprecated`.
kdavisk6 added a commit that referenced this issue Dec 31, 2019
Fixes #920

FeignException may contain the data from the response if the response is available and contains data.  However, the method `content` is ambiguious and does not reveal it's intent.  User's have expressed confusion as to if it is for the Request or the Response.

This change adds a new method `responseBody` to addresse this.  Use of content is now `@deprecated`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Issues that require updates to our documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants