Allow repeatable writes in StreamingHttpOutputMessage

[cachescrubber]

BufferingClientHttpRequestFactory creates requests which are not repeatable

A RestTemplate obtained form the following snippet will lose the default request configuration applied to the HttpComponentsClientHttpRequestFactory, in this case the setRedirectsEnabled(true) is not honored.

• Affected Version: spring-boot 3.2.0-M3
• Works with stable: spring-boot 3.1.4
• Affected http client library is Apache HttpComponents 5 (HttpComponentsClientHttpRequestFactory).

I prepared a reproducible example here. Just run com.example.httpclient5demo.Httpclient5DemoApplicationTests

		RestTemplate restTemplate = restTemplateBuilder
				.requestFactory(() -> {
					RequestConfig.Builder requestConfig = RequestConfig.custom()
							.setRedirectsEnabled(true);
					HttpClientBuilder httpClientBuilder = HttpClientBuilder.create()
							.setDefaultRequestConfig(requestConfig.build());
					HttpComponentsClientHttpRequestFactory requestFactory = new HttpComponentsClientHttpRequestFactory(httpClientBuilder.build());
					//return requestFactory;
					return new BufferingClientHttpRequestFactory(requestFactory);
				})
				.rootUri("https://httpbin.org")
				.build();
		String response = restTemplate.getForObject("/redirect-to?url=https://httpbin.org/base64/SFRUUEJJTiBpcyBhd2Vzb21l", String.class);
		assertThat(response).isEqualTo("HTTPBIN is awesome");

[wilkinsona]

Thanks for the report and sample project. This doesn't appear to be a Spring Boot problem. The following tests illustrate the behavior without using Spring Boot:

package com.example.httpclient5demo;

import static org.assertj.core.api.Assertions.assertThat;

import org.apache.hc.client5.http.config.RequestConfig;
import org.apache.hc.client5.http.impl.classic.HttpClientBuilder;
import org.junit.jupiter.api.Test;
import org.springframework.http.client.BufferingClientHttpRequestFactory;
import org.springframework.http.client.HttpComponentsClientHttpRequestFactory;
import org.springframework.web.client.RestTemplate;

class Httpclient5DemoApplicationTests {
	
	@Test
	void redirectsCanBeEnabled() {
		RestTemplate restTemplate = new RestTemplate();
		RequestConfig.Builder requestConfig = RequestConfig.custom()
				.setRedirectsEnabled(true);
		HttpClientBuilder httpClientBuilder = HttpClientBuilder.create()
				.setDefaultRequestConfig(requestConfig.build());
		HttpComponentsClientHttpRequestFactory requestFactory = new HttpComponentsClientHttpRequestFactory(httpClientBuilder.build());
		restTemplate.setRequestFactory(requestFactory);
		String response = restTemplate.getForObject("https://httpbin.org/redirect-to?url=https://httpbin.org/base64/SFRUUEJJTiBpcyBhd2Vzb21l", String.class);
		assertThat(response).isEqualTo("HTTPBIN is awesome");
	}

	@Test
	void redirectsCanBeEnabledWhenBuffering() {
		RestTemplate restTemplate = new RestTemplate();
		RequestConfig.Builder requestConfig = RequestConfig.custom()
				.setRedirectsEnabled(true);
		HttpClientBuilder httpClientBuilder = HttpClientBuilder.create()
				.setDefaultRequestConfig(requestConfig.build());
		HttpComponentsClientHttpRequestFactory requestFactory = new HttpComponentsClientHttpRequestFactory(httpClientBuilder.build());
		restTemplate.setRequestFactory(new BufferingClientHttpRequestFactory(requestFactory));
		String response = restTemplate.getForObject("https://httpbin.org/redirect-to?url=https://httpbin.org/base64/SFRUUEJJTiBpcyBhd2Vzb21l", String.class);
		assertThat(response).isEqualTo("HTTPBIN is awesome");
	}

}

redirectsCanBeEnabled passes but redirectsCanBeEnabledWhenBuffering fails. We'll transfer this to the Framework team so that they can take a look.

[cachescrubber]

Thank you @wilkinsona for analyzing the issue. I updated the example with your test cases.

[cachescrubber]

I spotted another issue. Basic Auth (401) and Proxy Auth (407) Challenges are not working when buffering is enabled.

	@Test
	void basicAuthCanBeEnabledWhenBuffering() {
		RestTemplate restTemplate = new RestTemplate();
		BasicCredentialsProvider credentialsProvider = new BasicCredentialsProvider();

		credentialsProvider.setCredentials(new AuthScope("httpbin.org", 443),
				new UsernamePasswordCredentials("demo", "secret".toCharArray()));

		HttpClientBuilder httpClientBuilder = HttpClientBuilder.create()
				.setDefaultCredentialsProvider(credentialsProvider);

		HttpComponentsClientHttpRequestFactory requestFactory = new HttpComponentsClientHttpRequestFactory(httpClientBuilder.build());
		restTemplate.setRequestFactory(new BufferingClientHttpRequestFactory(requestFactory));
		ResponseEntity<String> forEntity = restTemplate.getForEntity("https://httpbin.org//basic-auth/demo/secret", String.class);
		assertThat(forEntity.getStatusCode().is2xxSuccessful()).isTrue();
	}

The example has been updated with a test case.

[cachescrubber]

The issue is not a configuration issue, as I assumed initialy. I'll change the issue title accordingly.

It is caused by changes introduced in #30557 . BufferingClientHttpRequestWrapper is creating an empty body, which is passed to HttpComponents as org.springframework.http.client.HttpComponentsClientHttpRequest.BodyEntity which in turn implements org.apache.hc.core5.http.HttpEntity#isRepeatable to return false unconditionally.

HttpEntity#isRepeatable returning false disables all kind of functionality in the library where requests are repeated - redirects (3xx), authentication challenges (401,407) but also internal retries.

A quick fix would be to not pass in empty HttpComponentsClientHttpRequest.BodyEntity for requests where no body is expected (GET, HEAD, ...).

But I think the changes in #30557 are problematic in this regard. As it is implemented right now, even requests using the BufferingClientHttpRequestFactory are converted into a streaming body, even though they are in-memory anyway.

[cachescrubber]

Thanks @poutsma , my HttpComponents based test are good again. From reading the code I think at least OkHttp3ClientHttpRequest.BodyRequestBody#isOneShot need to be updated as well. Not sure for the other implementations.

[poutsma]

Thanks for spotting that, it should now be fixed.

[poutsma]

@cachescrubber I made some further related changes in 6dd93d4, which should enable repeatable writes for many types of request bodies, meaning that you typically don't need to wrap your request factory in a BufferingClientHttpRequestFactory any more. Please try a 6.1.0 Spring Framework snapshot and see if it works for you.

[cachescrubber]

Thanks for the heads-up. I use BufferingClientHttpRequestFactory to implement request/response logging in a ClientHttpRequestInterceptor. IIRC we needed the buffering in order to use response#getBody multiple times.

[karlovskiy]

Hi, I found this issue today during my investigation of redirects which stopped working after spring update.
I found that in this line we have
streamingOutputMessage.setBody(outputStream -> StreamUtils.copy(body, outputStream));
but if body was set like this with lambda than repeatable will be false.
Is it intended behavior ?
I thought that if we have byte array in InterceptingClientHttpRequest maybe body there also should be repeatable ?

PS: Currently, I resolved my redirect issue with wrapping HttpComponentsClientHttpRequestFactory with BufferingClientHttpRequestFactory.

[poutsma]

@karlovskiy well spotted, this should be fixed in 6.1.4.

[NicklasWallgren]

@poutsma

Are there any plans to add retryability support for multi-part requests inFormHttpMessageConverter#writeMultipart? The StreamingHttpOutputMessage#body lambda which is generated at

spring-framework/spring-web/src/main/java/org/springframework/http/converter/FormHttpMessageConverter.java

Line 488 in d8b506e

streamingOutputMessage.setBody(outputStream -> {

always return false

[poutsma]

Hi @NicklasWallgren,

I am no longer employed by Broadcom and no longer have commit rights on the Spring Framework, so I am not involved in its maintenance.