Updated endpoint discovery behavior for operations that require endpo…

…int discovery. Specifically, when endpoint discovery is required: 1. If endpoint discovery is disabled, throw an exception. Endpoint discovery is required. Disabling it should not be allowed. 2. If an endpoint override is specified, throw an exception. Endpoint discovery being required implies the endpoint MUST be discovered. 3. Introduce an "allowEndpointOverrideForEndpointDiscoveryRequiredOperations" customization flag that can be used by services that need to disable (2) before we can reach a more long-term state of consistency across the SDKs. This flag will treat the "endpointOverride" as the *discovered* endpoint. Motivation: 1. The behavior across the SDKs is inconsistent, so this brings the Java SDK more into line with the other SDKs. 2. This removes the ambiguity as to whether the "endpoint override" is the *discovery* endpoint or the *discovered* endpoint by just disallowing its use altogether. In the future we can add a "discoveryEndpointOverride" to codify "endpointOverride" as being the *discovered* endpoint.
aws · Aug 27, 2020 · c2c0e28 · c2c0e28
1 parent c5ece8a
commit c2c0e28
Show file tree

Hide file tree

Showing 15 changed files with 684 additions and 13 deletions.
diff --git a/...n/java/software/amazon/awssdk/codegen/model/config/customization/CustomizationConfig.java b/...n/java/software/amazon/awssdk/codegen/model/config/customization/CustomizationConfig.java
@@ -165,6 +165,13 @@ public class CustomizationConfig {
      */
     private boolean enableEndpointDiscoveryMethodRequired = false;
 
+    /**
+     * Allow a customer to set an endpoint override AND bypass endpoint discovery on their client even when endpoint discovery
+     * enabled is true and endpoint discovery is required for an operation. This customization should almost never be "true"
+     * because it creates a confusing customer experience.
+     */
+    private boolean allowEndpointOverrideForEndpointDiscoveryRequiredOperations = false;
+
     private CustomizationConfig() {
     }
 
@@ -420,4 +427,14 @@ public boolean isEnableEndpointDiscoveryMethodRequired() {
     public void setEnableEndpointDiscoveryMethodRequired(boolean enableEndpointDiscoveryMethodRequired) {
         this.enableEndpointDiscoveryMethodRequired = enableEndpointDiscoveryMethodRequired;
     }
+
+    public boolean allowEndpointOverrideForEndpointDiscoveryRequiredOperations() {
+        return allowEndpointOverrideForEndpointDiscoveryRequiredOperations;
+    }
+
+    public void setAllowEndpointOverrideForEndpointDiscoveryRequiredOperations(
+        boolean allowEndpointOverrideForEndpointDiscoveryRequiredOperations) {
+        this.allowEndpointOverrideForEndpointDiscoveryRequiredOperations =
+            allowEndpointOverrideForEndpointDiscoveryRequiredOperations;
+    }
 }
diff --git a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/client/AsyncClientClass.java b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/client/AsyncClientClass.java
@@ -166,6 +166,17 @@ private MethodSpec constructor(Builder classBuilder) {
                                  EndpointDiscoveryRefreshCache.class,
                                  poetExtensions.getClientClass(model.getNamingStrategy().getServiceName() +
                                                                "AsyncEndpointDiscoveryCacheLoader"));
+
+            if (model.getCustomizationConfig().allowEndpointOverrideForEndpointDiscoveryRequiredOperations()) {
+                builder.beginControlFlow("if (clientConfiguration.option(SdkClientOption.ENDPOINT_OVERRIDDEN) == "
+                                        + "Boolean.TRUE)");
+                builder.addStatement("log.warn($S)",
+                                     "Endpoint discovery is enabled for this client, and an endpoint override was also "
+                                     + "specified. This will disable endpoint discovery for methods that require it, instead "
+                                     + "using the specified endpoint override. This may or may not be what you intended.");
+                builder.endControlFlow();
+            }
+
             builder.endControlFlow();
         }
 
@@ -220,8 +231,37 @@ protected MethodSpec.Builder operationBody(MethodSpec.Builder builder, Operation
         builder.addCode(eventToByteBufferPublisher(opModel));
 
         if (opModel.getEndpointDiscovery() != null) {
+            builder.addStatement("boolean endpointDiscoveryEnabled = "
+                                 + "clientConfiguration.option(SdkClientOption.ENDPOINT_DISCOVERY_ENABLED)");
+            builder.addStatement("boolean endpointOverridden = "
+                                 + "clientConfiguration.option(SdkClientOption.ENDPOINT_OVERRIDDEN) == Boolean.TRUE");
+
+            if (opModel.getEndpointDiscovery().isRequired()) {
+                if (!model.getCustomizationConfig().allowEndpointOverrideForEndpointDiscoveryRequiredOperations()) {
+                    builder.beginControlFlow("if (endpointOverridden)");
+                    builder.addStatement("throw new $T($S)", IllegalStateException.class,
+                                         "This operation requires endpoint discovery, but an endpoint override was specified "
+                                         + "when the client was created. This is not supported.");
+                    builder.endControlFlow();
+
+                    builder.beginControlFlow("if (!endpointDiscoveryEnabled)");
+                    builder.addStatement("throw new $T($S)", IllegalStateException.class,
+                                         "This operation requires endpoint discovery, but endpoint discovery was disabled on the "
+                                         + "client.");
+                    builder.endControlFlow();
+                } else {
+                    builder.beginControlFlow("if (endpointOverridden)");
+                    builder.addStatement("endpointDiscoveryEnabled = false");
+                    builder.nextControlFlow("else if (!endpointDiscoveryEnabled)");
+                    builder.addStatement("throw new $T($S)", IllegalStateException.class,
+                                         "This operation requires endpoint discovery to be enabled, or for you to specify an "
+                                         + "endpoint override when the client is created.");
+                    builder.endControlFlow();
+                }
+            }
+
             builder.addStatement("$T cachedEndpoint = null", URI.class);
-            builder.beginControlFlow("if (clientConfiguration.option(SdkClientOption.ENDPOINT_DISCOVERY_ENABLED))");
+            builder.beginControlFlow("if (endpointDiscoveryEnabled)");
             builder.addStatement("\n\nString key = clientConfiguration.option($T.CREDENTIALS_PROVIDER).resolveCredentials()" +
                                  ".accessKeyId()", AwsClientOption.class);
             builder.addStatement("EndpointDiscoveryRequest endpointDiscoveryRequest = $T.builder().required($L)" +

diff --git a/codegen/src/main/java/software/amazon/awssdk/codegen/poet/client/SyncClientClass.java b/codegen/src/main/java/software/amazon/awssdk/codegen/poet/client/SyncClientClass.java
@@ -60,6 +60,7 @@
 import software.amazon.awssdk.metrics.MetricCollector;
 import software.amazon.awssdk.metrics.MetricPublisher;
 import software.amazon.awssdk.metrics.NoOpMetricCollector;
+import software.amazon.awssdk.utils.Logger;
 
 //TODO Make SyncClientClass extend SyncClientInterface (similar to what we do in AsyncClientClass)
 public class SyncClientClass implements ClassSpec {
@@ -86,6 +87,7 @@ public TypeSpec poetSpec() {
                                         .addSuperinterface(interfaceClass)
                                         .addJavadoc("Internal implementation of {@link $1T}.\n\n@see $1T#builder()",
                                                     interfaceClass)
+                                        .addField(logger())
                                         .addField(SyncClientHandler.class, "clientHandler", PRIVATE, FINAL)
                                         .addField(protocolSpec.protocolFactory(model))
                                         .addField(SdkClientConfiguration.class, "clientConfiguration", PRIVATE, FINAL)
@@ -119,6 +121,12 @@ public TypeSpec poetSpec() {
         return classBuilder.build();
     }
 
+    private FieldSpec logger() {
+        return FieldSpec.builder(Logger.class, "log", PRIVATE, STATIC, FINAL)
+                        .initializer("$T.loggerFor($T.class)", Logger.class, className)
+                        .build();
+    }
+
     private MethodSpec nameMethod() {
         return MethodSpec.methodBuilder("serviceName")
                          .addAnnotation(Override.class)
@@ -154,6 +162,17 @@ private MethodSpec constructor() {
                                  EndpointDiscoveryRefreshCache.class,
                                  poetExtensions.getClientClass(model.getNamingStrategy().getServiceName() +
                                                                "EndpointDiscoveryCacheLoader"));
+
+            if (model.getCustomizationConfig().allowEndpointOverrideForEndpointDiscoveryRequiredOperations()) {
+                builder.beginControlFlow("if (clientConfiguration.option(SdkClientOption.ENDPOINT_OVERRIDDEN) == "
+                                         + "Boolean.TRUE)");
+                builder.addStatement("log.warn(() -> $S)",
+                                     "Endpoint discovery is enabled for this client, and an endpoint override was also "
+                                     + "specified. This will disable endpoint discovery for methods that require it, instead "
+                                     + "using the specified endpoint override. This may or may not be what you intended.");
+                builder.endControlFlow();
+            }
+
             builder.endControlFlow();
         }
 
@@ -181,8 +200,37 @@ private List<MethodSpec> operationMethodSpecs(OperationModel opModel) {
         protocolSpec.errorResponseHandler(opModel).ifPresent(method::addCode);
 
         if (opModel.getEndpointDiscovery() != null) {
+            method.addStatement("boolean endpointDiscoveryEnabled = "
+                                + "clientConfiguration.option(SdkClientOption.ENDPOINT_DISCOVERY_ENABLED)");
+            method.addStatement("boolean endpointOverridden = "
+                                + "clientConfiguration.option(SdkClientOption.ENDPOINT_OVERRIDDEN) == Boolean.TRUE");
+
+            if (opModel.getEndpointDiscovery().isRequired()) {
+                if (!model.getCustomizationConfig().allowEndpointOverrideForEndpointDiscoveryRequiredOperations()) {
+                    method.beginControlFlow("if (endpointOverridden)");
+                    method.addStatement("throw new $T($S)", IllegalStateException.class,
+                                        "This operation requires endpoint discovery, but an endpoint override was specified "
+                                        + "when the client was created. This is not supported.");
+                    method.endControlFlow();
+
+                    method.beginControlFlow("if (!endpointDiscoveryEnabled)");
+                    method.addStatement("throw new $T($S)", IllegalStateException.class,
+                                        "This operation requires endpoint discovery, but endpoint discovery was disabled on the "
+                                        + "client.");
+                    method.endControlFlow();
+                } else {
+                    method.beginControlFlow("if (endpointOverridden)");
+                    method.addStatement("endpointDiscoveryEnabled = false");
+                    method.nextControlFlow("else if (!endpointDiscoveryEnabled)");
+                    method.addStatement("throw new $T($S)", IllegalStateException.class,
+                                        "This operation requires endpoint discovery to be enabled, or for you to specify an "
+                                        + "endpoint override when the client is created.");
+                    method.endControlFlow();
+                }
+            }
+
             method.addStatement("$T cachedEndpoint = null", URI.class);
-            method.beginControlFlow("if (clientConfiguration.option(SdkClientOption.ENDPOINT_DISCOVERY_ENABLED))");
+            method.beginControlFlow("if (endpointDiscoveryEnabled)");
             method.addStatement("\n\nString key = clientConfiguration.option($T.CREDENTIALS_PROVIDER)." +
                                 "resolveCredentials().accessKeyId()", AwsClientOption.class);
             method.addStatement("EndpointDiscoveryRequest endpointDiscoveryRequest = $T.builder().required($L)" +

diff --git a/...t/resources/software/amazon/awssdk/codegen/poet/client/test-endpoint-discovery-async.java b/...t/resources/software/amazon/awssdk/codegen/poet/client/test-endpoint-discovery-async.java
@@ -167,8 +167,18 @@ public CompletableFuture<TestDiscoveryIdentifiersRequiredResponse> testDiscovery
 
             HttpResponseHandler<AwsServiceException> errorResponseHandler = createErrorResponseHandler(protocolFactory,
                                                                                                        operationMetadata);
+            boolean endpointDiscoveryEnabled = clientConfiguration.option(SdkClientOption.ENDPOINT_DISCOVERY_ENABLED);
+            boolean endpointOverridden = clientConfiguration.option(SdkClientOption.ENDPOINT_OVERRIDDEN) == Boolean.TRUE;
+            if (endpointOverridden) {
+                throw new IllegalStateException(
+                    "This operation requires endpoint discovery, but an endpoint override was specified when the client was created. This is not supported.");
+            }
+            if (!endpointDiscoveryEnabled) {
+                throw new IllegalStateException(
+                    "This operation requires endpoint discovery, but endpoint discovery was disabled on the client.");
+            }
             URI cachedEndpoint = null;
-            if (clientConfiguration.option(SdkClientOption.ENDPOINT_DISCOVERY_ENABLED)) {
+            if (endpointDiscoveryEnabled) {
 
                 String key = clientConfiguration.option(AwsClientOption.CREDENTIALS_PROVIDER).resolveCredentials().accessKeyId();
                 EndpointDiscoveryRequest endpointDiscoveryRequest = EndpointDiscoveryRequest.builder().required(true)
@@ -231,8 +241,10 @@ public CompletableFuture<TestDiscoveryOptionalResponse> testDiscoveryOptional(
 
             HttpResponseHandler<AwsServiceException> errorResponseHandler = createErrorResponseHandler(protocolFactory,
                                                                                                        operationMetadata);
+            boolean endpointDiscoveryEnabled = clientConfiguration.option(SdkClientOption.ENDPOINT_DISCOVERY_ENABLED);
+            boolean endpointOverridden = clientConfiguration.option(SdkClientOption.ENDPOINT_OVERRIDDEN) == Boolean.TRUE;
             URI cachedEndpoint = null;
-            if (clientConfiguration.option(SdkClientOption.ENDPOINT_DISCOVERY_ENABLED)) {
+            if (endpointDiscoveryEnabled) {
 
                 String key = clientConfiguration.option(AwsClientOption.CREDENTIALS_PROVIDER).resolveCredentials().accessKeyId();
                 EndpointDiscoveryRequest endpointDiscoveryRequest = EndpointDiscoveryRequest.builder().required(false)
@@ -295,8 +307,18 @@ public CompletableFuture<TestDiscoveryRequiredResponse> testDiscoveryRequired(
 
             HttpResponseHandler<AwsServiceException> errorResponseHandler = createErrorResponseHandler(protocolFactory,
                                                                                                        operationMetadata);
+            boolean endpointDiscoveryEnabled = clientConfiguration.option(SdkClientOption.ENDPOINT_DISCOVERY_ENABLED);
+            boolean endpointOverridden = clientConfiguration.option(SdkClientOption.ENDPOINT_OVERRIDDEN) == Boolean.TRUE;
+            if (endpointOverridden) {
+                throw new IllegalStateException(
+                    "This operation requires endpoint discovery, but an endpoint override was specified when the client was created. This is not supported.");
+            }
+            if (!endpointDiscoveryEnabled) {
+                throw new IllegalStateException(
+                    "This operation requires endpoint discovery, but endpoint discovery was disabled on the client.");
+            }
             URI cachedEndpoint = null;
-            if (clientConfiguration.option(SdkClientOption.ENDPOINT_DISCOVERY_ENABLED)) {
+            if (endpointDiscoveryEnabled) {
 
                 String key = clientConfiguration.option(AwsClientOption.CREDENTIALS_PROVIDER).resolveCredentials().accessKeyId();
                 EndpointDiscoveryRequest endpointDiscoveryRequest = EndpointDiscoveryRequest.builder().required(true)

diff --git a/...st/resources/software/amazon/awssdk/codegen/poet/client/test-endpoint-discovery-sync.java b/...st/resources/software/amazon/awssdk/codegen/poet/client/test-endpoint-discovery-sync.java
@@ -38,6 +38,7 @@
 import software.amazon.awssdk.services.endpointdiscoverytest.transform.TestDiscoveryIdentifiersRequiredRequestMarshaller;
 import software.amazon.awssdk.services.endpointdiscoverytest.transform.TestDiscoveryOptionalRequestMarshaller;
 import software.amazon.awssdk.services.endpointdiscoverytest.transform.TestDiscoveryRequiredRequestMarshaller;
+import software.amazon.awssdk.utils.Logger;
 
 /**
  * Internal implementation of {@link EndpointDiscoveryTestClient}.
@@ -47,6 +48,8 @@
 @Generated("software.amazon.awssdk:codegen")
 @SdkInternalApi
 final class DefaultEndpointDiscoveryTestClient implements EndpointDiscoveryTestClient {
+    private static final Logger log = Logger.loggerFor(DefaultEndpointDiscoveryTestClient.class);
+
     private final SyncClientHandler clientHandler;
 
     private final AwsJsonProtocolFactory protocolFactory;
@@ -139,8 +142,18 @@ public TestDiscoveryIdentifiersRequiredResponse testDiscoveryIdentifiersRequired
 
         HttpResponseHandler<AwsServiceException> errorResponseHandler = createErrorResponseHandler(protocolFactory,
                                                                                                    operationMetadata);
+        boolean endpointDiscoveryEnabled = clientConfiguration.option(SdkClientOption.ENDPOINT_DISCOVERY_ENABLED);
+        boolean endpointOverridden = clientConfiguration.option(SdkClientOption.ENDPOINT_OVERRIDDEN) == Boolean.TRUE;
+        if (endpointOverridden) {
+            throw new IllegalStateException(
+                "This operation requires endpoint discovery, but an endpoint override was specified when the client was created. This is not supported.");
+        }
+        if (!endpointDiscoveryEnabled) {
+            throw new IllegalStateException(
+                "This operation requires endpoint discovery, but endpoint discovery was disabled on the client.");
+        }
         URI cachedEndpoint = null;
-        if (clientConfiguration.option(SdkClientOption.ENDPOINT_DISCOVERY_ENABLED)) {
+        if (endpointDiscoveryEnabled) {
 
             String key = clientConfiguration.option(AwsClientOption.CREDENTIALS_PROVIDER).resolveCredentials().accessKeyId();
             EndpointDiscoveryRequest endpointDiscoveryRequest = EndpointDiscoveryRequest.builder().required(true)
@@ -191,8 +204,10 @@ public TestDiscoveryOptionalResponse testDiscoveryOptional(TestDiscoveryOptional
 
         HttpResponseHandler<AwsServiceException> errorResponseHandler = createErrorResponseHandler(protocolFactory,
                                                                                                    operationMetadata);
+        boolean endpointDiscoveryEnabled = clientConfiguration.option(SdkClientOption.ENDPOINT_DISCOVERY_ENABLED);
+        boolean endpointOverridden = clientConfiguration.option(SdkClientOption.ENDPOINT_OVERRIDDEN) == Boolean.TRUE;
         URI cachedEndpoint = null;
-        if (clientConfiguration.option(SdkClientOption.ENDPOINT_DISCOVERY_ENABLED)) {
+        if (endpointDiscoveryEnabled) {
 
             String key = clientConfiguration.option(AwsClientOption.CREDENTIALS_PROVIDER).resolveCredentials().accessKeyId();
             EndpointDiscoveryRequest endpointDiscoveryRequest = EndpointDiscoveryRequest.builder().required(false)
@@ -242,8 +257,18 @@ public TestDiscoveryRequiredResponse testDiscoveryRequired(TestDiscoveryRequired
 
         HttpResponseHandler<AwsServiceException> errorResponseHandler = createErrorResponseHandler(protocolFactory,
                                                                                                    operationMetadata);
+        boolean endpointDiscoveryEnabled = clientConfiguration.option(SdkClientOption.ENDPOINT_DISCOVERY_ENABLED);
+        boolean endpointOverridden = clientConfiguration.option(SdkClientOption.ENDPOINT_OVERRIDDEN) == Boolean.TRUE;
+        if (endpointOverridden) {
+            throw new IllegalStateException(
+                "This operation requires endpoint discovery, but an endpoint override was specified when the client was created. This is not supported.");
+        }
+        if (!endpointDiscoveryEnabled) {
+            throw new IllegalStateException(
+                "This operation requires endpoint discovery, but endpoint discovery was disabled on the client.");
+        }
         URI cachedEndpoint = null;
-        if (clientConfiguration.option(SdkClientOption.ENDPOINT_DISCOVERY_ENABLED)) {
+        if (endpointDiscoveryEnabled) {
 
             String key = clientConfiguration.option(AwsClientOption.CREDENTIALS_PROVIDER).resolveCredentials().accessKeyId();
             EndpointDiscoveryRequest endpointDiscoveryRequest = EndpointDiscoveryRequest.builder().required(true)