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

[sberyozkin]

Description
Now that the OIDC client PR has been merged, it is possible to register the providers with MP RestClient or even JAX-RS clients like this:

@A
public interface MyRestClient{}

where @A is an extension specific annotation which indirectly registers some provider with the client.
But the following is not possible yet:

@A
@B
public interface MyRestClient{}

where more than one provider is registered indirectly via the custom annotations.

Implementation ideas

It should be simple enough: RestClientBase in quarkus-rest-client accepts, via the build step, a single provider found through a custom annotation like @A. It just needs to be updated to accept a List/Set.
Perhaps a test extension may also need to be introduced which would register @A and @B associated providers

[ghost]

/cc @phillip-kruger

[VasilisAndritsoudis]

Hello, I am willing to give it a shot, but I will need some guidance in order to complete the task quickly and efficiently.

[sberyozkin]

@VasilisAndritsoudis Great, thanks. Please take your time, it does not need to be completed asap.
So here is what is supported now:

1. RestClientBase constructor accepts Class<?> annotationProvider - I think it should accept a List (or Set) of classes instead.
2. This provider class is found in the build step and can be traced starting from the optional RestClientAnnotationProviderBuildItem parameter.
3. The extension can associate a provider with the annotation like this.

So I suppose the quarkus-rest client code should be updated to accept an optional list (or set - whichever works) of RestClientAnnotationProviderBuildItem.
And to test it we should likely need a test extension which would register more than one provider like this, not sure about the details right now, CC @gsmet @radcortez

[VasilisAndritsoudis]

@sberyozkin Great! I will start working first on implementing it with a list (or set), and when I am ready with that I will need some more info on how to proceed with testing.

[VasilisAndritsoudis]

@sberyozkin Last question. Do I need to be assigned to this issue or can I continue without?

[geoand]

@VasilisAndritsoudis I assigned it to you :)

[sberyozkin]

Cool, thanks @geoand

[geoand]

YW!

[radcortez]

Hum, I had a quick look into the current code and I couldn't find anything that validates if a custom annotation is applied to a valid provider. In the case of OidcClientRequestFilter it is fine because it is controlled by us, but for user defined providers we probably need that validation.

If the validation is in some obscure place, disregard this comment :)

[sberyozkin]

@radcortez Hey Roberto, what do you mean ?

It is just a shortcut to @RegisterProvider(MyProvider.class) - do you mean RestClientProcessor in this case verifies MyProvider.class is a valid JAX-RS provider interface ? I haven't seen it though - but if it does then the same can be done when MyProvider.class is brought in via an annotation like @A.

[radcortez]

Yes, but I guess that RESTEasy will handle that validation then. I'm now wondering if this is done at build time / runtime.

[sberyozkin]

@radcortez Yes - so I'd just let RestEasy do it too in this case - it will just ignore an object if it is not one of the recognized providers - but we can also easily do a pre-check for a few interfaces like ClientRequestFilter, MBR/MBW

[VasilisAndritsoudis]

I have a few questions:

• Firstly where can I find an example of the usage of the RestClient with annotations (until now I have only found this which to me seems like its working fine with multiple annotations).
• Secondly I am having a hard time understanding how to start a project after building it with quarkus. The information provided here does not help me a lot.

From the looks of it I will need some guidance in order to complete this task. If you need someone more qualified you can tell me and I can release this assignment.

[sberyozkin]

@VasilisAndritsoudis No problems - take your time - have a look around for more docs, examples, etc. re your 2nd point you just need to create an Eclipse or IntelliJ workspace and start applying the proposed changes. Take a week or so to find your way around and ping us later on. Please review the linked to docs and this task description - what is proposed here (and already partially implemented) is not documented yet

Thanks, Sergey

[VasilisAndritsoudis]

@sberyozkin I am glad to hear that. In the following days I will try my best to get a grip on the project and then I can more freely proceed with the requested task.

Have a great day,
Vasilis

[jtama]

I guess it takes times for the doc to be updated on quarkus.io but you have now exemples to register a provider in the doc.
https://github.com/quarkusio/quarkus/blob/master/docs/src/main/asciidoc/rest-client.adoc.

I guess the idea now is to create a fake extension with a new custom annotation to register a provider, and then test that rest client with more than one custom annotation, say your dummy annotation and the oidc client's one is correctly configured.

[sberyozkin]

Yeah something like that though I guess it might simpler not to involve the OIDC code into it at this stage; thanks. Perhaps a single test extension which would support 2 annotations.

[VasilisAndritsoudis]

@sberyozkin
I believe I have done everything I can at the moment since I am having a lot of trouble building the project, for some reason I get a lot of errors involving dependencies that maven could not find...

Anyway I am gonna show you my proposed changes before I make a PR:

First of all I have changed the RestCliendBase constructor to accent an array list of annotation classes like so:

private final ArrayList<Class<?>> annotationProviders;

public RestClientBase(Class<?> proxyType, String baseUriFromAnnotation, String propertyPrefix,
            ArrayList<Class<?>> annotationProviders) {
        this.proxyType = proxyType;
        this.baseUriFromAnnotation = baseUriFromAnnotation;
        this.propertyPrefix = propertyPrefix;
        this.annotationProviders = annotationProviders;
    }

I then changed the method configureProviders so it works with an array of providers like so:

private void configureProviders(RestClientBuilder builder) {
        Optional<String> maybeProviders = getOptionalDynamicProperty(REST_PROVIDERS, String.class);
        if (maybeProviders.isPresent()) {
            registerProviders(builder, maybeProviders.get());
        }
        for (Class<?> annotationProvider : annotationProviders) {
            if (annotationProvider != null) {
                builder.register(annotationProvider);
            }
        }
    }

I then worked on RestClientProcessor where the processInterfaces accept an array list of RestClientAnnotationProviderBuildItem like so:

Optional<ArrayList<RestClientAnnotationProviderBuildItem>> restClientAnnotationProviders,

Finally I tweaked the code inside the register function to work with an array list of providers like so:

public void register(RegistrationContext registrationContext) {
                final Config config = ConfigProvider.getConfig();

                for (Map.Entry<DotName, ClassInfo> entry : interfaces.entrySet()) {
                    DotName restClientName = entry.getKey();
                    BeanConfigurator<Object> configurator = registrationContext.configure(restClientName);
                    // The spec is not clear whether we should add superinterfaces too - let's keep aligned with SmallRye for now
                    configurator.addType(restClientName);
                    configurator.addQualifier(REST_CLIENT);
                    final String configPrefix = computeConfigPrefix(restClientName.toString(), entry.getValue());
                    final ScopeInfo scope = computeDefaultScope(capabilities, config, entry, configPrefix);
                    if (restClientAnnotationProviders.isPresent()) {
                        for (RestClientAnnotationProviderBuildItem restClientAnnotationProvider : restClientAnnotationProviders.get()) {
                            final Class<?> annotationProvider = checkAnnotationProviders(entry.getValue(),
                                    restClientAnnotationProvider);
                            configurator.scope(scope);
                            configurator.creator(m -> {
                                // return new RestClientBase(proxyType, baseUri).create();
                                ResultHandle interfaceHandle = m.loadClass(restClientName.toString());
                                ResultHandle baseUriHandle = m.load(getAnnotationParameter(entry.getValue(), "baseUri"));
                                ResultHandle configPrefixHandle = m.load(configPrefix);
                                ResultHandle annotationProviderHandle = annotationProvider == null ? m.loadNull()
                                        : m.loadClass(annotationProvider);
                                ResultHandle baseHandle = m.newInstance(
                                        MethodDescriptor.ofConstructor(RestClientBase.class, Class.class, String.class, String.class,
                                                Class.class),
                                        interfaceHandle, baseUriHandle, configPrefixHandle, annotationProviderHandle);
                                ResultHandle ret = m.invokeVirtualMethod(
                                        MethodDescriptor.ofMethod(RestClientBase.class, "create", Object.class), baseHandle);
                                m.returnValue(ret);
                            });
                        }
                    }
                    configurator.destroyer(BeanDestroyer.CloseableDestroyer.class);
                    configurator.done();
                }
            }

I believe this is what you suggested I did and I have done my best to deliver. Since I am very new to the project I do not have the background knowledge to know if my proposed changes are at all correct, so I worked based on my general understanding of how it should work.
I would appreciate some feedback even if its negative and feel free to tell me if there is anything else I can do.

Thanks,
Vasilis

[sberyozkin]

Hi @VasilisAndritsoudis sorry for a delay, I'm watching all the issues by periodic manual checks :-).

Yes, that sounds good, only replace ArrayList with List in the parameter type declarations.

We now have another issue #14466 which depends on this fix.
How about this: can you please open your first PR and then we can verify it against a reproducer linked to at #14466 ?

Thanks

[sberyozkin]

@VasilisAndritsoudis I think you should also update

public final class RestClientAnnotationProviderBuildItem extends SimpleBuildItem {

to be a multi build item

public final class RestClientAnnotationProviderBuildItem extends MultiBuildItem {

to fix #14466.

Give it a try please to the PR in the next few days so that we can try/test it early next week - don't worry about the tests for now :-), we'll push the tests to your branch once the fix is verified

[VasilisAndritsoudis]

@sberyozkin Alright, to whom should I make the PR and on what branch?

[sberyozkin]

@VasilisAndritsoudis one needs to fork the Quarkus repository, create a branch, and open a PR from that branch. Please also check https://github.com/quarkusio/quarkus/blob/master/CONTRIBUTING.md