Reject invalid databinding indexes - #15804
Conversation
Report binding errors for malformed or negative indexed binding paths before array, collection, or domain association access. Apply the same guard to the web data binder association path and cover the core and web binding behavior with regressions. Assisted-by: Hephaestus:openai/gpt-5.5
There was a problem hiding this comment.
Pull request overview
This PR hardens Grails data binding by rejecting malformed or negative indexed binding paths (e.g., list[-1], list[bad]) before attempting array/collection/domain-association access, converting what previously could throw or apply Groovy negative-index semantics into binding errors captured by the binding error listener flow.
Changes:
- Add
parseIndexedPropertyIndex(...)to centralize index parsing/validation and emit binding errors on malformed/negative indexes. - Use the new index parsing in both core (
SimpleDataBinder) and web (GrailsWebDataBinder) indexed-binding paths to prevent invalid index access. - Add regression tests covering negative and malformed indexes for Lists, arrays, and domain association collection binding.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| grails-web-databinding/src/main/groovy/grails/web/databinding/GrailsWebDataBinder.groovy | Validates indexed binding paths for domain association collections before attempting indexed updates. |
| grails-databinding-core/src/main/groovy/grails/databinding/SimpleDataBinder.groovy | Introduces shared index parsing/validation and uses it for array/collection indexed binding. |
| grails-databinding-core/src/test/groovy/grails/databinding/CollectionBindingSpec.groovy | Adds regression coverage ensuring negative/malformed indexes are rejected for List and array binding. |
| grails-test-suite-persistence/src/test/groovy/grails/web/databinding/GrailsWebDataBinderSpec.groovy | Adds web-layer regression coverage for rejecting invalid indexed domain association binding to Lists. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #15804 +/- ##
==============================
==============================
🚀 New features to boost your workflow:
|
|
Didn't this work in prior Grails versions? The negative index could be used to bind to the end of an array. I agree this is probably a good change, but it needs discussed. Also, this PR is not adhering to our PR policy - we need explanations for why this is necessary, etc. There are several checklists when creating a PR that are not completed by this PR. |
|
Thanks. I updated the PR body to follow the summary policy more explicitly: it now separates "What was found", "What was fixed", and "Verification". Yes, negative collection indexes likely worked before via Groovy list semantics. The concern is that indexed binding paths come from request parameters, so |
|
Maintenance pass complete. Copilot generated no inline review comments and there are no review threads to resolve. I reviewed the Codecov patch-coverage note for GrailsWebDataBinder: the behavior is covered by the persistence-suite GrailsWebDataBinderSpec tests rather than local grails-web-databinding module coverage, and I reran the focused CollectionBindingSpec plus GrailsWebDataBinderSpec successfully. The maintainer question about negative-index compatibility remains open as a behavior discussion, not resolved. |
Document that indexed binding to arrays, collections, and many-ended associations requires non-negative integer indexes, that invalid indexes are reported as binding errors without mutating the target, and that map keys are unaffected. Align the Set binding example comment with the integer index requirement. Assisted-by: opencode:gpt-5.5
|
Pre-release review pass (2026-07-02, dual-reviewer: GPT-5.5 oracle + Codex CLI): NEEDS-IMPROVEMENT, now addressed. Findings: the user-facing behavior change (negative/malformed indexes rejected as binding errors instead of Groovy negative-index semantics or exceptions) was undocumented in the data binding guide (high, docs-only) - fixed in |
|
@jamesfredley please fix the formatting on this summary |
|
Updated the summary formatting to follow the repository PR template and added the missing checklist context. I also reran the focused coverage for the invalid-index behavior. The tests cover the core list/array paths and the web-domain-association path in Verification: |
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
I'm ok with this change, but it is removing a feature that existed before. We should make sure this is raised in the weekly meeting.
Maintenance passMerged the latest
The maintainer question about negative-index compatibility (previously |
✅ All tests passed ✅🏷️ Commit: e0d3624 Learn more about TestLens at testlens.app. |
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
Withdrawing my earlier request to raise this at the weekly meeting. 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 - negative indexes were never part of that model. Grails extends indexed binding beyond arrays to collections, and this change aligns that extension with the spec's array semantics; the old books[-1] behavior was Groovy's putAt(-1) list extension leaking through the binder, not a supported feature. That makes the negative-index rejection an alignment with the beans model rather than a feature removal, and no further discussion is needed. Please update the PR description's compatibility note to reference the spec section instead of leaving the behavior question open. One genuine compatibility issue remains on the Set association path, which the spec does not cover in either direction - see the inline comment.
| } | ||
| } | ||
|
|
||
| protected Integer parseIndexedPropertyIndex(obj, IndexedPropertyReferenceDescriptor indexedPropertyReferenceDescriptor, |
There was a problem hiding this 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.
| try { | ||
| Integer index = Integer.parseInt(indexedPropertyReferenceDescriptor.index) | ||
| if (index < 0) { | ||
| throw new NumberFormatException(indexedPropertyReferenceDescriptor.index) |
There was a problem hiding this 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.
| if (referencedType != null && isDomainClass(referencedType)) { | ||
| needsBinding = false | ||
| if (Set.isAssignableFrom(metaProperty.type)) { | ||
| Integer index = parseIndexedPropertyIndex(obj, indexedPropertyReferenceDescriptor, val, listener, errors) |
There was a problem hiding this 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.
|
|
||
| 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. |
There was a problem hiding this 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.
|
Opened PR #16058 with the remaining review items from this thread. |
|
Closing in favor of #16058, which continues this work and addresses @jdaugherty's 2026-07-16 review feedback (JavaBeans §7.2 citation, the intentional-generic-error comment, and reverting the non-negative-integer requirement for |
Description
What was found
Indexed data-binding paths accepted index text before consistently validating it, which produced two unsafe outcomes:
books[-1].titleused Groovy negative-index semantics and mutated the last existing element.books[bad].titlecould throw during binding instead of being reported as binding errors.Because binding paths commonly come from request parameters, the binder should not let externally supplied negative indexes select an element different from the literal index.
What changed
Compatibility note
Negative collection indexes likely worked in prior Grails versions through Groovy list semantics. This branch intentionally treats them as invalid binding input for Grails 8, and the behavior question remains open for maintainer discussion.
Verification
./gradlew --no-daemon :grails-databinding-core:test --tests "grails.databinding.CollectionBindingSpec" :grails-test-suite-persistence:test --tests "grails.web.databinding.GrailsWebDataBinderSpec" :grails-web-databinding:checkGrailsWebDataBinderSpeccovers the web-domain-association path that Codecov reports outside localgrails-web-databindingmodule coverage.Contributor Checklist
Issue and Scope
8.0.x, where behavior-hardening changes are permitted for discussion.Code Quality
Licensing and Attribution
Documentation
Rebased on 8.0.x
Merged the latest
8.0.x(clean, no conflicts) and re-ranCollectionBindingSpec+GrailsWebDataBinderSpecsuccessfully. No Copilot comments; the negative-index compatibility change remains open for weekly-meeting discussion.