handled PR comments

Author: joviegas

http-clients/apache5-client/src/main/java/software/amazon/awssdk/http/apache5/Apache5HttpClient.java

@@ -454,7 +454,8 @@ public interface Builder extends SdkHttpClient.Builder<Apache5HttpClient.Builder
         Builder dnsResolver(DnsResolver dnsResolver);
 
         /**
-         * @deprecated this has been replaced with {{@link #tlsSocketStrategy(TlsSocketStrategy)}}
+         * @deprecated this has been replaced with {@link #tlsSocketStrategy(TlsSocketStrategy)}.
+         * This method is retained for backwards compatibility to ease migration from Apache HttpClient 4.5.x.
          * Configuration that defines a custom Socket factory. If set to a null value, a default factory is used.
          * <p>
          * When set to a non-null value, the use of a custom factory implies the configuration options TRUST_ALL_CERTIFICATES,
@@ -463,7 +464,6 @@ public interface Builder extends SdkHttpClient.Builder<Apache5HttpClient.Builder
         @Deprecated
         Builder socketFactory(ConnectionSocketFactory socketFactory);
 
-
         /**
          * Configure a custom TLS strategy for SSL/TLS connections.
          * This is the preferred method over the deprecated {@link #socketFactory(ConnectionSocketFactory)}.
@@ -532,7 +532,7 @@ private static final class DefaultBuilder implements Builder {
         private HttpRoutePlanner httpRoutePlanner;
         private CredentialsProvider credentialsProvider;
         private DnsResolver dnsResolver;
-        private ConnectionSocketFactory legacySocketFactory;
+        private ConnectionSocketFactory legacyConnectionSocketFactory;
         private TlsSocketStrategy tlsStrategy;
 
         private DefaultBuilder() {
@@ -656,8 +656,7 @@ public void setDnsResolver(DnsResolver dnsResolver) {
 
         @Override
         public Builder socketFactory(ConnectionSocketFactory socketFactory) {
-            this.legacySocketFactory = socketFactory;
-            this.tlsStrategy = null; // Clear any previously set strategy
+            this.legacyConnectionSocketFactory = socketFactory;
             return this;
         }
 
@@ -668,7 +667,6 @@ public void setSocketFactory(ConnectionSocketFactory socketFactory) {
         @Override
         public Builder tlsSocketStrategy(TlsSocketStrategy tlsSocketStrategy) {
             this.tlsStrategy = tlsSocketStrategy;
-            this.legacySocketFactory = null; // Clear any legacy factory
             return this;
         }
 
@@ -746,8 +744,8 @@ TlsSocketStrategy getEffectiveTlsStrategy() {
             if (tlsStrategy != null) {
                 return tlsStrategy;
             }
-            if (legacySocketFactory != null) {
-                return new ConnectionSocketFactoryToTlsStrategyAdapter(legacySocketFactory);
+            if (legacyConnectionSocketFactory != null) {
+                return new ConnectionSocketFactoryToTlsStrategyAdapter(legacyConnectionSocketFactory);
             }
             return null;
         }
@@ -758,6 +756,10 @@ private static class ApacheConnectionManagerFactory {
         public PoolingHttpClientConnectionManager create(Apache5HttpClient.DefaultBuilder configuration,
                                                  AttributeMap standardOptions) {
 
+            Validate.isTrue(configuration.tlsStrategy == null || configuration.legacyConnectionSocketFactory == null,
+                            "Cannot configure both tlsSocketStrategy and socketFactory. "
+                            + "The connectionSocketFactory is deprecated; use tlsSocketStrategy for new implementations.");
+
             TlsSocketStrategy tlsStrategy = getPreferredTlsStrategy(configuration, standardOptions);
 
             PoolingHttpClientConnectionManagerBuilder builder =

http-clients/apache5-client/src/main/java/software/amazon/awssdk/http/apache5/internal/conn/ConnectionSocketFactoryToTlsStrategyAdapter.java

@@ -23,6 +23,7 @@
 import org.apache.hc.client5.http.ssl.TlsSocketStrategy;
 import org.apache.hc.core5.http.protocol.HttpContext;
 import software.amazon.awssdk.annotations.SdkInternalApi;
+import software.amazon.awssdk.utils.Validate;
 
 /**
  * Adapter to wrap ConnectionSocketFactory as TlsSocketStrategy.
@@ -34,7 +35,7 @@ public class ConnectionSocketFactoryToTlsStrategyAdapter implements TlsSocketStr
     private final ConnectionSocketFactory socketFactory;
 
     public ConnectionSocketFactoryToTlsStrategyAdapter(ConnectionSocketFactory socketFactory) {
-        this.socketFactory = socketFactory;
+        this.socketFactory = Validate.paramNotNull(socketFactory, "socketFactory");
     }
 
     @Override

http-clients/apache5-client/src/test/java/software/amazon/awssdk/http/apache5/Apache5ClientTlsAuthTest.java

@@ -116,7 +116,7 @@ public void methodTeardown() {
     }
 
     @Test
-    public void canMakeHttpsRequestWhenKeyProviderConfigured() throws IOException {
+    public void prepareRequest_whenKeyProviderConfigured_successfullyMakesHttpsRequest() throws IOException {
         client = Apache5HttpClient.builder()
                 .tlsKeyManagersProvider(keyManagersProvider)
                 .build();
@@ -125,13 +125,13 @@ public void canMakeHttpsRequestWhenKeyProviderConfigured() throws IOException {
     }
 
     @Test
-    public void requestFailsWhenKeyProviderNotConfigured() throws IOException {
+    public void prepareRequest_whenKeyProviderNotConfigured_throwsSslException() throws IOException {
         client = Apache5HttpClient.builder().tlsKeyManagersProvider(NoneTlsKeyManagersProvider.getInstance()).build();
         assertThatThrownBy(() -> makeRequestWithHttpClient(client)).isInstanceOfAny(SSLException.class, SocketException.class);
     }
 
     @Test
-    public void authenticatesWithTlsProxy() throws IOException {
+    public void prepareRequest_whenTlsProxyConfigured_authenticatesSuccessfully() throws IOException {
         ProxyConfiguration proxyConfig = ProxyConfiguration.builder()
                 .endpoint(URI.create("https://localhost:" + wireMockServer.httpsPort()))
                 .build();
@@ -148,7 +148,7 @@ public void authenticatesWithTlsProxy() throws IOException {
     }
 
     @Test
-    public void defaultTlsKeyManagersProviderIsSystemPropertyProvider() throws IOException {
+    public void build_whenNoTlsKeyManagersProviderSet_usesSystemPropertyProvider() throws IOException {
         System.setProperty(SSL_KEY_STORE.property(), clientKeyStore.toAbsolutePath().toString());
         System.setProperty(SSL_KEY_STORE_TYPE.property(), CLIENT_STORE_TYPE);
         System.setProperty(SSL_KEY_STORE_PASSWORD.property(), STORE_PASSWORD);
@@ -164,7 +164,7 @@ public void defaultTlsKeyManagersProviderIsSystemPropertyProvider() throws IOExc
     }
 
     @Test
-    public void defaultTlsKeyManagersProviderIsSystemPropertyProvider_explicitlySetToNull() throws IOException {
+    public void build_whenTlsKeyManagersProviderExplicitlySetToNull_usesSystemPropertyProvider() throws IOException {
         System.setProperty(SSL_KEY_STORE.property(), clientKeyStore.toAbsolutePath().toString());
         System.setProperty(SSL_KEY_STORE_TYPE.property(), CLIENT_STORE_TYPE);
         System.setProperty(SSL_KEY_STORE_PASSWORD.property(), STORE_PASSWORD);
@@ -180,7 +180,7 @@ public void defaultTlsKeyManagersProviderIsSystemPropertyProvider_explicitlySetT
     }
 
     @Test
-    public void build_notSettingSocketFactory_configuresClientWithDefaultSocketFactory() throws Exception {
+    public void build_whenSocketFactoryNotSet_configuresDefaultSocketFactory() throws Exception {
         System.setProperty(SSL_KEY_STORE.property(), clientKeyStore.toAbsolutePath().toString());
         System.setProperty(SSL_KEY_STORE_TYPE.property(), CLIENT_STORE_TYPE);
         System.setProperty(SSL_KEY_STORE_PASSWORD.property(), STORE_PASSWORD);
@@ -212,7 +212,7 @@ public void build_notSettingSocketFactory_configuresClientWithDefaultSocketFacto
     }
 
     @Test
-    public void build_settingCustomSocketFactory_configuresClientWithGivenSocketFactory() throws IOException,
+    public void build_whenCustomSocketFactorySet_usesProvidedSocketFactory() throws IOException,
                                                                                                  NoSuchAlgorithmException,
                                                                                                  KeyManagementException {
         TlsKeyManagersProvider provider = FileStoreTlsKeyManagersProvider.create(clientKeyStore,
@@ -259,7 +259,7 @@ private HttpExecuteResponse makeRequestWithHttpClient(SdkHttpClient httpClient)
     }
 
     @Test
-    public void tls_strategy_configuration() throws Exception {
+    public void build_whenTlsSocketStrategyConfigured_usesProvidedStrategy() throws Exception {
         // Setup TLS context
         KeyManager[] keyManagers = keyManagersProvider.keyManagers();
         SSLContext sslContext = SSLContext.getInstance("TLS");
@@ -289,45 +289,32 @@ public void tls_strategy_configuration() throws Exception {
     }
 
     @Test
-    public void tls_strategy_overrides_legacy_factory() throws Exception {
+    public void build_whenBothTlsStrategyAndLegacyFactorySet_throwsIllegalArgumentException() throws Exception {
         // Setup TLS context
         KeyManager[] keyManagers = keyManagersProvider.keyManagers();
         SSLContext sslContext = SSLContext.getInstance("TLS");
         sslContext.init(keyManagers, null, null);
 
-        // Create spies for both approaches
+        // Create both socket factory and TLS strategy
         SSLConnectionSocketFactory legacyFactory = new SSLConnectionSocketFactory(
             sslContext,
             NoopHostnameVerifier.INSTANCE
         );
-        SSLConnectionSocketFactory legacyFactorySpy = Mockito.spy(legacyFactory);
 
         TlsSocketStrategy tlsStrategy = new SdkTlsSocketFactory(
             sslContext,
             NoopHostnameVerifier.INSTANCE
         );
-        TlsSocketStrategy tlsStrategySpy = Mockito.spy(tlsStrategy);
-
-        // Build client with both - TLS strategy should take precedence
-        client = Apache5HttpClient.builder()
-                                  .socketFactory(legacyFactorySpy)
-                                  .tlsSocketStrategy(tlsStrategySpy)  // This should override
-                                  .build();
-
-        // Make request
-        HttpExecuteResponse response = makeRequestWithHttpClient(client);
-        assertThat(response.httpResponse().isSuccessful()).isTrue();
-
-        // Verify only TLS strategy was used, not legacy
-        Mockito.verify(tlsStrategySpy).upgrade(
-            Mockito.any(Socket.class),
-            Mockito.anyString(),
-            Mockito.anyInt(),
-            Mockito.any(),
-            Mockito.any(HttpContext.class)
-        );
 
-        Mockito.verifyNoInteractions(legacyFactorySpy);
+        // Attempt to build client with both - should throw exception
+        assertThatThrownBy(() -> Apache5HttpClient.builder()
+                                                  .socketFactory(legacyFactory)
+                                                  .tlsSocketStrategy(tlsStrategy)
+                                                  .build())
+            .isInstanceOf(IllegalArgumentException.class)
+            .hasMessageContaining("Cannot configure both tlsSocketStrategy and socketFactory")
+            .hasMessageContaining("deprecated")
+            .hasMessageContaining("use tlsSocketStrategy");
     }
 
 }