feat: support chunked transfer encoding (#910)

Currently any time HttpRequest works with encoded data it encodes the data twice. Once for the actual stream and once for checking the length of the stream. Instead, when there is encoding just don't set the content length. This will cause the underlying transports, with a few tweaks, to use Transfer-Encoding: chunked. Fixes #648
googleapis · Dec 17, 2019 · b8d6abe · b8d6abe
1 parent 7d4a048
commit b8d6abe
Show file tree

Hide file tree

Showing 9 changed files with 135 additions and 34 deletions.
diff --git a/...lient-apache-v2/src/main/java/com/google/api/client/http/apache/v2/ApacheHttpRequest.java b/...lient-apache-v2/src/main/java/com/google/api/client/http/apache/v2/ApacheHttpRequest.java
@@ -23,9 +23,7 @@
 import org.apache.http.client.config.RequestConfig;
 import org.apache.http.client.methods.HttpRequestBase;
 
-/**
- * @author Yaniv Inbar
- */
+/** @author Yaniv Inbar */
 final class ApacheHttpRequest extends LowLevelHttpRequest {
   private final HttpClient httpClient;
 
@@ -38,11 +36,12 @@ final class ApacheHttpRequest extends LowLevelHttpRequest {
     this.httpClient = httpClient;
     this.request = request;
     // disable redirects as google-http-client handles redirects
-    this.requestConfig = RequestConfig.custom()
-        .setRedirectsEnabled(false)
-        .setNormalizeUri(false)
-        // TODO(chingor): configure in HttpClientBuilder when available
-        .setStaleConnectionCheckEnabled(false);
+    this.requestConfig =
+        RequestConfig.custom()
+            .setRedirectsEnabled(false)
+            .setNormalizeUri(false)
+            // TODO(chingor): configure in HttpClientBuilder when available
+            .setStaleConnectionCheckEnabled(false);
   }
 
   @Override
@@ -52,19 +51,22 @@ public void addHeader(String name, String value) {
 
   @Override
   public void setTimeout(int connectTimeout, int readTimeout) throws IOException {
-    requestConfig.setConnectTimeout(connectTimeout)
-        .setSocketTimeout(readTimeout);
+    requestConfig.setConnectTimeout(connectTimeout).setSocketTimeout(readTimeout);
   }
 
   @Override
   public LowLevelHttpResponse execute() throws IOException {
     if (getStreamingContent() != null) {
-      Preconditions.checkArgument(request instanceof HttpEntityEnclosingRequest,
+      Preconditions.checkState(
+          request instanceof HttpEntityEnclosingRequest,
           "Apache HTTP client does not support %s requests with content.",
           request.getRequestLine().getMethod());
       ContentEntity entity = new ContentEntity(getContentLength(), getStreamingContent());
       entity.setContentEncoding(getContentEncoding());
       entity.setContentType(getContentType());
+      if (getContentLength() == -1) {
+        entity.setChunked(true);
+      }
       ((HttpEntityEnclosingRequest) request).setEntity(entity);
     }
     request.setConfig(requestConfig.build());

diff --git a/...t-apache-v2/src/test/java/com/google/api/client/http/apache/v2/ApacheHttpRequestTest.java b/...t-apache-v2/src/test/java/com/google/api/client/http/apache/v2/ApacheHttpRequestTest.java
@@ -0,0 +1,59 @@
+/*
+ * Copyright 2019 Google LLC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
+ * in compliance with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package com.google.api.client.http.apache.v2;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import com.google.api.client.http.ByteArrayContent;
+import com.google.api.client.http.HttpContent;
+import com.google.api.client.http.InputStreamContent;
+import com.google.api.client.testing.http.apache.MockHttpClient;
+import java.io.ByteArrayInputStream;
+import java.nio.charset.StandardCharsets;
+import java.util.Arrays;
+import org.junit.Test;
+
+public class ApacheHttpRequestTest {
+
+  @Test
+  public void testContentLengthSet() throws Exception {
+    HttpExtensionMethod base = new HttpExtensionMethod("POST", "http://www.google.com");
+    ApacheHttpRequest request = new ApacheHttpRequest(new MockHttpClient(), base);
+    HttpContent content =
+        new ByteArrayContent("text/plain", "sample".getBytes(StandardCharsets.UTF_8));
+    request.setStreamingContent(content);
+    request.setContentLength(content.getLength());
+    request.execute();
+
+    assertFalse(base.getEntity().isChunked());
+    assertEquals(6, base.getEntity().getContentLength());
+  }
+
+  @Test
+  public void testChunked() throws Exception {
+    byte[] buf = new byte[300];
+    Arrays.fill(buf, (byte) ' ');
+    HttpExtensionMethod base = new HttpExtensionMethod("POST", "http://www.google.com");
+    ApacheHttpRequest request = new ApacheHttpRequest(new MockHttpClient(), base);
+    HttpContent content = new InputStreamContent("text/plain", new ByteArrayInputStream(buf));
+    request.setStreamingContent(content);
+    request.execute();
+
+    assertTrue(base.getEntity().isChunked());
+    assertEquals(-1, base.getEntity().getContentLength());
+  }
+}
diff --git a/...apache-v2/src/test/java/com/google/api/client/http/apache/v2/ApacheHttpTransportTest.java b/...apache-v2/src/test/java/com/google/api/client/http/apache/v2/ApacheHttpTransportTest.java
@@ -34,7 +34,6 @@
 import java.io.OutputStream;
 import java.net.InetSocketAddress;
 import java.nio.charset.StandardCharsets;
-import java.util.Random;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
 import org.apache.http.Header;
@@ -123,8 +122,8 @@ private void subtestUnsupportedRequestsWithContent(ApacheHttpRequest request, St
       throws IOException {
     try {
       execute(request);
-      fail("expected " + IllegalArgumentException.class);
-    } catch (IllegalArgumentException e) {
+      fail("expected " + IllegalStateException.class);
+    } catch (IllegalStateException e) {
       // expected
       assertEquals(e.getMessage(),
           "Apache HTTP client does not support " + method + " requests with content.");

diff --git a/google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java b/google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
@@ -15,7 +15,6 @@
 package com.google.api.client.http;
 
 import com.google.api.client.util.Beta;
-import com.google.api.client.util.IOUtils;
 import com.google.api.client.util.LoggingStreamingContent;
 import com.google.api.client.util.ObjectParser;
 import com.google.api.client.util.Preconditions;
@@ -141,7 +140,7 @@ public final class HttpRequest {
 
   /** HTTP request URL. */
   private GenericUrl url;
-  
+
   /** Timeout in milliseconds to establish a connection or {@code 0} for an infinite timeout. */
   private int connectTimeout = 20 * 1000;
 
@@ -172,7 +171,7 @@ public final class HttpRequest {
   /** The {@link BackOffPolicy} to use between retry attempts or {@code null} for none. */
   @Deprecated @Beta private BackOffPolicy backOffPolicy;
 
- /** Whether to automatically follow redirects ({@code true} by default). */
+  /** Whether to automatically follow redirects ({@code true} by default). */
   private boolean followRedirects = true;
 
   /** Whether to use raw redirect URLs ({@code false} by default). */
@@ -698,15 +697,13 @@ public HttpRequest setFollowRedirects(boolean followRedirects) {
     return this;
   }
 
-  /**
-   * Return whether to use raw redirect URLs. 
-   */
+  /** Return whether to use raw redirect URLs. */
   public boolean getUseRawRedirectUrls() {
     return useRawRedirectUrls;
   }
 
   /**
-   * Sets whether to use raw redirect URLs. 
+   * Sets whether to use raw redirect URLs.
    *
    * <p>The default value is {@code false}.
    */
@@ -938,7 +935,7 @@ public HttpResponse execute() throws IOException {
       final boolean contentRetrySupported = streamingContent == null || content.retrySupported();
       if (streamingContent != null) {
         final String contentEncoding;
-        final long contentLength;
+        long contentLength = -1;
         final String contentType = content.getType();
         // log content
         if (loggable) {
@@ -953,7 +950,6 @@ public HttpResponse execute() throws IOException {
         } else {
           contentEncoding = encoding.getName();
           streamingContent = new HttpEncodingStreamingContent(streamingContent, encoding);
-          contentLength = contentRetrySupported ? IOUtils.computeLength(streamingContent) : -1;
         }
         // append content headers to log buffer
         if (loggable) {
@@ -1222,7 +1218,7 @@ private static void addSpanAttribute(Span span, String key, String value) {
       span.putAttribute(key, AttributeValue.stringAttributeValue(value));
     }
   }
-  
+
   private static String getVersion() {
     // attempt to read the library's version from a properties file generated during the build
     // this value should be read and cached for later use

diff --git a/google-http-client/src/main/java/com/google/api/client/http/apache/ApacheHttpRequest.java b/google-http-client/src/main/java/com/google/api/client/http/apache/ApacheHttpRequest.java
@@ -25,9 +25,7 @@
 import org.apache.http.params.HttpConnectionParams;
 import org.apache.http.params.HttpParams;
 
-/**
- * @author Yaniv Inbar
- */
+/** @author Yaniv Inbar */
 final class ApacheHttpRequest extends LowLevelHttpRequest {
   private final HttpClient httpClient;
 
@@ -54,12 +52,16 @@ public void setTimeout(int connectTimeout, int readTimeout) throws IOException {
   @Override
   public LowLevelHttpResponse execute() throws IOException {
     if (getStreamingContent() != null) {
-      Preconditions.checkArgument(request instanceof HttpEntityEnclosingRequest,
+      Preconditions.checkState(
+          request instanceof HttpEntityEnclosingRequest,
           "Apache HTTP client does not support %s requests with content.",
           request.getRequestLine().getMethod());
       ContentEntity entity = new ContentEntity(getContentLength(), getStreamingContent());
       entity.setContentEncoding(getContentEncoding());
       entity.setContentType(getContentType());
+      if (getContentLength() == -1) {
+        entity.setChunked(true);
+      }
       ((HttpEntityEnclosingRequest) request).setEntity(entity);
     }
     return new ApacheHttpResponse(request, httpClient.execute(request));

diff --git a/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java b/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpRequest.java
@@ -49,6 +49,11 @@ public void addHeader(String name, String value) {
     connection.addRequestProperty(name, value);
   }
 
+  @VisibleForTesting
+  String getRequestProperty(String name) {
+    return connection.getRequestProperty(name);
+  }
+
   @Override
   public void setTimeout(int connectTimeout, int readTimeout) {
     connection.setReadTimeout(readTimeout);

diff --git a/...lient/src/main/java/com/google/api/client/testing/http/javanet/MockHttpURLConnection.java b/...lient/src/main/java/com/google/api/client/testing/http/javanet/MockHttpURLConnection.java
@@ -201,4 +201,8 @@ public String getHeaderField(String name) {
     List<String> values = headers.get(name);
     return values == null ? null : values.get(0);
   }
+
+  public int getChunkLength() {
+    return chunkLength;
+  }
 }
diff --git a/google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java b/google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java
@@ -987,7 +987,7 @@ public LowLevelHttpResponse execute() throws IOException {
             if (expectGZip) {
               assertEquals(HttpEncodingStreamingContent.class, getStreamingContent().getClass());
               assertEquals("gzip", getContentEncoding());
-              assertEquals(25, getContentLength());
+              assertEquals(-1, getContentLength());
             } else {
               assertFalse(
                   getStreamingContent().getClass().equals(HttpEncodingStreamingContent.class));
@@ -1227,7 +1227,9 @@ public void testExecute_curlLogger() throws Exception {
       if (message.startsWith("curl")) {
         found = true;
         assertTrue(message.contains("curl -v --compressed -H 'Accept-Encoding: gzip'"));
-        assertTrue(message.contains("-H 'User-Agent: Google-HTTP-Java-Client/" + HttpRequest.VERSION + " (gzip)'"));
+        assertTrue(
+            message.contains(
+                "-H 'User-Agent: Google-HTTP-Java-Client/" + HttpRequest.VERSION + " (gzip)'"));
         assertTrue(message.contains("' -- 'http://google.com/#q=a'\"'\"'b'\"'\"'c'"));
       }
     }
@@ -1258,7 +1260,9 @@ public void testExecute_curlLoggerWithContentEncoding() throws Exception {
         found = true;
         assertTrue(message.contains("curl -v --compressed -X POST -H 'Accept-Encoding: gzip'"));
         assertTrue(message.contains("-H 'User-Agent: " + HttpRequest.USER_AGENT_SUFFIX + "'"));
-        assertTrue(message.contains("-H 'Content-Type: text/plain; charset=UTF-8' -H 'Content-Encoding: gzip'"));
+        assertTrue(
+            message.contains(
+                "-H 'Content-Type: text/plain; charset=UTF-8' -H 'Content-Encoding: gzip'"));
         assertTrue(message.contains("-d '@-' -- 'http://google.com/#q=a'\"'\"'b'\"'\"'c' << $$$"));
       }
     }

diff --git a/google-http-client/src/test/java/com/google/api/client/http/javanet/NetHttpRequestTest.java b/google-http-client/src/test/java/com/google/api/client/http/javanet/NetHttpRequestTest.java
@@ -1,9 +1,8 @@
 package com.google.api.client.http.javanet;
 
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
+import static org.junit.Assert.*;
 
+import com.google.api.client.http.ByteArrayContent;
 import com.google.api.client.http.HttpContent;
 import com.google.api.client.http.InputStreamContent;
 import com.google.api.client.http.LowLevelHttpResponse;
@@ -15,6 +14,7 @@
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.net.URL;
+import java.nio.charset.StandardCharsets;
 import java.util.concurrent.TimeoutException;
 import org.junit.Test;
 
@@ -203,4 +203,34 @@ public void close() throws IOException {
       assertEquals("Error during close", e.getMessage());
     }
   }
+
+  @Test
+  public void testChunkedLengthSet() throws Exception {
+    MockHttpURLConnection connection = new MockHttpURLConnection(new URL(HttpTesting.SIMPLE_URL));
+    connection.setRequestMethod("POST");
+    NetHttpRequest request = new NetHttpRequest(connection);
+    InputStream is = NetHttpRequestTest.class.getClassLoader().getResourceAsStream("file.txt");
+    HttpContent content = new InputStreamContent("text/plain", is);
+    request.setStreamingContent(content);
+    request.setContentEncoding("gzip");
+    request.execute();
+
+    assertEquals(4096, connection.getChunkLength());
+    assertNull(request.getRequestProperty("Content-Length"));
+  }
+
+  @Test
+  public void testChunkedLengthNotSet() throws Exception {
+    MockHttpURLConnection connection = new MockHttpURLConnection(new URL(HttpTesting.SIMPLE_URL));
+    connection.setRequestMethod("POST");
+    NetHttpRequest request = new NetHttpRequest(connection);
+    HttpContent content =
+        new ByteArrayContent("text/plain", "sample".getBytes(StandardCharsets.UTF_8));
+    request.setStreamingContent(content);
+    request.setContentLength(content.getLength());
+    request.execute();
+
+    assertEquals(connection.getChunkLength(), -1);
+    assertEquals("6", request.getRequestProperty("Content-Length"));
+  }
 }