remove httpclient caching from DefaultHttpClientFactory (#1416)

* remove httpclient caching from DefaultHttpClientFactory Signed-off-by: liga-oz <liga.ozolina@sap.com> * remove logcaptor Signed-off-by: liga-oz <liga.ozolina@sap.com> --------- Signed-off-by: liga-oz <liga.ozolina@sap.com>
SAP · Jan 3, 2024 · 92d1888 · 92d1888
1 parent 7ff9eea
commit 92d1888
Show file tree

Hide file tree

Showing 3 changed files with 2 additions and 66 deletions.
diff --git a/token-client/pom.xml b/token-client/pom.xml
@@ -111,12 +111,6 @@
 			<version>1.4.14</version>
 			<scope>test</scope>
 		</dependency>
-		<dependency>
-			<groupId>io.github.hakky54</groupId>
-			<artifactId>logcaptor</artifactId>
-			<version>2.9.2</version>
-			<scope>test</scope>
-		</dependency>
         <dependency>
             <groupId>org.wiremock</groupId>
             <artifactId>wiremock-standalone</artifactId>

diff --git a/token-client/src/main/java/com/sap/cloud/security/client/DefaultHttpClientFactory.java b/token-client/src/main/java/com/sap/cloud/security/client/DefaultHttpClientFactory.java
@@ -16,15 +16,10 @@
 import org.apache.http.impl.client.HttpClientBuilder;
 import org.apache.http.impl.client.HttpClients;
 import org.apache.http.impl.conn.PoolingHttpClientConnectionManager;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 import javax.net.ssl.SSLContext;
 import java.io.IOException;
 import java.security.GeneralSecurityException;
-import java.util.Collections;
-import java.util.HashSet;
-import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.TimeUnit;
 
@@ -41,16 +36,13 @@
  * implementation of {@link HttpClientFactory}.
  */
 public class DefaultHttpClientFactory implements HttpClientFactory {
-	private static final Logger LOGGER = LoggerFactory.getLogger(DefaultHttpClientFactory.class);
 
 	private static final int DEFAULT_TIMEOUT = (int) TimeUnit.SECONDS.toMillis(5);
 	private static final int DEFAULT_SOCKET_TIMEOUT = (int) TimeUnit.SECONDS.toMillis(30);
 	private static final int MAX_CONNECTIONS_PER_ROUTE = 20; // default is 2
 	private static final int MAX_CONNECTIONS = 200;
 	private final ConcurrentHashMap<String, SslConnection> sslConnectionPool = new ConcurrentHashMap<>();
 	private final org.apache.http.client.config.RequestConfig requestConfig;
-	// reuse ssl connections
-	final Set<String> httpClientsCreated = Collections.synchronizedSet(new HashSet<>());
 
 	public DefaultHttpClientFactory() {
 		requestConfig = org.apache.http.client.config.RequestConfig.custom()
@@ -64,15 +56,11 @@ public DefaultHttpClientFactory() {
 	@Override
 	public CloseableHttpClient createClient(ClientIdentity clientIdentity) throws HttpClientException {
 		String clientId = clientIdentity != null ? clientIdentity.getId() : null;
-		if (httpClientsCreated.contains(clientId)) {
-			LOGGER.warn("Application has already created HttpClient for clientId = {}, please check.", clientId);
-		}
-		httpClientsCreated.add(clientId);
 		HttpClientBuilder httpClientBuilder = HttpClients.custom().setDefaultRequestConfig(requestConfig);
 
 		if (clientId != null && clientIdentity.isCertificateBased()) {
-			SslConnection connectionPool = sslConnectionPool.computeIfAbsent(clientId,
-					s -> new SslConnection(clientIdentity));
+			SslConnection connectionPool = sslConnectionPool.compute(clientId,
+					(s,c) -> new SslConnection(clientIdentity));
 			return httpClientBuilder
 					.setConnectionManager(connectionPool.poolingConnectionManager)
 					.setSSLContext(connectionPool.context)

diff --git a/token-client/src/test/java/com/sap/cloud/security/client/DefaultHttpClientFactoryTest.java b/token-client/src/test/java/com/sap/cloud/security/client/DefaultHttpClientFactoryTest.java
@@ -8,15 +8,12 @@
 import com.github.tomakehurst.wiremock.WireMockServer;
 import com.sap.cloud.security.config.ClientCredentials;
 import com.sap.cloud.security.config.ClientIdentity;
-import nl.altindag.log.LogCaptor;
 import org.apache.commons.io.IOUtils;
 import org.apache.http.HttpHeaders;
 import org.apache.http.HttpStatus;
-import org.apache.http.client.HttpClient;
 import org.apache.http.client.ResponseHandler;
 import org.apache.http.client.methods.HttpGet;
 import org.apache.http.impl.client.CloseableHttpClient;
-import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
 import org.mockito.Mockito;
@@ -25,9 +22,7 @@
 import java.nio.charset.StandardCharsets;
 
 import static com.github.tomakehurst.wiremock.client.WireMock.*;
-import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertNotSame;
 import static org.mockito.Mockito.when;
 
 class DefaultHttpClientFactoryTest {
@@ -37,7 +32,6 @@ class DefaultHttpClientFactoryTest {
 	private static final ClientIdentity config = Mockito.mock(ClientIdentity.class);
 	private static final ClientIdentity config2 = Mockito.mock(ClientIdentity.class);
 	private final DefaultHttpClientFactory cut = new DefaultHttpClientFactory();
-	private static LogCaptor logCaptor;
 
 	@BeforeAll
 	static void setup() throws IOException {
@@ -50,48 +44,8 @@ static void setup() throws IOException {
 		when(config2.getKey()).thenReturn(readFromFile("/privateRSAKey.txt"));
 		when(config2.getCertificate()).thenReturn(readFromFile("/certificates.txt"));
 		when(config2.isCertificateBased()).thenCallRealMethod();
-
-		logCaptor = LogCaptor.forClass(DefaultHttpClientFactory.class);
-	}
-
-	@AfterEach
-	void tearDown() {
-		logCaptor.clearLogs();
 	}
 
-	@Test
-	void createHttpClient_sameClientId() {
-		HttpClient client1 = cut.createClient(config);
-		HttpClient client2 = cut.createClient(config);
-
-		assertNotSame(client1, client2);
-
-		assertEquals(1, cut.httpClientsCreated.size());
-	}
-
-	@Test
-	void createHttpClient_differentClientId() {
-		HttpClient client1 = cut.createClient(config);
-		HttpClient client2 = cut.createClient(config2);
-
-		assertNotSame(client1, client2);
-
-		assertEquals(2, cut.httpClientsCreated.size());
-	}
-
-	@Test
-	void assertWarnWhenCalledMoreThanOnce() {
-		cut.createClient(config);
-		cut.createClient(config2);
-		assertThat(logCaptor.getWarnLogs()).isEmpty();
-
-		cut.createClient(config);
-		assertThat(logCaptor.getWarnLogs()).hasSize(1);
-		assertThat(logCaptor.getWarnLogs().get(0))
-				.startsWith("Application has already created HttpClient for clientId = theClientId, please check.");
-
-		logCaptor.clearLogs();
-	}
 
 	private static String readFromFile(String file) throws IOException {
 		return IOUtils.resourceToString(file, StandardCharsets.UTF_8);