diff --git a/core/src/main/java/feign/Logger.java b/core/src/main/java/feign/Logger.java index bc24c009..58ae0d1e 100644 --- a/core/src/main/java/feign/Logger.java +++ b/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); diff --git a/core/src/test/java/feign/client/DefaultClientTest.java b/core/src/test/java/feign/client/DefaultClientTest.java index 4a54174e..cf169705 100644 --- a/core/src/test/java/feign/client/DefaultClientTest.java +++ b/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=") diff --git a/httpclient/src/main/java/feign/httpclient/ApacheHttpClient.java b/httpclient/src/main/java/feign/httpclient/ApacheHttpClient.java index 27a8f2b5..36e3541a 100644 --- a/httpclient/src/main/java/feign/httpclient/ApacheHttpClient.java +++ b/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 * HttpClient. 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); } }; } - } diff --git a/httpclient/src/test/java/feign/httpclient/ApacheHttpClientTest.java b/httpclient/src/test/java/feign/httpclient/ApacheHttpClientTest.java index 96e734c6..e64d4f55 100644 --- a/httpclient/src/test/java/feign/httpclient/ApacheHttpClientTest.java +++ b/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=") diff --git a/okhttp/src/test/java/feign/okhttp/OkHttpClientTest.java b/okhttp/src/test/java/feign/okhttp/OkHttpClientTest.java index feeb2e0f..8a5bdb10 100644 --- a/okhttp/src/test/java/feign/okhttp/OkHttpClientTest.java +++ b/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=")