From 98daeabc0541939b2118044be5aaaa3e29fe2a38 Mon Sep 17 00:00:00 2001 From: Oleg Kalnichevski Date: Sun, 15 Mar 2020 10:32:54 +0100 Subject: [PATCH] HTTPCLIENT-2061: corrected sequence of request execution interceptors in classic HttpClient --- .../client5/testing/sync/TestRedirects.java | 48 +++++++++++++++++++ .../hc/client5/http/impl/ChainElement.java | 2 +- .../http/impl/classic/HttpClientBuilder.java | 28 +++++------ 3 files changed, 62 insertions(+), 16 deletions(-) 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 78b12b1b6..8ea330a25 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,6 +30,8 @@ import java.io.IOException; import java.net.URI; import java.net.URISyntaxException; import java.util.Arrays; +import java.util.Queue; +import java.util.concurrent.ConcurrentLinkedQueue; import org.apache.hc.client5.http.CircularRedirectException; import org.apache.hc.client5.http.ClientProtocolException; @@ -63,6 +65,7 @@ import org.apache.hc.core5.http.io.entity.EntityUtils; import org.apache.hc.core5.http.io.entity.StringEntity; import org.apache.hc.core5.http.message.BasicHeader; import org.apache.hc.core5.http.protocol.HttpContext; +import org.hamcrest.CoreMatchers; import org.junit.Assert; import org.junit.Test; @@ -657,4 +660,49 @@ public class TestRedirects extends LocalServerTestBase { } } + @Test + public void testCompressionHeaderRedirect() throws Exception { + final Queue values = new ConcurrentLinkedQueue<>(); + final HttpHost target = start(null, new Decorator() { + + @Override + public HttpServerRequestHandler decorate(final HttpServerRequestHandler requestHandler) { + return new RedirectingDecorator( + requestHandler, + new OldPathRedirectResolver("/oldlocation", "/random", HttpStatus.SC_MOVED_TEMPORARILY)) { + + @Override + public void handle(final ClassicHttpRequest request, + final ResponseTrigger responseTrigger, + final HttpContext context) throws HttpException, IOException { + final Header header = request.getHeader(HttpHeaders.ACCEPT_ENCODING); + if (header != null) { + values.add(header.getValue()); + } + super.handle(request, responseTrigger, context); + } + + }; + } + + }); + + final HttpClientContext context = HttpClientContext.create(); + + final HttpGet httpget = new HttpGet("/oldlocation/100"); + + try (final ClassicHttpResponse response = this.httpclient.execute(target, httpget, context)) { + final HttpRequest reqWrapper = context.getRequest(); + + Assert.assertEquals(HttpStatus.SC_OK, response.getCode()); + Assert.assertEquals(URIUtils.create(target, "/random/100"), reqWrapper.getUri()); + + EntityUtils.consume(response.getEntity()); + } + + Assert.assertThat(values.poll(), CoreMatchers.equalTo("gzip, x-gzip, deflate")); + Assert.assertThat(values.poll(), CoreMatchers.equalTo("gzip, x-gzip, deflate")); + Assert.assertThat(values.poll(), CoreMatchers.nullValue()); + } + } diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/ChainElement.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/ChainElement.java index 0c34f8319..5ebcf4b60 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/ChainElement.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/ChainElement.java @@ -34,6 +34,6 @@ package org.apache.hc.client5.http.impl; */ public enum ChainElement { - REDIRECT, BACK_OFF, RETRY, CACHING, PROTOCOL, CONNECT, MAIN_TRANSPORT + REDIRECT, COMPRESS, BACK_OFF, RETRY, CACHING, PROTOCOL, CONNECT, MAIN_TRANSPORT } diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/HttpClientBuilder.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/HttpClientBuilder.java index 573ebb01d..49e223e05 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/HttpClientBuilder.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/HttpClientBuilder.java @@ -871,17 +871,6 @@ public class HttpClientBuilder { } } - // Add redirect executor, if not disabled - if (!redirectHandlingDisabled) { - RedirectStrategy redirectStrategyCopy = this.redirectStrategy; - if (redirectStrategyCopy == null) { - redirectStrategyCopy = DefaultRedirectStrategy.INSTANCE; - } - execChainDefinition.addFirst( - new RedirectExec(routePlannerCopy, redirectStrategyCopy), - ChainElement.REDIRECT.name()); - } - if (!contentCompressionDisabled) { if (contentDecoderMap != null) { final List encodings = new ArrayList<>(contentDecoderMap.keySet()); @@ -892,14 +881,23 @@ public class HttpClientBuilder { final Registry decoderRegistry = b2.build(); execChainDefinition.addFirst( new ContentCompressionExec(encodings, decoderRegistry, true), - ChainElement.REDIRECT.name()); + ChainElement.COMPRESS.name()); } else { - execChainDefinition.addFirst( - new ContentCompressionExec(true), - ChainElement.REDIRECT.name()); + execChainDefinition.addFirst(new ContentCompressionExec(true), ChainElement.COMPRESS.name()); } } + // Add redirect executor, if not disabled + if (!redirectHandlingDisabled) { + RedirectStrategy redirectStrategyCopy = this.redirectStrategy; + if (redirectStrategyCopy == null) { + redirectStrategyCopy = DefaultRedirectStrategy.INSTANCE; + } + execChainDefinition.addFirst( + new RedirectExec(routePlannerCopy, redirectStrategyCopy), + ChainElement.REDIRECT.name()); + } + // Optionally, add connection back-off executor if (this.backoffManager != null && this.connectionBackoffStrategy != null) { execChainDefinition.addFirst(new BackoffStrategyExec(this.connectionBackoffStrategy, this.backoffManager),