From b8d6abe0367bd497b68831263753ad262914aa97 Mon Sep 17 00:00:00 2001 From: Cody Oss <6331106+codyoss@users.noreply.github.com> Date: Tue, 17 Dec 2019 10:20:24 -0700 Subject: [PATCH] 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 --- .../http/apache/v2/ApacheHttpRequest.java | 24 ++++---- .../http/apache/v2/ApacheHttpRequestTest.java | 59 +++++++++++++++++++ .../apache/v2/ApacheHttpTransportTest.java | 5 +- .../google/api/client/http/HttpRequest.java | 16 ++--- .../client/http/apache/ApacheHttpRequest.java | 10 ++-- .../client/http/javanet/NetHttpRequest.java | 5 ++ .../http/javanet/MockHttpURLConnection.java | 4 ++ .../api/client/http/HttpRequestTest.java | 10 +++- .../http/javanet/NetHttpRequestTest.java | 36 ++++++++++- 9 files changed, 135 insertions(+), 34 deletions(-) create mode 100644 google-http-client-apache-v2/src/test/java/com/google/api/client/http/apache/v2/ApacheHttpRequestTest.java diff --git a/google-http-client-apache-v2/src/main/java/com/google/api/client/http/apache/v2/ApacheHttpRequest.java b/google-http-client-apache-v2/src/main/java/com/google/api/client/http/apache/v2/ApacheHttpRequest.java index 29bd61ee7..ae2aac763 100644 --- a/google-http-client-apache-v2/src/main/java/com/google/api/client/http/apache/v2/ApacheHttpRequest.java +++ b/google-http-client-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/google-http-client-apache-v2/src/test/java/com/google/api/client/http/apache/v2/ApacheHttpRequestTest.java b/google-http-client-apache-v2/src/test/java/com/google/api/client/http/apache/v2/ApacheHttpRequestTest.java new file mode 100644 index 000000000..74c97244a --- /dev/null +++ b/google-http-client-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/google-http-client-apache-v2/src/test/java/com/google/api/client/http/apache/v2/ApacheHttpTransportTest.java b/google-http-client-apache-v2/src/test/java/com/google/api/client/http/apache/v2/ApacheHttpTransportTest.java index e6ca850ce..e9b93e9be 100644 --- a/google-http-client-apache-v2/src/test/java/com/google/api/client/http/apache/v2/ApacheHttpTransportTest.java +++ b/google-http-client-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 index 23ec3e168..9124e4906 100644 --- 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. * *

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 index b31b20594..355acfcd0 100644 --- 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 index aa0d8e3e4..1d043472b 100644 --- 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/google-http-client/src/main/java/com/google/api/client/testing/http/javanet/MockHttpURLConnection.java b/google-http-client/src/main/java/com/google/api/client/testing/http/javanet/MockHttpURLConnection.java index 467f495f4..2fba28b48 100644 --- a/google-http-client/src/main/java/com/google/api/client/testing/http/javanet/MockHttpURLConnection.java +++ b/google-http-client/src/main/java/com/google/api/client/testing/http/javanet/MockHttpURLConnection.java @@ -201,4 +201,8 @@ public String getHeaderField(String name) { List 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 index 154f3af16..bb47e98d7 100644 --- 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 index d4118328e..ae3606ca5 100644 --- 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")); + } }