Support RequestBody#contentLength. (#1227)

RequestBody supports optional content length.
palantir · Apr 28, 2021 · 89926b7 · 89926b7
1 parent 0e825d9
commit 89926b7
Show file tree

Hide file tree

Showing 7 changed files with 166 additions and 52 deletions.
diff --git a/changelog/@unreleased/pr-1227.v2.yml b/changelog/@unreleased/pr-1227.v2.yml
@@ -0,0 +1,5 @@
+type: improvement
+improvement:
+  description: RequestBody supports optional content length.
+  links:
+  - https://github.com/palantir/dialogue/pull/1227
diff --git a/...e-hc5-client/src/main/java/com/palantir/dialogue/hc5/ApacheHttpClientBlockingChannel.java b/...e-hc5-client/src/main/java/com/palantir/dialogue/hc5/ApacheHttpClientBlockingChannel.java
@@ -177,24 +177,41 @@ private static boolean requiresEmptyBody(Endpoint endpoint) {
     }
 
     private static void setBody(ClassicRequestBuilder builder, RequestBody body) {
-        builder.setEntity(new RequestBodyEntity(body, contentLength(builder)));
+        builder.setEntity(new RequestBodyEntity(body, contentLength(body, builder)));
     }
 
-    private static OptionalLong contentLength(ClassicRequestBuilder builder) {
+    private static OptionalLong contentLength(RequestBody requestBody, ClassicRequestBuilder builder) {
         Header contentLengthHeader = builder.getFirstHeader(HttpHeaders.CONTENT_LENGTH);
+        OptionalLong headerContentLength = OptionalLong.empty();
         if (contentLengthHeader != null) {
             builder.removeHeaders(HttpHeaders.CONTENT_LENGTH);
             String contentLengthValue = contentLengthHeader.getValue();
             try {
-                return OptionalLong.of(Long.parseLong(contentLengthValue));
+                headerContentLength = OptionalLong.of(Long.parseLong(contentLengthValue));
             } catch (NumberFormatException nfe) {
                 log.warn(
                         "Failed to parse content-length value '{}'",
                         SafeArg.of(HttpHeaders.CONTENT_LENGTH, contentLengthValue),
                         nfe);
             }
         }
-        return OptionalLong.empty();
+
+        if (headerContentLength.isPresent() && requestBody.contentLength().isPresent()) {
+            long headerContentLengthValue = headerContentLength.getAsLong();
+            long requestBodyContentLength = requestBody.contentLength().getAsLong();
+            if (headerContentLengthValue != requestBodyContentLength) {
+                log.warn(
+                        "Content lengths do not match",
+                        SafeArg.of(HttpHeaders.CONTENT_LENGTH, headerContentLengthValue),
+                        SafeArg.of("requestBodyContentLength", requestBodyContentLength));
+            }
+        }
+
+        if (headerContentLength.isPresent()) {
+            return headerContentLength;
+        }
+
+        return requestBody.contentLength();
     }
 
     private static final class HttpClientResponse implements Response {

diff --git a/...ache-hc5-client/src/test/java/com/palantir/dialogue/hc5/ApacheHttpClientChannelsTest.java b/...ache-hc5-client/src/test/java/com/palantir/dialogue/hc5/ApacheHttpClientChannelsTest.java
@@ -30,14 +30,19 @@
 import com.palantir.dialogue.Channel;
 import com.palantir.dialogue.HttpMethod;
 import com.palantir.dialogue.Request;
+import com.palantir.dialogue.RequestBody;
 import com.palantir.dialogue.Response;
 import com.palantir.dialogue.TestConfigurations;
 import com.palantir.dialogue.TestEndpoint;
 import com.palantir.logsafe.Arg;
 import com.palantir.logsafe.SafeLoggable;
 import com.palantir.tritium.metrics.registry.TaggedMetricRegistry;
+import java.io.IOException;
+import java.io.OutputStream;
 import java.net.UnknownHostException;
 import java.util.Map;
+import java.util.Optional;
+import java.util.OptionalLong;
 import okhttp3.mockwebserver.RecordedRequest;
 import org.junit.jupiter.api.Test;
 
@@ -119,19 +124,93 @@ public void metrics() throws Exception {
     }
 
     @Test
+    public void supportsContentLengthHeader() {
+        testContentLength(
+                Optional.of(Integer.toString(CONTENT.length)),
+                Optional.of(new RequestBody() {
+                    @Override
+                    public void writeTo(OutputStream output) throws IOException {
+                        output.write(CONTENT);
+                    }
+
+                    @Override
+                    public String contentType() {
+                        return "application/text";
+                    }
+
+                    @Override
+                    public boolean repeatable() {
+                        return true;
+                    }
+
+                    @Override
+                    public OptionalLong contentLength() {
+                        return OptionalLong.empty();
+                    }
+
+                    @Override
+                    public void close() {}
+                }),
+                CONTENT.length);
+    }
+
+    @Test
+    public void supportsContentLengthValueOnBody() {
+        testContentLength(Optional.empty(), Optional.of(body), CONTENT.length);
+    }
+
+    @Test
+    public void contentLengthHeaderPreferredToBody() {
+        int fakeLength = CONTENT.length - 1;
+        testContentLength(
+                Optional.of(Integer.toString(CONTENT.length)),
+                Optional.of(new RequestBody() {
+                    @Override
+                    public void writeTo(OutputStream output) throws IOException {
+                        output.write(CONTENT);
+                    }
+
+                    @Override
+                    public String contentType() {
+                        return "application/text";
+                    }
+
+                    @Override
+                    public boolean repeatable() {
+                        return true;
+                    }
+
+                    @Override
+                    public OptionalLong contentLength() {
+                        return OptionalLong.of(fakeLength);
+                    }
+
+                    @Override
+                    public void close() {}
+                }),
+                CONTENT.length);
+    }
+
+    @Test
+    public void unparseableContentLengthHeaderIsSupported() {
+        testContentLength(Optional.of("you can't parse me!"), Optional.of(body), CONTENT.length);
+    }
+
     @SuppressWarnings("FutureReturnValueIgnored")
-    public void supportsContentLengthHeader() throws InterruptedException {
-        endpoint.method = HttpMethod.POST;
-        request = Request.builder()
-                .from(request)
-                .body(body)
-                .putHeaderParams(HttpHeaders.CONTENT_LENGTH, Integer.toString(CONTENT.length))
-                .build();
-        channel.execute(endpoint, request);
-        RecordedRequest recordedRequest = server.takeRequest();
-        assertThat(recordedRequest.getMethod()).isEqualTo("POST");
-        assertThat(recordedRequest.getHeader(HttpHeaders.CONTENT_LENGTH)).isEqualTo(Integer.toString(CONTENT.length));
-        assertThat(recordedRequest.getHeader(HttpHeaders.TRANSFER_ENCODING)).isNull();
+    private void testContentLength(Optional<String> headerValue, Optional<RequestBody> body, long value) {
+        try {
+            endpoint.method = HttpMethod.POST;
+            Request.Builder builder = Request.builder().from(request).body(body);
+            headerValue.ifPresent(header -> builder.putHeaderParams(HttpHeaders.CONTENT_LENGTH, header));
+            request = builder.build();
+            channel.execute(endpoint, request);
+            RecordedRequest recordedRequest = server.takeRequest();
+            assertThat(recordedRequest.getMethod()).isEqualTo("POST");
+            assertThat(recordedRequest.getHeader(HttpHeaders.CONTENT_LENGTH)).isEqualTo(Long.toString(value));
+            assertThat(recordedRequest.getHeader(HttpHeaders.TRANSFER_ENCODING)).isNull();
+        } catch (InterruptedException e) {
+            throw new RuntimeException(e);
+        }
     }
 
     @SuppressWarnings("JdkObsolete")