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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
