Skip to content

[java] Keys: enforce CharSequence contract in charAt()#17166

Merged
asolntsev merged 5 commits intoSeleniumHQ:trunkfrom
seethinajayadileep:fix-scriptkey-charat-bounds
Mar 4, 2026
Merged

[java] Keys: enforce CharSequence contract in charAt()#17166
asolntsev merged 5 commits intoSeleniumHQ:trunkfrom
seethinajayadileep:fix-scriptkey-charat-bounds

Conversation

@seethinajayadileep
Copy link
Contributor

💥 What does this PR do?

Updates Keys.charAt(int index) to validate the index and throw
IndexOutOfBoundsException when the index is not 0.

Previously, the method returned the null character ('\u0000') for any
index other than 0, which silently violated the CharSequence contract.

This change ensures:

  • length() returns 1
  • charAt(0) returns the key code
  • charAt(index != 0) throws IndexOutOfBoundsException

This aligns the implementation with expected CharSequence behavior.


🔧 Implementation Notes

Keys represents a single Unicode PUA character. Since its length is
always 1, only index 0 is valid.

Instead of returning '\u0000' for invalid indexes (which can mask bugs
and produce incorrect comparisons), the method now explicitly throws
IndexOutOfBoundsException.

Alternative considered:

  • Keeping the previous behavior (returning '\u0000') — rejected because
    it silently hides incorrect usage and breaks contract expectations.

💡 Additional Considerations

  • No behavioral change for valid usage (charAt(0)).
  • Improves correctness when used in string comparisons or iteration.
  • No impact on toString(), subSequence(), or chord() behavior.

🔄 Types of changes

  • Bug fix (backwards compatible)

@qodo-code-review
Copy link
Contributor

Review Summary by Qodo

Enforce CharSequence contract in Keys.charAt() bounds checking

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Enforce bounds checking in Keys.charAt() method
• Throw IndexOutOfBoundsException for invalid indexes
• Comply with CharSequence contract requirements
• Prevent silent violations and incorrect comparisons

Grey Divider

File Changes

1. java/src/org/openqa/selenium/Keys.java 🐞 Bug fix +3/-3

Add bounds validation to Keys.charAt() method

• Changed charAt(int index) to validate index parameter
• Now throws IndexOutOfBoundsException when index != 0
• Removed silent return of null character ('\u0000')
• Ensures compliance with CharSequence interface contract

java/src/org/openqa/selenium/Keys.java


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Mar 3, 2026

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 📎 Requirement gaps (0)

Grey Divider


Action required

1. KeysTest expects charAt(10)📘 Rule violation ⛯ Reliability
Description
Keys.charAt(int) now throws IndexOutOfBoundsException for any index other than 0, but the
existing unit test still asserts that charAt(10) returns 0. This functional change needs
corresponding test updates/additions to avoid incorrect expectations and failing CI.
Code

java/src/org/openqa/selenium/Keys.java[R143-146]

+    if (index != 0) {
+      throw new IndexOutOfBoundsException("Index: " + index + ", Length: 1");
   }
-    return 0;
+    return keyCode;
Evidence
PR Compliance ID 5 requires updating/adding tests for functional changes. The PR changes
Keys.charAt to throw on index != 0, while the existing unit test still expects a 0 return
value for a non-zero index, so tests must be updated to match the new contract.

AGENTS.md
java/src/org/openqa/selenium/Keys.java[141-147]
java/test/org/openqa/selenium/KeysTest.java[37-40]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Keys.charAt(int)` now throws for `index != 0`, but `KeysTest.charAtOtherPositionReturnsZero()` still asserts a `0` return value, which no longer matches the new behavior.
## Issue Context
The production change enforces the `CharSequence` contract by throwing `IndexOutOfBoundsException` for invalid indices. The unit test suite should be updated to reflect this and prevent CI failures.
## Fix Focus Areas
- java/test/org/openqa/selenium/KeysTest.java[32-41]
- java/src/org/openqa/selenium/Keys.java[141-147]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

@selenium-ci selenium-ci added the C-java Java Bindings label Mar 3, 2026
@seethinajayadileep seethinajayadileep force-pushed the fix-scriptkey-charat-bounds branch from 4f2a4d1 to 2754996 Compare March 3, 2026 07:36
@cgoldberg
Copy link
Member

@seethinajayadileep I just approved the workflows to run for this PR, but we are having some issues so you will likely see some unrelated test failures in CI.

@asolntsev asolntsev added this to the 4.42.0 milestone Mar 4, 2026
@asolntsev asolntsev merged commit 5b3c36c into SeleniumHQ:trunk Mar 4, 2026
43 of 44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-java Java Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants