From 2b1d156acf9a29ab09b940ec07a6461445c2c91b Mon Sep 17 00:00:00 2001 From: Arjen Poutsma Date: Mon, 11 May 2009 14:52:14 +0000 Subject: [PATCH] SPR-5732 - When no type conversion strategy is found on a @Controller handler method bind target, a 500 error code should be returned not a 400. git-svn-id: https://src.springframework.org/svn/spring-framework/trunk@1144 50f2f4bb-b051-0410-bef5-90022cba6387 --- .../beans/BeanWrapperImpl.java | 12 +++++ .../ConversionNotSupportedException.java | 44 +++++++++++++++++++ .../beans/DirectFieldAccessor.java | 9 +++- .../beans/SimpleTypeConverter.java | 5 ++- .../beans/TypeConverterDelegate.java | 7 +-- .../DefaultHandlerExceptionResolver.java | 26 ++++++++++- .../ServletAnnotationControllerTests.java | 3 +- ...plateServletAnnotationControllerTests.java | 23 +++++++++- 8 files changed, 121 insertions(+), 8 deletions(-) create mode 100644 org.springframework.beans/src/main/java/org/springframework/beans/ConversionNotSupportedException.java diff --git a/org.springframework.beans/src/main/java/org/springframework/beans/BeanWrapperImpl.java b/org.springframework.beans/src/main/java/org/springframework/beans/BeanWrapperImpl.java index 0f169da8b76..e8a9ef385fa 100644 --- a/org.springframework.beans/src/main/java/org/springframework/beans/BeanWrapperImpl.java +++ b/org.springframework.beans/src/main/java/org/springframework/beans/BeanWrapperImpl.java @@ -355,6 +355,8 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra } catch (IllegalArgumentException ex) { throw new TypeMismatchException(value, requiredType, ex); + } catch (IllegalStateException ex) { + throw new ConversionNotSupportedException(value, requiredType, ex); } } @@ -382,6 +384,11 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra new PropertyChangeEvent(this.rootObject, this.nestedPath + propertyName, null, value); throw new TypeMismatchException(pce, pd.getPropertyType(), ex); } + catch (IllegalStateException ex) { + PropertyChangeEvent pce = + new PropertyChangeEvent(this.rootObject, this.nestedPath + propertyName, null, value); + throw new ConversionNotSupportedException(pce, pd.getPropertyType(), ex); + } } @@ -840,6 +847,11 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra new PropertyChangeEvent(this.rootObject, this.nestedPath + propertyName, oldValue, pv.getValue()); throw new TypeMismatchException(pce, pd.getPropertyType(), ex); } + catch (IllegalStateException ex) { + PropertyChangeEvent pce = + new PropertyChangeEvent(this.rootObject, this.nestedPath + propertyName, oldValue, pv.getValue()); + throw new ConversionNotSupportedException(pce, pd.getPropertyType(), ex); + } catch (IllegalAccessException ex) { PropertyChangeEvent pce = new PropertyChangeEvent(this.rootObject, this.nestedPath + propertyName, oldValue, pv.getValue()); diff --git a/org.springframework.beans/src/main/java/org/springframework/beans/ConversionNotSupportedException.java b/org.springframework.beans/src/main/java/org/springframework/beans/ConversionNotSupportedException.java new file mode 100644 index 00000000000..fab3fc38257 --- /dev/null +++ b/org.springframework.beans/src/main/java/org/springframework/beans/ConversionNotSupportedException.java @@ -0,0 +1,44 @@ +/* + * Copyright 2002-2009 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. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.beans; + +import java.beans.PropertyChangeEvent; + +/** + * Exception thrown when no suitable editor can be found to set a bean property. + * + * @author Arjen Poutsma + * @since 3.0 + */ +public class ConversionNotSupportedException extends TypeMismatchException { + + public ConversionNotSupportedException(PropertyChangeEvent propertyChangeEvent, Class requiredType) { + super(propertyChangeEvent, requiredType); + } + + public ConversionNotSupportedException(PropertyChangeEvent propertyChangeEvent, Class requiredType, Throwable cause) { + super(propertyChangeEvent, requiredType, cause); + } + + public ConversionNotSupportedException(Object value, Class requiredType) { + super(value, requiredType); + } + + public ConversionNotSupportedException(Object value, Class requiredType, Throwable cause) { + super(value, requiredType, cause); + } +} diff --git a/org.springframework.beans/src/main/java/org/springframework/beans/DirectFieldAccessor.java b/org.springframework.beans/src/main/java/org/springframework/beans/DirectFieldAccessor.java index b7ee738563b..e16dc2df1f0 100644 --- a/org.springframework.beans/src/main/java/org/springframework/beans/DirectFieldAccessor.java +++ b/org.springframework.beans/src/main/java/org/springframework/beans/DirectFieldAccessor.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2008 the original author or authors. + * Copyright 2002-2009 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. @@ -124,6 +124,10 @@ public class DirectFieldAccessor extends AbstractPropertyAccessor { PropertyChangeEvent pce = new PropertyChangeEvent(this.target, propertyName, oldValue, newValue); throw new TypeMismatchException(pce, field.getType(), ex); } + catch (IllegalStateException ex) { + PropertyChangeEvent pce = new PropertyChangeEvent(this.target, propertyName, oldValue, newValue); + throw new ConversionNotSupportedException(pce, field.getType(), ex); + } } public Object convertIfNecessary( @@ -134,6 +138,9 @@ public class DirectFieldAccessor extends AbstractPropertyAccessor { catch (IllegalArgumentException ex) { throw new TypeMismatchException(value, requiredType, ex); } + catch (IllegalStateException ex) { + throw new ConversionNotSupportedException(value, requiredType, ex); + } } } diff --git a/org.springframework.beans/src/main/java/org/springframework/beans/SimpleTypeConverter.java b/org.springframework.beans/src/main/java/org/springframework/beans/SimpleTypeConverter.java index b1143e9d2a2..3747c081f25 100644 --- a/org.springframework.beans/src/main/java/org/springframework/beans/SimpleTypeConverter.java +++ b/org.springframework.beans/src/main/java/org/springframework/beans/SimpleTypeConverter.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2006 the original author or authors. + * Copyright 2002-2009 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. @@ -49,6 +49,9 @@ public class SimpleTypeConverter extends PropertyEditorRegistrySupport implement catch (IllegalArgumentException ex) { throw new TypeMismatchException(value, requiredType, ex); } + catch (IllegalStateException ex) { + throw new ConversionNotSupportedException(value, requiredType, ex); + } } } diff --git a/org.springframework.beans/src/main/java/org/springframework/beans/TypeConverterDelegate.java b/org.springframework.beans/src/main/java/org/springframework/beans/TypeConverterDelegate.java index 4026ee055c0..44e401026e4 100644 --- a/org.springframework.beans/src/main/java/org/springframework/beans/TypeConverterDelegate.java +++ b/org.springframework.beans/src/main/java/org/springframework/beans/TypeConverterDelegate.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2008 the original author or authors. + * Copyright 2002-2009 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. @@ -209,7 +209,7 @@ class TypeConverterDelegate { } if (!ClassUtils.isAssignableValue(requiredType, convertedValue)) { - // Definitely doesn't match: throw IllegalArgumentException. + // Definitely doesn't match: throw IllegalArgumentException/IllegalStateException StringBuilder msg = new StringBuilder(); msg.append("Cannot convert value of type [").append(ClassUtils.getDescriptiveType(newValue)); msg.append("] to required type [").append(ClassUtils.getQualifiedName(requiredType)).append("]"); @@ -219,11 +219,12 @@ class TypeConverterDelegate { if (editor != null) { msg.append(": PropertyEditor [").append(editor.getClass().getName()).append( "] returned inappropriate value"); + throw new IllegalArgumentException(msg.toString()); } else { msg.append(": no matching editors or conversion strategy found"); + throw new IllegalStateException(msg.toString()); } - throw new IllegalArgumentException(msg.toString()); } } diff --git a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/support/DefaultHandlerExceptionResolver.java b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/support/DefaultHandlerExceptionResolver.java index 362294b75d5..b54c40786b0 100644 --- a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/support/DefaultHandlerExceptionResolver.java +++ b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/support/DefaultHandlerExceptionResolver.java @@ -23,6 +23,7 @@ import javax.servlet.http.HttpServletResponse; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.springframework.beans.ConversionNotSupportedException; import org.springframework.beans.TypeMismatchException; import org.springframework.core.Ordered; import org.springframework.http.MediaType; @@ -95,6 +96,9 @@ public class DefaultHandlerExceptionResolver extends AbstractHandlerExceptionRes return handleMissingServletRequestParameter((MissingServletRequestParameterException) ex, request, response, handler); } + else if (ex instanceof ConversionNotSupportedException) { + return handleConversionNotSupported((ConversionNotSupportedException) ex, request, response, handler); + } else if (ex instanceof TypeMismatchException) { return handleTypeMismatch((TypeMismatchException) ex, request, response, handler); } @@ -211,6 +215,27 @@ public class DefaultHandlerExceptionResolver extends AbstractHandlerExceptionRes return new ModelAndView(); } + /** + * Handle the case when a {@link org.springframework.web.bind.WebDataBinder} conversion cannot occur.

The default + * implementation sends an HTTP 500 error, and returns an empty {@code ModelAndView}. Alternatively, a fallback view + * could be chosen, or the TypeMismatchException could be rethrown as-is. + * + * @param ex the ConversionNotSupportedException to be handled + * @param request current HTTP request + * @param response current HTTP response + * @param handler the executed handler, or null if none chosen at the time of the exception (for example, + * if multipart resolution failed) + * @return a ModelAndView to render, or null if handled directly + * @throws Exception an Exception that should be thrown as result of the servlet request + */ + protected ModelAndView handleConversionNotSupported(ConversionNotSupportedException ex, + HttpServletRequest request, + HttpServletResponse response, + Object handler) throws Exception { + response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); + return new ModelAndView(); + } + /** * Handle the case when a {@link org.springframework.web.bind.WebDataBinder} conversion error occurs.

The default * implementation sends an HTTP 400 error, and returns an empty {@code ModelAndView}. Alternatively, a fallback view @@ -228,7 +253,6 @@ public class DefaultHandlerExceptionResolver extends AbstractHandlerExceptionRes HttpServletRequest request, HttpServletResponse response, Object handler) throws Exception { - response.sendError(HttpServletResponse.SC_BAD_REQUEST); return new ModelAndView(); } diff --git a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/ServletAnnotationControllerTests.java b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/ServletAnnotationControllerTests.java index ee297ee1eb5..9620c15b201 100644 --- a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/ServletAnnotationControllerTests.java +++ b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/ServletAnnotationControllerTests.java @@ -17,8 +17,8 @@ package org.springframework.web.servlet.mvc.annotation; import java.io.IOException; -import java.io.Writer; import java.io.Serializable; +import java.io.Writer; import java.lang.annotation.ElementType; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; @@ -106,6 +106,7 @@ import org.springframework.web.util.NestedServletException; /** * @author Juergen Hoeller * @author Sam Brannen + * @author Arjen Poutsma * @since 2.5 */ public class ServletAnnotationControllerTests { diff --git a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/UriTemplateServletAnnotationControllerTests.java b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/UriTemplateServletAnnotationControllerTests.java index 8c3d780f194..5f74887b1a3 100644 --- a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/UriTemplateServletAnnotationControllerTests.java +++ b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/UriTemplateServletAnnotationControllerTests.java @@ -74,7 +74,18 @@ public class UriTemplateServletAnnotationControllerTests { MockHttpServletRequest request = new MockHttpServletRequest("GET", "/hotels/42/dates/2008-11-18"); MockHttpServletResponse response = new MockHttpServletResponse(); servlet.service(request, response); - assertEquals("test-42", response.getContentAsString()); + assertEquals(200, response.getStatus()); + + request = new MockHttpServletRequest("GET", "/hotels/42/dates/2008-foo-bar"); + response = new MockHttpServletResponse(); + servlet.service(request, response); + assertEquals(400, response.getStatus()); + + initServlet(NonBindingUriTemplateController.class); + request = new MockHttpServletRequest("GET", "/hotels/42/dates/2008-foo-bar"); + response = new MockHttpServletResponse(); + servlet.service(request, response); + assertEquals(500, response.getStatus()); } @Test @@ -276,6 +287,16 @@ public class UriTemplateServletAnnotationControllerTests { } + @Controller + public static class NonBindingUriTemplateController { + + @RequestMapping("/hotels/{hotel}/dates/{date}") + public void handle(@PathVariable("hotel") String hotel, @PathVariable Date date, Writer writer) + throws IOException { + } + + } + @Controller @RequestMapping("/hotels/{hotel}") public static class RelativePathUriTemplateController {