[jira] [Commented] (JEXL-366) Fail to evaluate string and number comparison

["Hussachai Puripunpinyo (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/JEXL-366?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17532645#comment-17532645 ] 

Hussachai Puripunpinyo commented on JEXL-366:
---------------------------------------------

[~henrib] Can you shed some light on why you want to avoid using BigDecimal? 
Your change has a flaw when it fallbacks to String comparison. The following method truncate the decimal point.
{code:java}
private long comparableLong(Object arg) throws NumberFormatException {
        if (arg instanceof String) {
            String s = (String) arg;
            return s.isEmpty()? 0 :(long) Double.parseDouble((String) arg);
        } else {
            return toLong(arg);
        }
    } {code}
There is a bug when one operand is a string with decimal and another one is a numerable.
For example "1.01" == 1 This will return true for your change when it should not.

> Fail to evaluate string and number comparison
> ---------------------------------------------
>
>                 Key: JEXL-366
>                 URL: https://issues.apache.org/jira/browse/JEXL-366
>             Project: Commons JEXL
>          Issue Type: Improvement
>    Affects Versions: 3.2.1
>            Reporter: Hussachai Puripunpinyo
>            Assignee: Henri Biestro
>            Priority: Major
>             Fix For: 3.3
>
>
> The comparison logic doesn't cover the case when one operand is a string and another operand is a numerable type (int, short, long,..).
> The expected result for '1.0' == 1 should be true but it fails because the string comparison check is after the numerable type check. JEXL tries to parse '1.0' using toLong function and it fails with this error message `For input string: "1.0"`
> Moving a string comparison up before other number type checks will not cover some corner cases such as
> '1.00' == 1.0 // String comparison will yield false but it obviously doesn't make sense.
> The proposed change is to add the following code to handle the corner case when one operand is string and another operand is numerable. To cover this corner case, we can apply toBigDecimal to both *lhs* and *rhs* since it should cover any arbitrary number in a string form, and it handles other number types well.
> {code:java}
> if (isNumberable(left) || isNumberable(right)) {
>     if (left instanceof String || right instanceof String) {
>         final BigDecimal l = toBigDecimal(left);
>         final BigDecimal r = toBigDecimal(right);
>         return l.compareTo(r);
>     } else {
>         // this code block remains the same
>     }
>     return 0;
> } {code}
> JEXL syntax is very similar to ECMA script except a few small set that are not the same. So, I think following the ECMA spec for this comparison check makes sense.
> The following code is JavaScript and it can be used in the JEXL test to make sure that the behavior of comparison are the same. 
> Note that '1.0' == 1 yields true
> {code:java}
> function assert(condition, source) {
>     if (!condition) {
>         throw `Assertion failed for ${source}`;
>     }
> }
>   // Basic compare
> let exprs = [
>   "1 == 1", true,
>   "1 != 1", false,
>   "1 != 2", true,
>   "1 > 2", false,
>   "1 >= 2", false,
>   "1 < 2", true,
>   "1 <= 2", true,
>   // Int <-> Float Coercion
>   "1.0 == 1", true,
>   "1 == 1.0", true,
>   "1.1 != 1", true,
>   "1.1 < 2", true,
>   // numbers and strings
>   "'1' == 1", true,
>   "'' == 0", true, // empty string is coerced to zero (ECMA compliance)
>   "1.0 >= '1'", true,
>   "1.0 > '1'", false
> ];for (e = 0; e < exprs.length; e += 2) {
>   let stext = exprs[e];
>   let expected = exprs[e + 1];
>   assert(eval(stext) == expected, stext);
>   
> } {code}
>  



--
This message was sent by Atlassian Jira
(v8.20.7#820007)