[java] Fix silent overflow when coercing JSON numbers to integral types#17177
Open
seethinajayadileep wants to merge 4 commits intoSeleniumHQ:trunkfrom
Open
[java] Fix silent overflow when coercing JSON numbers to integral types#17177seethinajayadileep wants to merge 4 commits intoSeleniumHQ:trunkfrom
seethinajayadileep wants to merge 4 commits intoSeleniumHQ:trunkfrom
Conversation
Contributor
Review Summary by QodoFix silent overflow when coercing JSON numbers to integral types
WalkthroughsDescription• Prevents silent overflow when coercing JSON numbers to integral types • Validates numeric values are within target type bounds before coercion • Rejects fractional values for integral type targets • Returns BigDecimal instead of double from nextNumber() for precision File Changes1. java/src/org/openqa/selenium/json/JsonInput.java
|
Contributor
Code Review by Qodo
1. Overflow fix lacks tests
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+85
to
+130
| validateIntegralRange(number, stereotype); | ||
| return mapper.apply(number); | ||
| }; | ||
| } | ||
|
|
||
| private static void validateIntegralRange(Number number, Class<?> stereotype) { | ||
| // Only for integral targets | ||
| if (!(stereotype == Byte.class | ||
| || stereotype == Short.class | ||
| || stereotype == Integer.class | ||
| || stereotype == Long.class)) { | ||
| return; | ||
| } | ||
|
|
||
| final BigInteger value; | ||
| try { | ||
| // Parses "2147483648", "1e3", "1.0" safely; rejects "1.2" | ||
| BigDecimal bd = | ||
| (number instanceof BigDecimal) ? (BigDecimal) number : new BigDecimal(number.toString()); | ||
| value = bd.toBigIntegerExact(); | ||
| } catch (RuntimeException e) { | ||
| throw new JsonException( | ||
| "Expected an integer value for " + stereotype.getSimpleName() + ": " + number, e); | ||
| } | ||
|
|
||
| BigInteger min; | ||
| BigInteger max; | ||
|
|
||
| if (stereotype == Byte.class) { | ||
| min = BigInteger.valueOf(Byte.MIN_VALUE); | ||
| max = BigInteger.valueOf(Byte.MAX_VALUE); | ||
| } else if (stereotype == Short.class) { | ||
| min = BigInteger.valueOf(Short.MIN_VALUE); | ||
| max = BigInteger.valueOf(Short.MAX_VALUE); | ||
| } else if (stereotype == Integer.class) { | ||
| min = BigInteger.valueOf(Integer.MIN_VALUE); | ||
| max = BigInteger.valueOf(Integer.MAX_VALUE); | ||
| } else { // Long.class | ||
| min = BigInteger.valueOf(Long.MIN_VALUE); | ||
| max = BigInteger.valueOf(Long.MAX_VALUE); | ||
| } | ||
|
|
||
| if (value.compareTo(min) < 0 || value.compareTo(max) > 0) { | ||
| throw new JsonException( | ||
| "Numeric value out of range for " + stereotype.getSimpleName() + ": " + value); | ||
| } |
Contributor
There was a problem hiding this comment.
1. Overflow fix lacks tests 📘 Rule violation ⛯ Reliability
The new integral range validation changes coercion behavior (now throwing JsonException instead of silently overflowing) but no new/updated unit tests are included to lock in the expected behavior. This risks regressions and leaves key edge cases (fractional, out-of-range) unverified.
Agent Prompt
## Issue description
Behavior was changed to reject fractional/out-of-range coercions and to alter numeric representations, but unit tests were not updated/added to enforce the new expectations.
## Issue Context
The change is intended to prevent silent overflow for `Byte`/`Short`/`Integer`/`Long` coercions and to fail fast with `JsonException`.
## Fix Focus Areas
- java/src/org/openqa/selenium/json/NumberCoercer.java[85-131]
- java/src/org/openqa/selenium/json/JsonInput.java[257-269]
- java/test/org/openqa/selenium/json/JsonInputTest.java[74-79]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
seethinajayadileep
force-pushed
the
fix-numbercoercer-overflow
branch
from
69eff0d to
5ad714b
Compare
seethinajayadileep
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
💥 What does this PR do?
This PR fixes a silent overflow issue when coercing JSON numbers into Java integral types (
Byte,Short,Integer,Long).Previously,
NumberCoercerrelied onNumber#byteValue,shortValue,intValue, andlongValuethrough the mapper. These methods silently overflow when the numeric value is outside the target type range.Examples of the previous incorrect behavior:
"2147483648"coerced toIntegerresulted in-2147483648"100000"coerced toShortresulted in an incorrect wrapped valueThis change adds validation before applying the mapper:
BigDecimaltoBigIntegerExact()to ensure the value is an exact integerJsonExceptionif the value is fractional or out of rangeThis prevents silent numeric corruption during JSON deserialization.
🔧 Implementation Notes
The validation logic was added inside
NumberCoercerbefore applying the numeric mapper.Implementation steps:
NumbertoBigDecimaltoBigIntegerExact()to reject fractional valuesByte,Short,Integer,Long)JsonExceptionif the value is outside the allowed bounds🧪 Tests
Unit tests were added to verify the new behavior:
JsonExceptionThese tests ensure the overflow validation remains enforced.
🔄 Compatibility
JsonInput#nextNumber() behavior was preserved to continue returning Double values,
avoiding changes to existing callers that rely on the current runtime type.
This change applies to the Java binding. Other Selenium bindings may handle
numeric coercion differently and can be aligned in follow-up changes if needed.
💡 Additional Considerations
This change only affects invalid numeric inputs that previously produced incorrect wrapped values. Valid numeric inputs continue to behave the same.
🔄 Types of changes