Skip to content

Commit

Permalink
JEXL-417: fix precision loss in JexlArithmetic;
Browse files Browse the repository at this point in the history 
- added tests;
  • Loading branch information
Henri Biestro committed Nov 28, 2023
1 parent d09df7e commit 5ab46d9
Show file tree
Hide file tree
Showing 4 changed files with 114 additions and 66 deletions.
1 change: 1 addition & 0 deletions RELEASE-NOTES.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ New Features in 3.3.1:

Bugs Fixed in 3.3.1:
===================
* JEXL-417: JexlArithmetic looses precision during arithmetic operator execution
* JEXL-416: Null-valued pragma throws NPE in 3.3
* JEXL-415: Incorrect template eval result
* JEXL-414: SoftCache may suffer from race conditions
Expand Down
3 changes: 3 additions & 0 deletions src/changes/changes.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@
Allow 'trailing commas' or ellipsis while defining array, map and set literals
</action>
<!-- FIX -->
<action dev="henrib" type="fix" issue="JEXL-417" due-to="Robert Lucas" >
JexlArithmetic looses precision during arithmetic operator execution
</action>
<action dev="henrib" type="fix" issue="JEXL-416" due-to="William Price" >
Null-valued pragma throws NPE in 3.3
</action>
Expand Down
125 changes: 73 additions & 52 deletions src/main/java/org/apache/commons/jexl3/JexlArithmetic.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import java.util.Collection;
import java.util.Map;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

Expand Down Expand Up @@ -1062,14 +1063,14 @@ public Number narrowNumber(final Number original, final Class<?> narrow) {
protected Number narrowBigInteger(final Object lhs, final Object rhs, final BigInteger bigi) {
//coerce to long if possible
if ((isNumberable(lhs) || isNumberable(rhs))
&& bigi.compareTo(BIGI_LONG_MAX_VALUE) <= 0
&& bigi.compareTo(BIGI_LONG_MIN_VALUE) >= 0) {
&& bigi.compareTo(BIGI_LONG_MAX_VALUE) <= 0
&& bigi.compareTo(BIGI_LONG_MIN_VALUE) >= 0) {
// coerce to int if possible
final long l = bigi.longValue();
// coerce to int when possible (int being so often used in method parms)
if (!(lhs instanceof Long || rhs instanceof Long)
&& l <= Integer.MAX_VALUE
&& l >= Integer.MIN_VALUE) {
&& l <= Integer.MAX_VALUE
&& l >= Integer.MIN_VALUE) {
return (int) l;
}
return l;
Expand All @@ -1079,7 +1080,7 @@ protected Number narrowBigInteger(final Object lhs, final Object rhs, final BigI

/**
* Given a BigDecimal, attempt to narrow it to an Integer or Long if it fits and
* one of the arguments is a numberable.
* one of the arguments is numberable.
*
* @param lhs the left-hand side operand that lead to the bigd result
* @param rhs the right-hand side operand that lead to the bigd result
Expand Down Expand Up @@ -1144,17 +1145,42 @@ protected Number narrowLong(final Object lhs, final Object rhs, final long r) {

/**
* Checks if value class is a number that can be represented exactly in a long.
* <p>For convenience, booleans are converted as 1/0 (true/false).</p>
*
* @param value argument
* @return true if argument can be represented by a long
*/
protected Number asLongNumber(final Object value) {
return value instanceof Long
|| value instanceof Integer
|| value instanceof Short
|| value instanceof Byte
? (Number) value
: null;
return asLongNumber(strict, value);
}

/**
* Checks if value class is a number that can be represented exactly in a long.
* <p>For convenience, booleans are converted as 1/0 (true/false).</p>
*
* @param strict whether null argument is converted as 0 or remains null
* @param value argument
* @return a non-null value if argument can be represented by a long
*/
protected Number asLongNumber(final boolean strict, final Object value) {
if (value instanceof Long
|| value instanceof Integer
|| value instanceof Short
|| value instanceof Byte) {
return (Number) value;
}
if (value instanceof Boolean) {
Boolean b = (Boolean) value;
return b ? 1L : 0L;
}
if (value instanceof AtomicBoolean) {
AtomicBoolean b = (AtomicBoolean) value;
return b.get() ? 1L : 0L;
}
if (value == null && !strict) {
return 0L;
}
return null;
}

/**
Expand Down Expand Up @@ -1234,9 +1260,10 @@ public Object add(final Object left, final Object right) {
: left instanceof String && right instanceof String;
if (!strconcat) {
try {
final boolean strictCast = isStrict(JexlOperator.ADD);
// if both (non-null) args fit as long
final Number ln = asLongNumber(left);
final Number rn = asLongNumber(right);
final Number ln = asLongNumber(strictCast, left);
final Number rn = asLongNumber(strictCast, right);
if (ln != null && rn != null) {
final long x = ln.longValue();
final long y = rn.longValue();
Expand All @@ -1247,21 +1274,19 @@ public Object add(final Object left, final Object right) {
}
return narrowLong(left, right, result);
}
final boolean strictCast = isStrict(JexlOperator.ADD);
// if either are bigdecimal use that type
// if either are BigDecimal, use that type
if (left instanceof BigDecimal || right instanceof BigDecimal) {
final BigDecimal l = toBigDecimal(strictCast, left);
final BigDecimal r = toBigDecimal(strictCast, right);
final BigDecimal result = l.add(r, getMathContext());
return narrowBigDecimal(left, right, result);
return l.add(r, getMathContext());
}
// if either are floating point (double or float) use double
// if either are floating point (double or float), use double
if (isFloatingPointNumber(left) || isFloatingPointNumber(right)) {
final double l = toDouble(strictCast, left);
final double r = toDouble(strictCast, right);
return l + r;
}
// otherwise treat as (big) integers
// otherwise treat as BigInteger
final BigInteger l = toBigInteger(strictCast, left);
final BigInteger r = toBigInteger(strictCast, right);
final BigInteger result = l.add(r);
Expand All @@ -1285,9 +1310,10 @@ public Object divide(final Object left, final Object right) {
if (left == null && right == null) {
return controlNullNullOperands(JexlOperator.DIVIDE);
}
final boolean strictCast = isStrict(JexlOperator.DIVIDE);
// if both (non-null) args fit as long
final Number ln = asLongNumber(left);
final Number rn = asLongNumber(right);
final Number ln = asLongNumber(strictCast, left);
final Number rn = asLongNumber(strictCast, right);
if (ln != null && rn != null) {
final long x = ln.longValue();
final long y = rn.longValue();
Expand All @@ -1297,18 +1323,16 @@ public Object divide(final Object left, final Object right) {
final long result = x / y;
return narrowLong(left, right, result);
}
final boolean strictCast = isStrict(JexlOperator.DIVIDE);
// if either are bigdecimal use that type
// if either are BigDecimal, use that type
if (left instanceof BigDecimal || right instanceof BigDecimal) {
final BigDecimal l = toBigDecimal(strictCast, left);
final BigDecimal r = toBigDecimal(strictCast, right);
if (BigDecimal.ZERO.equals(r)) {
throw new ArithmeticException("/");
}
final BigDecimal result = l.divide(r, getMathContext());
return narrowBigDecimal(left, right, result);
return l.divide(r, getMathContext());
}
// if either are floating point (double or float) use double
// if either are floating point (double or float), use double
if (isFloatingPointNumber(left) || isFloatingPointNumber(right)) {
final double l = toDouble(strictCast, left);
final double r = toDouble(strictCast, right);
Expand All @@ -1317,7 +1341,7 @@ public Object divide(final Object left, final Object right) {
}
return l / r;
}
// otherwise treat as integers
// otherwise treat as BigInteger
final BigInteger l = toBigInteger(strictCast, left);
final BigInteger r = toBigInteger(strictCast, right);
if (BigInteger.ZERO.equals(r)) {
Expand All @@ -1339,9 +1363,10 @@ public Object mod(final Object left, final Object right) {
if (left == null && right == null) {
return controlNullNullOperands(JexlOperator.MOD);
}
final boolean strictCast = isStrict(JexlOperator.MOD);
// if both (non-null) args fit as long
final Number ln = asLongNumber(left);
final Number rn = asLongNumber(right);
final Number ln = asLongNumber(strictCast, left);
final Number rn = asLongNumber(strictCast, right);
if (ln != null && rn != null) {
final long x = ln.longValue();
final long y = rn.longValue();
Expand All @@ -1351,18 +1376,16 @@ public Object mod(final Object left, final Object right) {
final long result = x % y;
return narrowLong(left, right, result);
}
final boolean strictCast = isStrict(JexlOperator.MOD);
// if either are bigdecimal use that type
// if either are BigDecimal, use that type
if (left instanceof BigDecimal || right instanceof BigDecimal) {
final BigDecimal l = toBigDecimal(strictCast, left);
final BigDecimal r = toBigDecimal(strictCast, right);
if (BigDecimal.ZERO.equals(r)) {
throw new ArithmeticException("%");
}
final BigDecimal remainder = l.remainder(r, getMathContext());
return narrowBigDecimal(left, right, remainder);
return l.remainder(r, getMathContext());
}
// if either are floating point (double or float) use double
// if either are floating point (double or float), use double
if (isFloatingPointNumber(left) || isFloatingPointNumber(right)) {
final double l = toDouble(strictCast, left);
final double r = toDouble(strictCast, right);
Expand All @@ -1371,7 +1394,7 @@ public Object mod(final Object left, final Object right) {
}
return l % r;
}
// otherwise treat as integers
// otherwise treat as BigInteger
final BigInteger l = toBigInteger(strictCast, left);
final BigInteger r = toBigInteger(strictCast, right);
if (BigInteger.ZERO.equals(r)) {
Expand Down Expand Up @@ -1412,9 +1435,10 @@ public Object multiply(final Object left, final Object right) {
if (left == null && right == null) {
return controlNullNullOperands(JexlOperator.MULTIPLY);
}
// if both (non-null) args fit as int
final Number ln = asLongNumber(left);
final Number rn = asLongNumber(right);
final boolean strictCast = isStrict(JexlOperator.MULTIPLY);
// if both (non-null) args fit as long
final Number ln = asLongNumber(strictCast, left);
final Number rn = asLongNumber(strictCast, right);
if (ln != null && rn != null) {
final long x = ln.longValue();
final long y = rn.longValue();
Expand All @@ -1425,21 +1449,19 @@ public Object multiply(final Object left, final Object right) {
}
return narrowLong(left, right, result);
}
final boolean strictCast = isStrict(JexlOperator.MULTIPLY);
// if either are bigdecimal use that type
// if either are BigDecimal, use that type
if (left instanceof BigDecimal || right instanceof BigDecimal) {
final BigDecimal l = toBigDecimal(strictCast, left);
final BigDecimal r = toBigDecimal(strictCast, right);
final BigDecimal result = l.multiply(r, getMathContext());
return narrowBigDecimal(left, right, result);
return l.multiply(r, getMathContext());
}
// if either are floating point (double or float) use double
// if either are floating point (double or float), use double
if (isFloatingPointNumber(left) || isFloatingPointNumber(right)) {
final double l = toDouble(strictCast, left);
final double r = toDouble(strictCast, right);
return l * r;
}
// otherwise treat as integers
// otherwise treat as BigInteger
final BigInteger l = toBigInteger(strictCast, left);
final BigInteger r = toBigInteger(strictCast, right);
final BigInteger result = l.multiply(r);
Expand All @@ -1457,9 +1479,10 @@ public Object subtract(final Object left, final Object right) {
if (left == null && right == null) {
return controlNullNullOperands(JexlOperator.SUBTRACT);
}
final boolean strictCast = isStrict(JexlOperator.SUBTRACT);
// if both (non-null) args fit as long
final Number ln = asLongNumber(left);
final Number rn = asLongNumber(right);
final Number ln = asLongNumber(strictCast, left);
final Number rn = asLongNumber(strictCast, right);
if (ln != null && rn != null) {
final long x = ln.longValue();
final long y = rn.longValue();
Expand All @@ -1470,21 +1493,19 @@ public Object subtract(final Object left, final Object right) {
}
return narrowLong(left, right, result);
}
final boolean strictCast = isStrict(JexlOperator.SUBTRACT);
// if either are bigdecimal use that type
// if either are BigDecimal, use that type
if (left instanceof BigDecimal || right instanceof BigDecimal) {
final BigDecimal l = toBigDecimal(strictCast, left);
final BigDecimal r = toBigDecimal(strictCast, right);
final BigDecimal result = l.subtract(r, getMathContext());
return narrowBigDecimal(left, right, result);
return l.subtract(r, getMathContext());
}
// if either are floating point (double or float) use double
// if either are floating point (double or float), use double
if (isFloatingPointNumber(left) || isFloatingPointNumber(right)) {
final double l = toDouble(strictCast, left);
final double r = toDouble(strictCast, right);
return l - r;
}
// otherwise treat as integers
// otherwise treat as BigInteger
final BigInteger l = toBigInteger(strictCast, left);
final BigInteger r = toBigInteger(strictCast, right);
final BigInteger result = l.subtract(r);
Expand Down
51 changes: 37 additions & 14 deletions src/test/java/org/apache/commons/jexl3/ArithmeticTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -917,6 +917,17 @@ public void testCoerceBigDecimal() {
Assert.assertEquals(BigDecimal.valueOf(10), ja.toBigDecimal("10"));
Assert.assertEquals(BigDecimal.valueOf(1.), ja.toBigDecimal(true));
Assert.assertEquals(BigDecimal.valueOf(0.), ja.toBigDecimal(false));
// BigDecimal precision is kept when used as argument
BigDecimal a42 = BigDecimal.valueOf(42);
BigDecimal a49 = BigDecimal.valueOf(49);
JexlScript bde = JEXL.createScript("a * 6 / 7", "a");
Assert.assertEquals(a42, bde.execute(null, a49));
bde = JEXL.createScript("(a - 12) / 12", "a");
MathContext mc = ja.getMathContext();
BigDecimal b56 = BigDecimal.valueOf(56);
BigDecimal b12 = BigDecimal.valueOf(12);
BigDecimal b3dot666 = b56.subtract(b12, mc).divide(b12, mc);
Assert.assertEquals(b3dot666, bde.execute(null, b56));
}

@Test
Expand Down Expand Up @@ -1564,26 +1575,38 @@ public void testNaN() {
}

@Test
public void testNarrowBig() throws Exception {
public void testNarrowBigInteger() throws Exception {
final List<String> ls = Arrays.asList("zero", "one", "two");
asserter.setVariable("list",ls);
asserter.setVariable("aBigDecimal", new BigDecimal("1"));
asserter.setVariable("aBigInteger", new BigDecimal("1"));
asserter.assertExpression("list.get(aBigDecimal)", "one");
asserter.assertExpression("list.get(aBigInteger)", "one");
asserter.assertExpression("a -> list.get(a)", "zero", BigInteger.ZERO);
asserter.assertExpression("a -> list.get(a)", "one", BigInteger.ONE);
asserter.assertExpression("a -> list.get(2H)", "two");
BigInteger b42 = BigInteger.valueOf(42);
asserter.setVariable("bi10", BigInteger.valueOf(10));
asserter.setVariable("bi420", BigInteger.valueOf(420));
asserter.assertExpression("420 / bi10", b42);
asserter.assertExpression("420l / bi10", b42);
asserter.assertExpression("bi420 / 420", BigInteger.ONE);
asserter.assertExpression("bi420 / 420l", BigInteger.ONE);
asserter.assertExpression("bi420 / 420H", BigInteger.ONE);
}

@Test
public void testNarrowBigDecimal() throws Exception {
asserter.setVariable("bi420", BigInteger.valueOf(420));
asserter.setVariable("bi10", BigInteger.valueOf(10));
asserter.setVariable("bd420", new BigDecimal("420"));
asserter.setVariable("bd10", new BigDecimal("10"));
asserter.assertExpression("420 / bi10", 42);
asserter.assertExpression("420l / bi10", 42L);
asserter.assertExpression("bi420 / 420", 1);
asserter.assertExpression("bi420 / 420l", 1L);
asserter.assertExpression("bd420 / 10", new BigDecimal("42"));
final List<String> ls = Arrays.asList("zero", "one", "two");
asserter.setVariable("list", ls);
asserter.assertExpression("a -> list.get(a.intValue())", "zero", BigDecimal.ZERO);
asserter.assertExpression("a -> list.get(a.intValue())", "one", BigDecimal.ONE);
asserter.assertExpression("a -> list.get(2B.intValue())", "two");
BigDecimal bd42 = BigDecimal.valueOf(42);
asserter.setVariable("bd10", BigDecimal.valueOf(10d));
asserter.setVariable("bd420",BigDecimal.valueOf(420d));
asserter.assertExpression("420 / bd10", bd42);
asserter.assertExpression("420l / bd10", bd42);
asserter.assertExpression("420H / bd10", bd42);
asserter.assertExpression("bd420 / 10", bd42);
asserter.assertExpression("bd420 / 10H", bd42);
asserter.assertExpression("bd420 / 10B", bd42);
}

@Test
Expand Down

0 comments on commit 5ab46d9

Please sign in to comment.