hc-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ol...@apache.org
Subject [4/4] httpcomponents-client git commit: HTTPCLIENT-1893: Moved HttpContext state logic out of DefaultRedirectStrategy to the redirect execution interceptors; redesigned RedirectLocations class; refactored classic redirect execution interceptor unit tests
Date Wed, 03 Jan 2018 18:47:28 GMT
HTTPCLIENT-1893: Moved HttpContext state logic out of DefaultRedirectStrategy to the redirect execution interceptors; redesigned RedirectLocations class; refactored classic redirect execution interceptor unit tests


Project: http://git-wip-us.apache.org/repos/asf/httpcomponents-client/repo
Commit: http://git-wip-us.apache.org/repos/asf/httpcomponents-client/commit/47a0eb8b
Tree: http://git-wip-us.apache.org/repos/asf/httpcomponents-client/tree/47a0eb8b
Diff: http://git-wip-us.apache.org/repos/asf/httpcomponents-client/diff/47a0eb8b

Branch: refs/heads/master
Commit: 47a0eb8b65c2a98b50567bf6e0b4229a08035098
Parents: 71060c7
Author: Oleg Kalnichevski <olegk@apache.org>
Authored: Wed Jan 3 19:36:04 2018 +0100
Committer: Oleg Kalnichevski <olegk@apache.org>
Committed: Wed Jan 3 19:37:19 2018 +0100

----------------------------------------------------------------------
 .../sync/TestClientRequestExecution.java        |   6 +-
 .../hc/client5/testing/sync/TestRedirects.java  |   9 +-
 .../http/impl/DefaultRedirectStrategy.java      |  46 +---
 .../http/impl/async/AsyncRedirectExec.java      |  63 ++++--
 .../client5/http/impl/classic/RedirectExec.java |  64 ++++--
 .../http/protocol/HttpClientContext.java        |   9 +-
 .../http/protocol/RedirectLocations.java        | 121 +---------
 .../http/impl/TestDefaultRedirectStrategy.java  |  49 ----
 .../http/impl/classic/TestRedirectExec.java     | 226 +++++++++++--------
 .../http/protocol/TestRedirectLocation.java     |  12 +-
 10 files changed, 227 insertions(+), 378 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/httpcomponents-client/blob/47a0eb8b/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestClientRequestExecution.java
----------------------------------------------------------------------
diff --git a/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestClientRequestExecution.java b/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestClientRequestExecution.java
index f89d4a1..d0dd55d 100644
--- a/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestClientRequestExecution.java
+++ b/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestClientRequestExecution.java
@@ -29,12 +29,12 @@ package org.apache.hc.client5.testing.sync;
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
 import java.net.URI;
-import java.util.List;
 
 import org.apache.hc.client5.http.HttpRequestRetryHandler;
 import org.apache.hc.client5.http.classic.methods.HttpGet;
 import org.apache.hc.client5.http.classic.methods.HttpPost;
 import org.apache.hc.client5.http.protocol.HttpClientContext;
+import org.apache.hc.client5.http.protocol.RedirectLocations;
 import org.apache.hc.client5.http.utils.URIUtils;
 import org.apache.hc.core5.http.ClassicHttpRequest;
 import org.apache.hc.core5.http.ClassicHttpResponse;
@@ -252,8 +252,8 @@ public class TestClientRequestExecution extends LocalServerTestBase {
         final HttpRequest request = context.getRequest();
         Assert.assertEquals("/stuff", request.getRequestUri());
 
-        final List<URI> redirectLocations = context.getRedirectLocations();
-        final URI location = URIUtils.resolve(uri, target, redirectLocations);
+        final RedirectLocations redirectLocations = context.getRedirectLocations();
+        final URI location = URIUtils.resolve(uri, target, redirectLocations.getAll());
         Assert.assertEquals(uri, location);
     }
 

http://git-wip-us.apache.org/repos/asf/httpcomponents-client/blob/47a0eb8b/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestRedirects.java
----------------------------------------------------------------------
diff --git a/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestRedirects.java b/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestRedirects.java
index ab8ec57..aedefd6 100644
--- a/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestRedirects.java
+++ b/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestRedirects.java
@@ -30,7 +30,6 @@ import java.io.IOException;
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.util.Arrays;
-import java.util.List;
 
 import org.apache.hc.client5.http.CircularRedirectException;
 import org.apache.hc.client5.http.ClientProtocolException;
@@ -42,6 +41,7 @@ import org.apache.hc.client5.http.cookie.BasicCookieStore;
 import org.apache.hc.client5.http.cookie.CookieStore;
 import org.apache.hc.client5.http.impl.cookie.BasicClientCookie;
 import org.apache.hc.client5.http.protocol.HttpClientContext;
+import org.apache.hc.client5.http.protocol.RedirectLocations;
 import org.apache.hc.client5.http.utils.URIUtils;
 import org.apache.hc.core5.http.ClassicHttpRequest;
 import org.apache.hc.core5.http.ClassicHttpResponse;
@@ -282,8 +282,9 @@ public class TestRedirects extends LocalServerTestBase {
         Assert.assertEquals(HttpStatus.SC_MULTIPLE_CHOICES, response.getCode());
         Assert.assertEquals(URIUtils.create(target, "/oldlocation/"), reqWrapper.getUri());
 
-        final List<URI> redirects = context.getRedirectLocations();
-        Assert.assertNull(redirects);
+        final RedirectLocations redirects = context.getRedirectLocations();
+        Assert.assertNotNull(redirects);
+        Assert.assertEquals(0, redirects.size());
     }
 
     @Test
@@ -304,7 +305,7 @@ public class TestRedirects extends LocalServerTestBase {
         Assert.assertEquals(HttpStatus.SC_OK, response.getCode());
         Assert.assertEquals(URIUtils.create(target, "/newlocation/"), reqWrapper.getUri());
 
-        final List<URI> redirects = context.getRedirectLocations();
+        final RedirectLocations redirects = context.getRedirectLocations();
         Assert.assertNotNull(redirects);
         Assert.assertEquals(1, redirects.size());
 

http://git-wip-us.apache.org/repos/asf/httpcomponents-client/blob/47a0eb8b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/DefaultRedirectStrategy.java
----------------------------------------------------------------------
diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/DefaultRedirectStrategy.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/DefaultRedirectStrategy.java
index 75c3cda..54c9048 100644
--- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/DefaultRedirectStrategy.java
+++ b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/DefaultRedirectStrategy.java
@@ -30,13 +30,7 @@ package org.apache.hc.client5.http.impl;
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.util.Locale;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.ConcurrentMap;
 
-import org.apache.hc.client5.http.CircularRedirectException;
-import org.apache.hc.client5.http.config.RequestConfig;
-import org.apache.hc.client5.http.protocol.HttpClientContext;
-import org.apache.hc.client5.http.protocol.RedirectLocations;
 import org.apache.hc.client5.http.protocol.RedirectStrategy;
 import org.apache.hc.client5.http.utils.URIUtils;
 import org.apache.hc.core5.annotation.Contract;
@@ -52,8 +46,6 @@ import org.apache.hc.core5.http.protocol.HttpContext;
 import org.apache.hc.core5.net.URIBuilder;
 import org.apache.hc.core5.util.Args;
 import org.apache.hc.core5.util.TextUtils;
-import org.apache.logging.log4j.LogManager;
-import org.apache.logging.log4j.Logger;
 
 /**
  * Default implementation of {@link RedirectStrategy}. This strategy honors the restrictions
@@ -64,27 +56,11 @@ import org.apache.logging.log4j.Logger;
  *
  * @since 4.1
  */
-@Contract(threading = ThreadingBehavior.IMMUTABLE)
+@Contract(threading = ThreadingBehavior.STATELESS)
 public class DefaultRedirectStrategy implements RedirectStrategy {
 
-    private final Logger log = LogManager.getLogger(getClass());
-
     public static final DefaultRedirectStrategy INSTANCE = new DefaultRedirectStrategy();
 
-    private final ConcurrentMap<String, Boolean> safeMethods;
-
-    public DefaultRedirectStrategy(final String... safeMethods) {
-        super();
-        this.safeMethods = new ConcurrentHashMap<>();
-        for (final String safeMethod: safeMethods) {
-            this.safeMethods.put(safeMethod.toUpperCase(Locale.ROOT), Boolean.TRUE);
-        }
-    }
-
-    public DefaultRedirectStrategy() {
-        this("GET", "HEAD", "OPTIONS", "TRACE");
-    }
-
     @Override
     public boolean isRedirected(
             final HttpRequest request,
@@ -117,20 +93,12 @@ public class DefaultRedirectStrategy implements RedirectStrategy {
         Args.notNull(response, "HTTP response");
         Args.notNull(context, "HTTP context");
 
-        final HttpClientContext clientContext = HttpClientContext.adapt(context);
-
         //get the location header to find out where to redirect to
         final Header locationHeader = response.getFirstHeader("location");
         if (locationHeader == null) {
             throw new HttpException("Redirect location is missing");
         }
         final String location = locationHeader.getValue();
-        if (this.log.isDebugEnabled()) {
-            this.log.debug("Redirect requested to location '" + location + "'");
-        }
-
-        final RequestConfig config = clientContext.getRequestConfig();
-
         URI uri = createLocationURI(location);
         try {
             if (!uri.isAbsolute()) {
@@ -141,18 +109,6 @@ public class DefaultRedirectStrategy implements RedirectStrategy {
             throw new ProtocolException(ex.getMessage(), ex);
         }
 
-        RedirectLocations redirectLocations = (RedirectLocations) clientContext.getAttribute(
-                HttpClientContext.REDIRECT_LOCATIONS);
-        if (redirectLocations == null) {
-            redirectLocations = new RedirectLocations();
-            context.setAttribute(HttpClientContext.REDIRECT_LOCATIONS, redirectLocations);
-        }
-        if (!config.isCircularRedirectsAllowed()) {
-            if (redirectLocations.contains(uri)) {
-                throw new CircularRedirectException("Circular redirect to '" + uri + "'");
-            }
-        }
-        redirectLocations.add(uri);
         return uri;
     }
 

http://git-wip-us.apache.org/repos/asf/httpcomponents-client/blob/47a0eb8b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncRedirectExec.java
----------------------------------------------------------------------
diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncRedirectExec.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncRedirectExec.java
index 4f34bf6..cdd86b5 100644
--- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncRedirectExec.java
+++ b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncRedirectExec.java
@@ -28,8 +28,8 @@ package org.apache.hc.client5.http.impl.async;
 
 import java.io.IOException;
 import java.net.URI;
-import java.util.List;
 
+import org.apache.hc.client5.http.CircularRedirectException;
 import org.apache.hc.client5.http.HttpRoute;
 import org.apache.hc.client5.http.RedirectException;
 import org.apache.hc.client5.http.StandardMethods;
@@ -40,6 +40,7 @@ import org.apache.hc.client5.http.auth.AuthExchange;
 import org.apache.hc.client5.http.auth.AuthScheme;
 import org.apache.hc.client5.http.config.RequestConfig;
 import org.apache.hc.client5.http.protocol.HttpClientContext;
+import org.apache.hc.client5.http.protocol.RedirectLocations;
 import org.apache.hc.client5.http.protocol.RedirectStrategy;
 import org.apache.hc.client5.http.routing.HttpRoutePlanner;
 import org.apache.hc.client5.http.utils.URIUtils;
@@ -53,6 +54,7 @@ import org.apache.hc.core5.http.ProtocolException;
 import org.apache.hc.core5.http.message.BasicHttpRequest;
 import org.apache.hc.core5.http.nio.AsyncDataConsumer;
 import org.apache.hc.core5.http.nio.AsyncEntityProducer;
+import org.apache.hc.core5.util.LangUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 
@@ -75,6 +77,7 @@ class AsyncRedirectExec implements AsyncExecChainHandler {
         volatile int redirectCount;
         volatile HttpRequest currentRequest;
         volatile AsyncEntityProducer currentEntityProducer;
+        volatile RedirectLocations redirectLocations;
         volatile AsyncExecChain.Scope currentScope;
         volatile boolean reroute;
 
@@ -107,6 +110,16 @@ class AsyncRedirectExec implements AsyncExecChainHandler {
                     state.redirectCount++;
 
                     final URI redirectUri = redirectStrategy.getLocationURI(request, response, clientContext);
+                    if (log.isDebugEnabled()) {
+                        log.debug("Redirect requested to location '" + redirectUri + "'");
+                    }
+                    if (!config.isCircularRedirectsAllowed()) {
+                        if (state.redirectLocations.contains(redirectUri)) {
+                            throw new CircularRedirectException("Circular redirect to '" + redirectUri + "'");
+                        }
+                    }
+                    state.redirectLocations.add(redirectUri);
+
                     final int statusCode = response.getCode();
 
                     switch (statusCode) {
@@ -128,27 +141,27 @@ class AsyncRedirectExec implements AsyncExecChainHandler {
                         throw new ProtocolException("Redirect URI does not specify a valid host name: " + redirectUri);
                     }
 
-                    if (!currentRoute.getTargetHost().equals(newTarget)) {
-                        log.debug("New route required");
-                        state.reroute = true;
-                        final AuthExchange targetAuthExchange = clientContext.getAuthExchange(currentRoute.getTargetHost());
-                        log.debug("Resetting target auth state");
-                        targetAuthExchange.reset();
-                        if (currentRoute.getProxyHost() != null) {
-                            final AuthExchange proxyAuthExchange = clientContext.getAuthExchange(currentRoute.getProxyHost());
-                            final AuthScheme authScheme = proxyAuthExchange.getAuthScheme();
-                            if (authScheme != null && authScheme.isConnectionBased()) {
-                                log.debug("Resetting proxy auth state");
-                                proxyAuthExchange.reset();
+                    state.reroute = false;
+                    state.redirectURI = redirectUri;
+
+                    if (!LangUtils.equals(currentRoute.getTargetHost(), newTarget)) {
+                        final HttpRoute newRoute = routePlanner.determineRoute(newTarget, clientContext);
+                        if (!LangUtils.equals(currentRoute, newRoute)) {
+                            state.reroute = true;
+                            final AuthExchange targetAuthExchange = clientContext.getAuthExchange(currentRoute.getTargetHost());
+                            log.debug("Resetting target auth state");
+                            targetAuthExchange.reset();
+                            if (currentRoute.getProxyHost() != null) {
+                                final AuthExchange proxyAuthExchange = clientContext.getAuthExchange(currentRoute.getProxyHost());
+                                final AuthScheme authScheme = proxyAuthExchange.getAuthScheme();
+                                if (authScheme != null && authScheme.isConnectionBased()) {
+                                    log.debug("Resetting proxy auth state");
+                                    proxyAuthExchange.reset();
+                                }
                             }
+                            state.currentScope = new AsyncExecChain.Scope(scope.exchangeId, newRoute,
+                                    scope.originalRequest, scope.future, clientContext, scope.execRuntime);
                         }
-                        final HttpRoute newRoute = routePlanner.determineRoute(newTarget, clientContext);
-                        state.currentScope = new AsyncExecChain.Scope(scope.exchangeId, newRoute,
-                                scope.originalRequest, scope.future, clientContext, scope.execRuntime);
-                        state.redirectURI = redirectUri;
-                    } else {
-                        state.reroute = false;
-                        state.redirectURI = redirectUri;
                     }
                 }
                 if (state.redirectURI != null) {
@@ -200,10 +213,13 @@ class AsyncRedirectExec implements AsyncExecChainHandler {
             final AsyncExecChain chain,
             final AsyncExecCallback asyncExecCallback) throws HttpException, IOException {
         final HttpClientContext clientContext = scope.clientContext;
-        final List<URI> redirectLocations = clientContext.getRedirectLocations();
-        if (redirectLocations != null) {
-            redirectLocations.clear();
+        RedirectLocations redirectLocations = clientContext.getRedirectLocations();
+        if (redirectLocations == null) {
+            redirectLocations = new RedirectLocations();
+            clientContext.setAttribute(HttpClientContext.REDIRECT_LOCATIONS, redirectLocations);
         }
+        redirectLocations.clear();
+
         final RequestConfig config = clientContext.getRequestConfig();
 
         final State state = new State();
@@ -211,6 +227,7 @@ class AsyncRedirectExec implements AsyncExecChainHandler {
         state.redirectCount = 0;
         state.currentRequest = request;
         state.currentEntityProducer = entityProducer;
+        state.redirectLocations = redirectLocations;
         state.currentScope = scope;
 
         internalExecute(state, chain, asyncExecCallback);

http://git-wip-us.apache.org/repos/asf/httpcomponents-client/blob/47a0eb8b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/RedirectExec.java
----------------------------------------------------------------------
diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/RedirectExec.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/RedirectExec.java
index 99b02cb..80e5cbf 100644
--- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/RedirectExec.java
+++ b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/RedirectExec.java
@@ -29,8 +29,8 @@ package org.apache.hc.client5.http.impl.classic;
 
 import java.io.IOException;
 import java.net.URI;
-import java.util.List;
 
+import org.apache.hc.client5.http.CircularRedirectException;
 import org.apache.hc.client5.http.HttpRoute;
 import org.apache.hc.client5.http.RedirectException;
 import org.apache.hc.client5.http.StandardMethods;
@@ -42,6 +42,7 @@ import org.apache.hc.client5.http.classic.methods.HttpGet;
 import org.apache.hc.client5.http.classic.methods.RequestBuilder;
 import org.apache.hc.client5.http.config.RequestConfig;
 import org.apache.hc.client5.http.protocol.HttpClientContext;
+import org.apache.hc.client5.http.protocol.RedirectLocations;
 import org.apache.hc.client5.http.protocol.RedirectStrategy;
 import org.apache.hc.client5.http.routing.HttpRoutePlanner;
 import org.apache.hc.client5.http.utils.URIUtils;
@@ -56,6 +57,7 @@ import org.apache.hc.core5.http.HttpStatus;
 import org.apache.hc.core5.http.ProtocolException;
 import org.apache.hc.core5.http.io.entity.EntityUtils;
 import org.apache.hc.core5.util.Args;
+import org.apache.hc.core5.util.LangUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 
@@ -97,11 +99,12 @@ final class RedirectExec implements ExecChainHandler {
         Args.notNull(scope, "Scope");
 
         final HttpClientContext context = scope.clientContext;
-
-        final List<URI> redirectLocations = context.getRedirectLocations();
-        if (redirectLocations != null) {
-            redirectLocations.clear();
+        RedirectLocations redirectLocations = context.getRedirectLocations();
+        if (redirectLocations == null) {
+            redirectLocations = new RedirectLocations();
+            context.setAttribute(HttpClientContext.REDIRECT_LOCATIONS, redirectLocations);
         }
+        redirectLocations.clear();
 
         final RequestConfig config = context.getRequestConfig();
         final int maxRedirects = config.getMaxRedirects() > 0 ? config.getMaxRedirects() : 50;
@@ -122,6 +125,16 @@ final class RedirectExec implements ExecChainHandler {
                     redirectCount++;
 
                     final URI redirectUri = this.redirectStrategy.getLocationURI(currentRequest, response, context);
+                    if (this.log.isDebugEnabled()) {
+                        this.log.debug("Redirect requested to location '" + redirectUri + "'");
+                    }
+                    if (!config.isCircularRedirectsAllowed()) {
+                        if (redirectLocations.contains(redirectUri)) {
+                            throw new CircularRedirectException("Circular redirect to '" + redirectUri + "'");
+                        }
+                    }
+                    redirectLocations.add(redirectUri);
+
                     final ClassicHttpRequest originalRequest = scope.originalRequest;
                     ClassicHttpRequest redirect = null;
                     final int statusCode = response.getCode();
@@ -147,28 +160,29 @@ final class RedirectExec implements ExecChainHandler {
                                 redirectUri);
                     }
 
-                    HttpRoute currentRoute = currentScope.route;
-                    final boolean crossSiteRedirect = !currentRoute.getTargetHost().equals(newTarget);
-                    if (crossSiteRedirect) {
-
-                        final AuthExchange targetAuthExchange = context.getAuthExchange(currentRoute.getTargetHost());
-                        this.log.debug("Resetting target auth state");
-                        targetAuthExchange.reset();
-                        if (currentRoute.getProxyHost() != null) {
-                            final AuthExchange proxyAuthExchange = context.getAuthExchange(currentRoute.getProxyHost());
-                            final AuthScheme authScheme = proxyAuthExchange.getAuthScheme();
-                            if (authScheme != null && authScheme.isConnectionBased()) {
-                                this.log.debug("Resetting proxy auth state");
-                                proxyAuthExchange.reset();
+                    final HttpRoute currentRoute = currentScope.route;
+                    if (!LangUtils.equals(currentRoute.getTargetHost(), newTarget)) {
+                        final HttpRoute newRoute = this.routePlanner.determineRoute(newTarget, context);
+                        if (!LangUtils.equals(currentRoute, newRoute)) {
+                            log.debug("New route required");
+                            final AuthExchange targetAuthExchange = context.getAuthExchange(currentRoute.getTargetHost());
+                            this.log.debug("Resetting target auth state");
+                            targetAuthExchange.reset();
+                            if (currentRoute.getProxyHost() != null) {
+                                final AuthExchange proxyAuthExchange = context.getAuthExchange(currentRoute.getProxyHost());
+                                final AuthScheme authScheme = proxyAuthExchange.getAuthScheme();
+                                if (authScheme != null && authScheme.isConnectionBased()) {
+                                    this.log.debug("Resetting proxy auth state");
+                                    proxyAuthExchange.reset();
+                                }
                             }
+                            currentScope = new ExecChain.Scope(
+                                    currentScope.exchangeId,
+                                    newRoute,
+                                    currentScope.originalRequest,
+                                    currentScope.execRuntime,
+                                    currentScope.clientContext);
                         }
-                        currentRoute = this.routePlanner.determineRoute(newTarget, context);
-                        currentScope = new ExecChain.Scope(
-                                currentScope.exchangeId,
-                                currentRoute,
-                                currentScope.originalRequest,
-                                currentScope.execRuntime,
-                                currentScope.clientContext);
                     }
 
                     if (this.log.isDebugEnabled()) {

http://git-wip-us.apache.org/repos/asf/httpcomponents-client/blob/47a0eb8b/httpclient5/src/main/java/org/apache/hc/client5/http/protocol/HttpClientContext.java
----------------------------------------------------------------------
diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/protocol/HttpClientContext.java b/httpclient5/src/main/java/org/apache/hc/client5/http/protocol/HttpClientContext.java
index e717a90..7be4c16 100644
--- a/httpclient5/src/main/java/org/apache/hc/client5/http/protocol/HttpClientContext.java
+++ b/httpclient5/src/main/java/org/apache/hc/client5/http/protocol/HttpClientContext.java
@@ -27,9 +27,7 @@
 
 package org.apache.hc.client5.http.protocol;
 
-import java.net.URI;
 import java.util.HashMap;
-import java.util.List;
 import java.util.Map;
 
 import org.apache.hc.client5.http.HttpRoute;
@@ -67,7 +65,7 @@ public class HttpClientContext extends HttpCoreContext {
     public static final String HTTP_ROUTE   = "http.route";
 
     /**
-     * Attribute name of a {@link List} object that represents a collection of all
+     * Attribute name of a {@link RedirectLocations} object that represents a collection of all
      * redirect locations received in the process of request execution.
      */
     public static final String REDIRECT_LOCATIONS = "http.protocol.redirect-locations";
@@ -156,9 +154,8 @@ public class HttpClientContext extends HttpCoreContext {
         return getAttribute(HTTP_ROUTE, HttpRoute.class);
     }
 
-    @SuppressWarnings("unchecked")
-    public List<URI> getRedirectLocations() {
-        return getAttribute(REDIRECT_LOCATIONS, List.class);
+    public RedirectLocations getRedirectLocations() {
+        return getAttribute(REDIRECT_LOCATIONS, RedirectLocations.class);
     }
 
     public CookieStore getCookieStore() {

http://git-wip-us.apache.org/repos/asf/httpcomponents-client/blob/47a0eb8b/httpclient5/src/main/java/org/apache/hc/client5/http/protocol/RedirectLocations.java
----------------------------------------------------------------------
diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/protocol/RedirectLocations.java b/httpclient5/src/main/java/org/apache/hc/client5/http/protocol/RedirectLocations.java
index effcbee..22fb6ff 100644
--- a/httpclient5/src/main/java/org/apache/hc/client5/http/protocol/RedirectLocations.java
+++ b/httpclient5/src/main/java/org/apache/hc/client5/http/protocol/RedirectLocations.java
@@ -28,10 +28,8 @@
 package org.apache.hc.client5.http.protocol;
 
 import java.net.URI;
-import java.util.AbstractList;
 import java.util.ArrayList;
 import java.util.HashSet;
-import java.util.Iterator;
 import java.util.List;
 import java.util.Set;
 
@@ -41,7 +39,7 @@ import java.util.Set;
  *
  * @since 4.0
  */
-public class RedirectLocations extends AbstractList<Object> {
+public final class RedirectLocations {
 
     private final Set<URI> unique;
     private final List<URI> all;
@@ -68,23 +66,6 @@ public class RedirectLocations extends AbstractList<Object> {
     }
 
     /**
-     * Removes a URI from the collection.
-     */
-    public boolean remove(final URI uri) {
-        final boolean removed = this.unique.remove(uri);
-        if (removed) {
-            final Iterator<URI> it = this.all.iterator();
-            while (it.hasNext()) {
-                final URI current = it.next();
-                if (current.equals(uri)) {
-                    it.remove();
-                }
-            }
-        }
-        return removed;
-    }
-
-    /**
      * Returns all redirect {@link URI}s in the order they were added to the collection.
      *
      * @return list of all URIs
@@ -106,7 +87,6 @@ public class RedirectLocations extends AbstractList<Object> {
      *             {@code index &lt; 0 || index &gt;= size()})
      * @since 4.3
      */
-    @Override
     public URI get(final int index) {
         return this.all.get(index);
     }
@@ -119,106 +99,13 @@ public class RedirectLocations extends AbstractList<Object> {
      * @return the number of elements in this list
      * @since 4.3
      */
-    @Override
     public int size() {
         return this.all.size();
     }
 
-    /**
-     * Replaces the URI at the specified position in this list with the
-     * specified element (must be a URI).
-     *
-     * @param index
-     *            index of the element to replace
-     * @param element
-     *            URI to be stored at the specified position
-     * @return the URI previously at the specified position
-     * @throws UnsupportedOperationException
-     *             if the {@code set} operation is not supported by this list
-     * @throws ClassCastException
-     *             if the element is not a {@link URI}
-     * @throws NullPointerException
-     *             if the specified element is null and this list does not
-     *             permit null elements
-     * @throws IndexOutOfBoundsException
-     *             if the index is out of range (
-     *             {@code index &lt; 0 || index &gt;= size()})
-     * @since 4.3
-     */
-    @Override
-    public Object set(final int index, final Object element) {
-        final URI removed = this.all.set(index, (URI) element);
-        this.unique.remove(removed);
-        this.unique.add((URI) element);
-        if (this.all.size() != this.unique.size()) {
-            this.unique.addAll(this.all);
-        }
-        return removed;
-    }
-
-    /**
-     * Inserts the specified element at the specified position in this list
-     * (must be a URI). Shifts the URI currently at that position (if any) and
-     * any subsequent URIs to the right (adds one to their indices).
-     *
-     * @param index
-     *            index at which the specified element is to be inserted
-     * @param element
-     *            URI to be inserted
-     * @throws UnsupportedOperationException
-     *             if the {@code add} operation is not supported by this list
-     * @throws ClassCastException
-     *             if the element is not a {@link URI}
-     * @throws NullPointerException
-     *             if the specified element is null and this list does not
-     *             permit null elements
-     * @throws IndexOutOfBoundsException
-     *             if the index is out of range (
-     *             {@code index &lt; 0 || index &gt; size()})
-     * @since 4.3
-     */
-    @Override
-    public void add(final int index, final Object element) {
-        this.all.add(index, (URI) element);
-        this.unique.add((URI) element);
-    }
-
-    /**
-     * Removes the URI at the specified position in this list. Shifts any
-     * subsequent URIs to the left (subtracts one from their indices). Returns
-     * the URI that was removed from the list.
-     *
-     * @param index
-     *            the index of the URI to be removed
-     * @return the URI previously at the specified position
-     * @throws IndexOutOfBoundsException
-     *             if the index is out of range (
-     *             {@code index &lt; 0 || index &gt;= size()})
-     * @since 4.3
-     */
-    @Override
-    public URI remove(final int index) {
-        final URI removed = this.all.remove(index);
-        this.unique.remove(removed);
-        if (this.all.size() != this.unique.size()) {
-            this.unique.addAll(this.all);
-        }
-        return removed;
-    }
-
-    /**
-     * Returns {@code true} if this collection contains the specified element.
-     * More formally, returns {@code true} if and only if this collection
-     * contains at least one element {@code e} such that
-     * {@code (o==null&nbsp;?&nbsp;e==null&nbsp;:&nbsp;o.equals(e))}.
-     *
-     * @param o element whose presence in this collection is to be tested
-     * @return {@code true} if this collection contains the specified
-     *         element
-     */
-    @Override
-    public boolean contains(final Object o) {
-        return this.unique.contains(o);
+    public void clear() {
+        unique.clear();
+        all.clear();
     }
 
 }

http://git-wip-us.apache.org/repos/asf/httpcomponents-client/blob/47a0eb8b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/TestDefaultRedirectStrategy.java
----------------------------------------------------------------------
diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/TestDefaultRedirectStrategy.java b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/TestDefaultRedirectStrategy.java
index 1fc2eb2..6ca7e60 100644
--- a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/TestDefaultRedirectStrategy.java
+++ b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/TestDefaultRedirectStrategy.java
@@ -27,11 +27,9 @@
 package org.apache.hc.client5.http.impl;
 
 import java.net.URI;
-import java.util.List;
 
 import org.apache.hc.client5.http.classic.methods.HttpGet;
 import org.apache.hc.client5.http.classic.methods.HttpPost;
-import org.apache.hc.client5.http.config.RequestConfig;
 import org.apache.hc.client5.http.protocol.HttpClientContext;
 import org.apache.hc.core5.http.HttpException;
 import org.apache.hc.core5.http.HttpHeaders;
@@ -201,53 +199,6 @@ public class TestDefaultRedirectStrategy {
     }
 
     @Test
-    public void testGetLocationUriAllowCircularRedirects() throws Exception {
-        final DefaultRedirectStrategy redirectStrategy = new DefaultRedirectStrategy();
-        final HttpClientContext context = HttpClientContext.create();
-        final RequestConfig config = RequestConfig.custom().setCircularRedirectsAllowed(true).build();
-        context.setRequestConfig(config);
-        final URI uri1 = URI.create("http://localhost/stuff1");
-        final URI uri2 = URI.create("http://localhost/stuff2");
-        final URI uri3 = URI.create("http://localhost/stuff3");
-        final HttpGet httpget1 = new HttpGet("http://localhost/");
-        final HttpResponse response1 = new BasicHttpResponse(HttpStatus.SC_MOVED_TEMPORARILY, "Redirect");
-        response1.addHeader("Location", uri1.toASCIIString());
-        final HttpGet httpget2 = new HttpGet(uri1.toASCIIString());
-        final HttpResponse response2 = new BasicHttpResponse(HttpStatus.SC_MOVED_TEMPORARILY, "Redirect");
-        response2.addHeader("Location", uri2.toASCIIString());
-        final HttpGet httpget3 = new HttpGet(uri2.toASCIIString());
-        final HttpResponse response3 = new BasicHttpResponse(HttpStatus.SC_MOVED_TEMPORARILY, "Redirect");
-        response3.addHeader("Location", uri3.toASCIIString());
-        Assert.assertEquals(uri1, redirectStrategy.getLocationURI(httpget1, response1, context));
-        Assert.assertEquals(uri2, redirectStrategy.getLocationURI(httpget2, response2, context));
-        Assert.assertEquals(uri3, redirectStrategy.getLocationURI(httpget3, response3, context));
-
-        final List<URI> uris = context.getRedirectLocations();
-        Assert.assertNotNull(uris);
-        Assert.assertTrue(uris.contains(uri1));
-        Assert.assertTrue(uris.contains(uri2));
-        Assert.assertTrue(uris.contains(uri3));
-        Assert.assertEquals(3, uris.size());
-        Assert.assertEquals(uri1, uris.get(0));
-        Assert.assertEquals(uri2, uris.get(1));
-        Assert.assertEquals(uri3, uris.get(2));
-    }
-
-    @Test(expected=ProtocolException.class)
-    public void testGetLocationUriDisallowCircularRedirects() throws Exception {
-        final DefaultRedirectStrategy redirectStrategy = new DefaultRedirectStrategy();
-        final HttpClientContext context = HttpClientContext.create();
-        final HttpGet httpget = new HttpGet("http://localhost/stuff");
-        final RequestConfig config = RequestConfig.custom().setCircularRedirectsAllowed(false).build();
-        context.setRequestConfig(config);
-        final HttpResponse response = new BasicHttpResponse(HttpStatus.SC_MOVED_TEMPORARILY, "Redirect");
-        response.addHeader("Location", "http://localhost/stuff");
-        final URI uri = URI.create("http://localhost/stuff");
-        Assert.assertEquals(uri, redirectStrategy.getLocationURI(httpget, response, context));
-        redirectStrategy.getLocationURI(httpget, response, context);
-    }
-
-    @Test
     public void testGetLocationUriInvalidInput() throws Exception {
         final DefaultRedirectStrategy redirectStrategy = new DefaultRedirectStrategy();
         final HttpClientContext context = HttpClientContext.create();

http://git-wip-us.apache.org/repos/asf/httpcomponents-client/blob/47a0eb8b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestRedirectExec.java
----------------------------------------------------------------------
diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestRedirectExec.java b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestRedirectExec.java
index 7fdb4cc..80cca6a 100644
--- a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestRedirectExec.java
+++ b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestRedirectExec.java
@@ -30,8 +30,10 @@ import java.io.ByteArrayInputStream;
 import java.io.InputStream;
 import java.net.URI;
 import java.net.URISyntaxException;
+import java.util.Arrays;
 import java.util.List;
 
+import org.apache.hc.client5.http.CircularRedirectException;
 import org.apache.hc.client5.http.HttpRoute;
 import org.apache.hc.client5.http.RedirectException;
 import org.apache.hc.client5.http.auth.AuthExchange;
@@ -40,48 +42,52 @@ import org.apache.hc.client5.http.classic.ExecRuntime;
 import org.apache.hc.client5.http.classic.methods.HttpGet;
 import org.apache.hc.client5.http.config.RequestConfig;
 import org.apache.hc.client5.http.entity.EntityBuilder;
+import org.apache.hc.client5.http.impl.DefaultRedirectStrategy;
 import org.apache.hc.client5.http.impl.auth.BasicScheme;
 import org.apache.hc.client5.http.impl.auth.NTLMScheme;
 import org.apache.hc.client5.http.protocol.HttpClientContext;
+import org.apache.hc.client5.http.protocol.RedirectLocations;
 import org.apache.hc.client5.http.protocol.RedirectStrategy;
 import org.apache.hc.client5.http.routing.HttpRoutePlanner;
 import org.apache.hc.core5.http.ClassicHttpRequest;
 import org.apache.hc.core5.http.ClassicHttpResponse;
 import org.apache.hc.core5.http.HttpEntity;
 import org.apache.hc.core5.http.HttpException;
+import org.apache.hc.core5.http.HttpHeaders;
 import org.apache.hc.core5.http.HttpHost;
-import org.apache.hc.core5.http.HttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
 import org.apache.hc.core5.http.ProtocolException;
+import org.apache.hc.core5.http.message.BasicClassicHttpResponse;
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
+import org.junit.runner.RunWith;
 import org.mockito.ArgumentCaptor;
 import org.mockito.ArgumentMatcher;
 import org.mockito.ArgumentMatchers;
 import org.mockito.Mock;
 import org.mockito.Mockito;
-import org.mockito.MockitoAnnotations;
+import org.mockito.junit.MockitoJUnitRunner;
 
-@SuppressWarnings({"boxing","static-access"}) // test code
+@RunWith(MockitoJUnitRunner.class)
 public class TestRedirectExec {
 
     @Mock
     private HttpRoutePlanner httpRoutePlanner;
     @Mock
-    private RedirectStrategy redirectStrategy;
-    @Mock
     private ExecChain chain;
     @Mock
     private ExecRuntime endpoint;
 
+    private RedirectStrategy redirectStrategy;
     private RedirectExec redirectExec;
     private HttpHost target;
 
     @Before
     public void setup() throws Exception {
-        MockitoAnnotations.initMocks(this);
-        redirectExec = new RedirectExec(httpRoutePlanner, redirectStrategy);
         target = new HttpHost("localhost", 80);
+        redirectStrategy = Mockito.spy(new DefaultRedirectStrategy());
+        redirectExec = new RedirectExec(httpRoutePlanner, redirectStrategy);
     }
 
     @Test
@@ -90,19 +96,20 @@ public class TestRedirectExec {
         final HttpGet request = new HttpGet("/test");
         final HttpClientContext context = HttpClientContext.create();
 
-        final ClassicHttpResponse response1 = Mockito.mock(ClassicHttpResponse.class);
+        final ClassicHttpResponse response1 = Mockito.spy(new BasicClassicHttpResponse(HttpStatus.SC_MOVED_TEMPORARILY));
+        final URI redirect = new URI("http://localhost:80/redirect");
+        response1.setHeader(HttpHeaders.LOCATION, redirect.toASCIIString());
         final InputStream instream1 = Mockito.spy(new ByteArrayInputStream(new byte[] {1, 2, 3}));
         final HttpEntity entity1 = EntityBuilder.create()
                 .setStream(instream1)
                 .build();
-        Mockito.when(response1.getEntity()).thenReturn(entity1);
-        final ClassicHttpResponse response2 = Mockito.mock(ClassicHttpResponse.class);
+        response1.setEntity(entity1);
+        final ClassicHttpResponse response2 = Mockito.spy(new BasicClassicHttpResponse(HttpStatus.SC_OK));
         final InputStream instream2 = Mockito.spy(new ByteArrayInputStream(new byte[] {1, 2, 3}));
         final HttpEntity entity2 = EntityBuilder.create()
                 .setStream(instream2)
                 .build();
-        Mockito.when(response2.getEntity()).thenReturn(entity2);
-        final URI redirect = new URI("http://localhost:80/redirect");
+        response2.setEntity(entity2);
 
         Mockito.when(chain.proceed(
                 Mockito.same(request),
@@ -110,26 +117,12 @@ public class TestRedirectExec {
         Mockito.when(chain.proceed(
                 HttpRequestMatcher.matchesRequestUri(redirect),
                 Mockito.<ExecChain.Scope>any())).thenReturn(response2);
-        Mockito.when(redirectStrategy.isRedirected(
-                Mockito.same(request),
-                Mockito.same(response1),
-                Mockito.<HttpClientContext>any())).thenReturn(Boolean.TRUE);
-        Mockito.when(redirectStrategy.getLocationURI(
-                Mockito.same(request),
-                Mockito.same(response1),
-                Mockito.<HttpClientContext>any())).thenReturn(redirect);
-        Mockito.when(httpRoutePlanner.determineRoute(
-                Mockito.eq(target),
-                Mockito.<HttpClientContext>any())).thenReturn(route);
 
         final ExecChain.Scope scope = new ExecChain.Scope("test", route, request, endpoint, context);
         redirectExec.execute(request, scope, chain);
 
-        final ArgumentCaptor<ClassicHttpRequest> reqCaptor = ArgumentCaptor.forClass(
-                ClassicHttpRequest.class);
-        Mockito.verify(chain, Mockito.times(2)).proceed(
-                reqCaptor.capture(),
-                Mockito.same(scope));
+        final ArgumentCaptor<ClassicHttpRequest> reqCaptor = ArgumentCaptor.forClass(ClassicHttpRequest.class);
+        Mockito.verify(chain, Mockito.times(2)).proceed(reqCaptor.capture(), Mockito.same(scope));
 
         final List<ClassicHttpRequest> allValues = reqCaptor.getAllValues();
         Assert.assertNotNull(allValues);
@@ -137,7 +130,7 @@ public class TestRedirectExec {
         Assert.assertSame(request, allValues.get(0));
 
         Mockito.verify(response1, Mockito.times(1)).close();
-        Mockito.verify(instream1, Mockito.times(1)).close();
+        Mockito.verify(instream1, Mockito.times(2)).close();
         Mockito.verify(response2, Mockito.never()).close();
         Mockito.verify(instream2, Mockito.never()).close();
     }
@@ -153,23 +146,11 @@ public class TestRedirectExec {
                 .build();
         context.setRequestConfig(config);
 
-        final ClassicHttpResponse response1 = Mockito.mock(ClassicHttpResponse.class);
+        final ClassicHttpResponse response1 = Mockito.spy(new BasicClassicHttpResponse(HttpStatus.SC_MOVED_TEMPORARILY));
         final URI redirect = new URI("http://localhost:80/redirect");
+        response1.setHeader(HttpHeaders.LOCATION, redirect.toASCIIString());
 
-        Mockito.when(chain.proceed(
-                Mockito.<ClassicHttpRequest>any(),
-                Mockito.<ExecChain.Scope>any())).thenReturn(response1);
-        Mockito.when(redirectStrategy.isRedirected(
-                Mockito.<ClassicHttpRequest>any(),
-                Mockito.<HttpResponse>any(),
-                Mockito.<HttpClientContext>any())).thenReturn(Boolean.TRUE);
-        Mockito.when(redirectStrategy.getLocationURI(
-                Mockito.<ClassicHttpRequest>any(),
-                Mockito.<HttpResponse>any(),
-                Mockito.<HttpClientContext>any())).thenReturn(redirect);
-        Mockito.when(httpRoutePlanner.determineRoute(
-                Mockito.eq(target),
-                Mockito.<HttpClientContext>any())).thenReturn(route);
+        Mockito.when(chain.proceed(Mockito.<ClassicHttpRequest>any(), Mockito.<ExecChain.Scope>any())).thenReturn(response1);
 
         final ExecChain.Scope scope = new ExecChain.Scope("test", route, request, endpoint, context);
         redirectExec.execute(request, scope, chain);
@@ -181,26 +162,12 @@ public class TestRedirectExec {
         final HttpGet request = new HttpGet("/test");
         final HttpClientContext context = HttpClientContext.create();
 
-        final ClassicHttpResponse response1 = Mockito.mock(ClassicHttpResponse.class);
-        final ClassicHttpResponse response2 = Mockito.mock(ClassicHttpResponse.class);
+        final ClassicHttpResponse response1 = Mockito.spy(new BasicClassicHttpResponse(HttpStatus.SC_MOVED_TEMPORARILY));
         final URI redirect = new URI("/redirect");
+        response1.setHeader(HttpHeaders.LOCATION, redirect.toASCIIString());
         Mockito.when(chain.proceed(
                 Mockito.same(request),
                 Mockito.<ExecChain.Scope>any())).thenReturn(response1);
-        Mockito.when(chain.proceed(
-                HttpRequestMatcher.matchesRequestUri(redirect),
-                Mockito.<ExecChain.Scope>any())).thenReturn(response2);
-        Mockito.when(redirectStrategy.isRedirected(
-                Mockito.same(request),
-                Mockito.same(response1),
-                Mockito.<HttpClientContext>any())).thenReturn(Boolean.TRUE);
-        Mockito.when(redirectStrategy.getLocationURI(
-                Mockito.same(request),
-                Mockito.same(response1),
-                Mockito.<HttpClientContext>any())).thenReturn(redirect);
-        Mockito.when(httpRoutePlanner.determineRoute(
-                Mockito.eq(target),
-                Mockito.<HttpClientContext>any())).thenReturn(route);
 
         final ExecChain.Scope scope = new ExecChain.Scope("test", route, request, endpoint, context);
         redirectExec.execute(request, scope, chain);
@@ -223,27 +190,17 @@ public class TestRedirectExec {
         context.setAuthExchange(target, targetAuthExchange);
         context.setAuthExchange(proxy, proxyAuthExchange);
 
-        final ClassicHttpResponse response1 = Mockito.mock(ClassicHttpResponse.class);
-        final ClassicHttpResponse response2 = Mockito.mock(ClassicHttpResponse.class);
-        final HttpHost otherHost = new HttpHost("otherhost", 8888);
+        final ClassicHttpResponse response1 = Mockito.spy(new BasicClassicHttpResponse(HttpStatus.SC_MOVED_TEMPORARILY));
         final URI redirect = new URI("http://otherhost:8888/redirect");
+        response1.setHeader(HttpHeaders.LOCATION, redirect.toASCIIString());
+        final ClassicHttpResponse response2 = Mockito.spy(new BasicClassicHttpResponse(HttpStatus.SC_OK));
+        final HttpHost otherHost = new HttpHost("otherhost", 8888);
         Mockito.when(chain.proceed(
                 Mockito.same(request),
                 Mockito.<ExecChain.Scope>any())).thenReturn(response1);
         Mockito.when(chain.proceed(
                 HttpRequestMatcher.matchesRequestUri(redirect),
                 Mockito.<ExecChain.Scope>any())).thenReturn(response2);
-        Mockito.when(redirectStrategy.isRedirected(
-                Mockito.same(request),
-                Mockito.same(response1),
-                Mockito.<HttpClientContext>any())).thenReturn(Boolean.TRUE);
-        Mockito.when(redirectStrategy.getLocationURI(
-                Mockito.same(request),
-                Mockito.same(response1),
-                Mockito.<HttpClientContext>any())).thenReturn(redirect);
-        Mockito.when(httpRoutePlanner.determineRoute(
-                Mockito.eq(target),
-                Mockito.<HttpClientContext>any())).thenReturn(new HttpRoute(target));
         Mockito.when(httpRoutePlanner.determineRoute(
                 Mockito.eq(otherHost),
                 Mockito.<HttpClientContext>any())).thenReturn(new HttpRoute(otherHost));
@@ -261,24 +218,101 @@ public class TestRedirectExec {
         Assert.assertEquals(null, authExchange2.getAuthScheme());
     }
 
+    @Test
+    public void testAllowCircularRedirects() throws Exception {
+        final HttpRoute route = new HttpRoute(target);
+        final HttpClientContext context = HttpClientContext.create();
+        context.setRequestConfig(RequestConfig.custom()
+                .setCircularRedirectsAllowed(true)
+                .build());
+
+        final URI uri = URI.create("http://localhost/");
+        final HttpGet request = new HttpGet(uri);
+
+        final URI uri1 = URI.create("http://localhost/stuff1");
+        final URI uri2 = URI.create("http://localhost/stuff2");
+        final ClassicHttpResponse response1 = new BasicClassicHttpResponse(HttpStatus.SC_MOVED_TEMPORARILY);
+        response1.addHeader("Location", uri1.toASCIIString());
+        final ClassicHttpResponse response2 = new BasicClassicHttpResponse(HttpStatus.SC_MOVED_TEMPORARILY);
+        response2.addHeader("Location", uri2.toASCIIString());
+        final ClassicHttpResponse response3 = new BasicClassicHttpResponse(HttpStatus.SC_MOVED_TEMPORARILY);
+        response3.addHeader("Location", uri1.toASCIIString());
+        final ClassicHttpResponse response4 = new BasicClassicHttpResponse(HttpStatus.SC_OK);
+
+        Mockito.when(chain.proceed(
+                HttpRequestMatcher.matchesRequestUri(uri),
+                Mockito.<ExecChain.Scope>any())).thenReturn(response1);
+        Mockito.when(chain.proceed(
+                HttpRequestMatcher.matchesRequestUri(uri1),
+                Mockito.<ExecChain.Scope>any())).thenReturn(response2, response4);
+        Mockito.when(chain.proceed(
+                HttpRequestMatcher.matchesRequestUri(uri2),
+                Mockito.<ExecChain.Scope>any())).thenReturn(response3);
+        Mockito.when(httpRoutePlanner.determineRoute(
+                Mockito.eq(new HttpHost("localhost")),
+                Mockito.<HttpClientContext>any())).thenReturn(route);
+
+        final ExecChain.Scope scope = new ExecChain.Scope("test", route, request, endpoint, context);
+        redirectExec.execute(request, scope, chain);
+
+        final RedirectLocations uris = context.getRedirectLocations();
+        Assert.assertNotNull(uris);
+        Assert.assertEquals(Arrays.asList(uri1, uri2, uri1), uris.getAll());
+    }
+
+    @Test(expected=CircularRedirectException.class)
+    public void testGetLocationUriDisallowCircularRedirects() throws Exception {
+        final HttpRoute route = new HttpRoute(target);
+        final HttpClientContext context = HttpClientContext.create();
+        context.setRequestConfig(RequestConfig.custom()
+                .setCircularRedirectsAllowed(false)
+                .build());
+
+        final URI uri = URI.create("http://localhost/");
+        final HttpGet request = new HttpGet(uri);
+
+        final URI uri1 = URI.create("http://localhost/stuff1");
+        final URI uri2 = URI.create("http://localhost/stuff2");
+        final ClassicHttpResponse response1 = new BasicClassicHttpResponse(HttpStatus.SC_MOVED_TEMPORARILY);
+        response1.addHeader("Location", uri1.toASCIIString());
+        final ClassicHttpResponse response2 = new BasicClassicHttpResponse(HttpStatus.SC_MOVED_TEMPORARILY);
+        response2.addHeader("Location", uri2.toASCIIString());
+        final ClassicHttpResponse response3 = new BasicClassicHttpResponse(HttpStatus.SC_MOVED_TEMPORARILY);
+        response3.addHeader("Location", uri1.toASCIIString());
+        Mockito.when(httpRoutePlanner.determineRoute(
+                Mockito.eq(new HttpHost("localhost")),
+                Mockito.<HttpClientContext>any())).thenReturn(route);
+
+        Mockito.when(chain.proceed(
+                HttpRequestMatcher.matchesRequestUri(uri),
+                Mockito.<ExecChain.Scope>any())).thenReturn(response1);
+        Mockito.when(chain.proceed(
+                HttpRequestMatcher.matchesRequestUri(uri1),
+                Mockito.<ExecChain.Scope>any())).thenReturn(response2);
+        Mockito.when(chain.proceed(
+                HttpRequestMatcher.matchesRequestUri(uri2),
+                Mockito.<ExecChain.Scope>any())).thenReturn(response3);
+
+        final ExecChain.Scope scope = new ExecChain.Scope("test", route, request, endpoint, context);
+        redirectExec.execute(request, scope, chain);
+    }
+
     @Test(expected = RuntimeException.class)
     public void testRedirectRuntimeException() throws Exception {
         final HttpRoute route = new HttpRoute(target);
         final HttpGet request = new HttpGet("/test");
         final HttpClientContext context = HttpClientContext.create();
 
-        final ClassicHttpResponse response1 = Mockito.mock(ClassicHttpResponse.class);
+        final ClassicHttpResponse response1 = Mockito.spy(new BasicClassicHttpResponse(HttpStatus.SC_MOVED_TEMPORARILY));
+        final URI redirect = new URI("http://localhost:80/redirect");
+        response1.setHeader(HttpHeaders.LOCATION, redirect.toASCIIString());
         Mockito.when(chain.proceed(
                 Mockito.same(request),
                 Mockito.<ExecChain.Scope>any())).thenReturn(response1);
-        Mockito.when(redirectStrategy.isRedirected(
-                Mockito.same(request),
-                Mockito.same(response1),
-                Mockito.<HttpClientContext>any())).thenReturn(Boolean.TRUE);
-        Mockito.doThrow(new RuntimeException("Oppsie")).when(redirectStrategy.getLocationURI(
-                Mockito.same(request),
-                Mockito.same(response1),
-                Mockito.<HttpClientContext>any()));
+        Mockito.doThrow(new RuntimeException("Oppsie")).when(redirectStrategy).getLocationURI(
+                Mockito.<ClassicHttpRequest>any(),
+                Mockito.<ClassicHttpResponse>any(),
+                Mockito.<HttpClientContext>any());
 
         final ExecChain.Scope scope = new ExecChain.Scope("test", route, request, endpoint, context);
         try {
@@ -295,29 +329,27 @@ public class TestRedirectExec {
         final HttpGet request = new HttpGet("/test");
         final HttpClientContext context = HttpClientContext.create();
 
-        final ClassicHttpResponse response1 = Mockito.mock(ClassicHttpResponse.class);
+        final ClassicHttpResponse response1 = Mockito.spy(new BasicClassicHttpResponse(HttpStatus.SC_MOVED_TEMPORARILY));
+        final URI redirect = new URI("http://localhost:80/redirect");
+        response1.setHeader(HttpHeaders.LOCATION, redirect.toASCIIString());
         final InputStream instream1 = Mockito.spy(new ByteArrayInputStream(new byte[] {1, 2, 3}));
         final HttpEntity entity1 = EntityBuilder.create()
                 .setStream(instream1)
                 .build();
-        Mockito.when(response1.getEntity()).thenReturn(entity1);
+        response1.setEntity(entity1);
         Mockito.when(chain.proceed(
                 Mockito.same(request),
                 Mockito.<ExecChain.Scope>any())).thenReturn(response1);
-        Mockito.when(redirectStrategy.isRedirected(
-                Mockito.same(request),
-                Mockito.same(response1),
-                Mockito.<HttpClientContext>any())).thenReturn(Boolean.TRUE);
         Mockito.doThrow(new ProtocolException("Oppsie")).when(redirectStrategy).getLocationURI(
-                Mockito.same(request),
-                Mockito.same(response1),
+                Mockito.<ClassicHttpRequest>any(),
+                Mockito.<ClassicHttpResponse>any(),
                 Mockito.<HttpClientContext>any());
 
         final ExecChain.Scope scope = new ExecChain.Scope("test", route, request, endpoint, context);
         try {
             redirectExec.execute(request, scope, chain);
         } catch (final Exception ex) {
-            Mockito.verify(instream1).close();
+            Mockito.verify(instream1, Mockito.times(2)).close();
             Mockito.verify(response1).close();
             throw ex;
         }
@@ -325,17 +357,21 @@ public class TestRedirectExec {
 
     private static class HttpRequestMatcher implements ArgumentMatcher<ClassicHttpRequest> {
 
-        private final URI requestUri;
+        private final URI expectedRequestUri;
 
         HttpRequestMatcher(final URI requestUri) {
             super();
-            this.requestUri = requestUri;
+            this.expectedRequestUri = requestUri;
         }
 
         @Override
         public boolean matches(final ClassicHttpRequest argument) {
+            if (argument == null) {
+                return false;
+            }
             try {
-                return requestUri.equals(argument.getUri());
+                final URI requestUri = argument.getUri();
+                return this.expectedRequestUri.equals(requestUri);
             } catch (final URISyntaxException ex) {
                 return false;
             }

http://git-wip-us.apache.org/repos/asf/httpcomponents-client/blob/47a0eb8b/httpclient5/src/test/java/org/apache/hc/client5/http/protocol/TestRedirectLocation.java
----------------------------------------------------------------------
diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/protocol/TestRedirectLocation.java b/httpclient5/src/test/java/org/apache/hc/client5/http/protocol/TestRedirectLocation.java
index 95f4693..3923afc 100644
--- a/httpclient5/src/test/java/org/apache/hc/client5/http/protocol/TestRedirectLocation.java
+++ b/httpclient5/src/test/java/org/apache/hc/client5/http/protocol/TestRedirectLocation.java
@@ -56,7 +56,7 @@ public class TestRedirectLocation {
         Assert.assertTrue(locations.contains(uri3));
         Assert.assertFalse(locations.contains(new URI("/")));
 
-        List<URI> list = locations.getAll();
+        final List<URI> list = locations.getAll();
         Assert.assertNotNull(list);
         Assert.assertEquals(5, list.size());
         Assert.assertEquals(uri1, list.get(0));
@@ -64,16 +64,6 @@ public class TestRedirectLocation {
         Assert.assertEquals(uri2, list.get(2));
         Assert.assertEquals(uri3, list.get(3));
         Assert.assertEquals(uri3, list.get(4));
-
-        Assert.assertTrue(locations.remove(uri3));
-        Assert.assertTrue(locations.remove(uri1));
-        Assert.assertFalse(locations.remove(new URI("/")));
-
-        list = locations.getAll();
-        Assert.assertNotNull(list);
-        Assert.assertEquals(2, list.size());
-        Assert.assertEquals(uri2, list.get(0));
-        Assert.assertEquals(uri2, list.get(1));
     }
 
 }


Mime
View raw message