Spring WebClient and RestClient should offer a way to customize ClientHttpConnector/ClientHttpRequestFactory rather than replacing it

Both WebClient and RestClient have a unique hook point to configure the ClientHttpConnector and ClientHttpRequestFactory respectively. There are several convenient features that are inconsistent due to the lack of an abstraction.

MockRestServiceServer is using a mock-based ClientHttpRequestFactory so as soon as you try to configure one in your code, tests no longer bind to the mock server. This is as innocent as the following code (using Spring Boot helper classes):

public class AvailabilityCheckClient {

    private final RestClient client;

    public AvailabilityCheckClient(RestClient.Builder builder, Duration readTimeout) {
        this.client = builder
            .requestFactory(ClientHttpRequestFactories
                .get(ClientHttpRequestFactorySettings.DEFAULTS.withReadTimeout(readTimeout)))
            .build();
    }
        // ...
}

Spring Boot has improved SSL support which is also working via this mechanism. Same situation where everything works as expected up to a point where you need to tune anything on the request factory. See https://github.com/spring-projects/spring-boot/issues/36263.

Courtesy of @wilkinsona, more related issues that provide more context when triaging this.

Comment From: snicoll

Brainstorming with @poutsma, I've tried to give a consumer callback a go, as RestClient.Builder does that in several places already. Something like:

RestClient.Builder requestFactory(Consumer<ClientHttpRequestFactory> requestFactoryConsumer);

Looking at org.springframework.boot.web.client.ClientHttpRequestFactories, depending on the settings and the actual implementation, you may have to provide those in the constructor of the factory so the consumer-based callback does not handle all use cases.

It almost looks like we'd need some sort of backdoor so that the connection factory for the mock server is always taking precedence. The linked spring boot use case is a little different, where auto-configured behavior is lost when creating a ClientHttpRequestFactorySettings instance from scratch to tune settings in user config.

Comment From: rstoyanchev

I think there are two related, but separate issues. One is about multiple parties trying to customize ClientHttpRequestFactory, and the other about MockRestServiceServer depending on a specific factory type.

For the first issue, ClientHttpRequestFactory implementations have a small number of properties themselves, but many more are on the HTTP client, and that's expected to be initialized externally. This is why initialization by multiple parties has to start with a way to configure a shared underlying HTTP client, and that is the goal for https://github.com/spring-projects/spring-boot/issues/41138.

Given a specially configured HTTP client instance, the ClientHttpRequestFactory must be created with it, which means the factory instance must be set on RestClient.Builder. A consumer callback could help to further customize the factory itself, and I'm open to that option, but I think the factory is also better initializated externally, e.g. to allow customization via properties. I think that's the goal for https://github.com/spring-projects/spring-boot/issues/36266 is.

For the second issue, MockRestServiceServer injects a specific request factory type that must not be changed or customized. That RestClient.Builder instance then becomes specific to testing, and must not be shared with alternative types of use. In that sense, https://github.com/spring-projects/spring-boot/issues/38832#issuecomment-1861700762 is less a workaround, and more what I would expect, but perhaps Boot's testing support can make this transparent (via customizer as well?) to ensure that the builder for testing is for testing only.

In summary, I see both issues as something that's better solved on the side of Boot.

Comment From: mdaepp

Elaborating on @snicoll thoughts, I think that the method signature should rather be

RestClient.Builder requestFactory(UnaryOperator<ClientHttpRequestFactory> requestFactoryOperator);

instead of

RestClient.Builder requestFactory(Consumer<ClientHttpRequestFactory> requestFactoryConsumer);

This would allow for additional usage patterns such as

restClientBuilder.requestFactory(BufferingClientHttpRequestFactory::new);

Comment From: rstoyanchev

@mdaepp that would be of limited use. ClientHttpRequestFactories expose only a small number of properties of the underlying HttpClient, and expect those to be initialized externally. Therefore, allowing shared initialization of the underlying client is a broader, and the preferred solution.

Comment From: mdaepp

@rstoyanchev I agree that shared initialization of the underlying HttpClient is desirable. From my understanding, that will be done with https://github.com/spring-projects/spring-boot/issues/41138. However, DefaultRestClientBuilder does not allow for retrieval of HttpClientRequestFactory once it's set (on the Boot side of things, this currently done in RestClientAutoConfiguration). And DefaultRestClientBuilder is both package private and final (probably for a good reason), so it can't be modified. Let's say that I want to add an interceptor that logs requests and responses. This could be done via a RestClientCustomizer that adds a corresponding interceptor to RestClient.Builder. However, for the logger to work, request and response must be buffered (support for buffering has been removed on ClientHttpRequestFactorySettings). I agree that the proposed method

RestClient.Builder requestFactory(UnaryOperator<ClientHttpRequestFactory> requestFactoryOperator);

is of limited use as far as modifying the ClientHttpRequestFactory is concerned. However, it would still be very useful to be able to wrap the HttpClientRequestFactory with BufferingClientHttpRequestFactory (if it's not already an instance of it) via the proposed method. To me, the setup of ClientHttpRequestFactory including the initialization of the underlying HttpClient is a different aspect from logging. I'd very much welcome if my proposal (or any other means to retrieve the HttpClientRequestFactory from RestClient.Builder) would be considered for the reasons stated above.

Comment From: rstoyanchev

Thanks for the feedback.

For buffering specifically, we have an improvement planned in #33785 that would make it a RestClient.Builder level feature, and it should eliminate the need to wrap a request factory for the most part. Hopefully that covers your concern, but if not please elaborate further.

Comment From: mdaepp

@rstoyanchev Yes it does. I was missing that part in DefaultRestClient. Thanks for pointing this out!

Comment From: rstoyanchev

I've created #34013 for the MockRestServiceServer side of things.

For customizing the ClientHttpConnector/ClientHttpRequestFactory, a Consumer on RestClient.Builder isn't very helpful without some common abstraction or otherwise you need to downcast first. Spring Boot 3.4 offers a model for customizations via ClientHttpRequestFactoryBuilder and ClientHttpConnectorFactory, so we'll leave it at that for now.