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

DefaultResponseErrorHandler wastes the body of a response with an unknown status [SPR-16604] #21145

Closed
spring-issuemaster opened this issue Mar 16, 2018 · 3 comments
Assignees
Milestone

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Mar 16, 2018

Milos Cubrilo opened SPR-16604 and commented

RestTemplate.handleResponse calls ResponseErrorHandler.hasError(response) to check if it should delegate error handling to ResponseErrorHandler instance.

In case of an unknown status code, DefaultResponseErrorHandler.hasError swallows the exception thrown by DefaultResponseErrorHandler.getHttpStatusCode(response) and returns false.

This causes RestTemplate code to proceed with standard response handling, but without possibility to read message body, which has already been consumed during UnknownHttpStatusCodeException creation in DefaultResponseErrorHandler.getHttpStatusCode(response) call.


Affects: 4.3.14

Issue Links:

  • #20529 RestTemplate doesn't consistently tolerate unknown HTTP status codes
  • #20656 DefaultResponseErrorHandler.hasError doesn't tolerate unknown status codes
  • #19892 DefaultResponseErrorHandler should have its methods protected
  • #21993 DefaultResponseErrorHandler does not handle status values outside of HttpStatus enum
  • #21971 RestTemplate does not throw exception for custom error codes

Referenced from: pull request #1742, and commits 3b3f27d, d95bbb6

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Mar 16, 2018

Juergen Hoeller commented

Indeed, for hasError purposes, we should locally resolve the status code and catch the IllegalArgumentException instead. I'll revise this for 4.3.15.

Note that this has been resolved for 5.0 already where HTTP status code resolution has been streamlined in several places.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Mar 16, 2018

Denys Ivano commented

I also faced difficulties with unknown status codes. We have some logic based on catching and processing RestClientResponseException, especially it's subclass - UnknownHttpStatusCodeException. But in 4.3.12 new approach was introduced, so we can't even get response data w/o providing custom ResponseErrorHandler. Maybe it would be better to make hasError() method work the same way as it works in Spring Framework 5. I've submitted pull request which is able to resolve this issue. I hope this will help.
#1742

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Mar 28, 2018

Juergen Hoeller commented

Resolved through direct enum value comparisons in hasError (without an intermediate exception and therefore as analogous as possible to the 5.0 code).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.