Add default retry predicates to user provided ones (#5724)

* Add SDK and AWS defaults retry predicate when retry strategy contains user specified predicates. Add a configuration to opt-out, to have the retry strategy use only the use user-provided predicates.

* changelog, checkstyle

* checkstyle, to string

* checkstyle, cleanup

* use a new protected api, DefaultAwareRetryStrategy, for the methods related to default retry predicates in AwsDefaultClientBuilder

* add defaults explicitly to the retry strategy builder using RetryStrategyDefaults interface

* javadoc

.changes/next-release/bugfix-AWSSDKforJavav2-ac30dda.json

@@ -0,0 +1,6 @@
+{
+    "type": "bugfix",
+    "category": "AWS SDK for Java v2",
+    "contributor": "",
+    "description": "Add default retry predicates on top of user-provided ones"
+}

core/aws-core/pom.xml

@@ -165,6 +165,11 @@
             <artifactId>mockito-core</artifactId>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <groupId>org.mockito</groupId>
+            <artifactId>mockito-junit-jupiter</artifactId>
+            <scope>test</scope>
+        </dependency>
         <dependency>
             <groupId>nl.jqno.equalsverifier</groupId>
             <artifactId>equalsverifier</artifactId>

core/aws-core/src/main/java/software/amazon/awssdk/awscore/client/builder/AwsDefaultClientBuilder.java

@@ -65,7 +65,7 @@
 import software.amazon.awssdk.regions.ServiceMetadataAdvancedOption;
 import software.amazon.awssdk.regions.providers.DefaultAwsRegionProviderChain;
 import software.amazon.awssdk.retries.api.RetryStrategy;
-import software.amazon.awssdk.retries.internal.BaseRetryStrategy;
+import software.amazon.awssdk.retries.internal.DefaultAwareRetryStrategy;
 import software.amazon.awssdk.utils.AttributeMap;
 import software.amazon.awssdk.utils.AttributeMap.LazyValueSource;
 import software.amazon.awssdk.utils.CollectionUtils;
@@ -76,20 +76,18 @@
 /**
  * An SDK-internal implementation of the methods in {@link AwsClientBuilder}, {@link AwsAsyncClientBuilder} and
  * {@link AwsSyncClientBuilder}. This implements all methods required by those interfaces, allowing service-specific builders to
- * just
- * implement the configuration they wish to add.
+ * just implement the configuration they wish to add.
  *
  * <p>By implementing both the sync and async interface's methods, service-specific builders can share code between their sync
- * and
- * async variants without needing one to extend the other. Note: This only defines the methods in the sync and async builder
+ * and async variants without needing one to extend the other. Note: This only defines the methods in the sync and async builder
  * interfaces. It does not implement the interfaces themselves. This is because the sync and async client builder interfaces both
  * require a type-constrained parameter for use in fluent chaining, and a generic type parameter conflict is introduced into the
  * class hierarchy by this interface extending the builder interfaces themselves.</p>
  *
  * <p>Like all {@link AwsClientBuilder}s, this class is not thread safe.</p>
  *
  * @param <BuilderT> The type of builder, for chaining.
- * @param <ClientT> The type of client generated by this builder.
+ * @param <ClientT>  The type of client generated by this builder.
  */
 @SdkProtectedApi
 public abstract class AwsDefaultClientBuilder<BuilderT extends AwsClientBuilder<BuilderT, ClientT>, ClientT>
@@ -228,8 +226,8 @@ protected final SdkClientConfiguration setOverrides(SdkClientConfiguration confi
     }
 
     /**
-     * Check {@link SdkInternalTestAdvancedClientOption#ENDPOINT_OVERRIDDEN_OVERRIDE} to see if we should override the
-     * value returned by {@link SdkClientOption#CLIENT_ENDPOINT_PROVIDER}'s isEndpointOverridden.
+     * Check {@link SdkInternalTestAdvancedClientOption#ENDPOINT_OVERRIDDEN_OVERRIDE} to see if we should override the value
+     * returned by {@link SdkClientOption#CLIENT_ENDPOINT_PROVIDER}'s isEndpointOverridden.
      */
     private void checkEndpointOverriddenOverride(SdkClientConfiguration configuration, SdkClientConfiguration.Builder builder) {
         Optional<Boolean> endpointOverriddenOverride =
@@ -339,16 +337,16 @@ private ClientEndpointProvider resolveClientEndpointProvider(LazyValueSource con
     }
 
     /**
-     * Resolve the client endpoint. This code is only needed by old SDK client versions. Newer SDK client versions
-     * resolve this information from the client endpoint provider.
+     * Resolve the client endpoint. This code is only needed by old SDK client versions. Newer SDK client versions resolve this
+     * information from the client endpoint provider.
      */
     private URI resolveEndpoint(LazyValueSource config) {
         return config.get(SdkClientOption.CLIENT_ENDPOINT_PROVIDER).clientEndpoint();
     }
 
     /**
-     * Resolve whether the endpoint was overridden by the customer. This code is only needed by old SDK client
-     * versions. Newer SDK client versions resolve this information from the client endpoint provider.
+     * Resolve whether the endpoint was overridden by the customer. This code is only needed by old SDK client versions. Newer SDK
+     * client versions resolve this information from the client endpoint provider.
      */
     private boolean resolveEndpointOverridden(LazyValueSource config) {
         return config.get(SdkClientOption.CLIENT_ENDPOINT_PROVIDER).isEndpointOverridden();
@@ -423,28 +421,25 @@ private void configureRetryPolicy(SdkClientConfiguration.Builder config) {
 
     private void configureRetryStrategy(SdkClientConfiguration.Builder config) {
         RetryStrategy strategy = config.option(SdkClientOption.RETRY_STRATEGY);
-        if (strategy != null) {
-            // TODO(10/09/24) This is a temporal workaround and not a long term solution. It will fail to add the SDK and AWS
-            //  defaults if the users add one or more if their own retry predicates. A long term fix is needed that can
-            //  "remember" which defaults have been already applied, e.g.,
-            //    if (strategy.shouldAddDefaults("aws")) {
-            //       strategy = strategy.toBuilder()
-            //                          .applyMutation(AwsRetryStrategy::applyDefaults)
-            //                          .markDefaultsAdded("aws")
-            //                          .build();
-            //    }
-            if (strategy.maxAttempts() > 1
-                && (strategy instanceof BaseRetryStrategy)
-                && !((BaseRetryStrategy) strategy).hasRetryPredicates()
-            ) {
-                RetryStrategy.Builder<?, ?> builder = strategy.toBuilder();
-                SdkDefaultRetryStrategy.configureStrategy(builder);
-                AwsRetryStrategy.configureStrategy(builder);
-                config.option(SdkClientOption.RETRY_STRATEGY, builder.build());
-            }
+        if (strategy == null) {
+            config.lazyOption(SdkClientOption.RETRY_STRATEGY, this::resolveAwsRetryStrategy);
             return;
         }
-        config.lazyOption(SdkClientOption.RETRY_STRATEGY, this::resolveAwsRetryStrategy);
+
+        if (!strategy.useClientDefaults()) {
+            return;
+        }
+
+        if (!(strategy instanceof DefaultAwareRetryStrategy)) {
+            return;
+        }
+        DefaultAwareRetryStrategy defaultAwareRetryStrategy = (DefaultAwareRetryStrategy) strategy;
+        defaultAwareRetryStrategy = defaultAwareRetryStrategy.addDefaults(AwsRetryStrategy.retryStrategyDefaults());
+        defaultAwareRetryStrategy = defaultAwareRetryStrategy.addDefaults(SdkDefaultRetryStrategy.retryStrategyDefaults());
+
+        RetryStrategy.Builder<?, ?> strategyBuilder = defaultAwareRetryStrategy.toBuilder();
+        config.option(SdkClientOption.RETRY_STRATEGY, strategyBuilder.build());
+
     }
 
     private RetryStrategy resolveAwsRetryStrategy(LazyValueSource config) {
@@ -536,8 +531,8 @@ public final void setDefaultsMode(DefaultsMode defaultsMode) {
 
     /**
      * If the region is a FIPS pseudo region (contains "fips"), this method returns a pair of values, the left side being the
-     * region with the "fips" string removed, and the right being {@code true}. Otherwise, the region is returned
-     * unchanged, and the right will be empty.
+     * region with the "fips" string removed, and the right being {@code true}. Otherwise, the region is returned unchanged, and
+     * the right will be empty.
      */
     private static Pair<Region, Optional<Boolean>> transformFipsPseudoRegionIfNecessary(Region region) {
         String id = region.id();

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

@@ -26,13 +26,30 @@
 import software.amazon.awssdk.retries.LegacyRetryStrategy;
 import software.amazon.awssdk.retries.StandardRetryStrategy;
 import software.amazon.awssdk.retries.api.RetryStrategy;
+import software.amazon.awssdk.retries.internal.DefaultAwareRetryStrategy;
+import software.amazon.awssdk.retries.internal.RetryStrategyDefaults;
 
 /**
  * Retry strategies used by clients when communicating with AWS services.
  */
 @SdkPublicApi
 public final class AwsRetryStrategy {
 
+    private static final String DEFAULTS_NAME = "aws";
+
+    private static final RetryStrategyDefaults DEFAULTS_PREDICATES = new RetryStrategyDefaults() {
+        @Override
+        public String name() {
+            return DEFAULTS_NAME;
+        }
+
+        @Override
+        public void applyDefaults(RetryStrategy.Builder<?, ?> retryStrategyBuilder) {
+            configureStrategy(retryStrategyBuilder);
+            markDefaultsAdded(retryStrategyBuilder);
+        }
+    };
+
     private AwsRetryStrategy() {
     }
 
@@ -127,7 +144,9 @@ public static AdaptiveRetryStrategy adaptiveRetryStrategy() {
      * @return The given builder
      */
     public static <T extends RetryStrategy.Builder<T, ?>> T configure(T builder) {
-        return builder.retryOnException(AwsRetryStrategy::retryOnAwsRetryableErrors);
+        builder.retryOnException(AwsRetryStrategy::retryOnAwsRetryableErrors);
+        markDefaultsAdded(builder);
+        return builder;
     }
 
     /**
@@ -162,4 +181,15 @@ private static RetryStrategy legacyAdaptiveRetryStrategy() {
                                  .build();
     }
 
+    public static RetryStrategyDefaults retryStrategyDefaults() {
+        return DEFAULTS_PREDICATES;
+    }
+
+    private static void markDefaultsAdded(RetryStrategy.Builder<?, ?> builder) {
+        if (builder instanceof DefaultAwareRetryStrategy.Builder) {
+            DefaultAwareRetryStrategy.Builder b = (DefaultAwareRetryStrategy.Builder) builder;
+            b.markDefaultAdded(DEFAULTS_NAME);
+        }
+    }
+
 }

core/aws-core/src/test/java/software/amazon/awssdk/awscore/client/builder/DefaultAwsClientBuilderTest.java

@@ -35,29 +35,43 @@
 import java.time.Duration;
 import java.util.Arrays;
 import java.util.Optional;
-import org.junit.Before;
-import org.junit.Test;
-import org.junit.runner.RunWith;
+import java.util.function.Predicate;
+import java.util.stream.Stream;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
 import org.mockito.Mock;
-import org.mockito.junit.MockitoJUnitRunner;
 import software.amazon.awssdk.auth.credentials.AnonymousCredentialsProvider;
 import software.amazon.awssdk.auth.signer.Aws4Signer;
 import software.amazon.awssdk.awscore.client.config.AwsClientOption;
 import software.amazon.awssdk.awscore.internal.defaultsmode.AutoDefaultsModeDiscovery;
+import software.amazon.awssdk.awscore.retry.AwsRetryStrategy;
 import software.amazon.awssdk.core.client.config.ClientOverrideConfiguration;
 import software.amazon.awssdk.core.client.config.SdkClientConfiguration;
 import software.amazon.awssdk.core.client.config.SdkClientOption;
+import software.amazon.awssdk.core.internal.retry.SdkDefaultRetryStrategy;
 import software.amazon.awssdk.core.signer.Signer;
 import software.amazon.awssdk.http.SdkHttpClient;
 import software.amazon.awssdk.http.SdkHttpConfigurationOption;
 import software.amazon.awssdk.http.async.SdkAsyncHttpClient;
 import software.amazon.awssdk.regions.Region;
+import software.amazon.awssdk.retries.AdaptiveRetryStrategy;
+import software.amazon.awssdk.retries.DefaultRetryStrategy;
+import software.amazon.awssdk.retries.LegacyRetryStrategy;
+import software.amazon.awssdk.retries.StandardRetryStrategy;
+import software.amazon.awssdk.retries.api.BackoffStrategy;
+import software.amazon.awssdk.retries.api.RetryStrategy;
+import software.amazon.awssdk.retries.internal.BaseRetryStrategy;
 import software.amazon.awssdk.utils.AttributeMap;
+import org.mockito.junit.jupiter.MockitoExtension;
 
 /**
  * Validate the functionality of the {@link AwsDefaultClientBuilder}.
  */
-@RunWith(MockitoJUnitRunner.class)
+@ExtendWith(MockitoExtension.class)
 public class DefaultAwsClientBuilderTest {
 
     private static final AttributeMap MOCK_DEFAULTS = AttributeMap
@@ -71,16 +85,16 @@ public class DefaultAwsClientBuilderTest {
     private static final Signer TEST_SIGNER = Aws4Signer.create();
     private static final URI ENDPOINT = URI.create("https://example.com");
 
-    @Mock
+    @Mock(lenient = true)
     private SdkHttpClient.Builder defaultHttpClientBuilder;
 
-    @Mock
+    @Mock(lenient = true)
     private SdkAsyncHttpClient.Builder defaultAsyncHttpClientFactory;
 
     @Mock
     private AutoDefaultsModeDiscovery autoModeDiscovery;
 
-    @Before
+    @BeforeEach
     public void setup() {
         when(defaultHttpClientBuilder.buildWithDefaults(any())).thenReturn(mock(SdkHttpClient.class));
         when(defaultAsyncHttpClientFactory.buildWithDefaults(any())).thenReturn(mock(SdkAsyncHttpClient.class));
@@ -251,6 +265,102 @@ public void clientBuilderFieldsHaveBeanEquivalents() throws Exception {
         });
     }
 
+    @ParameterizedTest(name = "{0} - expectedPredicateAmount: {2}")
+    @MethodSource("retryArguments")
+    void retryStrategyConfiguration_shouldAddDefaultPredicatesWhenRequired(
+        String testDescription, RetryStrategy retryStrategy, int expectedPredicateAmount) {
+        TestClientBuilder builder = testClientBuilder()
+            .region(Region.US_EAST_1)
+            .overrideConfiguration(c -> c.retryStrategy(retryStrategy));
+        TestClient client = builder.build();
+
+        ClientOverrideConfiguration conf = client.clientConfiguration.asOverrideConfiguration();
+        assertThat(conf.retryStrategy()).isPresent();
+
+        RetryStrategy configuredRetryStrategy = conf.retryStrategy().get();
+        BaseRetryStrategy baseRetryStrategy = (BaseRetryStrategy) configuredRetryStrategy;
+        assertThat(baseRetryStrategy.shouldAddDefaults(AwsRetryStrategy.retryStrategyDefaults().name())).isFalse();
+        assertThat(baseRetryStrategy.shouldAddDefaults(SdkDefaultRetryStrategy.retryStrategyDefaults().name())).isFalse();
+        assertThat(baseRetryStrategy.retryPredicates()).hasSize(expectedPredicateAmount);
+    }
+
+    private static Stream<Arguments> retryArguments() {
+        return Stream.of(
+            Arguments.of("Standard - static method creation",
+                         SdkDefaultRetryStrategy.standardRetryStrategy(), 9),
+            Arguments.of("Standard - static builder method",
+                         DefaultRetryStrategy.standardStrategyBuilder().build(), 9),
+            Arguments.of("Standard - builder with retryOnException",
+                         StandardRetryStrategy.builder()
+                                              .retryOnException(TestException.class)
+                                              .backoffStrategy(BackoffStrategy.retryImmediately())
+                                              .throttlingBackoffStrategy(BackoffStrategy.retryImmediately())
+                                              .build(), 10),
+            Arguments.of("Standard - useClientDefaults=false",
+                         StandardRetryStrategy.builder()
+                                              .retryOnException(TestException.class)
+                                              .backoffStrategy(BackoffStrategy.retryImmediately())
+                                              .throttlingBackoffStrategy(BackoffStrategy.retryImmediately())
+                                              .useClientDefaults(false)
+                                              .build(), 1),
+            Arguments.of("Adaptive - static method creation",
+                         SdkDefaultRetryStrategy.adaptiveRetryStrategy(), 9),
+            Arguments.of("Adaptive - builder with retryOnException",
+                         AdaptiveRetryStrategy.builder()
+                                              .retryOnException(TestException.class)
+                                              .backoffStrategy(BackoffStrategy.retryImmediately())
+                                              .throttlingBackoffStrategy(BackoffStrategy.retryImmediately())
+                                              .build(), 10),
+            Arguments.of("Adaptive - useClientDefaults=false",
+                         AdaptiveRetryStrategy.builder()
+                                              .retryOnException(TestException.class)
+                                              .backoffStrategy(BackoffStrategy.retryImmediately())
+                                              .throttlingBackoffStrategy(BackoffStrategy.retryImmediately())
+                                              .useClientDefaults(false)
+                                              .build(), 1),
+            Arguments.of("Legacy - static method creation",
+                         SdkDefaultRetryStrategy.legacyRetryStrategy(), 9),
+            Arguments.of("Legacy - builder with retryOnException",
+                         LegacyRetryStrategy.builder()
+                                            .retryOnException(TestException.class)
+                                            .backoffStrategy(BackoffStrategy.retryImmediately())
+                                            .throttlingBackoffStrategy(BackoffStrategy.retryImmediately())
+                                            .build(), 10),
+            Arguments.of("Legacy - useClientDefaults=false",
+                         LegacyRetryStrategy.builder()
+                                            .retryOnException(TestException.class)
+                                            .backoffStrategy(BackoffStrategy.retryImmediately())
+                                            .throttlingBackoffStrategy(BackoffStrategy.retryImmediately())
+                                            .useClientDefaults(false)
+                                            .build(), 1)
+        );
+    }
+
+    @ParameterizedTest(name = "{0} - expectedPredicateAmount: ({2}+2)")
+    @MethodSource("retryArguments")
+    void retryBuilderRoundTrip_ShouldKeepDefaults_ShouldAddNewPredicates(
+        String testDescription, RetryStrategy retryStrategy, int expectedPredicateAmount) {
+        RetryStrategy.Builder<?, ?> retryBuilder = retryStrategy.toBuilder();
+        Predicate<Throwable> newDummyPredicate1 = t -> t instanceof RuntimeException;
+        Predicate<Throwable> newDummyPredicate2 = t -> t instanceof IllegalArgumentException;
+        retryBuilder.retryOnException(newDummyPredicate1);
+        retryBuilder.retryOnException(newDummyPredicate2);
+
+        TestClientBuilder clientBuilder = testClientBuilder()
+            .region(Region.US_EAST_1)
+            .overrideConfiguration(c -> c.retryStrategy(retryBuilder.build()));
+        TestClient client = clientBuilder.build();
+
+        ClientOverrideConfiguration conf = client.clientConfiguration.asOverrideConfiguration();
+        assertThat(conf.retryStrategy()).isPresent();
+
+        RetryStrategy configuredRetryStrategy = conf.retryStrategy().get();
+        BaseRetryStrategy baseRetryStrategy = (BaseRetryStrategy) configuredRetryStrategy;
+        assertThat(baseRetryStrategy.shouldAddDefaults(AwsRetryStrategy.retryStrategyDefaults().name())).isFalse();
+        assertThat(baseRetryStrategy.shouldAddDefaults(SdkDefaultRetryStrategy.retryStrategyDefaults().name())).isFalse();
+        assertThat(baseRetryStrategy.retryPredicates()).hasSize(expectedPredicateAmount + 2);
+    }
+
     private AwsClientBuilder<TestClientBuilder, TestClient> testClientBuilder() {
         ClientOverrideConfiguration overrideConfig =
             ClientOverrideConfiguration.builder()
@@ -354,4 +464,7 @@ protected AttributeMap serviceHttpConfig() {
             return MOCK_DEFAULTS;
         }
     }
+
+    private static class TestException extends Throwable {
+    }
 }