From b84fefc430cdf10d428b02f6d61009b7460ae26f Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Fri, 26 Aug 2016 17:56:00 +0200 Subject: [PATCH] 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 --- .../client/HttpMessageConverterExtractor.java | 40 +++++++----- .../HttpMessageConverterExtractorTests.java | 64 +++++++++++++++---- 2 files changed, 77 insertions(+), 27 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/web/client/HttpMessageConverterExtractor.java b/spring-web/src/main/java/org/springframework/web/client/HttpMessageConverterExtractor.java index 2f03908c8ee..5b8cb3bac48 100644 --- a/spring-web/src/main/java/org/springframework/web/client/HttpMessageConverterExtractor.java +++ b/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 implements ResponseExtractor { } 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 + "]"); diff --git a/spring-web/src/test/java/org/springframework/web/client/HttpMessageConverterExtractorTests.java b/spring-web/src/test/java/org/springframework/web/client/HttpMessageConverterExtractorTests.java index 9916e084afe..eafa4cd2e70 100644 --- a/spring-web/src/test/java/org/springframework/web/client/HttpMessageConverterExtractorTests.java +++ b/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 converter = mock(HttpMessageConverter.class); - List> 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 converter = mock(HttpMessageConverter.class); - List> 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 converter = mock(HttpMessageConverter.class); - List> 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 converter = mock(GenericHttpMessageConverter.class); - List> converters = createConverterList(converter); HttpHeaders responseHeaders = new HttpHeaders(); MediaType contentType = MediaType.TEXT_PLAIN; responseHeaders.setContentType(contentType); String expected = "Foo"; ParameterizedTypeReference> reference = new ParameterizedTypeReference>() {}; Type type = reference.getType(); - extractor = new HttpMessageConverterExtractor>(type, converters); + extractor = new HttpMessageConverterExtractor>(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 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 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> createConverterList( HttpMessageConverter converter) { List> converters = new ArrayList<>(1);