Fix various usability issues in Spring @RestControllerAdvice handling #5803
Conversation
This could happen if multiple exceptions with the same simple name but different package names were used
|
@gsmet this should be easy to review commit by commit |
.../spring-web/runtime/src/main/java/io/quarkus/spring/web/runtime/ResponseEntityConverter.java
Show resolved
Hide resolved
.../spring-web/runtime/src/main/java/io/quarkus/spring/web/runtime/ResponseEntityConverter.java
Outdated
Show resolved
Hide resolved
| if (returnType.asParameterizedType().arguments().size() == 1) { | ||
| Type responseEntityParameterType = returnType.asParameterizedType().arguments().get(0); | ||
| if (STRING.equals(responseEntityParameterType.name())) { | ||
| addDefaultJsonContentType = false; |
There was a problem hiding this comment.
There's no way in Spring to explicitly define the response media type?
There was a problem hiding this comment.
For the regular controller method sure there is and we handle it. But for error handler you either return an object and rely on the default or return a ResponseEntity where you can set whatever you like.
There was a problem hiding this comment.
Are you sure about that? Wouldn't it use the content type defined in the @RequestMapping annotation of the original method?
And just to be clear, we don't support overriding the content-type in the ResponseEntity response of the exception mapper?
There was a problem hiding this comment.
Are you sure about that? Wouldn't it use the content type defined in the
@RequestMappingannotation of the original method?
Yes it would, and that is what causes the problem, since the controller method uses a type that doesn't match what the exception handler returns.
And just to be clear, we don't support overriding the content-type in the
ResponseEntityresponse of the exception mapper?
That's part of the responsibility of the application code. If the application sets it, we don't change it:
There was a problem hiding this comment.
We discussed this on Zulip with @geoand. Our current support is a bit limited as we will always return JSON (except in some edge cases), even if the original method was pushing some XML.
That's not very user friendly and we will probably need to improve that. But that can probably wait so we decided to merge that one first.
There was a problem hiding this comment.
As discussed I will research this a bit more and open an issue for a follow-up implementation
...spring-web/deployment/src/main/java/io/quarkus/spring/web/deployment/SpringWebProcessor.java
Show resolved
Hide resolved
…ent type unless void or String Fixes: quarkusio#5796
It broke in 6d0025c
gsmet
added
the
triage/waiting-for-ci
2 participants
Fixes: #5796