Using DataBinder with an auto-growing list is broken [SPR-8828]

[spring-projects-issues]

Konstantin Kolinko opened SPR-8828 and commented

In our web application we use an auto-growing list that grows on get() operation.
We use data binding to map request parameters to fields of a form, and one field contains such a list. For example, a field with name "f[list][0]" will map to the 0th element of the list, if form element getF().get("list") is the list.

The application did work successfully in 3.0.5.RELEASE, but is failing in 3.0.6.RELEASE.

I am attaching JUnit test case that reproduces the issue.

Expected result:

JUnit test passes

Actual result in 3.0.6.RELEASE:

Data binding succeeds, but result validation fails:
java.lang.AssertionError: expected:<firstValue> but was:<null>
at bugreport.GrowingListDataBinderRegression.testDataBinding(GrowingListDataBinderRegression.java:133)

The list contents after data binding was expected to be ["firstElement", "secondElement"],
but the actual contents is [null, "secondElement"]. Both values were assigned to the same position in the list and only the second one survived.

Actual result in 3.1.0.RC1:

Data binding fails:
java.lang.IllegalStateException: Not a collection, array, or map: cannot resolve nested value types

Result of debugging in 3.0.6.RELEASE:

The problem is in class org.springframework.beans.BeanWrapperImpl in method setPropertyValue(PropertyTokenHolder,PropertyValue)

There is the following code in the "if (propValue instanceof List)" branch:

980: int size = list.size();
981: Object oldValue = null;
982: if (isExtractOldValueForEditor() && index < size) {
983: oldValue = list.get(index);
984: }
985: Object convertedValue = convertIfNecessary(...);
986:
987: if (index >= size && index < this.autoGrowCollectionLimit) {
...
999: list.add(convertedValue);
1000: }
1001: else {
...
1003: list.set(index, convertedValue);
...
1009: }

The problem is that list size is calculated on line 980 and remembered and is later used to decide whether to call list.add() or list.set(). If I start with empty list and there is incoming request parameter with name "f[list][0]", the list size on line 981 is evaluated to be zero.

The list.get() call on line 983 is skipped thanks to (&& index < size) check, but on line 985 the convertIfNecessary() call does its own getProperty() call and calls list.get(0) and that creates 0th element of the list. I will cite the stacktrace of that call below.

When we are on line 987 the value of size variable is still 0, but the list already contains one element. Execution continues to line 999, where list.add() is called.
When the method returns, the list contains two elements [null, "firstElement"] instead of just one ["firstElement"].

If anyone is curious, the list.get() call performed inside of convertIfNecessary() has the following stack trace:

Thread [main] (Suspended (breakpoint at line 790 in BeanWrapperImpl))
BeanWrapperImpl.getPropertyValue(BeanWrapperImpl$PropertyTokenHolder) line: 790
BeanWrapperImpl.getPropertyValue(String) line: 718
BeanWrapperImpl.getPropertyType(String) line: 366
BeanWrapperImpl(PropertyEditorRegistrySupport).findCustomEditor(Class, String) line: 341
TypeConverterDelegate.convertIfNecessary(String, Object, Object, Class<T>, TypeDescriptor) line: 134
BeanWrapperImpl.convertIfNecessary(String, Object, Object, Class<?>, TypeDescriptor) line: 466
BeanWrapperImpl.setPropertyValue(BeanWrapperImpl$PropertyTokenHolder, PropertyValue) line: 985

Proposed fix:

Add size = list.size(); call before line 987 in class BeanWrapperImpl.

---

Affects: 3.0.6, 3.1 RC1

Attachments:

• GrowingListDataBinderRegression.java (3.04 kB)

Referenced from: commits 8213d0c, 569426d

Backported to: 3.0.7