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

Customize list of exceptions to unwrap in resteasy-reactive #42089

Closed
kevinferrare opened this issue Jul 23, 2024 · 9 comments · Fixed by #42094
Closed

Customize list of exceptions to unwrap in resteasy-reactive #42089

kevinferrare opened this issue Jul 23, 2024 · 9 comments · Fixed by #42094
Labels
area/rest kind/enhancement New feature or request
Milestone

Comments

@kevinferrare
Copy link

Description

Exceptions handling declared via ServerExceptionMapper annotation are not called when the exception is wrapped into another exception.

Looking at quarkus code, it looks like the RuntimeExceptionMapper class is responsible for unwrapping exceptions, but it only unwraps exceptions that are in a predefined list computed at build time via build items.

Example for resteasy reactive extension:

I propose to add a configuration property to allow users to customize this list of exceptions to unwrap.

Implementation ideas

No response

@kevinferrare kevinferrare added the kind/enhancement New feature or request label Jul 23, 2024
@quarkus-bot
Copy link

quarkus-bot bot commented Jul 23, 2024

/cc @FroMage (resteasy-reactive), @geoand (resteasy-reactive), @stuartwdouglas (resteasy-reactive)

@geoand
Copy link
Contributor

geoand commented Jul 23, 2024

I think this makes sense

@gsmet
Copy link
Member

gsmet commented Jul 23, 2024

Is it a custom exception that you want to unwrap? Or something that would make sense in general?

@geoand
Copy link
Contributor

geoand commented Jul 23, 2024

It makes sense to have this be configurable.
I planned to do it when someone asked 😀

@gsmet
Copy link
Member

gsmet commented Jul 23, 2024

Oh yeah, I totally agree but I wanted to know the use case as it might be something we want to add as default anyway for better user experience.

@geoand
Copy link
Contributor

geoand commented Jul 24, 2024

One question I do have however is whether the feature should be enabled by a configuration property or some new annotation.

The reason the configuration property (although totally reasonable) can be suboptimal is that the IDE doesn't provide any assistance or validation of the value

geoand added a commit to geoand/quarkus that referenced this issue Jul 24, 2024
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
@geoand
Copy link
Contributor

geoand commented Jul 24, 2024

#42094 is what I have in mind

geoand added a commit to geoand/quarkus that referenced this issue Jul 24, 2024
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
@kevinferrare
Copy link
Author

Thank you for the quick and welcoming feedback!
@gsmet

Is it a custom exception that you want to unwrap? Or something that would make sense in general?
It is both, we have java.util.concurrent.CompletionException and a custom exception, thrown by an internal library.

@geoand
After opening the issue, I started to wonder what would be the drawback of also optionally providing the option of just unwrapping all exceptions unconditionally until a handler is found.

In our use case, I don't think this would lead to unwanted behaviours.

@geoand
Copy link
Contributor

geoand commented Jul 24, 2024

After opening the issue, I started to wonder what would be the drawback of also optionally providing the option of just unwrapping all exceptions unconditionally until a handler is found.

Although this might make sense, it is however not how the JAX-RS / Jakarta REST specifies things, so it's best not to make this the default behavior.

geoand added a commit to geoand/quarkus that referenced this issue Jul 24, 2024
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
geoand added a commit to geoand/quarkus that referenced this issue Jul 24, 2024
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
@geoand geoand closed this as completed in f743fa4 Jul 30, 2024
geoand added a commit that referenced this issue Jul 30, 2024
Introduce `@UnwrapException` for Quarkus REST
@quarkus-bot quarkus-bot bot added this to the 3.14 - main milestone Jul 30, 2024
holly-cummins pushed a commit to holly-cummins/quarkus that referenced this issue Jul 31, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/rest kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants