Use provided HttpClient consistently

This commit harmonizes the behavior of HttpComponents5MessageSender to
reuse a provided HttpClient, be it provided via constructor or property.

Closes gh-1512

Author: snicoll

spring-ws-core/src/main/java/org/springframework/ws/transport/http/HttpComponents5MessageSender.java

@@ -63,16 +63,15 @@ public class HttpComponents5MessageSender extends AbstractHttpWebServiceMessageS
 
 	private static final String HTTP_CLIENT_ALREADY_SET = "httpClient already set";
 
-	private HttpClient httpClient;
+	private final HttpComponents5ClientFactory clientFactory;
 
-	private HttpComponents5ClientFactory clientFactory;
+	private HttpClient httpClient;
 
 	/**
 	 * Create a new instance of the {@code HttpClientMessageSender} with a default
 	 * {@link HttpClient} that uses a default {@link PoolingHttpClientConnectionManager}.
 	 */
 	public HttpComponents5MessageSender() {
-
 		this.clientFactory = new HttpComponents5ClientFactory();
 		this.clientFactory.setClientBuilderCustomizer(
 				httpClientBuilder -> httpClientBuilder.addRequestInterceptorFirst(new RemoveSoapHeadersInterceptor()));
@@ -89,7 +88,7 @@ public HttpComponents5MessageSender() {
 	 * @param httpClient the HttpClient instance to use for this sender
 	 */
 	public HttpComponents5MessageSender(HttpClient httpClient) {
-
+		this();
 		Assert.notNull(httpClient, "httpClient must not be null");
 		this.httpClient = httpClient;
 	}
@@ -98,23 +97,19 @@ public HttpComponents5MessageSender(HttpClient httpClient) {
 	 * * @see HttpComponents5ClientFactory#setAuthScope(AuthScope)
 	 */
 	public void setAuthScope(AuthScope authScope) {
-
 		if (getHttpClient() != null) {
 			throw new IllegalStateException(HTTP_CLIENT_ALREADY_SET);
 		}
-
 		this.clientFactory.setAuthScope(authScope);
 	}
 
 	/*
 	 * * @see HttpComponents5ClientFactory#setCredentials(Credentials)
 	 */
 	public void setCredentials(Credentials credentials) {
-
 		if (getHttpClient() != null) {
 			throw new IllegalStateException(HTTP_CLIENT_ALREADY_SET);
 		}
-
 		this.clientFactory.setCredentials(credentials);
 	}
 
@@ -127,63 +122,64 @@ public HttpClient getHttpClient() {
 
 	/**
 	 * Set the {@code HttpClient} used by this message sender.
+	 * <p>
+	 * This effectively disable any customization and does not change the given
+	 * {@code HttpClient} in any way. As such, it does not set timeouts, nor does it
+	 * {@linkplain HttpClientBuilder#addRequestInterceptorFirst(HttpRequestInterceptor)
+	 * add} the {@link RemoveSoapHeadersInterceptor}.
+	 * @param httpClient the HttpClient to use
 	 */
 	public void setHttpClient(HttpClient httpClient) {
 		this.httpClient = httpClient;
 	}
 
-	/*
-	 * * @see HttpComponents5ClientFactory#setConnectionTimeout(Duration)
+	/**
+	 * Set the timeout until a connection is established.
+	 * @see HttpComponents5ClientFactory#setConnectionTimeout(Duration)
 	 */
 	public void setConnectionTimeout(Duration timeout) {
-
 		if (getHttpClient() != null) {
 			throw new IllegalStateException(HTTP_CLIENT_ALREADY_SET);
 		}
-
 		this.clientFactory.setConnectionTimeout(timeout);
 	}
 
-	/*
-	 * * @see HttpComponents5ClientFactory#setReadTimeout(Duration)
+	/**
+	 * Set the socket read timeout for the underlying HttpClient.
+	 * @see HttpComponents5ClientFactory#setReadTimeout(Duration)
 	 */
 	public void setReadTimeout(Duration timeout) {
-
 		if (getHttpClient() != null) {
 			throw new IllegalStateException(HTTP_CLIENT_ALREADY_SET);
 		}
-
 		this.clientFactory.setReadTimeout(timeout);
 	}
 
-	/*
-	 * * @see HttpComponents5ClientFactory#setMaxTotalConnections(int)
+	/**
+	 * Sets the maximum number of connections allowed for the underlying HttpClient.
+	 * @see HttpComponents5ClientFactory#setMaxTotalConnections(int)
 	 */
 	public void setMaxTotalConnections(int maxTotalConnections) {
-
 		if (getHttpClient() != null) {
 			throw new IllegalStateException(HTTP_CLIENT_ALREADY_SET);
 		}
-
 		this.clientFactory.setMaxTotalConnections(maxTotalConnections);
 	}
 
-	/*
-	 * * @see HttpComponents5ClientFactory#setMaxConnectionsPerHost(Map)
+	/**
+	 * Sets the maximum number of connections per host for the underlying HttpClient.
+	 * @see HttpComponents5ClientFactory#setMaxConnectionsPerHost(Map)
 	 */
 	public void setMaxConnectionsPerHost(Map<String, String> maxConnectionsPerHost) {
-
 		if (getHttpClient() != null) {
 			throw new IllegalStateException(HTTP_CLIENT_ALREADY_SET);
 		}
-
 		this.clientFactory.setMaxConnectionsPerHost(maxConnectionsPerHost);
 	}
 
 	@Override
 	public void afterPropertiesSet() throws Exception {
-
-		if (this.clientFactory != null) {
+		if (getHttpClient() == null) {
 			this.httpClient = this.clientFactory.getObject();
 		}
 	}

spring-ws-core/src/test/java/org/springframework/ws/transport/http/HttpComponents5MessageSenderTest.java

@@ -25,30 +25,39 @@
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatCode;
 
+/**
+ * Tests for {@link HttpComponents5MessageSender}.
+ *
+ * @author Lars Uffmann
+ * @author Stephane Nicoll
+ */
 class HttpComponents5MessageSenderTest {
 
 	@Test
 	void afterPropertiesSetShouldProperlyInitializeHttpClient() throws Exception {
-
 		HttpComponents5MessageSender messageSender = new HttpComponents5MessageSender();
 		assertThat(messageSender.getHttpClient()).isNull();
-
-		Duration timeout = Duration.ofSeconds(1);
-		assertThatCode(() -> messageSender.setConnectionTimeout(timeout)).doesNotThrowAnyException();
+		messageSender.setConnectionTimeout(Duration.ofSeconds(1));
 
 		messageSender.afterPropertiesSet();
 		assertThat(messageSender.getHttpClient()).isNotNull();
 	}
 
 	@Test
-	void afterPropertiesSetShouldUseAlreadyProvidedHttpClientIfAvailable() throws Exception {
-
+	void afterPropertiesSetShouldUseAlreadyProvidedHttpClientIfAvailableWithConstructor() throws Exception {
 		CloseableHttpClient httpClient = HttpClientBuilder.create().build();
 		HttpComponents5MessageSender messageSender = new HttpComponents5MessageSender(httpClient);
+		assertThatCode(() -> messageSender.setConnectionTimeout(Duration.ofSeconds(1)))
+			.isInstanceOf(IllegalStateException.class);
+		messageSender.afterPropertiesSet();
+		assertThat(messageSender.getHttpClient()).isSameAs(httpClient);
+	}
 
-		Duration timeout = Duration.ofSeconds(1);
-		assertThatCode(() -> messageSender.setConnectionTimeout(timeout)).isInstanceOf(IllegalStateException.class);
-
+	@Test
+	void afterPropertiesSetShouldUseAlreadyProvidedHttpClientIfAvailableWithProperty() throws Exception {
+		CloseableHttpClient httpClient = HttpClientBuilder.create().build();
+		HttpComponents5MessageSender messageSender = new HttpComponents5MessageSender();
+		messageSender.setHttpClient(httpClient);
 		messageSender.afterPropertiesSet();
 		assertThat(messageSender.getHttpClient()).isSameAs(httpClient);
 	}