From 2ad2022058d4a2e3d29ea793ede2cb8c09f9102e Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Mon, 17 May 2010 21:59:02 +0000 Subject: [PATCH] revised BeanWrapper's exception wrapping to consistently handle ConversionExceptions (SPR-7177) --- .../beans/BeanWrapperImpl.java | 218 ++++++++---------- .../beans/DirectFieldAccessor.java | 3 +- .../beans/TypeConverterDelegate.java | 62 +---- .../validation/DataBinderTests.java | 58 +++++ 4 files changed, 156 insertions(+), 185 deletions(-) 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 255ebb435c7..90ae3b5008b 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 @@ -422,8 +422,8 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra return false; } - public T convertIfNecessary( - Object value, Class requiredType, MethodParameter methodParam) throws TypeMismatchException { + public T convertIfNecessary(Object value, Class requiredType, MethodParameter methodParam) + throws TypeMismatchException { try { return this.typeConverterDelegate.convertIfNecessary(value, requiredType, methodParam); } @@ -441,6 +441,39 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra } } + private Object convertIfNecessary(String propertyName, Object oldValue, Object newValue, Class requiredType, + TypeDescriptor td) throws TypeMismatchException { + try { + return this.typeConverterDelegate.convertIfNecessary(propertyName, oldValue, newValue, requiredType, td); + } + catch (ConverterNotFoundException ex) { + PropertyChangeEvent pce = + new PropertyChangeEvent(this.rootObject, this.nestedPath + propertyName, oldValue, newValue); + throw new ConversionNotSupportedException(pce, td.getType(), ex); + } + catch (ConversionException ex) { + PropertyChangeEvent pce = + new PropertyChangeEvent(this.rootObject, this.nestedPath + propertyName, oldValue, newValue); + throw new TypeMismatchException(pce, requiredType, ex); + } + catch (IllegalStateException ex) { + PropertyChangeEvent pce = + new PropertyChangeEvent(this.rootObject, this.nestedPath + propertyName, oldValue, newValue); + throw new ConversionNotSupportedException(pce, requiredType, ex); + } + catch (IllegalArgumentException ex) { + PropertyChangeEvent pce = + new PropertyChangeEvent(this.rootObject, this.nestedPath + propertyName, oldValue, newValue); + throw new TypeMismatchException(pce, requiredType, ex); + } + } + + private Object convertIfNecessary(String propertyName, Object oldValue, Object newValue, Class requiredType) + throws TypeMismatchException { + + return convertIfNecessary(propertyName, oldValue, newValue, requiredType, TypeDescriptor.valueOf(requiredType)); + } + /** * Convert the given value for the specified property to the latter's type. *

This method is only intended for optimizations in a BeanFactory. @@ -457,19 +490,14 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra throw new InvalidPropertyException(getRootClass(), this.nestedPath + propertyName, "No property '" + propertyName + "' found"); } - try { - return this.typeConverterDelegate.convertIfNecessary(null, value, pd); - } - catch (IllegalArgumentException ex) { - PropertyChangeEvent pce = - 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); - } + return convertForProperty(propertyName, null, value, pd); + } + + private Object convertForProperty(String propertyName, Object oldValue, Object newValue, PropertyDescriptor pd) + throws TypeMismatchException { + + return convertIfNecessary(propertyName, oldValue, newValue, pd.getPropertyType(), + new PropertyTypeDescriptor(pd, BeanUtils.getWriteMethodParameter(pd))); } @@ -690,7 +718,6 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra } Object value; - if (System.getSecurityManager() != null) { try { value = AccessController.doPrivileged(new PrivilegedExceptionAction() { @@ -704,7 +731,7 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra } } else { - value = readMethod.invoke(object, (Object[]) null); + value = readMethod.invoke(object, (Object[]) null); } if (tokens.keys != null) { @@ -761,7 +788,7 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra Class mapKeyType = GenericCollectionTypeResolver.getMapKeyReturnType(pd.getReadMethod(), i + 1); // IMPORTANT: Do not pass full property name in here - property editors // must not kick in for map keys but rather only for map values. - Object convertedMapKey = this.typeConverterDelegate.convertIfNecessary(key, mapKeyType); + Object convertedMapKey = convertIfNecessary(null, null, key, mapKeyType); // Pass full property name and old value in here, since we want full // conversion ability for map values. value = map.get(convertedMapKey); @@ -776,15 +803,6 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra } return value; } - catch (InvocationTargetException ex) { - throw new InvalidPropertyException(getRootClass(), this.nestedPath + propertyName, - "Getter for property '" + actualName + "' threw exception", ex); - } - - catch (IllegalAccessException ex) { - throw new InvalidPropertyException(getRootClass(), this.nestedPath + propertyName, - "Illegal attempt to get property '" + actualName + "' threw exception", ex); - } catch (IndexOutOfBoundsException ex) { throw new InvalidPropertyException(getRootClass(), this.nestedPath + propertyName, "Index of out of bounds in property path '" + propertyName + "'", ex); @@ -793,10 +811,18 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra throw new InvalidPropertyException(getRootClass(), this.nestedPath + propertyName, "Invalid index in property path '" + propertyName + "'", ex); } - catch (Exception ex) { + catch (TypeMismatchException ex) { throw new InvalidPropertyException(getRootClass(), this.nestedPath + propertyName, "Invalid index in property path '" + propertyName + "'", ex); } + catch (InvocationTargetException ex) { + throw new InvalidPropertyException(getRootClass(), this.nestedPath + propertyName, + "Getter for property '" + actualName + "' threw exception", ex); + } + catch (Exception ex) { + throw new InvalidPropertyException(getRootClass(), this.nestedPath + propertyName, + "Illegal attempt to get property '" + actualName + "' threw exception", ex); + } } private Object growArrayIfNecessary(Object array, int index, String name) { @@ -819,7 +845,10 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra } } - private void growCollectionIfNecessary(Collection collection, int index, String name, PropertyDescriptor pd, int nestingLevel) { + @SuppressWarnings("unchecked") + private void growCollectionIfNecessary( + Collection collection, int index, String name, PropertyDescriptor pd, int nestingLevel) { + if (!this.autoGrowNestedPaths) { return; } @@ -907,19 +936,8 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra if (isExtractOldValueForEditor()) { oldValue = Array.get(propValue, arrayIndex); } - Object convertedValue = this.typeConverterDelegate.convertIfNecessary( - propertyName, oldValue, pv.getValue(), requiredType); - Array.set(propValue, Integer.parseInt(key), convertedValue); - } - catch (IllegalArgumentException ex) { - PropertyChangeEvent pce = - new PropertyChangeEvent(this.rootObject, this.nestedPath + propertyName, oldValue, pv.getValue()); - throw new TypeMismatchException(pce, requiredType, ex); - } - catch (IllegalStateException ex) { - PropertyChangeEvent pce = - new PropertyChangeEvent(this.rootObject, this.nestedPath + propertyName, oldValue, pv.getValue()); - throw new ConversionNotSupportedException(pce, requiredType, ex); + Object convertedValue = convertIfNecessary(propertyName, oldValue, pv.getValue(), requiredType); + Array.set(propValue, arrayIndex, convertedValue); } catch (IndexOutOfBoundsException ex) { throw new InvalidPropertyException(getRootClass(), this.nestedPath + propertyName, @@ -936,31 +954,23 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra if (isExtractOldValueForEditor() && index < list.size()) { oldValue = list.get(index); } - try { - Object convertedValue = this.typeConverterDelegate.convertIfNecessary( - propertyName, oldValue, pv.getValue(), requiredType); - if (index < list.size()) { - list.set(index, convertedValue); - } - else if (index >= list.size()) { - for (int i = list.size(); i < index; i++) { - try { - list.add(null); - } - catch (NullPointerException ex) { - throw new InvalidPropertyException(getRootClass(), this.nestedPath + propertyName, - "Cannot set element with index " + index + " in List of size " + - list.size() + ", accessed using property path '" + propertyName + - "': List does not support filling up gaps with null elements"); - } - } - list.add(convertedValue); - } + Object convertedValue = convertIfNecessary(propertyName, oldValue, pv.getValue(), requiredType); + if (index < list.size()) { + list.set(index, convertedValue); } - catch (IllegalArgumentException ex) { - PropertyChangeEvent pce = - new PropertyChangeEvent(this.rootObject, this.nestedPath + propertyName, oldValue, pv.getValue()); - throw new TypeMismatchException(pce, requiredType, ex); + else if (index >= list.size()) { + for (int i = list.size(); i < index; i++) { + try { + list.add(null); + } + catch (NullPointerException ex) { + throw new InvalidPropertyException(getRootClass(), this.nestedPath + propertyName, + "Cannot set element with index " + index + " in List of size " + + list.size() + ", accessed using property path '" + propertyName + + "': List does not support filling up gaps with null elements"); + } + } + list.add(convertedValue); } } else if (propValue instanceof Map) { @@ -970,34 +980,18 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra Class mapValueType = GenericCollectionTypeResolver.getMapValueReturnType( pd.getReadMethod(), tokens.keys.length); Map map = (Map) propValue; - Object convertedMapKey; - Object convertedMapValue; - try { - // IMPORTANT: Do not pass full property name in here - property editors - // must not kick in for map keys but rather only for map values. - convertedMapKey = this.typeConverterDelegate.convertIfNecessary(key, mapKeyType); - } - catch (IllegalArgumentException ex) { - PropertyChangeEvent pce = - new PropertyChangeEvent(this.rootObject, this.nestedPath + propertyName, null, pv.getValue()); - throw new TypeMismatchException(pce, mapKeyType, ex); - } + // IMPORTANT: Do not pass full property name in here - property editors + // must not kick in for map keys but rather only for map values. + Object convertedMapKey = convertIfNecessary(null, null, key, mapKeyType); Object oldValue = null; if (isExtractOldValueForEditor()) { oldValue = map.get(convertedMapKey); } - try { - // Pass full property name and old value in here, since we want full - // conversion ability for map values. - convertedMapValue = this.typeConverterDelegate.convertIfNecessary( - propertyName, oldValue, pv.getValue(), mapValueType, - new MethodParameter(pd.getReadMethod(), -1, tokens.keys.length + 1)); - } - catch (IllegalArgumentException ex) { - PropertyChangeEvent pce = - new PropertyChangeEvent(this.rootObject, this.nestedPath + propertyName, oldValue, pv.getValue()); - throw new TypeMismatchException(pce, mapValueType, ex); - } + // Pass full property name and old value in here, since we want full + // conversion ability for map values. + Object convertedMapValue = convertIfNecessary( + propertyName, oldValue, pv.getValue(), mapValueType, + new TypeDescriptor(new MethodParameter(pd.getReadMethod(), -1, tokens.keys.length + 1))); map.put(convertedMapKey, convertedMapValue); } else { @@ -1038,7 +1032,8 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra else { if (isExtractOldValueForEditor() && pd.getReadMethod() != null) { final Method readMethod = pd.getReadMethod(); - if (!Modifier.isPublic(readMethod.getDeclaringClass().getModifiers()) && !readMethod.isAccessible()) { + if (!Modifier.isPublic(readMethod.getDeclaringClass().getModifiers()) && + !readMethod.isAccessible()) { if (System.getSecurityManager()!= null) { AccessController.doPrivileged(new PrivilegedAction() { public Object run() { @@ -1057,7 +1052,7 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra public Object run() throws Exception { return readMethod.invoke(object); } - },acc); + }, acc); } else { oldValue = readMethod.invoke(object); @@ -1073,7 +1068,7 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra } } } - valueToApply = this.typeConverterDelegate.convertIfNecessary(oldValue, originalValue, pd); + valueToApply = convertForProperty(propertyName, oldValue, originalValue, pd); } pv.getOriginalPropertyValue().conversionNecessary = (valueToApply != originalValue); } @@ -1094,7 +1089,6 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra } } final Object value = valueToApply; - if (System.getSecurityManager() != null) { try { AccessController.doPrivileged(new PrivilegedExceptionAction() { @@ -1103,14 +1097,17 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra return null; } }, acc); - } catch (PrivilegedActionException ex) { + } + catch (PrivilegedActionException ex) { throw ex.getException(); } } else { - writeMethod.invoke(object, value); + writeMethod.invoke(this.object, value); } - + } + catch (TypeMismatchException ex) { + throw ex; } catch (InvocationTargetException ex) { PropertyChangeEvent propertyChangeEvent = @@ -1122,34 +1119,9 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra throw new MethodInvocationException(propertyChangeEvent, ex.getTargetException()); } } - catch (ConverterNotFoundException ex) { - PropertyChangeEvent pce = - new PropertyChangeEvent(this.rootObject, this.nestedPath + propertyName, oldValue, pv.getValue()); - throw new ConversionNotSupportedException(pce, pd.getPropertyType(), ex); - } - catch (ConversionException ex) { - PropertyChangeEvent pce = - 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 (IllegalArgumentException ex) { - PropertyChangeEvent pce = - new PropertyChangeEvent(this.rootObject, this.nestedPath + propertyName, oldValue, pv.getValue()); - throw new TypeMismatchException(pce, pd.getPropertyType(), ex); - } - catch (IllegalAccessException ex) { - PropertyChangeEvent pce = - new PropertyChangeEvent(this.rootObject, this.nestedPath + propertyName, oldValue, pv.getValue()); - throw new MethodInvocationException(pce, ex); - } catch (Exception ex) { PropertyChangeEvent pce = - new PropertyChangeEvent(this.rootObject, this.nestedPath + propertyName, oldValue, pv.getValue()); + new PropertyChangeEvent(this.rootObject, this.nestedPath + propertyName, oldValue, pv.getValue()); throw new MethodInvocationException(pce, ex); } } 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 04425df2115..891f085cd2b 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 @@ -124,7 +124,8 @@ public class DirectFieldAccessor extends AbstractPropertyAccessor { try { ReflectionUtils.makeAccessible(field); oldValue = field.get(this.target); - Object convertedValue = this.typeConverterDelegate.convertIfNecessary(oldValue, newValue, field); + Object convertedValue = this.typeConverterDelegate.convertIfNecessary( + field.getName(), oldValue, newValue, field.getType(), new TypeDescriptor(field)); field.set(this.target, convertedValue); } catch (ConverterNotFoundException 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 ab6781101b2..dedda9eb1f1 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 @@ -77,18 +77,6 @@ class TypeConverterDelegate { } - /** - * Convert the value to the specified required type. - * @param newValue the proposed new value - * @param requiredType the type we must convert to - * (or null if not known, for example in case of a collection element) - * @return the new value, possibly the result of type conversion - * @throws IllegalArgumentException if type conversion failed - */ - public T convertIfNecessary(Object newValue, Class requiredType) throws IllegalArgumentException { - return convertIfNecessary(null, null, newValue, requiredType, TypeDescriptor.valueOf(requiredType)); - } - /** * Convert the value to the specified required type. * @param newValue the proposed new value @@ -123,54 +111,6 @@ class TypeConverterDelegate { return convertIfNecessary(propertyName, oldValue, newValue, requiredType, TypeDescriptor.valueOf(requiredType)); } - /** - * Convert the value to the required type (if necessary from a String), - * for the specified property. - * @param propertyName name of the property - * @param oldValue the previous value, if available (may be null) - * @param newValue the proposed new value - * @param requiredType the type we must convert to - * (or null if not known, for example in case of a collection element) - * @param methodParam the method parameter that is the target of the conversion - * @return the new value, possibly the result of type conversion - * @throws IllegalArgumentException if type conversion failed - */ - public T convertIfNecessary(String propertyName, Object oldValue, Object newValue, - Class requiredType, MethodParameter methodParam) throws IllegalArgumentException { - - return convertIfNecessary(propertyName, oldValue, newValue, requiredType, new TypeDescriptor(methodParam)); - } - - /** - * Convert the value to the required type for the specified property. - * @param oldValue the previous value, if available (may be null) - * @param newValue the proposed new value - * @param descriptor the JavaBeans descriptor for the property - * @return the new value, possibly the result of type conversion - * @throws IllegalArgumentException if type conversion failed - */ - public Object convertIfNecessary(Object oldValue, Object newValue, PropertyDescriptor descriptor) - throws IllegalArgumentException { - - return convertIfNecessary(descriptor.getName(), oldValue, newValue, descriptor.getPropertyType(), - new PropertyTypeDescriptor(descriptor, BeanUtils.getWriteMethodParameter(descriptor))); - } - - /** - * Convert the value to the required type for the specified property. - * @param oldValue the previous value, if available (may be null) - * @param newValue the proposed new value - * @param field the field that is the target of the conversion - * @return the new value, possibly the result of type conversion - * @throws IllegalArgumentException if type conversion failed - */ - public Object convertIfNecessary(Object oldValue, Object newValue, Field field) - throws IllegalArgumentException { - - return convertIfNecessary(field.getName(), oldValue, newValue, field.getType(), new TypeDescriptor(field)); - } - - /** * Convert the value to the required type (if necessary from a String), * for the specified property. @@ -184,7 +124,7 @@ class TypeConverterDelegate { * @throws IllegalArgumentException if type conversion failed */ @SuppressWarnings("unchecked") - private T convertIfNecessary(String propertyName, Object oldValue, Object newValue, + public T convertIfNecessary(String propertyName, Object oldValue, Object newValue, Class requiredType, TypeDescriptor typeDescriptor) throws IllegalArgumentException { Object convertedValue = newValue; diff --git a/org.springframework.context/src/test/java/org/springframework/validation/DataBinderTests.java b/org.springframework.context/src/test/java/org/springframework/validation/DataBinderTests.java index baba01a67e7..c6b662fe8e5 100644 --- a/org.springframework.context/src/test/java/org/springframework/validation/DataBinderTests.java +++ b/org.springframework.context/src/test/java/org/springframework/validation/DataBinderTests.java @@ -22,6 +22,7 @@ import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; import java.io.ObjectInputStream; import java.io.ObjectOutputStream; +import java.util.List; import java.util.Locale; import java.util.Map; import java.util.Set; @@ -349,6 +350,49 @@ public class DataBinderTests extends TestCase { } } + public void testBindingWithFormatterAgainstList() { + BeanWithIntegerList tb = new BeanWithIntegerList(); + DataBinder binder = new DataBinder(tb); + FormattingConversionService conversionService = new FormattingConversionService(); + ConversionServiceFactory.addDefaultConverters(conversionService); + conversionService.addFormatterForFieldType(Float.class, new NumberFormatter()); + binder.setConversionService(conversionService); + MutablePropertyValues pvs = new MutablePropertyValues(); + pvs.add("integerList[0]", "1"); + + LocaleContextHolder.setLocale(Locale.GERMAN); + try { + binder.bind(pvs); + assertEquals(new Integer(1), tb.getIntegerList().get(0)); + assertEquals("1", binder.getBindingResult().getFieldValue("integerList[0]")); + } + finally { + LocaleContextHolder.resetLocaleContext(); + } + } + + public void testBindingErrorWithFormatterAgainstList() { + BeanWithIntegerList tb = new BeanWithIntegerList(); + DataBinder binder = new DataBinder(tb); + FormattingConversionService conversionService = new FormattingConversionService(); + ConversionServiceFactory.addDefaultConverters(conversionService); + conversionService.addFormatterForFieldType(Float.class, new NumberFormatter()); + binder.setConversionService(conversionService); + MutablePropertyValues pvs = new MutablePropertyValues(); + pvs.add("integerList[0]", "1x2"); + + LocaleContextHolder.setLocale(Locale.GERMAN); + try { + binder.bind(pvs); + assertTrue(tb.getIntegerList().isEmpty()); + assertEquals("1x2", binder.getBindingResult().getFieldValue("integerList[0]")); + assertTrue(binder.getBindingResult().hasFieldErrors("integerList[0]")); + } + finally { + LocaleContextHolder.resetLocaleContext(); + } + } + public void testBindingWithFormatterAgainstFields() { TestBean tb = new TestBean(); DataBinder binder = new DataBinder(tb); @@ -1436,6 +1480,20 @@ public class DataBinderTests extends TestCase { } + private static class BeanWithIntegerList { + + private List integerList; + + public List getIntegerList() { + return integerList; + } + + public void setIntegerList(List integerList) { + this.integerList = integerList; + } + } + + private static class Book { private String Title;