Support more than one custom annotation which registers MP REST Client provider

[VasilisAndritsoudis]

Fixes #14141.
Fixes #14466.

[sberyozkin]

@VasilisAndritsoudis very good, thanks for giving it a try :-).
Can you please replace ArrayList parameter declarations with List for now ?
I'll experiment with your PR next week and may push some updates.
Nice work

[gsmet]

Thanks!

I added some comments. Also it would be better with a test and with all the commits stashed. Not sure how complex it would be to add a test with a local REST service?

[VasilisAndritsoudis]

I changed the parameter declaration as @gsmet suggested, however I am not sure what the other suggestion means.
Should I just delete the specified lines?

[VasilisAndritsoudis]

Oh sorry, I didn't realize the Optional was removed so the if should go. I am going to change this as well.

[VasilisAndritsoudis]

@sberyozkin I think I fixed everything you asked. If there is anything else let me know. :)

[sberyozkin]

@VasilisAndritsoudis

@sberyozkin I think I fixed everything you asked. If there is anything else let me know. :)

Can you please check my comment above about the for loop ?

@gsmet I reckon I should add a test extension to this branch which will register two different annotations; can also prototype a test which would verify the PR... Then I can ask @VasilisAndritsoudis to help to male sure the test passes

[sberyozkin]

@VasilisAndritsoudis I've squashed and also pushed some updates to make sure the multiple providers are picked up - please review the updates. We have an internal issue which can be resolved by accelerating this PR, so I've decided to give it a go. I'll work on some test support now, thanks

[VasilisAndritsoudis]

@sberyozkin Looks great and seems like its working. I hope I was of help to you :)

[sberyozkin]

@VasilisAndritsoudis Thanks, we'll need to test it.
Also - I'd like to thank you for your effort with this PR. I'd say it is not a simple good first issue so I appreciate you taking it on and giving it a good try. It saved me some time as the main code was already nearly ready.
Lets complete this PR, I'll push some tests.
In meantime - please also review other open issues - we look forward to more PRs from you :-)

[sberyozkin]

Good morning @gsmet, can you please restart this one for 1.12.0.CR1 ? I'm working on one more integration test (I'll ask Alexey for some guidance) but the existing one I've added to this branch is already sufficiently covering this new code as discussed on Zulip. #14466 would be good to resolve in time for 1.12.0.CR1, thanks

[sberyozkin]

Never mind :-), I pushed the prototype of the new test to this branch, so it is not ready for a restart yet

[sberyozkin]

@gsmet Hi, I've dropped the integration test prototype for now from this PR, as it should not really be holding it, if I can handle it with Alexey's help then I'd open a follow up PR. Hope it is ok to restart it, cancel if needed please

[sberyozkin]

@gsmet This is green now. One integration test is available. There is another which I'm going to work upon, given the feedback from Alexey (based on a dedicated test extension) - but it is not essential for this PR - it would only test a different variation of using the custom annotations - and I believe that there is no much time or bandwidth would be available until tomorrow anyway... thanks

Comments addressed.

[sberyozkin]

Hi @gsmet, and also @radcortez and @mkouba

Let me summarize what this PR does. It enhances a bit the earlier code introduced as part of the quarkus-oidc-client PR to support cases like

(1)

@RegisterRestClient
@AccessToken
interface MyClient1 {}

@RegisterRestClient
@OidcClientFilter
interface MyClient2 {}

This is specifically related to #14466 and is already tested in this PR

or
(2)

@RegisterRestClient
@AccessToken
@TraceCalls
interface MyClient1 {}

where in both cases the client providers are registered via custom annotations (as opposed to @RegisterProvider(SomeProvider.class))

To make it work a given extension registers one or more annotation:providerclass pair via a RestAnnotationProviderBuildItem

A few simple updates to RestAnnotationProviderBuildItem (it is multi item now), RestClientProcessor (I looked at how Martin was creating the array handles in Arc etc and did something similar) and also RestClientBase are done.

Integration test verifying the case (1) has been added.

@aloubyansky Alexey has explained how a second case (2) may be tested - it requires a test exception - I'm on it but it may take some time to fix - the integration test in this PR stresses all the added code already...

[sberyozkin]

@radcortez Roberto, I recall now we were also discussing verifying these are the valid providers - but indeed this is what Resteasy does anyway...So I thought of not duplicating it...

[sberyozkin]

@aloubyansky Hi Alexey - I've added one more extension based test to integration-tests/maven and I have to admit I'm simply awed, it is kind of tricky to get started :-), but then it just rocks, thanks for your help.

@radcortez @gsmet

So I've added one more missing test covering the case2 above:
https://github.com/quarkusio/quarkus/pull/14711/files#diff-c8d6cbd031069f059263db7a1a9f9f4c2f75528e519791926521e3dd35d72604

and

https://github.com/quarkusio/quarkus/pull/14711/files#diff-0714a2489cd058f71070f4403de9bfa443ec5343199755f9a9767f16dbed085c

here you can see a rest client with 2 custom annotations - it leads to 2 filters added and custom headers - checked...

[sberyozkin]

I propose backporting this PR: it only affects a very new and specific path within RestCliientProcessor/RestClientBase - and while the existing integration-tests/oidc-token-propagation has been updated to support multiple annotations, the other test, integration-tests/oidc-client which also depends on this feature has not been changed and works as expected - so it feels it is a safe update, it will just need to settle a bit in the snapshot...

[radcortez]

Do we really need to add the test extension in the maven integration module? Seem confusing, I would never expect to find something like this there.

Can't we create the extension in a proper integration-test module? It doesn't work?

[aloubyansky]

IMO, if the extension is test-specific, there is nothing wrong with that. We have other tests in the maven module that verify certain use-cases when extension modules are found in a multimodule application project.
OTOH, we do have one test extension under integration-tests - class-transformer but that one is referenced from multiple tests and its location may also be questionable in a sense that it's not actually a test module but a kind of common dependency.

[sberyozkin]

Hi @aloubyansky @radcortez thanks.
I can imagine if we start adding more and more specific (not quite general) cases to integration-tests/maven then it may overload integration-tests/maven. I don't really see it happening a lot as the specific cases requiring test extensions (like in this PR) and not common. It just happens that integration-tests/maven has the right infrastructure to make it work.
I don't mind having something like integration-tests/rest-client-provider-extension which would copy the relevant integration-tests/maven classes there and have only a single test extension inside. integration-tests/maven has some utils like DevMode utils etc, so I'd need to copy all of those classes.
Alexey @aloubyansky, if you think it may be worth it then I'll have np doing it. Just let me know please :-)

[radcortez]

I don't mind having something like integration-tests/rest-client-provider-extension which would copy the relevant integration-tests/maven classes there and have only a single test extension inside. integration-tests/maven has some utils like DevMode utils etc, so I'd need to copy all of those classes.

But does it require to have the maven infrastructure? Can't be just added like an extension in the integration tests and testes in the deployment module with QuarkusUnitTest and @RegisterExtension? Or that is not a reliable way to test this?

Anyway, I don't want to block this, I was just wondering if there was an easier way to do it :)

[sberyozkin]

@radcortez No, it does not work like that unfortunately :-). I've tried different options, tried to add the extension to test-framework, than to a new test-extensions module, before Alexey saved me from my attempts and pointed to integration-tests/maven.

I was actually returning here to propose to Alexey @aloubyansky to move some of the code to test-framework out of integration-tests/maven so that one can indeed easily embed the custom extension inside an individual integration-test/some-test module.

But until then I guess we should keep it in the current form - I'll be happy to migrate this test extension asap.
I guess I'll merge soon unless there will be more comments :-) please keep them flowing if you'd like, np, I'll keep PR open for another few hours in case more comments will be added. Also CC @gsmet

[radcortez]

Sure. Go ahead.

[sberyozkin]

@radcortez sounds good, thanks