Preserve fractional bound in BigIntegerValidator range checks#407
Merged
Conversation
garydgregory
requested changes
Jun 22, 2026
garydgregory
left a comment
Member
There was a problem hiding this comment.
@sahvx655-wq
Thank you for the PR.
Please see my comment.
| if (value instanceof BigInteger) { | ||
| return new BigDecimal((BigInteger) value); | ||
| } | ||
| if (value instanceof Long) { |
Member
There was a problem hiding this comment.
Hello @sahvx655-wq
You missed the java.math.BigDecimal.valueOf(double) shortcut here.
Contributor
Author
There was a problem hiding this comment.
Good catch. I've added a Double branch using BigDecimal.valueOf(((Double) value).doubleValue()) so it matches the toBigInteger helper just below, which already takes that path with the same "no need to roundtrip with a string" note. The fall-through stays on new BigDecimal(value.toString()) for the remaining types (Float keeps its own canonical short form that way rather than picking up the widened double representation).
Pushed in 3b48460 and the range tests still pass.
garydgregory
added a commit
that referenced
this pull request
Jun 22, 2026
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
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.
The Number-typed minValue and maxValue overrides convert both operands with toBigInteger before comparing, and toBigInteger truncates a non-integer bound towards zero. A fractional bound therefore loses its fractional part before the test, so the verdict is wrong: minValue(BigInteger 5, 5.9) returns true although 5 is below 5.9 (the bound is floored to 5), and maxValue(BigInteger -5, -5.9) returns true although -5 is above -5.9. I noticed this while checking these overloads against BigDecimalValidator, which already compares the same mixed Number bounds exactly, so the two validators disagree on identical inputs.
The fix compares both operands as BigDecimal so a non-integer bound keeps its value. Whole-number bounds and BigIntegers outside the long range are unaffected, and keeping the comparison inside the override mirrors BigDecimalValidator rather than pushing rounding onto callers. Left unfixed, any range check given a Double, Float or BigDecimal bound with a fractional part can admit values just outside the requested range.