Restore Apache HTTP4 retries for bodyless idempotent requests

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

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<String, Collection<String>> entry : request.headers().entrySet())

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);
   }
 }