From 24c30eac49c2fd75c528290fa9144a60b961c635 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Tue, 12 Jun 2012 14:54:16 -0400 Subject: [PATCH] Raise RestClientException for unknown status codes HttpStatus cannot be created with an unknown status code. If a server returns a status code that's not in the HttpStatus enum values, an IllegalArgumentException is raised. Rather than allowing it to propagate as such, this change ensures the actual exception raised is a RestClientException. Issue: SPR-9406 Backport-Issue: SPR-9502 --- .../resources/changelog.txt | 1 + .../client/DefaultResponseErrorHandler.java | 46 ++++++---- .../web/client/HttpClientErrorException.java | 45 +++++++--- .../web/client/HttpServerErrorException.java | 40 ++++++--- .../web/client/HttpStatusCodeException.java | 83 +++++++++++++------ .../DefaultResponseErrorHandlerTests.java | 40 +++++++-- 6 files changed, 179 insertions(+), 76 deletions(-) diff --git a/build-spring-framework/resources/changelog.txt b/build-spring-framework/resources/changelog.txt index 9a91eca8b1..1c93d46e2e 100644 --- a/build-spring-framework/resources/changelog.txt +++ b/build-spring-framework/resources/changelog.txt @@ -19,6 +19,7 @@ Changes in version 3.1.2 (2012-06-??) * add Jackson 2 HttpMessageConverter and View types * translate IOException from Jackson to HttpMessageNotReadableException * fix content negotiation issue when sorting selected media types by quality value +* raise RestClientException instead of IllegalArgumentException for unknown status codes Changes in version 3.1.1 (2012-02-16) diff --git a/org.springframework.web/src/main/java/org/springframework/web/client/DefaultResponseErrorHandler.java b/org.springframework.web/src/main/java/org/springframework/web/client/DefaultResponseErrorHandler.java index 0615f7a823..af0b676021 100644 --- a/org.springframework.web/src/main/java/org/springframework/web/client/DefaultResponseErrorHandler.java +++ b/org.springframework.web/src/main/java/org/springframework/web/client/DefaultResponseErrorHandler.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2011 the original author or authors. + * Copyright 2002-2012 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. @@ -20,6 +20,7 @@ import java.io.IOException; import java.io.InputStream; import java.nio.charset.Charset; +import org.springframework.http.HttpHeaders; import org.springframework.http.HttpStatus; import org.springframework.http.MediaType; import org.springframework.http.client.ClientHttpResponse; @@ -28,10 +29,11 @@ import org.springframework.util.FileCopyUtils; /** * Default implementation of the {@link ResponseErrorHandler} interface. * - *

This error handler checks for the status code on the {@link ClientHttpResponse}: any code with series - * {@link org.springframework.http.HttpStatus.Series#CLIENT_ERROR} or - * {@link org.springframework.http.HttpStatus.Series#SERVER_ERROR} is considered to be an error. - * This behavior can be changed by overriding the {@link #hasError(HttpStatus)} method. + *

This error handler checks for the status code on the {@link ClientHttpResponse}: any + * code with series {@link org.springframework.http.HttpStatus.Series#CLIENT_ERROR} or + * {@link org.springframework.http.HttpStatus.Series#SERVER_ERROR} is considered to be an + * error. This behavior can be changed by overriding the {@link #hasError(HttpStatus)} + * method. * * @author Arjen Poutsma * @since 3.0 @@ -43,7 +45,18 @@ public class DefaultResponseErrorHandler implements ResponseErrorHandler { * Delegates to {@link #hasError(HttpStatus)} with the response status code. */ public boolean hasError(ClientHttpResponse response) throws IOException { - return hasError(response.getStatusCode()); + return hasError(getStatusCode(response)); + } + + private HttpStatus getStatusCode(ClientHttpResponse response) throws IOException { + HttpStatus statusCode; + try { + statusCode = response.getStatusCode(); + } + catch (IllegalArgumentException ex) { + throw new RestClientException("Unknown status code [" + response.getRawStatusCode() + "]"); + } + return statusCode; } /** @@ -67,15 +80,16 @@ public class DefaultResponseErrorHandler implements ResponseErrorHandler { * and a {@link RestClientException} in other cases. */ public void handleError(ClientHttpResponse response) throws IOException { - HttpStatus statusCode = response.getStatusCode(); - MediaType contentType = response.getHeaders().getContentType(); + HttpStatus statusCode = getStatusCode(response); + HttpHeaders headers = response.getHeaders(); + MediaType contentType = headers.getContentType(); Charset charset = contentType != null ? contentType.getCharSet() : null; byte[] body = getResponseBody(response); switch (statusCode.series()) { case CLIENT_ERROR: - throw new HttpClientErrorException(statusCode, response.getStatusText(), body, charset); + throw new HttpClientErrorException(statusCode, response.getStatusText(), headers, body, charset); case SERVER_ERROR: - throw new HttpServerErrorException(statusCode, response.getStatusText(), body, charset); + throw new HttpServerErrorException(statusCode, response.getStatusText(), headers, body, charset); default: throw new RestClientException("Unknown status code [" + statusCode + "]"); } @@ -83,15 +97,15 @@ public class DefaultResponseErrorHandler implements ResponseErrorHandler { private byte[] getResponseBody(ClientHttpResponse response) { try { - InputStream responseBody = response.getBody(); - if (responseBody != null) { - return FileCopyUtils.copyToByteArray(responseBody); - } + InputStream responseBody = response.getBody(); + if (responseBody != null) { + return FileCopyUtils.copyToByteArray(responseBody); + } } catch (IOException ex) { - // ignore + // ignore } - return new byte[0]; + return new byte[0]; } } diff --git a/org.springframework.web/src/main/java/org/springframework/web/client/HttpClientErrorException.java b/org.springframework.web/src/main/java/org/springframework/web/client/HttpClientErrorException.java index 11f583de3b..5d5ef0b40e 100644 --- a/org.springframework.web/src/main/java/org/springframework/web/client/HttpClientErrorException.java +++ b/org.springframework.web/src/main/java/org/springframework/web/client/HttpClientErrorException.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2010 the original author or authors. + * Copyright 2002-2012 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. @@ -18,6 +18,7 @@ package org.springframework.web.client; import java.nio.charset.Charset; +import org.springframework.http.HttpHeaders; import org.springframework.http.HttpStatus; /** @@ -29,8 +30,12 @@ import org.springframework.http.HttpStatus; */ public class HttpClientErrorException extends HttpStatusCodeException { + private static final long serialVersionUID = 5177019431887513952L; + + /** - * Construct a new instance of {@code HttpClientErrorException} based on a {@link HttpStatus}. + * Construct a new instance of {@code HttpClientErrorException} based on an + * {@link HttpStatus}. * @param statusCode the status code */ public HttpClientErrorException(HttpStatus statusCode) { @@ -38,7 +43,8 @@ public class HttpClientErrorException extends HttpStatusCodeException { } /** - * Construct a new instance of {@code HttpClientErrorException} based on a {@link HttpStatus} and status text. + * Construct a new instance of {@code HttpClientErrorException} based on an + * {@link HttpStatus} and status text. * @param statusCode the status code * @param statusText the status text */ @@ -47,18 +53,31 @@ public class HttpClientErrorException extends HttpStatusCodeException { } /** - * Construct a new instance of {@code HttpClientErrorException} based on a {@link HttpStatus}, status text, and - * response body content. - * - * @param statusCode the status code - * @param statusText the status text - * @param responseBody the response body content, may be {@code null} + * Construct a new instance of {@code HttpClientErrorException} based on an + * {@link HttpStatus}, status text, and response body content. + * @param statusCode the status code + * @param statusText the status text + * @param responseBody the response body content, may be {@code null} * @param responseCharset the response body charset, may be {@code null} */ - public HttpClientErrorException(HttpStatus statusCode, - String statusText, - byte[] responseBody, - Charset responseCharset) { + public HttpClientErrorException(HttpStatus statusCode, String statusText, + byte[] responseBody, Charset responseCharset) { super(statusCode, statusText, responseBody, responseCharset); } + + /** + * Construct a new instance of {@code HttpClientErrorException} based on an + * {@link HttpStatus}, status text, and response body content. + * @param statusCode the status code + * @param statusText the status text + * @param responseHeaders the response headers, may be {@code null} + * @param responseBody the response body content, may be {@code null} + * @param responseCharset the response body charset, may be {@code null} + * @since 3.2 + */ + public HttpClientErrorException(HttpStatus statusCode, String statusText, + HttpHeaders responseHeaders, byte[] responseBody, Charset responseCharset) { + super(statusCode, statusText, responseHeaders, responseBody, responseCharset); + } + } diff --git a/org.springframework.web/src/main/java/org/springframework/web/client/HttpServerErrorException.java b/org.springframework.web/src/main/java/org/springframework/web/client/HttpServerErrorException.java index 3828b036d8..07bd013a52 100644 --- a/org.springframework.web/src/main/java/org/springframework/web/client/HttpServerErrorException.java +++ b/org.springframework.web/src/main/java/org/springframework/web/client/HttpServerErrorException.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2010 the original author or authors. + * Copyright 2002-2012 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. @@ -18,6 +18,7 @@ package org.springframework.web.client; import java.nio.charset.Charset; +import org.springframework.http.HttpHeaders; import org.springframework.http.HttpStatus; /** @@ -29,9 +30,12 @@ import org.springframework.http.HttpStatus; */ public class HttpServerErrorException extends HttpStatusCodeException { + private static final long serialVersionUID = -2915754006618138282L; + + /** - * Construct a new instance of {@code HttpServerErrorException} based on a {@link HttpStatus}. - * + * Construct a new instance of {@code HttpServerErrorException} based on an + * {@link HttpStatus}. * @param statusCode the status code */ public HttpServerErrorException(HttpStatus statusCode) { @@ -39,8 +43,8 @@ public class HttpServerErrorException extends HttpStatusCodeException { } /** - * Construct a new instance of {@code HttpServerErrorException} based on a {@link HttpStatus} and status text. - * + * Construct a new instance of {@code HttpServerErrorException} based on an + * {@link HttpStatus} and status text. * @param statusCode the status code * @param statusText the status text */ @@ -49,19 +53,31 @@ public class HttpServerErrorException extends HttpStatusCodeException { } /** - * Construct a new instance of {@code HttpServerErrorException} based on a {@link HttpStatus}, status text, and - * response body content. - * + * Construct a new instance of {@code HttpServerErrorException} based on an + * {@link HttpStatus}, status text, and response body content. * @param statusCode the status code * @param statusText the status text * @param responseBody the response body content, may be {@code null} * @param responseCharset the response body charset, may be {@code null} * @since 3.0.5 */ - public HttpServerErrorException(HttpStatus statusCode, - String statusText, - byte[] responseBody, - Charset responseCharset) { + public HttpServerErrorException(HttpStatus statusCode, String statusText, + byte[] responseBody, Charset responseCharset) { super(statusCode, statusText, responseBody, responseCharset); } + + /** + * Construct a new instance of {@code HttpServerErrorException} based on a + * {@link HttpStatus}, status text, and response body content. + * @param statusCode the status code + * @param statusText the status text + * @param responseHeaders the response headers, may be {@code null} + * @param responseBody the response body content, may be {@code null} + * @param responseCharset the response body charset, may be {@code null} + * @since 3.2 + */ + public HttpServerErrorException(HttpStatus statusCode, String statusText, + HttpHeaders responseHeaders, byte[] responseBody, Charset responseCharset) { + super(statusCode, statusText, responseHeaders, responseBody, responseCharset); + } } diff --git a/org.springframework.web/src/main/java/org/springframework/web/client/HttpStatusCodeException.java b/org.springframework.web/src/main/java/org/springframework/web/client/HttpStatusCodeException.java index 3a9355a06a..106ec761d6 100644 --- a/org.springframework.web/src/main/java/org/springframework/web/client/HttpStatusCodeException.java +++ b/org.springframework.web/src/main/java/org/springframework/web/client/HttpStatusCodeException.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2010 the original author or authors. + * Copyright 2002-2012 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. @@ -19,17 +19,21 @@ package org.springframework.web.client; import java.io.UnsupportedEncodingException; import java.nio.charset.Charset; +import org.springframework.http.HttpHeaders; import org.springframework.http.HttpStatus; /** * Abstract base class for exceptions based on an {@link HttpStatus}. * * @author Arjen Poutsma + * @author Chris Beams * @since 3.0 */ public abstract class HttpStatusCodeException extends RestClientException { - private static final Charset DEFAULT_CHARSET = Charset.forName("ISO-8859-1"); + private static final long serialVersionUID = -5807494703720513267L; + + private static final String DEFAULT_CHARSET = "ISO-8859-1"; private final HttpStatus statusCode; @@ -37,34 +41,36 @@ public abstract class HttpStatusCodeException extends RestClientException { private final byte[] responseBody; - private final Charset responseCharset; + private final HttpHeaders responseHeaders; + + private final String responseCharset; + /** - * Construct a new instance of {@code HttpStatusCodeException} based on a {@link HttpStatus}. - * + * Construct a new instance of {@code HttpStatusCodeException} based on an + * {@link HttpStatus}. * @param statusCode the status code */ protected HttpStatusCodeException(HttpStatus statusCode) { - this(statusCode, statusCode.name(), null, null); + this(statusCode, statusCode.name(), null, null, null); } /** - * Construct a new instance of {@code HttpStatusCodeException} based on a {@link HttpStatus} and status text. - * + * Construct a new instance of {@code HttpStatusCodeException} based on an + * {@link HttpStatus} and status text. * @param statusCode the status code * @param statusText the status text */ protected HttpStatusCodeException(HttpStatus statusCode, String statusText) { - this(statusCode, statusText, null, null); + this(statusCode, statusText, null, null, null); } /** - * Construct a new instance of {@code HttpStatusCodeException} based on a {@link HttpStatus}, status text, and - * response body content. - * - * @param statusCode the status code - * @param statusText the status text - * @param responseBody the response body content, may be {@code null} + * Construct a new instance of {@code HttpStatusCodeException} based on an + * {@link HttpStatus}, status text, and response body content. + * @param statusCode the status code + * @param statusText the status text + * @param responseBody the response body content, may be {@code null} * @param responseCharset the response body charset, may be {@code null} * @since 3.0.5 */ @@ -72,29 +78,54 @@ public abstract class HttpStatusCodeException extends RestClientException { String statusText, byte[] responseBody, Charset responseCharset) { - super(statusCode.value() + " " + statusText); - this.statusCode = statusCode; - this.statusText = statusText; - this.responseBody = responseBody != null ? responseBody : new byte[0]; - this.responseCharset = responseCharset != null ? responseCharset : DEFAULT_CHARSET; + this(statusCode, statusText, null, responseBody, responseCharset); } /** - * Returns the HTTP status code. + * Construct a new instance of {@code HttpStatusCodeException} based on an + * {@link HttpStatus}, status text, and response body content. + * @param statusCode the status code + * @param statusText the status text + * @param responseHeaders the response headers, may be {@code null} + * @param responseBody the response body content, may be {@code null} + * @param responseCharset the response body charset, may be {@code null} + * @since 3.2 + */ + protected HttpStatusCodeException(HttpStatus statusCode, String statusText, + HttpHeaders responseHeaders, byte[] responseBody, Charset responseCharset) { + super(statusCode.value() + " " + statusText); + this.statusCode = statusCode; + this.statusText = statusText; + this.responseHeaders = responseHeaders; + this.responseBody = responseBody != null ? responseBody : new byte[0]; + this.responseCharset = responseCharset != null ? responseCharset.name() : DEFAULT_CHARSET; + } + + + /** + * Return the HTTP status code. */ public HttpStatus getStatusCode() { return this.statusCode; } /** - * Returns the HTTP status text. + * Return the HTTP status text. */ public String getStatusText() { return this.statusText; } /** - * Returns the response body as a byte array. + * Return the HTTP response headers. + * @since 3.2 + */ + public HttpHeaders getResponseHeaders() { + return this.responseHeaders; + } + + /** + * Return the response body as a byte array. * * @since 3.0.5 */ @@ -103,17 +134,17 @@ public abstract class HttpStatusCodeException extends RestClientException { } /** - * Returns the response body as a string. - * + * Return the response body as a string. * @since 3.0.5 */ public String getResponseBodyAsString() { try { - return new String(responseBody, responseCharset.name()); + return new String(responseBody, responseCharset); } catch (UnsupportedEncodingException ex) { // should not occur throw new InternalError(ex.getMessage()); } } + } diff --git a/org.springframework.web/src/test/java/org/springframework/web/client/DefaultResponseErrorHandlerTests.java b/org.springframework.web/src/test/java/org/springframework/web/client/DefaultResponseErrorHandlerTests.java index 0c9866c19b..36547e848f 100644 --- a/org.springframework.web/src/test/java/org/springframework/web/client/DefaultResponseErrorHandlerTests.java +++ b/org.springframework.web/src/test/java/org/springframework/web/client/DefaultResponseErrorHandlerTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2011 the original author or authors. + * Copyright 2002-2012 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. @@ -19,19 +19,23 @@ package org.springframework.web.client; import java.io.ByteArrayInputStream; import java.io.IOException; +import org.junit.Before; +import org.junit.Test; + import org.springframework.http.HttpHeaders; import org.springframework.http.HttpStatus; import org.springframework.http.MediaType; import org.springframework.http.client.ClientHttpResponse; -import org.junit.Before; -import org.junit.Test; - import static org.easymock.EasyMock.*; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; -/** @author Arjen Poutsma */ +import static org.junit.Assert.*; + +/** + * Unit tests for {@link DefaultResponseErrorHandler}. + * + * @author Arjen Poutsma + */ public class DefaultResponseErrorHandlerTests { private DefaultResponseErrorHandler handler; @@ -64,7 +68,7 @@ public class DefaultResponseErrorHandlerTests { verify(response); } - @Test(expected = HttpClientErrorException.class) + @Test public void handleError() throws Exception { HttpHeaders headers = new HttpHeaders(); headers.setContentType(MediaType.TEXT_PLAIN); @@ -76,7 +80,13 @@ public class DefaultResponseErrorHandlerTests { replay(response); - handler.handleError(response); + try { + handler.handleError(response); + fail("expected HttpClientErrorException"); + } + catch (HttpClientErrorException e) { + assertSame(headers, e.getResponseHeaders()); + } verify(response); } @@ -114,4 +124,16 @@ public class DefaultResponseErrorHandlerTests { verify(response); } + + // SPR-9406 + + @Test(expected=RestClientException.class) + public void unknownStatusCode() throws Exception { + expect(response.getStatusCode()).andThrow(new IllegalArgumentException("No matching constant for 999")); + expect(response.getRawStatusCode()).andReturn(999); + + replay(response); + + handler.handleError(response); + } }