Make execution attributes immutable and fix other PR comments

Author: Nicholas DeJaco

core/aws-core/src/main/java/software/amazon/awssdk/awscore/client/handler/AwsClientHandlerUtils.java

@@ -40,11 +40,7 @@
 import software.amazon.awssdk.core.client.config.SdkClientOption;
 import software.amazon.awssdk.core.client.handler.ClientExecutionParams;
 import software.amazon.awssdk.core.http.ExecutionContext;
-import software.amazon.awssdk.core.interceptor.ExecutionAttributes;
-import software.amazon.awssdk.core.interceptor.ExecutionInterceptorChain;
-import software.amazon.awssdk.core.interceptor.InterceptorContext;
-import software.amazon.awssdk.core.interceptor.SdkExecutionAttribute;
-import software.amazon.awssdk.core.interceptor.SdkInternalExecutionAttribute;
+import software.amazon.awssdk.core.interceptor.*;
 import software.amazon.awssdk.core.metrics.CoreMetric;
 import software.amazon.awssdk.core.signer.Signer;
 import software.amazon.awssdk.http.SdkHttpFullRequest;
@@ -106,8 +102,7 @@ static <InputT extends SdkRequest, OutputT extends SdkResponse> ExecutionContext
             executionAttributes = clientOverrideExecutionAttributes.merge(executionAttributes);
         }
 
-        Optional<RequestOverrideConfiguration> requestOverrideConfiguration =
-                (Optional<RequestOverrideConfiguration>) originalRequest.overrideConfiguration();
+        Optional<? extends RequestOverrideConfiguration> requestOverrideConfiguration = originalRequest.overrideConfiguration();
         if (requestOverrideConfiguration.isPresent()) {
             executionAttributes = requestOverrideConfiguration.get().executionAttributes().merge(executionAttributes);
         }

core/sdk-core/src/main/java/software/amazon/awssdk/core/RequestOverrideConfiguration.java

@@ -29,6 +29,7 @@
 import software.amazon.awssdk.annotations.SdkPublicApi;
 import software.amazon.awssdk.core.interceptor.ExecutionAttribute;
 import software.amazon.awssdk.core.interceptor.ExecutionAttributes;
+import software.amazon.awssdk.core.interceptor.UnmodifiableExecutionAttributes;
 import software.amazon.awssdk.core.signer.Signer;
 import software.amazon.awssdk.metrics.MetricPublisher;
 import software.amazon.awssdk.utils.CollectionUtils;
@@ -58,7 +59,7 @@ protected RequestOverrideConfiguration(Builder<?> builder) {
         this.apiCallAttemptTimeout = Validate.isPositiveOrNull(builder.apiCallAttemptTimeout(), "apiCallAttemptTimeout");
         this.signer = builder.signer();
         this.metricPublishers = Collections.unmodifiableList(new ArrayList<>(builder.metricPublishers()));
-        this.executionAttributes = builder.executionAttributes().build();
+        this.executionAttributes = new UnmodifiableExecutionAttributes(builder.executionAttributes().build());
     }
 
     /**

core/sdk-core/src/main/java/software/amazon/awssdk/core/client/config/ClientOverrideConfiguration.java

@@ -28,6 +28,7 @@
 import software.amazon.awssdk.core.interceptor.ExecutionAttribute;
 import software.amazon.awssdk.core.interceptor.ExecutionAttributes;
 import software.amazon.awssdk.core.interceptor.ExecutionInterceptor;
+import software.amazon.awssdk.core.interceptor.UnmodifiableExecutionAttributes;
 import software.amazon.awssdk.core.retry.RetryMode;
 import software.amazon.awssdk.core.retry.RetryPolicy;
 import software.amazon.awssdk.core.sync.ResponseTransformer;
@@ -74,7 +75,7 @@ private ClientOverrideConfiguration(Builder builder) {
         this.defaultProfileFile = builder.defaultProfileFile();
         this.defaultProfileName = builder.defaultProfileName();
         this.metricPublishers = Collections.unmodifiableList(new ArrayList<>(builder.metricPublishers()));
-        this.executionAttributes = builder.executionAttributes().build();
+        this.executionAttributes = new UnmodifiableExecutionAttributes(builder.executionAttributes().build());
     }
 
     @Override

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

@@ -16,6 +16,9 @@
 package software.amazon.awssdk.core.interceptor;
 
 import software.amazon.awssdk.annotations.SdkPublicApi;
+import software.amazon.awssdk.core.RequestOverrideConfiguration;
+
+import java.util.Objects;
 
 /**
  * An attribute attached to a particular execution, stored in {@link ExecutionAttributes}.
@@ -57,4 +60,32 @@ public ExecutionAttribute(String name) {
     public String toString() {
         return name;
     }
+
+    /**
+     * This override considers execution attributes with the same name
+     * to be the same object for the purpose of attribute merge.
+     * @return boolean indicating whether the objects are equal or not.
+     */
+    @Override
+    public boolean equals(Object o) {
+        if (this == o) {
+            return true;
+        }
+        if (o == null || getClass() != o.getClass()) {
+            return false;
+        }
+
+        ExecutionAttribute that = (ExecutionAttribute) o;
+        return that.name.equals(this.name);
+    }
+
+    /**
+     * This override considers execution attributes with the same name
+     * to be the same object for the purpose of attribute merge.
+     * @return hash code
+     */
+    @Override
+    public int hashCode() {
+        return Objects.hashCode(name);
+    }
 }

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

@@ -31,14 +31,14 @@
  */
 @SdkPublicApi
 @NotThreadSafe
-public final class ExecutionAttributes implements ToCopyableBuilder<ExecutionAttributes.Builder, ExecutionAttributes> {
+public class ExecutionAttributes implements ToCopyableBuilder<ExecutionAttributes.Builder, ExecutionAttributes> {
     private final Map<ExecutionAttribute<?>, Object> attributes;
 
     public ExecutionAttributes() {
         this.attributes = new HashMap<>();
     }
 
-    private ExecutionAttributes(Map<? extends ExecutionAttribute<?>, ?> attributes) {
+    protected ExecutionAttributes(Map<? extends ExecutionAttribute<?>, ?> attributes) {
         this.attributes = new HashMap<>(attributes);
     }
 
@@ -66,20 +66,13 @@ public <U> ExecutionAttributes putAttribute(ExecutionAttribute<U> attribute, U v
         return this;
     }
 
-    /**
-     * Add all execution attributes from the provided collection to the internal collection of attributes.
-     */
-    public <U> ExecutionAttributes putAllAttributes(Map<ExecutionAttribute<?>, ?> executionAttributes) {
-        this.attributes.putAll(executionAttributes);
-        return this;
-    }
-
     /**
     * Merge attributes of a lower precedence into the current higher precedence current collection.
     */
-    public <U> ExecutionAttributes merge(ExecutionAttributes lowerPrecedenceExecutionAttributes) {
-        lowerPrecedenceExecutionAttributes.getAttributes().forEach(this.attributes::putIfAbsent);
-        return this;
+    public ExecutionAttributes merge(ExecutionAttributes lowerPrecedenceExecutionAttributes) {
+        Map<ExecutionAttribute<?>, Object> copiedAttributes = new HashMap<>(this.attributes);
+        lowerPrecedenceExecutionAttributes.getAttributes().forEach(copiedAttributes::putIfAbsent);
+        return new ExecutionAttributes(copiedAttributes);
     }
 
     /**
@@ -110,7 +103,7 @@ public static final class Builder implements CopyableBuilder<ExecutionAttributes
         private Builder() {
         }
 
-        @SuppressWarnings("unchecked") // Cast is safe due to implementation of {@link #put}
+
         public <T> T get(ExecutionAttribute<T> key) {
             Validate.notNull(key, "Key to retrieve must not be null.");
             return (T) executionAttributes.get(key);

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

@@ -0,0 +1,32 @@
+/*
+ * 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;
+
+public  class UnmodifiableExecutionAttributes extends ExecutionAttributes {
+    public UnmodifiableExecutionAttributes(ExecutionAttributes executionAttributes) {
+        super(executionAttributes.getAttributes());
+    }
+
+    @Override
+    public <U> ExecutionAttributes putAttribute(ExecutionAttribute<U> attribute, U value) {
+        throw new UnsupportedOperationException();
+    }
+
+    @Override
+    public <U> ExecutionAttributes putAttributeIfAbsent(ExecutionAttribute<U> attribute, U value) {
+        throw new UnsupportedOperationException();
+    }
+}

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

@@ -17,6 +17,7 @@
 
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.Assert.fail;
 import static org.mockito.Mockito.mock;
 
 import java.util.ArrayList;
@@ -26,6 +27,7 @@
 import java.util.List;
 import java.util.Map;
 import org.junit.Test;
+import software.amazon.awssdk.core.client.config.ClientOverrideConfiguration;
 import software.amazon.awssdk.core.interceptor.ExecutionAttribute;
 import software.amazon.awssdk.metrics.MetricPublisher;
 import software.amazon.awssdk.utils.ImmutableMap;
@@ -183,6 +185,25 @@ public void executionAttributes_createsCopy() {
         assertThat(overrideConfig.executionAttributes().getAttribute(testAttribute)).isEqualTo(expectedValue);
     }
 
+    @Test
+    public void executionAttributes_isImmutable() {
+        Map<ExecutionAttribute<?>, Object> executionAttributes = new HashMap<>();
+
+        ExecutionAttribute testAttribute = new ExecutionAttribute("TestAttribute");
+        String expectedValue = "Value1";
+        executionAttributes.put(testAttribute, expectedValue);
+
+        SdkRequestOverrideConfiguration overrideConfig = SdkRequestOverrideConfiguration.builder()
+                .executionAttributes(executionAttributes)
+                .build();
+        try {
+            overrideConfig.executionAttributes().putAttribute(testAttribute, 2);
+            fail("Expected unsupported operation exception");
+        } catch(Exception ex) {
+            assertThat(ex instanceof UnsupportedOperationException).isTrue();
+        }
+    }
+
     @Test
     public void executionAttributes_maintainsAllAdded() {
         Map<ExecutionAttribute, Object> executionAttributeObjectMap = new HashMap<>();

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

@@ -16,8 +16,7 @@
 package software.amazon.awssdk.core.client.config;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.*;
 import static org.mockito.Mockito.mock;
 
 import java.util.ArrayList;
@@ -198,6 +197,26 @@ public void executionAttributes_createsCopy() {
         assertThat(overrideConfig.executionAttributes().getAttribute(testAttribute)).isEqualTo(expectedValue);
     }
 
+    @Test
+    public void executionAttributes_isImmutable() {
+        Map<ExecutionAttribute<?>, Object> executionAttributes = new HashMap<>();
+
+        ExecutionAttribute testAttribute = new ExecutionAttribute("TestAttribute");
+        String expectedValue = "Value1";
+        executionAttributes.put(testAttribute, expectedValue);
+
+        ClientOverrideConfiguration overrideConfig = ClientOverrideConfiguration.builder()
+                .executionAttributes(executionAttributes)
+                .build();
+
+        try {
+            overrideConfig.executionAttributes().putAttribute(testAttribute, 2);
+            fail("Expected unsupported operation exception");
+        } catch(Exception ex) {
+            assertThat(ex instanceof UnsupportedOperationException).isTrue();
+        }
+    }
+
     @Test
     public void executionAttributes_maintainsAllAdded() {
         Map<ExecutionAttribute, Object> executionAttributeObjectMap = new HashMap<>();

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

@@ -149,4 +149,29 @@ public void asyncClient_requestOverrideExecutionAttributesHavePrecedence() {
             async.close();
         }
     }
+
+    @Test
+    public void testExecutionAttributesMerge() {
+        ExecutionAttributes lowerPrecedence = new ExecutionAttributes();
+        for (int i = 0; i < 3; i++) {
+            lowerPrecedence.putAttribute(getAttribute(i), 1);
+        }
+
+        ExecutionAttributes higherPrecendence = new ExecutionAttributes();
+        for (int i = 2; i < 4; i++) {
+            higherPrecendence.putAttribute(getAttribute(i), 2);
+        }
+
+        ExecutionAttributes expectedAttributes = new ExecutionAttributes();
+        for (int i = 0; i < 4; i++) {
+            expectedAttributes.putAttribute(getAttribute(i),  i >= 2 ? 2 : 1);
+        }
+
+        ExecutionAttributes mergedAttributes = higherPrecendence.merge(lowerPrecedence);
+        assertThat(mergedAttributes.getAttributes()).isEqualTo(expectedAttributes.getAttributes());
+    }
+
+    private ExecutionAttribute getAttribute(int i) {
+        return new ExecutionAttribute<>("TestAttribute" + i);
+    }
 }