From 51da4d51a935107384ac8d65dc145dd3fa96466e Mon Sep 17 00:00:00 2001 From: Tom Granot Date: Mon, 28 Sep 2020 18:12:36 +0300 Subject: [PATCH] Account for recurring HEADs in Head302Test Originally, when setFollowRedirect is set to true (like in Head302Test), the expected flow is HEAD --> GET (based on the behaviour of Redirect30xInterceptor - see https://github.com/AsyncHttpClient/async-http-client/commit/3c25a42289bf679cfff40ca5ef0e77be85215deb for the reasoning). Following a change to the interceptor (to fix https://github.com/AsyncHttpClient/async-http-client/issues/1728), Head302Test started going on an infinite loop caused by the way the handler in the test was set up (to account for the original HEAD --> GET flow and not the new HEAD --> HEAD --> HEAD.... flow). This PR does 3 things: * Changes Redirect30xInterceptor to set all redirects of a HEAD request to be HEADs as well * Change Head302Test to account for the new flow * Notes a flaky test in AsyncStreamHandlerTest that was found during work on the PR --- .../handler/intercept/Redirect30xInterceptor.java | 5 +++-- .../asynchttpclient/AsyncStreamHandlerTest.java | 2 ++ .../test/java/org/asynchttpclient/Head302Test.java | 14 +++++++++++--- 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/client/src/main/java/org/asynchttpclient/netty/handler/intercept/Redirect30xInterceptor.java b/client/src/main/java/org/asynchttpclient/netty/handler/intercept/Redirect30xInterceptor.java index d56b90fd24..a2ddbd9467 100644 --- a/client/src/main/java/org/asynchttpclient/netty/handler/intercept/Redirect30xInterceptor.java +++ b/client/src/main/java/org/asynchttpclient/netty/handler/intercept/Redirect30xInterceptor.java @@ -38,6 +38,8 @@ import static io.netty.handler.codec.http.HttpHeaderNames.*; import static org.asynchttpclient.util.HttpConstants.Methods.GET; +import static org.asynchttpclient.util.HttpConstants.Methods.HEAD; +import static org.asynchttpclient.util.HttpConstants.Methods.OPTIONS; import static org.asynchttpclient.util.HttpConstants.ResponseStatusCodes.*; import static org.asynchttpclient.util.HttpUtils.followRedirect; import static org.asynchttpclient.util.MiscUtils.isNonEmpty; @@ -87,7 +89,7 @@ public boolean exitAfterHandlingRedirect(Channel channel, String originalMethod = request.getMethod(); boolean switchToGet = !originalMethod.equals(GET) - && (statusCode == MOVED_PERMANENTLY_301 || statusCode == SEE_OTHER_303 || (statusCode == FOUND_302 && !config.isStrict302Handling())); + && !originalMethod.equals(OPTIONS) && !originalMethod.equals(HEAD) && (statusCode == MOVED_PERMANENTLY_301 || statusCode == SEE_OTHER_303 || (statusCode == FOUND_302 && !config.isStrict302Handling())); boolean keepBody = statusCode == TEMPORARY_REDIRECT_307 || statusCode == PERMANENT_REDIRECT_308 || (statusCode == FOUND_302 && config.isStrict302Handling()); final RequestBuilder requestBuilder = new RequestBuilder(switchToGet ? GET : originalMethod) @@ -126,7 +128,6 @@ else if (isNonEmpty(request.getBodyParts())) { HttpHeaders responseHeaders = response.headers(); String location = responseHeaders.get(LOCATION); Uri newUri = Uri.create(future.getUri(), location); - LOGGER.debug("Redirecting to {}", newUri); CookieStore cookieStore = config.getCookieStore(); diff --git a/client/src/test/java/org/asynchttpclient/AsyncStreamHandlerTest.java b/client/src/test/java/org/asynchttpclient/AsyncStreamHandlerTest.java index 1547872aaa..6d30acfb6f 100644 --- a/client/src/test/java/org/asynchttpclient/AsyncStreamHandlerTest.java +++ b/client/src/test/java/org/asynchttpclient/AsyncStreamHandlerTest.java @@ -428,6 +428,8 @@ public Integer onCompleted() { })); } + // This test is flaky - see https://github.com/AsyncHttpClient/async-http-client/issues/1728#issuecomment-699962325 + // For now, just run again if fails @Test(groups = "online") public void asyncOptionsTest() throws Throwable { diff --git a/client/src/test/java/org/asynchttpclient/Head302Test.java b/client/src/test/java/org/asynchttpclient/Head302Test.java index 2072f3dbb3..2a3f5bf294 100644 --- a/client/src/test/java/org/asynchttpclient/Head302Test.java +++ b/client/src/test/java/org/asynchttpclient/Head302Test.java @@ -70,9 +70,17 @@ public Response onCompleted(Response response) throws Exception { private static class Head302handler extends AbstractHandler { public void handle(String s, org.eclipse.jetty.server.Request r, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException { if ("HEAD".equalsIgnoreCase(request.getMethod())) { - response.setStatus(HttpServletResponse.SC_FOUND); // 302 - response.setHeader("Location", request.getPathInfo() + "_moved"); - } else if ("GET".equalsIgnoreCase(request.getMethod())) { + // See https://github.com/AsyncHttpClient/async-http-client/issues/1728#issuecomment-700007980 + // When setFollowRedirect == TRUE, a follow-up request to a HEAD request will also be a HEAD. + // This will cause an infinite loop, which will error out once the maximum amount of redirects is hit (default 5). + // Instead, we (arbitrarily) choose to allow for 3 redirects and then return a 200. + if(request.getRequestURI().endsWith("_moved_moved_moved")) { + response.setStatus(HttpServletResponse.SC_OK); + } else { + response.setStatus(HttpServletResponse.SC_FOUND); // 302 + response.setHeader("Location", request.getPathInfo() + "_moved"); + } + } else if ("GET".equalsIgnoreCase(request.getMethod()) ) { response.setStatus(HttpServletResponse.SC_OK); } else { response.setStatus(HttpServletResponse.SC_FORBIDDEN);