ApacheHttp5Client uses Options to follow Redirects (#2104)

* Added setRedirectsEnabled and test cases

* Formatting

* Added same for Async Client

* Formatting

* Formatting

* Formatting

* Formatting

* formatting

hc5/src/main/java/feign/hc5/ApacheHttp5Client.java

@@ -82,14 +82,14 @@ public final class ApacheHttp5Client implements Client {
       throw new IOException("URL '" + request.url() + "' couldn't be parsed into a URI", e);
     }
     final HttpHost target = HttpHost.create(URI.create(request.url()));
-    final HttpClientContext context = configureTimeouts(options);
+    final HttpClientContext context = configureTimeoutsAndRedirection(options);
 
     final ClassicHttpResponse httpResponse =
         (ClassicHttpResponse) client.execute(target, httpUriRequest, context);
     return toFeignResponse(httpResponse, request);
   }
 
-  protected HttpClientContext configureTimeouts(Request.Options options) {
+  protected HttpClientContext configureTimeoutsAndRedirection(Request.Options options) {
     final HttpClientContext context = new HttpClientContext();
     // per request timeouts
     final RequestConfig requestConfig =
@@ -98,6 +98,7 @@ public final class ApacheHttp5Client implements Client {
             : RequestConfig.custom())
                 .setConnectTimeout(options.connectTimeout(), options.connectTimeoutUnit())
                 .setResponseTimeout(options.readTimeout(), options.readTimeoutUnit())
+                .setRedirectsEnabled(options.isFollowRedirects())
                 .build();
     context.setRequestConfig(requestConfig);
     return context;

hc5/src/main/java/feign/hc5/AsyncApacheHttp5Client.java

@@ -50,16 +50,16 @@ public final class AsyncApacheHttp5Client implements AsyncClient<HttpClientConte
     this(createStartedClient());
   }
 
+  public AsyncApacheHttp5Client(CloseableHttpAsyncClient client) {
+    this.client = client;
+  }
+
   private static CloseableHttpAsyncClient createStartedClient() {
     final CloseableHttpAsyncClient client = HttpAsyncClients.custom().build();
     client.start();
     return client;
   }
 
-  public AsyncApacheHttp5Client(CloseableHttpAsyncClient client) {
-    this.client = client;
-  }
-
   @Override
   public CompletableFuture<Response> execute(Request request,
                                              Options options,
@@ -86,14 +86,14 @@ public final class AsyncApacheHttp5Client implements AsyncClient<HttpClientConte
     };
 
     client.execute(httpUriRequest,
-        configureTimeouts(options, requestContext.orElseGet(HttpClientContext::new)),
+        configureTimeoutsAndRedirection(options, requestContext.orElseGet(HttpClientContext::new)),
         callback);
 
     return result;
   }
 
-  protected HttpClientContext configureTimeouts(Request.Options options,
-                                                HttpClientContext context) {
+  protected HttpClientContext configureTimeoutsAndRedirection(Request.Options options,
+                                                              HttpClientContext context) {
     // per request timeouts
     final RequestConfig requestConfig =
         (client instanceof Configurable
@@ -101,6 +101,7 @@ public final class AsyncApacheHttp5Client implements AsyncClient<HttpClientConte
             : RequestConfig.custom())
                 .setConnectTimeout(options.connectTimeout(), options.connectTimeoutUnit())
                 .setResponseTimeout(options.readTimeout(), options.readTimeoutUnit())
+                .setRedirectsEnabled(options.isFollowRedirects())
                 .build();
     context.setRequestConfig(requestConfig);
     return context;

hc5/src/test/java/feign/hc5/ApacheHttp5ClientTest.java

@@ -13,17 +13,22 @@
  */
 package feign.hc5;
 
+import static java.util.concurrent.TimeUnit.SECONDS;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertThrows;
 import static org.junit.Assume.assumeTrue;
-import org.apache.hc.client5.http.classic.HttpClient;
 import org.apache.hc.client5.http.impl.classic.HttpClientBuilder;
 import org.junit.Test;
 import java.nio.charset.StandardCharsets;
+import javax.ws.rs.GET;
 import javax.ws.rs.PUT;
 import javax.ws.rs.Path;
 import javax.ws.rs.QueryParam;
 import feign.Feign;
 import feign.Feign.Builder;
+import feign.FeignException;
+import feign.Request;
 import feign.client.AbstractClientTest;
 import feign.jaxrs.JAXRSContract;
 import okhttp3.mockwebserver.MockResponse;
@@ -41,11 +46,7 @@ public class ApacheHttp5ClientTest extends AbstractClientTest {
 
   @Test
   public void queryParamsAreRespectedWhenBodyIsEmpty() throws InterruptedException {
-    final HttpClient httpClient = HttpClientBuilder.create().build();
-    final JaxRsTestInterface testInterface = Feign.builder()
-        .contract(new JAXRSContract())
-        .client(new ApacheHttp5Client(httpClient))
-        .target(JaxRsTestInterface.class, "http://localhost:" + server.getPort());
+    final JaxRsTestInterface testInterface = buildTestInterface();
 
     server.enqueue(new MockResponse().setBody("foo"));
     server.enqueue(new MockResponse().setBody("foo"));
@@ -61,6 +62,56 @@ public class ApacheHttp5ClientTest extends AbstractClientTest {
     assertEquals("", request2.getBody().readString(StandardCharsets.UTF_8));
   }
 
+  @Test
+  public void followRedirectsIsTrue() throws InterruptedException {
+    final JaxRsTestInterface testInterface = buildTestInterface();
+
+    String redirectPath = getRedirectionUrl();
+    server.enqueue(buildMockResponseWithLocationHeader(redirectPath));
+    server.enqueue(new MockResponse().setBody("redirected"));
+    Request.Options options = buildRequestOptions(true);
+
+    Object response = testInterface.withOptions(options);
+    assertNotNull(response);
+    assertEquals("redirected", response);
+    assertEquals("/withRequestOptions", server.takeRequest().getPath());
+  }
+
+  @Test
+  public void followRedirectsIsFalse() throws InterruptedException {
+    final JaxRsTestInterface testInterface = buildTestInterface();
+
+    String redirectPath = getRedirectionUrl();
+    server.enqueue(buildMockResponseWithLocationHeader(redirectPath));
+    Request.Options options = buildRequestOptions(false);
+
+    FeignException feignException =
+        assertThrows(FeignException.class, () -> testInterface.withOptions(options));
+    assertEquals(302, feignException.status());
+    assertEquals(redirectPath,
+        feignException.responseHeaders().get("location").stream().findFirst().orElse(null));
+    assertEquals("/withRequestOptions", server.takeRequest().getPath());
+  }
+
+  private JaxRsTestInterface buildTestInterface() {
+    return Feign.builder()
+        .contract(new JAXRSContract())
+        .client(new ApacheHttp5Client(HttpClientBuilder.create().build()))
+        .target(JaxRsTestInterface.class, "http://localhost:" + server.getPort());
+  }
+
+  private MockResponse buildMockResponseWithLocationHeader(String redirectPath) {
+    return new MockResponse().setResponseCode(302).addHeader("location", redirectPath);
+  }
+
+  private String getRedirectionUrl() {
+    return "http://localhost:" + server.getPort() + "/redirected";
+  }
+
+  private Request.Options buildRequestOptions(boolean followRedirects) {
+    return new Request.Options(1, SECONDS, 1, SECONDS, followRedirects);
+  }
+
   @Override
   public void testVeryLongResponseNullLength() {
     assumeTrue("HC5 client seems to hang with response size equalto Long.MAX", false);
@@ -80,5 +131,9 @@ public class ApacheHttp5ClientTest extends AbstractClientTest {
     @PUT
     @Path("/withoutBody")
     String withoutBody(@QueryParam("foo") String foo);
+
+    @GET
+    @Path("/withRequestOptions")
+    String withOptions(Request.Options options);
   }
 }

hc5/src/test/java/feign/hc5/AsyncApacheHttp5ClientTest.java

@@ -14,14 +14,18 @@
 package feign.hc5;
 
 import static feign.assertj.MockWebServerAssertions.assertThat;
+import static java.util.concurrent.TimeUnit.SECONDS;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.data.MapEntry.entry;
 import static org.hamcrest.CoreMatchers.isA;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 import com.google.gson.Gson;
 import com.google.gson.reflect.TypeToken;
+import org.apache.hc.client5.http.impl.classic.HttpClientBuilder;
 import org.apache.hc.client5.http.protocol.HttpClientContext;
 import org.junit.Rule;
 import org.junit.Test;
@@ -37,8 +41,10 @@ import feign.Feign.ResponseMappingDecoder;
 import feign.Request.HttpMethod;
 import feign.Target.HardCodedTarget;
 import feign.codec.*;
+import feign.jaxrs.JAXRSContract;
 import feign.querymap.BeanQueryMapEncoder;
 import feign.querymap.FieldQueryMapEncoder;
+import kotlin.text.Charsets;
 import okhttp3.mockwebserver.MockResponse;
 import okhttp3.mockwebserver.MockWebServer;
 import okio.Buffer;
@@ -765,6 +771,56 @@ public class AsyncApacheHttp5ClientTest {
     checkCFCompletedSoon(cf);
   }
 
+  @Test
+  public void followRedirectsIsTrue() throws Throwable {
+
+    String redirectPath = "/redirected";
+
+    server.enqueue(buildMockResponseWithLocationHeader(redirectPath));
+    server.enqueue(new MockResponse().setBody("redirectedBody"));
+    Request.Options options = buildRequestOptions(true);
+
+    final TestInterfaceAsync api =
+        new TestInterfaceAsyncBuilder().options(options)
+            .target("http://localhost:" + server.getPort());
+
+    Response response = unwrap(api.response());
+    assertNotNull(response);
+    assertEquals(200, response.status());
+    assertEquals("redirectedBody", Util.toString(response.body().asReader(Util.UTF_8)));
+    assertEquals("/", server.takeRequest().getPath());
+    assertEquals("/redirected", server.takeRequest().getPath());
+  }
+
+  @Test
+  public void followRedirectsIsFalse() throws Throwable {
+    String redirectPath = "/redirected";
+
+    server.enqueue(buildMockResponseWithLocationHeader(redirectPath));
+    Request.Options options = buildRequestOptions(false);
+
+    final TestInterfaceAsync api =
+        new TestInterfaceAsyncBuilder().options(options)
+            .target("http://localhost:" + server.getPort());
+
+    Response response = unwrap(api.response());
+    final String path = response.headers().get("location").stream().findFirst().orElse(null);
+    assertNotNull(response);
+    assertNotNull(path);
+    assertEquals(302, response.status());
+    assertEquals("/", server.takeRequest().getPath());
+    assertTrue(path.contains("/redirected"));
+  }
+
+  private MockResponse buildMockResponseWithLocationHeader(String redirectPath) {
+    return new MockResponse().setResponseCode(302).addHeader("location",
+        "http://localhost:" + server.getPort() + redirectPath);
+  }
+
+  private Request.Options buildRequestOptions(boolean followRedirects) {
+    return new Request.Options(1, SECONDS, 1, SECONDS, followRedirects);
+  }
+
   public interface TestInterfaceAsync {
 
     @RequestLine("POST /")
@@ -965,6 +1021,11 @@ public class AsyncApacheHttp5ClientTest {
       return this;
     }
 
+    TestInterfaceAsyncBuilder options(Request.Options options) {
+      delegate.options(options);
+      return this;
+    }
+
     TestInterfaceAsync target(String url) {
       return delegate.target(TestInterfaceAsync.class, url);
     }