-
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
Introduce @UnwrapException
for Quarkus REST
#42094
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
* Used to configure that an exception (or exceptions) should be unwrapped during exception handling. | ||
* <p> | ||
* Unwrapping means that when an {@link Exception} of the configured type is thrown and no | ||
* {@code jakarta.ws.rs.ext.ExceptionMapper} exists, |
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.
This is a bit subtle. This unwrapping only happens when there's no registered mapper for the direct exception type?
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.
Correct
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.
Do you think this should be mentioned in the docs? (The guide, not just the javadoc)
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.
It is implied, no?
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.
I did not find it implied, no. How I read it was that the minute we declare that an exception should be unwrapped, it will never match an exceptoin mapper.
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.
If you want to push an update with what you have in mind, I'm perfectly fine with that
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.
I'm not sure how to phrase this in the docs, especially given that it's already a TIP followed by a NOTE, I feel like this succession of tips/notes should be turned into sub-sections :-/
Also, I'm now realising that it should probably be a warning (or error?) if we have both an exception mapper for T
and an unwrap exception for T
, no? In this case, the exception will never be unwrapped.
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.
Also, I'm now realising that it should probably be a warning (or error?) if we have both an exception mapper for T and an unwrap exception for T, no? In this case, the exception will never be unwrapped.
Yeah, that can be a good follow-up
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.
I'm not sure how to phrase this in the docs, especially given that it's already a TIP followed by a NOTE, I feel like this succession of tips/notes should be turned into sub-sections :-/
Yeah, very likely so.
This allows users to configure exceptions whose cause will be checked against exception mappers. This capability already existed in Quarkus REST and was used to map some internal exceptions, but with the new annotation users can opt into the feature for whatever exceptions make sense for their use case. Closes: quarkusio#42089
Status for workflow
|
Status for workflow
|
@FroMage I am going to merge this one and we can improve the docs in later PRs |
🙈 The PR is closed and the preview is expired. |
This allows users to configure exceptions
whose cause will be checked against exception mappers.
This capability already existed in Quarkus REST and was used to map some internal exceptions, but with the new annotation users can opt into the feature for whatever exceptions make sense for their use case.