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

ConstraintViolationException returned as HttpMediaTypeNotAcceptableException #23421

Closed
membersound opened this issue Aug 6, 2019 · 7 comments
Assignees
Labels
for: external-project Needs a fix in external project

Comments

@membersound
Copy link

membersound commented Aug 6, 2019

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.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Aug 6, 2019
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Dec 20, 2019

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

@rstoyanchev rstoyanchev self-assigned this Dec 20, 2019
@rstoyanchev rstoyanchev added status: invalid An issue that we don't feel is valid for: external-project Needs a fix in external project and removed status: waiting-for-triage An issue we've not yet triaged or decided on status: invalid An issue that we don't feel is valid labels Dec 20, 2019
@bclozel
Copy link
Member

bclozel commented Dec 20, 2019

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.

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jan 2, 2020

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.

@bclozel
Copy link
Member

bclozel commented Jan 2, 2020

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?

@rstoyanchev
Copy link
Contributor

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.

@membersound
Copy link
Author

membersound commented Jan 3, 2020

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...

@bclozel
Copy link
Member

bclozel commented Jan 3, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: external-project Needs a fix in external project
Projects
None yet
Development

No branches or pull requests

4 participants