jdaugherty commented on code in PR #15804:
URL: https://github.com/apache/grails-core/pull/15804#discussion_r3596976592
##########
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.
Review Comment:
State the rationale here rather than presenting this as an arbitrary rule:
array and positional collection binding follows the JavaBeans indexed-property
model (spec v1.01, section 7.2), which only defines non-negative `int` indexes
with array semantics. Note the spec does not cover `Set` binding at all - see
my comment on the `Set` branch in `GrailsWebDataBinder`. If `Set` keys remain
arbitrary grouping keys per the existing documented contract, the change below
to "non-negative integers that only need to be unique" overstates the
restriction and should be reverted for the `Set` case.
##########
grails-databinding-core/src/main/groovy/grails/databinding/SimpleDataBinder.groovy:
##########
@@ -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)
Review Comment:
Since this path binds untrusted request data, the reported binding error
must stay generic - indistinguishable from the malformed-index case - so the
source cannot tell we handle negative indexes explicitly. The current shape
achieves that, but only by accident of throwing `NumberFormatException` as
control flow. Please add a code comment documenting that the uniform error is
intentional, so a future cleanup does not "improve" it into a distinct, more
descriptive message.
##########
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)
Review Comment:
This changes documented behavior that the JavaBeans rationale does not cover
in either direction: spec indexed properties are array-typed with `int`
accessors, so `Set` binding keys have no spec standing at all - the
arbitrary-unique-key convention here is purely Grails' own documented contract
(the guide says the values "can be anything as long as they are unique within
the Map"). The old code only reached `Integer.parseInt` in the
add-queried-instance path, so updating an existing `Set` element by id (e.g.
`albums[foo]: [id: 1, ...]`) never parsed the index and worked with non-numeric
keys; parsing at the top of the branch now rejects those previously valid
paths. Requiring a non-negative integer here also falsely implies the key is
positional when a `Set` has no positions. Either defer the parse to the
`addElementToCollectionAt` call as before, or treat `Set` keys like map keys.
##########
grails-databinding-core/src/main/groovy/grails/databinding/SimpleDataBinder.groovy:
##########
@@ -394,6 +400,22 @@ class SimpleDataBinder implements DataBinder {
}
}
+ protected Integer parseIndexedPropertyIndex(obj,
IndexedPropertyReferenceDescriptor indexedPropertyReferenceDescriptor,
Review Comment:
Rejecting negative indexes is the correct behavior: the JavaBeans
specification (v1.01, section 7.2) defines indexed properties as array-typed
properties with paired `int`-indexed accessors, where an invalid index may
throw `ArrayIndexOutOfBoundsException`. Grails' indexed binding to collections
is an extension of that model, and this change aligns the extension with the
spec's array semantics - the prior `[-1]` behavior was Groovy list semantics
leaking through, never valid under the beans model. Please reference the spec
section in this method's groovydoc and in the PR description so the rationale
is on record.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
