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

Exception hierarchy under HttpClientException and HttpServerException for the RestTemplate [SPR-15404] #19967

Closed
spring-projects-issues opened this issue Mar 30, 2017 · 6 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

spring-projects-issues commented Mar 30, 2017

Jerzy Krolak opened SPR-15404 and commented

The default RestTemplate error handler throws HttpClientErrorException or HttpServerErrorException, which are quite general, compared to the variety of HTTP error codes and their REST semantics.

In a heavily rest-based application, introducing subclasses of HttpClientErrorException and HttpServerErrorException could simplify quite a lot of my code.

Sample code, actual:

try {
    Book book = restTemplate.getForObject("http://example.com/books/1", Book.class);
    return book;
} catch (HttpClientErrorException e) {
    if (e.getStatusCode() == HttpStatus.NOT_FOUND) {
        // handle the 404 error
    }
    throw e; // can't do anything about other http errors, anyway
}

Sample code, possible:

try {
    Book book = restTemplate.getForObject("http://example.com/books/1", Book.class);
    return book;
} catch (HttpNotFoundErrorException e) {
        // handle the 404 error
}

This change should be pretty easy to introduce, and by subclassing HttpClientErrorException and HttpServerErrorException should be backward compatible.

I could prepare an appropriate pull request, if you like the general idea.


Affects: 5.0.8

Issue Links:

Referenced from: commits 7f0e348, 2216964

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Jul 2, 2018

Jerzy Krolak commented

I know it's been over a year, but I still see a point in implementing this feature.

I've written a piece of code to illustrate my idea - maybe you could have a look at that: https://github.com/jerzykrlk/spring-framework/pull/1/files

I can polish it up later.

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Jul 2, 2018

Rossen Stoyanchev commented

On a first glance it seems okay to me. Arjen Poutsma or Juergen Hoeller?

The implementation creates a package cycle where .client depends on .client.response and vice-versa. A sub-class of DefaultResponseErrorHandler together with the exceptions could break the cycle. Or the exceptions would move up but they're too many.

Another idea is to have them nested in either HttpClientErrorException or HttpServerErrorException with a slightly abbreviated name so it becomes:

} catch (HttpClientErrorException.BadRequest ex) {
    // handle the 400
}

or:

} catch (HttpClientErrorException.BadRequest400 ex) {
    // handle the 400
}

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Jul 3, 2018

Arjen Poutsma commented

Looks good to me as well. I like Rossen's idea of nesting the exceptions in their "parent", as I don't think we should introduce so many new public types (and files!) this late in RestTemplate's lifecycle.

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Jul 3, 2018

Jerzy Krolak commented

Thanks for your feedback - I'll update the code tomorrow. Meanwhile, a few more questions:

  • I added just most common HTTP errors. What other exceptions you'd like to see?
  • what javadoc / constructors should I add - same as in HttpClientErrorException?

and one more general:

  • have you considered adding a preview of the response body in the exception message? So that, instead of this:
org.springframework.web.client.HttpClientErrorException.BadRequest: 400 Bad Request
    at org.springframework.web.client.DefaultResponseErrorHandler.handleError(DefaultResponseErrorHandler.java:91) ~[spring-web-4.2.4.RELEASE.jar:4.2.4.RELEASE]
    at org.springframework.web.client.RestTemplate.handleResponse(RestTemplate.java:641) ~[spring-web-4.2.4.RELEASE.jar:4.2.4.RELEASE]

... I could see this:

org.springframework.web.client.HttpClientErrorException.BadRequest: 400 Bad Request [<html><head><title>page not found</title>...]
    at org.springframework.web.client.DefaultResponseErrorHandler.handleError(DefaultResponseErrorHandler.java:91) ~[spring-web-4.2.4.RELEASE.jar:4.2.4.RELEASE]
    at org.springframework.web.client.RestTemplate.handleResponse(RestTemplate.java:641) ~[spring-web-4.2.4.RELEASE.jar:4.2.4.RELEASE]

or even nicer...

org.springframework.web.client.HttpClientErrorException.Conflict: 409 Conflict [{
"error": {
 "errors": [
  {
   "domain": "global",
   "reason": "conflict",
   "message": "You already own this bucket. Please select another name."
  }
 ],
 "code": 409,
 "message": "You already own this bucket. Please select another name."
 }
}]
    at org.springframework.web.client.DefaultResponseErrorHandler.handleError(DefaultResponseErrorHandler.java:91) ~[spring-web-4.2.4.RELEASE.jar:4.2.4.RELEASE]
    at org.springframework.web.client.RestTemplate.handleResponse(RestTemplate.java:641) ~[spring-web-4.2.4.RELEASE.jar:4.2.4.RELEASE]

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Jul 4, 2018

Jerzy Krolak commented

Hi. I've updated the code to static inner class exceptions - see https://github.com/jerzykrlk/spring-framework/pull/1/files - can you have a look now?

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Aug 6, 2018

Jerzy Krolak commented

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

2 participants