If a client sends a request with accept header not matching the endpoints produces MediaType: should the webservice respond with 406 not acceptable (eg when ConstraintValidationExceptions occur)? - this is the current implementation. In cause you would want to stick to it, the issue can be closed directly.
If not - my suggestion: wouldn't it be better if spring would expose the real underlying error eg as message body, while still preserving the 406?
Example to reproduce:
If my endpoint does not produce json or xml, but any other media type like text/plain, or text/event-stream, then request validations are return as HttpMediaTypeNotAcceptableException.
Which is wrong, they should at least return the real error somehow.
Example:
@RestController
@Validated
public class TestServlet {
@GetMapping(value = "/test", produces = MediaType.APPLICATION_JSON_VALUE)
public void test(@RequestParam @Valid @NotBlank String name) {
}
@GetMapping(value = "/test2", produces = MediaType.TEXT_PLAIN_VALUE)
public void test2(@RequestParam @Valid @NotBlank String name) {
}
}
If the first method is called: localhost:8080/test?name= with Accept:application/json:
Then the correct error response is shown:
{
"timestamp": "2019-08-06T12:07:12.186+0000",
"status": 500,
"error": "Internal Server Error",
"message": "test.name: must not be blank",
"path": "/test"
}
But for the 2nd method, the real validation error is hidden behind a 406 Not Acceptable:
localhost:8080/test2?name= with Accept:text/plain.
Returns 406 status code with empty message body.
I assume this is because spring tries to return the validation error as another media type that is not text/plain, and then fails.
I think even if the client requests an accept http header explicit, errors should still be getting exposed somehow!
Error in logs:
2019-08-06 14:08:31.414 ERROR 3933 --- [nio-8080-exec-8] o.a.c.c.C.[.[.[/].[dispatcherServlet] : Servlet.service() for servlet [dispatcherServlet] in context with path [] threw exception [Request processing failed; nested exception is javax.validation.ConstraintViolationException: test.name: must not be blank with root cause
javax.validation.ConstraintViolationException: test.name: must not be blank
at org.springframework.validation.beanvalidation.MethodValidationInterceptor.invoke(MethodValidationInterceptor.java:116) ~[spring-context-5.1.8.RELEASE.jar:5.1.8.RELEASE]
at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:186) ~[spring-aop-5.1.8.RELEASE.jar:5.1.8.RELEASE]
at org.springframework.aop.framework.CglibAopProxy$DynamicAdvisedInterceptor.intercept(CglibAopProxy.java:688) ~[spring-aop-5.1.8.RELEASE.jar:5.1.8.RELEASE]
at de.test.PersonServlet$$EnhancerBySpringCGLIB$$c3619ed5.test(<generated>) ~[classes/:na]
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[na:na]
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) ~[na:na]
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[na:na]
at java.base/java.lang.reflect.Method.invoke(Method.java:566) ~[na:na]
at org.springframework.web.method.support.InvocableHandlerMethod.doInvoke(InvocableHandlerMethod.java:190) ~[spring-web-5.1.8.RELEASE.jar:5.1.8.RELEASE]
at org.springframework.web.method.support.InvocableHandlerMethod.invokeForRequest(InvocableHandlerMethod.java:138) ~[spring-web-5.1.8.RELEASE.jar:5.1.8.RELEASE]
at org.springframework.web.servlet.mvc.method.annotation.ServletInvocableHandlerMethod.invokeAndHandle(ServletInvocableHandlerMethod.java:104) ~[spring-webmvc-5.1.8.RELEASE.jar:5.1.8.RELEASE]
at org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.invokeHandlerMethod(RequestMappingHandlerAdapter.java:892) ~[spring-webmvc-5.1.8.RELEASE.jar:5.1.8.RELEASE]
at org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.handleInternal(RequestMappingHandlerAdapter.java:797) ~[spring-webmvc-5.1.8.RELEASE.jar:5.1.8.RELEASE]
at org.springframework.web.servlet.mvc.method.AbstractHandlerMethodAdapter.handle(AbstractHandlerMethodAdapter.java:87) ~[spring-webmvc-5.1.8.RELEASE.jar:5.1.8.RELEASE]
at org.springframework.web.servlet.DispatcherServlet.doDispatch(DispatcherServlet.java:1039) ~[spring-webmvc-5.1.8.RELEASE.jar:5.1.8.RELEASE]
at org.springframework.web.servlet.DispatcherServlet.doService(DispatcherServlet.java:942) ~[spring-webmvc-5.1.8.RELEASE.jar:5.1.8.RELEASE]
at org.springframework.web.servlet.FrameworkServlet.processRequest(FrameworkServlet.java:1005) ~[spring-webmvc-5.1.8.RELEASE.jar:5.1.8.RELEASE]
at org.springframework.web.servlet.FrameworkServlet.doGet(FrameworkServlet.java:897) ~[spring-webmvc-5.1.8.RELEASE.jar:5.1.8.RELEASE]
at javax.servlet.http.HttpServlet.service(HttpServlet.java:634) ~[tomcat-embed-core-9.0.21.jar:9.0.21]
at org.springframework.web.servlet.FrameworkServlet.service(FrameworkServlet.java:882) ~[spring-webmvc-5.1.8.RELEASE.jar:5.1.8.RELEASE]
at javax.servlet.http.HttpServlet.service(HttpServlet.java:741) ~[tomcat-embed-core-9.0.21.jar:9.0.21]
at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:231) ~[tomcat-embed-core-9.0.21.jar:9.0.21]
at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) ~[tomcat-embed-core-9.0.21.jar:9.0.21]
at org.apache.tomcat.websocket.server.WsFilter.doFilter(WsFilter.java:53) ~[tomcat-embed-websocket-9.0.21.jar:9.0.21]
at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193) ~[tomcat-embed-core-9.0.21.jar:9.0.21]
at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) ~[tomcat-embed-core-9.0.21.jar:9.0.21]
at org.springframework.web.filter.RequestContextFilter.doFilterInternal(RequestContextFilter.java:99) ~[spring-web-5.1.8.RELEASE.jar:5.1.8.RELEASE]
at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:109) ~[spring-web-5.1.8.RELEASE.jar:5.1.8.RELEASE]
at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193) ~[tomcat-embed-core-9.0.21.jar:9.0.21]
at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) ~[tomcat-embed-core-9.0.21.jar:9.0.21]
at org.springframework.web.filter.FormContentFilter.doFilterInternal(FormContentFilter.java:92) ~[spring-web-5.1.8.RELEASE.jar:5.1.8.RELEASE]
at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:109) ~[spring-web-5.1.8.RELEASE.jar:5.1.8.RELEASE]
at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193) ~[tomcat-embed-core-9.0.21.jar:9.0.21]
at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) ~[tomcat-embed-core-9.0.21.jar:9.0.21]
at org.springframework.web.filter.HiddenHttpMethodFilter.doFilterInternal(HiddenHttpMethodFilter.java:93) ~[spring-web-5.1.8.RELEASE.jar:5.1.8.RELEASE]
at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:109) ~[spring-web-5.1.8.RELEASE.jar:5.1.8.RELEASE]
at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193) ~[tomcat-embed-core-9.0.21.jar:9.0.21]
at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) ~[tomcat-embed-core-9.0.21.jar:9.0.21]
at org.springframework.web.filter.CharacterEncodingFilter.doFilterInternal(CharacterEncodingFilter.java:200) ~[spring-web-5.1.8.RELEASE.jar:5.1.8.RELEASE]
at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:109) ~[spring-web-5.1.8.RELEASE.jar:5.1.8.RELEASE]
at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193) ~[tomcat-embed-core-9.0.21.jar:9.0.21]
at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) ~[tomcat-embed-core-9.0.21.jar:9.0.21]
at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:202) ~[tomcat-embed-core-9.0.21.jar:9.0.21]
at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:96) ~[tomcat-embed-core-9.0.21.jar:9.0.21]
at org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:490) ~[tomcat-embed-core-9.0.21.jar:9.0.21]
at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:139) ~[tomcat-embed-core-9.0.21.jar:9.0.21]
at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:92) ~[tomcat-embed-core-9.0.21.jar:9.0.21]
at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:74) ~[tomcat-embed-core-9.0.21.jar:9.0.21]
at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:343) ~[tomcat-embed-core-9.0.21.jar:9.0.21]
at org.apache.coyote.http11.Http11Processor.service(Http11Processor.java:408) ~[tomcat-embed-core-9.0.21.jar:9.0.21]
at org.apache.coyote.AbstractProcessorLight.process(AbstractProcessorLight.java:66) ~[tomcat-embed-core-9.0.21.jar:9.0.21]
at org.apache.coyote.AbstractProtocol$ConnectionHandler.process(AbstractProtocol.java:853) ~[tomcat-embed-core-9.0.21.jar:9.0.21]
at org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun(NioEndpoint.java:1587) ~[tomcat-embed-core-9.0.21.jar:9.0.21]
at org.apache.tomcat.util.net.SocketProcessorBase.run(SocketProcessorBase.java:49) ~[tomcat-embed-core-9.0.21.jar:9.0.21]
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) ~[na:na]
at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) ~[na:na]
at org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:61) ~[tomcat-embed-core-9.0.21.jar:9.0.21]
at java.base/java.lang.Thread.run(Thread.java:834) ~[na:na]
2019-08-06 14:08:31.417 WARN 3933 --- [nio-8080-exec-8] .w.s.m.s.DefaultHandlerExceptionResolver : Resolved [org.springframework.web.HttpMediaTypeNotAcceptableException: Could not find acceptable representation]
spring-boot-2.1.6 with spring-webmvc-5.1.8.
Comment From: rstoyanchev
There was a fix in Spring Framework 5.1, see #20865 to ensure that error response are not impacted the produces condition mapping, which would have been an issue for this scenario, and I have confirmed that with Spring Framework only this works as expected returning the underlying error.
In Spring Boot however, unhandled errors are rendered with a response body either as HTML or as JSON, and the "text/plain" matches neither. So technically Boot is correct in not rendering the content types it can render error content with aren't acceptable.
The answer might be in some way of customizing Boot's error handling? Or perhaps the Boot team might consider some enhancement for this? Either way it is not something we can fix in the Spring Framework so I'm going to close this.
/cc @bclozel
Comment From: bclozel
Here are a few ideas:
- Spring Boot could not honor the Accept header and return application/json anyway. I don't really like that solution as it goes against HTTP's intent and clients probably won't be able to read that response anyway. Also what should be the default choice? Developers might have an opinion, but in the end the HTTP clients are consuming the response
- we could try and serialize the error anyway. In theory, we could do that for text/plain with some specific format but this is not a solution generally applicable
Back to the first idea, I think the HTTP clients are fully in control here and could request something like Accept: text/plain, application/json; q=0.9. This is in my opinion the proper way for a client to express that it wants text/plain as a format but is willing to take application/json as a fallback option. This provides an opt-in solution and gives full control to the HTTP clients.
I can't think of any other feature that would improve this in Spring Framework or Spring Boot for now.
Comment From: rstoyanchev
There are also media types for error details (e.g. "application/problem+json") defined in RFC 7807 that could be added by a client.
@bclozel one extra option is to not render error details at all. The original error message is already lost as it is. This will at least ensure the original status code is not lost as well.
That said rendering error details in JSON might not be so bad either since arguably clients don't depend on parsing an error response like they do for success. In the worst case they'll fail to understand the error details but at least they have the status code and some indication about the error that can be read by a human.
Comment From: bclozel
Yes, not rendering the error could be an improvement here. In Spring Boot we're using an annotated ErrorController for that, and returning a ResponseEntity<Map<String, Object>>, basically delegating to Spring MVC for writing the response. I don't think there is a way to know that writing the response body won't work beforehand, and that we should skip the body.
I haven't seen "application/problem+json" being used in the wild for now. It could certainly be a nice replacement for the custom error message format provided by Spring Boot. I'm wondering though where applications should point to for the type field. It's supposed to be an URL containing human-readable information about the type of error. Do you think we should consider that at the Spring Boot level? This would solve this issue as well, as you've underlined.
So far, we only have the media type for that in our MediaType enum in Framework, and I'm not sure how we could improve the infrastructure for that. Maybe create dedicated exceptions and a class reproducing the expected data structure for this?
Comment From: rstoyanchev
I was thinking, if the supported media types for error details are known, then ErrorController could do some basic checks of its own to determine if those media types are acceptable. That would help it decide whether to render content or not.
As for the type field from RFC 7807, it says that it is optional, so to start it could be blank. Perhaps some sort of configurable Exception type to URL mapping could be used to allow configuring this further. Adding support for such a type field to existing exception would be a challenge, but it could be an option for @ResponseStatus annotated exceptions.
Comment From: membersound
That said rendering error details in JSON might not be so bad either since arguably clients don't depend on parsing an error response like they do for success. In the worst case they'll fail to understand the error details but at least they have the status code and some indication about the error that can be read by a human.
I think the last is crucial: most applications will catch on any status code != 200 OK, and then forward or log the error (and message body) to the developer. So it's more important to retain the original http error status code here! (as clients may add custom error handling on that integer). But clients won't hardly ever parse the error-message-freetext payload, which we thus should less focus on.
Swallowing the original error with a HttpMediaTypeNotAcceptableException is probably the worst option for end consumers. It's like having a big hole in the highway on a rainy day, and the radiostation saying don't drive, there's water on the road...
Comment From: bclozel
I've created spring-projects/spring-boot#19522 to improve that behavior in Spring Boot. I've also created spring-projects/spring-boot#19525 to consider the problem details RFC.