DataBinder uses a default limit of 256 for array/collection auto-growing (SPR-7842)

org.springframework.beans/src/main/java/org/springframework/beans/BeanWrapper.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2009 the original author or authors.
+ * Copyright 2002-2011 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.
@@ -92,4 +92,15 @@ public interface BeanWrapper extends ConfigurablePropertyAccessor {
 	 */
 	boolean isAutoGrowNestedPaths();
 
+	/**
+	 * Specify a limit for array and collection auto-growing.
+	 * <p>Default is unlimited on a plain BeanWrapper.
+	 */
+	void setAutoGrowCollectionLimit(int autoGrowCollectionLimit);
+
+	/**
+	 * Return the limit for array and collection auto-growing.
+	 */
+	int getAutoGrowCollectionLimit();
+
 }

org.springframework.beans/src/main/java/org/springframework/beans/BeanWrapperImpl.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2010 the original author or authors.
+ * Copyright 2002-2011 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.
@@ -120,6 +120,8 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra
 
 	private boolean autoGrowNestedPaths = false;
 
+	private int autoGrowCollectionLimit = Integer.MAX_VALUE;
+
 
 	/**
 	 * Create new empty BeanWrapperImpl. Wrapped instance needs to be set afterwards.
@@ -184,6 +186,7 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra
 		setWrappedInstance(object, nestedPath, superBw.getWrappedInstance());
 		setExtractOldValueForEditor(superBw.isExtractOldValueForEditor());
 		setAutoGrowNestedPaths(superBw.isAutoGrowNestedPaths());
+		setAutoGrowCollectionLimit(superBw.getAutoGrowCollectionLimit());
 		setConversionService(superBw.getConversionService());
 		setSecurityContext(superBw.acc);
 	}
@@ -251,22 +254,38 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra
 	}
 
 	/**
-	 * If this BeanWrapper should "auto grow" nested paths.
-	 * When true, auto growth is triggered on nested paths when null values are encountered.
-	 * When true, auto growth is triggered on collection properties when out of bounds indexes are accessed.
-	 * Default is false.
+	 * Set whether this BeanWrapper should attempt to "auto-grow" a nested path that contains a null value.
+	 * <p>If "true", a null path location will be populated with a default object value and traversed
+	 * instead of resulting in a {@link NullValueInNestedPathException}. Turning this flag on also
+	 * enables auto-growth of collection elements when accessing an out-of-bounds index.
+	 * <p>Default is "false" on a plain BeanWrapper.
 	 */
 	public void setAutoGrowNestedPaths(boolean autoGrowNestedPaths) {
 		this.autoGrowNestedPaths = autoGrowNestedPaths;
 	}
 
 	/**
-	 * If this BeanWrapper should "auto grow" nested paths.
+	 * Return whether "auto-growing" of nested paths has been activated.
 	 */
 	public boolean isAutoGrowNestedPaths() {
 		return this.autoGrowNestedPaths;
 	}
 
+	/**
+	 * Specify a limit for array and collection auto-growing.
+	 * <p>Default is unlimited on a plain BeanWrapper.
+	 */
+	public void setAutoGrowCollectionLimit(int autoGrowCollectionLimit) {
+		this.autoGrowCollectionLimit = autoGrowCollectionLimit;
+	}
+
+	/**
+	 * Return the limit for array and collection auto-growing.
+	 */
+	public int getAutoGrowCollectionLimit() {
+		return this.autoGrowCollectionLimit;
+	}
+
 	/**
 	 * Set the security context used during the invocation of the wrapped instance methods.
 	 * Can be null.
@@ -594,7 +613,7 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra
 		setPropertyValue(tokens, pv);
 		return pv.getValue();
 	}
-	
+
 	private PropertyValue createDefaultPropertyValue(PropertyTokenHolder tokens) {
 		Class type = getPropertyType(tokens.canonicalName);
 		if (type == null) {
@@ -604,7 +623,7 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra
 		Object defaultValue = newValue(type, tokens.canonicalName);
 		return new PropertyValue(tokens.canonicalName, defaultValue);
 	}
-	
+
 	private Object newValue(Class<?> type, String name) {
 		try {
 			if (type.isArray()) {
@@ -634,7 +653,7 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra
 					"Could not instantiate property type [" + type.getName() + "] to auto-grow nested property path: " + ex);
 		}
 	}
-	
+
 	/**
 	 * Create a new nested BeanWrapper instance.
 	 * <p>Default implementation creates a BeanWrapperImpl instance.
@@ -834,15 +853,16 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra
 			return array;
 		}
 		int length = Array.getLength(array);
-		if (index >= length) {
+		if (index >= length && index < this.autoGrowCollectionLimit) {
 			Class<?> componentType = array.getClass().getComponentType();
 			Object newArray = Array.newInstance(componentType, index + 1);
 			System.arraycopy(array, 0, newArray, 0, length);
 			for (int i = length; i < Array.getLength(newArray); i++) {
 				Array.set(newArray, i, newValue(componentType, name));
 			}
+			// TODO this is not efficient because conversion may create a copy ... set directly because we know it is assignable.
 			setPropertyValue(name, newArray);
-			return newArray;
+			return getPropertyValue(name);
 		}
 		else {
 			return array;
@@ -856,7 +876,8 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra
 		if (!this.autoGrowNestedPaths) {
 			return;
 		}
-		if (index >= collection.size()) {
+		int size = collection.size();
+		if (index >= size && index < this.autoGrowCollectionLimit) {
 			Class elementType = GenericCollectionTypeResolver.getCollectionReturnType(pd.getReadMethod(), nestingLevel);
 			if (elementType != null) {
 				for (int i = collection.size(); i < index + 1; i++) {
@@ -932,13 +953,13 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra
 						"Cannot access indexed value in property referenced " +
 						"in indexed property path '" + propertyName + "': returned null");
 			}
-			else if (propValue.getClass().isArray()) {
+			if (propValue.getClass().isArray()) {
 				PropertyDescriptor pd = getCachedIntrospectionResults().getPropertyDescriptor(actualName);
 				Class requiredType = propValue.getClass().getComponentType();
 				int arrayIndex = Integer.parseInt(key);
 				Object oldValue = null;
 				try {
-					if (isExtractOldValueForEditor()) {
+					if (isExtractOldValueForEditor() && arrayIndex < Array.getLength(propValue)) {
 						oldValue = Array.get(propValue, arrayIndex);
 					}
 					Object convertedValue = convertIfNecessary(propertyName, oldValue, pv.getValue(), requiredType,
@@ -956,29 +977,36 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra
 						pd.getReadMethod(), tokens.keys.length);
 				List list = (List) propValue;
 				int index = Integer.parseInt(key);
+				int size = list.size();
 				Object oldValue = null;
-				if (isExtractOldValueForEditor() && index < list.size()) {
+				if (isExtractOldValueForEditor() && index < size) {
 					oldValue = list.get(index);
 				}
 				Object convertedValue = convertIfNecessary(propertyName, oldValue, pv.getValue(), requiredType,
 						new PropertyTypeDescriptor(pd, new MethodParameter(pd.getReadMethod(), -1), requiredType));
-				if (index < list.size()) {
-					list.set(index, convertedValue);
-				}
-				else if (index >= list.size()) {
-					for (int i = list.size(); i < index; i++) {
+				if (index >= size && index < this.autoGrowCollectionLimit) {
+					for (int i = 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 +
+									size + ", accessed using property path '" + propertyName +
 									"': List does not support filling up gaps with null elements");
 						}
 					}
 					list.add(convertedValue);
 				}
+				else {
+					try {
+						list.set(index, convertedValue);
+					}
+					catch (IndexOutOfBoundsException ex) {
+						throw new InvalidPropertyException(getRootClass(), this.nestedPath + propertyName,
+								"Invalid list index in property path '" + propertyName + "'", ex);
+					}
+				}
 			}
 			else if (propValue instanceof Map) {
 				PropertyDescriptor pd = getCachedIntrospectionResults().getPropertyDescriptor(actualName);

org.springframework.beans/src/test/java/org/springframework/beans/BeanWrapperAutoGrowingTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2010 the original author or authors.
+ * Copyright 2002-2011 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,10 +19,11 @@ package org.springframework.beans;
 import java.util.List;
 import java.util.Map;
 
-import static org.junit.Assert.*;
 import org.junit.Before;
 import org.junit.Test;
 
+import static org.junit.Assert.*;
+
 /**
  * @author Keith Donald
  * @author Juergen Hoeller
@@ -117,6 +118,19 @@ public class BeanWrapperAutoGrowingTests {
 		assertNotNull(wrapper.getPropertyValue("list[3]"));
 	}
 
+	@Test
+	public void getPropertyValueAutoGrowListFailsAgainstLimit() {
+		wrapper.setAutoGrowCollectionLimit(2);
+		try {
+			assertNotNull(wrapper.getPropertyValue("list[4]"));
+			fail("Should have thrown InvalidPropertyException");
+		}
+		catch (InvalidPropertyException ex) {
+			// expected
+			assertTrue(ex.getRootCause() instanceof IndexOutOfBoundsException);
+		}
+	}
+
 	@Test
 	public void getPropertyValueAutoGrowMultiDimensionalList() {
 		assertNotNull(wrapper.getPropertyValue("multiList[0][0]"));

org.springframework.context/src/main/java/org/springframework/validation/BeanPropertyBindingResult.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2009 the original author or authors.
+ * Copyright 2002-2011 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.
@@ -46,6 +46,8 @@ public class BeanPropertyBindingResult extends AbstractPropertyBindingResult imp
 
 	private final boolean autoGrowNestedPaths;
 
+	private final int autoGrowCollectionLimit;
+
 	private transient BeanWrapper beanWrapper;
 
 
@@ -55,7 +57,7 @@ public class BeanPropertyBindingResult extends AbstractPropertyBindingResult imp
 	 * @param objectName the name of the target object
 	 */
 	public BeanPropertyBindingResult(Object target, String objectName) {
-		this(target, objectName, true);
+		this(target, objectName, true, Integer.MAX_VALUE);
 	}
 
 	/**
@@ -63,11 +65,13 @@ public class BeanPropertyBindingResult extends AbstractPropertyBindingResult imp
 	 * @param target the target bean to bind onto
 	 * @param objectName the name of the target object
 	 * @param autoGrowNestedPaths whether to "auto-grow" a nested path that contains a null value
+	 * @param autoGrowCollectionLimit the limit for array and collection auto-growing
 	 */
-	public BeanPropertyBindingResult(Object target, String objectName, boolean autoGrowNestedPaths) {
+	public BeanPropertyBindingResult(Object target, String objectName, boolean autoGrowNestedPaths, int autoGrowCollectionLimit) {
 		super(objectName);
 		this.target = target;
 		this.autoGrowNestedPaths = autoGrowNestedPaths;
+		this.autoGrowCollectionLimit = autoGrowCollectionLimit;
 	}
 
 
@@ -87,6 +91,7 @@ public class BeanPropertyBindingResult extends AbstractPropertyBindingResult imp
 			this.beanWrapper = createBeanWrapper();
 			this.beanWrapper.setExtractOldValueForEditor(true);
 			this.beanWrapper.setAutoGrowNestedPaths(this.autoGrowNestedPaths);
+			this.beanWrapper.setAutoGrowCollectionLimit(this.autoGrowCollectionLimit);
 		}
 		return this.beanWrapper;
 	}

org.springframework.context/src/main/java/org/springframework/validation/DataBinder.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2010 the original author or authors.
+ * Copyright 2002-2011 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.
@@ -105,6 +105,9 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter {
 	/** Default object name used for binding: "target" */
 	public static final String DEFAULT_OBJECT_NAME = "target";
 
+	/** Default limit for array and collection growing: 256 */
+	public static final int DEFAULT_AUTO_GROW_COLLECTION_LIMIT = 256;
+
 
 	/**
 	 * We'll create a lot of DataBinder instances: Let's use a static logger.
@@ -127,6 +130,8 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter {
 
 	private boolean autoGrowNestedPaths = true;
 
+	private int autoGrowCollectionLimit = DEFAULT_AUTO_GROW_COLLECTION_LIMIT;
+
 	private String[] allowedFields;
 
 	private String[] disallowedFields;
@@ -199,6 +204,22 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter {
 		return this.autoGrowNestedPaths;
 	}
 
+	/**
+	 * Specify the limit for array and collection auto-growing.
+	 * <p>Default is 256, preventing OutOfMemoryErrors in case of large indexes.
+	 * Raise this limit if your auto-growing needs are unusually high.
+	 */
+	public void setAutoGrowCollectionLimit(int autoGrowCollectionLimit) {
+		this.autoGrowCollectionLimit = autoGrowCollectionLimit;
+	}
+
+	/**
+	 * Return the current limit for array and collection auto-growing.
+	 */
+	public int getAutoGrowCollectionLimit() {
+		return this.autoGrowCollectionLimit;
+	}
+
 	/**
 	 * Initialize standard JavaBean property access for this DataBinder.
 	 * <p>This is the default; an explicit call just leads to eager initialization.
@@ -207,7 +228,8 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter {
 	public void initBeanPropertyAccess() {
 		Assert.state(this.bindingResult == null,
 				"DataBinder is already initialized - call initBeanPropertyAccess before other configuration methods");
-		this.bindingResult = new BeanPropertyBindingResult(getTarget(), getObjectName(), isAutoGrowNestedPaths());
+		this.bindingResult = new BeanPropertyBindingResult(
+				getTarget(), getObjectName(), isAutoGrowNestedPaths(), getAutoGrowCollectionLimit());
 		if (this.conversionService != null) {
 			this.bindingResult.initConversion(this.conversionService);
 		}
@@ -509,17 +531,14 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter {
 		return this.conversionService;
 	}
 
-	@SuppressWarnings("unchecked")
 	public void registerCustomEditor(Class requiredType, PropertyEditor propertyEditor) {
 		getPropertyEditorRegistry().registerCustomEditor(requiredType, propertyEditor);
 	}
 
-	@SuppressWarnings("unchecked")
 	public void registerCustomEditor(Class requiredType, String field, PropertyEditor propertyEditor) {
 		getPropertyEditorRegistry().registerCustomEditor(requiredType, field, propertyEditor);
 	}
 
-	@SuppressWarnings("unchecked")
 	public PropertyEditor findCustomEditor(Class requiredType, String propertyPath) {
 		return getPropertyEditorRegistry().findCustomEditor(requiredType, propertyPath);
 	}
@@ -700,8 +719,7 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter {
 	 * @throws BindException if there were any errors in the bind operation
 	 * @see BindingResult#getModel()
 	 */
-	@SuppressWarnings("unchecked")
-	public Map close() throws BindException {
+	public Map<?, ?> close() throws BindException {
 		if (getBindingResult().hasErrors()) {
 			throw new BindException(getBindingResult());
 		}

org.springframework.context/src/test/java/org/springframework/validation/DataBinderTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2010 the original author or authors.
+ * Copyright 2002-2011 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.
@@ -34,6 +34,7 @@ import org.springframework.beans.BeanWithObjectProperty;
 import org.springframework.beans.DerivedTestBean;
 import org.springframework.beans.ITestBean;
 import org.springframework.beans.IndexedTestBean;
+import org.springframework.beans.InvalidPropertyException;
 import org.springframework.beans.MutablePropertyValues;
 import org.springframework.beans.NotWritablePropertyException;
 import org.springframework.beans.NullValueInNestedPathException;
@@ -1486,7 +1487,6 @@ public class DataBinderTests extends TestCase {
 		MutablePropertyValues mpvs = new MutablePropertyValues();
 		mpvs.add("name", name);
 		mpvs.add("beanName", beanName);
-
 		binder.bind(mpvs);
 
 		assertEquals(name, testBean.getName());
@@ -1495,6 +1495,62 @@ public class DataBinderTests extends TestCase {
 		assertEquals("beanName", disallowedFields[0]);
 	}
 
+	public void testAutoGrowWithinDefaultLimit() throws Exception {
+		TestBean testBean = new TestBean();
+		DataBinder binder = new DataBinder(testBean, "testBean");
+
+		MutablePropertyValues mpvs = new MutablePropertyValues();
+		mpvs.add("friends[4]", "");
+		binder.bind(mpvs);
+
+		assertEquals(5, testBean.getFriends().size());
+	}
+
+	public void testAutoGrowBeyondDefaultLimit() throws Exception {
+		TestBean testBean = new TestBean();
+		DataBinder binder = new DataBinder(testBean, "testBean");
+
+		MutablePropertyValues mpvs = new MutablePropertyValues();
+		mpvs.add("friends[256]", "");
+		try {
+			binder.bind(mpvs);
+			fail("Should have thrown InvalidPropertyException");
+		}
+		catch (InvalidPropertyException ex) {
+			// expected
+			assertTrue(ex.getRootCause() instanceof IndexOutOfBoundsException);
+		}
+	}
+
+	public void testAutoGrowWithinCustomLimit() throws Exception {
+		TestBean testBean = new TestBean();
+		DataBinder binder = new DataBinder(testBean, "testBean");
+		binder.setAutoGrowCollectionLimit(10);
+
+		MutablePropertyValues mpvs = new MutablePropertyValues();
+		mpvs.add("friends[4]", "");
+		binder.bind(mpvs);
+
+		assertEquals(5, testBean.getFriends().size());
+	}
+
+	public void testAutoGrowBeyondCustomLimit() throws Exception {
+		TestBean testBean = new TestBean();
+		DataBinder binder = new DataBinder(testBean, "testBean");
+		binder.setAutoGrowCollectionLimit(10);
+
+		MutablePropertyValues mpvs = new MutablePropertyValues();
+		mpvs.add("friends[16]", "");
+		try {
+			binder.bind(mpvs);
+			fail("Should have thrown InvalidPropertyException");
+		}
+		catch (InvalidPropertyException ex) {
+			// expected
+			assertTrue(ex.getRootCause() instanceof IndexOutOfBoundsException);
+		}
+	}
+
 
 	private static class BeanWithIntegerList {