Spring Provide a way to supply the client builder for an HttpServiceGroup

Shouldn't the ProxyHttpServiceGroup::createProxies method set the proxyFactoryBuilder's exchangeAdapter only if it isn't set already?

As already reported in https://github.com/spring-projects/spring-boot/issues/47855, I'm trying to integrate my existing RestClient and WebClient auto-configuration with what is added to Boot 4.0.0-RC1. However, whatever I configure with the following is ignored:

@Configuration
@ImportHttpServices(group = "bidule", types = {BiduleApi.class})
@ImportHttpServices(group = "machin", types = {MachinApi.class})
public class RestConfiguration {

  @Bean
  RestClientHttpServiceGroupConfigurer groupConfigurer(RestClient biduleClient,
      RestClient machinClient) {
    return groups -> {
      groups.forEachGroup((group, clientBuilder, factoryBuilder) -> {
        if ("bidule".equals(group.name())) {
          factoryBuilder.exchangeAdapter(RestClientAdapter.create(biduleClient));
        } else if ("machin".equals(group.name())) {
          factoryBuilder.exchangeAdapter(RestClientAdapter.create(machinClient));
        }
      });
    };
  }
}

Comment From: oscarfanchin

The issue has not been analyzed yet, I think it will take some time, for the auto config of the clients (without security) I use this starter httpexchange-spring-boot-starter, since I understood the new functionality that gives you problems it should replace it

Comment From: ch4mpy

@oscarfanchin, no, httpexchange-spring-boot-starter is not an alternative as: - As far as I know, it doesn't allow switching the underlying REST client @Bean to use (RestClient or WebClient) with one that isn't auto-configured by httpexchange-spring-boot-starter itself. In the sample above, biduleClient and machinClient are such beans. - Options for the underlying REST client bean auto-configured by httpexchange-spring-boot-starter are not enough for my needs. I happen to work behind an HTTP proxy, the APIs I call require different authorization mechanisms, and some internal APIs use self-signed SSL certificates, requiring me to tweak the SSL bundles.

Comment From: rstoyanchev

You mean to provide a fully configured client, but currently we assume the client is built through group configurer mechanism with participation from Boot (for properties, customizers, etc), Spring Security, Spring Cloud, etc, to bring advanced config like you mention under #35728.

We can support this, but setting the HttpExchangeAdatper is a rather indirect way. We could provide a first class option to set the client (instance) for a group, and it would signal to us that there is no need to initialize and build a client.

Is it typically one client instance per group?

Comment From: ch4mpy

We could provide a first class option to set the client (instance) for a group, and it would signal to us that there is no need to initialize and build a client

@rstoyanchev that would be great.

My need is to de-couple the group and client: - a client might be used by sevral groups - a client might be used not only by @HttpExchange proxies: the client used by a group could be injected and used directly by any @Component (like another @Service than the generated one or a @Controller) - the client used by a group might be configured upfront of the group definition (with something else than the "official" Boot DSL for @HttpExchange proxies)

I realized only this morning that new methods on the RestClient.Builder and WebClient.Builder to set all properties from another instance (or an already built client) could do the trick without touching the ProxyHttpServiceGroup.

If the client builder for a @HttpExchange proxy group can be configured from an already configured client (or builder), then we can reuse this already defined client(Builder) for several groups, and for injection in components.

Comment From: rstoyanchev

a client might be used for several groups

Could you elaborate on this? The way we define a group is: HTTP service interfaces that share the same client setup (and also HttpServiceProxyFactory).

I realized only this morning that new methods on the RestClient.Builder and WebClient.Builder to set all properties from another instance (or an already built client)

This is easily achievable, but it has downsides. It's also an indirect way of achieving the goal. It results in creating a client builder even though the client to use is already built, and is not easy to discover.

In addition, while I don't know your case to re-use a client for several groups, the builder copy approach would result in multiple client instances (with the same config) rather than re-using the same instance.

Comment From: rstoyanchev

The generic type on HttpServiceGroupConfigurer is for the client builder type, so rather than adding another one for the client type, this will have to be a callback to initialize the client builder. Something like:

@Bean
RestClientHttpServiceGroupConfigurer configurer(
        RestClient.Builder biduleClientBuilder, RestClient.Builder machinClientBuilder) {

    return groups -> {
        groups.filterByName("bidule").forEachClient(group -> biduleClientBuilder);
        groups.filterByName("machin").forEachClient(group -> machinClientBuilder);
    };
}

Comment From: ch4mpy

Could you elaborate on this? The way we define a group is: HTTP service interfaces that share the same client setup (and also HttpServiceProxyFactory).

I'm tempted to define groups based on the OpenAPI spec I used to generate the @HttpExchange interfaces from.

In the case where I have an OpenAPI spec per service, for instance users-service.openapi.json, quizzes-service.openapi.json, and online-payment-service.openapi.json, I would find handy to have something like:

@ImportHttpServices(group = "users", basePackages = "com.machin.user", client = "dmzClientWithCustomSslBundle")
@ImportHttpServices(group = "quizzes", basePackages = "com.machin.quiz", client = "dmzClientWithCustomSslBundle")
@ImportHttpServices(group = "online-payment", basePackages = "com.bidule.paymentorder", client = "webClientWithProxyConf")

Rather than

@ImportHttpServices(group = "dmz", basePackages = {"com.machin.user", "com.machin.quiz"}, client = "dmzClientWithCustomSslBundle")
@ImportHttpServices(group = "web", basePackages = "com.bidule.paymentorder", client = "webClientWithProxyConf")

Which I find less expressive and also less aligned with how I design services: the fact that services are deployed on the same network and can share the same client conf is an implementation detail that can change over time.

But I could leave with any of the options above. The main reason why I'd like the client(Builder) to be "injectable" is the 3rd of my points above. I moved from @FeignClient to generated @HttpExchange proxies more than 6 month ago. The projects I worked on required complicated client configuration, namely: - HTTP proxy (with authentication) - Basic and Bearer authorization (for the second, using the client credentials flow or forwarding the bearer in the security context of an oauth2ResourceServer) - timeouts tuning

Because what was expensive for me was the client configuration (not the @HttpExchange proxies instantiation), I created a starter to perform some client auto-configuration. So, @ImportHttpServices can simplify my projects REST configuration only if I can keep clients' auto-configuration.

Regarding your last comment, I'm not sure. Do you intend to change the type of the parameter expected by forEachClient, the ClientCallback::withClient signature, or to make somehow the ProxyHttpServiceGroup::clientBuilder property mutable (or provided at build time)?

The two options below would work out of the box for me ("my" client auto-configuration starter can expose builders instead of clients to provide application with complete control on pre-configured beans):

@Configuration
@ImportHttpServices(group = "users", basePackages = "com.machin.user")
@ImportHttpServices(group = "quizzes", basePackages = "com.machin.quiz")
@ImportHttpServices(group = "online-payment", basePackages = "com.bidule.paymentorder")
public class RestConfiguration {
  @Bean
  RestClientHttpServiceGroupConfigurer configurer(RestClient.Builder dmzClientWithCustomSslBundleBuilder,
      RestClient.Builder webClientWithProxyConfBuilder) {
    return groups -> {
      groups.filterByName("users", "quizzes")
          .forEachClient((group, clientBuilder) -> group.client(dmzClientWithCustomSslBundleBuilder));
      groups.filterByName("machin")
          .forEachClient((group, clientBuilder) -> clientBuilder.resetWith(webClientWithProxyConfBuilder));
    };
  }
}

But this is not so different from what I'm trying to do with forEachGroup in the oppening comment (and which should work already).

A client property (or clientBuider, or adapter) on @ImportHttpServices would probably be a bigger step forward in terms of usability as it would remove the need for writing a RestClientHttpServiceGroupConfigurer.

Comment From: ch4mpy

Also, will the ProxyHttpServiceGroup::createProxies method keep mutating the proxyFactoryBuilder (initial subject of this ticket, the improvement on the client conf was more the subject of https://github.com/spring-projects/spring-framework/issues/35728)?

To me this is a bug: with the current implementation, any configuration of the exchangeAdapter in a forEachGroup is ignored.

A quick fix could be to moving proxyFactoryBuilder.exchangeAdapter(initExchangeAdapter()) from the createProxies method to the constructor:

ProxyHttpServiceGroup(HttpServiceGroup group) {
  this.declaredGroup = group;
  this.groupAdapter = getGroupAdapter(group.clientType());
  this.clientBuilder = this.groupAdapter.createClientBuilder();
  // Initialize the proxyFactoryBuilder in the constructor
  this.proxyFactoryBuilder.exchangeAdapter(initExchangeAdapter());
} 

public Map<Class<?>, Object> createProxies() {
  Map<Class<?>, Object> map = new LinkedHashMap<>(httpServiceTypes().size());
  // Do not mutate the proxyFactoryBuilder here
  HttpServiceProxyFactory factory = this.proxyFactoryBuilder.build();
  httpServiceTypes().forEach(type -> map.put(type, factory.createClient(type)));
  return map;
}

Comment From: rstoyanchev

The exchange adapter cannot be created earlier because it needs the client instance, but we need to keep the client builder open for customizations till the end. In other words, the group configurer is in charge of creating the client, and overriding the exchange adapter is not feasible by design. At best we can detect and reject it as illegal.

I don't see how bean references to adapters on the import annotations in addition to beans for those adapters is better vs group configurer as a single mechanism for initializing (or supplying) the underlying client for each group. Perhaps you can consider rebasing your starter on the group configurers as well, similar to the way Spring Boot, Spring Cloud, and Spring Security use it provide advanced ways for initialiazation.

I've done some refactoring locally to prepare first, and on top of it I've added an InitializingClientCallback that works as a function to create the client builder to use. The snippet from my last comment shows how it would look.

Comment From: ch4mpy

@rstoyanchev the new void forEachClient(InitializingClientCallback<CB> callback); from 09105eb provides me with a straight-forward way to hook "my starter" into @ImportHttpServices by injecting the client builder. Thank you very much for that.

The exchange adapter cannot be created earlier because it needs the client instance

I had overlooked that. Still, I don't understand why we should initExchangeAdapter if it is already set. However, to check that, we would need an accessor on the HttpServiceProxyFactory.Builder.

the group configurer is in charge of creating the client, and overriding the exchange adapter is not feasible by design. At best we can detect and reject it as illegal.

I find it confusing that we can set anything on the HttpServiceProxyFactory.Builder but the exchange adapter.

If one chooses to override the exchange adapter, then he should understand that he's responsible for the underlying client.

I don't see how bean references to adapters on the import annotations in addition to beans for those adapters is better vs group configurer as a single mechanism for initializing (or supplying) the underlying client for each group

@Configuration
@ImportHttpServices(group = "users", basePackages = "com.machin.user")
@ImportHttpServices(group = "quizzes", basePackages = "com.machin.quiz")
@ImportHttpServices(group = "online-payment", basePackages = "com.bidule.paymentorder")
public class RestConfiguration {
  @Bean
  RestClientHttpServiceGroupConfigurer restClientBuilderHttpServiceGroupConfigurer(RestClient.Builder dmzClientWithCustomSslBundleBuilder,
      RestClient.Builder webClientWithProxyConfBuilder) {
    return groups -> {
      groups.filterByName("users", "quizzes").forEachClient((group) -> dmzClientWithCustomSslBundleBuilder));
      groups.filterByName("online-payment").forEachClient((group) -> webClientWithProxyConfBuilder);
    };
  }
}

vs

@Configuration
@ImportHttpServices(group = "users", basePackages = "com.machin.user", client = "dmzClientWithCustomSslBundleBuilder")
@ImportHttpServices(group = "quizzes", basePackages = "com.machin.quiz", client = "dmzClientWithCustomSslBundleBuilder")
@ImportHttpServices(group = "online-payment", basePackages = "com.bidule.paymentorder", client = "webClientWithProxyConf")
public class RestConfiguration {}

But I understand that there are some challanges (bean definition order and framework simplicity & maintainability).

Perhaps you can consider rebasing your starter on the group configurers

I am. I'll add an http-service-groups configuration property for each client to auto-configure. It will list the groups for which the client builder should be used. As a 1st step, I'll generate the definition for the restClientBuilderHttpServiceGroupConfigurer bean above. As a 2nd, I might make the definition of the client(Builder) bean definition optional (inline the conf in the group configurer), for the case where the configured application does not need the client elsewhere than auto-configured HTTP service groups.

Comment From: rstoyanchev

To elaborate on initExchangeAdapter, the goal is to create HttpServiceProxyFactory, and that includes client initialization by multiple parties. Setting the exchange adapter cuts this short and prevents collaborative participation. Any further attempt to customize the client would need to be ignored or prevented, and neither is great if it can be avoided.

The InitializingClientCallback that was added instead allows to provide a starting point for the client builder, but otherwise allows further customizations. This is in better keeping with the goal of collaborative initialization.

We could possibly create a base builder in HttpServiceProxyFactory that doesn't have a method to set the exchange adapter, and expose that from the group configurer callbacks. It would make it clear it's only customizations on either the client or the proxy factory builders that are possible, in addition to providing the starting point for a client builder.

Comment From: ch4mpy

@rstoyanchev now that Spring 7 and Boot 4 are out, I'm migrating apps.

I can't get .forEachClient to work because of the assertion at the beginning of ConfigurableGroup::applyClientCallback(HttpServiceGroupConfigurer.InitializingClientCallback<CB> callback): when using @ImportHttpServices, the clientBuilder is always initialised before I can call the groups configurer.

What is the reason for forbidding the replacement of the clientBuilder?

How should I provide an initial clientBuilder?

Comment From: rstoyanchev

The idea is InitializingClientCallback should be first in the order to create the clientBuilder. If the clientBuilder is already created, it means another configurer triggered default creation earlier by applying customizations, and those would be undone if the clientBuilder is replaced.

You can debug to see which configurer triggers default creation, and try using ordering.

Comment From: ch4mpy

As the ticket is around @ImportHttpServices, I omitted it in my last question. I should have been more specific. Sorry about that.

How should I provide an initial clientBuilder to a group declared using @ImportHttpServices?

The auto-configuration for @ImportHttpServices triggers the clientBuilder initialisation, which makes the following fail:

@Configuration
@ImportHttpServices(group = "users", basePackages = "com.machin.user")
@ImportHttpServices(group = "quizzes", basePackages = "com.machin.quiz")
@ImportHttpServices(group = "online-payment", basePackages = "com.bidule.paymentorder")
public class RestConfiguration {
  @Bean
  RestClientHttpServiceGroupConfigurer restClientBuilderHttpServiceGroupConfigurer(
      RestClient.Builder dmzClientWithCustomSslBundleBuilder,
      RestClient.Builder webClientWithProxyConfBuilder) {
    return groups -> {
      groups.filterByName("users", "quizzes").forEachClient((group) -> dmzClientWithCustomSslBundleBuilder));
      groups.filterByName("online-payment").forEachClient((group) -> webClientWithProxyConfBuilder);
    };
  }
}

those would be undone if the clientBuilder is replaced.

Isn't this what dependency injection is about? There are many places in the framework where dependencies can be replaced. I agree that the framework should provide auto-configuration with reasonable defaults. A DSL to tweak these defaults can be useful in some situations, but it should not prevent from taking full control on dependencies.

Comment From: rstoyanchev

In your example, you should be able to add @Order(Ordered.HIGHEST_PRECEDENCE) to the RestClientHttpServiceGroupConfigurer bean, and that should ensure it is applied first. However, at the moment PropertiesRestClientHttpServiceGroupConfigurer sets the same order for itself, and it's not possible to be ahead of it reliably. I've created https://github.com/spring-projects/spring-boot/issues/48296.

Isn't this what dependency injection is about? There are many places in the framework where dependencies can be replaced.

I don't see it that way. Here we have a higher level config mechanism that facilitates coordinated initialization among independent parties, and it's relevant to set expectations. A better analogy would be FactoryBean and BeanPostProcessor's.

In terms of ensuring the ordering, we could make the callbacks deferred to ensure InitializingCallback is applied first, and we may yet consider that depending on the outcome of the Boot issue. That said this is bit of niche case, and relying on ordering might be good enough. I am wondering, as you move towards using group configurers, whether the need for the initializing callback wlll also fade away for you.