Set empty request body if it was null (#1778)

Co-authored-by: Kevin Davis <[email protected]>
OpenFeign · Oct 7, 2022 · e3db282 · e3db282
1 parent 4ba51e5
commit e3db282
Show file tree

Hide file tree

Showing 8 changed files with 68 additions and 47 deletions.
diff --git a/core/src/main/java/feign/Client.java b/core/src/main/java/feign/Client.java
@@ -65,7 +65,7 @@ class Default implements Client {
 
     /**
      * Disable the request body internal buffering for {@code HttpURLConnection}.
-     * 
+     *
      * @see HttpURLConnection#setFixedLengthStreamingMode(int)
      * @see HttpURLConnection#setFixedLengthStreamingMode(long)
      * @see HttpURLConnection#setChunkedStreamingMode(int)
@@ -74,7 +74,7 @@ class Default implements Client {
 
     /**
      * Create a new client, which disable request buffering by default.
-     * 
+     *
      * @param sslContextFactory SSLSocketFactory for secure https URL connections.
      * @param hostnameVerifier the host name verifier.
      */
@@ -86,7 +86,7 @@ public Default(SSLSocketFactory sslContextFactory, HostnameVerifier hostnameVeri
 
     /**
      * Create a new client.
-     * 
+     *
      * @param sslContextFactory SSLSocketFactory for secure https URL connections.
      * @param hostnameVerifier the host name verifier.
      * @param disableRequestBuffering Disable the request body internal buffering for
@@ -196,8 +196,19 @@ HttpURLConnection convertAndSend(Request request, Options options) throws IOExce
         connection.addRequestProperty("Accept", "*/*");
       }
 
-      if (request.body() != null) {
-        if (disableRequestBuffering) {
+      boolean hasEmptyBody = false;
+      byte[] body = request.body();
+      if (body == null && request.httpMethod().isWithBody()) {
+        body = new byte[0];
+        hasEmptyBody = true;
+      }
+
+      if (body != null) {
+        /*
+         * Ignore disableRequestBuffering flag if the empty body was set, to ensure that internal
+         * retry logic applies to such requests.
+         */
+        if (disableRequestBuffering && !hasEmptyBody) {
           if (contentLength != null) {
             connection.setFixedLengthStreamingMode(contentLength);
           } else {
@@ -212,7 +223,7 @@ HttpURLConnection convertAndSend(Request request, Options options) throws IOExce
           out = new DeflaterOutputStream(out);
         }
         try {
-          out.write(request.body());
+          out.write(body);
         } finally {
           try {
             out.close();

diff --git a/core/src/main/java/feign/Request.java b/core/src/main/java/feign/Request.java
@@ -30,7 +30,21 @@
 public final class Request implements Serializable {
 
   public enum HttpMethod {
-    GET, HEAD, POST, PUT, DELETE, CONNECT, OPTIONS, TRACE, PATCH
+    GET, HEAD, POST(true), PUT(true), DELETE, CONNECT, OPTIONS, TRACE, PATCH(true);
+
+    private final boolean withBody;
+
+    HttpMethod() {
+      this(false);
+    }
+
+    HttpMethod(boolean withBody) {
+      this.withBody = withBody;
+    }
+
+    public boolean isWithBody() {
+      return this.withBody;
+    }
   }
 
   public enum ProtocolVersion {

diff --git a/core/src/test/java/feign/client/AbstractClientTest.java b/core/src/test/java/feign/client/AbstractClientTest.java
@@ -203,7 +203,7 @@ protected void log(String configKey, String format, Object... args) {}
   }
 
   @Test
-  public void noResponseBodyForPost() {
+  public void noResponseBodyForPost() throws Exception {
     server.enqueue(new MockResponse());
 
     TestInterface api = newBuilder()
@@ -213,7 +213,7 @@ public void noResponseBodyForPost() {
   }
 
   @Test
-  public void noResponseBodyForPut() {
+  public void noResponseBodyForPut() throws Exception {
     server.enqueue(new MockResponse());
 
     TestInterface api = newBuilder()

diff --git a/core/src/test/java/feign/client/DefaultClientTest.java b/core/src/test/java/feign/client/DefaultClientTest.java
@@ -13,11 +13,17 @@
  */
 package feign.client;
 
-import static org.assertj.core.api.Assertions.assertThat;
-import static org.hamcrest.core.Is.isA;
-import static org.junit.Assert.assertEquals;
-import java.io.ByteArrayInputStream;
-import java.io.ByteArrayOutputStream;
+import feign.Client;
+import feign.Client.Proxied;
+import feign.Feign;
+import feign.Feign.Builder;
+import feign.RetryableException;
+import feign.assertj.MockWebServerAssertions;
+import okhttp3.mockwebserver.MockResponse;
+import okhttp3.mockwebserver.SocketPolicy;
+import org.junit.Test;
+import javax.net.ssl.HostnameVerifier;
+import javax.net.ssl.SSLSession;
 import java.io.IOException;
 import java.net.HttpURLConnection;
 import java.net.InetSocketAddress;
@@ -26,20 +32,11 @@
 import java.net.Proxy.Type;
 import java.net.SocketAddress;
 import java.net.URL;
-import java.nio.charset.StandardCharsets;
-import java.util.zip.DeflaterOutputStream;
-import java.util.zip.GZIPOutputStream;
-import javax.net.ssl.HostnameVerifier;
-import javax.net.ssl.SSLSession;
-import okio.Buffer;
-import org.junit.Test;
-import feign.Client;
-import feign.Client.Proxied;
-import feign.Feign;
-import feign.Feign.Builder;
-import feign.RetryableException;
-import okhttp3.mockwebserver.MockResponse;
-import okhttp3.mockwebserver.SocketPolicy;
+import java.util.Collections;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.entry;
+import static org.hamcrest.core.Is.isA;
+import static org.junit.Assert.assertEquals;
 
 /**
  * Tests client-specific behavior, such as ensuring Content-Length is sent when specified.
@@ -97,6 +94,22 @@ public void testPatch() throws Exception {
     super.testPatch();
   }
 
+  @Override
+  public void noResponseBodyForPost() throws Exception {
+    super.noResponseBodyForPost();
+    MockWebServerAssertions.assertThat(server.takeRequest())
+        .hasMethod("POST")
+        .hasHeaders(entry("Content-Length", Collections.singletonList("0")));
+  }
+
+  @Override
+  public void noResponseBodyForPut() throws Exception {
+    super.noResponseBodyForPut();
+    MockWebServerAssertions.assertThat(server.takeRequest())
+        .hasMethod("PUT")
+        .hasHeaders(entry("Content-Length", Collections.singletonList("0")));
+  }
+
   @Test
   @Override
   public void noResponseBodyForPatch() {

diff --git a/java11/pom.xml b/java11/pom.xml
@@ -51,16 +51,6 @@
       <scope>test</scope>
     </dependency>
 
-    <dependency>
-      <groupId>org.assertj</groupId>
-      <artifactId>assertj-core</artifactId>
-      <scope>test</scope>
-    </dependency>
-    <dependency>
-      <groupId>junit</groupId>
-      <artifactId>junit</artifactId>
-      <scope>test</scope>
-    </dependency>
     <dependency>
       <groupId>io.github.openfeign</groupId>
       <artifactId>feign-core</artifactId>

diff --git a/jaxrs2/src/test/java/feign/jaxrs2/JAXRSClientTest.java b/jaxrs2/src/test/java/feign/jaxrs2/JAXRSClientTest.java
@@ -51,7 +51,7 @@ public void testPatch() throws Exception {
   }
 
   @Override
-  public void noResponseBodyForPut() {
+  public void noResponseBodyForPut() throws Exception {
     try {
       super.noResponseBodyForPut();
     } catch (final IllegalStateException e) {

diff --git a/okhttp/src/main/java/feign/okhttp/OkHttpClient.java b/okhttp/src/main/java/feign/okhttp/OkHttpClient.java
@@ -77,10 +77,7 @@ static Request toOkHttpRequest(feign.Request input) {
     }
 
     byte[] inputBody = input.body();
-    boolean isMethodWithBody =
-        HttpMethod.POST == input.httpMethod() || HttpMethod.PUT == input.httpMethod()
-            || HttpMethod.PATCH == input.httpMethod();
-    if (isMethodWithBody) {
+    if (input.httpMethod().isWithBody()) {
       requestBuilder.removeHeader("Content-Type");
       if (inputBody == null) {
         // write an empty BODY to conform with okhttp 2.4.0+

diff --git a/pom.xml b/pom.xml
@@ -517,13 +517,9 @@
         </dependencies>
       </plugin>
 
-      <!-- Ensures checksums are added to published jars -->
       <plugin>
         <artifactId>maven-install-plugin</artifactId>
         <version>${maven-install-plugin.version}</version>
-        <configuration>
-          <createChecksum>true</createChecksum>
-        </configuration>
       </plugin>
 
       <plugin>