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

WebDataBinder type conversion failures should result in 400, not 500 [SPR-5622] #10293

Closed
spring-issuemaster opened this Issue Mar 26, 2009 · 7 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Collaborator

spring-issuemaster commented Mar 26, 2009

Arjen Poutsma opened SPR-5622 and commented

When data cannot be bound by a WebDataBinder, this results in a TypeMismatchException, and eventually a status code 500: internal server error. This should be a 400: Bad Request


Issue Links:

  • #10295 A default HandlerExceptionResolver that resolves standard Spring exceptions ("is superseded by")

Referenced from: commits 880eb9e

@spring-issuemaster

This comment has been minimized.

Collaborator

spring-issuemaster commented Mar 27, 2009

Grzegorz Borkowski commented

The response code should be definitively 400.
The problem is with response body. In many (or most) cases service will return also some description of the actual problem. If the service serves XML, it will probably have to be in XML. If it serves JSON, then in JSON form, if it serves HTML, ...etc.
So there should be some way to get access to the actual exception and let the programmer decide what to do with it. Perhaps instead of TypeMismatchException there should be some more specific Exception thrown (say PathVariableTypeMismatchException) so that it can be easily recognized and handled.
On the other hand, if programmer don't want to bother with it, then probably framework on its own should return code 400 with perhaps generic error message in plain text.
Those are only my ideas, left for discussion :)
On the other hand, similar problem relates to e.g. request body OXM or OJM mapping errors, so perhaps it is already solved.

@spring-issuemaster

This comment has been minimized.

Collaborator

spring-issuemaster commented Mar 27, 2009

Arjen Poutsma commented

We're going to have a DefaultHandlerExceptionResolver that is loaded by default (hence the name ;) ) and sets the status code to something in the 4xx range for all common Spring exceptions. See #10295.

Additionally, we're going to allow for controller methods to be annotated with an annotation (something like @ExceptionHandler) to be able to do custom error resolution there, including returning custom XML/JSON/whatever. See #9354.

Finally, we're going to allow you to annotate your own exceptions with @StatusCode, and specify the response code for your own business exceptions. Similar to @SoapFault in SWS. See #10296

I think these three options will give you enough possibilities of handling exceptions, while having sensible defaults.

@spring-issuemaster

This comment has been minimized.

Collaborator

spring-issuemaster commented Mar 28, 2009

Grzegorz Borkowski commented

This sounds really good to me (and I'm really impressed by so short implementation time! thanks)

@spring-issuemaster

This comment has been minimized.

Collaborator

spring-issuemaster commented Jul 23, 2009

Grzegorz Borkowski commented

There is one problem with current implementation (3.0 M3), not sure if it's related to this issue or should I create another one.
By default DispatcherServlet registers 3 exception handlers: DefaultHandlerExceptionResolver, ResponseStatusExceptionResolver and AnnotationMethodHandlerResolver. This is ok, as for simple cases I use ResponseStatusExceptionResolver (eg. I have EntityNotFoundException marked with @ResponseStatus(404) - really cool).
However I need to be able to process some exceptions (Spring exceptions, not mine) in more specific way: for example if @PathVariable conversion fails, I want to send more detailed information to the client instead of plain "Bad request". So it seems logical to me that I add one more ExceptionHandlerResolver to those three default ones. The JavaDoc for DispatcherServer sais "Additional HandlerExceptionResolvers can be added through the application context. HandlerExceptionResolver can be given any bean name (they are tested by type). " From this it seems logical that by declaring any bean of type HandlerExceptionResolver it will be added to the list of default handlers.
By it doesn't work. After declaring any exception resolver, the default ones are no longer used: so all @ResponseStatus-es etc no longer work. Looks like I have to manually declare all those three default handlers in the context. IMHO, it makes no sense, and the default handlers should stay there, as their don't do any harm. This would be also consisted with DispatcherServlet javadoc.

@spring-issuemaster

This comment has been minimized.

Collaborator

spring-issuemaster commented Jul 26, 2009

Arjen Poutsma commented

I think that it's best if we change the javadoc. In some cases, you might not want to have the defaults, and by adding them, we would make it impossible to replace them.

@spring-issuemaster

This comment has been minimized.

Collaborator

spring-issuemaster commented Jul 26, 2009

Arjen Poutsma commented

Reopening to change the javadoc.

@spring-issuemaster

This comment has been minimized.

Collaborator

spring-issuemaster commented Jul 27, 2009

Arjen Poutsma commented

Improved javadoc.

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