Skip to content

Commit bce145c

Browse files
snicollsnicoll
committed
Merged HttpClient defaults with local customizations
Update HttpComponents wrapper to merge local customizations with the default of the current HttpClient instead of overriding everything. This is available as from HttpComponents 4.4. that exposes the default request config from the client via the Configurable interface. If the client does not implement such interface, the previous behaviour is applied Issue: SPR-12583
1 parent 3662ad4 commit bce145c

File tree

4 files changed

+205
-27
lines changed

4 files changed

+205
-27
lines changed

‎spring-web/src/main/java/org/springframework/http/client/HttpComponentsClientHttpRequestFactory.java‎

Lines changed: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,7 @@ public ClientHttpRequest createRequest(URI uri, HttpMethod httpMethod) throws IO
217217
config = ((Configurable) httpRequest).getConfig();
218218
}
219219
if (config == null) {
220-
config = this.requestConfig;
220+
config = createRequestConfig(client);
221221
}
222222
if (config != null) {
223223
context.setAttribute(HttpClientContext.REQUEST_CONFIG, config);
@@ -231,6 +231,43 @@ public ClientHttpRequest createRequest(URI uri, HttpMethod httpMethod) throws IO
231231
}
232232
}
233233

234+
/**
235+
* Create a default {@link RequestConfig} to use with the given client. Can
236+
* return {@code null} to indicate that no custom request config should be set
237+
* and the defaults of the {@link HttpClient} should be used.
238+
* <p>The default implementation tries to merge the defaults of the client with the
239+
* local customizations of this instance, if any.
240+
* @param client the client
241+
* @return the RequestConfig to use
242+
*/
243+
protected RequestConfig createRequestConfig(CloseableHttpClient client) {
244+
if (client instanceof Configurable) {
245+
RequestConfig clientRequestConfig = ((Configurable) client).getConfig();
246+
return mergeRequestConfig(clientRequestConfig);
247+
}
248+
return this.requestConfig;
249+
}
250+
251+
private RequestConfig mergeRequestConfig(RequestConfig defaultRequestConfig) {
252+
if (this.requestConfig == null) { // nothing to merge
253+
return defaultRequestConfig;
254+
}
255+
RequestConfig.Builder builder = RequestConfig.copy(defaultRequestConfig);
256+
int connectTimeout = this.requestConfig.getConnectTimeout();
257+
if (connectTimeout >= 0) {
258+
builder.setConnectTimeout(connectTimeout);
259+
}
260+
int connectionRequestTimeout = this.requestConfig.getConnectionRequestTimeout();
261+
if (connectionRequestTimeout >= 0) {
262+
builder.setConnectionRequestTimeout(connectionRequestTimeout);
263+
}
264+
int socketTimeout = this.requestConfig.getSocketTimeout();
265+
if (socketTimeout >= 0) {
266+
builder.setSocketTimeout(socketTimeout);
267+
}
268+
return builder.build();
269+
}
270+
234271
/**
235272
* Create a Commons HttpMethodBase object for the given HTTP method and URI specification.
236273
* @param httpMethod the HTTP method

‎spring-web/src/main/java/org/springframework/remoting/httpinvoker/HttpComponentsHttpInvokerRequestExecutor.java‎

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import org.apache.http.StatusLine;
2929
import org.apache.http.client.HttpClient;
3030
import org.apache.http.client.config.RequestConfig;
31+
import org.apache.http.client.methods.Configurable;
3132
import org.apache.http.client.methods.HttpPost;
3233
import org.apache.http.config.Registry;
3334
import org.apache.http.config.RegistryBuilder;
@@ -269,12 +270,39 @@ protected HttpPost createHttpPost(HttpInvokerClientConfiguration config) throws
269270
* Create a {@link RequestConfig} for the given configuration. Can return {@code null}
270271
* to indicate that no custom request config should be set and the defaults of the
271272
* {@link HttpClient} should be used.
273+
* <p>The default implementation tries to merge the defaults of the client with the
274+
* local customizations of the instance, if any.
272275
* @param config the HTTP invoker configuration that specifies the
273276
* target service
274277
* @return the RequestConfig to use
275278
*/
276279
protected RequestConfig createRequestConfig(HttpInvokerClientConfiguration config) {
277-
return (this.requestConfig != null ? this.requestConfig : null);
280+
HttpClient client = getHttpClient();
281+
if (client instanceof Configurable) {
282+
RequestConfig clientRequestConfig = ((Configurable) client).getConfig();
283+
return mergeRequestConfig(clientRequestConfig);
284+
}
285+
return this.requestConfig;
286+
}
287+
288+
private RequestConfig mergeRequestConfig(RequestConfig defaultRequestConfig) {
289+
if (this.requestConfig == null) { // nothing to merge
290+
return defaultRequestConfig;
291+
}
292+
RequestConfig.Builder builder = RequestConfig.copy(defaultRequestConfig);
293+
int connectTimeout = this.requestConfig.getConnectTimeout();
294+
if (connectTimeout >= 0) {
295+
builder.setConnectTimeout(connectTimeout);
296+
}
297+
int connectionRequestTimeout = this.requestConfig.getConnectionRequestTimeout();
298+
if (connectionRequestTimeout >= 0) {
299+
builder.setConnectionRequestTimeout(connectionRequestTimeout);
300+
}
301+
int socketTimeout = this.requestConfig.getSocketTimeout();
302+
if (socketTimeout >= 0) {
303+
builder.setSocketTimeout(socketTimeout);
304+
}
305+
return builder.build();
278306
}
279307

280308
/**

‎spring-web/src/test/java/org/springframework/http/client/HttpComponentsClientHttpRequestFactoryTests.java‎

Lines changed: 70 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import org.apache.http.HttpEntityEnclosingRequest;
2222
import org.apache.http.client.HttpClient;
2323
import org.apache.http.client.config.RequestConfig;
24+
import org.apache.http.client.methods.Configurable;
2425
import org.apache.http.client.methods.HttpUriRequest;
2526
import org.apache.http.client.protocol.HttpClientContext;
2627
import org.apache.http.impl.client.CloseableHttpClient;
@@ -29,6 +30,7 @@
2930
import org.springframework.http.HttpMethod;
3031

3132
import static org.junit.Assert.*;
33+
import static org.mockito.Mockito.*;
3234

3335
public class HttpComponentsClientHttpRequestFactoryTests extends AbstractHttpRequestFactoryTestCase {
3436

@@ -93,29 +95,80 @@ public void customHttpClientUsesItsDefault() throws Exception {
9395
}
9496

9597
@Test
96-
public void defaultSettingsOfHttpClientLostOnExecutorCustomization() throws Exception {
97-
CloseableHttpClient client = HttpClientBuilder.create()
98-
.setDefaultRequestConfig(RequestConfig.custom().setConnectTimeout(1234).build())
99-
.build();
98+
public void defaultSettingsOfHttpClientMergedOnExecutorCustomization() throws Exception {
99+
RequestConfig defaultConfig = RequestConfig.custom().setConnectTimeout(1234).build();
100+
CloseableHttpClient client = mock(CloseableHttpClient.class,
101+
withSettings().extraInterfaces(Configurable.class));
102+
Configurable configurable = (Configurable) client;
103+
when(configurable.getConfig()).thenReturn(defaultConfig);
104+
100105
HttpComponentsClientHttpRequestFactory hrf = new HttpComponentsClientHttpRequestFactory(client);
106+
assertSame("Default client configuration is expected", defaultConfig, retrieveRequestConfig(hrf));
101107

102-
URI uri = new URI(baseUrl + "/status/ok");
103-
HttpComponentsClientHttpRequest request = (HttpComponentsClientHttpRequest)
104-
hrf.createRequest(uri, HttpMethod.GET);
108+
hrf.setConnectionRequestTimeout(4567);
109+
RequestConfig requestConfig = retrieveRequestConfig(hrf);
110+
assertNotNull(requestConfig);
111+
assertEquals(4567, requestConfig.getConnectionRequestTimeout());
112+
// Default connection timeout merged
113+
assertEquals(1234, requestConfig.getConnectTimeout());
114+
}
105115

106-
assertNull("No custom config should be set with a custom HttpClient",
107-
request.getHttpContext().getAttribute(HttpClientContext.REQUEST_CONFIG));
116+
@Test
117+
public void localSettingsOverrideClientDefaultSettings() throws Exception {
118+
RequestConfig defaultConfig = RequestConfig.custom()
119+
.setConnectTimeout(1234).setConnectionRequestTimeout(6789).build();
120+
CloseableHttpClient client = mock(CloseableHttpClient.class,
121+
withSettings().extraInterfaces(Configurable.class));
122+
Configurable configurable = (Configurable) client;
123+
when(configurable.getConfig()).thenReturn(defaultConfig);
108124

109-
hrf.setConnectionRequestTimeout(4567);
110-
HttpComponentsClientHttpRequest request2 = (HttpComponentsClientHttpRequest)
111-
hrf.createRequest(uri, HttpMethod.GET);
112-
Object requestConfigAttribute = request2.getHttpContext().getAttribute(HttpClientContext.REQUEST_CONFIG);
113-
assertNotNull(requestConfigAttribute);
114-
RequestConfig requestConfig = (RequestConfig) requestConfigAttribute;
125+
HttpComponentsClientHttpRequestFactory hrf = new HttpComponentsClientHttpRequestFactory(client);
126+
hrf.setConnectTimeout(5000);
115127

116-
assertEquals(4567, requestConfig.getConnectionRequestTimeout());
117-
// No way to access the request config of the HTTP client so no way to "merge" our customizations
128+
RequestConfig requestConfig = retrieveRequestConfig(hrf);
129+
assertEquals(5000, requestConfig.getConnectTimeout());
130+
assertEquals(6789, requestConfig.getConnectionRequestTimeout());
131+
assertEquals(-1, requestConfig.getSocketTimeout());
132+
}
133+
134+
@Test
135+
public void mergeBasedOnCurrentHttpClient() throws Exception {
136+
RequestConfig defaultConfig = RequestConfig.custom()
137+
.setSocketTimeout(1234).build();
138+
final CloseableHttpClient client = mock(CloseableHttpClient.class,
139+
withSettings().extraInterfaces(Configurable.class));
140+
Configurable configurable = (Configurable) client;
141+
when(configurable.getConfig()).thenReturn(defaultConfig);
142+
143+
HttpComponentsClientHttpRequestFactory hrf = new HttpComponentsClientHttpRequestFactory() {
144+
@Override
145+
public HttpClient getHttpClient() {
146+
return client;
147+
}
148+
};
149+
hrf.setReadTimeout(5000);
150+
151+
RequestConfig requestConfig = retrieveRequestConfig(hrf);
118152
assertEquals(-1, requestConfig.getConnectTimeout());
153+
assertEquals(-1, requestConfig.getConnectionRequestTimeout());
154+
assertEquals(5000, requestConfig.getSocketTimeout());
155+
156+
// Update the Http client so that it returns an updated config
157+
RequestConfig updatedDefaultConfig = RequestConfig.custom()
158+
.setConnectTimeout(1234).build();
159+
when(configurable.getConfig()).thenReturn(updatedDefaultConfig);
160+
hrf.setReadTimeout(7000);
161+
RequestConfig requestConfig2 = retrieveRequestConfig(hrf);
162+
assertEquals(1234, requestConfig2.getConnectTimeout());
163+
assertEquals(-1, requestConfig2.getConnectionRequestTimeout());
164+
assertEquals(7000, requestConfig2.getSocketTimeout());
165+
}
166+
167+
private RequestConfig retrieveRequestConfig(HttpComponentsClientHttpRequestFactory factory) throws Exception {
168+
URI uri = new URI(baseUrl + "/status/ok");
169+
HttpComponentsClientHttpRequest request = (HttpComponentsClientHttpRequest)
170+
factory.createRequest(uri, HttpMethod.GET);
171+
return (RequestConfig) request.getHttpContext().getAttribute(HttpClientContext.REQUEST_CONFIG);
119172
}
120173

121174
@Test

‎spring-web/src/test/java/org/springframework/remoting/httpinvoker/HttpComponentsHttpInvokerRequestExecutorTests.java‎

Lines changed: 68 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020

2121
import org.apache.http.client.HttpClient;
2222
import org.apache.http.client.config.RequestConfig;
23+
import org.apache.http.client.methods.Configurable;
2324
import org.apache.http.client.methods.HttpPost;
2425
import org.apache.http.impl.client.CloseableHttpClient;
2526
import org.apache.http.impl.client.HttpClientBuilder;
@@ -90,23 +91,82 @@ public void customHttpClientUsesItsDefault() throws IOException {
9091
}
9192

9293
@Test
93-
public void defaultSettingsOfHttpClientLostOnExecutorCustomization() throws IOException {
94-
CloseableHttpClient client = HttpClientBuilder.create()
95-
.setDefaultRequestConfig(RequestConfig.custom().setConnectTimeout(1234).build())
96-
.build();
94+
public void defaultSettingsOfHttpClientMergedOnExecutorCustomization() throws IOException {
95+
RequestConfig defaultConfig = RequestConfig.custom().setConnectTimeout(1234).build();
96+
CloseableHttpClient client = mock(CloseableHttpClient.class,
97+
withSettings().extraInterfaces(Configurable.class));
98+
Configurable configurable = (Configurable) client;
99+
when(configurable.getConfig()).thenReturn(defaultConfig);
100+
97101
HttpComponentsHttpInvokerRequestExecutor executor =
98102
new HttpComponentsHttpInvokerRequestExecutor(client);
99-
100103
HttpInvokerClientConfiguration config = mockHttpInvokerClientConfiguration("http://fake-service");
101104
HttpPost httpPost = executor.createHttpPost(config);
102-
assertNull("No custom config should be set with a custom HttpClient", httpPost.getConfig());
105+
assertSame("Default client configuration is expected", defaultConfig, httpPost.getConfig());
103106

104107
executor.setConnectionRequestTimeout(4567);
105108
HttpPost httpPost2 = executor.createHttpPost(config);
106109
assertNotNull(httpPost2.getConfig());
107110
assertEquals(4567, httpPost2.getConfig().getConnectionRequestTimeout());
108-
// No way to access the request config of the HTTP client so no way to "merge" our customizations
109-
assertEquals(-1, httpPost2.getConfig().getConnectTimeout());
111+
// Default connection timeout merged
112+
assertEquals(1234, httpPost2.getConfig().getConnectTimeout());
113+
}
114+
115+
@Test
116+
public void localSettingsOverrideClientDefaultSettings() throws Exception {
117+
RequestConfig defaultConfig = RequestConfig.custom()
118+
.setConnectTimeout(1234).setConnectionRequestTimeout(6789).build();
119+
CloseableHttpClient client = mock(CloseableHttpClient.class,
120+
withSettings().extraInterfaces(Configurable.class));
121+
Configurable configurable = (Configurable) client;
122+
when(configurable.getConfig()).thenReturn(defaultConfig);
123+
124+
HttpComponentsHttpInvokerRequestExecutor executor =
125+
new HttpComponentsHttpInvokerRequestExecutor(client);
126+
executor.setConnectTimeout(5000);
127+
128+
HttpInvokerClientConfiguration config = mockHttpInvokerClientConfiguration("http://fake-service");
129+
HttpPost httpPost = executor.createHttpPost(config);
130+
RequestConfig requestConfig = httpPost.getConfig();
131+
assertEquals(5000, requestConfig.getConnectTimeout());
132+
assertEquals(6789, requestConfig.getConnectionRequestTimeout());
133+
assertEquals(-1, requestConfig.getSocketTimeout());
134+
}
135+
136+
@Test
137+
public void mergeBasedOnCurrentHttpClient() throws Exception {
138+
RequestConfig defaultConfig = RequestConfig.custom()
139+
.setSocketTimeout(1234).build();
140+
final CloseableHttpClient client = mock(CloseableHttpClient.class,
141+
withSettings().extraInterfaces(Configurable.class));
142+
Configurable configurable = (Configurable) client;
143+
when(configurable.getConfig()).thenReturn(defaultConfig);
144+
145+
HttpComponentsHttpInvokerRequestExecutor executor =
146+
new HttpComponentsHttpInvokerRequestExecutor() {
147+
@Override
148+
public HttpClient getHttpClient() {
149+
return client;
150+
}
151+
};
152+
executor.setReadTimeout(5000);
153+
HttpInvokerClientConfiguration config = mockHttpInvokerClientConfiguration("http://fake-service");
154+
HttpPost httpPost = executor.createHttpPost(config);
155+
RequestConfig requestConfig = httpPost.getConfig();
156+
assertEquals(-1, requestConfig.getConnectTimeout());
157+
assertEquals(-1, requestConfig.getConnectionRequestTimeout());
158+
assertEquals(5000, requestConfig.getSocketTimeout());
159+
160+
// Update the Http client so that it returns an updated config
161+
RequestConfig updatedDefaultConfig = RequestConfig.custom()
162+
.setConnectTimeout(1234).build();
163+
when(configurable.getConfig()).thenReturn(updatedDefaultConfig);
164+
executor.setReadTimeout(7000);
165+
HttpPost httpPost2 = executor.createHttpPost(config);
166+
RequestConfig requestConfig2 = httpPost2.getConfig();
167+
assertEquals(1234, requestConfig2.getConnectTimeout());
168+
assertEquals(-1, requestConfig2.getConnectionRequestTimeout());
169+
assertEquals(7000, requestConfig2.getSocketTimeout());
110170
}
111171

112172
@Test

0 commit comments

Comments
 (0)