jdaugherty commented on code in PR #16058:
URL: https://github.com/apache/grails-core/pull/16058#discussion_r3674706090
##########
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)
Review Comment:
This is the other half of the `Set` issue I raised on #15804, and it is
still open. `Set` reaches this branch (`Collection.isAssignableFrom(Set)` is
true), so a `Set`-typed property still requires a non-negative integer key here
— even though the parsed index is dead for `Set`s: `isOkToAddElementAt` ignores
`index` when the collection is a `Set` (it only checks `collection.size() <
autoGrowCollectionLimit`), and `addElementToCollectionAt` then calls
`collection.add(val)`. Nothing downstream consumes the value.
The consequence is that the fix in `GrailsWebDataBinder` only covers the
update-existing-by-id path. When the map has no `id`,
`getIdentifierValueFrom(val)` returns null, `needsBinding` stays true, and the
call falls through to this branch. So the same association accepts an arbitrary
grouping key for an update and rejects it for an insert:
```
authors[foo]: [id: <existing>, name: 'Renamed'] -> binds (your fix)
authors[foo]: [name: 'Brand New Author'] -> binding error, nothing
added
authors[0]: [name: 'Brand New Author'] -> binds
```
A plain non-domain `Set` is rejected outright: `setOfCodes[foo]` yields a
binding error and leaves the property null.
Please move the parse inside the non-`Set` path so the index is only
required where it is actually used, e.g.:
```groovy
} else if (Collection.isAssignableFrom(propertyType)) {
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 (!isSet) {
indexedInstance = collectionInstance[index]
}
```
That makes the core binder agree with the `Set` branch in
`GrailsWebDataBinder` and with the guide text this PR adds.
##########
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.
Review Comment:
This is much better than the previous wording, and the map-key sentence is
accurate — I confirmed `players[guitar]` and even a literal `-1` map key still
bind as keys.
But "`Set` association keys remain arbitrary grouping keys" overstates what
the code does today, in two ways:
- It is only true on the update-existing-by-id path. Adding a *new* element
with no `id` falls through to `SimpleDataBinder`, which still requires a
non-negative integer — see my comment on the `Collection` branch there.
- It is scoped to *association* keys, but a reader will reasonably apply it
to the generic `Set` paragraph immediately below. A plain `Set<String>`
property is not an association and is rejected: `setOfCodes[foo]` produces a
binding error.
If you take the `SimpleDataBinder` fix, this sentence becomes true as
written and nothing here needs to change. If you would rather keep the current
behavior, then this sentence has to say that arbitrary keys apply only when
updating an existing element of a `Set` association by `id` — which is a
contract worth avoiding, so I would prefer the code fix.
##########
grails-databinding-core/src/test/groovy/grails/databinding/CollectionBindingSpec.groovy:
##########
@@ -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) {
Review Comment:
Missing `@Override`. `DataBindingListenerAdapter.bindingError(BindingError,
Object)` is a Java method, so this compiles either way, but a typo in the name
or arity would silently stop collecting errors and the assertions would pass
against an always-empty list. The equivalent listener added in
`GrailsWebDataBinderSpec` in this same PR does annotate it — worth matching:
```groovy
@Override
void bindingError(BindingError error, errors) {
bindingErrors << error
}
```
##########
grails-test-suite-persistence/src/test/groovy/grails/web/databinding/GrailsWebDataBinderSpec.groovy:
##########
@@ -800,6 +800,34 @@ class GrailsWebDataBinderSpec extends Specification
implements DataTest {
updatedA3.name == 'Author Tres'
}
+ void 'Test updating Set elements by id with non-numeric grouping keys'() {
Review Comment:
Good test, and it does pin the regression I flagged. Two gaps that matter
for keeping it pinned:
1. Nothing covers a *negative* key on the `Set` path. `authors[-1]: [id:
<existing>, ...]` is accepted today (I verified: it binds, no error), and that
acceptance is deliberate — a `Set` key is not a position. But in a PR whose
entire purpose is rejecting `[-1]`, an untested acceptance is the first thing a
future "consistency" cleanup will remove. Please add a case asserting
`authors[-1]` with an `id` still binds and raises no binding error.
2. Nothing covers adding a *new* element to a `Set` with a non-numeric key.
That case currently fails (`authors[foo]: [name: 'Brand New Author']` ->
binding error, nothing added). Once the `SimpleDataBinder` branch is fixed it
should bind, so a test here would both drive that fix and lock it in.
A plain non-domain `Set` case belongs in `CollectionBindingSpec` alongside
the new `List`/array tests, for the same reason.
##########
grails-databinding-core/src/main/groovy/grails/databinding/SimpleDataBinder.groovy:
##########
@@ -394,6 +400,37 @@ class SimpleDataBinder implements DataBinder {
}
}
+ /**
+ * Parses an indexed binding path segment as a non-negative integer.
+ * <p>
+ * 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).
+ * </p>
+ *
+ * @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) {
+
+ try {
+ Integer index =
Integer.parseInt(indexedPropertyReferenceDescriptor.index)
+ if (index < 0) {
+ // Intentional: report the same generic NumberFormatException
as a
+ // malformed index so untrusted request data cannot
distinguish that
+ // negative indexes are handled explicitly.
+ throw new
NumberFormatException(indexedPropertyReferenceDescriptor.index)
Review Comment:
The comment is the right idea but the code does not deliver what it claims,
so as written it will mislead a future reader into leaving a real gap in place.
`Integer.parseInt` builds its own message, and this `throw` supplies a
different one, so the two rejections are distinguishable by message:
```
listOfCodes[-1] -> NumberFormatException, message: -1
listOfCodes[bad] -> NumberFormatException, message: For input string: "bad"
```
That difference is not internal: `GrailsWebDataBindingListener` line 55 does
`def defaultMessage = error.cause?.message ?: 'Data Binding Failed'`, so the
message becomes the `FieldError`'s `defaultMessage` and is rendered whenever no
message code resolves. The shape of the message therefore reveals that the
index parsed successfully and was rejected by a separate check. As a side
effect, a rejected `[-1]` currently surfaces to the user as the bare text `-1`,
which is not a usable error message either.
Construct the exception identically on both paths rather than inheriting
`parseInt`'s message — then the comment's guarantee actually holds:
```groovy
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.
addBindingError(obj, indexedPropertyReferenceDescriptor.toString(),
val,
new NumberFormatException("For input string:
\"${rawIndex}\""), listener, errors)
return null
}
index
}
```
This also drops the throw-as-control-flow, which is what made the original
shape fragile.
##########
grails-web-databinding/src/main/groovy/grails/web/databinding/GrailsWebDataBinder.groovy:
##########
@@ -448,7 +450,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)
Review Comment:
The behavior is right, but a bare `0` in an argument position named `index`
reads as "add at the front" and invites someone to "fix" it back to the parsed
key. Worth stating why any value is inert here: this branch is guarded by
`Set.isAssignableFrom(metaProperty.type)`, and for a `Set`
`addElementToCollectionAt` never uses the index — `isOkToAddElementAt` only
checks `collection.size() < autoGrowCollectionLimit` and the add is
`collection.add(val)`.
Folding that into the comment you already added at the top of the branch
would be enough, e.g. "...Selection is by id, and `addElementToCollectionAt`
ignores the index for `Set`s, so the value passed below is inert."
--
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]
