Copilot commented on code in PR #394:
URL: https://github.com/apache/commons-validator/pull/394#discussion_r3407990462
##########
src/main/java/org/apache/commons/validator/routines/BigIntegerValidator.java:
##########
@@ -157,7 +158,10 @@ public boolean minValue(final BigInteger value, final long
min) {
*/
@Override
protected Object processParsedValue(final Object value, final Format
formatter) {
- return BigInteger.valueOf(((Number) value).longValue());
+ if (value instanceof Long) {
+ return BigInteger.valueOf(((Long) value).longValue());
+ }
+ return new BigDecimal(value.toString()).toBigInteger();
}
Review Comment:
processParsedValue() now returns BigInteger values above Long.MAX_VALUE, but
this class’s range helpers (isInRange/maxValue/minValue) still compare via
value.longValue(). For values outside the long range, BigInteger.longValue()
overflows/wraps, which can make maxValue(...) and isInRange(...) return true
for numbers that are actually > max (e.g., Long.MAX_VALUE+1 wraps negative and
will pass `<= max`). To fully close the correctness hole described in the PR,
update those comparisons to use BigInteger.compareTo(BigInteger.valueOf(bound))
instead of longValue().
##########
src/test/java/org/apache/commons/validator/routines/BigIntegerValidatorTest.java:
##########
@@ -134,4 +134,19 @@ void testBigIntegerValidatorMethods() {
assertFalse(BigIntegerValidator.getInstance().isValid(xxxx, pattern),
"isValid(B) pattern");
assertFalse(BigIntegerValidator.getInstance().isValid(patternVal,
pattern, Locale.GERMAN), "isValid(B) both");
}
+
+ /**
+ * Test a value larger than {@link Long#MAX_VALUE} keeps its magnitude
instead of being
+ * clamped to {@link Long#MAX_VALUE}.
+ */
+ @Test
+ void testBigIntegerAboveLongMaxValue() {
+ // One past Long.MAX_VALUE, so NumberFormat parses it as a Double
rather than a Long.
+ final BigInteger aboveLong =
BigInteger.valueOf(Long.MAX_VALUE).add(BigInteger.ONE);
+ final String input = aboveLong.toString();
+ final BigInteger result =
BigIntegerValidator.getInstance().validate(input, "#");
+ assertTrue(result.compareTo(BigInteger.valueOf(Long.MAX_VALUE)) > 0,
"value clamped to Long.MAX_VALUE");
+ // BigDecimalValidator already preserves the magnitude, so the two
must agree
+ assertEquals(BigDecimalValidator.getInstance().validate(input,
"#").toBigInteger(), result);
+ }
Review Comment:
The new test validates that validate() no longer clamps to Long.MAX_VALUE,
but it doesn’t assert the downstream behavior that motivated this PR: that
range/max checks reject oversized inputs. Adding assertions for maxValue(...)
and isInRange(...) here will prevent regressions and will also reveal if those
helpers still overflow via longValue().
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
