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 reactive: use ClientLogger bean, if present #33868

Merged
merged 1 commit into from
Jun 9, 2023
Merged

REST reactive: use ClientLogger bean, if present #33868

merged 1 commit into from
Jun 9, 2023

Conversation

mickroll
Copy link
Contributor

@mickroll mickroll commented Jun 7, 2023

With org.jboss.resteasy.reactive.client.api.ClientLogger, there is already an api to implement a custom REST reactive client logger. For now, the only way to use it, is to set it while building org.jboss.resteasy.reactive.client.impl.ClientBuilderImpl manually. All other REST reactive clients fall back to using org.jboss.resteasy.reactive.client.logging.DefaultClientLogger.

This PR adds bean pickup logic: if a CDI bean for ClientLogger is found, it is used. Otherwise, use DefaultClientLogger.

As Arc is not a dependency in the org.jboss.resteasy.reactive.client context (and shouldn't be if i understand correctly), i added the pickup logic to io.quarkus.rest.client.reactive.runtime.RestClientBuilderImpl, which in turn uses ClientBuilderImpl.
But there is another place where ClientBuilderImpl is used: io.quarkus.keycloak.admin.client.reactive.runtime.ResteasyReactiveClientProvider. So i added the pickup logic there as well.

The solution was discussed here: #33332

cc @michalvavrik, @geoand

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 7, 2023

Thanks for your pull request!

The title of 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.

@mickroll mickroll changed the title use ClientLogger bean, if present REST reactive: use ClientLogger bean, if present Jun 7, 2023
@geoand
Copy link
Contributor

geoand commented Jun 7, 2023

cc @Sgitario

Copy link
Contributor

@Sgitario Sgitario left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add a simple test case that ensures this works when:

  • Injecting the REST Client via @RestClient
  • Programmatically using QuarkusRestClientBuilderImpl
  • Programmatically using RestClientBuilderImpl

@github-actions
Copy link

github-actions bot commented Jun 7, 2023

🙈 The PR is closed and the preview is expired.

@quarkus-bot

This comment has been minimized.

1 similar comment
@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@mickroll
Copy link
Contributor Author

mickroll commented Jun 8, 2023

(sorry for all the force pushes. i currently don't have a working local dev environment for quarkus and the builds in my github codespace keep dying with exit code 143, suspected SIGTERM)

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@mickroll
Copy link
Contributor Author

mickroll commented Jun 8, 2023

I would add a simple test case that ensures this works when:
* Injecting the REST Client via @RestClient
* Programmatically using QuarkusRestClientBuilderImpl
* Programmatically using RestClientBuilderImpl

@Sgitario I added both a test using @RestClient and a test using QuarkusRestClientBuilderImpl. The latter internally delegates to RestClientBuilderImpl so i did not write an extra test for that. I tried to keep close to how ResponseExceptionMapper is tested.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@Sgitario
Copy link
Contributor

Sgitario commented Jun 9, 2023

I would add a simple test case that ensures this works when:

  • Injecting the REST Client via @RestClient
  • Programmatically using QuarkusRestClientBuilderImpl
  • Programmatically using RestClientBuilderImpl

@Sgitario I added both a test using @RestClient and a test using QuarkusRestClientBuilderImpl. The latter internally delegates to RestClientBuilderImpl so i did not write an extra test for that. I tried to keep close to how ResponseExceptionMapper is tested.

Please, be sure you build the modules you have modified using the Maven command before pushing the commit to avoid formatting issues. For example, running mvn clean install -Dquickly would do it.

Also, the linting issue with the documentation still stands.

@mickroll
Copy link
Contributor Author

mickroll commented Jun 9, 2023

Sorry for the push-mess in this PR.
I have finally managed to setup a local dev environment.

For the doc linting issue: I added "TIP: " in front of it and followed the recommendation to replace "via" with "through". The Documentation CI build is successful. Or am i missing something else there?

adds integration tests for ClientLogger
@mickroll mickroll requested a review from Sgitario June 9, 2023 08:12
@quarkus-bot
Copy link

quarkus-bot bot commented Jun 9, 2023

✔️ 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.

@Sgitario
Copy link
Contributor

Sgitario commented Jun 9, 2023

@michalvavrik do you want to take another look before merging this one?

@michalvavrik
Copy link
Member

thanks @Sgitario , keycloak-admin-client-reactive part LGTM

@Sgitario Sgitario merged commit 7157277 into quarkusio:main Jun 9, 2023
@quarkus-bot quarkus-bot bot added this to the 3.2 - main milestone Jun 9, 2023
@mickroll mickroll deleted the rest-reactive-custom-logger branch June 12, 2023 08:10
@mickroll
Copy link
Contributor Author

Thanks for your patience, @Sgitario and @michalvavrik

@Sgitario
Copy link
Contributor

Thanks for your patience, @Sgitario and @michalvavrik

Thanks to you for your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants