UnwrapException ineffective when a subclass of the declared exception is mapped via ServerExceptionMapper

[kevinferrare]

Describe the bug

I am trying to use UnwrapException annotation for an exception type which parent is also mapped in an exception mapper via ServerExceptionMapper:

@UnwrapException({ WebApplicationException.class })
public class ExceptionsMappers {
  @ServerExceptionMapper
  public RestResponse<Error> mapUnhandledException(RuntimeException ex) {
    // Custom error for any other unhandled exceptions
    ...
  }

  @ServerExceptionMapper
  public RestResponse<Error> mapJsonProcessingException(JsonProcessingException ex) {
    // Custom error for jackson parsing issues, usually wrapped into WebApplicationException
    ...
  }
}

This makes UnwrapException ineffective because I guess it matches WebApplicationException as RuntimeException which is already mapped in mapUnhandledException, and as stated in the javadoc:

Unwrapping means that when an Exception of the configured type is thrown and no jakarta.ws.rs.ext.ExceptionMapper exists, then RESTEasy Reactive will attempt to locate an ExceptionMapper for the cause of the Exception.

Is this the expected behaviour?
In that case, is there a way to use UnwrapException while also providing custom error messages for unhandled cases?

If not, can this be improved?

Expected behavior

No response

Actual behavior

No response

How to Reproduce?

No response

Output of uname -a or ver

6.11.0-26-generic #26~24.04.1-Ubuntu

Output of java -version

java version "21.0.4" 2024-07-16 LTS

Quarkus version or git rev

3.20.0.redhat-00002

Build tool (ie. output of mvnw --version or gradlew --version)

Apache Maven 3.9.9

Additional information

No response

[quarkus-bot]

/cc @FroMage (rest)

[geoand]

Can you perhaps attach an example application with a couple small tests showing exactly how you hoping Quarkus would behave vs how it currently behaves?

Thanks

[kevinferrare]

Sure, I wrote a small sample app with 2 tests that I expected to pass, but second one fails:
quarkus-48197-reproducer.zip

[geoand]

Thanks a lot.

Actually the behavior you are seeing is what the JAX-RS / Jakarta REST specification dictates, so changing this could potentially cause a lot of disruption, which is something we don't want.
I would suggest avoiding the use of very general exceptions like RuntimeException thus sidestepping the problem

[kevinferrare]

I am not sure to understand why this is related to jax-rs, UnwrapException is quarkus specific right?

In any case, the need we have is to use a custom grammar in case there are errors, this is why we are catching any unmapped exception to ensure the client almost never gets a format they cant parse.

This works fine by the way, it's just that UnwrapException doesn't work in this setup.

So I was wondering if there is a proper way to do this or if this is just a limitation that means we won't be able to use the feature.

[FroMage]

Unwrapping means that when an Exception of the configured type is thrown and no jakarta.ws.rs.ext.ExceptionMapper exists, then RESTEasy Reactive will attempt to locate an ExceptionMapper for the cause of the Exception.

What is happening is correct according to this definition: in this case, we have a WebApplicationException (I assume) thrown, and there is a matching ExceptionMapper for it, so we don't unwrap it.

Now, we could make this conditional and always unwrap before attempting to locate any matching ExceptionMapper: @UnwrapException(always = true, value = WebApplicationException.class}, but we'd have to add that functionality.

I do understand the value of having a safety blanket mapper for RuntimeException. We could tell you to special-case your safety mapper to delegate back in case of WebApplicationException:

public class ExceptionsMappers {
  @ServerExceptionMapper
  public RestResponse<Error> mapUnhandledException(RuntimeException ex) {
    if(ex instanceof WebApplicationException && ex.getCause() instanceof JsonProcessingException cause) {
      return mapJsonProcessingException(cause);
    }
    // Custom error for any other unhandled exceptions
    ...
  }

  @ServerExceptionMapper
  public RestResponse<Error> mapJsonProcessingException(JsonProcessingException ex) {
    // Custom error for jackson parsing issues, usually wrapped into WebApplicationException
    ...
  }
}

But this is annoying to write, and it doesn't compose well: how do we delegate to other exception mappers? And finally it puts the burden of adapting the potential different return types and injected parameters of every delegated exception mapper.

Perhaps if we had a way to rethrow an exception from an exception mapper, and have it go back via exception mappers?

public class ExceptionsMappers {
  @ServerExceptionMapper(mapExceptionsThrown = true)
  public RestResponse<Error> mapUnhandledException(RuntimeException ex) {
    if(ex instanceof WebApplicationException {
      throw ex; // this would go back via the mapper due to the special annotation config
    }
    // Custom error for any other unhandled exceptions
    ...
  }

  @ServerExceptionMapper
  public RestResponse<Error> mapJsonProcessingException(JsonProcessingException ex) {
    // Custom error for jackson parsing issues, usually wrapped into WebApplicationException
    ...
  }
}

Or an API we have that allows to hook back into the exception mappers?

public class ExceptionsMappers {
  @ServerExceptionMapper
  public RestResponse<Error> mapUnhandledException(RuntimeException ex, /* injected API */ ExceptionMapperHandler mapper) {
    if(ex instanceof WebApplicationException {
      return mapper.mapException(ex.getCause()); // this would go back via the mapper
    }
    // Custom error for any other unhandled exceptions
    ...
  }

  @ServerExceptionMapper
  public RestResponse<Error> mapJsonProcessingException(JsonProcessingException ex) {
    // Custom error for jackson parsing issues, usually wrapped into WebApplicationException
    ...
  }
}

But this would be constrainted by return types again.

My opinion is that @UnwrapException(always = true, value = WebApplicationException.class} would be the easiest to write and explain.

WDYT @geoand ?

[geoand]

WDYT @geoand ?

I am +1 for the following suggestion you made:

Now, we could make this conditional and always unwrap before attempting to locate any matching ExceptionMapper: @UnwrapException(always = true, value = WebApplicationException.class}, but we'd have to add that functionality.

[FroMage]

Yeah, that seems the best option so far. So let's reopen, then.

@kevinferrare do you feel up to providing a PR for this? We can help.

[kevinferrare]

I never contributed code to quarkus but I can try :)