Skip to content

Commit e310aae

Browse files
author
Adrian Cole
committed
Recognizes reason phrase is nullable and not set when using http/2
See httpwg/http2-spec#202 Fixes OpenFeign#382
1 parent 9bbd865 commit e310aae

File tree

8 files changed

+123
-13
lines changed

8 files changed

+123
-13
lines changed

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

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -152,10 +152,9 @@ Response convertResponse(HttpURLConnection connection) throws IOException {
152152
int status = connection.getResponseCode();
153153
String reason = connection.getResponseMessage();
154154

155-
if (status < 0 || reason == null) {
156-
// invalid response
157-
throw new IOException(format("Invalid HTTP executing %s %s", connection.getRequestMethod(),
158-
connection.getURL()));
155+
if (status < 0) {
156+
throw new IOException(format("Invalid status(%s) executing %s %s", status,
157+
connection.getRequestMethod(), connection.getURL()));
159158
}
160159

161160
Map<String, Collection<String>> headers = new LinkedHashMap<String, Collection<String>>();

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,9 @@ void logRetry(String configKey, Level logLevel) {
7777

7878
protected Response logAndRebufferResponse(String configKey, Level logLevel, Response response,
7979
long elapsedTime) throws IOException {
80-
log(configKey, "<--- HTTP/1.1 %s %s (%sms)", response.status(), response.reason(), elapsedTime);
80+
String reason = response.reason() != null && logLevel.compareTo(Level.NONE) > 0 ?
81+
" " + response.reason() : "";
82+
log(configKey, "<--- HTTP/1.1 %s%s (%sms)", response.status(), reason, elapsedTime);
8183
if (logLevel.ordinal() >= Level.HEADERS.ordinal()) {
8284

8385
for (String field : response.headers().keySet()) {

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

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -46,9 +46,8 @@ public final class Response implements Closeable {
4646
private Response(int status, String reason, Map<String, Collection<String>> headers, Body body) {
4747
checkState(status >= 200, "Invalid status code: %s", status);
4848
this.status = status;
49-
this.reason = checkNotNull(reason, "reason");
50-
LinkedHashMap<String, Collection<String>>
51-
copyOf =
49+
this.reason = reason; //nullable
50+
LinkedHashMap<String, Collection<String>> copyOf =
5251
new LinkedHashMap<String, Collection<String>>();
5352
copyOf.putAll(checkNotNull(headers, "headers"));
5453
this.headers = Collections.unmodifiableMap(copyOf);
@@ -84,6 +83,11 @@ public int status() {
8483
return status;
8584
}
8685

86+
/**
87+
* Nullable and not set when using http/2
88+
*
89+
* See https://github.com/http2/http2-spec/issues/202
90+
*/
8791
public String reason() {
8892
return reason;
8993
}
@@ -101,16 +105,15 @@ public Body body() {
101105

102106
@Override
103107
public String toString() {
104-
StringBuilder builder = new StringBuilder();
105-
builder.append("HTTP/1.1 ").append(status).append(' ').append(reason).append('\n');
108+
StringBuilder builder = new StringBuilder("HTTP/1.1 ").append(status);
109+
if (reason != null) builder.append(' ').append(reason);
110+
builder.append('\n');
106111
for (String field : headers.keySet()) {
107112
for (String value : valuesOrEmpty(headers, field)) {
108113
builder.append(field).append(": ").append(value).append('\n');
109114
}
110115
}
111-
if (body != null) {
112-
builder.append('\n').append(body);
113-
}
116+
if (body != null) builder.append('\n').append(body);
114117
return builder.toString();
115118
}
116119

‎core/src/test/java/feign/LoggerTest.java‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,38 @@ public void levelEmits() throws IOException, InterruptedException {
111111
}
112112
}
113113

114+
@RunWith(Parameterized.class)
115+
public static class ReasonPhraseOptional extends LoggerTest {
116+
117+
private final Level logLevel;
118+
119+
public ReasonPhraseOptional(Level logLevel, List<String> expectedMessages) {
120+
this.logLevel = logLevel;
121+
logger.expectMessages(expectedMessages);
122+
}
123+
124+
@Parameters
125+
public static Iterable<Object[]> data() {
126+
return Arrays.asList(new Object[][]{
127+
{Level.BASIC, Arrays.asList(
128+
"\\[SendsStuff#login\\] ---> POST http://localhost:[0-9]+/ HTTP/1.1",
129+
"\\[SendsStuff#login\\] <--- HTTP/1.1 200 \\([0-9]+ms\\)")},
130+
});
131+
}
132+
133+
@Test
134+
public void reasonPhraseOptional() throws IOException, InterruptedException {
135+
server.enqueue(new MockResponse().setStatus("HTTP/1.1 " + 200));
136+
137+
SendsStuff api = Feign.builder()
138+
.logger(logger)
139+
.logLevel(logLevel)
140+
.target(SendsStuff.class, "http://localhost:" + server.getUrl("").getPort());
141+
142+
api.login("netflix", "denominator", "password");
143+
}
144+
}
145+
114146
@RunWith(Parameterized.class)
115147
public static class ReadTimeoutEmitsTest extends LoggerTest {
116148

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

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
/*
2+
* Copyright 2013 Netflix, Inc.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package feign;
17+
18+
import org.junit.Test;
19+
20+
import java.util.Collection;
21+
import java.util.Collections;
22+
23+
import static feign.assertj.FeignAssertions.assertThat;
24+
25+
public class ResponseTest {
26+
27+
@Test
28+
public void reasonPhraseIsOptional() {
29+
Response response = Response.create(200, null /* reason phrase */, Collections.
30+
<String, Collection<String>>emptyMap(), new byte[0]);
31+
32+
assertThat(response.reason()).isNull();
33+
assertThat(response.toString()).isEqualTo("HTTP/1.1 200\n\n");
34+
}
35+
}

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,19 @@ public void parsesRequestAndResponse() throws IOException, InterruptedException
8383
.hasBody("foo");
8484
}
8585

86+
@Test
87+
public void reasonPhraseIsOptional() throws IOException, InterruptedException {
88+
server.enqueue(new MockResponse().setStatus("HTTP/1.1 " + 200));
89+
90+
TestInterface api =
91+
Feign.builder().target(TestInterface.class, "http://localhost:" + server.getPort());
92+
93+
Response response = api.post("foo");
94+
95+
assertThat(response.status()).isEqualTo(200);
96+
assertThat(response.reason()).isNull();
97+
}
98+
8699
@Test
87100
public void parsesErrorResponse() throws IOException, InterruptedException {
88101
thrown.expect(FeignException.class);

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,19 @@ public void parsesRequestAndResponse() throws IOException, InterruptedException
7171
.hasBody("foo");
7272
}
7373

74+
@Test
75+
public void reasonPhraseIsOptional() throws IOException, InterruptedException {
76+
server.enqueue(new MockResponse().setStatus("HTTP/1.1 " + 200));
77+
78+
TestInterface api =
79+
Feign.builder().target(TestInterface.class, "http://localhost:" + server.getPort());
80+
81+
Response response = api.post("foo");
82+
83+
assertThat(response.status()).isEqualTo(200);
84+
assertThat(response.reason()).isNull();
85+
}
86+
7487
@Test
7588
public void parsesResponseMissingLength() throws IOException, InterruptedException {
7689
server.enqueue(new MockResponse().setChunkedBody("foo", 1));

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,19 @@ public void parsesRequestAndResponse() throws IOException, InterruptedException
6969
.hasBody("foo");
7070
}
7171

72+
@Test
73+
public void reasonPhraseIsOptional() throws IOException, InterruptedException {
74+
server.enqueue(new MockResponse().setStatus("HTTP/1.1 " + 200));
75+
76+
TestInterface api =
77+
Feign.builder().target(TestInterface.class, "http://localhost:" + server.getPort());
78+
79+
Response response = api.post("foo");
80+
81+
assertThat(response.status()).isEqualTo(200);
82+
assertThat(response.reason()).isNull();
83+
}
84+
7285
@Test
7386
public void parsesErrorResponse() throws IOException, InterruptedException {
7487
thrown.expect(FeignException.class);

0 commit comments

Comments
 (0)