GH-934: Update Default Client to handle Compression correctly (#1349)

Fixes: #934, #1208

This change updates the Input Stream handling when using the Default
client implementation to detect when a response is `gzipped` and
wrap it in a `GZipInputStream`.

This addresses any issues related to compression when using the
default client.

In addition, removed the implicit parsing of the
body during toString.  This was also brought up in #1208 and it came up
during testing of this change.  Users should be using our `asReader`
and other methods to access the response body.

* Adding Deflate support
* Added Deflate Support and removed implicit response body reading
* Refactored Gzip and Deflate Conditions
* Corrected formatting and line-endings

core/src/main/java/feign/Client.java

@@ -19,9 +19,9 @@
 import static feign.Util.ENCODING_GZIP;
 import static feign.Util.checkArgument;
 import static feign.Util.checkNotNull;
-import static feign.Util.isBlank;
 import static feign.Util.isNotBlank;
 import static java.lang.String.format;
+import feign.Request.Options;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
@@ -35,11 +35,12 @@
 import java.util.List;
 import java.util.Map;
 import java.util.zip.DeflaterOutputStream;
+import java.util.zip.GZIPInputStream;
 import java.util.zip.GZIPOutputStream;
+import java.util.zip.InflaterInputStream;
 import javax.net.ssl.HostnameVerifier;
 import javax.net.ssl.HttpsURLConnection;
 import javax.net.ssl.SSLSocketFactory;
-import feign.Request.Options;
 
 /**
  * Submits HTTP {@link Request requests}. Implementations are expected to be thread-safe.
@@ -129,7 +130,13 @@ Response convertResponse(HttpURLConnection connection, Request request) throws I
       if (status >= 400) {
         stream = connection.getErrorStream();
       } else {
-        stream = connection.getInputStream();
+        if (this.isGzip(connection.getHeaderFields().get(CONTENT_ENCODING))) {
+          stream = new GZIPInputStream(connection.getInputStream());
+        } else if (this.isDeflate(connection.getHeaderFields().get(CONTENT_ENCODING))) {
+          stream = new InflaterInputStream(connection.getInputStream());
+        } else {
+          stream = connection.getInputStream();
+        }
       }
       return Response.builder()
           .status(status)
@@ -163,10 +170,8 @@ HttpURLConnection convertAndSend(Request request, Options options) throws IOExce
       connection.setRequestMethod(request.httpMethod().name());
 
       Collection<String> contentEncodingValues = request.headers().get(CONTENT_ENCODING);
-      boolean gzipEncodedRequest =
-          contentEncodingValues != null && contentEncodingValues.contains(ENCODING_GZIP);
-      boolean deflateEncodedRequest =
-          contentEncodingValues != null && contentEncodingValues.contains(ENCODING_DEFLATE);
+      boolean gzipEncodedRequest = this.isGzip(contentEncodingValues);
+      boolean deflateEncodedRequest = this.isDeflate(contentEncodingValues);
 
       boolean hasAcceptHeader = false;
       Integer contentLength = null;
@@ -216,6 +221,18 @@ HttpURLConnection convertAndSend(Request request, Options options) throws IOExce
       }
       return connection;
     }
+
+    private boolean isGzip(Collection<String> contentEncodingValues) {
+      return contentEncodingValues != null
+          && !contentEncodingValues.isEmpty()
+          && contentEncodingValues.contains(ENCODING_GZIP);
+    }
+
+    private boolean isDeflate(Collection<String> contentEncodingValues) {
+      return contentEncodingValues != null
+          && !contentEncodingValues.isEmpty()
+          && contentEncodingValues.contains(ENCODING_DEFLATE);
+    }
   }
 
   /**

core/src/main/java/feign/Response.java

@@ -284,14 +284,6 @@ public void close() throws IOException {
       inputStream.close();
     }
 
-    @Override
-    public String toString() {
-      try {
-        return new String(toByteArray(inputStream), UTF_8);
-      } catch (Exception e) {
-        return super.toString();
-      }
-    }
   }
 
   private static final class ByteArrayBody implements Response.Body {
@@ -347,10 +339,6 @@ public Reader asReader(Charset charset) throws IOException {
     @Override
     public void close() throws IOException {}
 
-    @Override
-    public String toString() {
-      return decodeOrDefault(data, UTF_8, "Binary data");
-    }
   }
 
   private static Map<String, Collection<String>> caseInsensitiveCopyOf(Map<String, Collection<String>> headers) {

core/src/test/java/feign/AsyncFeignTest.java

@@ -91,18 +91,6 @@ public void postTemplateParamsResolve() throws Exception {
         "{\"customer_name\": \"netflix\", \"user_name\": \"denominator\", \"password\": \"password\"}");
   }
 
-  @Test
-  public void responseCoercesToStringBody() throws Throwable {
-    server.enqueue(new MockResponse().setBody("foo"));
-
-    TestInterfaceAsync api =
-        new TestInterfaceAsyncBuilder().target("http://localhost:" + server.getPort());
-
-    Response response = unwrap(api.response());
-    assertTrue(response.body().isRepeatable());
-    assertEquals("foo", response.body().toString());
-  }
-
   @Test
   public void postFormParams() throws Exception {
     server.enqueue(new MockResponse().setBody("foo"));

core/src/test/java/feign/FeignTest.java

@@ -92,17 +92,6 @@ public void postTemplateParamsResolve() throws Exception {
             "{\"customer_name\": \"netflix\", \"user_name\": \"denominator\", \"password\": \"password\"}");
   }
 
-  @Test
-  public void responseCoercesToStringBody() {
-    server.enqueue(new MockResponse().setBody("foo"));
-
-    TestInterface api = new TestInterfaceBuilder().target("http://localhost:" + server.getPort());
-
-    Response response = api.response();
-    assertTrue(response.body().isRepeatable());
-    assertEquals("foo", response.body().toString());
-  }
-
   @Test
   public void postFormParams() throws Exception {
     server.enqueue(new MockResponse().setBody("foo"));

core/src/test/java/feign/FeignUnderAsyncTest.java

@@ -89,17 +89,6 @@ public void postTemplateParamsResolve() throws Exception {
             "{\"customer_name\": \"netflix\", \"user_name\": \"denominator\", \"password\": \"password\"}");
   }
 
-  @Test
-  public void responseCoercesToStringBody() {
-    server.enqueue(new MockResponse().setBody("foo"));
-
-    TestInterface api = new TestInterfaceBuilder().target("http://localhost:" + server.getPort());
-
-    Response response = api.response();
-    assertTrue(response.body().isRepeatable());
-    assertEquals("foo", response.body().toString());
-  }
-
   @Test
   public void postFormParams() throws Exception {
     server.enqueue(new MockResponse().setBody("foo"));

core/src/test/java/feign/ResponseTest.java

@@ -14,6 +14,7 @@
 package feign;
 
 import feign.Request.HttpMethod;
+import java.nio.charset.StandardCharsets;
 import org.assertj.core.util.Lists;
 import org.junit.Test;
 import java.util.Arrays;
@@ -38,7 +39,7 @@ public void reasonPhraseIsOptional() {
         .build();
 
     assertThat(response.reason()).isNull();
-    assertThat(response.toString()).isEqualTo("HTTP/1.1 200\n\n");
+    assertThat(response.toString()).startsWith("HTTP/1.1 200");
   }
 
   @Test

core/src/test/java/feign/client/DefaultClientTest.java

@@ -16,6 +16,8 @@
 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 java.io.IOException;
 import java.net.HttpURLConnection;
 import java.net.InetSocketAddress;
@@ -24,8 +26,12 @@
 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;
@@ -144,4 +150,60 @@ public void canCreateWithExplicitCredentials() throws Exception {
     assertThat(connection).isNotNull().isInstanceOf(HttpURLConnection.class);
   }
 
+
+  @Test
+  public void canSupportGzip() throws Exception {
+    /* enqueue a zipped response */
+    final String responseData = "Compressed Data";
+    server.enqueue(new MockResponse()
+        .addHeader("Content-Encoding", "gzip")
+        .setBody(new Buffer().write(compress(responseData))));
+
+    TestInterface api = newBuilder()
+        .target(TestInterface.class, "http://localhost:" + server.getPort());
+
+    String result = api.get();
+
+    /* verify that the response is unzipped */
+    assertThat(result).isNotNull()
+        .isEqualToIgnoringCase(responseData);
+
+  }
+
+  @Test
+  public void canSupportDeflate() throws Exception {
+    /* enqueue a zipped response */
+    final String responseData = "Compressed Data";
+    server.enqueue(new MockResponse()
+        .addHeader("Content-Encoding", "deflate")
+        .setBody(new Buffer().write(deflate(responseData))));
+
+    TestInterface api = newBuilder()
+        .target(TestInterface.class, "http://localhost:" + server.getPort());
+
+    String result = api.get();
+
+    /* verify that the response is unzipped */
+    assertThat(result).isNotNull()
+        .isEqualToIgnoringCase(responseData);
+
+  }
+
+  private byte[] compress(String data) throws Exception {
+    try (ByteArrayOutputStream bos = new ByteArrayOutputStream(data.length())) {
+      GZIPOutputStream gzipOutputStream = new GZIPOutputStream(bos);
+      gzipOutputStream.write(data.getBytes(StandardCharsets.UTF_8), 0, data.length());
+      gzipOutputStream.close();
+      return bos.toByteArray();
+    }
+  }
+
+  private byte[] deflate(String data) throws Exception {
+    try (ByteArrayOutputStream bos = new ByteArrayOutputStream(data.length())) {
+      DeflaterOutputStream deflaterOutputStream = new DeflaterOutputStream(bos);
+      deflaterOutputStream.write(data.getBytes(StandardCharsets.UTF_8), 0, data.length());
+      deflaterOutputStream.close();
+      return bos.toByteArray();
+    }
+  }
 }

hc5/src/test/java/feign/hc5/AsyncApacheHttp5ClientTest.java

@@ -76,18 +76,6 @@ public void postTemplateParamsResolve() throws Exception {
         "{\"customer_name\": \"netflix\", \"user_name\": \"denominator\", \"password\": \"password\"}");
   }
 
-  @Test
-  public void responseCoercesToStringBody() throws Throwable {
-    server.enqueue(new MockResponse().setBody("foo"));
-
-    final TestInterfaceAsync api =
-        new TestInterfaceAsyncBuilder().target("http://localhost:" + server.getPort());
-
-    final Response response = unwrap(api.response());
-    assertTrue(response.body().isRepeatable());
-    assertEquals("foo", response.body().toString());
-  }
-
   @Test
   public void postFormParams() throws Exception {
     server.enqueue(new MockResponse().setBody("foo"));

okhttp/src/test/java/feign/okhttp/OkHttpClientTest.java

@@ -22,6 +22,7 @@
 import feign.assertj.MockWebServerAssertions;
 import feign.client.AbstractClientTest;
 import feign.Feign;
+import java.nio.charset.StandardCharsets;
 import java.util.Collections;
 import java.util.concurrent.TimeUnit;
 import okhttp3.mockwebserver.MockResponse;
@@ -97,7 +98,8 @@ public void testFollowRedirect() throws Exception {
     Response response = api.get();
     // Response length should not be null
     assertEquals(200, response.status());
-    assertEquals(expectedBody, response.body().toString());
+    String payload = Util.toString(response.body().asReader(StandardCharsets.UTF_8));
+    assertEquals(expectedBody, payload);
 
   }
 

scripts/no-git-changes.sh

@@ -13,7 +13,6 @@
 # the License.
 #
 
-
 set -euo pipefail
 set -x
 
@@ -28,4 +27,3 @@ else
   echo "Please run 'mvn clean install' locally to format files"
   exit 1
 fi
-