-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Controller advice exception content type #6099
Controller advice exception content type #6099
Conversation
Thanks Gytis! I'll take a look soon. Can you please squash the commits though? |
I was planning to squash them after the review, thought it make it easier for you to review the changes. But if you prefer, I could squash them now. |
You can go ahead and squash, reviewing shouldn't be a problem. |
I took a quick look at it and it looks good! But of course I'll need to spend more time with it. |
ddaf068
to
f168677
Compare
.../java/io/quarkus/spring/web/deployment/ControllerAdviceAbstractExceptionMapperGenerator.java
Show resolved
Hide resolved
...ns/spring-web/deployment/src/main/java/io/quarkus/spring/web/deployment/ResponseBuilder.java
Show resolved
Hide resolved
...ing-web/runtime/src/main/java/io/quarkus/spring/web/runtime/ResponseContentTypeResolver.java
Outdated
Show resolved
Hide resolved
...ion-tests/spring-web/src/main/java/io/quarkus/it/spring/web/ExceptionThrowingController.java
Outdated
Show resolved
Hide resolved
...ion-tests/spring-web/src/main/java/io/quarkus/it/spring/web/ExceptionThrowingController.java
Outdated
Show resolved
Hide resolved
...ion-tests/spring-web/src/main/java/io/quarkus/it/spring/web/ExceptionThrowingController.java
Outdated
Show resolved
Hide resolved
Seems like the imports are not sorted:
So what you need to do is basically do an |
f168677
to
9cfdca1
Compare
Thanks for the updates @gytis! I'll try and a proper review soon |
@geoand I'll let you tell me if you want that backported. |
@gsmet I hope to be able to review tomorrow and hopefully we can get it in for backporting |
@gytis this part of the docs should also be updated: https://github.com/quarkusio/quarkus/blob/master/docs/src/main/asciidoc/spring-web.adoc#L247 |
.../java/io/quarkus/spring/web/deployment/ControllerAdviceAbstractExceptionMapperGenerator.java
Outdated
Show resolved
Hide resolved
Excellent work if I may say so @gytis! Once you take care of the minor comments I have we can get this in. |
Hmm, it's a bit tricky, because we've only changed @ExceptionHandler content type for controller advice. |
9cfdca1
to
d178624
Compare
OK let's leave it as is for now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @gytis!!!
Fixes #5816
cc @geoand