Reject negative index values in DefaultResolver.getIndex

[rootvector2]

getIndex parses the subscript with Integer.parseInt and returns it unvalidated, but the same method uses -1 as its documented "not indexed" sentinel, so getIndex("stringArray[-1]") is indistinguishable from getIndex("stringArray") and getIndex("stringArray[-7]") hands the caller a negative subscript. PropertyUtilsBean.getIndexedProperty/setIndexedProperty are unaffected because they have their own index < 0 guard, but the three callers that branch on index >= 0 fall through to the non-indexed path and write the whole property instead of one element. BeanUtils.copyProperty(bean, "stringArray[-1]", "x") and LocaleBeanUtils.setProperty(bean, "stringArray[-1]", "x") both replace a five-element String[] with ["x"] and throw nothing, while BeanUtils.setProperty and PropertyUtils.setIndexedProperty reject the same expression. Found while auditing DefaultResolver against its call sites after noticing getIndex can return a value that collides with its own sentinel.

Indexes are zero-relative, so a negative subscript is rejected in getIndex with the same IllegalArgumentException it already throws for a non-numeric one. Keeping the check where the subscript is parsed covers all five call sites at once, and DefaultResolver is the only Resolver implementation. No existing test used a negative index; the new assertions in DefaultResolverTest.testGetIndex fail without the runtime change.

• Read the contribution guidelines for this project.
• Read the ASF Generative Tooling Guidance if you use Artificial Intelligence (AI).
• I used AI to create any part of, or all of, this pull request. Which AI tool was used to create this pull request, and to what extent did it contribute?
• Run a successful build using the default Maven goal with mvn; that's mvn on the command line by itself.
• Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best practice.
• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.

[Copilot]

Pull request overview

This pull request fixes an inconsistency in DefaultResolver.getIndex(...) by rejecting negative subscripts, preventing negative indices from colliding with the -1 “not indexed” sentinel and avoiding unintended fall-through to non-indexed property paths.

Changes:

• Reject negative indexed subscripts in DefaultResolver.getIndex by throwing IllegalArgumentException.
• Update Resolver/DefaultResolver Javadoc to document that negative indices are invalid.
• Extend DefaultResolverTest.testGetIndex with assertions covering negative indices and error messages.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/main/java/org/apache/commons/beanutils2/expression/DefaultResolver.java	Adds a negative-index guard to prevent -1 sentinel collisions and unintended non-indexed behavior.
src/main/java/org/apache/commons/beanutils2/expression/Resolver.java	Updates API documentation to reflect that negative indices are invalid.
src/test/java/org/apache/commons/beanutils2/expression/DefaultResolverTest.java	Adds regression tests ensuring negative indices throw IllegalArgumentException.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[garydgregory]

@rootvector2
Java has am unchecked IndexOutOfBoundsException. Should it be used here? Are there other call sites that use IAE for this category of problems instead of IndexOutOfBoundsException?

[rootvector2]

The repo's convention is IAE for this category: PropertyUtilsBean.getIndexedProperty and setIndexedProperty both throw IllegalArgumentException when the index parsed from the expression is negative ("Invalid indexed property ..."), and getIndex itself already throws IAE for a non-numeric subscript with the same "Invalid index value" message this check reuses. IndexOutOfBoundsException is used for the other category, a well-formed index that falls outside the actual array or List at access time; PropertyUtilsBean propagates it from the underlying access. A negative subscript is a malformed expression rather than an out-of-range access, and at parse time there's no collection to be out of bounds of. One more practical point: both PropertyUtilsBean call sites catch IllegalArgumentException from resolver.getIndex and rewrap it with bean-class context, so an IndexOutOfBoundsException thrown there would bypass those handlers and change their existing message contract.