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

RestClient support #3930

Merged
merged 4 commits into from
Jan 17, 2025
Merged

Conversation

jacekbilski
Copy link
Contributor

Adds support for using RestClient to register an app, so far only RestTemplate or WebClient were supported.

I was trying to follow how the other two are implemented, configured and tested. My app using locally build snapshot version can register itself while having only RestClient configured.

As a bonus I fixed one typo. Also I've added logging exceptions when registration fails, it was difficult for me to figure out why registration was failing, when Spring Boot Admin was swallowing exceptions. But feel free to reject that part if you feel like it's too much.

Closes #3906.

@jacekbilski jacekbilski requested a review from a team as a code owner December 19, 2024 09:26
@jacekbilski jacekbilski changed the title Rest client support RestClient support Dec 19, 2024
@andreasfritz
Copy link
Contributor

Hi @jacekbilski
thanks for your contribution!

I have tested your code.
In my examples RestTemplateBuilder and RestClientBuilder are both present by default. So the conditions for both configurations are true. The new RestClient "wins" and gets created. This would be a breaking change for all users of the RestTemplate.
Can you please refine the conditions.

Thanks!

@jacekbilski
Copy link
Contributor Author

Hi @andreasfritz,
I've added @ConditionalOnMissingBean(RestTemplateBuilder.class) on top of RestClientRegistrationClientConfig, that should do the trick.
Unfortunately, this forces me to duplicate what's in this RestClientRegistrationClientConfig in my own application, because my app apparently does use RestTemplate to communicate with OIDC server. That means that Spring Boot will also create BlockingRegistrationClient instead of RestClientRegistrationClient, and RestTemplate is not configured to do authorized calls. But that's my problem. I have not found a way to "convince" Spring Boot to create RestClientRegistrationClient, apart from creating it by myself.
At least I don't need to maintain RestClientRegistrationClient on my side.

@jacekbilski
Copy link
Contributor Author

Hi @andreasfritz,
are there any chances you (or someone else) could review this again soon? I'd like to know if this is going in the right direction, plus I'd like to be able to use it in my app.

@hzpz
Copy link
Collaborator

hzpz commented Jan 14, 2025

Hey @jacekbilski,
sorry to keep you waiting. We recently discussed this pull request but haven't come to a conclusion yet.

These are our findings so far:

  • Your initial pull request (everything but the last commit) was indeed a breaking change but not exactly in the way we first thought. Since the auto-configuration for BlockingRegistrationClient creates its own RestTemplate (from its own RestTemplateBuilder) it is independent of any RestTemplate configured by the user. The RestClient was introduced with Spring Boot 3.2.0 and as @andreasfritz already pointed out, if both RestTemplateBuilder and RestClient.Builder beans are available (which should always be the case with Spring Boot >= 3.2.0), then Spring Boot Admin would create a RestClientRegistrationClient. Since RestClientRegistrationClientConfig uses an existing RestClient.Builder, Spring Boot Admin's behaviour might change if the user already configured the injected RestClient.Builder.
  • With your last commit, Spring Boot Admin would probably never create a RestClientRegistrationClient on its own unless you seriously mess with Spring Boot's auto-configuration (remember, with Spring Boot =>3.2.0 both RestTemplateBuilder and RestClient.Builder beans should always be available). That is why you had to copy RestClientRegistrationClientConfig to your application. That way, you force Spring Boot Admin's auto-configuration for a RegistrationClient to back off entirely.

We will discuss this pull request further on friday and will get back to you.

@jacekbilski
Copy link
Contributor Author

I do agree, that today RestClientRegistrationClient will never be created automatically. It would be my wish, but I understand your point, that that would be a breaking change and could cause issues for some users.

The more I think of it, the more doubts I have about SpringBootAdminClientAutoConfiguration.RestClientRegistrationClientConfig. If it's never triggered, at least in Spring Boot 3.2, why do we need it at all.

You're also right about the RestClient.Builder. It was also giving me an uneasy feeling. On one hand I'm using something someone prepared in a certain way, for whatever reason, and I should trust it, although I have no guarantee that it would work for me. On the other I cannot just create a new RestClient on my own, because I might be missing some special configuration. In my case, for example, RestClient alone, without RestTemplate would not work, because Spring Security uses RestTemplate to talk to OAuth Server, and my special RestClient configuration is exactly about adding to every call a bearer token that I receive from OAuth Server.

With this PR I was hoping to move some responsibility/code to an external project, so that I don't need to maintain it by myself. But now I'm not so sure its the best idea. I'll wait until Friday to see what you decide.

@SteKoe SteKoe force-pushed the RestClient_support branch from 2dff866 to 7cc80ad Compare January 17, 2025 07:12
@SteKoe SteKoe force-pushed the RestClient_support branch from 7cc80ad to 7c4d69a Compare January 17, 2025 07:12
@SteKoe
Copy link
Contributor

SteKoe commented Jan 17, 2025

We will merge this PR as potentially BREAKING change. As discussed with others, we will stick to our given versioning pattern and will not do a semantic version change here.

@andreasfritz andreasfritz enabled auto-merge (squash) January 17, 2025 07:22
@hzpz
Copy link
Collaborator

hzpz commented Jan 17, 2025

Hey @jacekbilski,
as @SteKoe already mentioned, we decided to drop your last commit and include the rest as a breaking change. This way, Spring Boot < 3.2.0 will still create a BlockingRegistrationClient, where as Spring Boot >= 3.2.0 will use the new RestClientRegistrationClient.

I do think it can be beneficial to just have to configure the RestClient.Builder instead of providing a complete RegistrationClient if you need some special request handling.

@andreasfritz andreasfritz merged commit 7442e7b into codecentric:master Jan 17, 2025
1 check passed
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 89.65517% with 3 lines in your changes missing coverage. Please review.

Project coverage is 82.26%. Comparing base (4f9d0ae) to head (7c4d69a).
Report is 1948 commits behind head on master.

Files with missing lines Patch % Lines
...config/SpringBootAdminClientAutoConfiguration.java 72.72% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3930      +/-   ##
============================================
- Coverage     83.64%   82.26%   -1.39%     
+ Complexity     1247     1227      -20     
============================================
  Files           156      164       +8     
  Lines          3644     4132     +488     
  Branches        258      262       +4     
============================================
+ Hits           3048     3399     +351     
- Misses          464      611     +147     
+ Partials        132      122      -10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jacekbilski
Copy link
Contributor Author

Thanks for the news. I'll be now waiting for the release and will use it ASAP.

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

Successfully merging this pull request may close these issues.

Use RestClientBuilder, if available, to create RegistrationClient
5 participants