Skip to content

Commit aca9c4f

Browse files
Carter KozakAdrian Cole
Carter Kozak
authored and
Adrian Cole
committed
Returns null when Content-Length > Integer.MAX_VALUE (OpenFeign#410)
OkHttp client should not bail out when response body is >2gb Instead, we return null length and the response can be processed similarly to a chunked body. Also, shares common client tests in AbstractClientTest
1 parent 088df43 commit aca9c4f

8 files changed

Lines changed: 350 additions & 595 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,8 @@
1+
### Version 8.18
2+
* Content-Length response bodies with lengths greater than Integer.MAX_VALUE report null length
3+
* Previously the OkhttpClient would throw an exception, and ApacheHttpClient
4+
would report a wrong, possibly negative value
5+
16
### Version 8.17
27
* Adds support to RxJava Completable via `HystrixFeign` builder with fallback support
38
* Upgraded hystrix-core to 1.4.26

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -125,9 +125,10 @@ public void close() {
125125
public interface Body extends Closeable {
126126

127127
/**
128-
* length in bytes, if known. Null if not. <br> <br><br><b>Note</b><br> This is an integer as
129-
* most implementations cannot do bodies greater than 2GB. Moreover, the scope of this interface
130-
* doesn't include large bodies.
128+
* length in bytes, if known. Null if unknown or greater than {@link Integer#MAX_VALUE}.
129+
*
130+
* <br><br><br><b>Note</b><br> This is an integer as
131+
* most implementations cannot do bodies greater than 2GB.
131132
*/
132133
Integer length();
133134

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

Lines changed: 246 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,246 @@
1+
package feign.client;
2+
3+
import java.io.ByteArrayInputStream;
4+
import java.io.IOException;
5+
6+
import org.junit.Rule;
7+
import org.junit.Test;
8+
import org.junit.rules.ExpectedException;
9+
10+
import feign.Client;
11+
import feign.Feign.Builder;
12+
import feign.FeignException;
13+
import feign.Headers;
14+
import feign.Logger;
15+
import feign.Param;
16+
import feign.RequestLine;
17+
import feign.Response;
18+
import feign.assertj.MockWebServerAssertions;
19+
import okhttp3.mockwebserver.MockResponse;
20+
import okhttp3.mockwebserver.MockWebServer;
21+
22+
import static java.util.Arrays.asList;
23+
24+
import static org.assertj.core.api.Assertions.assertThat;
25+
import static org.junit.Assert.assertEquals;
26+
27+
import static feign.Util.UTF_8;
28+
29+
/**
30+
* {@link AbstractClientTest} can be extended to run a set of tests against any {@link Client} implementation.
31+
*/
32+
public abstract class AbstractClientTest {
33+
34+
@Rule
35+
public final ExpectedException thrown = ExpectedException.none();
36+
@Rule
37+
public final MockWebServer server = new MockWebServer();
38+
39+
/**
40+
* Create a Feign {@link Builder} with a client configured
41+
*/
42+
public abstract Builder newBuilder();
43+
44+
/**
45+
* Some client implementation tests should override this
46+
* test if the PATCH operation is unsupported.
47+
*/
48+
@Test
49+
public void testPatch() throws Exception {
50+
server.enqueue(new MockResponse().setBody("foo"));
51+
server.enqueue(new MockResponse());
52+
53+
TestInterface api = newBuilder()
54+
.target(TestInterface.class, "http://localhost:" + server.getPort());
55+
56+
assertEquals("foo", api.patch(""));
57+
58+
MockWebServerAssertions.assertThat(server.takeRequest())
59+
.hasHeaders("Accept: text/plain", "Content-Length: 0") // Note: OkHttp adds content length.
60+
.hasNoHeaderNamed("Content-Type")
61+
.hasMethod("PATCH");
62+
}
63+
64+
@Test
65+
public void parsesRequestAndResponse() throws IOException, InterruptedException {
66+
server.enqueue(new MockResponse().setBody("foo").addHeader("Foo: Bar"));
67+
68+
TestInterface api = newBuilder()
69+
.target(TestInterface.class, "http://localhost:" + server.getPort());
70+
71+
Response response = api.post("foo");
72+
73+
assertThat(response.status()).isEqualTo(200);
74+
assertThat(response.reason()).isEqualTo("OK");
75+
assertThat(response.headers())
76+
.containsEntry("Content-Length", asList("3"))
77+
.containsEntry("Foo", asList("Bar"));
78+
assertThat(response.body().asInputStream())
79+
.hasContentEqualTo(new ByteArrayInputStream("foo".getBytes(UTF_8)));
80+
81+
MockWebServerAssertions.assertThat(server.takeRequest()).hasMethod("POST")
82+
.hasPath("/?foo=bar&foo=baz&qux=")
83+
.hasHeaders("Foo: Bar", "Foo: Baz", "Qux: ", "Accept: */*", "Content-Length: 3")
84+
.hasBody("foo");
85+
}
86+
87+
@Test
88+
public void reasonPhraseIsOptional() throws IOException, InterruptedException {
89+
server.enqueue(new MockResponse().setStatus("HTTP/1.1 " + 200));
90+
91+
TestInterface api = newBuilder()
92+
.target(TestInterface.class, "http://localhost:" + server.getPort());
93+
94+
Response response = api.post("foo");
95+
96+
assertThat(response.status()).isEqualTo(200);
97+
assertThat(response.reason()).isNullOrEmpty();
98+
}
99+
100+
@Test
101+
public void parsesErrorResponse() throws IOException, InterruptedException {
102+
thrown.expect(FeignException.class);
103+
thrown.expectMessage("status 500 reading TestInterface#get(); content:\n" + "ARGHH");
104+
105+
server.enqueue(new MockResponse().setResponseCode(500).setBody("ARGHH"));
106+
107+
TestInterface api = newBuilder()
108+
.target(TestInterface.class, "http://localhost:" + server.getPort());
109+
110+
api.get();
111+
}
112+
113+
@Test
114+
public void safeRebuffering() throws IOException, InterruptedException {
115+
server.enqueue(new MockResponse().setBody("foo"));
116+
117+
TestInterface api = newBuilder()
118+
.logger(new Logger(){
119+
@Override
120+
protected void log(String configKey, String format, Object... args) {
121+
}
122+
})
123+
.logLevel(Logger.Level.FULL) // rebuffers the body
124+
.target(TestInterface.class, "http://localhost:" + server.getPort());
125+
126+
api.post("foo");
127+
}
128+
129+
/** This shows that is a no-op or otherwise doesn't cause an NPE when there's no content. */
130+
@Test
131+
public void safeRebuffering_noContent() throws IOException, InterruptedException {
132+
server.enqueue(new MockResponse().setResponseCode(204));
133+
134+
TestInterface api = newBuilder()
135+
.logger(new Logger(){
136+
@Override
137+
protected void log(String configKey, String format, Object... args) {
138+
}
139+
})
140+
.logLevel(Logger.Level.FULL) // rebuffers the body
141+
.target(TestInterface.class, "http://localhost:" + server.getPort());
142+
143+
api.post("foo");
144+
}
145+
146+
@Test
147+
public void noResponseBodyForPost() {
148+
server.enqueue(new MockResponse());
149+
150+
TestInterface api = newBuilder()
151+
.target(TestInterface.class, "http://localhost:" + server.getPort());
152+
153+
api.noPostBody();
154+
}
155+
156+
@Test
157+
public void noResponseBodyForPut() {
158+
server.enqueue(new MockResponse());
159+
160+
TestInterface api = newBuilder()
161+
.target(TestInterface.class, "http://localhost:" + server.getPort());
162+
163+
api.noPutBody();
164+
}
165+
166+
@Test
167+
public void parsesResponseMissingLength() throws IOException, InterruptedException {
168+
server.enqueue(new MockResponse().setChunkedBody("foo", 1));
169+
170+
TestInterface api = newBuilder()
171+
.target(TestInterface.class, "http://localhost:" + server.getPort());
172+
173+
Response response = api.post("testing");
174+
assertThat(response.status()).isEqualTo(200);
175+
assertThat(response.reason()).isEqualTo("OK");
176+
assertThat(response.body().length()).isNull();
177+
assertThat(response.body().asInputStream())
178+
.hasContentEqualTo(new ByteArrayInputStream("foo".getBytes(UTF_8)));
179+
}
180+
181+
@Test
182+
public void postWithSpacesInPath() throws IOException, InterruptedException {
183+
server.enqueue(new MockResponse().setBody("foo"));
184+
185+
TestInterface api = newBuilder()
186+
.target(TestInterface.class, "http://localhost:" + server.getPort());
187+
188+
Response response = api.post("current documents", "foo");
189+
190+
MockWebServerAssertions.assertThat(server.takeRequest()).hasMethod("POST")
191+
.hasPath("/path/current%20documents/resource")
192+
.hasBody("foo");
193+
}
194+
195+
@Test
196+
public void testVeryLongResponseNullLength() throws Exception {
197+
server.enqueue(new MockResponse()
198+
.setBody("AAAAAAAA")
199+
.addHeader("Content-Length", Long.MAX_VALUE));
200+
TestInterface api = newBuilder()
201+
.target(TestInterface.class, "http://localhost:" + server.getPort());
202+
203+
Response response = api.post("foo");
204+
// Response length greater than Integer.MAX_VALUE should be null
205+
assertThat(response.body().length()).isNull();
206+
}
207+
208+
@Test
209+
public void testResponseLength() throws Exception {
210+
server.enqueue(new MockResponse()
211+
.setBody("test"));
212+
TestInterface api = newBuilder()
213+
.target(TestInterface.class, "http://localhost:" + server.getPort());
214+
215+
Integer expected = 4;
216+
Response response = api.post("");
217+
Integer actual = response.body().length();
218+
assertEquals(expected, actual);
219+
}
220+
221+
public interface TestInterface {
222+
223+
@RequestLine("POST /?foo=bar&foo=baz&qux=")
224+
@Headers({"Foo: Bar", "Foo: Baz", "Qux: ", "Content-Type: text/plain"})
225+
Response post(String body);
226+
227+
@RequestLine("POST /path/{to}/resource")
228+
@Headers("Accept: text/plain")
229+
Response post(@Param("to") String to, String body);
230+
231+
@RequestLine("GET /")
232+
@Headers("Accept: text/plain")
233+
String get();
234+
235+
@RequestLine("PATCH /")
236+
@Headers("Accept: text/plain")
237+
String patch(String body);
238+
239+
@RequestLine("POST")
240+
String noPostBody();
241+
242+
@RequestLine("PUT")
243+
String noPutBody();
244+
}
245+
246+
}

0 commit comments

Comments
 (0)