Skip to content

Commit b84fefc

Browse files
bclozelbclozel
committed
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
1 parent 7fdb892 commit b84fefc

File tree

2 files changed

+77
-27
lines changed

2 files changed

+77
-27
lines changed

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

+24-16
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2015 the original author or authors.
2+
* Copyright 2002-2016 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -27,6 +27,7 @@
2727
import org.springframework.http.client.ClientHttpResponse;
2828
import org.springframework.http.converter.GenericHttpMessageConverter;
2929
import org.springframework.http.converter.HttpMessageConverter;
30+
import org.springframework.http.converter.HttpMessageNotReadableException;
3031
import org.springframework.util.Assert;
3132

3233
/**
@@ -84,27 +85,34 @@ public T extractData(ClientHttpResponse response) throws IOException {
8485
}
8586
MediaType contentType = getContentType(responseWrapper);
8687

87-
for (HttpMessageConverter<?> messageConverter : this.messageConverters) {
88-
if (messageConverter instanceof GenericHttpMessageConverter) {
89-
GenericHttpMessageConverter<?> genericMessageConverter = (GenericHttpMessageConverter<?>) messageConverter;
90-
if (genericMessageConverter.canRead(this.responseType, null, contentType)) {
91-
if (logger.isDebugEnabled()) {
92-
logger.debug("Reading [" + this.responseType + "] as \"" +
93-
contentType + "\" using [" + messageConverter + "]");
88+
try {
89+
for (HttpMessageConverter<?> messageConverter : this.messageConverters) {
90+
if (messageConverter instanceof GenericHttpMessageConverter) {
91+
GenericHttpMessageConverter<?> genericMessageConverter =
92+
(GenericHttpMessageConverter<?>) messageConverter;
93+
if (genericMessageConverter.canRead(this.responseType, null, contentType)) {
94+
if (logger.isDebugEnabled()) {
95+
logger.debug("Reading [" + this.responseType + "] as \"" +
96+
contentType + "\" using [" + messageConverter + "]");
97+
}
98+
return (T) genericMessageConverter.read(this.responseType, null, responseWrapper);
9499
}
95-
return (T) genericMessageConverter.read(this.responseType, null, responseWrapper);
96100
}
97-
}
98-
if (this.responseClass != null) {
99-
if (messageConverter.canRead(this.responseClass, contentType)) {
100-
if (logger.isDebugEnabled()) {
101-
logger.debug("Reading [" + this.responseClass.getName() + "] as \"" +
102-
contentType + "\" using [" + messageConverter + "]");
101+
if (this.responseClass != null) {
102+
if (messageConverter.canRead(this.responseClass, contentType)) {
103+
if (logger.isDebugEnabled()) {
104+
logger.debug("Reading [" + this.responseClass.getName() + "] as \"" +
105+
contentType + "\" using [" + messageConverter + "]");
106+
}
107+
return (T) messageConverter.read((Class) this.responseClass, responseWrapper);
103108
}
104-
return (T) messageConverter.read((Class) this.responseClass, responseWrapper);
105109
}
106110
}
107111
}
112+
catch(IOException | HttpMessageNotReadableException exc) {
113+
throw new RestClientException("Error while extracting response for type ["
114+
+ this.responseType + "] and content type [" + contentType + "]", exc);
115+
}
108116

109117
throw new RestClientException("Could not extract response: no suitable HttpMessageConverter found " +
110118
"for response type [" + this.responseType + "] and content type [" + contentType + "]");

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

+53-11
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,11 @@
2222
import java.util.ArrayList;
2323
import java.util.List;
2424

25+
import org.hamcrest.Matchers;
2526
import org.junit.Before;
27+
import org.junit.Rule;
2628
import org.junit.Test;
29+
import org.junit.rules.ExpectedException;
2730

2831
import org.springframework.core.ParameterizedTypeReference;
2932
import org.springframework.http.HttpHeaders;
@@ -33,6 +36,7 @@
3336
import org.springframework.http.client.ClientHttpResponse;
3437
import org.springframework.http.converter.GenericHttpMessageConverter;
3538
import org.springframework.http.converter.HttpMessageConverter;
39+
import org.springframework.http.converter.HttpMessageNotReadableException;
3640

3741
import static org.junit.Assert.*;
3842
import static org.mockito.BDDMockito.*;
@@ -41,13 +45,17 @@
4145
* Test fixture for {@link HttpMessageConverter}.
4246
*
4347
* @author Arjen Poutsma
48+
* @author Brian Clozel
4449
*/
4550
public class HttpMessageConverterExtractorTests {
4651

4752
private HttpMessageConverterExtractor<?> extractor;
4853

4954
private ClientHttpResponse response;
5055

56+
@Rule
57+
public final ExpectedException exception = ExpectedException.none();
58+
5159
@Before
5260
public void createMocks() {
5361
response = mock(ClientHttpResponse.class);
@@ -104,8 +112,6 @@ public void zeroContentLength() throws IOException {
104112
@SuppressWarnings("unchecked")
105113
public void emptyMessageBody() throws IOException {
106114
HttpMessageConverter<String> converter = mock(HttpMessageConverter.class);
107-
List<HttpMessageConverter<?>> converters = new ArrayList<>();
108-
converters.add(converter);
109115
HttpHeaders responseHeaders = new HttpHeaders();
110116
extractor = new HttpMessageConverterExtractor<>(String.class, createConverterList(converter));
111117
given(response.getStatusCode()).willReturn(HttpStatus.OK);
@@ -120,13 +126,11 @@ public void emptyMessageBody() throws IOException {
120126
@SuppressWarnings("unchecked")
121127
public void normal() throws IOException {
122128
HttpMessageConverter<String> converter = mock(HttpMessageConverter.class);
123-
List<HttpMessageConverter<?>> converters = new ArrayList<>();
124-
converters.add(converter);
125129
HttpHeaders responseHeaders = new HttpHeaders();
126130
MediaType contentType = MediaType.TEXT_PLAIN;
127131
responseHeaders.setContentType(contentType);
128132
String expected = "Foo";
129-
extractor = new HttpMessageConverterExtractor<>(String.class, converters);
133+
extractor = new HttpMessageConverterExtractor<>(String.class, createConverterList(converter));
130134
given(response.getStatusCode()).willReturn(HttpStatus.OK);
131135
given(response.getHeaders()).willReturn(responseHeaders);
132136
given(response.getBody()).willReturn(new ByteArrayInputStream(expected.getBytes()));
@@ -138,20 +142,19 @@ public void normal() throws IOException {
138142
assertEquals(expected, result);
139143
}
140144

141-
@Test(expected = RestClientException.class)
145+
@Test
142146
@SuppressWarnings("unchecked")
143147
public void cannotRead() throws IOException {
144148
HttpMessageConverter<String> converter = mock(HttpMessageConverter.class);
145-
List<HttpMessageConverter<?>> converters = new ArrayList<>();
146-
converters.add(converter);
147149
HttpHeaders responseHeaders = new HttpHeaders();
148150
MediaType contentType = MediaType.TEXT_PLAIN;
149151
responseHeaders.setContentType(contentType);
150-
extractor = new HttpMessageConverterExtractor<>(String.class, converters);
152+
extractor = new HttpMessageConverterExtractor<>(String.class, createConverterList(converter));
151153
given(response.getStatusCode()).willReturn(HttpStatus.OK);
152154
given(response.getHeaders()).willReturn(responseHeaders);
153155
given(response.getBody()).willReturn(new ByteArrayInputStream("Foobar".getBytes()));
154156
given(converter.canRead(String.class, contentType)).willReturn(false);
157+
exception.expect(RestClientException.class);
155158

156159
extractor.extractData(response);
157160
}
@@ -160,14 +163,13 @@ public void cannotRead() throws IOException {
160163
@SuppressWarnings("unchecked")
161164
public void generics() throws IOException {
162165
GenericHttpMessageConverter<String> converter = mock(GenericHttpMessageConverter.class);
163-
List<HttpMessageConverter<?>> converters = createConverterList(converter);
164166
HttpHeaders responseHeaders = new HttpHeaders();
165167
MediaType contentType = MediaType.TEXT_PLAIN;
166168
responseHeaders.setContentType(contentType);
167169
String expected = "Foo";
168170
ParameterizedTypeReference<List<String>> reference = new ParameterizedTypeReference<List<String>>() {};
169171
Type type = reference.getType();
170-
extractor = new HttpMessageConverterExtractor<List<String>>(type, converters);
172+
extractor = new HttpMessageConverterExtractor<List<String>>(type, createConverterList(converter));
171173
given(response.getStatusCode()).willReturn(HttpStatus.OK);
172174
given(response.getHeaders()).willReturn(responseHeaders);
173175
given(response.getBody()).willReturn(new ByteArrayInputStream(expected.getBytes()));
@@ -179,6 +181,46 @@ public void generics() throws IOException {
179181
assertEquals(expected, result);
180182
}
181183

184+
@Test // SPR-13592
185+
@SuppressWarnings("unchecked")
186+
public void converterThrowsIOException() throws IOException {
187+
HttpMessageConverter<String> converter = mock(HttpMessageConverter.class);
188+
HttpHeaders responseHeaders = new HttpHeaders();
189+
MediaType contentType = MediaType.TEXT_PLAIN;
190+
responseHeaders.setContentType(contentType);
191+
extractor = new HttpMessageConverterExtractor<>(String.class, createConverterList(converter));
192+
given(response.getStatusCode()).willReturn(HttpStatus.OK);
193+
given(response.getHeaders()).willReturn(responseHeaders);
194+
given(response.getBody()).willReturn(new ByteArrayInputStream("Foobar".getBytes()));
195+
given(converter.canRead(String.class, contentType)).willThrow(IOException.class);
196+
exception.expect(RestClientException.class);
197+
exception.expectMessage("Error while extracting response for type " +
198+
"[class java.lang.String] and content type [text/plain]");
199+
exception.expectCause(Matchers.instanceOf(IOException.class));
200+
201+
extractor.extractData(response);
202+
}
203+
204+
@Test // SPR-13592
205+
@SuppressWarnings("unchecked")
206+
public void converterThrowsHttpMessageNotReadableException() throws IOException {
207+
HttpMessageConverter<String> converter = mock(HttpMessageConverter.class);
208+
HttpHeaders responseHeaders = new HttpHeaders();
209+
MediaType contentType = MediaType.TEXT_PLAIN;
210+
responseHeaders.setContentType(contentType);
211+
extractor = new HttpMessageConverterExtractor<>(String.class, createConverterList(converter));
212+
given(response.getStatusCode()).willReturn(HttpStatus.OK);
213+
given(response.getHeaders()).willReturn(responseHeaders);
214+
given(response.getBody()).willReturn(new ByteArrayInputStream("Foobar".getBytes()));
215+
given(converter.canRead(String.class, contentType)).willThrow(HttpMessageNotReadableException.class);
216+
exception.expect(RestClientException.class);
217+
exception.expectMessage("Error while extracting response for type " +
218+
"[class java.lang.String] and content type [text/plain]");
219+
exception.expectCause(Matchers.instanceOf(HttpMessageNotReadableException.class));
220+
221+
extractor.extractData(response);
222+
}
223+
182224
private List<HttpMessageConverter<?>> createConverterList(
183225
HttpMessageConverter<?> converter) {
184226
List<HttpMessageConverter<?>> converters = new ArrayList<>(1);

0 commit comments

Comments
 (0)