rest icon indicating copy to clipboard operation
rest copied to clipboard

Optionally remove ResponseProcessingException

Open t1 opened this issue 5 years ago • 11 comments

I have a ClientResponseFilter that throws unchecked business exceptions. JAX-RS wraps them in ResponseProcessingException, which forces the client code to unwrap them manually.

I'd like to have a way to opt-out of this wrapping. Maybe an annotation on the ClientResponseFilter?

I could work on the details, if the general idea is accepted.

t1 avatar Jan 28 '20 04:01 t1

First step to convince the community of such a change would be to write more about why you think this commonly is nededed / what the general benefit is. :-)

mkarg avatar Jan 28 '20 06:01 mkarg

To be honest, I like how JAX-RS is currently handling this case. Throwing unchecked exceptions from a JAX-RS component is treated as an "error" and therefore wrapping the error in an exception described like this makes sense to me:

JAX-RS client-side runtime processing exception thrown to indicate that response processing has failed

And IF you throw business exceptions and want to access them later on, unwrapping is just one line of code, right? Or am I missing something?

chkal avatar Jan 28 '20 07:01 chkal

@mkarg : Thanks for the hint ;-)

My use-case (as stated in #839) is that I want to map Problem Details (http response entities describing the exact cause for a status code as standardized in RFC-7807) to custom business exceptions containing all those details.

Wrapping these exceptions into a ResponseProcessingException requires the client code to unwrap it again, e.g.:

    public Shipment order(String articleId) {
        try {
            try {
                return target() // a configured `WebTarget`
                    .path("/orders").request(APPLICATION_JSON_TYPE)
                    .post(Entity.form(new Form().param("article", article)));
            } catch (ResponseProcessingException e) {
                throw (RuntimeException) e.getCause();
            }
        } catch (OutOfCreditException e) {
            displayOutOfCredit(e.getBalance());
            return null;
        }
    }

Instead of:

    public Shipment order(String articleId) {
        try {
            return target() // a configured `WebTarget`
                .path("/orders").request(APPLICATION_JSON_TYPE)
                .post(Entity.form(new Form().param("article", article)));
        } catch (OutOfCreditException e) {
            displayOutOfCredit(e.getBalance());
            return null;
        }
    }

It's only 4 lines, sure; but it's mixing technical concerns with business concerns.

It is an error, yes. But it's not a technical problem that requires the client to go back into a pristine state (e.g. rolling back a transaction), logging the stacktrace, and retry. It's a business error that has to be handled properly. I don't see the benefit of wrapping the exception in this case.

t1 avatar Jan 28 '20 08:01 t1

Thanks for explaining your use case. Let me share a few thoughts.

I wonder if it wouldn't make more sense to implement this in a CDI interceptor. Something like:

    @ProblemAware
    public Shipment order(String articleId) {
        return target() // a configured `WebTarget`
            .path("/orders").request(APPLICATION_JSON_TYPE)
            .post(Entity.form(new Form().param("article", article)));
    }

In this case a ProblemAwareInterceptor could catch ResponseProcessingException and translate it accordingly.

Just a thought...

chkal avatar Feb 01 '20 11:02 chkal

@chkal : That's a great idea; it still requires the @ProblemAware artifact, but it's better than unwrapping the ResponseProcessingException. I think I'll implement that.

An option to not wrap exceptions in the first place would be even better. And it should be easy to add. I can work on a specific proposal, if it has a chance of being accepted.

t1 avatar Feb 02 '20 10:02 t1

Once JAX-RS resources become CDI beans automatically, you'll be able to dinamycally add the interceptor via a CDI extension.

This is already possible when bean discovery mode is set to "all".

El dom., 2 feb. 2020 11:56, Rüdiger zu Dohna [email protected] escribió:

@chkal https://github.com/chkal : That's a great idea; it still requires the @ProblemAware artifact, but it's better than unwrapping the ResponseProcessingException. I think I'll implement that.

An option to not wrap exceptions in the first place would be even better. And it should be easy to add. I can work on a specific proposal, if has a chance of being accepted.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/eclipse-ee4j/jaxrs-api/issues/838?email_source=notifications&email_token=AAQC44HPWWVO6JY3R7PVODTRA2RFRA5CNFSM4KMMG64KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKRT3VQ#issuecomment-581123542, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAQC44BEGYF3Y5BTHBXJ4GTRA2RFRANCNFSM4KMMG64A .

ggam avatar Feb 02 '20 13:02 ggam

I'd +1 the idea of not having to unwrap the exception from ResponseProcessingException. (This reminds me of the @ApplicationException that solved unwrapping EJB exceptions back then (bottom of page here, if you're interested)).

I'm not having a clear preference on how to solve that, but it would improve the usage if folks would not have to unwrap nor obfuscate/hide that handling in an interceptor. So @t1's original suggestion would be nice to have, IMO.

sdaschner avatar Feb 04 '20 14:02 sdaschner

Once JAX-RS resources become CDI beans automatically, you'll be able to dinamycally add the interceptor via a CDI extension. This is already possible when bean discovery mode is set to "all". El dom., 2 feb. 2020 11:56, Rüdiger zu Dohna [email protected] escribió:

Or adding @Path as a bean defining annotation. With CDI in the picture, there are enough tools for a library built on top of JAX-RS to do this unwrapping auto-magically.

spericas avatar Feb 05 '20 14:02 spericas

Or adding @Path as a bean defining annotation. With CDI in the picture, there are enough tools for a library built on top of JAX-RS to do this unwrapping auto-magically.

I second that. IMHO everything which can be done already with some custom code ontop shall never be part of JAX-RS. Only those things that cannot be done otherwise should become part of the spec. Having said that, I really do plea for us spinning off a new project implementing standard solutions ontop of JAX-RS, like "JAX-RS Commons". :-)

mkarg avatar Feb 08 '20 14:02 mkarg

Well, I actually meant that. Problem is CDI doesn't have a portable way to do that as of now...

El mié., 5 feb. 2020 15:03, Santiago Pericasgeertsen < [email protected]> escribió:

Once JAX-RS resources become CDI beans automatically, you'll be able to dinamycally add the interceptor via a CDI extension. This is already possible when bean discovery mode is set to "all". El dom., 2 feb. 2020 11:56, Rüdiger zu Dohna [email protected] escribió:

Or adding @Path as a bean defining annotation. With CDI in the picture, there are enough tools for a library built on top of JAX-RS to do this unwrapping auto-magically.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/eclipse-ee4j/jaxrs-api/issues/838?email_source=notifications&email_token=AAQC44DP4P6CZ322AVBGEFLRBLBKJA5CNFSM4KMMG64KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEK3Q42Y#issuecomment-582422123, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAQC44E4SRSEGLXGPSNOWULRBLBKJANCNFSM4KMMG64A .

ggam avatar Feb 08 '20 14:02 ggam

Well, I actually meant that. Problem is CDI doesn't have a portable way to do that as of now...

Right, that's just Weld. It would be really nice for CDI.next to support that.

spericas avatar Feb 08 '20 15:02 spericas