Reject invalid databinding indexes

grails-databinding-core/src/main/groovy/grails/databinding/SimpleDataBinder.groovy

@@ -333,13 +333,19 @@ class SimpleDataBinder implements DataBinder {
         }
 
         if (propertyType.isArray()) {
-            def index = Integer.parseInt(indexedPropertyReferenceDescriptor.index)
+            Integer index = parseIndexedPropertyIndex(obj, indexedPropertyReferenceDescriptor, val, listener, errors)
+            if (index == null) {
+                return
+            }
             def array = initializeArray(obj, propName, propertyType.componentType, index)
             if (array != null) {
                 addElementToArrayAt(array, index, val)
             }
         } else if (Collection.isAssignableFrom(propertyType)) {
-            def index = Integer.parseInt(indexedPropertyReferenceDescriptor.index)
+            Integer index = parseIndexedPropertyIndex(obj, indexedPropertyReferenceDescriptor, val, listener, errors)
+            if (index == null) {
+                return
+            }
             Collection collectionInstance = initializeCollection(obj, propName, propertyType)
             def indexedInstance = null
             if (!(Set.isAssignableFrom(propertyType))) {
@@ -394,6 +400,22 @@ class SimpleDataBinder implements DataBinder {
         }
     }
 
+    protected Integer parseIndexedPropertyIndex(obj, IndexedPropertyReferenceDescriptor indexedPropertyReferenceDescriptor,
+        val, DataBindingListener listener, errors) {
+
+        try {
+            Integer index = Integer.parseInt(indexedPropertyReferenceDescriptor.index)
+            if (index < 0) {
+                throw new NumberFormatException(indexedPropertyReferenceDescriptor.index)
+            }
+            index
+        }
+        catch (NumberFormatException e) {
+            addBindingError(obj, indexedPropertyReferenceDescriptor.toString(), val, e, listener, errors)
+            null
+        }
+    }
+
     @CompileStatic(TypeCheckingMode.SKIP)
     protected initializeArray(obj, String propertyName, Class arrayType, int index) {
         Object[] array = obj[propertyName]

grails-databinding-core/src/test/groovy/grails/databinding/CollectionBindingSpec.groovy

@@ -18,8 +18,8 @@
  */
 package grails.databinding
 
-import grails.databinding.SimpleDataBinder;
-import grails.databinding.SimpleMapDataBindingSource;
+import grails.databinding.errors.BindingError
+import grails.databinding.events.DataBindingListenerAdapter
 import spock.lang.Specification
 
 class CollectionBindingSpec extends Specification {
@@ -111,6 +111,60 @@ class CollectionBindingSpec extends Specification {
         company.departments[2].numberOfEmployees == 99
     }
 
+    void 'Test negative indexed binding to a List is rejected'() {
+        given:
+        def binder = new SimpleDataBinder()
+        def company = new Company()
+        def listener = new CollectionBindingListener()
+
+        when:
+        binder.bind company, new SimpleMapDataBindingSource([
+                'departments[-1]': [name: 'Bad Department'],
+                'departments[0]': [name: 'Department Zero']]), listener
+
+        then:
+        company.departments.size() == 1
+        company.departments[0].name == 'Department Zero'
+        listener.bindingErrors.size() == 1
+        listener.bindingErrors[0].propertyName == 'departments[-1]'
+        listener.bindingErrors[0].rejectedValue == [name: 'Bad Department']
+    }
+
+    void 'Test malformed indexed binding to a List is rejected'() {
+        given:
+        def binder = new SimpleDataBinder()
+        def company = new Company()
+        def listener = new CollectionBindingListener()
+
+        when:
+        binder.bind company, new SimpleMapDataBindingSource([
+                'departments[bad]': [name: 'Bad Department'],
+                'departments[0]': [name: 'Department Zero']]), listener
+
+        then:
+        company.departments.size() == 1
+        company.departments[0].name == 'Department Zero'
+        listener.bindingErrors.size() == 1
+        listener.bindingErrors[0].propertyName == 'departments[bad]'
+        listener.bindingErrors[0].rejectedValue == [name: 'Bad Department']
+    }
+
+    void 'Test negative indexed binding to an array is rejected'() {
+        given:
+        def binder = new SimpleDataBinder()
+        def library = new Library()
+        def listener = new CollectionBindingListener()
+
+        when:
+        binder.bind library, new SimpleMapDataBindingSource(['codes[-1]': 'bad', 'codes[0]': 'good']), listener
+
+        then:
+        library.codes as List == ['good']
+        listener.bindingErrors.size() == 1
+        listener.bindingErrors[0].propertyName == 'codes[-1]'
+        listener.bindingErrors[0].rejectedValue == 'bad'
+    }
+
     void 'Test binding to an untyped List'() {
         given:
         def binder = new SimpleDataBinder()
@@ -156,9 +210,22 @@ class Company {
     List<Department> departments
 }
 
+class Library {
+    String[] codes
+}
+
 class Department {
     String name
     Integer numberOfEmployees
     List listOfCodes
     Set setOfCodes
 }
+
+class CollectionBindingListener extends DataBindingListenerAdapter {
+
+    List<BindingError> bindingErrors = []
+
+    void bindingError(BindingError error, errors) {
+        bindingErrors << error
+    }
+}

grails-doc/src/en/guide/theWebLayer/controllers/dataBinding.adoc

@@ -128,13 +128,15 @@ assert band.albums[1].numberOfTracks == 7
 
 That code would work in the same way if `albums` were an array instead of a `List`.
 
+NOTE: When binding to an array, a `Collection`, or a many-ended domain association by index, the value inside square brackets must be a non-negative integer. Entries such as `albums[-1]` or `albums[bogus]` are rejected as binding errors. The error field name includes the offending indexed segment, that binding path is skipped, and the target array, collection, or association is not changed by that entry. Map keys are not interpreted as numeric indexes, so keys such as `players[guitar]` remain valid map keys.
+
 Note that when binding to a `Set` the structure of the `Map` being bound to the `Set` is the same as that of a `Map` being bound to a `List` but since a `Set` is unordered, the indexes don't necessarily correspond to the order of elements in the `Set`.  In the code example above, if `albums` were a `Set` instead of a `List`, the `bindingMap` could look exactly the same but 'Foxtrot' might be the first album in the `Set` or it might be the second.  When updating existing elements in a `Set` the `Map` being assigned to the `Set` must have `id` elements in it which represent the element in the `Set` being updated, as in the following example:
 
 [source,groovy]
 ----
 /*
  * The value of the indexes 0 and 1 in albums[0] and albums[1] are arbitrary
- * values that can be anything as long as they are unique within the Map.
+ * non-negative integers that only need to be unique within the Map.
  * They do not correspond to the order of elements in albums because albums
  * is a Set.
  */
@@ -516,7 +518,7 @@ class AccountingController {
 ==== Data binding and type conversion errors
 
 
-Sometimes when performing data binding it is not possible to convert a particular String into a particular target type. This results in a type conversion error. Grails will retain type conversion errors inside the link:{domainClassesRef}errors.html[errors] property of a Grails domain class. For example:
+Sometimes when performing data binding it is not possible to convert a particular String into a particular target type. This results in a type conversion error. Grails will retain type conversion errors inside the link:{domainClassesRef}errors.html[errors] property of a Grails domain class. Invalid indexed array, collection, and many-ended association binding paths, such as negative or non-integer indexes, are also retained as binding errors, and the field name identifies the indexed path that was rejected. For example:
 
 [source,groovy]
 ----

grails-test-suite-persistence/src/test/groovy/grails/web/databinding/GrailsWebDataBinderSpec.groovy

@@ -957,6 +957,66 @@ class GrailsWebDataBinderSpec extends Specification implements DataTest {
         publisher.publications[0].title == 'Definitive Guide To Grails 2'
     }
 
+    void 'Test negative indexed domain association binding to a List is rejected'() {
+
+        given:
+        def bindingErrors = [] as List<BindingError>
+        def listener = new DataBindingListenerAdapter() {
+            @Override void bindingError(BindingError error, Object errors) {
+                bindingErrors << error
+            }
+        }
+        def publication = new Publication(title: 'Definitive Guide To Grails', author: new Author(name: 'Author Name'))
+        def publisher = new Publisher(name: 'Apress').save()
+        publisher.addToPublications(publication)
+        publisher.save(flush: true)
+
+        when:
+        binder.bind(publisher, new SimpleMapDataBindingSource([
+            'publications[-1]': [
+                id: publication.id,
+                title: 'Definitive Guide To Grails 2'
+            ]
+        ]), listener)
+
+        then:
+        publisher.publications.size() == 1
+        publisher.publications[0].title == 'Definitive Guide To Grails'
+        bindingErrors.size() == 1
+        bindingErrors[0].propertyName == 'publications[-1]'
+        bindingErrors[0].rejectedValue == [id: publication.id, title: 'Definitive Guide To Grails 2']
+    }
+
+    void 'Test malformed indexed domain association binding to a List is rejected'() {
+
+        given:
+        def bindingErrors = [] as List<BindingError>
+        def listener = new DataBindingListenerAdapter() {
+            @Override void bindingError(BindingError error, Object errors) {
+                bindingErrors << error
+            }
+        }
+        def publication = new Publication(title: 'Definitive Guide To Grails', author: new Author(name: 'Author Name'))
+        def publisher = new Publisher(name: 'Apress').save()
+        publisher.addToPublications(publication)
+        publisher.save(flush: true)
+
+        when:
+        binder.bind(publisher, new SimpleMapDataBindingSource([
+            'publications[bad]': [
+                id: publication.id,
+                title: 'Definitive Guide To Grails 2'
+            ]
+        ]), listener)
+
+        then:
+        publisher.publications.size() == 1
+        publisher.publications[0].title == 'Definitive Guide To Grails'
+        bindingErrors.size() == 1
+        bindingErrors[0].propertyName == 'publications[bad]'
+        bindingErrors[0].rejectedValue == [id: publication.id, title: 'Definitive Guide To Grails 2']
+    }
+
     void 'Test using @BindUsing to initialize property with a type other than the declared type'() {
 
         given:

grails-web-databinding/src/main/groovy/grails/web/databinding/GrailsWebDataBinder.groovy

@@ -434,6 +434,10 @@ class GrailsWebDataBinder extends SimpleDataBinder {
                 if (referencedType != null && isDomainClass(referencedType)) {
                     needsBinding = false
                     if (Set.isAssignableFrom(metaProperty.type)) {
+                        Integer index = parseIndexedPropertyIndex(obj, indexedPropertyReferenceDescriptor, val, listener, errors)
+                        if (index == null) {
+                            return
+                        }
                         def collection = initializeCollection(obj, propName, metaProperty.type)
                         def instance
                         if (collection != null) {
@@ -448,7 +452,7 @@ class GrailsWebDataBinder extends SimpleDataBinder {
                                 Exception e = new IllegalArgumentException(message)
                                 addBindingError(obj, propName, idValue, e, listener, errors)
                             } else {
-                                addElementToCollectionAt(obj, propName, collection, Integer.parseInt(indexedPropertyReferenceDescriptor.index), instance)
+                                addElementToCollectionAt(obj, propName, collection, index, instance)
                             }
                         }
                         if (instance != null) {
@@ -459,8 +463,11 @@ class GrailsWebDataBinder extends SimpleDataBinder {
                             }
                         }
                     } else if (Collection.isAssignableFrom(metaProperty.type)) {
+                        Integer idx = parseIndexedPropertyIndex(obj, indexedPropertyReferenceDescriptor, val, listener, errors)
+                        if (idx == null) {
+                            return
+                        }
                         def collection = initializeCollection(obj, propName, metaProperty.type)
-                        def idx = Integer.parseInt(indexedPropertyReferenceDescriptor.index)
                         if ('null' == idValue) {
                             if (idx < collection.size()) {
                                 def element = collection[idx]