diff --git a/sdk/core/azure-core/src/main/java/com/azure/core/http/policy/DefaultRedirectStrategy.java b/sdk/core/azure-core/src/main/java/com/azure/core/http/policy/DefaultRedirectStrategy.java deleted file mode 100644 index 882cfb10f3acc..0000000000000 --- a/sdk/core/azure-core/src/main/java/com/azure/core/http/policy/DefaultRedirectStrategy.java +++ /dev/null @@ -1,87 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -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.HttpResponse; -import com.azure.core.util.logging.ClientLogger; - -import java.util.HashSet; -import java.util.Set; - -/** - * A default implementation of {@link RedirectStrategy} that uses the status code to determine if to redirect - * between each retry attempt. - */ -public class DefaultRedirectStrategy implements RedirectStrategy { - - // Based on Stamp specific redirects design doc - static final int MAX_REDIRECT_RETRIES = 10; - private static final String LOCATION_HEADER_NAME = "Location"; - - private final int maxRetries; - private final String locationHeader; - private final Set redirectMethods; - - /** - * Creates an instance of {@link DefaultRedirectStrategy}. - * - * @param maxRetries The max number of redirect attempts that can be made. - */ - public DefaultRedirectStrategy(int maxRetries, String locationHeader, Set redirectableMethods) { - this.maxRetries = maxRetries; - this.locationHeader = locationHeader; - this.redirectMethods = redirectableMethods; - } - - /** - * Creates an instance of {@link DefaultRedirectStrategy}. - * - * @param maxRetries The max number of redirect attempts that can be made. - */ - public DefaultRedirectStrategy(int maxRetries) { - if (maxRetries < 0) { - ClientLogger logger = new ClientLogger(DefaultRedirectStrategy.class); - throw logger.logExceptionAsError(new IllegalArgumentException("Max retries cannot be less than 0.")); - } - this.maxRetries = maxRetries; - this.locationHeader = LOCATION_HEADER_NAME; - this.redirectMethods = new HashSet() { - { - add(HttpMethod.GET); - add(HttpMethod.HEAD); - } - }; - } - - @Override - public int getMaxRetries() { - return maxRetries; - } - - @Override - public boolean shouldAttemptRedirect(HttpHeaders responseHeaders, int tryCount, - Set attemptedRedirectLocations) { - if (tryCount > MAX_REDIRECT_RETRIES) { - logger.error(String.format("Request has been redirected more than %d times.", MAX_REDIRECT_RETRIES)); - return false; - } - if (attemptedRedirectLocations.contains(responseHeaders.get(LOCATION_HEADER_NAME))) { - logger.error(String.format("Request was redirected more than once to: %s", - responseHeaders.get(LOCATION_HEADER_NAME))); - return false; - } - return true; - } - - public String getLocationHeader() { - return locationHeader; - } - - public Set getRedirectableMethods() { - return redirectMethods; - } -} diff --git a/sdk/core/azure-core/src/main/java/com/azure/core/http/policy/MaxAttemptRedirectStrategy.java b/sdk/core/azure-core/src/main/java/com/azure/core/http/policy/MaxAttemptRedirectStrategy.java new file mode 100644 index 0000000000000..4a382c8c0967c --- /dev/null +++ b/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 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 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() { + { + add(HttpMethod.GET); + add(HttpMethod.HEAD); + } + }; + } + + @Override + public int getMaxAttempts() { + return maxAttempts; + } + + @Override + public boolean shouldAttemptRedirect(String redirectUrl, int tryCount, int maxRedirects, + Set 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 getAllowedMethods() { + return redirectMethods; + } +} diff --git a/sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RedirectPolicy.java b/sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RedirectPolicy.java index c2644dafa460a..1798975124c32 100644 --- a/sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RedirectPolicy.java +++ b/sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RedirectPolicy.java @@ -18,81 +18,79 @@ 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 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 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 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 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 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; @@ -100,7 +98,6 @@ private boolean isRedirectableStatusCode(int statusCode) { private static String tryGetRedirectHeader(HttpHeaders headers, String headerName) { String headerValue = headers.getValue(headerName); - return CoreUtils.isNullOrEmpty(headerValue) ? null : headerValue; } } diff --git a/sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RedirectStrategy.java b/sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RedirectStrategy.java index 9552e3584ddba..4b2f30322e688 100644 --- a/sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RedirectStrategy.java +++ b/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 getRedirectableMethods(); /** - * - * @param - * + * @return the set of redirect allowed methods. */ + Set 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 attemptedRedirectLocations); - + boolean shouldAttemptRedirect(String redirectUrl, int tryCount, int maxRedirects, + Set attemptedRedirectUrls); } diff --git a/sdk/core/azure-core/src/test/java/com/azure/core/http/policy/RedirectPolicyTest.java b/sdk/core/azure-core/src/test/java/com/azure/core/http/policy/RedirectPolicyTest.java index efa7556d19bd2..c802ac175003a 100644 --- a/sdk/core/azure-core/src/test/java/com/azure/core/http/policy/RedirectPolicyTest.java +++ b/sdk/core/azure-core/src/test/java/com/azure/core/http/policy/RedirectPolicyTest.java @@ -3,17 +3,19 @@ package com.azure.core.http.policy; +import com.azure.core.http.HttpClient; import com.azure.core.http.HttpHeaders; +import com.azure.core.http.HttpMethod; import com.azure.core.http.HttpPipeline; import com.azure.core.http.HttpPipelineBuilder; import com.azure.core.http.HttpRequest; import com.azure.core.http.HttpResponse; -import com.azure.core.http.HttpMethod; -import com.azure.core.http.HttpClient; import com.azure.core.http.MockHttpResponse; +import com.azure.core.http.clients.NoOpHttpClient; import org.junit.jupiter.api.Test; import reactor.core.publisher.Mono; +import java.net.MalformedURLException; import java.net.URL; import java.util.HashMap; import java.util.Map; @@ -25,7 +27,7 @@ public class RedirectPolicyTest { @Test - public void retryWith308Test() throws Exception { + public void defaultRedirectWhen308() throws Exception { RecordingHttpClient httpClient = new RecordingHttpClient(request -> { if (request.getUrl().toString().equals("http://localhost/")) { Map headers = new HashMap<>(); @@ -49,58 +51,53 @@ public void retryWith308Test() throws Exception { assertEquals(200, response.getStatusCode()); } - // @Test - // public void retryMaxTest() throws Exception { - // RecordingHttpClient httpClient = new RecordingHttpClient(request -> { - // Map headers = new HashMap<>(); - // headers.put("Location", "http://redirecthost/"); - // HttpHeaders httpHeader = new HttpHeaders(headers); - // return Mono.just(new MockHttpResponse(request, 308, httpHeader)); - // }); - // - // HttpPipeline pipeline = new HttpPipelineBuilder() - // .httpClient(httpClient) - // .policies(new AzureMonitorRedirectPolicy()) - // .build(); - // - // HttpResponse response = pipeline.send(new HttpRequest(HttpMethod.GET, - // new URL("http://localhost/"))).block(); - // // redirect is captured only 3 times - // assertEquals(11, httpClient.getCount()); - // assertEquals(308, response.getStatusCode()); - // } - // - // @Test - // public void retryWith308MultipleRequestsTest() throws Exception { - // RecordingHttpClient httpClient = new RecordingHttpClient(request -> { - // if (request.getUrl().toString().equals("http://localhost/")) { - // Map headers = new HashMap<>(); - // headers.put("Location", "http://redirecthost/"); - // HttpHeaders httpHeader = new HttpHeaders(headers); - // return Mono.just(new MockHttpResponse(request, 308, httpHeader)); - // } else { - // return Mono.just(new MockHttpResponse(request, 200)); - // } - // }); - // - // HttpPipeline pipeline = new HttpPipelineBuilder() - // .httpClient(httpClient) - // .policies(new AzureMonitorRedirectPolicy()) - // .build(); - // - // assertEquals(0, httpClient.getCount()); - // HttpResponse response1 = pipeline.send(new HttpRequest(HttpMethod.GET, - // new URL("http://localhost/"))).block(); - // assertEquals(200, response1.getStatusCode()); - // assertEquals(2, httpClient.getCount()); - // - // httpClient.resetCount(); - // HttpResponse response2 = pipeline.send(new HttpRequest(HttpMethod.GET, - // new URL("http://localhost/"))).block(); - // assertEquals(200, response2.getStatusCode()); - // //Make sure the future requests are sent directly to http://redirecthost/ - // assertEquals(1, httpClient.getCount()); - // } + @Test + public void redirectForNAttempts() throws MalformedURLException { + final int[] requestCount = {1}; + RecordingHttpClient httpClient = new RecordingHttpClient(request -> { + Map headers = new HashMap<>(); + headers.put("Location", "http://redirecthost/" + requestCount[0]); + HttpHeaders httpHeader = new HttpHeaders(headers); + requestCount[0]++; + return Mono.just(new MockHttpResponse(request, 308, httpHeader)); + }); + + HttpPipeline pipeline = new HttpPipelineBuilder() + .httpClient(httpClient) + .policies(new RedirectPolicy(new MaxAttemptRedirectStrategy(5))) + .build(); + + HttpResponse response = pipeline.send(new HttpRequest(HttpMethod.GET, + new URL("http://localhost/"))).block(); + + assertEquals(5, httpClient.getCount()); + assertEquals(308, response.getStatusCode()); + } + + @Test + public void noRedirectPolicyTest() throws Exception { + final HttpPipeline pipeline = new HttpPipelineBuilder() + .httpClient(new NoOpHttpClient() { + + @Override + public Mono send(HttpRequest request) { + if (request.getUrl().toString().equals("http://localhost/")) { + Map headers = new HashMap<>(); + headers.put("Location", "http://redirecthost/"); + HttpHeaders httpHeader = new HttpHeaders(headers); + return Mono.just(new MockHttpResponse(request, 308, httpHeader)); + } else { + return Mono.just(new MockHttpResponse(request, 200)); + } + } + }) + .build(); + + HttpResponse response = pipeline.send(new HttpRequest(HttpMethod.GET, + new URL("http://localhost/"))).block(); + + assertEquals(308, response.getStatusCode()); + } static class RecordingHttpClient implements HttpClient { @@ -120,7 +117,6 @@ public Mono send(HttpRequest httpRequest) { int getCount() { return count.get(); } - void resetCount() { count.set(0); }