Use system properties in ApacheRestfulClientFactory (#2241)

* Use system properties in ApacheRestfulClientFactory

#2192

* Add changelog entry for #2192

hapi-fhir-client/src/main/java/ca/uhn/fhir/rest/client/apache/ApacheHttpClient.java

@@ -45,7 +45,7 @@ import ca.uhn.fhir.rest.server.exceptions.InternalErrorException;
  */
 public class ApacheHttpClient extends BaseHttpClient implements IHttpClient {
 
-	private HttpClient myClient;
+	private final HttpClient myClient;
 
 	public ApacheHttpClient(HttpClient theClient, StringBuilder theUrl, Map<String, List<String>> theIfNoneExistParams, String theIfNoneExistString, RequestTypeEnum theRequestType, List<Header> theHeaders) {
 		super(theUrl, theIfNoneExistParams, theIfNoneExistString, theRequestType, theHeaders);
@@ -89,8 +89,7 @@ public class ApacheHttpClient extends BaseHttpClient implements IHttpClient {
 
 	@Override
 	protected IHttpRequest createHttpRequest() {
-		IHttpRequest retVal = createHttpRequest((HttpEntity)null);
+		return createHttpRequest((HttpEntity)null);
-		return retVal;
 	}
 
 	@Override
@@ -101,19 +100,17 @@ public class ApacheHttpClient extends BaseHttpClient implements IHttpClient {
 		 * the newer ones for whatever reason.
 		 */
 		ByteArrayEntity entity = new ByteArrayEntity(content);
-		IHttpRequest retVal = createHttpRequest(entity);
+		return createHttpRequest(entity);
-		return retVal;
 	}
 
 	private ApacheHttpRequest createHttpRequest(HttpEntity theEntity) {
 		HttpRequestBase request = constructRequestBase(theEntity);
-		ApacheHttpRequest result = new ApacheHttpRequest(myClient, request);
+		return new ApacheHttpRequest(myClient, request);
-		return result;
 	}
 
 	@Override
 	protected IHttpRequest createHttpRequest(Map<String, List<String>> theParams) {
-		List<NameValuePair> parameters = new ArrayList<NameValuePair>();
+		List<NameValuePair> parameters = new ArrayList<>();
 		for (Entry<String, List<String>> nextParam : theParams.entrySet()) {
 			List<String> value = nextParam.getValue();
 			for (String s : value) {
@@ -122,8 +119,7 @@ public class ApacheHttpClient extends BaseHttpClient implements IHttpClient {
 		}
 
 		UrlEncodedFormEntity entity = createFormEntity(parameters);
-		IHttpRequest retVal = createHttpRequest(entity);
+		return createHttpRequest(entity);
-		return retVal;
 	}
 
 
@@ -136,11 +132,6 @@ public class ApacheHttpClient extends BaseHttpClient implements IHttpClient {
 		 * which one we use anyhow.
 		 */
 		ByteArrayEntity entity = new ByteArrayEntity(theContents.getBytes(Constants.CHARSET_UTF8));
-		IHttpRequest retVal = createHttpRequest(entity);
+		return createHttpRequest(entity);
-		return retVal;
 	}
-
-
-
-
 }

hapi-fhir-client/src/main/java/ca/uhn/fhir/rest/client/apache/ApacheRestfulClientFactory.java

@@ -100,8 +100,11 @@ public class ApacheRestfulClientFactory extends RestfulClientFactory {
 					.setProxy(myProxy)
 					.build();
 
-			HttpClientBuilder builder = HttpClients.custom().setConnectionManager(connectionManager)
+			HttpClientBuilder builder = getHttpClientBuilder()
-					.setDefaultRequestConfig(defaultRequestConfig).disableCookieManagement();
+					.useSystemProperties()
+					.setConnectionManager(connectionManager)
+					.setDefaultRequestConfig(defaultRequestConfig)
+					.disableCookieManagement();
 
 			if (myProxy != null && StringUtils.isNotBlank(getProxyUsername()) && StringUtils.isNotBlank(getProxyPassword())) {
 				CredentialsProvider credsProvider = new BasicCredentialsProvider();
@@ -118,6 +121,10 @@ public class ApacheRestfulClientFactory extends RestfulClientFactory {
 		return myHttpClient;
 	}
 
+	protected HttpClientBuilder getHttpClientBuilder() {
+		return HttpClients.custom();
+	}
+
 	@Override
 	protected void resetHttpClient() {
 		this.myHttpClient = null;

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_3_0/2192-apache-client-proxy.yaml

@@ -0,0 +1,4 @@
+---
+type: fix
+issue: 2192
+title: "ApacheRestfulClientFactory now uses system properties for proxy configuration"

hapi-fhir-structures-dstu2/pom.xml

@@ -143,6 +143,12 @@
 
 
 		<!-- UNIT TEST DEPENDENCIES -->
+		<dependency>
+			<groupId>org.junit-pioneer</groupId>
+			<artifactId>junit-pioneer</artifactId>
+			<version>1.1.0</version>
+			<scope>test</scope>
+		</dependency>
 		<dependency>
 			<groupId>net.sf.json-lib</groupId>
 			<artifactId>json-lib</artifactId>

hapi-fhir-structures-dstu2/src/test/java/ca/uhn/fhir/rest/client/apache/ApacheRestfulClientFactoryTest.java

@@ -1,9 +1,29 @@
 package ca.uhn.fhir.rest.client.apache;
 
-import static org.junit.jupiter.api.Assertions.assertEquals;
+import java.io.IOException;
-import static org.junit.jupiter.api.Assertions.fail;
+import java.util.function.Consumer;
 
+import static org.junit.jupiter.api.Assertions.*;
+import static org.mockito.Mockito.*;
+
+import org.apache.http.ConnectionReuseStrategy;
+import org.apache.http.HttpException;
+import org.apache.http.HttpHost;
+import org.apache.http.client.AuthenticationStrategy;
+import org.apache.http.client.UserTokenHandler;
+import org.apache.http.client.methods.*;
+import org.apache.http.client.protocol.HttpClientContext;
+import org.apache.http.conn.ConnectionKeepAliveStrategy;
+import org.apache.http.conn.HttpClientConnectionManager;
+import org.apache.http.conn.routing.HttpRoute;
+import org.apache.http.impl.client.HttpClientBuilder;
+import org.apache.http.impl.execchain.ClientExecChain;
+import org.apache.http.protocol.HttpProcessor;
+import org.apache.http.protocol.HttpRequestExecutor;
 import org.junit.jupiter.api.Test;
+import org.junitpioneer.jupiter.SetSystemProperty;
+import org.mockito.ArgumentCaptor;
+import org.mockito.internal.stubbing.defaultanswers.ReturnsDeepStubs;
 
 import ca.uhn.fhir.context.FhirContext;
 import ca.uhn.fhir.rest.client.exceptions.FhirClientConnectionException;
@@ -37,4 +57,78 @@ public class ApacheRestfulClientFactoryTest {
 			assertEquals("Failed to retrieve the server metadata statement during client initialization. URL used was http://127.0.0.1:22225metadata", e.getMessage());
 		}
 	}
+
+	@Test
+	public void testGetNativeHttpClientWithProvidedProxy() throws IOException, HttpException {
+		HttpRoute httpRoute = executeRequestAndCaptureHttpRoute(
+				f -> f.setProxy("theHost", 0));
+
+		HttpHost proxyHost = httpRoute.getProxyHost();
+		assertNotNull(proxyHost.getHostName());
+		assertEquals("theHost", proxyHost.getHostName());
+		assertEquals(0, proxyHost.getPort());
+	}
+
+	@Test
+	@SetSystemProperty(key = "http.proxyHost", value = "hostFromSystemProperty")
+	@SetSystemProperty(key = "http.proxyPort", value = "1234")
+	public void testGetNativeHttpClientWithSystemProxy() throws IOException, HttpException {
+		HttpRoute httpRoute = executeRequestAndCaptureHttpRoute(f -> {
+		});
+
+		HttpHost proxyHost = httpRoute.getProxyHost();
+		assertNotNull(proxyHost.getHostName());
+		assertEquals("hostFromSystemProperty", proxyHost.getHostName());
+		assertEquals(1234, proxyHost.getPort());
+	}
+
+	@Test
+	@SetSystemProperty(key = "http.proxyHost", value = "hostFromSystemProperty")
+	@SetSystemProperty(key = "http.proxyPort", value = "1234")
+	public void testGetNativeHttpClientWithSystemAndProvidedProxy() throws IOException, HttpException {
+		HttpRoute httpRoute = executeRequestAndCaptureHttpRoute(
+				f -> f.setProxy("providedProxy", 0));
+
+		HttpHost proxyHost = httpRoute.getProxyHost();
+		assertNotNull(proxyHost.getHostName());
+		assertEquals("providedProxy", proxyHost.getHostName());
+		assertEquals(0, proxyHost.getPort());
+	}
+
+	private HttpRoute executeRequestAndCaptureHttpRoute(Consumer<ApacheRestfulClientFactory> factoryConsumer)
+			throws IOException, HttpException {
+		ClientExecChain mainExec = mock(ClientExecChain.class);
+		ArgumentCaptor<HttpRoute> httpRouteCaptor = ArgumentCaptor.forClass(HttpRoute.class);
+		when(mainExec.execute(
+				any(HttpRoute.class),
+				any(HttpRequestWrapper.class),
+				any(HttpClientContext.class),
+				any(HttpExecutionAware.class)))
+				.thenReturn(mock(CloseableHttpResponse.class, new ReturnsDeepStubs()));
+
+		ApacheRestfulClientFactory testableSut = createTestableSut(mainExec);
+		factoryConsumer.accept(testableSut);
+		testableSut.getNativeHttpClient().execute(new HttpGet("http://somewhere.net"));
+
+		verify(mainExec).execute(
+				httpRouteCaptor.capture(),
+				any(HttpRequestWrapper.class),
+				any(HttpClientContext.class),
+				any(HttpExecutionAware.class));
+		return httpRouteCaptor.getValue();
+	}
+
+	private ApacheRestfulClientFactory createTestableSut(ClientExecChain mainExec) {
+		return new ApacheRestfulClientFactory() {
+			@Override
+			protected HttpClientBuilder getHttpClientBuilder() {
+				return new HttpClientBuilder() {
+					@Override
+					protected ClientExecChain createMainExec(HttpRequestExecutor requestExec, HttpClientConnectionManager connManager, ConnectionReuseStrategy reuseStrategy, ConnectionKeepAliveStrategy keepAliveStrategy, HttpProcessor proxyHttpProcessor, AuthenticationStrategy targetAuthStrategy, AuthenticationStrategy proxyAuthStrategy, UserTokenHandler userTokenHandler) {
+						return mainExec;
+					}
+				};
+			}
+		};
+	}
 }