Compare exact values in DoubleValidator range checks by sahvx655-wq · Pull Request #410 · apache/commons-validator

sahvx655-wq · 2026-06-24T05:50:33Z

The Number-typed range overloads on DoubleValidator are inherited from AbstractNumberValidator, which tests value.doubleValue() against bound.doubleValue(). That narrows the bound to a double before comparing, so a BigDecimal or BigInteger bound carrying more significant digits than a double can hold is rounded onto the value first. 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 defer to the superclass compareTo(Number, Number), so the bound's precision survives; this mirrors 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.

This was originally paired with the same change to FloatValidator, but I've dropped that half: at the magnitude where the bound-narrowing bug bites (around 2^53) a float spans roughly 10^9 per step, so the bound that triggers the bug always lands inside the value's round-trip interval, where Float.toString (which drives compareTo) differs between JDKs. That made the float regression test pass on JDK 21 but fail on JDK 8, and there's no portable way to pin it. DoubleValidator has no such window, so I've scoped this PR to it.

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.

garydgregory

Hello @sahvx655-wq
Please rebase on git master and see my comments.

garydgregory · 2026-06-24T12:12:03Z


    private static final FloatValidator VALIDATOR = new FloatValidator();

+    private static boolean isFinite(final Number value) {


Rebase on git master and reuse AbstractNumberValidator.isFinite(Number).

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.

garydgregory · 2026-06-24T12:13:44Z


    private static final DoubleValidator VALIDATOR = new DoubleValidator();

+    private static boolean isFinite(final Number value) {


Rebase on git master and reuse AbstractNumberValidator.isFinite(Number).

Done. Rebased on master and dropped the local helper in both DoubleValidator and FloatValidator so they use the inherited isFinite(Number).

garydgregory · 2026-06-25T11:58:08Z

+        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


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...

Simplified to BigDecimal.valueOf(maxExactInt) in the Double test. Same value, less noise.

garydgregory · 2026-06-27T11:05:31Z

+     */
+    @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();


Reuse the superclass' compareTo(Number, Number).

Switched over. Both minValue and maxValue in both validators now call the superclass compareTo(value, bound) instead of building the BigDecimal by hand.

garydgregory · 2026-06-27T11:05:41Z

+     */
+    @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();


Reuse the superclass' compareTo(Number, Number).

garydgregory · 2026-06-27T11:06:39Z

+     */
+    @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();


It looks like BigDecimal.valueOf(value.doubleValue()) looses precision.

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.

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.

sahvx655-wq · 2026-06-27T19:51:50Z

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.

sahvx655-wq · 2026-07-02T06:28:36Z

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.

garydgregory changed the title ~~compare exact values in DoubleValidator and FloatValidator range checks~~ Compare exact values in DoubleValidator and FloatValidator range checks Jun 27, 2026

garydgregory requested changes Jun 27, 2026

View reviewed changes

sahvx655-wq force-pushed the double-float-exact-bound branch from 6c209e2 to abd6bba Compare June 27, 2026 19:50

garydgregory mentioned this pull request Jul 1, 2026

Handle non-finite bounds in BigIntegerValidator range checks #414

Open

Compare exact values in DoubleValidator range checks

88399fc

sahvx655-wq force-pushed the double-float-exact-bound branch from abd6bba to 88399fc Compare July 2, 2026 06:27

sahvx655-wq changed the title ~~Compare exact values in DoubleValidator and FloatValidator range checks~~ Compare exact values in DoubleValidator range checks Jul 2, 2026

Javadoc

84ab5c3

garydgregory merged commit 23273bb into apache:master Jul 2, 2026
10 checks passed

garydgregory added a commit that referenced this pull request Jul 2, 2026

Compare exact values in DoubleValidator range checks (#410).

e066bd8


		private static final FloatValidator VALIDATOR = new FloatValidator();

		private static boolean isFinite(final Number value) {


		private static final DoubleValidator VALIDATOR = new DoubleValidator();

		private static boolean isFinite(final Number value) {

Uh oh!

Conversation

sahvx655-wq commented Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

garydgregory left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garydgregory Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sahvx655-wq commented Jun 27, 2026

Uh oh!

sahvx655-wq commented Jul 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sahvx655-wq commented Jun 24, 2026 •

edited

Loading

garydgregory Jun 25, 2026 •

edited

Loading