Updated after self review

aws · Sep 30, 2023 · 0c791d8 · 0c791d8
1 parent de6e4dc
commit 0c791d8
Show file tree

Hide file tree

Showing 20 changed files with 307 additions and 290 deletions.
diff --git a/core/crt-core/src/main/java/software/amazon/awssdk/crtcore/CrtProxyConfiguration.java b/core/crt-core/src/main/java/software/amazon/awssdk/crtcore/CrtProxyConfiguration.java
@@ -15,12 +15,9 @@
 
 package software.amazon.awssdk.crtcore;
 
-import static software.amazon.awssdk.utils.ProxyConfigProvider.getProxyConfig;
+import static software.amazon.awssdk.utils.ProxyConfigProvider.fromSystemEnvironmentSettings;
 
-import java.util.Collections;
-import java.util.HashSet;
 import java.util.Objects;
-import java.util.Set;
 import software.amazon.awssdk.annotations.SdkPublicApi;
 import software.amazon.awssdk.utils.ProxyConfigProvider;
 import software.amazon.awssdk.utils.ProxySystemSetting;
@@ -46,8 +43,9 @@ protected CrtProxyConfiguration(DefaultBuilder<?> builder) {
         this.useEnvironmentVariableValues = builder.useEnvironmentVariableValues;
         this.scheme = builder.scheme;
 
-        ProxyConfigProvider proxyConfigProvider = getProxyConfig(builder.useSystemPropertyValues,
-                                                                 builder.useEnvironmentVariableValues ,builder.scheme);
+        ProxyConfigProvider proxyConfigProvider = fromSystemEnvironmentSettings(builder.useSystemPropertyValues,
+                                                                                builder.useEnvironmentVariableValues ,
+                                                                                builder.scheme);
         this.host = builder.host != null || proxyConfigProvider == null ? builder.host : proxyConfigProvider.host();
         this.port = builder.port != 0 || proxyConfigProvider == null ? builder.port : proxyConfigProvider.port();
         this.username = ! StringUtils.isEmpty(builder.username) || proxyConfigProvider == null ? builder.username :
@@ -201,6 +199,15 @@ public interface Builder {
          */
         Builder useSystemPropertyValues(Boolean useSystemPropertyValues);
 
+        /**
+         * The option whether to use environment variable values from {@link ProxySystemSetting} if any of the config options
+         * are missing. The value is set to "true" by default which means SDK will automatically use environment variable values
+         * if options are not provided during building the {@link CrtProxyConfiguration} object. To disable this behavior, set
+         * this value to false.
+         *
+         * @param useEnvironmentVariableValues The option whether to use environment variable values
+         * @return This object for method chaining.
+         */
         Builder useEnvironmentVariableValues(Boolean useEnvironmentVariableValues);
 
 
@@ -277,8 +284,6 @@ public B setUseEnvironmentVariableValues(Boolean useEnvironmentVariableValues) {
             return (B) this;
         }
 
-
-
         public void setUseSystemPropertyValues(Boolean useSystemPropertyValues) {
             useSystemPropertyValues(useSystemPropertyValues);
         }

diff --git a/...ts/apache-client/src/main/java/software/amazon/awssdk/http/apache/ProxyConfiguration.java b/...ts/apache-client/src/main/java/software/amazon/awssdk/http/apache/ProxyConfiguration.java
@@ -15,7 +15,7 @@
 
 package software.amazon.awssdk.http.apache;
 
-import static software.amazon.awssdk.utils.ProxyConfigProvider.getProxyConfig;
+import static software.amazon.awssdk.utils.ProxyConfigProvider.fromSystemEnvironmentSettings;
 import static software.amazon.awssdk.utils.StringUtils.isEmpty;
 
 import java.net.URI;
@@ -25,7 +25,6 @@
 import software.amazon.awssdk.annotations.SdkPublicApi;
 import software.amazon.awssdk.utils.ProxyConfigProvider;
 import software.amazon.awssdk.utils.ProxySystemSetting;
-import software.amazon.awssdk.utils.StringUtils;
 import software.amazon.awssdk.utils.ToString;
 import software.amazon.awssdk.utils.Validate;
 import software.amazon.awssdk.utils.builder.CopyableBuilder;
@@ -46,37 +45,42 @@ public final class ProxyConfiguration implements ToCopyableBuilder<ProxyConfigur
     private final Set<String> nonProxyHosts;
     private final Boolean preemptiveBasicAuthenticationEnabled;
     private final Boolean useSystemPropertyValues;
-    private final Boolean useEnvironmentVariablesValues;
     private final String host;
     private final int port;
     private final String scheme;
+    private final Boolean useEnvironmentVariablesValues;
 
     /**
      * Initialize this configuration. Private to require use of {@link #builder()}.
      */
     private ProxyConfiguration(DefaultClientProxyConfigurationBuilder builder) {
         this.endpoint = builder.endpoint;
-        String resolvedScheme = resolveScheme(builder);
+        String resolvedScheme =  endpoint != null ? endpoint.getScheme() : builder.scheme;
         this.scheme = resolvedScheme;
-        ProxyConfigProvider proxyConfiguration = getProxyConfig(builder.useSystemPropertyValues, builder.useEnvironmentVariableValues ,resolvedScheme);
-        this.username = !StringUtils.isEmpty(builder.username) || proxyConfiguration == null ? builder.username :
+        ProxyConfigProvider proxyConfiguration = fromSystemEnvironmentSettings(builder.useSystemPropertyValues,
+                                                                               builder.useEnvironmentVariableValues,
+                                                                               resolvedScheme);
+        this.username = !isEmpty(builder.username) || proxyConfiguration == null ? builder.username :
                         proxyConfiguration.userName().orElseGet(() -> builder.username);
-        this.password = !StringUtils.isEmpty(builder.password) || proxyConfiguration == null ? builder.password :
+        this.password = !isEmpty(builder.password) || proxyConfiguration == null ? builder.password :
                         proxyConfiguration.password().orElseGet(() -> builder.password);
         this.ntlmDomain = builder.ntlmDomain;
         this.ntlmWorkstation = builder.ntlmWorkstation;
         this.nonProxyHosts = builder.nonProxyHosts != null || proxyConfiguration == null ? builder.nonProxyHosts :
                              proxyConfiguration.nonProxyHosts();
         this.preemptiveBasicAuthenticationEnabled = builder.preemptiveBasicAuthenticationEnabled == null ? Boolean.FALSE :
-                builder.preemptiveBasicAuthenticationEnabled;
+                                                    builder.preemptiveBasicAuthenticationEnabled;
         this.useSystemPropertyValues = builder.useSystemPropertyValues;
         this.useEnvironmentVariablesValues = builder.useEnvironmentVariableValues;
-        this.host = resolveHost(scheme , proxyConfiguration);
-        this.port = resolvePort(proxyConfiguration);
+        if (this.endpoint != null) {
+            this.host = endpoint.getHost();
+            this.port = endpoint.getPort();
+        } else {
+            this.host = proxyConfiguration != null ? proxyConfiguration.host() : null;
+            this.port = proxyConfiguration != null ? proxyConfiguration.port() : 0;
+        }
     }
 
-
-
     /**
      * Returns the proxy host name from the configured endpoint if set, else from the "https.proxyHost" or "http.proxyHost" system
      * property, based on the scheme used, if {@link Builder#useSystemPropertyValues(Boolean)} is set to true.
@@ -193,29 +197,6 @@ public String toString() {
                        .build();
     }
 
-
-    private String resolveHost(String scheme, ProxyConfigProvider proxyConfigProvider) {
-        if (endpoint != null) {
-            return endpoint.getHost();
-        }
-        return proxyConfigProvider.host();
-    }
-
-    private int resolvePort(ProxyConfigProvider proxyConfigProvider) {
-        int port = 0;
-
-        if (endpoint != null) {
-            port = endpoint.getPort();
-        } else if(proxyConfigProvider != null){
-            return proxyConfigProvider.port();
-        }
-        return port;
-    }
-
-    public String resolveScheme(DefaultClientProxyConfigurationBuilder builder) {
-        return endpoint != null ? endpoint.getScheme() : builder.scheme;
-    }
-
     /**
      * Uses the configuration options, system setting property and returns the final value of the given member.
      */
@@ -283,9 +264,26 @@ public interface Builder extends CopyableBuilder<Builder, ProxyConfiguration> {
          */
         Builder useSystemPropertyValues(Boolean useSystemPropertyValues);
 
+        /**
+         * Option whether to use environment variable values for proxy configuration if any of the config options are missing.
+         *
+         * This value is set to "true" by default, which means the SDK will automatically use environment variable values
+         * for proxy configuration options that are not provided during the building of the {@link ProxyConfiguration} object.
+         * To disable this behavior, set this value to "false".
+         *
+         * @param useEnvironmentVariableValues The option whether to use environment variable values.
+         * @return This object for method chaining.
+         */
         Builder useEnvironmentVariableValues(Boolean useEnvironmentVariableValues);
 
-
+        /**
+         * The HTTP scheme to use for connecting to the proxy. Valid values are {@code http} and {@code https}.
+         * <p>
+         * The client defaults to {@code http} if none is given.
+         *
+         * @param scheme The proxy scheme.
+         * @return This object for method chaining.
+         */
         Builder scheme(String scheme);
 
     }
@@ -304,7 +302,7 @@ private static final class DefaultClientProxyConfigurationBuilder implements Bui
         private Boolean preemptiveBasicAuthenticationEnabled;
         private Boolean useSystemPropertyValues = Boolean.TRUE;
         private Boolean useEnvironmentVariableValues = Boolean.TRUE;
-        private String scheme ;
+        private String scheme = "http";
 
         @Override
         public Builder endpoint(URI endpoint) {
@@ -420,7 +418,6 @@ public void setUseEnvironmentVariableValues(Boolean useEnvironmentVariableValues
             useEnvironmentVariableValues(useEnvironmentVariableValues);
         }
 
-
         @Override
         public ProxyConfiguration build() {
             return new ProxyConfiguration(this);

diff --git a/...s/apache-client/src/test/java/software/amazon/awssdk/http/apache/ApacheHttpProxyTest.java b/...s/apache-client/src/test/java/software/amazon/awssdk/http/apache/ApacheHttpProxyTest.java
@@ -30,6 +30,7 @@ protected void assertProxyConfiguration(TestProxySetting userSetProxySettings,
                                             Boolean useSystemProperty,
                                             Boolean useEnvironmentVariable,
                                             String protocol) {
+
         ProxyConfiguration.Builder builder = ProxyConfiguration.builder();
 
         if (userSetProxySettings != null) {
@@ -56,7 +57,7 @@ protected void assertProxyConfiguration(TestProxySetting userSetProxySettings,
                 builder.nonProxyHosts(nonProxyHosts);
             }
         }
-        if(!"http".equals(protocol)){
+        if (!"http".equals(protocol)) {
             builder.scheme(protocol);
         }
         if (useSystemProperty != null) {
@@ -72,5 +73,4 @@ protected void assertProxyConfiguration(TestProxySetting userSetProxySettings,
         assertThat(proxyConfiguration.password()).isEqualTo(expectedProxySettings.getPassword());
         assertThat(proxyConfiguration.nonProxyHosts()).isEqualTo(expectedProxySettings.getNonProxyHosts());
     }
-
 }
diff --git a/...pache-client/src/test/java/software/amazon/awssdk/http/apache/ProxyConfigurationTest.java b/...pache-client/src/test/java/software/amazon/awssdk/http/apache/ProxyConfigurationTest.java
@@ -27,10 +27,8 @@
 
 public class ProxyConfigurationTest {
 
-
     private static final EnvironmentVariableHelper ENVIRONMENT_VARIABLE_HELPER = new EnvironmentVariableHelper();
 
-
     @BeforeEach
     public void setup() {
         clearProxyProperties();
@@ -40,20 +38,15 @@ public void setup() {
     public static void cleanup() {
         clearProxyProperties();
         ENVIRONMENT_VARIABLE_HELPER.reset();
-
-
     }
 
-
     @Test
     void testEndpointValues_Http_SystemPropertyEnabled() {
         String host = "foo.com";
         int port = 7777;
         System.setProperty("http.proxyHost", host);
         System.setProperty("http.proxyPort", Integer.toString(port));
-
         ENVIRONMENT_VARIABLE_HELPER.set("http_proxy", "http://UserOne:passwordSecret@bar.com:555/");
-
         ProxyConfiguration config = ProxyConfiguration.builder().useSystemPropertyValues(true).build();
 
         assertThat(config.host()).isEqualTo(host);
@@ -62,7 +55,6 @@ void testEndpointValues_Http_SystemPropertyEnabled() {
 
     }
 
-
     @Test
     void testEndpointValues_Http_EnvironmentVariableEnabled() {
         String host = "bar.com";
@@ -80,8 +72,6 @@ void testEndpointValues_Http_EnvironmentVariableEnabled() {
         assertThat(config.scheme()).isNull();
     }
 
-
-
     @Test
     void testEndpointValues_Https_SystemPropertyEnabled() {
         String host = "foo.com";

diff --git a/...ents/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/ProxyConfiguration.java b/...ents/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/ProxyConfiguration.java
@@ -116,6 +116,9 @@ public interface Builder extends CrtProxyConfiguration.Builder, CopyableBuilder<
         Builder useSystemPropertyValues(Boolean useSystemPropertyValues);
 
 
+        @Override
+        Builder useEnvironmentVariableValues(Boolean useEnvironmentVariableValues);
+
         @Override
         ProxyConfiguration build();
     }

diff --git a/...lients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/CrtHttpProxyTest.java b/...lients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/CrtHttpProxyTest.java
@@ -17,10 +17,7 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 
-import java.net.URI;
 import java.net.URISyntaxException;
-import java.util.Set;
-import software.amazon.awssdk.crtcore.CrtProxyConfiguration;
 import software.amazon.awssdk.http.HttpProxyTestSuite;
 import software.amazon.awssdk.http.proxy.TestProxySetting;
 
@@ -39,10 +36,10 @@ protected void assertProxyConfiguration(TestProxySetting userSetProxySettings,
             String userName = userSetProxySettings.getUserName();
             String password = userSetProxySettings.getPassword();
 
-            if(hostName != null){
+            if (hostName != null) {
                 proxyBuilder.host(hostName);
             }
-            if(portNumber != null){
+            if (portNumber != null) {
                 proxyBuilder.port(portNumber);
             }
             if (userName != null) {
@@ -53,7 +50,7 @@ protected void assertProxyConfiguration(TestProxySetting userSetProxySettings,
             }
         }
 
-        if(!"http".equals(protocol)){
+        if (!"http".equals(protocol)) {
             proxyBuilder.scheme(protocol);
         }
         if (useSystemProperty != null) {

diff --git a/...ty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/ProxyConfiguration.java b/...ty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/ProxyConfiguration.java
@@ -20,6 +20,7 @@
 import java.util.Set;
 import software.amazon.awssdk.annotations.SdkPublicApi;
 import software.amazon.awssdk.utils.ProxyConfigProvider;
+import software.amazon.awssdk.utils.ProxyEnvironmentSetting;
 import software.amazon.awssdk.utils.ProxySystemSetting;
 import software.amazon.awssdk.utils.StringUtils;
 import software.amazon.awssdk.utils.builder.CopyableBuilder;
@@ -48,9 +49,10 @@ private ProxyConfiguration(BuilderImpl builder) {
         this.useEnvironmentVariablesValues = builder.useEnvironmentVariablesValues;
         this.scheme = builder.scheme;
 
-        ProxyConfigProvider proxyConfigProvider = ProxyConfigProvider.getProxyConfig(builder.useSystemPropertyValues,
-                                                                                     builder.useEnvironmentVariablesValues,
-                                                                                     builder.scheme);
+        ProxyConfigProvider proxyConfigProvider =
+            ProxyConfigProvider.fromSystemEnvironmentSettings(builder.useSystemPropertyValues,
+                                                              builder.useEnvironmentVariablesValues,
+                                                              builder.scheme);
 
         this.host = builder.host != null || proxyConfigProvider == null ? builder.host : proxyConfigProvider.host();
         this.port = builder.port != 0 || proxyConfigProvider == null ? builder.port : proxyConfigProvider.port();
@@ -235,13 +237,21 @@ public interface Builder extends CopyableBuilder<Builder, ProxyConfiguration> {
         Builder useSystemPropertyValues(Boolean useSystemPropertyValues);
 
 
+        /**
+         * Set the option whether to use environment variable values for {@link ProxyEnvironmentSetting} if any of the config
+         * options are missing. The value is set to "true" by default, enabling the SDK to automatically use environment variable
+         * values for proxy configuration options that are not provided during building the {@link ProxyConfiguration} object. To
+         * disable this behavior, set this value to "false".
+         *
+         * @param useEnvironmentVariablesValues The option whether to use environment variable values
+         * @return This object for method chaining.
+         */
         Builder useEnvironmentVariablesValues(Boolean useEnvironmentVariablesValues);
 
     }
 
-
     private static final class BuilderImpl implements Builder {
-        private String scheme;
+        private String scheme = "http";
         private String host;
         private int port = 0;
         private String username;
@@ -283,7 +293,6 @@ public Builder port(int port) {
             return this;
         }
 
-
         @Override
         public Builder nonProxyHosts(Set<String> nonProxyHosts) {
             if (nonProxyHosts != null) {
@@ -316,7 +325,6 @@ public void setUseSystemPropertyValues(Boolean useSystemPropertyValues) {
             useSystemPropertyValues(useSystemPropertyValues);
         }
 
-
         @Override
         public Builder useEnvironmentVariablesValues(Boolean useEnvironmentVariablesValues) {
             this.useEnvironmentVariablesValues = useEnvironmentVariablesValues;
@@ -327,7 +335,6 @@ public void setUseEnvironmentVariablesValues(Boolean useEnvironmentVariablesValu
             useEnvironmentVariablesValues(useEnvironmentVariablesValues);
         }
 
-
         @Override
         public ProxyConfiguration build() {
             return new ProxyConfiguration(this);