diff --git a/grails-databinding-core/src/main/groovy/grails/databinding/SimpleDataBinder.groovy b/grails-databinding-core/src/main/groovy/grails/databinding/SimpleDataBinder.groovy index e0049ed6beb..3684d990e21 100755 --- a/grails-databinding-core/src/main/groovy/grails/databinding/SimpleDataBinder.groovy +++ b/grails-databinding-core/src/main/groovy/grails/databinding/SimpleDataBinder.groovy @@ -333,16 +333,29 @@ 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) + // Sets use the bracket key only as a grouping token; the parsed index is never + // consumed (isOkToAddElementAt / addElementToCollectionAt ignore it). Require a + // non-negative integer index only for List/Collection paths that actually use it. + boolean isSet = Set.isAssignableFrom(propertyType) + Integer index = 0 + if (!isSet) { + index = parseIndexedPropertyIndex(obj, indexedPropertyReferenceDescriptor, val, listener, errors) + if (index == null) { + return + } + } Collection collectionInstance = initializeCollection(obj, propName, propertyType) def indexedInstance = null - if (!(Set.isAssignableFrom(propertyType))) { + if (!isSet) { indexedInstance = collectionInstance[index] } if (indexedInstance == null) { @@ -394,6 +407,42 @@ class SimpleDataBinder implements DataBinder { } } + /** + * Parses an indexed binding path segment as a non-negative integer. + *

+ * Indexed properties follow the JavaBeans model (spec v1.01, section 7.2): + * array-typed properties with paired {@code int}-indexed accessors. Grails + * extends that model to positional collection binding; negative indexes are + * rejected because they are not part of the beans model (the prior + * {@code [-1]} behavior was Groovy list semantics leaking through the binder). + *

+ * + * @return the parsed index, or {@code null} when the segment is rejected as a binding error + */ + protected Integer parseIndexedPropertyIndex(obj, IndexedPropertyReferenceDescriptor indexedPropertyReferenceDescriptor, + val, DataBindingListener listener, errors) { + + String rawIndex = indexedPropertyReferenceDescriptor.index + Integer index = null + try { + index = Integer.parseInt(rawIndex) + } + catch (NumberFormatException ignored) { + // handled below, with the same error as a negative index + } + + if (index == null || index < 0) { + // Intentional: malformed and negative indexes are reported identically, so + // untrusted request data cannot tell that negative indexes are handled by a + // separate check. Do not "improve" this into two distinct messages. + // GrailsWebDataBindingListener copies cause.message into FieldError.defaultMessage. + addBindingError(obj, indexedPropertyReferenceDescriptor.toString(), val, + new NumberFormatException("For input string: \"${rawIndex}\""), listener, errors) + return null + } + index + } + @CompileStatic(TypeCheckingMode.SKIP) protected initializeArray(obj, String propertyName, Class arrayType, int index) { Object[] array = obj[propertyName] diff --git a/grails-databinding-core/src/test/groovy/grails/databinding/CollectionBindingSpec.groovy b/grails-databinding-core/src/test/groovy/grails/databinding/CollectionBindingSpec.groovy index 6dafb35ba2e..eebd2ecdb74 100755 --- a/grails-databinding-core/src/test/groovy/grails/databinding/CollectionBindingSpec.groovy +++ b/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() @@ -133,6 +187,44 @@ class CollectionBindingSpec extends Specification { dept.setOfCodes.contains 'Rush' } + void 'Test binding to a Set with non-numeric grouping keys'() { + given: + def binder = new SimpleDataBinder() + def dept = new Department() + def listener = new CollectionBindingListener() + + when: + binder.bind dept, new SimpleMapDataBindingSource([ + 'setOfCodes[foo]': 2112, + 'setOfCodes[bar]': 'Rush' + ]), listener + + then: + dept.setOfCodes.size() == 2 + dept.setOfCodes.contains 2112 + dept.setOfCodes.contains 'Rush' + listener.bindingErrors.empty + } + + void 'Test binding to a Set with negative grouping keys'() { + given: + def binder = new SimpleDataBinder() + def dept = new Department() + def listener = new CollectionBindingListener() + + when: + binder.bind dept, new SimpleMapDataBindingSource([ + 'setOfCodes[-1]': 2112, + 'setOfCodes[-2]': 'Rush' + ]), listener + + then: + dept.setOfCodes.size() == 2 + dept.setOfCodes.contains 2112 + dept.setOfCodes.contains 'Rush' + listener.bindingErrors.empty + } + void 'Test binding to an unitialized untyped Map'() { given: def binder = new SimpleDataBinder() @@ -156,9 +248,23 @@ class Company { List departments } +class Library { + String[] codes +} + class Department { String name Integer numberOfEmployees List listOfCodes Set setOfCodes } + +class CollectionBindingListener extends DataBindingListenerAdapter { + + List bindingErrors = [] + + @Override + void bindingError(BindingError error, errors) { + bindingErrors << error + } +} diff --git a/grails-doc/src/en/guide/introduction/whatsNew.adoc b/grails-doc/src/en/guide/introduction/whatsNew.adoc index aa8b28f1b17..543c2efd148 100644 --- a/grails-doc/src/en/guide/introduction/whatsNew.adoc +++ b/grails-doc/src/en/guide/introduction/whatsNew.adoc @@ -277,3 +277,14 @@ be enabled (`grails.mongodb.transactional = true`). Spring Data repositories are session are shared, and the two object-mapping models stay separate. See the link:{mongodb5Guide}index.html#springDataInterop[Spring Data MongoDB Interoperability] section of the GORM for MongoDB guide for details. + +==== Indexed Data Binding Changes + +Indexed array, collection, and positional-association binding now follows the JavaBeans indexed-property model +(spec v1.01, section 7.2): only non-negative `int` indexes are accepted. Paths such as `books[-1].title` or +`albums[bogus]` are rejected as binding errors instead of mutating the last element via Groovy `putAt(-1)` semantics +or throwing an uncaught exception. The offending path is skipped and the target is left unchanged by that entry. + +`Set` association keys remain arbitrary grouping tokens (selection is by `id`), so values such as `authors[foo]` and +`authors[-1]` continue to bind for Sets. See the link:theWebLayer.html#dataBinding[Data Binding] guide and the +xref:upgrading#upgrading80x[upgrade guide] for details. diff --git a/grails-doc/src/en/guide/theWebLayer/controllers/dataBinding.adoc b/grails-doc/src/en/guide/theWebLayer/controllers/dataBinding.adoc index 017661bfb4e..da9122c6e01 100644 --- a/grails-doc/src/en/guide/theWebLayer/controllers/dataBinding.adoc +++ b/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: Array and positional collection binding follow the JavaBeans indexed-property model (spec v1.01, section 7.2), which only defines non-negative `int` indexes with array semantics. 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. The JavaBeans specification does not cover `Set` binding; `Set` association keys remain arbitrary grouping keys (see below). 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. + * The value of the indexes used in albums[0] and albums[1] are + * arbitrary values 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, List, and positional 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] ---- diff --git a/grails-doc/src/en/guide/upgrading/upgrading80x.adoc b/grails-doc/src/en/guide/upgrading/upgrading80x.adoc index 2d777e80ccb..af727c6db87 100644 --- a/grails-doc/src/en/guide/upgrading/upgrading80x.adoc +++ b/grails-doc/src/en/guide/upgrading/upgrading80x.adoc @@ -825,6 +825,9 @@ The Spring annotations still work, so this is non-blocking, but new code should * **Namespaced link generation is namespace-aware.** When a link, form action, pagination link, sortable column link, redirect, chain, or include targets a controller without an explicit `namespace`, Grails now resolves the namespace automatically. In the normal case, where only one controller has the target name, `controller` and `action` generate the correct namespaced or non-namespaced URL. Ambiguity only occurs when multiple controllers share the same name. In that case, specify `namespace` to choose the target explicitly. Pass `namespace: null` from Groovy code or `namespace=""` in a GSP tag to target the non-namespaced controller explicitly. +* **Indexed data binding rejects negative and malformed indexes.** +Array, List, and other positional collection or association binding now requires a non-negative integer index, aligning with the JavaBeans indexed-property model (spec v1.01, section 7.2). Request parameters such as `books[-1].title` or `albums[bogus]` become binding errors and no longer select a different element or throw an uncaught exception. Audit existing form markup and request parameters for negative or non-numeric indexes on arrays and Lists. `Set` association keys remain arbitrary grouping tokens (selection is by `id`), so `authors[foo]` and `authors[-1]` continue to work for Sets. + ==== 21. Tag Library Test Cleanup Changes Grails 8 removes the `purgeTagLibMetaClass` test hook used by some web and TagLib unit tests. diff --git a/grails-test-suite-persistence/src/test/groovy/grails/web/databinding/GrailsWebDataBinderSpec.groovy b/grails-test-suite-persistence/src/test/groovy/grails/web/databinding/GrailsWebDataBinderSpec.groovy index 17001a0afb9..4a3019507d9 100644 --- a/grails-test-suite-persistence/src/test/groovy/grails/web/databinding/GrailsWebDataBinderSpec.groovy +++ b/grails-test-suite-persistence/src/test/groovy/grails/web/databinding/GrailsWebDataBinderSpec.groovy @@ -800,6 +800,105 @@ class GrailsWebDataBinderSpec extends Specification implements DataTest { updatedA3.name == 'Author Tres' } + void 'Test updating Set elements by id with non-numeric grouping keys'() { + + given: + def publisher = new Publisher(name: 'Some Publisher') + + when: + def a1 = new Author(name: 'Author One').save() + def a2 = new Author(name: 'Author Two').save() + publisher.addToAuthors(a1) + publisher.addToAuthors(a2) + + then: + a1.id != null + a2.id != null + + when: + binder.bind(publisher, new SimpleMapDataBindingSource([ + 'authors[foo]': [id: a2.id, name: 'Author Dos'], + 'authors[bar]': [id: a1.id, name: 'Author Uno'] + ])) + def updatedA1 = publisher.authors.find { it.id == a1.id } + def updatedA2 = publisher.authors.find { it.id == a2.id } + + then: + updatedA1.name == 'Author Uno' + updatedA2.name == 'Author Dos' + } + + void 'Test updating Set elements by id with negative grouping keys'() { + + given: + def publisher = new Publisher(name: 'Some Publisher') + def bindingErrors = [] as List + def listener = new DataBindingListenerAdapter() { + @Override void bindingError(BindingError error, Object errors) { + bindingErrors << error + } + } + + when: + def a1 = new Author(name: 'Author One').save() + def a2 = new Author(name: 'Author Two').save() + publisher.addToAuthors(a1) + publisher.addToAuthors(a2) + + then: + a1.id != null + a2.id != null + + when: + // Negative keys are grouping tokens for Sets, not positions - must not bind-error. + binder.bind(publisher, new SimpleMapDataBindingSource([ + 'authors[-1]': [id: a2.id, name: 'Author Dos'], + 'authors[-2]': [id: a1.id, name: 'Author Uno'] + ]), listener) + def updatedA1 = publisher.authors.find { it.id == a1.id } + def updatedA2 = publisher.authors.find { it.id == a2.id } + + then: + bindingErrors.empty + updatedA1.name == 'Author Uno' + updatedA2.name == 'Author Dos' + } + + void 'Test adding new Set elements with non-numeric grouping keys'() { + + given: + def publisher = new Publisher(name: 'Some Publisher') + def bindingErrors = [] as List + def listener = new DataBindingListenerAdapter() { + @Override void bindingError(BindingError error, Object errors) { + bindingErrors << error + } + } + + when: + def a1 = new Author(name: 'Author One').save() + publisher.addToAuthors(a1) + + then: + a1.id != null + + when: + binder.bind(publisher, new SimpleMapDataBindingSource([ + 'authors[foo]': [name: 'Brand New Author'], + 'authors[bar]': [name: 'Another New Author'] + ]), listener) + def existing = publisher.authors.find { it.id == a1.id } + def brandNew = publisher.authors.find { it.name == 'Brand New Author' } + def another = publisher.authors.find { it.name == 'Another New Author' } + + then: + bindingErrors.empty + existing.name == 'Author One' + brandNew + another + publisher.authors.size() == 3 + } + void 'Test updating Set elements by id'() { given: @@ -957,6 +1056,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 + 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 + 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: diff --git a/grails-web-databinding/src/main/groovy/grails/web/databinding/GrailsWebDataBinder.groovy b/grails-web-databinding/src/main/groovy/grails/web/databinding/GrailsWebDataBinder.groovy index 80a87452dd7..81c4ca49fcd 100644 --- a/grails-web-databinding/src/main/groovy/grails/web/databinding/GrailsWebDataBinder.groovy +++ b/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)) { + // Set association keys are grouping keys (not positions); do not + // require a numeric index. Selection is by id, and + // addElementToCollectionAt ignores the index for Sets, so the + // value passed below is inert. 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, 0, 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]