Reject invalid databinding indexes (continues #15804)

[sanjana2505006]

Continues #15804.

Built on James's work and addressed the open review comments from @jdaugherty.

Main ones:

• cite JavaBeans §7.2 for rejecting negative/malformed indexes (and note that the generic error is intentional)
• don't require integer keys for Set associations — those are just grouping keys, selection is still by id
• docs + a test for authors[foo] style Set updates

Ran CollectionBindingSpec, GrailsWebDataBinderSpec, and :grails-web-databinding:check.

[jdaugherty]

Hi @sanjana2505006 is the intent to replace @jamesfredley 's PR?

[jamesfredley]

@jdaugherty since this includes my commits, yes this will replace it. External contributors can't commit to our origin branches.

This could alternatively targeted the other PR's branch, but either is fine.

[jdaugherty]

The main issues I have with this is the lack of warning in the upgrade guide & the big one revolves around set. It looks like Set is no longer supported, but Set mirrors a map more than list. I'm ok with the negative values not being supported in a Set, but Set seems valid to me and this breaks that support. I had AI do the review and tweaked the output, that review is what follows.

Thanks for picking this up. Three of the four points I raised on #15804 are genuinely resolved: the groovydoc now cites JavaBeans v1.01 §7.2, the Set update-by-id path no longer parses the grouping key, and the guide states the rationale instead of presenting the restriction as an arbitrary rule. I verified the Set fix behaves as intended — authors[foo]: [id: <existing>, ...] and even authors[-1]: [id: <existing>, ...] now bind cleanly.

Two of the four are only partially addressed, and both are inline below:

1. The Set fix stops at GrailsWebDataBinder. SimpleDataBinder.processIndexedProperty still parses and requires a non-negative integer for every Collection, Set included — so within the same Set association the grouping key is arbitrary when the map carries an id and mandatory-integer when it does not. I confirmed this: authors[foo]: [name: 'Brand New Author'] produces a binding error and adds nothing, while authors[0]: [name: 'Brand New Author'] binds. Same for a plain Set<String> property: setOfCodes[foo] is rejected. The parsed index is provably dead for Sets, so this restriction buys nothing.

2. The new code comment asserts a property the code does not have. The negative and malformed paths do not produce the same error — NumberFormatException messages are -1 and For input string: "bad" respectively, and GrailsWebDataBindingListener (line 55) copies error.cause?.message into the FieldError defaultMessage. An inaccurate comment is worse than none, because the next reader will trust it.

Missing what's new / upgrade notice. This changes how request parameters bind in an existing application — books[-1].title previously mutated the last element via Groovy's putAt(-1) and now reports a binding error. That is exactly the class of change the release docs need to carry, and neither grails-doc/src/en/guide/introduction/whatsNew.adoc nor grails-doc/src/en/guide/upgrading/upgrading80x.adoc mentions it. Please add:

• a ==== subsection to whatsNew.adoc describing the hardening (indexed array/collection/positional-association binding now requires non-negative integer indexes; invalid paths become binding errors rather than throwing or silently selecting a different element), and
• a corresponding entry in upgrading80x.adoc — either a bullet under ==== 20. Other Default Behavior Changes or its own numbered section — telling upgraders what to look for in existing form markup and request parameters.

The §7.2 reasoning belongs in both, since it is what makes this an alignment with the beans model rather than a feature removal.

Housekeeping. The squashed commit message should not carry the second commit's "Align the Set binding example comment with the integer index requirement" line — the third commit reverses exactly that, so the claim would land in history inverted. Also, #15804 is still open; if this supersedes it, that one should be closed so the two do not drift.

[jdaugherty]

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 Sets: 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.:

} 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.

[jdaugherty]

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:

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.

[jdaugherty]

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 Sets, so the value passed below is inert."

[jdaugherty]

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.

[jdaugherty]

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.

[jdaugherty]

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:

@Override
void bindingError(BindingError error, errors) {
    bindingErrors << error
}

[sanjana2505006]

Addressed the review in 5c6c833
Set keys no longer require an integer in SimpleDataBinder, bad/negative index errors use the same message, added the Set tests, upgrade/whats-new notes, @Override on the listener, and the inert index comment.
@jdaugherty Happy to tweak anything I missed.