Fixes NPE when apache client rebuffers content

When log level is full, the response body is rebuffered. The Apache
client had a bug where it allowed `toInputStream` to return null. This
fixes that bug and backfills tests for the other two clients.

Fixes #255

core/src/main/java/feign/Logger.java

@@ -143,11 +143,6 @@ public abstract class Logger {
    * logs to the category {@link Logger} at {@link java.util.logging.Level#FINE}.
    */
   public static class ErrorLogger extends Logger {
-
-    final java.util.logging.Logger
-        logger =
-        java.util.logging.Logger.getLogger(Logger.class.getName());
-
     @Override
     protected void log(String configKey, String format, Object... args) {
       System.err.printf(methodTag(configKey) + format + "%n", args);

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

@@ -34,6 +34,7 @@ import feign.Client;
 import feign.Feign;
 import feign.FeignException;
 import feign.Headers;
+import feign.Logger;
 import feign.RequestLine;
 import feign.Response;
 
@@ -151,6 +152,39 @@ public class DefaultClientTest {
     assertEquals(2, server.getRequestCount());
   }
 
+  @Test
+  public void safeRebuffering() throws IOException, InterruptedException {
+    server.enqueue(new MockResponse().setBody("foo"));
+
+    TestInterface api = Feign.builder()
+        .logger(new Logger(){
+          @Override
+          protected void log(String configKey, String format, Object... args) {
+          }
+        })
+        .logLevel(Logger.Level.FULL) // rebuffers the body
+        .target(TestInterface.class, "http://localhost:" + server.getPort());
+
+    api.post("foo");
+  }
+
+  /** This shows that is a no-op or otherwise doesn't cause an NPE when there's no content. */
+  @Test
+  public void safeRebuffering_noContent() throws IOException, InterruptedException {
+    server.enqueue(new MockResponse().setResponseCode(204));
+
+    TestInterface api = Feign.builder()
+        .logger(new Logger(){
+          @Override
+          protected void log(String configKey, String format, Object... args) {
+          }
+        })
+        .logLevel(Logger.Level.FULL) // rebuffers the body
+        .target(TestInterface.class, "http://localhost:" + server.getPort());
+
+    api.post("foo");
+  }
+
   interface TestInterface {
 
     @RequestLine("POST /?foo=bar&foo=baz&qux=")

httpclient/src/main/java/feign/httpclient/ApacheHttpClient.java

@@ -15,10 +15,6 @@
  */
 package feign.httpclient;
 
-import feign.Client;
-import feign.Request;
-import feign.Response;
-import feign.Util;
 import org.apache.http.Header;
 import org.apache.http.HttpEntity;
 import org.apache.http.HttpResponse;
@@ -35,7 +31,6 @@ import org.apache.http.entity.StringEntity;
 import org.apache.http.impl.client.HttpClientBuilder;
 import org.apache.http.util.EntityUtils;
 
-import java.io.ByteArrayInputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
@@ -50,6 +45,13 @@ import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 
+import feign.Client;
+import feign.Request;
+import feign.Response;
+import feign.Util;
+
+import static feign.Util.UTF_8;
+
 /**
  * This module directs Feign's http requests to Apache's
  * <a href="https://hc.apache.org/httpcomponents-client-ga/">HttpClient</a>. Ex.
@@ -165,43 +167,36 @@ public final class ApacheHttpClient implements Client {
   }
 
   Response.Body toFeignBody(HttpResponse httpResponse) throws IOException {
-    HttpEntity entity = httpResponse.getEntity();
-    final Integer length = entity != null && entity.getContentLength() != -1 ?
-            (int) entity.getContentLength() :
-            null;
-    final InputStream input = entity != null ?
-            new ByteArrayInputStream(EntityUtils.toByteArray(entity)) :
-            null;
-
+    final HttpEntity entity = httpResponse.getEntity();
+    if (entity == null) {
+      return null;
+    }
     return new Response.Body() {
 
-      @Override
-      public void close() throws IOException {
-        if (input != null) {
-          input.close();
-        }
-      }
-
       @Override
       public Integer length() {
-        return length;
+        return entity.getContentLength() < 0 ? (int) entity.getContentLength() : null;
       }
 
       @Override
       public boolean isRepeatable() {
-        return false;
+        return entity.isRepeatable();
       }
 
       @Override
       public InputStream asInputStream() throws IOException {
-        return input;
+        return entity.getContent();
       }
 
       @Override
       public Reader asReader() throws IOException {
-        return new InputStreamReader(input);
+        return new InputStreamReader(asInputStream(), UTF_8);
+      }
+
+      @Override
+      public void close() throws IOException {
+        EntityUtils.consume(entity);
       }
     };
   }
-
 }

httpclient/src/test/java/feign/httpclient/ApacheHttpClientTest.java

@@ -20,6 +20,7 @@ import com.squareup.okhttp.mockwebserver.rule.MockWebServerRule;
 import feign.Feign;
 import feign.FeignException;
 import feign.Headers;
+import feign.Logger;
 import feign.RequestLine;
 import feign.Response;
 import org.junit.Rule;
@@ -96,6 +97,41 @@ public class ApacheHttpClientTest {
         .hasMethod("PATCH");
   }
 
+  @Test
+  public void safeRebuffering() throws IOException, InterruptedException {
+    server.enqueue(new MockResponse().setBody("foo"));
+
+    TestInterface api = Feign.builder()
+        .client(new ApacheHttpClient())
+        .logger(new Logger(){
+          @Override
+          protected void log(String configKey, String format, Object... args) {
+          }
+        })
+        .logLevel(Logger.Level.FULL) // rebuffers the body
+        .target(TestInterface.class, "http://localhost:" + server.getPort());
+
+    api.post("foo");
+  }
+
+  /** This shows that is a no-op or otherwise doesn't cause an NPE when there's no content. */
+  @Test
+  public void safeRebuffering_noContent() throws IOException, InterruptedException {
+    server.enqueue(new MockResponse().setResponseCode(204));
+
+    TestInterface api = Feign.builder()
+        .client(new ApacheHttpClient())
+        .logger(new Logger(){
+          @Override
+          protected void log(String configKey, String format, Object... args) {
+          }
+        })
+        .logLevel(Logger.Level.FULL) // rebuffers the body
+        .target(TestInterface.class, "http://localhost:" + server.getPort());
+
+    api.post("foo");
+  }
+
   interface TestInterface {
 
     @RequestLine("POST /?foo=bar&foo=baz&qux=")

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

@@ -29,6 +29,7 @@ import java.io.IOException;
 import feign.Feign;
 import feign.FeignException;
 import feign.Headers;
+import feign.Logger;
 import feign.RequestLine;
 import feign.Response;
 
@@ -100,6 +101,41 @@ public class OkHttpClientTest {
         .hasMethod("PATCH");
   }
 
+  @Test
+  public void safeRebuffering() throws IOException, InterruptedException {
+    server.enqueue(new MockResponse().setBody("foo"));
+
+    TestInterface api = Feign.builder()
+        .client(new OkHttpClient())
+        .logger(new Logger(){
+          @Override
+          protected void log(String configKey, String format, Object... args) {
+          }
+        })
+        .logLevel(Logger.Level.FULL) // rebuffers the body
+        .target(TestInterface.class, "http://localhost:" + server.getPort());
+
+    api.post("foo");
+  }
+
+  /** This shows that is a no-op or otherwise doesn't cause an NPE when there's no content. */
+  @Test
+  public void safeRebuffering_noContent() throws IOException, InterruptedException {
+    server.enqueue(new MockResponse().setResponseCode(204));
+
+    TestInterface api = Feign.builder()
+        .client(new OkHttpClient())
+        .logger(new Logger(){
+          @Override
+          protected void log(String configKey, String format, Object... args) {
+          }
+        })
+        .logLevel(Logger.Level.FULL) // rebuffers the body
+        .target(TestInterface.class, "http://localhost:" + server.getPort());
+
+    api.post("foo");
+  }
+
   interface TestInterface {
 
     @RequestLine("POST /?foo=bar&foo=baz&qux=")