Prevent duplicate ExecutionAttribute names and fix name for FIPS_ENDPOINT_ENABLED attribute (#2968)

ExecutionAttributes are used as a key for looking up values in ExecutionAttributes maps. Having two attributes with the same name would be harmful since it may unintentionally override another attribute in the map, or allow an attribute lookup to accidentally reference another attribute's value.

ExecutionAttributes are conventionally referenced via static constants. We can leverage this convention to ensure that no two attributes are ever created with the same name.

Note: This has the possibility to be a breaking change if, and only if, users are creating ExecutionAttributes rather than referencing ones that already exist. Such a scenario seems highly unlikely since it breaks the clear convention of referencing these as static constants, and because ExecutionAttributes are primarily used internally within the SDK.

Author: Bennett-Lynch

.changes/next-release/bugfix-AWSSDKforJavav2-075aece.json

@@ -0,0 +1,6 @@
+{
+    "category": "AWS SDK for Java v2", 
+    "contributor": "", 
+    "type": "bugfix", 
+    "description": "Prevent duplicate ExecutionAttribute names and fix name for FIPS_ENDPOINT_ENABLED attribute"
+}

core/aws-core/src/main/java/software/amazon/awssdk/awscore/AwsExecutionAttribute.java

@@ -49,7 +49,7 @@ public final class AwsExecutionAttribute extends SdkExecutionAttribute {
      * Whether fips endpoints were enabled for this request.
      */
     public static final ExecutionAttribute<Boolean> FIPS_ENDPOINT_ENABLED =
-        new ExecutionAttribute<>("DualstackEndpointsEnabled");
+        new ExecutionAttribute<>("FipsEndpointsEnabled");
 
     private AwsExecutionAttribute() {
     }

core/sdk-core/src/main/java/software/amazon/awssdk/core/interceptor/ExecutionAttribute.java

@@ -16,6 +16,8 @@
 package software.amazon.awssdk.core.interceptor;
 
 import java.util.Objects;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
 import software.amazon.awssdk.annotations.SdkPublicApi;
 
 /**
@@ -42,7 +44,8 @@
  */
 @SdkPublicApi
 public final class ExecutionAttribute<T> {
-
+    private static final ConcurrentMap<String, ExecutionAttribute<?>> NAME_HISTORY = new ConcurrentHashMap<>();
+    
     private final String name;
 
     /**
@@ -52,6 +55,20 @@ public final class ExecutionAttribute<T> {
      */
     public ExecutionAttribute(String name) {
         this.name = name;
+        ensureUnique();
+    }
+
+    private void ensureUnique() {
+        ExecutionAttribute<?> prev = NAME_HISTORY.putIfAbsent(name, this);
+        if (prev != null) {
+            throw new IllegalArgumentException(String.format("No duplicate ExecutionAttribute names allowed but both "
+                                                             + "ExecutionAttributes %s and %s have the same name: %s. " 
+                                                             + "ExecutionAttributes should be referenced from a shared static " 
+                                                             + "constant to protect against erroneous or unexpected collisions.",
+                                                             Integer.toHexString(System.identityHashCode(prev)),
+                                                             Integer.toHexString(System.identityHashCode(this)),
+                                                             name));
+        }
     }
 
     @Override

core/sdk-core/src/main/java/software/amazon/awssdk/core/interceptor/ExecutionAttributes.java

@@ -20,6 +20,7 @@
 import java.util.Map;
 import software.amazon.awssdk.annotations.NotThreadSafe;
 import software.amazon.awssdk.annotations.SdkPublicApi;
+import software.amazon.awssdk.utils.ToString;
 import software.amazon.awssdk.utils.Validate;
 import software.amazon.awssdk.utils.builder.CopyableBuilder;
 import software.amazon.awssdk.utils.builder.ToCopyableBuilder;
@@ -126,6 +127,13 @@ public int hashCode() {
         return attributes != null ? attributes.hashCode() : 0;
     }
 
+    @Override
+    public String toString() {
+        return ToString.builder("ExecutionAttributes")
+                       .add("attributes", attributes.keySet())
+                       .build();
+    }
+
     public static ExecutionAttributes unmodifiableExecutionAttributes(ExecutionAttributes attributes) {
         return new UnmodifiableExecutionAttributes(attributes);
     }

core/sdk-core/src/test/java/software/amazon/awssdk/core/RequestOverrideConfigurationTest.java

@@ -26,6 +26,8 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
 import nl.jqno.equalsverifier.EqualsVerifier;
 import org.junit.jupiter.api.Test;
 import software.amazon.awssdk.core.interceptor.ExecutionAttribute;
@@ -36,6 +38,13 @@
 import software.amazon.awssdk.utils.ImmutableMap;
 
 public class RequestOverrideConfigurationTest {
+    private static final ConcurrentMap<String, ExecutionAttribute<Object>> ATTR_POOL = new ConcurrentHashMap<>();
+
+    private static ExecutionAttribute<Object> attr(String name) {
+        name = RequestOverrideConfigurationTest.class.getName() + ":" + name;
+        return ATTR_POOL.computeIfAbsent(name, ExecutionAttribute::new);
+    }
+    
     private static final String HEADER = "header";
     private static final String QUERY_PARAM = "queryparam";
 
@@ -56,7 +65,7 @@ public void toBuilder_minimal() {
 
     @Test
     public void toBuilder_maximal() {
-        ExecutionAttribute testAttribute = new ExecutionAttribute("TestAttribute");
+        ExecutionAttribute testAttribute = attr("TestAttribute");
         String expectedValue = "Value1";
 
         RequestOverrideConfiguration configuration = SdkRequestOverrideConfiguration.builder()
@@ -210,7 +219,7 @@ public void addMetricPublisher_listPreviouslyAdded_appendedToList() {
     public void executionAttributes_createsCopy() {
         ExecutionAttributes executionAttributes = new ExecutionAttributes();
 
-        ExecutionAttribute testAttribute = new ExecutionAttribute("TestAttribute");
+        ExecutionAttribute testAttribute = attr("TestAttribute");
         String expectedValue = "Value1";
         executionAttributes.putAttribute(testAttribute, expectedValue);
 
@@ -226,7 +235,7 @@ public void executionAttributes_createsCopy() {
     @Test
     public void executionAttributes_isImmutable() {
         ExecutionAttributes executionAttributes = new ExecutionAttributes();
-        ExecutionAttribute testAttribute = new ExecutionAttribute("TestAttribute");
+        ExecutionAttribute testAttribute = attr("TestAttribute");
         String expectedValue = "Value1";
         executionAttributes.putAttribute(testAttribute, expectedValue);
 
@@ -252,7 +261,7 @@ public void executionAttributes_isImmutable() {
     public void executionAttributes_maintainsAllAdded() {
         Map<ExecutionAttribute, Object> executionAttributeObjectMap = new HashMap<>();
         for (int i = 0; i < 5; i++) {
-            executionAttributeObjectMap.put(new ExecutionAttribute<>("Attribute" + i), mock(Object.class));
+            executionAttributeObjectMap.put(attr("Attribute" + i), mock(Object.class));
         }
 
         SdkRequestOverrideConfiguration.Builder builder = SdkRequestOverrideConfiguration.builder();
@@ -269,12 +278,12 @@ public void executionAttributes_maintainsAllAdded() {
     public void executionAttributes_overwritesPreviouslyAdded() {
         ExecutionAttributes executionAttributes = new ExecutionAttributes();
         for (int i = 0; i < 5; i++) {
-            executionAttributes.putAttribute(new ExecutionAttribute<>("Attribute" + i), mock(Object.class));
+            executionAttributes.putAttribute(attr("Attribute" + i), mock(Object.class));
         }
 
         SdkRequestOverrideConfiguration.Builder builder = SdkRequestOverrideConfiguration.builder();
 
-        builder.putExecutionAttribute(new ExecutionAttribute("AddedAttribute"), mock(Object.class));
+        builder.putExecutionAttribute(attr("AddedAttribute"), mock(Object.class));
         builder.executionAttributes(executionAttributes);
         SdkRequestOverrideConfiguration overrideConfig = builder.build();
         assertThat(overrideConfig.executionAttributes().getAttributes()).isEqualTo(executionAttributes.getAttributes());
@@ -284,13 +293,13 @@ public void executionAttributes_overwritesPreviouslyAdded() {
     public void executionAttributes_listPreviouslyAdded_appendedToList() {
         ExecutionAttributes executionAttributes = new ExecutionAttributes();
         for (int i = 0; i < 5; i++) {
-            executionAttributes.putAttribute(new ExecutionAttribute<>("Attribute" + i), mock(Object.class));
+            executionAttributes.putAttribute(attr("Attribute" + i), mock(Object.class));
         }
 
         SdkRequestOverrideConfiguration.Builder builder = SdkRequestOverrideConfiguration.builder();
 
         builder.executionAttributes(executionAttributes);
-        ExecutionAttribute addedAttribute = new ExecutionAttribute("AddedAttribute");
+        ExecutionAttribute addedAttribute = attr("AddedAttribute");
         Object addedValue = mock(Object.class);
 
         builder.putExecutionAttribute(addedAttribute, addedValue);
@@ -303,7 +312,7 @@ public void executionAttributes_listPreviouslyAdded_appendedToList() {
     public void testConfigurationEquals() {
         ExecutionAttributes executionAttributes = new ExecutionAttributes();
         for (int i = 0; i < 5; i++) {
-            executionAttributes.putAttribute(new ExecutionAttribute<>("Attribute" + i), mock(Object.class));
+            executionAttributes.putAttribute(attr("Attribute" + i), mock(Object.class));
         }
 
         SdkRequestOverrideConfiguration request1Override  = SdkRequestOverrideConfiguration.builder()

core/sdk-core/src/test/java/software/amazon/awssdk/core/client/config/ClientOverrideConfigurationTest.java

@@ -27,6 +27,8 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
 import org.junit.jupiter.api.Test;
 import software.amazon.awssdk.core.interceptor.ExecutionAttribute;
 import software.amazon.awssdk.core.interceptor.ExecutionAttributes;
@@ -36,6 +38,12 @@
 import software.amazon.awssdk.utils.ImmutableMap;
 
 public class ClientOverrideConfigurationTest {
+    private static final ConcurrentMap<String, ExecutionAttribute<Object>> ATTR_POOL = new ConcurrentHashMap<>();
+
+    private static ExecutionAttribute<Object> attr(String name) {
+        name = ClientOverrideConfigurationTest.class.getName() + ":" + name;
+        return ATTR_POOL.computeIfAbsent(name, ExecutionAttribute::new);
+    }
 
     @Test
     public void addingSameItemTwice_shouldOverride() {
@@ -188,7 +196,7 @@ public void addMetricPublisher_listPreviouslyAdded_appendedToList() {
     public void executionAttributes_createsCopy() {
         ExecutionAttributes executionAttributes = new ExecutionAttributes();
 
-        ExecutionAttribute testAttribute = new ExecutionAttribute("TestAttribute");
+        ExecutionAttribute testAttribute = attr("TestAttribute");
         String expectedValue = "Value1";
         executionAttributes.putAttribute(testAttribute, expectedValue);
 
@@ -204,7 +212,7 @@ public void executionAttributes_createsCopy() {
     public void executionAttributes_isImmutable() {
         ExecutionAttributes executionAttributes = new ExecutionAttributes();
 
-        ExecutionAttribute testAttribute = new ExecutionAttribute("TestAttribute");
+        ExecutionAttribute testAttribute = attr("TestAttribute");
         String expectedValue = "Value1";
         executionAttributes.putAttribute(testAttribute, expectedValue);
 
@@ -224,7 +232,7 @@ public void executionAttributes_isImmutable() {
     public void executionAttributes_maintainsAllAdded() {
         Map<ExecutionAttribute, Object> executionAttributeObjectMap = new HashMap<>();
         for (int i = 0; i < 5; i++) {
-            executionAttributeObjectMap.put(new ExecutionAttribute<>("Attribute" + i), mock(Object.class));
+            executionAttributeObjectMap.put(attr("Attribute" + i), mock(Object.class));
         }
 
         ClientOverrideConfiguration.Builder builder = ClientOverrideConfiguration.builder();
@@ -241,12 +249,12 @@ public void executionAttributes_maintainsAllAdded() {
     public void executionAttributes_overwritesPreviouslyAdded() {
         ExecutionAttributes executionAttributes = new ExecutionAttributes();
         for (int i = 0; i < 5; i++) {
-            executionAttributes.putAttribute(new ExecutionAttribute<>("Attribute" + i), mock(Object.class));
+            executionAttributes.putAttribute(attr("Attribute" + i), mock(Object.class));
         }
 
         ClientOverrideConfiguration.Builder builder = ClientOverrideConfiguration.builder();
 
-        builder.putExecutionAttribute(new ExecutionAttribute("AddedAttribute"), mock(Object.class));
+        builder.putExecutionAttribute(attr("AddedAttribute"), mock(Object.class));
         builder.executionAttributes(executionAttributes);
         ClientOverrideConfiguration overrideConfig = builder.build();
         assertThat(overrideConfig.executionAttributes().getAttributes()).isEqualTo(executionAttributes.getAttributes());
@@ -256,13 +264,13 @@ public void executionAttributes_overwritesPreviouslyAdded() {
     public void executionAttributes_listPreviouslyAdded_appendedToList() {
         ExecutionAttributes executionAttributes = new ExecutionAttributes();
         for (int i = 0; i < 5; i++) {
-            executionAttributes.putAttribute(new ExecutionAttribute<>("Attribute" + i), mock(Object.class));
+            executionAttributes.putAttribute(attr("Attribute" + i), mock(Object.class));
         }
 
         ClientOverrideConfiguration.Builder builder = ClientOverrideConfiguration.builder();
 
         builder.executionAttributes(executionAttributes);
-        ExecutionAttribute addedAttribute = new ExecutionAttribute("AddedAttribute");
+        ExecutionAttribute addedAttribute = attr("AddedAttribute");
         Object addedValue = mock(Object.class);
 
         builder.putExecutionAttribute(addedAttribute, addedValue);

core/sdk-core/src/test/java/software/amazon/awssdk/core/interceptor/ExecutionAttributeTest.java

@@ -0,0 +1,35 @@
+/*
+ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License").
+ * You may not use this file except in compliance with the License.
+ * A copy of the License is located at
+ *
+ *  http://aws.amazon.com/apache2.0
+ *
+ * or in the "license" file accompanying this file. This file is distributed
+ * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+ * express or implied. See the License for the specific language governing
+ * permissions and limitations under the License.
+ */
+
+package software.amazon.awssdk.core.interceptor;
+
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+class ExecutionAttributeTest {
+
+    @Test
+    @DisplayName("Ensure that two different ExecutionAttributes are not allowed to have the same name (and hash key)")
+    void testUniqueness() {
+        String name = "ExecutionAttributeTest";
+        ExecutionAttribute<Integer> first = new ExecutionAttribute<>(name);
+        assertThatThrownBy(() -> {
+            ExecutionAttribute<Integer> second = new ExecutionAttribute<>(name);
+        }).isInstanceOf(IllegalArgumentException.class)
+          .hasMessageContaining(name);
+    }
+}

test/codegen-generated-classes-test/src/test/java/software/amazon/awssdk/services/ExecutionAttributesTest.java

@@ -6,6 +6,8 @@
 import static org.mockito.Mockito.mock;
 
 import java.util.concurrent.CompletionException;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
@@ -24,6 +26,13 @@
 import software.amazon.awssdk.services.protocolrestjson.model.AllTypesRequest;
 
 public class ExecutionAttributesTest {
+    private static final ConcurrentMap<String, ExecutionAttribute<Object>> ATTR_POOL = new ConcurrentHashMap<>();
+
+    private static ExecutionAttribute<Object> attr(String name) {
+        name = ExecutionAttributesTest.class.getName() + ":" + name;
+        return ATTR_POOL.computeIfAbsent(name, ExecutionAttribute::new);
+    }
+    
     @Rule
     public ExpectedException thrown = ExpectedException.none();
 
@@ -78,7 +87,7 @@ public void asyncClient_clientOverrideExecutionAttribute() {
         ExecutionInterceptor interceptor = mock(ExecutionInterceptor.class);
         ArgumentCaptor<ExecutionAttributes> attributesCaptor = ArgumentCaptor.forClass(ExecutionAttributes.class);
         doThrow(new RuntimeException("BOOM")).when(interceptor).beforeExecution(any(Context.BeforeExecution.class), attributesCaptor.capture());
-        ExecutionAttribute testAttribute = new ExecutionAttribute<>("TestAttribute");
+        ExecutionAttribute testAttribute = attr("TestAttribute");
         String testValue = "TestValue";
 
         ProtocolRestJsonAsyncClient async = ProtocolRestJsonAsyncClient.builder()
@@ -103,7 +112,7 @@ public void asyncClient_requestOverrideExecutionAttribute() {
         ExecutionInterceptor interceptor = mock(ExecutionInterceptor.class);
         ArgumentCaptor<ExecutionAttributes> attributesCaptor = ArgumentCaptor.forClass(ExecutionAttributes.class);
         doThrow(new RuntimeException("BOOM")).when(interceptor).beforeExecution(any(Context.BeforeExecution.class), attributesCaptor.capture());
-        ExecutionAttribute testAttribute = new ExecutionAttribute<>("TestAttribute");
+        ExecutionAttribute testAttribute = attr("TestAttribute");
         String testValue = "TestValue";
 
         ProtocolRestJsonAsyncClient async = ProtocolRestJsonAsyncClient.builder()
@@ -131,7 +140,7 @@ public void asyncClient_requestOverrideExecutionAttributesHavePrecedence() {
         ExecutionInterceptor interceptor = mock(ExecutionInterceptor.class);
         ArgumentCaptor<ExecutionAttributes> attributesCaptor = ArgumentCaptor.forClass(ExecutionAttributes.class);
         doThrow(new RuntimeException("BOOM")).when(interceptor).beforeExecution(any(Context.BeforeExecution.class), attributesCaptor.capture());
-        ExecutionAttribute testAttribute = new ExecutionAttribute<>("TestAttribute");
+        ExecutionAttribute testAttribute = attr("TestAttribute");
         String testValue = "TestValue";
 
         ProtocolRestJsonAsyncClient async = ProtocolRestJsonAsyncClient.builder()
@@ -160,7 +169,7 @@ public void syncClient_requestOverrideExecutionAttribute() {
         ExecutionInterceptor interceptor = mock(ExecutionInterceptor.class);
         ArgumentCaptor<ExecutionAttributes> attributesCaptor = ArgumentCaptor.forClass(ExecutionAttributes.class);
         doThrow(new RuntimeException("BOOM")).when(interceptor).beforeExecution(any(Context.BeforeExecution.class), attributesCaptor.capture());
-        ExecutionAttribute testAttribute = new ExecutionAttribute<>("TestAttribute");
+        ExecutionAttribute testAttribute = attr("TestAttribute");
         String testValue = "TestValue";
 
         ProtocolRestJsonClient sync = ProtocolRestJsonClient.builder()
@@ -188,7 +197,7 @@ public void syncClient_requestOverrideExecutionAttributesHavePrecedence() {
         ExecutionInterceptor interceptor = mock(ExecutionInterceptor.class);
         ArgumentCaptor<ExecutionAttributes> attributesCaptor = ArgumentCaptor.forClass(ExecutionAttributes.class);
         doThrow(new RuntimeException("BOOM")).when(interceptor).beforeExecution(any(Context.BeforeExecution.class), attributesCaptor.capture());
-        ExecutionAttribute testAttribute = new ExecutionAttribute<>("TestAttribute");
+        ExecutionAttribute testAttribute = attr("TestAttribute");
         String testValue = "TestValue";
 
         ProtocolRestJsonClient sync = ProtocolRestJsonClient.builder()
@@ -254,6 +263,6 @@ public void testAttributeEquals() {
     }
 
     private ExecutionAttribute getAttribute(int i) {
-        return new ExecutionAttribute<>("TestAttribute" + i);
+        return attr("TestAttribute" + i);
     }
 }