DefaultResponseExceptionMapper should be removed from Spec and reworked

[patriot1burke]

The DefaultResponseExceptionMapper causes issues for application development when proxy methods return a Response object. When app developers have a Response return type this usually means they want to handle all aspects of the response: success and error conditions. Having DefaultResponseExceptionMapper on by default will mean the app developer will have to switch off this default mapper.

Having a Response return type is quite common so I propose that DefaultResponseExceptionMapper be removed. Instead the language in the spec should specify what happens in error condititions. Specifically that if there is no exception mapper for the response and the return type is not a Response, then error status codes should be mapped to the WebApplicationException exception hierarchy. Furthermore, the spec should also deal with redirect response codes and map those to the appropriate web application exception (RedirectionException).

Finally, just want to say I'm not some random dude. I'm the founder of Resteasy, author of Restful Java and was Red Hat's rep on JAX-RS specification. I wrote a Resteasy extension back in 2012 very similar to Rest Client MP. So, I've seen this type of rest client proxy in action and used it myself on a number of projects. I think the spec would be much improved with my suggestion. Thanks!

[andymc12]

Hi @patriot1burke - thanks for the suggestion!

This would be a breaking change since users expecting the default response mapper would no longer see those exceptions after upgrading to a release with this proposed change. Not a deal-breaker, just means we'd have to put it in a 2.0 release.

I definitely agree with being able to either handle redirects or to throw the more specific RedirectionException.

What I'm not so sure about is treating the exception mapping behavior differently based on the return type of the method. One of the major advantages of the MP Rest Client is that you can return type-safe objects directly without needing to call response.readEntity(...) methods, etc. Ideally, developers would avoid returning Response object types and would return things like Widget, Record, Person, etc. In the cases where a non-Response type is returned, there generally needs to be an exception mapper - and having a default mapper on out of the box works for most cases. I think things get more complex if we say "When the return type is Response, then there is no default exception mapper, but when the return type is anything else, then there is - unless you disable it."

What would you think about adding a disableDefaultMapper attribute to the @RegisterRestClient annotation? Right now, there is already a way for users to disable the default mapper if using the RestClientBuilder programmatic API - but I don't think there is a way to disable it in the code if using CDI injection. We would need to keep the default as false, but that is a bit easier for developers to disable it than using MP Config.

Thanks again!

[andreas-eberle]

What I'm not so sure about is treating the exception mapping behavior differently based on the return type of the method. One of the major advantages of the MP Rest Client is that you can return type-safe objects directly without needing to call response.readEntity(...) methods, etc. Ideally, developers would avoid returning Response object types and would return things like Widget, Record, Person, etc. In the cases where a non-Response type is returned, there generally needs to be an exception mapper - and having a default mapper on out of the box works for most cases. I think things get more complex if we say "When the return type is Response, then there is no default exception mapper, but when the return type is anything else, then there is - unless you disable it."

Like @patriot1burke stated, I also think it is the expected behavior that I do not get an exception when I want the Response as the return type of a request.

E.g. I need to query an API to see if an ID is valid. If it is, I do some stuff, if it isn't I do some other stuff. Getting an exception in this case is totally unnecessary and not a nice way to code. Instead, I just want to have a method like this:

    @HEAD
    @Path("/ids/{someId}")
    fun sometimesDoesntExist(@PathParam("someId") someId: String): Response

If I would just get the response for every status code, I can wrap this method with a simple if to get my boolean idExists flag. With the current implementation I need to catch an exception which had to be created first... this is probably the most expensive workaround for a simple if.

I understand that removing the exception behavior is a breaking change and has to wait until 2.0. However, your proposed way of disabling this behavior with the @RegisterRestClient annotation would not be a breaking change and provide a quicker way to at least give the users an option to decide themselves.

[svenhaag]

Hi,

I agree with @patriot1burke. In my use-case I return a DTO which should also be returned for a specific HTTP status code (503) which is not possible currently as I always get a WebApplicatoinException, despite I provided a custom exception mapper which returns null for this very status code. Please see https://issues.redhat.com/browse/RESTEASY-2882.
I would be very glad to have the proposed solution of @patriot1burke. Maybe in v3? :(

Cheers