From 26a04ae7048583b8a9abd4851140e18e48b3a5fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C4=ABga?= <72249435+liga-oz@users.noreply.github.com> Date: Tue, 2 Jan 2024 19:11:19 +0100 Subject: [PATCH 1/2] remove httpclient caching from DefaultHttpClientFactory Signed-off-by: liga-oz --- .../client/DefaultHttpClientFactory.java | 16 +------ .../client/DefaultHttpClientFactoryTest.java | 46 ------------------- 2 files changed, 2 insertions(+), 60 deletions(-) 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 index 980d1e4cbe..1046809f6a 100644 --- 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,7 +36,6 @@ * 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); @@ -49,8 +43,6 @@ public class DefaultHttpClientFactory implements HttpClientFactory { private static final int MAX_CONNECTIONS = 200; private final ConcurrentHashMap sslConnectionPool = new ConcurrentHashMap<>(); private final org.apache.http.client.config.RequestConfig requestConfig; - // reuse ssl connections - final Set 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 index 14a8e19bcc..fd6aa80e98 100644 --- 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); From 6c305c5d332bdc08a63f0b0cc4902879482fc17d Mon Sep 17 00:00:00 2001 From: liga-oz Date: Wed, 3 Jan 2024 10:00:52 +0100 Subject: [PATCH 2/2] remove logcaptor Signed-off-by: liga-oz --- token-client/pom.xml | 6 ------ 1 file changed, 6 deletions(-) diff --git a/token-client/pom.xml b/token-client/pom.xml index 466c64daa4..9f804bef7b 100644 --- a/token-client/pom.xml +++ b/token-client/pom.xml @@ -111,12 +111,6 @@ 1.4.14 test - - io.github.hakky54 - logcaptor - 2.9.2 - test - org.wiremock wiremock-standalone