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

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

@@ -25,7 +25,7 @@ import org.springframework.http.HttpStatus;
 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

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

@@ -23,7 +23,6 @@ import java.util.List;
 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;
@@ -80,10 +79,12 @@ public class HttpMessageConverterExtractor<T> implements ResponseExtractor<T> {
 	@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) {
@@ -93,7 +94,7 @@ public class HttpMessageConverterExtractor<T> implements ResponseExtractor<T> {
 						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) {
@@ -102,7 +103,7 @@ public class HttpMessageConverterExtractor<T> implements ResponseExtractor<T> {
 						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);
 				}
 			}
 		}
@@ -122,39 +123,4 @@ public class HttpMessageConverterExtractor<T> implements ResponseExtractor<T> {
 		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;
-	}
-
 }

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

@@ -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();
+	}
+}

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

@@ -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;
@@ -26,6 +27,7 @@ import org.junit.Test;
 
 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;
@@ -73,6 +75,17 @@ public class HttpMessageConverterExtractorTests {
 		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);
@@ -87,6 +100,22 @@ public class HttpMessageConverterExtractorTests {
 		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 {
@@ -100,8 +129,9 @@ public class HttpMessageConverterExtractorTests {
 		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);
 
@@ -120,28 +150,13 @@ public class HttpMessageConverterExtractorTests {
 		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
-	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 {
 		GenericHttpMessageConverter<String> converter = mock(GenericHttpMessageConverter.class);
 		List<HttpMessageConverter<?>> converters = createConverterList(converter);
@@ -154,8 +169,9 @@ public class HttpMessageConverterExtractorTests {
 		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);
 

spring-web/src/test/java/org/springframework/web/client/RestTemplateTests.java

@@ -16,6 +16,7 @@
 
 package org.springframework.web.client;
 
+import java.io.ByteArrayInputStream;
 import java.io.IOException;
 import java.net.URI;
 import java.util.Collections;
@@ -31,6 +32,7 @@ import org.junit.Test;
 import org.springframework.core.ParameterizedTypeReference;
 import org.springframework.http.HttpEntity;
 import org.springframework.http.HttpHeaders;
+import org.springframework.http.HttpInputMessage;
 import org.springframework.http.HttpMethod;
 import org.springframework.http.HttpStatus;
 import org.springframework.http.MediaType;
@@ -170,14 +172,15 @@ public class RestTemplateTests {
 		given(request.getHeaders()).willReturn(requestHeaders);
 		given(request.execute()).willReturn(response);
 		given(errorHandler.hasError(response)).willReturn(false);
+		String expected = "Hello World";
 		HttpHeaders responseHeaders = new HttpHeaders();
 		responseHeaders.setContentType(textPlain);
 		responseHeaders.setContentLength(10);
 		given(response.getStatusCode()).willReturn(HttpStatus.OK);
 		given(response.getHeaders()).willReturn(responseHeaders);
+		given(response.getBody()).willReturn(new ByteArrayInputStream(expected.getBytes()));
 		given(converter.canRead(String.class, textPlain)).willReturn(true);
-		String expected = "Hello World";
+		given(converter.read(eq(String.class), any(HttpInputMessage.class))).willReturn(expected);
-		given(converter.read(String.class, response)).willReturn(expected);
 		HttpStatus status = HttpStatus.OK;
 		given(response.getStatusCode()).willReturn(status);
 		given(response.getStatusText()).willReturn(status.getReasonPhrase());
@@ -205,6 +208,7 @@ public class RestTemplateTests {
 		responseHeaders.setContentLength(10);
 		given(response.getStatusCode()).willReturn(HttpStatus.OK);
 		given(response.getHeaders()).willReturn(responseHeaders);
+		given(response.getBody()).willReturn(new ByteArrayInputStream("Foo".getBytes()));
 		given(converter.canRead(String.class, contentType)).willReturn(false);
 		HttpStatus status = HttpStatus.OK;
 		given(response.getStatusCode()).willReturn(status);
@@ -232,14 +236,15 @@ public class RestTemplateTests {
 		given(request.getHeaders()).willReturn(requestHeaders);
 		given(request.execute()).willReturn(response);
 		given(errorHandler.hasError(response)).willReturn(false);
+		String expected = "Hello World";
 		HttpHeaders responseHeaders = new HttpHeaders();
 		responseHeaders.setContentType(textPlain);
 		responseHeaders.setContentLength(10);
 		given(response.getStatusCode()).willReturn(HttpStatus.OK);
 		given(response.getHeaders()).willReturn(responseHeaders);
+		given(response.getBody()).willReturn(new ByteArrayInputStream(expected.getBytes()));
 		given(converter.canRead(String.class, textPlain)).willReturn(true);
-		String expected = "Hello World";
+		given(converter.read(eq(String.class), any(HttpInputMessage.class))).willReturn(expected);
-		given(converter.read(String.class, response)).willReturn(expected);
 		given(response.getStatusCode()).willReturn(HttpStatus.OK);
 		HttpStatus status = HttpStatus.OK;
 		given(response.getStatusCode()).willReturn(status);
@@ -405,14 +410,15 @@ public class RestTemplateTests {
 		converter.write(request, null, this.request);
 		given(this.request.execute()).willReturn(response);
 		given(errorHandler.hasError(response)).willReturn(false);
+		Integer expected = 42;
 		HttpHeaders responseHeaders = new HttpHeaders();
 		responseHeaders.setContentType(textPlain);
 		responseHeaders.setContentLength(10);
 		given(response.getStatusCode()).willReturn(HttpStatus.OK);
 		given(response.getHeaders()).willReturn(responseHeaders);
-		Integer expected = 42;
+		given(response.getBody()).willReturn(new ByteArrayInputStream(expected.toString().getBytes()));
 		given(converter.canRead(Integer.class, textPlain)).willReturn(true);
-		given(converter.read(Integer.class, response)).willReturn(expected);
+		given(converter.read(eq(Integer.class), any(HttpInputMessage.class))).willReturn(expected);
 		HttpStatus status = HttpStatus.OK;
 		given(response.getStatusCode()).willReturn(status);
 		given(response.getStatusText()).willReturn(status.getReasonPhrase());
@@ -437,14 +443,15 @@ public class RestTemplateTests {
 		converter.write(request, null, this.request);
 		given(this.request.execute()).willReturn(response);
 		given(errorHandler.hasError(response)).willReturn(false);
+		Integer expected = 42;
 		HttpHeaders responseHeaders = new HttpHeaders();
 		responseHeaders.setContentType(textPlain);
 		responseHeaders.setContentLength(10);
 		given(response.getStatusCode()).willReturn(HttpStatus.OK);
 		given(response.getHeaders()).willReturn(responseHeaders);
-		Integer expected = 42;
+		given(response.getBody()).willReturn(new ByteArrayInputStream(expected.toString().getBytes()));
 		given(converter.canRead(Integer.class, textPlain)).willReturn(true);
-		given(converter.read(Integer.class, response)).willReturn(expected);
+		given(converter.read(eq(Integer.class), any(HttpInputMessage.class))).willReturn(expected);
 		given(response.getStatusCode()).willReturn(HttpStatus.OK);
 		HttpStatus status = HttpStatus.OK;
 		given(response.getStatusCode()).willReturn(status);
@@ -615,14 +622,16 @@ public class RestTemplateTests {
 		converter.write(body, null, this.request);
 		given(this.request.execute()).willReturn(response);
 		given(errorHandler.hasError(response)).willReturn(false);
+		Integer expected = 42;
 		HttpHeaders responseHeaders = new HttpHeaders();
 		responseHeaders.setContentType(MediaType.TEXT_PLAIN);
 		responseHeaders.setContentLength(10);
 		given(response.getStatusCode()).willReturn(HttpStatus.OK);
 		given(response.getHeaders()).willReturn(responseHeaders);
-		Integer expected = 42;
+		given(response.getBody()).willReturn(new ByteArrayInputStream(expected.toString().getBytes()));
 		given(converter.canRead(Integer.class, MediaType.TEXT_PLAIN)).willReturn(true);
 		given(converter.read(Integer.class, response)).willReturn(expected);
+		given(converter.read(eq(Integer.class), any(HttpInputMessage.class))).willReturn(expected);
 		given(response.getStatusCode()).willReturn(HttpStatus.OK);
 		HttpStatus status = HttpStatus.OK;
 		given(response.getStatusCode()).willReturn(status);
@@ -658,14 +667,15 @@ public class RestTemplateTests {
 		converter.write(requestBody, null, this.request);
 		given(this.request.execute()).willReturn(response);
 		given(errorHandler.hasError(response)).willReturn(false);
+		List<Integer> expected = Collections.singletonList(42);
 		HttpHeaders responseHeaders = new HttpHeaders();
 		responseHeaders.setContentType(MediaType.TEXT_PLAIN);
 		responseHeaders.setContentLength(10);
 		given(response.getStatusCode()).willReturn(HttpStatus.OK);
 		given(response.getHeaders()).willReturn(responseHeaders);
-		List<Integer> expected = Collections.singletonList(42);
+		given(response.getBody()).willReturn(new ByteArrayInputStream(new Integer(42).toString().getBytes()));
 		given(converter.canRead(intList.getType(), null, MediaType.TEXT_PLAIN)).willReturn(true);
-		given(converter.read(intList.getType(), null, response)).willReturn(expected);
+		given(converter.read(eq(intList.getType()), eq(null), any(HttpInputMessage.class))).willReturn(expected);
 		given(response.getStatusCode()).willReturn(HttpStatus.OK);
 		HttpStatus status = HttpStatus.OK;
 		given(response.getStatusCode()).willReturn(status);