Skip to content

Commit 0225a77

Browse files
bclozelbclozel
committed
Revisit empty body response support in HTTP client
Prior to this commit, HTTP responses without body (response status 204 or 304, Content-Length: 0) were handled properly by RestTemplates. But some other cases were not properly managed, throwing exceptions for valid HTTP responses. This commit better handles HTTP responses, using a response wrapper that can tell if a response: * has no message body (HTTP status 1XX, 204, 304 or Content-Length:0) * has an empty message body This covers rfc7230 Section 3.3.3. Issue: SPR-8016
1 parent d57f7f8 commit 0225a77

File tree

5 files changed

+184
-70
lines changed

5 files changed

+184
-70
lines changed

‎spring-web/src/main/java/org/springframework/http/client/BufferingClientHttpResponseWrapper.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@
2525
import org.springframework.util.StreamUtils;
2626

2727
/**
28-
* Simple implementation of {@link ClientHttpResponse} that reads the request's body into memory,
28+
* Simple implementation of {@link ClientHttpResponse} that reads the response's body into memory,
2929
* thus allowing for multiple invocations of {@link #getBody()}.
3030
*
3131
* @author Arjen Poutsma

‎spring-web/src/main/java/org/springframework/web/client/HttpMessageConverterExtractor.java

Lines changed: 6 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@
2323
import org.apache.commons.logging.Log;
2424
import org.apache.commons.logging.LogFactory;
2525

26-
import org.springframework.http.HttpHeaders;
2726
import org.springframework.http.HttpStatus;
2827
import org.springframework.http.MediaType;
2928
import org.springframework.http.client.ClientHttpResponse;
@@ -80,10 +79,12 @@ public HttpMessageConverterExtractor(Type responseType, List<HttpMessageConverte
8079
@Override
8180
@SuppressWarnings({ "unchecked", "rawtypes" })
8281
public T extractData(ClientHttpResponse response) throws IOException {
83-
if (!hasMessageBody(response)) {
82+
83+
MessageBodyClientHttpResponseWrapper responseWrapper = new MessageBodyClientHttpResponseWrapper(response);
84+
if(!responseWrapper.hasMessageBody() || responseWrapper.hasEmptyMessageBody()) {
8485
return null;
8586
}
86-
MediaType contentType = getContentType(response);
87+
MediaType contentType = getContentType(responseWrapper);
8788

8889
for (HttpMessageConverter<?> messageConverter : this.messageConverters) {
8990
if (messageConverter instanceof GenericHttpMessageConverter) {
@@ -93,7 +94,7 @@ public T extractData(ClientHttpResponse response) throws IOException {
9394
logger.debug("Reading [" + this.responseType + "] as \"" +
9495
contentType + "\" using [" + messageConverter + "]");
9596
}
96-
return (T) genericMessageConverter.read(this.responseType, null, response);
97+
return (T) genericMessageConverter.read(this.responseType, null, responseWrapper);
9798
}
9899
}
99100
if (this.responseClass != null) {
@@ -102,7 +103,7 @@ public T extractData(ClientHttpResponse response) throws IOException {
102103
logger.debug("Reading [" + this.responseClass.getName() + "] as \"" +
103104
contentType + "\" using [" + messageConverter + "]");
104105
}
105-
return (T) messageConverter.read((Class) this.responseClass, response);
106+
return (T) messageConverter.read((Class) this.responseClass, responseWrapper);
106107
}
107108
}
108109
}
@@ -122,39 +123,4 @@ private MediaType getContentType(ClientHttpResponse response) {
122123
return contentType;
123124
}
124125

125-
/**
126-
* Indicates whether the given response has a message body.
127-
* <p>Default implementation returns {@code false} for:
128-
* <ul>
129-
* <li>a response status of {@code 204} or {@code 304}</li>
130-
* <li>a {@code Content-Length} of {@code 0}</li>
131-
* <li>no indication of content (no {@code Content-Length} nor {@code Transfer-encoding: chunked}) and
132-
* a ({@code Connection: closed}) header. See rfc7230 section 3.4</li>
133-
* </ul>
134-
*
135-
* @param response the response to check for a message body
136-
* @return {@code true} if the response has a body, {@code false} otherwise
137-
* @throws IOException in case of I/O errors
138-
*/
139-
protected boolean hasMessageBody(ClientHttpResponse response) throws IOException {
140-
HttpStatus responseStatus = response.getStatusCode();
141-
if (responseStatus == HttpStatus.NO_CONTENT ||
142-
responseStatus == HttpStatus.NOT_MODIFIED) {
143-
return false;
144-
}
145-
HttpHeaders headers = response.getHeaders();
146-
long contentLength = headers.getContentLength();
147-
if(contentLength == 0) {
148-
return false;
149-
}
150-
boolean chunked = headers.containsKey(HttpHeaders.TRANSFER_ENCODING)
151-
&& headers.get(HttpHeaders.TRANSFER_ENCODING).contains("chunked");
152-
boolean closed = headers.containsKey(HttpHeaders.CONNECTION)
153-
&& headers.getConnection().contains("close");
154-
if(!chunked && contentLength == -1 && closed) {
155-
return false;
156-
}
157-
return true;
158-
}
159-
160126
}

‎spring-web/src/main/java/org/springframework/web/client/MessageBodyClientHttpResponseWrapper.java

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
package org.springframework.web.client;
2+
3+
import java.io.IOException;
4+
import java.io.InputStream;
5+
import java.io.PushbackInputStream;
6+
7+
import org.springframework.http.HttpHeaders;
8+
import org.springframework.http.HttpStatus;
9+
import org.springframework.http.client.ClientHttpResponse;
10+
11+
/**
12+
* Implementation of {@link ClientHttpResponse} that can not only check if the response
13+
* has a message body, but also if its length is 0 (i.e. empty) by actually reading the input stream.
14+
*
15+
* @author Brian Clozel
16+
* @since 4.1
17+
* @see <a href="http://tools.ietf.org/html/rfc7230#section-3.3.3">rfc7230 Section 3.3.3</a>
18+
*/
19+
class MessageBodyClientHttpResponseWrapper implements ClientHttpResponse {
20+
21+
private PushbackInputStream pushbackInputStream;
22+
23+
private final ClientHttpResponse response;
24+
25+
public MessageBodyClientHttpResponseWrapper(ClientHttpResponse response) throws IOException {
26+
this.response = response;
27+
}
28+
29+
/**
30+
* Indicates whether the response has a message body.
31+
*
32+
* <p>Implementation returns {@code false} for:
33+
* <ul>
34+
* <li>a response status of {@code 1XX}, {@code 204} or {@code 304}</li>
35+
* <li>a {@code Content-Length} header of {@code 0}</li>
36+
* </ul>
37+
*
38+
* @return {@code true} if the response has a message body, {@code false} otherwise
39+
* @throws IOException in case of I/O errors
40+
*/
41+
public boolean hasMessageBody() throws IOException {
42+
HttpStatus responseStatus = this.getStatusCode();
43+
if (responseStatus.is1xxInformational() || responseStatus == HttpStatus.NO_CONTENT ||
44+
responseStatus == HttpStatus.NOT_MODIFIED) {
45+
return false;
46+
}
47+
else if(this.getHeaders().getContentLength() == 0) {
48+
return false;
49+
}
50+
return true;
51+
}
52+
53+
/**
54+
* Indicates whether the response has an empty message body.
55+
*
56+
* <p>Implementation tries to read the first bytes of the response stream:
57+
* <ul>
58+
* <li>if no bytes are available, the message body is empty</li>
59+
* <li>otherwise it is not empty and the stream is reset to its start for further reading</li>
60+
* </ul>
61+
*
62+
* @return {@code true} if the response has a zero-length message body, {@code false} otherwise
63+
* @throws IOException in case of I/O errors
64+
*/
65+
public boolean hasEmptyMessageBody() throws IOException {
66+
InputStream body = this.response.getBody();
67+
if (body == null) {
68+
return true;
69+
}
70+
else if (body.markSupported()) {
71+
body.mark(1);
72+
if (body.read() == -1) {
73+
return true;
74+
}
75+
else {
76+
body.reset();
77+
return false;
78+
}
79+
}
80+
else {
81+
this.pushbackInputStream = new PushbackInputStream(body);
82+
int b = pushbackInputStream.read();
83+
if (b == -1) {
84+
return true;
85+
}
86+
else {
87+
pushbackInputStream.unread(b);
88+
return false;
89+
}
90+
}
91+
}
92+
93+
@Override
94+
public HttpStatus getStatusCode() throws IOException {
95+
return response.getStatusCode();
96+
}
97+
98+
@Override
99+
public int getRawStatusCode() throws IOException {
100+
return response.getRawStatusCode();
101+
}
102+
103+
@Override
104+
public String getStatusText() throws IOException {
105+
return response.getStatusText();
106+
}
107+
108+
@Override
109+
public void close() {
110+
response.close();
111+
}
112+
113+
@Override
114+
public InputStream getBody() throws IOException {
115+
return this.pushbackInputStream != null ? this.pushbackInputStream : response.getBody();
116+
}
117+
118+
@Override
119+
public HttpHeaders getHeaders() {
120+
return response.getHeaders();
121+
}
122+
}

‎spring-web/src/test/java/org/springframework/web/client/HttpMessageConverterExtractorTests.java

Lines changed: 34 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
package org.springframework.web.client;
1818

19+
import java.io.ByteArrayInputStream;
1920
import java.io.IOException;
2021
import java.lang.reflect.Type;
2122
import java.util.ArrayList;
@@ -26,6 +27,7 @@
2627

2728
import org.springframework.core.ParameterizedTypeReference;
2829
import org.springframework.http.HttpHeaders;
30+
import org.springframework.http.HttpInputMessage;
2931
import org.springframework.http.HttpStatus;
3032
import org.springframework.http.MediaType;
3133
import org.springframework.http.client.ClientHttpResponse;
@@ -73,6 +75,17 @@ public void notModified() throws IOException {
7375
assertNull(result);
7476
}
7577

78+
@Test
79+
public void informational() throws IOException {
80+
HttpMessageConverter<?> converter = mock(HttpMessageConverter.class);
81+
extractor = new HttpMessageConverterExtractor<String>(String.class, createConverterList(converter));
82+
given(response.getStatusCode()).willReturn(HttpStatus.CONTINUE);
83+
84+
Object result = extractor.extractData(response);
85+
86+
assertNull(result);
87+
}
88+
7689
@Test
7790
public void zeroContentLength() throws IOException {
7891
HttpMessageConverter<?> converter = mock(HttpMessageConverter.class);
@@ -87,6 +100,22 @@ public void zeroContentLength() throws IOException {
87100
assertNull(result);
88101
}
89102

103+
@Test
104+
@SuppressWarnings("unchecked")
105+
public void emptyMessageBody() throws IOException {
106+
HttpMessageConverter<String> converter = mock(HttpMessageConverter.class);
107+
List<HttpMessageConverter<?>> converters = new ArrayList<HttpMessageConverter<?>>();
108+
converters.add(converter);
109+
HttpHeaders responseHeaders = new HttpHeaders();
110+
extractor = new HttpMessageConverterExtractor<String>(String.class, createConverterList(converter));
111+
given(response.getStatusCode()).willReturn(HttpStatus.OK);
112+
given(response.getHeaders()).willReturn(responseHeaders);
113+
given(response.getBody()).willReturn(new ByteArrayInputStream("".getBytes()));
114+
115+
Object result = extractor.extractData(response);
116+
assertNull(result);
117+
}
118+
90119
@Test
91120
@SuppressWarnings("unchecked")
92121
public void normal() throws IOException {
@@ -100,8 +129,9 @@ public void normal() throws IOException {
100129
extractor = new HttpMessageConverterExtractor<String>(String.class, converters);
101130
given(response.getStatusCode()).willReturn(HttpStatus.OK);
102131
given(response.getHeaders()).willReturn(responseHeaders);
132+
given(response.getBody()).willReturn(new ByteArrayInputStream(expected.getBytes()));
103133
given(converter.canRead(String.class, contentType)).willReturn(true);
104-
given(converter.read(String.class, response)).willReturn(expected);
134+
given(converter.read(eq(String.class), any(HttpInputMessage.class))).willReturn(expected);
105135

106136
Object result = extractor.extractData(response);
107137

@@ -120,28 +150,13 @@ public void cannotRead() throws IOException {
120150
extractor = new HttpMessageConverterExtractor<String>(String.class, converters);
121151
given(response.getStatusCode()).willReturn(HttpStatus.OK);
122152
given(response.getHeaders()).willReturn(responseHeaders);
153+
given(response.getBody()).willReturn(new ByteArrayInputStream("Foobar".getBytes()));
123154
given(converter.canRead(String.class, contentType)).willReturn(false);
124155

125156
extractor.extractData(response);
126157
}
127158

128159
@Test
129-
public void connectionClose() throws IOException {
130-
HttpMessageConverter<String> converter = mock(HttpMessageConverter.class);
131-
List<HttpMessageConverter<?>> converters = new ArrayList<HttpMessageConverter<?>>();
132-
converters.add(converter);
133-
HttpHeaders responseHeaders = new HttpHeaders();
134-
responseHeaders.setConnection("close");
135-
extractor = new HttpMessageConverterExtractor<String>(String.class, createConverterList(converter));
136-
given(response.getStatusCode()).willReturn(HttpStatus.OK);
137-
given(response.getHeaders()).willReturn(responseHeaders);
138-
139-
Object result = extractor.extractData(response);
140-
assertNull(result);
141-
}
142-
143-
@Test
144-
@SuppressWarnings("unchecked")
145160
public void generics() throws IOException {
146161
GenericHttpMessageConverter<String> converter = mock(GenericHttpMessageConverter.class);
147162
List<HttpMessageConverter<?>> converters = createConverterList(converter);
@@ -154,8 +169,9 @@ public void generics() throws IOException {
154169
extractor = new HttpMessageConverterExtractor<List<String>>(type, converters);
155170
given(response.getStatusCode()).willReturn(HttpStatus.OK);
156171
given(response.getHeaders()).willReturn(responseHeaders);
172+
given(response.getBody()).willReturn(new ByteArrayInputStream(expected.getBytes()));
157173
given(converter.canRead(type, null, contentType)).willReturn(true);
158-
given(converter.read(type, null, response)).willReturn(expected);
174+
given(converter.read(eq(type), eq(null), any(HttpInputMessage.class))).willReturn(expected);
159175

160176
Object result = extractor.extractData(response);
161177

0 commit comments

Comments
 (0)