Rest Client Reactive - Filters not executed for non-successful return codes

[michalszynkiewicz]

CC @geoand I will try to attach a minimal reproducer tomorrow

[geoand]

That would be great, thanks.

I'm actually very surprised the JAX-RS TCK doesn't test this...

[michalszynkiewicz]

AFAIR the MicroProfile TCK tests this by returning an erroneous response from a filter, maybe JAX-RS one does it this way too?

[geoand]

Not sure TBH.

BTW, reading https://quarkusio.zulipchat.com/#narrow/stream/187030-users/topic/reactive.20server.20.26.20client it seems like that issue was about ResponseExceptionMapper classes, but this issue is about ClientResponseFilter. Is that intentional?

Is ResponseExceptionMapper handled properly by the new REST Client?

[geoand]

Ah, now I see how it works.

So basically the rest-client engages ResponseExceptionMapper by implementing a ClientResponseFilter, so the problem is actually that ClientResponseFilter is not executed when an exception is thrown.

[michalszynkiewicz]

Running the test from the example project you can see that the ClientResponseCompleteRestHandler is throwing an exception.

I think the exception could be thrown later, in ClientResponseCompleteRestHandler and not in ClientSetResponseEntityRestHandler if the status code is wrong.
Or maybe we could set the checkSuccessfulFamily to false for rest client? I'm not sure what's the best approach.

[michalszynkiewicz]

changing how rest client reactive handles ResponseExceptionMapper is also an option :)

[geoand]

I'll look at it more closely either later on today or tomorrow

…

On Thu, Apr 22, 2021, 11:20 Michał Szynkiewicz ***@***.***> wrote: changing how rest client reactive handles ResponseExceptionMapper is also an option :) — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#16702 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABBMDP55APNIHAU7IRKNHRDTJ7L5ZANCNFSM43LBBATA> .

[geoand]

In https://github.com/geoand/quarkus/tree/%2316702 I made the ClientResponseFilter part of the abort handling chain, however the JAX-RS TCK did not pass (there was a test that failed with a NPE because it expected the HTTP status to be set).

Moreover, even with this change in place, the reproducer does not engage the MyResponseExceptionMapper.
So it looks like some other strategy will be needed to handle ResponseExceptionMapper.

Finally, in this specific, it doesn't really make sense IMHO to have ResponseExceptionMapper as the there was no response from the server - processing fails with java.net.UnknownHostException: restcountries.eu3: Name or service not known

So @michalszynkiewicz it's really up to you how you want to proceed on the MP REST Client. We can certainly make changes to the JAX-RS Client, but we still need to pass the TCK

[michalszynkiewicz]

For unknown host, I agree, we should fail as we do now, I think.

I don't fully follow what happened in the jax-rs test, tried looking at it in the browser and it is not easy to follow :D

[geoand]

The spec isn't clear on whether or not ClientResponseFilter should be executed when there is an exception or not and the NPE in failing test isn't exactly indicative.

Does the old rest-client depend on this behavior?

[michalszynkiewicz]

Classic RestEasy uses a ClientResponseFilter for exception mapping:
https://github.com/resteasy/Resteasy/blob/main/resteasy-client-microprofile-base/src/main/java/org/jboss/resteasy/microprofile/client/ExceptionMapping.java

Does this answer your question?

[geoand]

OK, I guess then that I'll just disable the failing TCK test and open a PR.
But like I said, it won't be enough to fix the original problem, as it seems like the new rest-client does not register MyResponseExceptionMapper .
That you'll have to look into

[michalszynkiewicz]

have you checked with the project I attached or the one the reporter provided? The latter was missing @RegisterProvider(...) on the client side IIRC

[geoand]

Yes, I enabled that and still the mapper never seems to be engaged

[geoand]

Here it is: #16731

[michalszynkiewicz]

I'll see why the exception mapper is not triggered tomorrow.

[michalszynkiewicz]

@geoand, programmatically registered exception mapper works with your changes :)

I'll try to create a fix for mappers registered with @RegisterProvider today

[geoand]

Cool :)