Improve converter config in RestClient.Builder

Internally maintain a chain of HttpMessageConverters.ClientBuilder
consumers in addition to the List of converters.

List based methods apply to the list.
HttpMessageConverters based methods are composed into a Consumer.

At build() time prepare a single HttpMessageConverters.ClientBuilder.
Insert list based converters first.
Apply HttpMessageConverters consumers after that.

Deprecate both List methods. Eventually, HttpMessageConverters should
be the main mechanism. In the mean time we layer them as described.

Closes gh-35578

Author: rstoyanchev

spring-web/src/main/java/org/springframework/web/client/DefaultRestClientBuilder.java

@@ -19,7 +19,6 @@
 import java.net.URI;
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.Collections;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
@@ -111,6 +110,8 @@ final class DefaultRestClientBuilder implements RestClient.Builder {
 
 	private @Nullable List<HttpMessageConverter<?>> messageConverters;
 
+	private @Nullable Consumer<HttpMessageConverters.ClientBuilder> convertersConfigurer;
+
 	private ObservationRegistry observationRegistry = ObservationRegistry.NOOP;
 
 	private @Nullable ClientRequestObservationConvention observationConvention;
@@ -142,6 +143,7 @@ public DefaultRestClientBuilder(DefaultRestClientBuilder other) {
 		this.initializers = (other.initializers != null) ? new ArrayList<>(other.initializers) : null;
 		this.requestFactory = other.requestFactory;
 		this.messageConverters = (other.messageConverters != null ? new ArrayList<>(other.messageConverters) : null);
+		this.convertersConfigurer = other.convertersConfigurer;
 		this.observationRegistry = other.observationRegistry;
 		this.observationConvention = other.observationConvention;
 	}
@@ -363,25 +365,30 @@ public RestClient.Builder requestFactory(ClientHttpRequestFactory requestFactory
 	@Override
 	@SuppressWarnings("removal")
 	public RestClient.Builder messageConverters(Consumer<List<HttpMessageConverter<?>>> configurer) {
-		configurer.accept(initMessageConverters());
+		if (this.messageConverters == null) {
+			this.messageConverters = new ArrayList<>();
+			HttpMessageConverters.forClient().registerDefaults().build().forEach(this.messageConverters::add);
+		}
+		configurer.accept(this.messageConverters);
 		validateConverters(this.messageConverters);
 		return this;
 	}
 
 	@Override
+	@SuppressWarnings("removal")
 	public RestClient.Builder messageConverters(Iterable<HttpMessageConverter<?>> messageConverters) {
 		validateConverters(messageConverters);
 		List<HttpMessageConverter<?>> converters = new ArrayList<>();
-		messageConverters.forEach(converter -> converters.add(converter));
-		this.messageConverters = Collections.unmodifiableList(converters);
+		messageConverters.forEach(converters::add);
+		this.messageConverters = converters;
 		return this;
 	}
 
 	@Override
 	public RestClient.Builder configureMessageConverters(Consumer<HttpMessageConverters.ClientBuilder> configurer) {
-		HttpMessageConverters.ClientBuilder clientBuilder = HttpMessageConverters.forClient();
-		configurer.accept(clientBuilder);
-		return messageConverters(clientBuilder.build());
+		this.convertersConfigurer = (this.convertersConfigurer != null ?
+				this.convertersConfigurer.andThen(configurer) : configurer);
+		return this;
 	}
 
 	@Override
@@ -403,20 +410,11 @@ public RestClient.Builder apply(Consumer<RestClient.Builder> builderConsumer) {
 		return this;
 	}
 
-	@SuppressWarnings("removal")
-	private List<HttpMessageConverter<?>> initMessageConverters() {
-		if (this.messageConverters == null) {
-			this.messageConverters = new ArrayList<>();
-			HttpMessageConverters.forClient().registerDefaults().build().forEach(this.messageConverters::add);
-		}
-		return this.messageConverters;
-	}
-
-	private void validateConverters(@Nullable Iterable<HttpMessageConverter<?>> messageConverters) {
-		Assert.notNull(messageConverters, "At least one HttpMessageConverter is required");
-		Assert.isTrue(messageConverters.iterator().hasNext(), "At least one HttpMessageConverter is required");
-		messageConverters.forEach(converter ->
-				Assert.notNull(converter, "The HttpMessageConverter list must not contain null elements"));
+	private void validateConverters(@Nullable Iterable<HttpMessageConverter<?>> converters) {
+		Assert.notNull(converters, "At least one HttpMessageConverter is required");
+		Assert.isTrue(converters.iterator().hasNext(), "At least one HttpMessageConverter is required");
+		converters.forEach(converter -> Assert.notNull(converter,
+				"The HttpMessageConverter list must not contain null elements"));
 	}
 
 
@@ -427,14 +425,12 @@ public RestClient.Builder clone() {
 
 	@Override
 	public RestClient build() {
+
 		ClientHttpRequestFactory requestFactory = initRequestFactory();
 		UriBuilderFactory uriBuilderFactory = initUriBuilderFactory();
-
 		HttpHeaders defaultHeaders = copyDefaultHeaders();
 		MultiValueMap<String, String> defaultCookies = copyDefaultCookies();
-
-		List<HttpMessageConverter<?>> converters =
-				(this.messageConverters != null ? this.messageConverters : initMessageConverters());
+		List<HttpMessageConverter<?>> converters = initMessageConverters();
 
 		return new DefaultRestClient(
 				requestFactory, this.interceptors, this.bufferingPredicate, this.initializers,
@@ -495,4 +491,22 @@ private UriBuilderFactory initUriBuilderFactory() {
 		return CollectionUtils.unmodifiableMultiValueMap(copy);
 	}
 
+	private List<HttpMessageConverter<?>> initMessageConverters() {
+		HttpMessageConverters.ClientBuilder builder = HttpMessageConverters.forClient();
+		if (this.messageConverters == null && this.convertersConfigurer == null) {
+			builder.registerDefaults();
+		}
+		else {
+			if (this.messageConverters != null) {
+				this.messageConverters.forEach(builder::customMessageConverter);
+			}
+			if (this.convertersConfigurer != null) {
+				this.convertersConfigurer.accept(builder);
+			}
+		}
+		List<HttpMessageConverter<?>> result = new ArrayList<>();
+		builder.build().forEach(result::add);
+		return result;
+	}
+
 }

spring-web/src/main/java/org/springframework/web/client/RestClient.java

@@ -449,31 +449,43 @@ Builder defaultStatusHandler(Predicate<HttpStatusCode> statusPredicate,
 
 		/**
 		 * Configure the message converters for the {@code RestClient} to use.
-		 * @param configurer the configurer to apply on the list of default
-		 * {@link HttpMessageConverter} pre-initialized
+		 * Multiple consumers are composed together and applied to a single
+		 * {@link HttpMessageConverters.ClientBuilder} instance.
+		 * @param configurer the configurer to apply on an empty {@link HttpMessageConverters.ClientBuilder}.
 		 * @return this builder
-		 * @see #messageConverters(Iterable)
-		 * @deprecated since 7.0 in favor of {@link #configureMessageConverters(Consumer)}
+		 * @since 7.0
 		 */
-		@Deprecated(since = "7.0", forRemoval = true)
-		Builder messageConverters(Consumer<List<HttpMessageConverter<?>>> configurer);
+		Builder configureMessageConverters(Consumer<HttpMessageConverters.ClientBuilder> configurer);
 
 		/**
-		 * Set the message converters for the {@code RestClient} to use.
-		 * @param messageConverters the list of {@link HttpMessageConverter} to use
+		 * Set the message converters to use.
+		 * <p><strong>Note:</strong> As of 7.0, the converters provided here
+		 * populate a {@link HttpMessageConverters.ClientBuilder} initially, and
+		 * after that the same builder is initialized further through the
+		 * configurers provided via {@link #configureMessageConverters(Consumer)}.
+		 * @param messageConverters the converters to use
 		 * @return this builder
 		 * @since 6.2
-		 * @see #configureMessageConverters(Consumer)
+		 * @deprecated since 7.0 in favor of {@link #configureMessageConverters(Consumer)}
 		 */
+		@Deprecated(since = "7.0", forRemoval = true)
 		Builder messageConverters(Iterable<HttpMessageConverter<?>> messageConverters);
 
 		/**
-		 * Configure the message converters for the {@code RestClient} to use.
-		 * @param configurer the configurer to apply on an empty {@link HttpMessageConverters.ClientBuilder}.
+		 * Customize the message converters to use, which is either the default
+		 * converters, or those provided via {@link #messageConverters(Iterable)}.
+		 * The consumer is applied immediately to the internal list
+		 * <p><strong>Note:</strong> As of 7.0, the list of converters customized
+		 * here is used to populate a {@link HttpMessageConverters.ClientBuilder}
+		 * initially, and after that the same builder is initialized further
+		 * through the configurers provided via {@link #configureMessageConverters(Consumer)}.
+		 * @param configurer the configurer to apply on the list of default
+		 * {@link HttpMessageConverter} pre-initialized
 		 * @return this builder
-		 * @since 7.0
+		 * @deprecated since 7.0 in favor of {@link #configureMessageConverters(Consumer)}
 		 */
-		Builder configureMessageConverters(Consumer<HttpMessageConverters.ClientBuilder> configurer);
+		@Deprecated(since = "7.0", forRemoval = true)
+		Builder messageConverters(Consumer<List<HttpMessageConverter<?>>> configurer);
 
 		/**
 		 * Configure the {@link io.micrometer.observation.ObservationRegistry} to use

spring-web/src/test/java/org/springframework/web/client/RestClientBuilderTests.java

@@ -112,6 +112,7 @@ void defaultUri() {
 		assertThat(fieldValue("baseUrl", defaultBuilder)).isEqualTo(baseUrl.toString());
 	}
 
+	@SuppressWarnings("removal")
 	@Test
 	void messageConvertersList() {
 		StringHttpMessageConverter stringConverter = new StringHttpMessageConverter();
@@ -126,13 +127,15 @@ void messageConvertersList() {
 				.containsExactly(stringConverter);
 	}
 
+	@SuppressWarnings("removal")
 	@Test
 	void messageConvertersListEmpty() {
 		RestClient.Builder builder = RestClient.builder();
 		List<HttpMessageConverter<?>> converters = Collections.emptyList();
 		assertThatIllegalArgumentException().isThrownBy(() -> builder.messageConverters(converters));
 	}
 
+	@SuppressWarnings("removal")
 	@Test
 	void messageConvertersListWithNullElement() {
 		RestClient.Builder builder = RestClient.builder();
@@ -147,9 +150,9 @@ void configureMessageConverters() {
 		RestClient.Builder builder = RestClient.builder();
 		builder.configureMessageConverters(clientBuilder -> clientBuilder.stringMessageConverter(stringConverter));
 		assertThat(builder).isInstanceOf(DefaultRestClientBuilder.class);
-		DefaultRestClientBuilder defaultBuilder = (DefaultRestClientBuilder) builder;
+		DefaultRestClient restClient = (DefaultRestClient) builder.build();
 
-		assertThat(fieldValue("messageConverters", defaultBuilder))
+		assertThat(fieldValue("messageConverters", restClient))
 				.asInstanceOf(InstanceOfAssertFactories.LIST)
 				.hasExactlyElementsOfTypes(StringHttpMessageConverter.class, AllEncompassingFormHttpMessageConverter.class);
 	}