Skip to content

Commit 511e884

Browse files
author
Adrian Cole
committed
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 OpenFeign#255
1 parent abe7e12 commit 511e884

5 files changed

Lines changed: 126 additions & 30 deletions

File tree

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

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -143,11 +143,6 @@ public enum Level {
143143
* logs to the category {@link Logger} at {@link java.util.logging.Level#FINE}.
144144
*/
145145
public static class ErrorLogger extends Logger {
146-
147-
final java.util.logging.Logger
148-
logger =
149-
java.util.logging.Logger.getLogger(Logger.class.getName());
150-
151146
@Override
152147
protected void log(String configKey, String format, Object... args) {
153148
System.err.printf(methodTag(configKey) + format + "%n", args);

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

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
import feign.Feign;
3535
import feign.FeignException;
3636
import feign.Headers;
37+
import feign.Logger;
3738
import feign.RequestLine;
3839
import feign.Response;
3940

@@ -151,6 +152,39 @@ public void retriesFailedHandshake() throws IOException, InterruptedException {
151152
assertEquals(2, server.getRequestCount());
152153
}
153154

155+
@Test
156+
public void safeRebuffering() throws IOException, InterruptedException {
157+
server.enqueue(new MockResponse().setBody("foo"));
158+
159+
TestInterface api = Feign.builder()
160+
.logger(new Logger(){
161+
@Override
162+
protected void log(String configKey, String format, Object... args) {
163+
}
164+
})
165+
.logLevel(Logger.Level.FULL) // rebuffers the body
166+
.target(TestInterface.class, "http://localhost:" + server.getPort());
167+
168+
api.post("foo");
169+
}
170+
171+
/** This shows that is a no-op or otherwise doesn't cause an NPE when there's no content. */
172+
@Test
173+
public void safeRebuffering_noContent() throws IOException, InterruptedException {
174+
server.enqueue(new MockResponse().setResponseCode(204));
175+
176+
TestInterface api = Feign.builder()
177+
.logger(new Logger(){
178+
@Override
179+
protected void log(String configKey, String format, Object... args) {
180+
}
181+
})
182+
.logLevel(Logger.Level.FULL) // rebuffers the body
183+
.target(TestInterface.class, "http://localhost:" + server.getPort());
184+
185+
api.post("foo");
186+
}
187+
154188
interface TestInterface {
155189

156190
@RequestLine("POST /?foo=bar&foo=baz&qux=")

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

Lines changed: 20 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,6 @@
1515
*/
1616
package feign.httpclient;
1717

18-
import feign.Client;
19-
import feign.Request;
20-
import feign.Response;
21-
import feign.Util;
2218
import org.apache.http.Header;
2319
import org.apache.http.HttpEntity;
2420
import org.apache.http.HttpResponse;
@@ -35,7 +31,6 @@
3531
import org.apache.http.impl.client.HttpClientBuilder;
3632
import org.apache.http.util.EntityUtils;
3733

38-
import java.io.ByteArrayInputStream;
3934
import java.io.IOException;
4035
import java.io.InputStream;
4136
import java.io.InputStreamReader;
@@ -50,6 +45,13 @@
5045
import java.util.List;
5146
import java.util.Map;
5247

48+
import feign.Client;
49+
import feign.Request;
50+
import feign.Response;
51+
import feign.Util;
52+
53+
import static feign.Util.UTF_8;
54+
5355
/**
5456
* This module directs Feign's http requests to Apache's
5557
* <a href="https://hc.apache.org/httpcomponents-client-ga/">HttpClient</a>. Ex.
@@ -165,43 +167,36 @@ Response toFeignResponse(HttpResponse httpResponse) throws IOException {
165167
}
166168

167169
Response.Body toFeignBody(HttpResponse httpResponse) throws IOException {
168-
HttpEntity entity = httpResponse.getEntity();
169-
final Integer length = entity != null && entity.getContentLength() != -1 ?
170-
(int) entity.getContentLength() :
171-
null;
172-
final InputStream input = entity != null ?
173-
new ByteArrayInputStream(EntityUtils.toByteArray(entity)) :
174-
null;
175-
170+
final HttpEntity entity = httpResponse.getEntity();
171+
if (entity == null) {
172+
return null;
173+
}
176174
return new Response.Body() {
177175

178-
@Override
179-
public void close() throws IOException {
180-
if (input != null) {
181-
input.close();
182-
}
183-
}
184-
185176
@Override
186177
public Integer length() {
187-
return length;
178+
return entity.getContentLength() < 0 ? (int) entity.getContentLength() : null;
188179
}
189180

190181
@Override
191182
public boolean isRepeatable() {
192-
return false;
183+
return entity.isRepeatable();
193184
}
194185

195186
@Override
196187
public InputStream asInputStream() throws IOException {
197-
return input;
188+
return entity.getContent();
198189
}
199190

200191
@Override
201192
public Reader asReader() throws IOException {
202-
return new InputStreamReader(input);
193+
return new InputStreamReader(asInputStream(), UTF_8);
194+
}
195+
196+
@Override
197+
public void close() throws IOException {
198+
EntityUtils.consume(entity);
203199
}
204200
};
205201
}
206-
207202
}

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

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import feign.Feign;
2121
import feign.FeignException;
2222
import feign.Headers;
23+
import feign.Logger;
2324
import feign.RequestLine;
2425
import feign.Response;
2526
import org.junit.Rule;
@@ -96,6 +97,41 @@ public void patch() throws IOException, InterruptedException {
9697
.hasMethod("PATCH");
9798
}
9899

100+
@Test
101+
public void safeRebuffering() throws IOException, InterruptedException {
102+
server.enqueue(new MockResponse().setBody("foo"));
103+
104+
TestInterface api = Feign.builder()
105+
.client(new ApacheHttpClient())
106+
.logger(new Logger(){
107+
@Override
108+
protected void log(String configKey, String format, Object... args) {
109+
}
110+
})
111+
.logLevel(Logger.Level.FULL) // rebuffers the body
112+
.target(TestInterface.class, "http://localhost:" + server.getPort());
113+
114+
api.post("foo");
115+
}
116+
117+
/** This shows that is a no-op or otherwise doesn't cause an NPE when there's no content. */
118+
@Test
119+
public void safeRebuffering_noContent() throws IOException, InterruptedException {
120+
server.enqueue(new MockResponse().setResponseCode(204));
121+
122+
TestInterface api = Feign.builder()
123+
.client(new ApacheHttpClient())
124+
.logger(new Logger(){
125+
@Override
126+
protected void log(String configKey, String format, Object... args) {
127+
}
128+
})
129+
.logLevel(Logger.Level.FULL) // rebuffers the body
130+
.target(TestInterface.class, "http://localhost:" + server.getPort());
131+
132+
api.post("foo");
133+
}
134+
99135
interface TestInterface {
100136

101137
@RequestLine("POST /?foo=bar&foo=baz&qux=")

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

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import feign.Feign;
3030
import feign.FeignException;
3131
import feign.Headers;
32+
import feign.Logger;
3233
import feign.RequestLine;
3334
import feign.Response;
3435

@@ -100,6 +101,41 @@ public void patch() throws IOException, InterruptedException {
100101
.hasMethod("PATCH");
101102
}
102103

104+
@Test
105+
public void safeRebuffering() throws IOException, InterruptedException {
106+
server.enqueue(new MockResponse().setBody("foo"));
107+
108+
TestInterface api = Feign.builder()
109+
.client(new OkHttpClient())
110+
.logger(new Logger(){
111+
@Override
112+
protected void log(String configKey, String format, Object... args) {
113+
}
114+
})
115+
.logLevel(Logger.Level.FULL) // rebuffers the body
116+
.target(TestInterface.class, "http://localhost:" + server.getPort());
117+
118+
api.post("foo");
119+
}
120+
121+
/** This shows that is a no-op or otherwise doesn't cause an NPE when there's no content. */
122+
@Test
123+
public void safeRebuffering_noContent() throws IOException, InterruptedException {
124+
server.enqueue(new MockResponse().setResponseCode(204));
125+
126+
TestInterface api = Feign.builder()
127+
.client(new OkHttpClient())
128+
.logger(new Logger(){
129+
@Override
130+
protected void log(String configKey, String format, Object... args) {
131+
}
132+
})
133+
.logLevel(Logger.Level.FULL) // rebuffers the body
134+
.target(TestInterface.class, "http://localhost:" + server.getPort());
135+
136+
api.post("foo");
137+
}
138+
103139
interface TestInterface {
104140

105141
@RequestLine("POST /?foo=bar&foo=baz&qux=")

0 commit comments

Comments
 (0)