From eb50eec85b6e5d053ff39e697328f01ea061b5e1 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Sun, 3 Jul 2011 20:39:19 +0000 Subject: [PATCH] DataBinder uses a default limit of 256 for array/collection auto-growing (SPR-7842) --- .../springframework/beans/BeanWrapper.java | 13 +++- .../beans/BeanWrapperImpl.java | 70 +++++++++++++------ .../beans/BeanWrapperAutoGrowingTests.java | 18 ++++- .../validation/BeanPropertyBindingResult.java | 11 ++- .../validation/DataBinder.java | 32 +++++++-- .../validation/DataBinderTests.java | 60 +++++++++++++++- 6 files changed, 168 insertions(+), 36 deletions(-) diff --git a/org.springframework.beans/src/main/java/org/springframework/beans/BeanWrapper.java b/org.springframework.beans/src/main/java/org/springframework/beans/BeanWrapper.java index 3469d8506ab..aea348d6fdd 100644 --- a/org.springframework.beans/src/main/java/org/springframework/beans/BeanWrapper.java +++ b/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. + *

Default is unlimited on a plain BeanWrapper. + */ + void setAutoGrowCollectionLimit(int autoGrowCollectionLimit); + + /** + * Return the limit for array and collection auto-growing. + */ + int getAutoGrowCollectionLimit(); + } 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 b42d41205db..4be994aa955 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 @@ -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. + *

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. + *

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. + *

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. *

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); diff --git a/org.springframework.beans/src/test/java/org/springframework/beans/BeanWrapperAutoGrowingTests.java b/org.springframework.beans/src/test/java/org/springframework/beans/BeanWrapperAutoGrowingTests.java index 691b89c78d3..b5de8e43960 100644 --- a/org.springframework.beans/src/test/java/org/springframework/beans/BeanWrapperAutoGrowingTests.java +++ b/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]")); diff --git a/org.springframework.context/src/main/java/org/springframework/validation/BeanPropertyBindingResult.java b/org.springframework.context/src/main/java/org/springframework/validation/BeanPropertyBindingResult.java index 3437e672b9f..9daface3422 100644 --- a/org.springframework.context/src/main/java/org/springframework/validation/BeanPropertyBindingResult.java +++ b/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; } diff --git a/org.springframework.context/src/main/java/org/springframework/validation/DataBinder.java b/org.springframework.context/src/main/java/org/springframework/validation/DataBinder.java index 6653905900a..a5d17cfd006 100644 --- a/org.springframework.context/src/main/java/org/springframework/validation/DataBinder.java +++ b/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. + *

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. *

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()); } 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 210944e24ed..8588740ff24 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 @@ -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 {