add testing, clean up

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/MaxAttemptRedirectStrategy.java

@@ -0,0 +1,91 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+package com.azure.core.http.policy;
+
+import com.azure.core.http.HttpMethod;
+import com.azure.core.util.logging.ClientLogger;
+
+import java.util.HashSet;
+import java.util.Objects;
+import java.util.Set;
+
+/**
+ * A default implementation of {@link RedirectStrategy} that uses the provided try count, and status code
+ * to determine if request should be redirected.
+ */
+public class MaxAttemptRedirectStrategy implements RedirectStrategy {
+
+    // Based on Stamp specific redirects design doc
+    static final int MAX_REDIRECT_ATTEMPTS = 10;
+    private static final String LOCATION_HEADER_NAME = "Location";
+
+    private int maxAttempts;
+    private final String locationHeader;
+    private final Set<HttpMethod> redirectMethods;
+
+    /**
+     * Creates an instance of {@link MaxAttemptRedirectStrategy}.
+     *
+     * @param maxAttempts The max number of redirect attempts that can be made.
+     * @param locationHeader The header name containing the redirect URL.
+     * @param allowedMethods The set of {@link HttpMethod} that are allowed to be redirected.
+     *
+     * @throws NullPointerException if {@code locationHeader} is {@code null}.
+     */
+    public MaxAttemptRedirectStrategy(int maxAttempts, String locationHeader, Set<HttpMethod> allowedMethods) {
+        this.maxAttempts = maxAttempts;
+        this.locationHeader = Objects.requireNonNull(locationHeader, "'locationHeader' cannot be null.");
+        this.redirectMethods = allowedMethods;
+    }
+
+    /**
+     * Creates an instance of {@link MaxAttemptRedirectStrategy}.
+     *
+     * @param maxAttempts The max number of redirect attempts that can be made.
+     */
+    public MaxAttemptRedirectStrategy(int maxAttempts) {
+        this.maxAttempts = maxAttempts;
+        if (maxAttempts < 0){
+            ClientLogger logger = new ClientLogger(MaxAttemptRedirectStrategy.class);
+            throw logger.logExceptionAsError(new IllegalArgumentException("Max attempts cannot be less than 0."));
+        }
+        this.locationHeader = LOCATION_HEADER_NAME;
+        this.redirectMethods = new HashSet<HttpMethod>() {
+            {
+                add(HttpMethod.GET);
+                add(HttpMethod.HEAD);
+            }
+        };
+    }
+
+    @Override
+    public int getMaxAttempts() {
+        return maxAttempts;
+    }
+
+    @Override
+    public boolean shouldAttemptRedirect(String redirectUrl, int tryCount, int maxRedirects,
+                                         Set<String> attemptedRedirectUrls) {
+        if (tryCount >= maxRedirects) {
+            logger.error(String.format("Request has been redirected more than %d times.", MAX_REDIRECT_ATTEMPTS));
+            return false;
+        }
+
+        if (attemptedRedirectUrls.contains(redirectUrl)) {
+            logger.error(String.format("Request was redirected more than once to: %s", redirectUrl));
+            return false;
+        }
+        return true;
+    }
+
+    @Override
+    public String getLocationHeader() {
+        return locationHeader;
+    }
+
+    @Override
+    public Set<HttpMethod> getAllowedMethods() {
+        return redirectMethods;
+    }
+}

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RedirectPolicy.java

@@ -18,89 +18,86 @@
 import java.util.Set;
 
 /**
- * A HttpPipeline policy that retries when a HTTP Redirect is received as response.
+ * A HttpPipeline policy that redirects when an HTTP Redirect is received as response.
  */
 public final class RedirectPolicy implements HttpPipelinePolicy {
 
     private static final int PERMANENT_REDIRECT_STATUS_CODE = 308;
-    Set<String> attemptedRedirectLocations = new HashSet<>();
 
     // Based on Stamp specific redirects design doc
-    private static final int MAX_REDIRECT_RETRIES = 10;
-    private String redirectedEndpointUrl;
-
+    private static final int MAX_REDIRECT_ATTEMPTS = 10;
+    private final RedirectStrategy redirectStrategy;
 
-    private final RedirectStrategy retryStrategy;
+    private Set<String> attemptedRedirectUrls = new HashSet<>();
+    private String redirectedEndpointUrl;
 
     /**
-     * Creates {@link RedirectPolicy} with default {@link DefaultRedirectStrategy} as {@link RedirectStrategy} and
-     * use the provided {@code statusCode} to determine if this request should be retried
-     * and MAX_REDIRECT_RETRIES for the retry count.
+     * Creates {@link RedirectPolicy} with default {@link MaxAttemptRedirectStrategy} as {@link RedirectStrategy} and
+     * use the provided {@code statusCode} to determine if this request should be redirected
+     * and MAX_REDIRECT_ATTEMPTS for the try count.
      */
     public RedirectPolicy() {
-        this(new DefaultRedirectStrategy(MAX_REDIRECT_RETRIES));
+        this(new MaxAttemptRedirectStrategy(MAX_REDIRECT_ATTEMPTS));
     }
 
     /**
-     * Creates {@link RedirectPolicy} with default {@link DefaultRedirectStrategy} as {@link RedirectStrategy} and
-     * use the provided {@code statusCode} to determine if this request should be retried.
+     * Creates {@link RedirectPolicy} with default {@link MaxAttemptRedirectStrategy} as {@link RedirectStrategy} and
+     * use the provided {@code statusCode} to determine if this request should be redirected.
      *
-     * @param retryStrategy The {@link RetryStrategy} used for retries.
+     * @param redirectStrategy The {@link RedirectStrategy} used for redirection.
      * @throws NullPointerException When {@code statusCode} is null.
      */
-    public RedirectPolicy(RedirectStrategy retryStrategy) {
-        this.retryStrategy = Objects.requireNonNull(retryStrategy, "'retryStrategy' cannot be null.");
+    public RedirectPolicy(RedirectStrategy redirectStrategy) {
+        this.redirectStrategy = Objects.requireNonNull(redirectStrategy, "'redirectStrategy' cannot be null.");
     }
 
     @Override
     public Mono<HttpResponse> process(HttpPipelineCallContext context, HttpPipelineNextPolicy next) {
-        return attemptRetry(context, next, context.getHttpRequest(), 0);
+        return attemptRedirect(context, next, context.getHttpRequest(), 1);
     }
 
     /**
      * Function to process through the HTTP Response received in the pipeline
-     * and retry sending the request with new redirect url.
+     * and redirect sending the request with new redirect url.
      */
-    private Mono<HttpResponse> attemptRetry(final HttpPipelineCallContext context,
-                                            final HttpPipelineNextPolicy next,
-                                            final HttpRequest originalHttpRequest,
-                                            final int retryCount) {
-        // make sure the context is not modified during retry, except for the URL
+    private Mono<HttpResponse> attemptRedirect(final HttpPipelineCallContext context,
+                                               final HttpPipelineNextPolicy next,
+                                               final HttpRequest originalHttpRequest,
+                                               final int redirectAttempt) {
+        // make sure the context is not modified during redirect, except for the URL
         context.setHttpRequest(originalHttpRequest.copy());
         if (this.redirectedEndpointUrl != null) {
             context.getHttpRequest().setUrl(this.redirectedEndpointUrl);
         }
         return next.clone().process()
             .flatMap(httpResponse -> {
-                if (isRedirectableStatusCode(httpResponse.getStatusCode()) &&
-                    isRedirectableMethod(httpResponse.getRequest().getHttpMethod()) &&
-                    retryStrategy.shouldAttemptRedirect(httpResponse.getHeaders(), retryCount,
-                        attemptedRedirectLocations)) {
-                    String responseLocation =
-                        tryGetRedirectHeader(httpResponse.getHeaders(), retryStrategy.getLocationHeader());
-                    if (responseLocation != null) {
-                        attemptedRedirectLocations.add(responseLocation);
-                        this.redirectedEndpointUrl = responseLocation;
-                        return attemptRetry(context, next, originalHttpRequest, retryCount + 1);
-                    }
+                String responseLocation =
+                    tryGetRedirectHeader(httpResponse.getHeaders(), redirectStrategy.getLocationHeader());
+                if (isValidRedirectStatusCode(httpResponse.getStatusCode()) &&
+                    isAllowedRedirectMethod(httpResponse.getRequest().getHttpMethod()) &&
+                    responseLocation != null &&
+                    redirectStrategy.shouldAttemptRedirect(responseLocation, redirectAttempt,
+                        redirectStrategy.getMaxAttempts(), attemptedRedirectUrls)) {
+                    attemptedRedirectUrls.add(responseLocation);
+                    this.redirectedEndpointUrl = responseLocation;
+                    return attemptRedirect(context, next, originalHttpRequest, redirectAttempt + 1);
                 }
                 return Mono.just(httpResponse);
             });
     }
 
-    private boolean isRedirectableMethod(HttpMethod httpMethod) {
-        return retryStrategy.getRedirectableMethods().contains(httpMethod);
+    private boolean isAllowedRedirectMethod(HttpMethod httpMethod) {
+        return redirectStrategy.getAllowedMethods().contains(httpMethod);
     }
 
-    private boolean isRedirectableStatusCode(int statusCode) {
+    private boolean isValidRedirectStatusCode(int statusCode) {
         return statusCode == HttpURLConnection.HTTP_MOVED_TEMP
             || statusCode == HttpURLConnection.HTTP_MOVED_PERM
             || statusCode == PERMANENT_REDIRECT_STATUS_CODE;
     }
 
     private static String tryGetRedirectHeader(HttpHeaders headers, String headerName) {
         String headerValue = headers.getValue(headerName);
-
         return CoreUtils.isNullOrEmpty(headerValue) ? null : headerValue;
     }
 }

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RedirectStrategy.java

@@ -3,49 +3,45 @@
 
 package com.azure.core.http.policy;
 
-import com.azure.core.http.HttpHeaders;
 import com.azure.core.http.HttpMethod;
-import com.azure.core.http.HttpPipelineCallContext;
-import com.azure.core.http.HttpPipelineNextPolicy;
-import com.azure.core.http.HttpRequest;
-import com.azure.core.http.HttpResponse;
 import com.azure.core.util.logging.ClientLogger;
 
-import java.net.HttpURLConnection;
 import java.util.Set;
 
 /**
- * The interface for determining the retry strategy used in {@link RetryPolicy}.
+ * The interface for determining the redirect strategy used in {@link RedirectPolicy}.
  */
 public interface RedirectStrategy {
-
-
     ClientLogger logger = new ClientLogger(RedirectStrategy.class);
 
     /**
-     * Max number of retry attempts to be make.
+     * Max number of redirect attempts to be made.
      *
-     * @return The max number of retry attempts.
+     * @return The max number of redirect attempts.
+     */
+    int getMaxAttempts();
+
+    /**
+     * @return the value of the header, or null if the header doesn't exist in the response.
      */
-    int getMaxRetries();
     String getLocationHeader();
-    Set<HttpMethod> getRedirectableMethods();
 
     /**
-     *
-     * @param
-     *
+     * @return the set of redirect allowed methods.
      */
+    Set<HttpMethod> getAllowedMethods();
+
     /**
-     * Determines if the url should be redirected between each retry.
+     * Determines if the url should be redirected between each try.
      *
-     * @param responseHeaders the ongoing request headers
-     * @param tryCount redirect retries so far
-     * @param attemptedRedirectLocations attempted redirect retries locations so far
+     * @param redirectUrl the redirect url present in the response headers
+     * @param tryCount redirect attempts so far
+     * @param maxRedirects maximum number of redirects allowed
+     * @param attemptedRedirectUrls attempted redirect locations so far
      *
      * @return {@code true} if the request should be redirected, {@code false}
      * otherwise
      */
-    boolean shouldAttemptRedirect(HttpHeaders responseHeaders, int tryCount, Set<String> attemptedRedirectLocations);
-
+    boolean shouldAttemptRedirect(String redirectUrl, int tryCount, int maxRedirects,
+                                  Set<String> attemptedRedirectUrls);
 }