diff --git a/CHANGELOG.md b/CHANGELOG.md index 4fcf6dfac2..b228c8a7fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,12 @@ +### Version 13.18 + +* `ApacheHttpClient` no longer sets an empty entity on requests without a body for the idempotent + methods `GET`, `HEAD`, `OPTIONS`, `TRACE` and `DELETE`. Apache HttpClient treats every request + without an entity as idempotent and retries it when the connection is lost before the response + arrives; as an entity-enclosing request, a bodyless `GET` was never retried. Bodyless `POST`, + `PUT`, `PATCH`, `CONNECT` and `QUERY` requests keep their empty entity, so their request shape is + unchanged and they gain no internal retry (#2115). + ### Version 13.17 * `Request` now accepts a `null` `Request.Body` and treats it as "no body", instead of throwing a diff --git a/httpclient/src/main/java/feign/httpclient/ApacheHttpClient.java b/httpclient/src/main/java/feign/httpclient/ApacheHttpClient.java index 010c229cb7..3de0e80d28 100644 --- a/httpclient/src/main/java/feign/httpclient/ApacheHttpClient.java +++ b/httpclient/src/main/java/feign/httpclient/ApacheHttpClient.java @@ -145,13 +145,32 @@ HttpUriRequest toHttpUriRequest(Request request, Request.Options options) } requestBuilder.setEntity(entity); - } else { + } else if (!canOmitEmptyEntity(request.httpMethod())) { requestBuilder.setEntity(new ByteArrayEntity(new byte[0])); } return requestBuilder.build(); } + /** + * Whether a request without a body can be sent without an (empty) entity. Apache HttpClient's + * retry handler treats every request without an entity as idempotent and retries it when the + * connection is lost, so only idempotent methods may omit the entity. Every other method keeps + * the empty entity, so its request shape stays as before. + */ + private static boolean canOmitEmptyEntity(Request.HttpMethod httpMethod) { + switch (httpMethod) { + case GET: + case HEAD: + case OPTIONS: + case TRACE: + case DELETE: + return true; + default: + return false; + } + } + private ContentType getContentType(Request request) { ContentType contentType = null; for (Map.Entry> entry : request.headers().entrySet()) diff --git a/httpclient/src/test/java/feign/httpclient/ApacheHttpClientTest.java b/httpclient/src/test/java/feign/httpclient/ApacheHttpClientTest.java index 4ab3601425..9684660f41 100644 --- a/httpclient/src/test/java/feign/httpclient/ApacheHttpClientTest.java +++ b/httpclient/src/test/java/feign/httpclient/ApacheHttpClientTest.java @@ -22,22 +22,36 @@ import feign.Feign; import feign.Feign.Builder; import feign.FeignException; +import feign.Request; import feign.Request.Options; +import feign.Response; import feign.RetryableException; import feign.Retryer; +import feign.Util; import feign.client.AbstractClientTest; import feign.jaxrs.JAXRSContract; +import java.io.IOException; import java.nio.charset.StandardCharsets; +import java.util.Collections; import javax.ws.rs.GET; +import javax.ws.rs.POST; import javax.ws.rs.PUT; import javax.ws.rs.Path; import javax.ws.rs.QueryParam; import okhttp3.mockwebserver.MockResponse; import okhttp3.mockwebserver.RecordedRequest; +import okhttp3.mockwebserver.SocketPolicy; +import org.apache.http.HttpEntity; +import org.apache.http.HttpEntityEnclosingRequest; import org.apache.http.ProtocolException; import org.apache.http.client.ClientProtocolException; +import org.apache.http.client.methods.HttpUriRequest; +import org.apache.http.impl.client.CloseableHttpClient; import org.apache.http.impl.client.HttpClientBuilder; +import org.apache.http.util.EntityUtils; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; /** Tests client-specific behavior, such as ensuring Content-Length is sent when specified. */ public class ApacheHttpClientTest extends AbstractClientTest { @@ -112,6 +126,164 @@ void notFollowRedirectIsRespected() throws InterruptedException { assertThat(server.takeRequest().getPath()).isEqualTo("/withOptions"); } + /** + * Apache HttpClient's internal retry handler treats every request without an entity as idempotent + * and retries it when the connection is lost before the response arrives. A bodyless request of + * an idempotent method is sent without an entity, so it is retried once. The requests here go to + * the client directly, so the retry can only come from Apache and not from a Feign {@link + * Retryer}. + */ + @ParameterizedTest + @EnumSource( + value = Request.HttpMethod.class, + names = {"GET", "HEAD", "OPTIONS", "TRACE", "DELETE"}) + void bodylessIdempotentRequestIsRetriedAfterLostConnection(Request.HttpMethod httpMethod) + throws Exception { + server.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.DISCONNECT_AFTER_REQUEST)); + server.enqueue(successResponse(httpMethod)); + + try (CloseableHttpClient httpClient = HttpClientBuilder.create().build(); + Response response = execute(httpClient, request(httpMethod, null))) { + assertThat(response.status()).isEqualTo(200); + if (httpMethod != Request.HttpMethod.HEAD) { + assertThat(Util.toString(response.body().asReader(StandardCharsets.UTF_8))).isEqualTo("ok"); + } + } + + assertThat(server.getRequestCount()).isEqualTo(2); + } + + /** + * A bodyless request of every other method keeps its empty entity, so Apache HttpClient does not + * retry it after a lost connection. + */ + @ParameterizedTest + @EnumSource( + value = Request.HttpMethod.class, + names = {"POST", "PUT", "PATCH", "QUERY"}) + void bodylessRequestThatMayNotBeRetriedIsNotRetriedAfterLostConnection( + Request.HttpMethod httpMethod) throws Exception { + server.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.DISCONNECT_AFTER_REQUEST)); + server.enqueue(successResponse(httpMethod)); + + try (CloseableHttpClient httpClient = HttpClientBuilder.create().build()) { + assertThrows(IOException.class, () -> execute(httpClient, request(httpMethod, null))); + } + + assertThat(server.getRequestCount()).isEqualTo(1); + } + + /** + * A request that carries a body keeps its entity, so Apache HttpClient does not retry it after a + * lost connection. Only POST is exercised over the wire: MockWebServer does not read a request + * body of a GET, so a bodyful GET hangs the exchange instead of reaching the server. + */ + @Test + void postWithBodyIsNotRetriedAfterLostConnection() throws Exception { + server.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.DISCONNECT_AFTER_REQUEST)); + server.enqueue(successResponse(Request.HttpMethod.POST)); + + try (CloseableHttpClient httpClient = HttpClientBuilder.create().build()) { + assertThrows( + IOException.class, () -> execute(httpClient, request(Request.HttpMethod.POST, "bar"))); + } + + assertThat(server.getRequestCount()).isEqualTo(1); + } + + /** + * A GET that carries a body is still converted into an entity-enclosing request that carries that + * body, so Apache HttpClient keeps treating it as non-idempotent. The conversion is asserted + * rather than the exchange, because MockWebServer does not read a request body of a GET. + */ + @Test + void getWithBodyIsConvertedToEntityEnclosingRequest() throws Exception { + final Request request = + Request.create( + Request.HttpMethod.GET, + "http://localhost:" + server.getPort() + "/withoutBody?foo=foo", + Collections.emptyMap(), + "bar".getBytes(StandardCharsets.UTF_8), + null, + null); + + try (CloseableHttpClient httpClient = HttpClientBuilder.create().build()) { + final HttpUriRequest httpUriRequest = + new ApacheHttpClient(httpClient) + .toHttpUriRequest(request, new Options(2, SECONDS, 2, SECONDS, true)); + + assertThat(httpUriRequest).isInstanceOf(HttpEntityEnclosingRequest.class); + assertThat(httpUriRequest.getMethod()).isEqualTo("GET"); + assertThat(httpUriRequest.getURI().getRawQuery()).isEqualTo("foo=foo"); + + final HttpEntity entity = ((HttpEntityEnclosingRequest) httpUriRequest).getEntity(); + assertThat(entity).isNotNull(); + assertThat(EntityUtils.toString(entity, StandardCharsets.UTF_8)).isEqualTo("bar"); + } + } + + /** + * Query parameters of a bodyless GET keep travelling on the request line, now that the request + * carries no entity. + */ + @Test + void bodylessGetKeepsQueryParamsOnRequestLine() throws Exception { + server.enqueue(new MockResponse().setBody("ok")); + + final JaxRsTestInterface testInterface = buildTestInterface(); + + assertThat(testInterface.getWithoutBody("foo")).isEqualTo("ok"); + final RecordedRequest request = server.takeRequest(); + assertThat(request.getMethod()).isEqualTo("GET"); + assertThat(request.getPath()).isEqualTo("/withoutBody?foo=foo"); + } + + /** + * Bodyless POST and PUT keep the empty body and the query parameters on the request line, so + * their request shape is the same as before. + */ + @Test + void bodylessPostAndPutKeepQueryParamsOnRequestLine() throws Exception { + server.enqueue(new MockResponse().setBody("ok")); + server.enqueue(new MockResponse().setBody("ok")); + + final JaxRsTestInterface testInterface = buildTestInterface(); + + assertThat(testInterface.postWithoutBody("foo")).isEqualTo("ok"); + final RecordedRequest postRequest = server.takeRequest(); + assertThat(postRequest.getMethod()).isEqualTo("POST"); + assertThat(postRequest.getPath()).isEqualTo("/withoutBody?foo=foo"); + assertThat(postRequest.getBody().readString(StandardCharsets.UTF_8)).isEqualTo(""); + + assertThat(testInterface.withoutBody("foo")).isEqualTo("ok"); + final RecordedRequest putRequest = server.takeRequest(); + assertThat(putRequest.getMethod()).isEqualTo("PUT"); + assertThat(putRequest.getPath()).isEqualTo("/withoutBody?foo=foo"); + assertThat(putRequest.getBody().readString(StandardCharsets.UTF_8)).isEqualTo(""); + } + + private Response execute(CloseableHttpClient httpClient, Request request) throws IOException { + return new ApacheHttpClient(httpClient) + .execute(request, new Options(2, SECONDS, 2, SECONDS, true)); + } + + private Request request(Request.HttpMethod httpMethod, String body) { + return Request.create( + httpMethod, + "http://localhost:" + server.getPort() + "/withoutBody", + Collections.emptyMap(), + body == null ? null : body.getBytes(StandardCharsets.UTF_8), + null, + null); + } + + /** A HEAD response has no body for the client, so only the status is left to assert. */ + private static MockResponse successResponse(Request.HttpMethod httpMethod) { + return httpMethod == Request.HttpMethod.HEAD + ? new MockResponse().setResponseCode(200) + : new MockResponse().setBody("ok"); + } + private JaxRsTestInterface buildTestInterface() { return Feign.builder() .contract(new JAXRSContract()) @@ -142,5 +314,13 @@ public interface JaxRsTestInterface { @GET @Path("/withOptions") public String withOptions(Options options); + + @GET + @Path("/withoutBody") + public String getWithoutBody(@QueryParam("foo") String foo); + + @POST + @Path("/withoutBody") + public String postWithoutBody(@QueryParam("foo") String foo); } }