Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

REST client reactive should allow to instantiate clients based on (external) classes without @RegisterRestClient annotation #37007

Closed
lw-mcno opened this issue Nov 10, 2023 · 11 comments · Fixed by #37580
Labels
Milestone

Comments

@lw-mcno
Copy link

lw-mcno commented Nov 10, 2023

Description

During the migration from rest-client to rest-client-reactive, it has turned out, that the reactive doesn't allow to instantiate clients based on external (imported from dependencies) interface definitions (lacking the @RegisterRestClient annotation).

In the rest-client classic, this configuration alone is sufficient to instantiate a client bean (providing that MyRestServiceClass contains appropriate JAX-RS annotations):

rest-client:
  "myPackage.MyRestServiceClass":
    scope: jakarta.enterprise.context.Singleton
    url: ${my-service.url}

Implementation ideas

It was discussed in the Zulip chat, and suggested by @Ladicek, that such a functionality (automatically adding @RegisterRestClient during build time to classes injected into @RestClient-annotated slots), could be implemented using a build step.

@lw-mcno lw-mcno added the kind/enhancement New feature or request label Nov 10, 2023
Copy link

quarkus-bot bot commented Nov 10, 2023

/cc @cescoffier (rest-client), @geoand (rest-client)

@lw-mcno
Copy link
Author

lw-mcno commented Nov 10, 2023

@cescoffier, @geoand issue created as discussed.

@geoand
Copy link
Contributor

geoand commented Nov 10, 2023

So simply using the configuration above makes the classic rest client work? Nothing else involved?

@lw-mcno
Copy link
Author

lw-mcno commented Nov 10, 2023

Yes, with the same configuration, the classic client just instantiates and injects the bean, whereas the rest reactive client doesn't produce a candidate for injection (no client gets instantiated).

@geoand
Copy link
Contributor

geoand commented Nov 13, 2023

I personally think that enabling this feature from the old client makes sense.

@Serkan80
Copy link

The scope should be default Singleton imo and not required to be specified in the configuration.

Because only ApplicationScope is needed for mocking in tests and this can already be done.

@geoand
Copy link
Contributor

geoand commented Nov 24, 2023

@lw-mcno do you have an example of this working for RESTEasy Classic that I can use?

@lw-mcno
Copy link
Author

lw-mcno commented Nov 27, 2023

Hi @geoand

I've added a simple reproducer here: https://github.com/lw-mcno/quarkus-rest-client-classic-without-registration
The client interface would normally come from a dependency, but it's located in the repository for simplicity.
If you change the dependency to quarkus-rest-client-reactive-jackson, it will not allow to instantiate the client.

BTW. is it allowed to have reactive server and classic clients? From my experiments here, it doesn't seem so. When I had quarkus-resteasy (server) and rest-client-reactive (client) as dependencies, I've gotten a message, that it is not allowed to mix classic and reactive server (sic!) components.

@geoand
Copy link
Contributor

geoand commented Dec 7, 2023

@lw-mcno thanks.

So I just saw that the classic REST Client unconditionally registers the interface as a bean, even if there is no configuration. That is something I am not willing to add to the reactive version of the client.

However, I do see merit in the proposal to register the client as a bean when

quarkus:
  rest-client:
    "org.acme.ExampleRestService":
      scope: whatever

is set.

@geoand
Copy link
Contributor

geoand commented Dec 7, 2023

#37580 is what I have in mind

@lw-mcno
Copy link
Author

lw-mcno commented Dec 7, 2023

@geoand : Thanks for looking into this and great that the functionality can be extended to accomodate our use case.

geoand added a commit to geoand/quarkus that referenced this issue Dec 7, 2023
geoand added a commit to geoand/quarkus that referenced this issue Dec 7, 2023
geoand added a commit that referenced this issue Dec 7, 2023
Make REST Client a bean when the scope property is set in config
@quarkus-bot quarkus-bot bot added this to the 3.7 - main milestone Dec 7, 2023
holly-cummins pushed a commit to holly-cummins/quarkus that referenced this issue Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants