Wrap RestTemplate extractor exceptions in RestClientExceptions

When using a `RestTemplate` instance within a Spring MVC application,
client exceptions may propagate in the MVC stack and can be wrongly
mapped by server `ExceptionHandlers`, leading to a wrong HTTP response
sent to the MVC client.

The `RestTemplate` instance uses `HttpMessageConverter` to decode
the remote service responses; and when those fail decoding an HTTP
response, they can throw an `HttpMessageNotReadableException`. That
exception then bubbles up through the `HttpMessageConverterExtractor`,
`RestTemplate` and the whole MVC stack, later mapped to HTTP 400
responses, since those exceptions can also be throws by the server stack
when the incoming requests can't be deserialized.

This commit wraps all `IOException` and `HttpMessageNotReadableException`
instances thrown by the extractor into `RestClientException`` instances.
It's now easier to consistently handle client exceptions and avoid such
edge cases.

Issue: SPR-13592

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2015 the original author or authors.
+ * Copyright 2002-2016 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -27,6 +27,7 @@ import org.springframework.http.MediaType;
 import org.springframework.http.client.ClientHttpResponse;
 import org.springframework.http.converter.GenericHttpMessageConverter;
 import org.springframework.http.converter.HttpMessageConverter;
+import org.springframework.http.converter.HttpMessageNotReadableException;
 import org.springframework.util.Assert;
 
 /**
@@ -84,27 +85,34 @@ public class HttpMessageConverterExtractor<T> implements ResponseExtractor<T> {
 		}
 		MediaType contentType = getContentType(responseWrapper);
 
-		for (HttpMessageConverter<?> messageConverter : this.messageConverters) {
-			if (messageConverter instanceof GenericHttpMessageConverter) {
-				GenericHttpMessageConverter<?> genericMessageConverter = (GenericHttpMessageConverter<?>) messageConverter;
-				if (genericMessageConverter.canRead(this.responseType, null, contentType)) {
-					if (logger.isDebugEnabled()) {
-						logger.debug("Reading [" + this.responseType + "] as \"" +
-								contentType + "\" using [" + messageConverter + "]");
+		try {
+			for (HttpMessageConverter<?> messageConverter : this.messageConverters) {
+				if (messageConverter instanceof GenericHttpMessageConverter) {
+					GenericHttpMessageConverter<?> genericMessageConverter =
+							(GenericHttpMessageConverter<?>) messageConverter;
+					if (genericMessageConverter.canRead(this.responseType, null, contentType)) {
+						if (logger.isDebugEnabled()) {
+							logger.debug("Reading [" + this.responseType + "] as \"" +
+									contentType + "\" using [" + messageConverter + "]");
+						}
+						return (T) genericMessageConverter.read(this.responseType, null, responseWrapper);
 					}
-					return (T) genericMessageConverter.read(this.responseType, null, responseWrapper);
 				}
-			}
-			if (this.responseClass != null) {
-				if (messageConverter.canRead(this.responseClass, contentType)) {
-					if (logger.isDebugEnabled()) {
-						logger.debug("Reading [" + this.responseClass.getName() + "] as \"" +
-								contentType + "\" using [" + messageConverter + "]");
+				if (this.responseClass != null) {
+					if (messageConverter.canRead(this.responseClass, contentType)) {
+						if (logger.isDebugEnabled()) {
+							logger.debug("Reading [" + this.responseClass.getName() + "] as \"" +
+									contentType + "\" using [" + messageConverter + "]");
+						}
+						return (T) messageConverter.read((Class) this.responseClass, responseWrapper);
 					}
-					return (T) messageConverter.read((Class) this.responseClass, responseWrapper);
 				}
 			}
 		}
+		catch(IOException | HttpMessageNotReadableException exc) {
+			throw new RestClientException("Error while extracting response for type ["
+					+ this.responseType + "] and content type [" + contentType + "]", exc);
+		}
 
 		throw new RestClientException("Could not extract response: no suitable HttpMessageConverter found " +
 				"for response type [" + this.responseType + "] and content type [" + contentType + "]");

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

@@ -22,8 +22,11 @@ import java.lang.reflect.Type;
 import java.util.ArrayList;
 import java.util.List;
 
+import org.hamcrest.Matchers;
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 
 import org.springframework.core.ParameterizedTypeReference;
 import org.springframework.http.HttpHeaders;
@@ -33,6 +36,7 @@ import org.springframework.http.MediaType;
 import org.springframework.http.client.ClientHttpResponse;
 import org.springframework.http.converter.GenericHttpMessageConverter;
 import org.springframework.http.converter.HttpMessageConverter;
+import org.springframework.http.converter.HttpMessageNotReadableException;
 
 import static org.junit.Assert.*;
 import static org.mockito.BDDMockito.*;
@@ -41,6 +45,7 @@ import static org.mockito.BDDMockito.*;
  * Test fixture for {@link HttpMessageConverter}.
  *
  * @author Arjen Poutsma
+ * @author Brian Clozel
  */
 public class HttpMessageConverterExtractorTests {
 
@@ -48,6 +53,9 @@ public class HttpMessageConverterExtractorTests {
 
 	private ClientHttpResponse response;
 
+	@Rule
+	public final ExpectedException exception = ExpectedException.none();
+
 	@Before
 	public void createMocks() {
 		response = mock(ClientHttpResponse.class);
@@ -104,8 +112,6 @@ public class HttpMessageConverterExtractorTests {
 	@SuppressWarnings("unchecked")
 	public void emptyMessageBody() throws IOException {
 		HttpMessageConverter<String> converter = mock(HttpMessageConverter.class);
-		List<HttpMessageConverter<?>> converters = new ArrayList<>();
-		converters.add(converter);
 		HttpHeaders responseHeaders = new HttpHeaders();
 		extractor = new HttpMessageConverterExtractor<>(String.class, createConverterList(converter));
 		given(response.getStatusCode()).willReturn(HttpStatus.OK);
@@ -120,13 +126,11 @@ public class HttpMessageConverterExtractorTests {
 	@SuppressWarnings("unchecked")
 	public void normal() throws IOException {
 		HttpMessageConverter<String> converter = mock(HttpMessageConverter.class);
-		List<HttpMessageConverter<?>> converters = new ArrayList<>();
-		converters.add(converter);
 		HttpHeaders responseHeaders = new HttpHeaders();
 		MediaType contentType = MediaType.TEXT_PLAIN;
 		responseHeaders.setContentType(contentType);
 		String expected = "Foo";
-		extractor = new HttpMessageConverterExtractor<>(String.class, converters);
+		extractor = new HttpMessageConverterExtractor<>(String.class, createConverterList(converter));
 		given(response.getStatusCode()).willReturn(HttpStatus.OK);
 		given(response.getHeaders()).willReturn(responseHeaders);
 		given(response.getBody()).willReturn(new ByteArrayInputStream(expected.getBytes()));
@@ -138,20 +142,19 @@ public class HttpMessageConverterExtractorTests {
 		assertEquals(expected, result);
 	}
 
-	@Test(expected = RestClientException.class)
+	@Test
 	@SuppressWarnings("unchecked")
 	public void cannotRead() throws IOException {
 		HttpMessageConverter<String> converter = mock(HttpMessageConverter.class);
-		List<HttpMessageConverter<?>> converters = new ArrayList<>();
-		converters.add(converter);
 		HttpHeaders responseHeaders = new HttpHeaders();
 		MediaType contentType = MediaType.TEXT_PLAIN;
 		responseHeaders.setContentType(contentType);
-		extractor = new HttpMessageConverterExtractor<>(String.class, converters);
+		extractor = new HttpMessageConverterExtractor<>(String.class, createConverterList(converter));
 		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);
+		exception.expect(RestClientException.class);
 
 		extractor.extractData(response);
 	}
@@ -160,14 +163,13 @@ public class HttpMessageConverterExtractorTests {
 	@SuppressWarnings("unchecked")
 	public void generics() throws IOException {
 		GenericHttpMessageConverter<String> converter = mock(GenericHttpMessageConverter.class);
-		List<HttpMessageConverter<?>> converters = createConverterList(converter);
 		HttpHeaders responseHeaders = new HttpHeaders();
 		MediaType contentType = MediaType.TEXT_PLAIN;
 		responseHeaders.setContentType(contentType);
 		String expected = "Foo";
 		ParameterizedTypeReference<List<String>> reference = new ParameterizedTypeReference<List<String>>() {};
 		Type type = reference.getType();
-		extractor = new HttpMessageConverterExtractor<List<String>>(type, converters);
+		extractor = new HttpMessageConverterExtractor<List<String>>(type, createConverterList(converter));
 		given(response.getStatusCode()).willReturn(HttpStatus.OK);
 		given(response.getHeaders()).willReturn(responseHeaders);
 		given(response.getBody()).willReturn(new ByteArrayInputStream(expected.getBytes()));
@@ -179,6 +181,46 @@ public class HttpMessageConverterExtractorTests {
 		assertEquals(expected, result);
 	}
 
+	@Test // SPR-13592
+	@SuppressWarnings("unchecked")
+	public void converterThrowsIOException() throws IOException {
+		HttpMessageConverter<String> converter = mock(HttpMessageConverter.class);
+		HttpHeaders responseHeaders = new HttpHeaders();
+		MediaType contentType = MediaType.TEXT_PLAIN;
+		responseHeaders.setContentType(contentType);
+		extractor = new HttpMessageConverterExtractor<>(String.class, createConverterList(converter));
+		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)).willThrow(IOException.class);
+		exception.expect(RestClientException.class);
+		exception.expectMessage("Error while extracting response for type " +
+				"[class java.lang.String] and content type [text/plain]");
+		exception.expectCause(Matchers.instanceOf(IOException.class));
+
+		extractor.extractData(response);
+	}
+
+	@Test // SPR-13592
+	@SuppressWarnings("unchecked")
+	public void converterThrowsHttpMessageNotReadableException() throws IOException {
+		HttpMessageConverter<String> converter = mock(HttpMessageConverter.class);
+		HttpHeaders responseHeaders = new HttpHeaders();
+		MediaType contentType = MediaType.TEXT_PLAIN;
+		responseHeaders.setContentType(contentType);
+		extractor = new HttpMessageConverterExtractor<>(String.class, createConverterList(converter));
+		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)).willThrow(HttpMessageNotReadableException.class);
+		exception.expect(RestClientException.class);
+		exception.expectMessage("Error while extracting response for type " +
+				"[class java.lang.String] and content type [text/plain]");
+		exception.expectCause(Matchers.instanceOf(HttpMessageNotReadableException.class));
+
+		extractor.extractData(response);
+	}
+
 	private List<HttpMessageConverter<?>> createConverterList(
 			HttpMessageConverter<?> converter) {
 		List<HttpMessageConverter<?>> converters = new ArrayList<>(1);