Uni.onFailure() does not work with RestEasy Reactive client and ClientWebApplicationException in prod and dev modes

[sberyozkin]

Describe the bug

The following code works when the tests are run:

https://github.com/quarkusio/quarkus-quickstarts/blob/development/security-openid-connect-client-quickstart/src/main/java/org/acme/security/openid/connect/client/FrontendResource.java#L34

where a ClientWebExceptionException is handle by Uni.onFailure.recoverWith - the test needs to assert that 403 is returned from a downstream service.

However in both dev and prod mode that Uni.onFailure.recoverWith is not reached and ClientWebExceptionException escaped leading to 500 returned to the client, so I had to add an ExceptionMapper: https://github.com/quarkusio/quarkus-quickstarts/pull/1108/files

Expected behavior

No response

Actual behavior

No response

How to Reproduce?

To run the tests, only do mvn clean install in quickstarts/security-openid-connect-client-quickstarts - all the tests pass.

Then you can try quarkus:dev, then go to http://localhost:8080/q/dev/, choose OIDC/keycloak, press on Login to SinglePageApplication, then login as alice:alice, then choose test with AccessToken, use /frontend/admin-name-with-propagated-token, and before https://github.com/quarkusio/quarkus-quickstarts/pull/1108/files gets applied you will get 500, or disable the exception mapper if it is already applied. Perhaps it can be simpler to avoid Keycloak and have the same test structure and have a downstream ProtectedResource throw some exception and try to recover in FrontendResource

Output of uname -a or ver

No response

Output of java -version

No response

GraalVM version (if different from Java)

No response

Quarkus version or git rev

No response

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

No response

Additional information

No response

[quarkus-bot[bot]]

/cc @FroMage, @geoand, @stuartwdouglas

[sberyozkin]

If this is done by design, let only the tests see the escaped exceptions, then please feel free to close

[geoand]

Hey @sberyozkin,

so IIUC when running the tests you get the expected behavior but not in dev mode and prod mode?

[sberyozkin]

Hey @geoand Exactly, when running the tests the onFailure().recoverWith() is executed, in dev and prod - not.

[geoand]

That's really weird... I'll take a look

[sberyozkin]

@geoand Hi, yes, I had the tests passing and then went to try it in devmode UI and it failed and then it also failed in prod.

[geoand]

Unfortunately I don't understand what is going on here... In both cases the control flow reaches https://github.com/quarkusio/quarkus/blob/2.9.0.Final/independent-projects/resteasy-reactive/client/runtime/src/main/java/org/jboss/resteasy/reactive/client/impl/RestClientRequestContext.java#L330 with the proper ClientWebApplicationException, but then in the test case the Uni's recoverWith is called, why in the case of dev mode it's not...

Perhaps @cescoffier has ideas about this

[sberyozkin]

@geoand Thanks for confirming it is the case

[geoand]

Sorry I could not help more...

[sberyozkin]

Np at all, the demo works with the exception mapper added, so it is not a blocker

[cescoffier]

First, I have the same behavior in test, dev, and prod. If I remove the exception mapper, I get a 500.

But, I'm not totally sure I understand.

The application gets an authenticated request and then uses a REST client to access a restricted resource. The access is not allowed, so the client throws an Exception (a ClientWebApplicationException where the cause is the WebApplicationException).

It seems that the question is: why would this ClientWebApplicationException not be used to produce the 403 response? This is what the mapper does. However, I'm not sure it's the right thing to do. This inner exception (the ClientWebApplicationException) should not be pushed to the client. That's an implementation detail that can actually leak details on the infrastructure. So, responding with a 500 is more correct in this case (IMHO).

Now, if we want to handle this, we can just do the following:

    @Override
    protected void handleUnrecoverableError(Throwable throwable) {     
        if (throwable instanceof ClientWebApplicationException && throwable.getCause() != null) {
              result.completeExceptionally(throwable.getCause());
        } else {
            result.completeExceptionally(throwable);
       }
    }

Then, we complete with the WebApplicationException which will be used to produce the 403. But again, I'm not sure it's the right thing to do.

[FroMage]

@cescoffier The questions was more that a Mutiny recoverWith handler was only invoked in TEST mode, and not DEV nor PROD.

That's what the puzzle is. I'm not sure what you tested it with, Clément, because if I follow the link to recoverWith in the issue description I get code which does not have recoverWith anymore, so perhaps it was changed since?

[cescoffier]

I think it got changed. I updated the reproducer and for me it always went in the same branch (so same behavior in prod, test and dev). Now we can discuss if it's the right behavior, but at least it's the same.

[FroMage]

Which same branch? Did recoverFrom get called?

[cescoffier]

I can't remember. I would need to rebuild my reproducer.

[FroMage]

Well, if it seemed logical to you, it probably worked, so let's close this?