Skip to content

Commit

Permalink
Revisit empty body response support in HTTP client
Browse files Browse the repository at this point in the history
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
  • Loading branch information
bclozel committed Jan 8, 2015
1 parent 213a3fd commit b6675b6
Show file tree
Hide file tree
Showing 5 changed files with 184 additions and 70 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
import org.springframework.util.StreamUtils;

/**
* Simple implementation of {@link ClientHttpResponse} that reads the request's body into memory,
* Simple implementation of {@link ClientHttpResponse} that reads the response's body into memory,
* thus allowing for multiple invocations of {@link #getBody()}.
*
* @author Arjen Poutsma
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;

import org.springframework.http.HttpHeaders;
import org.springframework.http.HttpStatus;
import org.springframework.http.MediaType;
import org.springframework.http.client.ClientHttpResponse;
Expand Down Expand Up @@ -80,10 +79,12 @@ public HttpMessageConverterExtractor(Type responseType, List<HttpMessageConverte
@Override
@SuppressWarnings({ "unchecked", "rawtypes" })
public T extractData(ClientHttpResponse response) throws IOException {
if (!hasMessageBody(response)) {

MessageBodyClientHttpResponseWrapper responseWrapper = new MessageBodyClientHttpResponseWrapper(response);
if(!responseWrapper.hasMessageBody() || responseWrapper.hasEmptyMessageBody()) {
return null;
}
MediaType contentType = getContentType(response);
MediaType contentType = getContentType(responseWrapper);

for (HttpMessageConverter<?> messageConverter : this.messageConverters) {
if (messageConverter instanceof GenericHttpMessageConverter) {
Expand All @@ -93,7 +94,7 @@ public T extractData(ClientHttpResponse response) throws IOException {
logger.debug("Reading [" + this.responseType + "] as \"" +
contentType + "\" using [" + messageConverter + "]");
}
return (T) genericMessageConverter.read(this.responseType, null, response);
return (T) genericMessageConverter.read(this.responseType, null, responseWrapper);
}
}
if (this.responseClass != null) {
Expand All @@ -102,7 +103,7 @@ public T extractData(ClientHttpResponse response) throws IOException {
logger.debug("Reading [" + this.responseClass.getName() + "] as \"" +
contentType + "\" using [" + messageConverter + "]");
}
return (T) messageConverter.read((Class) this.responseClass, response);
return (T) messageConverter.read((Class) this.responseClass, responseWrapper);
}
}
}
Expand All @@ -122,39 +123,4 @@ private MediaType getContentType(ClientHttpResponse response) {
return contentType;
}

/**
* Indicates whether the given response has a message body.
* <p>Default implementation returns {@code false} for:
* <ul>
* <li>a response status of {@code 204} or {@code 304}</li>
* <li>a {@code Content-Length} of {@code 0}</li>
* <li>no indication of content (no {@code Content-Length} nor {@code Transfer-encoding: chunked}) and
* a ({@code Connection: closed}) header. See rfc7230 section 3.4</li>
* </ul>
*
* @param response the response to check for a message body
* @return {@code true} if the response has a body, {@code false} otherwise
* @throws IOException in case of I/O errors
*/
protected boolean hasMessageBody(ClientHttpResponse response) throws IOException {
HttpStatus responseStatus = response.getStatusCode();
if (responseStatus == HttpStatus.NO_CONTENT ||
responseStatus == HttpStatus.NOT_MODIFIED) {
return false;
}
HttpHeaders headers = response.getHeaders();
long contentLength = headers.getContentLength();
if(contentLength == 0) {
return false;
}
boolean chunked = headers.containsKey(HttpHeaders.TRANSFER_ENCODING)
&& headers.get(HttpHeaders.TRANSFER_ENCODING).contains("chunked");
boolean closed = headers.containsKey(HttpHeaders.CONNECTION)
&& headers.getConnection().contains("close");
if(!chunked && contentLength == -1 && closed) {
return false;
}
return true;
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
package org.springframework.web.client;

import java.io.IOException;
import java.io.InputStream;
import java.io.PushbackInputStream;

import org.springframework.http.HttpHeaders;
import org.springframework.http.HttpStatus;
import org.springframework.http.client.ClientHttpResponse;

/**
* Implementation of {@link ClientHttpResponse} that can not only check if the response
* has a message body, but also if its length is 0 (i.e. empty) by actually reading the input stream.
*
* @author Brian Clozel
* @since 4.1
* @see <a href="http://tools.ietf.org/html/rfc7230#section-3.3.3">rfc7230 Section 3.3.3</a>
*/
class MessageBodyClientHttpResponseWrapper implements ClientHttpResponse {

private PushbackInputStream pushbackInputStream;

private final ClientHttpResponse response;

public MessageBodyClientHttpResponseWrapper(ClientHttpResponse response) throws IOException {
this.response = response;
}

/**
* Indicates whether the response has a message body.
*
* <p>Implementation returns {@code false} for:
* <ul>
* <li>a response status of {@code 1XX}, {@code 204} or {@code 304}</li>
* <li>a {@code Content-Length} header of {@code 0}</li>
* </ul>
*
* @return {@code true} if the response has a message body, {@code false} otherwise
* @throws IOException in case of I/O errors
*/
public boolean hasMessageBody() throws IOException {
HttpStatus responseStatus = this.getStatusCode();
if (responseStatus.is1xxInformational() || responseStatus == HttpStatus.NO_CONTENT ||
responseStatus == HttpStatus.NOT_MODIFIED) {
return false;
}
else if(this.getHeaders().getContentLength() == 0) {
return false;
}
return true;
}

/**
* Indicates whether the response has an empty message body.
*
* <p>Implementation tries to read the first bytes of the response stream:
* <ul>
* <li>if no bytes are available, the message body is empty</li>
* <li>otherwise it is not empty and the stream is reset to its start for further reading</li>
* </ul>
*
* @return {@code true} if the response has a zero-length message body, {@code false} otherwise
* @throws IOException in case of I/O errors
*/
public boolean hasEmptyMessageBody() throws IOException {
InputStream body = this.response.getBody();
if (body == null) {
return true;
}
else if (body.markSupported()) {
body.mark(1);
if (body.read() == -1) {
return true;
}
else {
body.reset();
return false;
}
}
else {
this.pushbackInputStream = new PushbackInputStream(body);
int b = pushbackInputStream.read();
if (b == -1) {
return true;
}
else {
pushbackInputStream.unread(b);
return false;
}
}
}

@Override
public HttpStatus getStatusCode() throws IOException {
return response.getStatusCode();
}

@Override
public int getRawStatusCode() throws IOException {
return response.getRawStatusCode();
}

@Override
public String getStatusText() throws IOException {
return response.getStatusText();
}

@Override
public void close() {
response.close();
}

@Override
public InputStream getBody() throws IOException {
return this.pushbackInputStream != null ? this.pushbackInputStream : response.getBody();
}

@Override
public HttpHeaders getHeaders() {
return response.getHeaders();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package org.springframework.web.client;

import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.lang.reflect.Type;
import java.util.ArrayList;
Expand All @@ -26,6 +27,7 @@

import org.springframework.core.ParameterizedTypeReference;
import org.springframework.http.HttpHeaders;
import org.springframework.http.HttpInputMessage;
import org.springframework.http.HttpStatus;
import org.springframework.http.MediaType;
import org.springframework.http.client.ClientHttpResponse;
Expand Down Expand Up @@ -73,6 +75,17 @@ public void notModified() throws IOException {
assertNull(result);
}

@Test
public void informational() throws IOException {
HttpMessageConverter<?> converter = mock(HttpMessageConverter.class);
extractor = new HttpMessageConverterExtractor<String>(String.class, createConverterList(converter));
given(response.getStatusCode()).willReturn(HttpStatus.CONTINUE);

Object result = extractor.extractData(response);

assertNull(result);
}

@Test
public void zeroContentLength() throws IOException {
HttpMessageConverter<?> converter = mock(HttpMessageConverter.class);
Expand All @@ -87,6 +100,22 @@ public void zeroContentLength() throws IOException {
assertNull(result);
}

@Test
@SuppressWarnings("unchecked")
public void emptyMessageBody() throws IOException {
HttpMessageConverter<String> converter = mock(HttpMessageConverter.class);
List<HttpMessageConverter<?>> converters = new ArrayList<HttpMessageConverter<?>>();
converters.add(converter);
HttpHeaders responseHeaders = new HttpHeaders();
extractor = new HttpMessageConverterExtractor<String>(String.class, createConverterList(converter));
given(response.getStatusCode()).willReturn(HttpStatus.OK);
given(response.getHeaders()).willReturn(responseHeaders);
given(response.getBody()).willReturn(new ByteArrayInputStream("".getBytes()));

Object result = extractor.extractData(response);
assertNull(result);
}

@Test
@SuppressWarnings("unchecked")
public void normal() throws IOException {
Expand All @@ -100,8 +129,9 @@ public void normal() throws IOException {
extractor = new HttpMessageConverterExtractor<String>(String.class, converters);
given(response.getStatusCode()).willReturn(HttpStatus.OK);
given(response.getHeaders()).willReturn(responseHeaders);
given(response.getBody()).willReturn(new ByteArrayInputStream(expected.getBytes()));
given(converter.canRead(String.class, contentType)).willReturn(true);
given(converter.read(String.class, response)).willReturn(expected);
given(converter.read(eq(String.class), any(HttpInputMessage.class))).willReturn(expected);

Object result = extractor.extractData(response);

Expand All @@ -120,27 +150,12 @@ public void cannotRead() throws IOException {
extractor = new HttpMessageConverterExtractor<String>(String.class, converters);
given(response.getStatusCode()).willReturn(HttpStatus.OK);
given(response.getHeaders()).willReturn(responseHeaders);
given(response.getBody()).willReturn(new ByteArrayInputStream("Foobar".getBytes()));
given(converter.canRead(String.class, contentType)).willReturn(false);

extractor.extractData(response);
}

@Test
@SuppressWarnings("unchecked")
public void connectionClose() throws IOException {
HttpMessageConverter<String> converter = mock(HttpMessageConverter.class);
List<HttpMessageConverter<?>> converters = new ArrayList<HttpMessageConverter<?>>();
converters.add(converter);
HttpHeaders responseHeaders = new HttpHeaders();
responseHeaders.setConnection("close");
extractor = new HttpMessageConverterExtractor<String>(String.class, createConverterList(converter));
given(response.getStatusCode()).willReturn(HttpStatus.OK);
given(response.getHeaders()).willReturn(responseHeaders);

Object result = extractor.extractData(response);
assertNull(result);
}

@Test
@SuppressWarnings("unchecked")
public void generics() throws IOException {
Expand All @@ -155,8 +170,9 @@ public void generics() throws IOException {
extractor = new HttpMessageConverterExtractor<List<String>>(type, converters);
given(response.getStatusCode()).willReturn(HttpStatus.OK);
given(response.getHeaders()).willReturn(responseHeaders);
given(response.getBody()).willReturn(new ByteArrayInputStream(expected.getBytes()));
given(converter.canRead(type, null, contentType)).willReturn(true);
given(converter.read(type, null, response)).willReturn(expected);
given(converter.read(eq(type), eq(null), any(HttpInputMessage.class))).willReturn(expected);

Object result = extractor.extractData(response);

Expand Down
Loading

0 comments on commit b6675b6

Please sign in to comment.