Return response body in FeignException from errorReading (#2618)#3415
Return response body in FeignException from errorReading (#2618)#3415seonwooj0810 wants to merge 1 commit into
Conversation
velo
left a comment
There was a problem hiding this comment.
The response-body/header fix in core/src/main/java/feign/FeignException.java has focused coverage in FeignExceptionTest, and I do not see a compatibility or security concern in the patch itself. I still cannot approve this head while the required ci/circleci: pr-build check is failing. Please get that check green, then I can re-review.
) FeignException.errorReading passed request.body() and request.headers() into the constructor's responseBody/responseHeaders parameters, so FeignException.responseBody()/contentUTF8()/responseHeaders() exposed the request data instead of the response when a read failure occurred while decoding a response. Read the response body (mirroring errorStatus, tolerating a streamed or already-consumed body) and pass the response headers instead. Signed-off-by: seonwoo_jung <79202163+seonwooj0810@users.noreply.github.com>
0da224c to
a8e3ac0
Compare
|
Thanks @velo. The failing Verified locally on the rebased branch: |
velo
left a comment
There was a problem hiding this comment.
The current FeignException.errorReading change has a focused test for preserving the response body and headers, and I do not see a compatibility or security concern in the diff itself. I still cannot approve this head because the required ci/circleci: pr-build check is failing. Please get that check green, then I can take another pass.
Fixes #2618
Problem
FeignException.errorReading(...)builds the exception by passingrequest.body()andrequest.headers()into the constructor parameters namedresponseBody/responseHeaders:As reported in #2618, this means that when an
IOExceptionis thrown while reading/decoding the response (seeInvocationContext.decodeandResponseHandler.handleResponse),FeignException.responseBody()/content()/contentUTF8()/responseHeaders()return the request body and headers instead of the response. The maintainer confirmed in the thread that this looks like a bug.Fix
Read the response body into a
byte[]and pass the response headers, mirroring the existingerrorStatus(...)method. Reading is wrapped in atry/catch (IOException ignored)so a streamed or already-consumed body degrades gracefully to an empty body rather than throwing (consistent witherrorStatus).Test evidence
Strengthened the existing
FeignExceptionTest.canCreateWithRequestAndResponseto assert the content: the exception now carries the response body ("response") rather than the request body ("data"), and exposes the response headers. Before the fix this assertion fails (contentUTF8()returned"data").Verification done: (1) no in-flight PR (
gh pr list --search "responseBody"/"errorReading"empty); (2) the only self-claim is a 2024-11-03 question that never produced a PR (>21 days stale); (3) code-focused change inFeignException.java+ test; (4) confirmed therequest.body()pattern still present on current master; (5) maintainer acknowledged the bug in-thread. Commit is DCO signed-off; google-java-format validation passes.