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

Create separate handler stereotype for RESTful web services [SPR-8406] #13053

Closed
spring-issuemaster opened this Issue Jun 6, 2011 · 15 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

spring-issuemaster commented Jun 6, 2011

Arjen Poutsma opened SPR-8406 and commented

The @Controller stereotype is tailored for a role within an MVC environment. As such, it makes certain assumptions that are not completely valid in a web services scenario (for instance, implicit view name resolution, and the ModelAndView concept).

We could add another stereotype (eg. @Resource) that suits a web service scenario better. For instance, it would not (implicitly) use the ModelAndView paradigm, and default to having @Request- and @ResponseBody on all methods.


Issue Links:

  • #13928 Spring MVC: Guidance on reporting un-handled errors as data when implementing a REST API
  • #13948 DefaultHandlerExceptionResolver doesn't handle BindException but does handled other data binding related exceptions
  • #12029 void method in MVC controller that is not annotated with @ResponseStatus and does not take a HttpServletResponse as parameter results in misleading 404 ("supersedes")

12 votes, 15 watchers

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Jan 11, 2012

Rossen Stoyanchev commented

One example of where a REST controller stereotype would be useful is not allowing exceptions to propagate to the servlet container and as a result be presented as an HTML-formatted stack trace. A 500 error with no body would be the preferred default response.

Also a BindException currently results in a 500 but could be sent back as a 400 error when the handler is a web service controller (vs the error page, generic or not, that would be expected for browser form submissions).

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented May 11, 2012

Eugen Paraschiv commented

Better REST support makes perfect sense; however, a completely separate annotation seems somewhat of an overkill - the fact that the existing support was able to largely accommodate the RESTful architecture is a good thing, so it would make more sense to improve on that than replace it.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented May 11, 2012

Rossen Stoyanchev commented

The original intent was to avoid having to have @RequestBody and @ResponseBody annotations everywhere and assume those by default. It is true however that the use of @ResponseBody is somewhat orthogonal and is not required for writing XML, JSON, etc.

Nevertheless there are other cases where we could make better assumptions or choose different defaults. For example automatic handling for BindException or sending error codes in the body of the response instead of the HttpServletResponse.sendError which sends an error wrapped in HTML.

I'll leave this open for now and we'll see if it's needed when we address the related issues. Further discussion and comments welcome!

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Jul 30, 2012

Adib Saikali commented

I am trying to do something that I think should be very simple, but it is turning out to very difficult with the current SpringMVC architecture.

For my rest web service what i want to be able to control the body of the error response, I am trying to implement the suggestions at http://blog.apigee.com/detail/restful_api_design_what_about_errors/ so whenever there is a problem I want a chance to generate my standard RestError object as the that best practice suggests. This blog post http://www.stormpath.com/blog/spring-mvc-rest-exception-handling-best-practices-part-2 shows an example of how to do with with an ExceptionResolver.

The problem is that I am having to figure out every type of exception that Spring can throw, catch it in my resolver and then generate my error message, essentially I have to replicate the code in DefaultHandlerExceptionResolver.doResolve this is deeply unsatisfactory because who knows what new types of exceptions will be added in spring 3.2 and so keeping my exception resolver in sync with the spring one will be a challenge.

So I think having a @Resource is much more natural way to code a restful service if there was a clear way to allow the code to return an alternative response body after spring hand a chance to determine the http response code. I should not have to read and understand every exception type and when it is thrown to get custom error handling.

Lest say we had annotation something like.

@ResponseError
public @ResponseBody RestError errorHandler(HttpStatus status, Exception e, ... other params as need that would have been sent to @RequestMapping)
{
// write code here to generate a custom response body.
}

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Aug 21, 2012

Rossen Stoyanchev commented

Adib, thanks for the comments. There is now an ResponseEntityExceptionHandler base class that is functionally equivalent to the DefaultHandlerExceptionResolver but allows for preparing error content to be written to the body of the response. See 1cf4a2facd4fa0784fd10a767114011a79164abd.

This class is not a HandlerExceptionResolver per se. Rather the intent is that sub-classes have the new @ControllerAdvice annotation, which can make @ExceptionHandler methods applicable globally. See e65b930e7ad63b909bd2977bff806322477f8a91 for more details. Also keep in mind the ability to customize the default Servlet container error page.

I think the above referenced changes should help improve the ability to report error content.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Aug 27, 2012

Rossen Stoyanchev commented

ExceptionHandlerSupport has been renamed to ResponseEntityExceptionHandler emphasizing the fact it returns a ResponseEntity (rather than a ModelAndView) and relies on message converters.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Nov 24, 2012

Michael Osipov commented

Rossen, is there a guarantee that the status code issued by a ResponeEntity is set with response.setStatus and not response.setError?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Nov 26, 2012

Michael Osipov commented

Rossen, another issue. The @ControllerAdvice attaches to all controllers in the scanned base package. What if I have ViewControllers and ResponseEntityController in the same base package? It would handle both and return a ResponseEntity. Wouldn't it? If so, the annotation should take a list of Controller classes for which it does generic handling.

In general, I'd like to avoid to setup two DispatcherServlets, one for Views and one for ResponseEntities.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Nov 26, 2012

Rossen Stoyanchev commented

By ViewControllers you mean annotated controllers that rely on view resolution by returning a view name as opposed to writing directly to the response through a ResponseEntity?

What do you today if an exception is raised from a method returning a ResponseEntity as opposed to a view name? Do you have two DispatcherServlet's each with a different exception handling strategy? Keep in mind that ExceptionHandlerExceptionResolver, which is what supports @ExceptionHandler methods has a setMappedHandlerClasses property that can be used to specify which controllers it applies to.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Nov 26, 2012

Michael Osipov commented

By ViewControllers you mean annotated controllers that rely on view resolution by returning a view name as opposed to writing directly to the response through a ResponseEntity?

Exactly, ViewControllers return a view name string and RestControllers ResponseEntities.

What do you today if an exception is raised from a method returning a ResponseEntity as opposed to a view name? Do you have two DispatcherServlet's each with a different exception handling strategy?

I (would) use @ExceptionHandler and return a ResponseEntity with an appropriate return code and an optional body when the state needs further explanation. If this would be an exception from a view controller, I'd rather do forward:/errors/4xx or better yet call response.setError(4xx,"..."). The problem yet is that I do not know where an exception really came from. I do use one DispatcherServlet at the moment but I am thinking about having two of them one for / and one for /rest with two exception handling strategies. The big problem is that you cannot catch exceptions globally and handle them appropriately with one DispatcherServlet.
Furthermore, I haven't come up with a better approach yet to gain full control over the seperated controller types. Unfortunately, I am not the only one with this problem as you can see in the many JIRA tickets and stackoverflow.

Keep in mind that ExceptionHandlerExceptionResolver, which is what supports @ExceptionHandler methods has a setMappedHandlerClasses property that can be used to specify which controllers it applies to.

Thanks, I was not aware of that. How can I effectively utilize that to seperate my stuff? Declare two ExceptionHandlerExceptionResolvers? I am missing the big picture at the moment. With the release of ResponseEntityExceptionHandler there should be a chapter in the Spring docs about the separation.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Nov 26, 2012

Rossen Stoyanchev commented

You can have any number of HandlerExceptionResolver instances. For view controllers, it's also common to use SimpleMappingExceptionResolver.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Nov 26, 2012

Michael Osipov commented

OK, that feasible through the DispatcherServlet config. If I do register two handlers where each handles View and ResponseEntity and I do throw a NoSuchRequestHandlingMethodException from a ViewController and a ResponseEntityController how am I supposed to detect that in my HandlerExceptionResolver. Obviously that part of the Spring documenation does not reveal this secret to me. Again, I miss the big picture here.
The big problem with most HandlerExceptionResolvers is that they use sendError and the container must contruct an error page as per Servlet spec. This is unwanted in REST API. This is what I am struggling with.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Nov 27, 2012

Rossen Stoyanchev commented

how am I supposed to detect that in my HandlerExceptionResolver.

That's where the setMappedHandlers property I mentioned above comes in.

The big problem with most HandlerExceptionResolvers is that they use sendError and the container must contruct an error page as per Servlet spec.

There is not much we can do about that. Even the response status is set to a 4xx or 5xx status (instead of sendError), without a body, Servlet containers can still send an HTML error page. You can customize the default servlet container error page or make sure the response body has some error content.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Nov 27, 2012

Rossen Stoyanchev commented

Resolving as "Won't Fix" as simply introducing a new stereotype in itself doesn't provide sufficient benefit. We can instead focus on making concrete improvements in support of applications built for humans vs for an automated client as we did with the introduction of a ResponseEntityExceptionHandler, global @ExceptionHandler methods, the ContentNegotiationStrategy and ContentNeogtiationManager, and others in Spring Framework 3.2.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Nov 28, 2012

Michael Osipov commented

bq. How am I supposed to detect that in my HandlerExceptionResolver?

That's where the setMappedHandlers property I mentioned above comes in.

As far as I can see, that method is located in the AbstractHandlerExceptionResolver but the argument is not generic, so what input types are allowed.

Eventhough, if this one accepts controllers, I would not be able to handle everything under a path like /rest unless I define a fake RestControlller, I guess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment