quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.64k stars 2.64k forks source link

Remove @RestClient annotation and handle like normal bean #41188

Open R4V3NN0 opened 3 months ago

R4V3NN0 commented 3 months ago

Description

I wondered if it would be possible to remove the @RestClient annotation.

Currently I can use the RestClient in my class like this.

package org.acme;

import jakarta.ws.rs.GET;
import jakarta.ws.rs.Path;
import java.util.Set;
import org.eclipse.microprofile.rest.client.inject.RestClient;

@Path("/hello")
public class GreetingResource {

    @RestClient
    @Inject // this seems not to be mandatory 
    MyRemoteService myRemoteService;

    @GET
    public Set<MyRemoteService.Extension> hello()
    {
        return myRemoteService.getExtensionsById("io.quarkus:quarkus-rest");
    }
}

and the rest client is configured like this.

package org.acme;

import jakarta.enterprise.context.ApplicationScoped;
import org.eclipse.microprofile.rest.client.inject.RegisterRestClient;

import jakarta.ws.rs.GET;
import jakarta.ws.rs.Path;
import jakarta.ws.rs.QueryParam;
import java.util.List;
import java.util.Set;

@RegisterRestClient(baseUri = "https://stage.code.quarkus.io/api")
@ApplicationScoped
public interface MyRemoteService {

    @GET
    @Path("/extensions")
    Set<Extension> getExtensionsById(@QueryParam("id") String id);

    class Extension {
        public String id;
        public String name;
        public String shortName;
        public List<String> keywords;
    }
}

Now I thought to myself if we could completely remove the @RestClient annotation and instead handle it like a normal bean which can be injected with constructor injection. This would result in code like this

package org.acme;

import jakarta.ws.rs.GET;
import jakarta.ws.rs.Path;
import java.util.Set;

@Path("/hello")
public class GreetingResource {

    private final MyRemoteService myRemoteService;

    public GreetingResource(MyRemoteService myRemoteService) {
        this.myRemoteService = myRemoteService;
    }

    @GET
    public Set<MyRemoteService.Extension> hello()
    {
        return myRemoteService.getExtensionsById("io.quarkus:quarkus-rest");
    }
}

if we do it like this my other class does not even have to know that we are injecting a rest client instead of a normal bean. I think we could decouple both classes more from each other and testing may become easier if we could use constructor injection instead of field injection.

Does this make any sense or did I miss some real obvious points?

Implementation ideas

No response

manofthepeace commented 3 months ago

@RestClient is a qualifier. Perhaps more than that, but it does not prevent you do do constructor injection, you have to put the qualifier in the signature, as you would do with a "normal" qualified bean. In your exemple above, the following should work.

    public GreetingResource(@RestClient MyRemoteService myRemoteService) {
        this.myRemoteService = myRemoteService;
    }
quarkus-bot[bot] commented 3 months ago

/cc @cescoffier (rest-client)

geoand commented 3 months ago

I've personally wanted this for years, but IIRC the current behavior is what the Microprofile REST Client spec defines (right @radcortez ?).

Moreover, if we changed it, it could break existing users

hamburml commented 3 months ago

@Inject annotation can only be removed when using Quarkus, when using other frameworks you may need it. https://download.eclipse.org/microprofile/microprofile-rest-client-3.0/microprofile-rest-client-spec-3.0.html#restcdi

And for constructor injection you normally still need the @Inject annotation.

By the way, you are not forced to use these annotations. You can get a rest client via QuarkusRestClientBuilder or

CDI.current().select(MyServiceClient.class, RestClient.LITERAL).get();

I have not yet tried the latter. The mp spec mentions it.

radcortez commented 3 months ago

I've personally wanted this for years, but IIRC the current behavior is what the Microprofile REST Client spec defines (right @radcortez ?).

Actually, the REST Client specification states that the @RestClient annotation is only required when you have multiple beans of the same type. If you only have a single implementation for the bean type, the @RestClient annotation is optional.

Rest Client interfaces may be injected as CDI beans. The runtime must create a CDI bean for each interface annotated with RegisterRestClient. The bean created will include a qualifier @RestClient to differentiate the use as an API call against any other beans registered of the same type. Based on the rules of how CDI resolves bean, you are only required to use the qualifier if you have multiple beans of the same type. Any injection point or programmatic look up that uses the qualifier RestClient is expected to be resolved by the MicroProfile Rest Client runtime.

I've confirmed that our implementation is mandatory, so based on the spec text, I believe we can make it optional.

radcortez commented 3 months ago

BTW, if you use the programmatic API to create the REST Client and then a producer method, it gives the exact same result and the @RestClient annotation is not required. Something like:

@Produces
@Dependent
Api api() {
    return RestClientBuilder.newBuilder()
                            .baseUri(...)
                            .build(Api.class);
}

@Inject
Api Api;
geoand commented 3 months ago

Thanks @radcortez!

Rest Client interfaces may be injected as CDI beans. The runtime must create a CDI bean for each interface annotated with RegisterRestClient. The bean created will include a qualifier https://github.com/restclient to differentiate the use as an API call against any other beans registered of the same type. Based on the rules of how CDI resolves bean, you are only required to use the qualifier if you have multiple beans of the same type. Any injection point or programmatic look up that uses the qualifier RestClient is expected to be resolved by the MicroProfile Rest Client runtime.

I can think of a few ways to make this work, but I'ld like to hear from @mkouba and @manovotn on what the best way to do this would

mkouba commented 3 months ago

Thanks @radcortez!

Rest Client interfaces may be injected as CDI beans. The runtime must create a CDI bean for each interface annotated with RegisterRestClient. The bean created will include a qualifier https://github.com/restclient to differentiate the use as an API call against any other beans registered of the same type. Based on the rules of how CDI resolves bean, you are only required to use the qualifier if you have multiple beans of the same type. Any injection point or programmatic look up that uses the qualifier RestClient is expected to be resolved by the MicroProfile Rest Client runtime.

I can think of a few ways to make this work, but I'ld like to hear from @mkouba and @manovotn on what the best way to do this would

Well, the way I interpret the MP Rest Client spec is that the @RestClient qualifier is mandatory. Esp. the sentence "The bean created will include a qualifier @RestClient to differentiate the use as an API call against any other beans registered of the same type." is IMO pretty clear that the bean provided by MP Rest Client must include the @RestClient qualifier and if it does it cannot be used to satisfy an injection point with @Default qualifier.

The "Based on the rules of how CDI resolves bean, you are only required to use the qualifier if you have multiple beans of the same type." is IMO just a simplified explanation how CDI resolution works.

" Any injection point or programmatic look up that uses the qualifier RestClient is expected to be resolved by the MicroProfile Rest Client runtime." is again very clear that only injection points with @RestClient are handled by MP Rest Client.

Finally, there's another sentence below the examples snippets: "The qualifier is used to differentiate use cases of the interface that are managed by this runtime, versus use cases that may be managed by other runtimes." which again indicates that @RestClient is required.

manovotn commented 3 months ago

Rest Client interfaces may be injected as CDI beans. The runtime must create a CDI bean for each interface annotated with RegisterRestClient. The bean created will include a qualifier @restclient to differentiate the use as an API call against any other beans registered of the same type. Based on the rules of how CDI resolves bean, you are only required to use the qualifier if you have multiple beans of the same type. Any injection point or programmatic look up that uses the qualifier RestClient is expected to be resolved by the MicroProfile Rest Client runtime.

The wording here says that it will always contain the qualifier - in the case presented here, your bean will have type MyRemoteService and will have qualifiers [@Any, @RestClient]. However, the interpretation of CDI rules here is not very well written - it mentions a possible clash with any other (user defined) bean which would have the same type, in which case you must use qualifier. The case where you don't need to specify @RestClient is if you had something like @Inject @Any MyRemoteService.

The thing is, if you do:

@Inject
MyRemoteService

You are in fact asking for a bean with type MyRemoteService and with implied qualifier @Default - and default qualifier is only added if there is no custom qualifier, which isn't the case whenever you add @RestClient as well.

geoand commented 3 months ago

Interesting for sure.

For me just reading

Based on the rules of how CDI resolves bean, you are only required to use the qualifier if you have multiple beans of the same type

I was under the impression that not using the qualifier was allowed

mkouba commented 3 months ago

For me just reading

Based on the rules of how CDI resolves bean, you are only required to use the qualifier if you have multiple beans of the same type

I was under the impression that not using the qualifier was allowed

This is IMO confusing and shoult not be in the spec at all.

manovotn commented 3 months ago

Interesting for sure.

For me just reading

Based on the rules of how CDI resolves bean, you are only required to use the qualifier if you have multiple beans of the same type

I was under the impression that not using the qualifier was allowed

I agree that the spec wording is misleading but that's not how it will work - you can try with any custom bean with qualifier :) We might want to clarify that in the MP spec text - it seems to me that the initial idea was just to underline that the qualifier was added to prevent accidental type clashes during bean resolution (which is totally correct approach).

Just as an FYI, here's the CDI spec bit about @Default qualifier and why it's not going to be there.

geoand commented 3 months ago

Updating the wording of the MP REST spec would definitely be nice :)

manovotn commented 3 months ago

Updating the wording of the MP REST spec would definitely be nice :)

I'll make a note and send a PR

geoand commented 3 months ago

🙏

radcortez commented 3 months ago

As an alternative, can't we propose dropping the @RestClient as mandatory?

mkouba commented 3 months ago

As an alternative, can't we propose dropping the @RestClient as mandatory?

Hm, we should think carefully if it's really worth it. I don't think it's a big obstacle or something really annoying. Also in Quarkus, you can omit the @Inject for field injection and just use @RestClient MyRemoteService service (as mentioned in https://github.com/quarkusio/quarkus/issues/41188#issuecomment-2167520914). So no big difference.

Do we know how often is the rest client interface implemented by some other bean? Does it happen often/never?

If @RestClient is optional (on the injection point) then we'll have to provide a bean to satisfy an injection point without @RestClient. Now, if there's a bean Bravo that implements MyRemoteService as well then we'll get a deployment error - ambiguous dependency. How do we handle this breaking change? We could instruct the user to add a qualifier on the Bravo which would be acceptable only if we're sure that it only happens rarely.

manovotn commented 3 months ago

Martin is right, I don't think it's worth the hassle for a single annotation.

TBF the requirement to have it is completely correct from CDI perspective as it eliminates potential type ambiguity and clearly shows who own those beans.

That being said, from the top of my head, you could technically make it optional by:

manovotn commented 3 months ago

Here's the PR - https://github.com/eclipse/microprofile-rest-client/pull/382