You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "sahvx655-wq (via GitHub)" <gi...@apache.org> on 2026/06/24 05:50:35 UTC
[PR] compare exact values in DoubleValidator and FloatValidator range checks [commons-validator]
sahvx655-wq opened a new pull request, #410:
URL: https://github.com/apache/commons-validator/pull/410
The Number-typed range overloads on `DoubleValidator` and `FloatValidator` are inherited from `AbstractNumberValidator`, which tests `value.doubleValue()` against `bound.doubleValue()`. While reviewing the exact-comparison work already done on the `BigInteger` and `BigDecimal` validators I noticed the two floating-point validators still take that narrowing path, so a `BigDecimal` or `BigInteger` bound carrying more significant digits than a `double` can hold is rounded onto the value before the test. `minValue(2^53, BigInteger 2^53 + 1)` returns `true` and `maxValue(2^53, 2^53 - 0.5)` returns `true`, both wrong: a value that sits outside the bound is reported as in range, and it then slips through any `isInRange` guard built on these methods.
The two overrides keep the bound as a `BigDecimal` and compare the value's `double` against it, so the bound's precision survives; this follows the `compareTo` comparisons already used in `BigIntegerValidator` and `BigDecimalValidator`. A non-finite operand keeps the old `doubleValue()` comparison so the documented infinity and NaN behaviour is unchanged. Placing the check in the callee means `isInRange(Number, Number, Number)` picks it up through virtual dispatch rather than each caller needing its own guard.
- [x] Read the [contribution guidelines](CONTRIBUTING.md) for this project.
- [ ] Read the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) 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?
- [x] Run a successful build using the default [Maven](https://maven.apache.org/) goal with `mvn`; that's `mvn` on the command line by itself.
- [x] 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.
- [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] 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.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Compare exact values in DoubleValidator and FloatValidator range checks [commons-validator]
Posted by "sahvx655-wq (via GitHub)" <gi...@apache.org>.
sahvx655-wq commented on code in PR #410:
URL: https://github.com/apache/commons-validator/pull/410#discussion_r3486674831
##########
src/main/java/org/apache/commons/validator/routines/DoubleValidator.java:
##########
@@ -68,6 +69,16 @@ public class DoubleValidator extends AbstractNumberValidator {
private static final DoubleValidator VALIDATOR = new DoubleValidator();
+ private static boolean isFinite(final Number value) {
Review Comment:
Done. Rebased on master and dropped the local helper in both DoubleValidator and FloatValidator so they use the inherited isFinite(Number).
##########
src/main/java/org/apache/commons/validator/routines/DoubleValidator.java:
##########
@@ -183,6 +194,42 @@ public boolean minValue(final Double value, final double min) {
return minValue(value.doubleValue(), min);
}
+ /**
+ * Tests if the value is less than or equal to a maximum, comparing the exact values.
+ *
+ * <p>
+ * This overrides the {@link Number} overload inherited from the superclass, which narrows the bound to a {@code double} before comparing and so loses
+ * precision for a {@code BigDecimal} or {@code BigInteger} bound that carries more significant digits than a {@code double} can hold. A non-finite
+ * {@link Double} or {@link Float} operand keeps the {@code doubleValue()} comparison so the documented infinity behaviour is unchanged.
+ * </p>
+ *
+ * @param value The value validation is being performed on.
+ * @param max The maximum value.
+ * @return {@code true} if the value is less than or equal to the maximum.
+ */
+ @Override
+ public boolean maxValue(final Number value, final Number max) {
+ return isFinite(value) && isFinite(max) ? BigDecimal.valueOf(value.doubleValue()).compareTo(toBigDecimal(max)) <= 0 : value.doubleValue() <= max.doubleValue();
Review Comment:
Switched over. Both minValue and maxValue in both validators now call the superclass compareTo(value, bound) instead of building the BigDecimal by hand.
##########
src/main/java/org/apache/commons/validator/routines/FloatValidator.java:
##########
@@ -183,6 +194,42 @@ public boolean minValue(final Float value, final float min) {
return minValue(value.floatValue(), min);
}
+ /**
+ * Tests if the value is less than or equal to a maximum, comparing the exact values.
+ *
+ * <p>
+ * This overrides the {@link Number} overload inherited from the superclass, which narrows the bound to a {@code double} before comparing and so loses
+ * precision for a {@code BigDecimal} or {@code BigInteger} bound that carries more significant digits than a {@code double} can hold. A non-finite
+ * {@link Double} or {@link Float} operand keeps the {@code doubleValue()} comparison so the documented infinity behaviour is unchanged.
+ * </p>
+ *
+ * @param value The value validation is being performed on.
+ * @param max The maximum value.
+ * @return {@code true} if the value is less than or equal to the maximum.
+ */
+ @Override
+ public boolean maxValue(final Number value, final Number max) {
+ return isFinite(value) && isFinite(max) ? BigDecimal.valueOf(value.doubleValue()).compareTo(toBigDecimal(max)) <= 0 : value.doubleValue() <= max.doubleValue();
Review Comment:
Right, that was the wrong basis. Going through compareTo(value, max) runs the value through toBigDecimal, which for a Float uses Float.toString, so the float is compared at its own precision (9.007199E15) rather than the extra digits doubleValue() invents when it widens the float. I adjusted the FloatValidatorTest expectations to that value.
##########
src/test/java/org/apache/commons/validator/routines/DoubleValidatorTest.java:
##########
@@ -132,6 +134,26 @@ void testDoubleRangeMinMaxNaN() {
assertFalse(validator.maxValue(Double.NaN, 20), "maxValue() NaN");
}
+ /**
+ * Test the {@link Number} range checks against a bound that carries more precision than a {@code double}.
+ * 2^53 is the largest integer with an exact {@code double} representation, so 2^53 + 1 cannot be narrowed
+ * onto the value: a value of 2^53 is below a minimum of 2^53 + 1 and above a maximum of 2^53 - 0.5.
+ */
+ @Test
+ void testDoubleNumberRangeExactBound() {
+ final DoubleValidator validator = (DoubleValidator) strictValidator;
+ final long maxExactInt = 1L << 53; // 2^53
+ final Double value = Double.valueOf(maxExactInt);
+ final BigInteger above = BigInteger.valueOf(maxExactInt).add(BigInteger.ONE); // 2^53 + 1
+ final BigInteger below = BigInteger.valueOf(maxExactInt).subtract(BigInteger.ONE); // 2^53 - 1
+ final BigDecimal justBelow = new BigDecimal(BigInteger.valueOf(maxExactInt)).subtract(BigDecimal.valueOf(0.5)); // 2^53 - 0.5
Review Comment:
Simplified to BigDecimal.valueOf(maxExactInt) in the Double test. Same value, less noise.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Compare exact values in DoubleValidator and FloatValidator range checks [commons-validator]
Posted by "sahvx655-wq (via GitHub)" <gi...@apache.org>.
sahvx655-wq commented on PR #410:
URL: https://github.com/apache/commons-validator/pull/410#issuecomment-4820909917
Rebased on master and reused the pulled-up helpers throughout: both validators drop the local isFinite copy for the inherited isFinite(Number), and minValue/maxValue now defer to the superclass compareTo(Number, Number) rather than assembling the BigDecimal locally. The test bound is BigDecimal.valueOf(...) now too.
On the float precision point: you were right that BigDecimal.valueOf(value.doubleValue()) was wrong. compareTo runs the value through toBigDecimal, and for a Float that uses Float.toString, so the comparison sits at the float's own decimal value (9.007199E15) instead of the trailing digits doubleValue() fabricates when it widens the float. That moved the FloatValidatorTest expectations onto 9.007199E15, which is the honest bound for a float at that magnitude. mvn clean verify is green.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Compare exact values in DoubleValidator and FloatValidator range checks [commons-validator]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #410:
URL: https://github.com/apache/commons-validator/pull/410#discussion_r3466970639
##########
src/main/java/org/apache/commons/validator/routines/DoubleValidator.java:
##########
@@ -68,6 +69,16 @@ public class DoubleValidator extends AbstractNumberValidator {
private static final DoubleValidator VALIDATOR = new DoubleValidator();
+ private static boolean isFinite(final Number value) {
Review Comment:
Rebase on git master and reuse AbstractNumberValidator.isFinite(Number).
##########
src/test/java/org/apache/commons/validator/routines/DoubleValidatorTest.java:
##########
@@ -132,6 +134,26 @@ void testDoubleRangeMinMaxNaN() {
assertFalse(validator.maxValue(Double.NaN, 20), "maxValue() NaN");
}
+ /**
+ * Test the {@link Number} range checks against a bound that carries more precision than a {@code double}.
+ * 2^53 is the largest integer with an exact {@code double} representation, so 2^53 + 1 cannot be narrowed
+ * onto the value: a value of 2^53 is below a minimum of 2^53 + 1 and above a maximum of 2^53 - 0.5.
+ */
+ @Test
+ void testDoubleNumberRangeExactBound() {
+ final DoubleValidator validator = (DoubleValidator) strictValidator;
+ final long maxExactInt = 1L << 53; // 2^53
+ final Double value = Double.valueOf(maxExactInt);
+ final BigInteger above = BigInteger.valueOf(maxExactInt).add(BigInteger.ONE); // 2^53 + 1
+ final BigInteger below = BigInteger.valueOf(maxExactInt).subtract(BigInteger.ONE); // 2^53 - 1
+ final BigDecimal justBelow = new BigDecimal(BigInteger.valueOf(maxExactInt)).subtract(BigDecimal.valueOf(0.5)); // 2^53 - 0.5
Review Comment:
Why is this not just `BigDecimal.valueOf(maxExactInt)` instead of `new BigDecimal(BigInteger.valueOf(maxExactInt))`?
##########
src/main/java/org/apache/commons/validator/routines/DoubleValidator.java:
##########
@@ -183,6 +194,42 @@ public boolean minValue(final Double value, final double min) {
return minValue(value.doubleValue(), min);
}
+ /**
+ * Tests if the value is less than or equal to a maximum, comparing the exact values.
+ *
+ * <p>
+ * This overrides the {@link Number} overload inherited from the superclass, which narrows the bound to a {@code double} before comparing and so loses
+ * precision for a {@code BigDecimal} or {@code BigInteger} bound that carries more significant digits than a {@code double} can hold. A non-finite
+ * {@link Double} or {@link Float} operand keeps the {@code doubleValue()} comparison so the documented infinity behaviour is unchanged.
+ * </p>
+ *
+ * @param value The value validation is being performed on.
+ * @param max The maximum value.
+ * @return {@code true} if the value is less than or equal to the maximum.
+ */
+ @Override
+ public boolean maxValue(final Number value, final Number max) {
+ return isFinite(value) && isFinite(max) ? BigDecimal.valueOf(value.doubleValue()).compareTo(toBigDecimal(max)) <= 0 : value.doubleValue() <= max.doubleValue();
Review Comment:
Reuse the superclass' compareTo(Number, Number).
##########
src/main/java/org/apache/commons/validator/routines/FloatValidator.java:
##########
@@ -183,6 +194,42 @@ public boolean minValue(final Float value, final float min) {
return minValue(value.floatValue(), min);
}
+ /**
+ * Tests if the value is less than or equal to a maximum, comparing the exact values.
+ *
+ * <p>
+ * This overrides the {@link Number} overload inherited from the superclass, which narrows the bound to a {@code double} before comparing and so loses
+ * precision for a {@code BigDecimal} or {@code BigInteger} bound that carries more significant digits than a {@code double} can hold. A non-finite
+ * {@link Double} or {@link Float} operand keeps the {@code doubleValue()} comparison so the documented infinity behaviour is unchanged.
+ * </p>
+ *
+ * @param value The value validation is being performed on.
+ * @param max The maximum value.
+ * @return {@code true} if the value is less than or equal to the maximum.
+ */
+ @Override
+ public boolean maxValue(final Number value, final Number max) {
+ return isFinite(value) && isFinite(max) ? BigDecimal.valueOf(value.doubleValue()).compareTo(toBigDecimal(max)) <= 0 : value.doubleValue() <= max.doubleValue();
Review Comment:
It looks like `BigDecimal.valueOf(value.doubleValue())` looses precision.
##########
src/main/java/org/apache/commons/validator/routines/FloatValidator.java:
##########
@@ -68,6 +69,16 @@ public class FloatValidator extends AbstractNumberValidator {
private static final FloatValidator VALIDATOR = new FloatValidator();
+ private static boolean isFinite(final Number value) {
Review Comment:
Rebase on git master and reuse AbstractNumberValidator.isFinite(Number).
##########
src/main/java/org/apache/commons/validator/routines/DoubleValidator.java:
##########
@@ -183,6 +194,42 @@ public boolean minValue(final Double value, final double min) {
return minValue(value.doubleValue(), min);
}
+ /**
+ * Tests if the value is less than or equal to a maximum, comparing the exact values.
+ *
+ * <p>
+ * This overrides the {@link Number} overload inherited from the superclass, which narrows the bound to a {@code double} before comparing and so loses
+ * precision for a {@code BigDecimal} or {@code BigInteger} bound that carries more significant digits than a {@code double} can hold. A non-finite
+ * {@link Double} or {@link Float} operand keeps the {@code doubleValue()} comparison so the documented infinity behaviour is unchanged.
+ * </p>
+ *
+ * @param value The value validation is being performed on.
+ * @param max The maximum value.
+ * @return {@code true} if the value is less than or equal to the maximum.
+ */
+ @Override
+ public boolean maxValue(final Number value, final Number max) {
+ return isFinite(value) && isFinite(max) ? BigDecimal.valueOf(value.doubleValue()).compareTo(toBigDecimal(max)) <= 0 : value.doubleValue() <= max.doubleValue();
+ }
+
+ /**
+ * Tests if the value is greater than or equal to a minimum, comparing the exact values.
+ *
+ * <p>
+ * This overrides the {@link Number} overload inherited from the superclass, which narrows the bound to a {@code double} before comparing and so loses
+ * precision for a {@code BigDecimal} or {@code BigInteger} bound that carries more significant digits than a {@code double} can hold. A non-finite
+ * {@link Double} or {@link Float} operand keeps the {@code doubleValue()} comparison so the documented infinity behaviour is unchanged.
+ * </p>
+ *
+ * @param value The value validation is being performed on.
+ * @param min The minimum value.
+ * @return {@code true} if the value is greater than or equal to the minimum.
+ */
+ @Override
+ public boolean minValue(final Number value, final Number min) {
+ return isFinite(value) && isFinite(min) ? BigDecimal.valueOf(value.doubleValue()).compareTo(toBigDecimal(min)) >= 0 : value.doubleValue() >= min.doubleValue();
Review Comment:
Reuse the superclass' compareTo(Number, Number).
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Compare exact values in DoubleValidator and FloatValidator range checks [commons-validator]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #410:
URL: https://github.com/apache/commons-validator/pull/410#discussion_r3474170191
##########
src/test/java/org/apache/commons/validator/routines/DoubleValidatorTest.java:
##########
@@ -132,6 +134,26 @@ void testDoubleRangeMinMaxNaN() {
assertFalse(validator.maxValue(Double.NaN, 20), "maxValue() NaN");
}
+ /**
+ * Test the {@link Number} range checks against a bound that carries more precision than a {@code double}.
+ * 2^53 is the largest integer with an exact {@code double} representation, so 2^53 + 1 cannot be narrowed
+ * onto the value: a value of 2^53 is below a minimum of 2^53 + 1 and above a maximum of 2^53 - 0.5.
+ */
+ @Test
+ void testDoubleNumberRangeExactBound() {
+ final DoubleValidator validator = (DoubleValidator) strictValidator;
+ final long maxExactInt = 1L << 53; // 2^53
+ final Double value = Double.valueOf(maxExactInt);
+ final BigInteger above = BigInteger.valueOf(maxExactInt).add(BigInteger.ONE); // 2^53 + 1
+ final BigInteger below = BigInteger.valueOf(maxExactInt).subtract(BigInteger.ONE); // 2^53 - 1
+ final BigDecimal justBelow = new BigDecimal(BigInteger.valueOf(maxExactInt)).subtract(BigDecimal.valueOf(0.5)); // 2^53 - 0.5
Review Comment:
Why is this not just `BigDecimal.valueOf(maxExactInt)` instead of `new BigDecimal(BigInteger.valueOf(maxExactInt))` but since this is Double validator instead of a "Big" validator is might be OK...
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Compare exact values in DoubleValidator range checks [commons-validator]
Posted by "sahvx655-wq (via GitHub)" <gi...@apache.org>.
sahvx655-wq commented on code in PR #410:
URL: https://github.com/apache/commons-validator/pull/410#discussion_r3510929189
##########
src/main/java/org/apache/commons/validator/routines/FloatValidator.java:
##########
@@ -68,6 +69,16 @@ public class FloatValidator extends AbstractNumberValidator {
private static final FloatValidator VALIDATOR = new FloatValidator();
+ private static boolean isFinite(final Number value) {
Review Comment:
Dropped the FloatValidator change from this PR, so this thread is moot. Reasoning in the PR comment: the float override can't be pinned by a JDK-portable test at 2^53.
##########
src/main/java/org/apache/commons/validator/routines/FloatValidator.java:
##########
@@ -183,6 +194,42 @@ public boolean minValue(final Float value, final float min) {
return minValue(value.floatValue(), min);
}
+ /**
+ * Tests if the value is less than or equal to a maximum, comparing the exact values.
+ *
+ * <p>
+ * This overrides the {@link Number} overload inherited from the superclass, which narrows the bound to a {@code double} before comparing and so loses
+ * precision for a {@code BigDecimal} or {@code BigInteger} bound that carries more significant digits than a {@code double} can hold. A non-finite
+ * {@link Double} or {@link Float} operand keeps the {@code doubleValue()} comparison so the documented infinity behaviour is unchanged.
+ * </p>
+ *
+ * @param value The value validation is being performed on.
+ * @param max The maximum value.
+ * @return {@code true} if the value is less than or equal to the maximum.
+ */
+ @Override
+ public boolean maxValue(final Number value, final Number max) {
+ return isFinite(value) && isFinite(max) ? BigDecimal.valueOf(value.doubleValue()).compareTo(toBigDecimal(max)) <= 0 : value.doubleValue() <= max.doubleValue();
Review Comment:
Removed the FloatValidator override entirely, so this is no longer in play. The precision point is exactly why it can't be tested portably at that magnitude; details in the PR comment.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Compare exact values in DoubleValidator range checks [commons-validator]
Posted by "sahvx655-wq (via GitHub)" <gi...@apache.org>.
sahvx655-wq commented on PR #410:
URL: https://github.com/apache/commons-validator/pull/410#issuecomment-4862828727
Sorry this sat red for a week. Chased down the CI failure: only the JDK 8 job actually failed, the rest of the matrix was cancelled by fail-fast, so it looked worse than it was. The culprit was my own testFloatNumberRangeExactBound. A float at 2^53 has a round-trip interval about a float ULP wide (~10^9 there), and compareTo runs the value through toBigDecimal, which for a Float goes via Float.toString. JDK 19+ emits the shortest form (9.007199E15) while JDK 8's older algorithm emits the exact 2^53, and my bound sat between the two, so the assertion flipped by JDK.
The deeper problem is that any bound precise enough to trigger the double-narrowing bug has to fall within one double ULP of the value, which at that magnitude is always inside the float's round-trip interval, so there's no portable way to pin the float override in a test. I've dropped FloatValidator and scoped the PR to Double, where doubleValue() is exact and the test is stable (it already passed on JDK 8). The Double overrides defer to the superclass compareTo(Number, Number) and the inherited isFinite(Number) as you asked, and the test bound is BigDecimal.valueOf(...). mvn clean verify is green locally.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org