Reject invalid databinding indexes by jamesfredley · Pull Request #15804 · apache/grails-core

jamesfredley · 2026-07-01T12:18:00Z

The Problem

Indexed data-binding paths accepted the index text before consistently validating it, which produced two bad outcomes:

Negative indexes like books[-1].title were interpreted with Groovy's negative-collection-indexing semantics and mutated the wrong element. Since the binding path normally comes from request parameters or another external source, treating items[-1] as "mutate the last existing item" lets an externally-supplied index select a different element than the literal index says.
Malformed indexes like books[bad].title could throw during binding instead of being reported as a normal binding error.

Oracle review found the same unsafe path in GrailsWebDataBinder for data-source-aware domain-association binding, so this covers both the core binder and the web binder override.

The Fix

Treat negative and malformed indexed paths as invalid binding input.

Add a shared indexed-property parser that accepts only non-negative integer indexes.
Report invalid indexed paths as binding errors and skip the mutation instead of applying Groovy negative-index semantics or throwing mid-bind.
Apply the guard before array, collection, and web-domain-association access.
Map-key binding is unchanged - map keys are not numeric collection indexes, so only array/collection/association indexes are validated.

Files

SimpleDataBinder.groovy - shared non-negative integer index parser + error path.
GrailsWebDataBinder.groovy - same guard on the domain-association path.
CollectionBindingSpec.groovy, GrailsWebDataBinderSpec.groovy - regressions for List/array and web-domain association binding.
dataBinding.adoc - documents the non-negative integer index requirement and the map-key exception.

Testing

:grails-databinding-core:test --tests "grails.databinding.CollectionBindingSpec", :grails-web-databinding:check, and :grails-test-suite-persistence:test --tests "...GrailsWebDataBinderSpec" pass. Aggregate report: 13,654 tests, 0 failures.

Open discussion

Whether apps relied on the prior Groovy negative-index behavior is a product/behavior question left open for maintainers; this branch treats negative indexes as invalid input.

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

Copilot

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 · 2026-07-01T12:45:57Z

Codecov Report

❌ Patch coverage is 63.15789% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 49.4870%. Comparing base (7150c7b) to head (e2e5674).
⚠️ Report is 17 commits behind head on 8.0.x.

Files with missing lines	Patch %	Lines
.../grails/web/databinding/GrailsWebDataBinder.groovy	0.0000%	7 Missing ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##                8.0.x     #15804        +/-   ##
==================================================
+ Coverage     49.4679%   49.4870%   +0.0191%     
- Complexity      16684      16704        +20     
==================================================
  Files            1947       1947                
  Lines           92468      92489        +21     
  Branches        16150      16157         +7     
==================================================
+ Hits            45742      45770        +28     
+ Misses          39620      39612         -8     
- Partials         7106       7107         +1

Files with missing lines	Coverage Δ
.../groovy/grails/databinding/SimpleDataBinder.groovy	`75.8794% <100.0000%> (+0.6217%)`	⬆️
.../grails/web/databinding/GrailsWebDataBinder.groovy	`31.2500% <0.0000%> (-0.4123%)`	⬇️

... and 6 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

jdaugherty · 2026-07-01T12:54:26Z

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.

jamesfredley · 2026-07-01T14:26:51Z

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 items[-1] can mutate the last existing element instead of being treated as an invalid user-supplied index. I agree this behavior change should be discussed by maintainers, so I am leaving that discussion open rather than marking it resolved.

jamesfredley · 2026-07-02T01:43:00Z

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

jamesfredley · 2026-07-02T04:51:15Z

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 e2e56748f3, including aligning the stale Set example comment ("values can be anything") with the actual integer-index requirement. Reviewers confirmed the rejection design (binding error + skip, no mutation) matches existing binding contracts, the guard covers core and web-binder association paths, and positive oversized indexes remain governed by the pre-existing autoGrowCollectionLimit cap (not new in this PR).

testlens-app · 2026-07-02T06:06:26Z

🚨 TestLens detected 1 failed test 🚨

Here is what you can do:

Inspect the test failures carefully.
If you are convinced that some of the tests are flaky, you can mute them below.
Finally, trigger a rerun by checking the rerun checkbox.

Test Summary

CI - Groovy Joint Validation Build / build_grails > :grails-test-examples-scaffolding:integrationTest

Test	Runs
UserControllerSpec > User list	❌

🏷️ Commit: e2e5674
▶️ Tests: 32751 executed
⚪️ Checks: 44/44 completed

Test Failures

UserControllerSpec > User list (:grails-test-examples-scaffolding:integrationTest in CI - Groovy Joint Validation Build / build_grails)

geb.waiting.WaitTimeoutException: condition did not pass in 10 seconds (failed with exception)
	at geb.waiting.Wait.waitFor(Wait.groovy:128)
	at geb.waiting.PotentiallyWaitingExecutor.execute(PotentiallyWaitingExecutor.groovy:31)
	at geb.Page.verifyThisPageAtOnly(Page.groovy:424)
	at geb.Page.getAtVerificationResult(Page.groovy:217)
	at geb.Page.verifyAt(Page.groovy:188)
	at geb.Browser.doAt(Browser.groovy:1208)
	at geb.Browser.at(Browser.groovy:410)
	at geb.Browser.to(Browser.groovy:566)
	at geb.Browser.to(Browser.groovy:543)
	at geb.Browser.to(Browser.groovy:532)
	at grails.plugin.geb.support.delegate.BrowserDelegate$Trait$Helper.to(BrowserDelegate.groovy:160)
	at com.example.UserControllerSpec.User list(UserControllerSpec.groovy:46)
Caused by: Assertion failed: 

title == pageTitle
|     |  |
|     |  'User List'
|     false
'Please sign in'

	at com.example.pages.UserListPage._clinit__closure1(UserListPage.groovy:28)
	at com.example.pages.UserListPage._clinit__closure1(UserListPage.groovy)
	at geb.waiting.Wait.waitFor(Wait.groovy:117)
	... 11 more

Muted Tests

Select tests to mute in this pull request:

UserControllerSpec > User list

Reuse successful test results:

♻️ Only rerun the tests that failed or were muted before

Click the checkbox to trigger a rerun:

Rerun jobs

Learn more about TestLens at testlens.app.

jdaugherty · 2026-07-02T11:50:26Z

@jamesfredley please fix the formatting on this summary

Copilot AI review requested due to automatic review settings July 1, 2026 12:18

github-project-automation Bot added this to Apache Grails Jul 1, 2026

Copilot started reviewing on behalf of jamesfredley July 1, 2026 12:18 View session

Copilot AI reviewed Jul 1, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Reject invalid databinding indexes#15804

Reject invalid databinding indexes#15804
jamesfredley wants to merge 2 commits into
8.0.xfrom
fix/databinding-negative-index

jamesfredley commented Jul 1, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

codecov Bot commented Jul 1, 2026 •

edited

Loading

Uh oh!

jdaugherty commented Jul 1, 2026

Uh oh!

jamesfredley commented Jul 1, 2026

Uh oh!

jamesfredley commented Jul 2, 2026

Uh oh!

jamesfredley commented Jul 2, 2026

Uh oh!

testlens-app Bot commented Jul 2, 2026 •

edited

Loading

Uh oh!

jdaugherty commented Jul 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Uh oh!

Conversation

jamesfredley commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The Problem

The Fix

Files

Testing

Open discussion

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

codecov Bot commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jdaugherty commented Jul 1, 2026

Uh oh!

jamesfredley commented Jul 1, 2026

Uh oh!

jamesfredley commented Jul 2, 2026

Uh oh!

jamesfredley commented Jul 2, 2026

Uh oh!

testlens-app Bot commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🚨 TestLens detected 1 failed test 🚨

Test Summary

CI - Groovy Joint Validation Build / build_grails > :grails-test-examples-scaffolding:integrationTest

Test Failures

Muted Tests

Uh oh!

jdaugherty commented Jul 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jamesfredley commented Jul 1, 2026 •

edited

Loading

codecov Bot commented Jul 1, 2026 •

edited

Loading

testlens-app Bot commented Jul 2, 2026 •

edited

Loading