Port resteasy fix for sub-resources

[maxr2011-tech11]

Port resteasy fix for sub-resources from resteasy/resteasy-microprofile#241

• Closes: classic resteasy-client: ensure sub-resources are also proxied #42851

[quarkus-bot]

Thanks for your pull request!

Your pull request does not follow our editorial rules. Could you have a look?

• title should preferably start with an uppercase character (if it makes sense!)

_{This message is automatically generated by a bot.}

[geoand]

Thanks a lot for tackling this!

I'll leave this to @manovotn and @jamezp to review. I just added a trivial comment.

Also keep in mind that when all the changes have been done, we'll need the commits to be squashed.

Finally, having a test verifying the change would be nice

[gsmet]

I concur it would be very nice to have a test. You should be able to add one in the deployment module (and worst case in the corresponding integration-tests module).

[manovotn]

@maxr2011-tech11 please try to add a test for this change. The resteasy-microprofile PR you linked also has one, so it should be a matter of mimicking that same scenario here in the deployment module of this extension.

Otherwise, it looks OK.

[maxr2011]

@manovotn just added the SubResourceTest to the deployment module of the resteasy-classic/resteasy-client extension. also I did rewrite it a bit to use the quarkus integrated rest client inside the internal quarkus junit module instead of the Arquillian extension which resteasy uses (not sure if this works out)

CC @geoand

[maxr2011]

@geoand
@manovotn

The pipeline checks report, that there are some tests failing, specifically those where a rest client is involved, so it has to be something in the code. No idea what it could be since I am not that deeply into the implementation of the rest client, maybe you could have a look or involve someone who has more insights?

CC @stesie

[manovotn]

@geoand @manovotn

The pipeline checks report, that there are some tests failing, specifically those where a rest client is involved, so it has to be something in the code. No idea what it could be since I am not that deeply into the implementation of the rest client, maybe you could have a look or involve someone who has more insights?

CC @stesie

First of all, the test you added doesn't pass.
You can run that easily locally by building quarkus and then running the test:

• Fast build with - mvn -Dquickly -Dno-test-modules -Dskip.gradle.build=true -T 4C
• Execute the test you added with - mvn clean test -Dtest=SubResourceTest -f extensions/resteasy-classic/resteasy-client/deployment/

From a quick glance, you forgot to register the exception mapper to the rest client builder. This affects two of the tests.
So instead of:

        try (final ClientRootResource rootResource = RestClientBuilder.newBuilder().baseUrl(url)
                .build(ClientRootResource.class)) {

You want to do:

        try (final ClientRootResource rootResource = RestClientBuilder.newBuilder().baseUrl(url)
                .register(TestExceptionMapper.class)
                .build(ClientRootResource.class)) {

[manovotn]

@maxr2011-tech11 I've pushed a commit fixing the test since I already had it locally.

I also added @Unremovable to the exception mapper - if it is supposed to be a bean (which I don't know?) you will need that as otherwise Quarkus will sweep it away as unused because it has no direct injection points.
Without the annotation, you can see a CDI warning in the output where some internals attempt to resolve it but fail.

[maxr2011]

@manovotn Just finalized the pull-request and fixed the issue inside the QuarkusProxyInvocationHandler:createProxy method to work correctly for the QuarkusRestClientBuilder, also cleaned up the Test.

-> Pipeline Checks should now be passing

CC @geoand @stesie

[geoand]

I have launched CI

[manovotn]

@manovotn Just finalized the pull-request and fixed the issue inside the QuarkusProxyInvocationHandler:createProxy method to work correctly for the QuarkusRestClientBuilder, also cleaned up the Test.

-> Pipeline Checks should now be passing

CC @geoand @stesie

Right, I was just looking into it.
I do have a silly question though - what is the difference between the test you are using now and the original one you added? You are no longer using QuarkusRestClientBuilder API. Does that make a difference?

Also, running the test locally I am seeing the following logged (which may or may not be due to how the test is written, looking into that):

2024-09-04 13:17:43,223 WARN [org.jbo.res.res.i18n] (main) RESTEASY002160: Provider instance io.quarkus.restclient.exception.SubResourceTest$TestExceptionMapper is already registered. 2nd registration is being ignored.

[maxr2011]

@manovotn Just finalized the pull-request and fixed the issue inside the QuarkusProxyInvocationHandler:createProxy method to work correctly for the QuarkusRestClientBuilder, also cleaned up the Test.
-> Pipeline Checks should now be passing
CC @geoand @stesie

Right, I was just looking into it. I do have a silly question though - what is the difference between the test you are using now and the original one you added? You are no longer using QuarkusRestClientBuilder API. Does that make a difference?

Also, running the test locally I am seeing the following logged (which may or may not be due to how the test is written, looking into that):

2024-09-04 13:17:43,223 WARN [org.jbo.res.res.i18n] (main) RESTEASY002160: Provider instance io.quarkus.restclient.exception.SubResourceTest$TestExceptionMapper is already registered. 2nd registration is being ignored.

Also to answer this one:
I was wrong, the QuarkusRestClientBuilder is used just like within the Annotations and is equivalent to
RestClientBuilder.newBuilder().baseUrl(url).build(ClientRootResource.class);

but is directly injected into here:

@RestClient
ClientRootResource clientRootResource;

Just found out during debugging. So it should work either way.

But if you want I can add additional tests that use the QuarkusRestClientBuilder instead of the Annotations (but I don't think that would be necessary, since also the other test classes in general prefer to use the Annotations)

[manovotn]

Just found out during debugging. So it should work either way.

Yes, I also found that out through debugging.
I was just curious why you changed it :)

But if you want I can add additional tests that use the QuarkusRestClientBuilder instead of the Annotations (but I don't think that would be necessary, since also the other test classes in general prefer to use the Annotations)

Since we now know it is used in either approach, you can keep it as is.

Assuming the CI passes, I have no further issues with this PR but we should wait for @jamezp to take a look as well.

[jamezp]

Does this need a CDI qualifying annotation?

[maxr2011]

will annotate it as 'Singleton' since it is also registered as @Provider it should be fine either way

[manovotn]

There is no annotation needed here but it doesn't really matter that it is present.

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 9161d88.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

[manovotn]

@maxr2011-tech11 thanks for contributing! 👍