WW-5118 Fix OgnlOps equal

[aleksandr-m]

Originally, found on StackOverflow - https://stackoverflow.com/questions/66138007/struts-ognl-long-overflow

[lukaszlenart]

Looks good, @aleksandr-m could you link the issue from SO?
@harawata @quaff @JCgH4164838Gh792C124B5 any objections?

[harawata]

Thank you for the heads-up, @lukaszlenart !

This change may break the following test involving a custom number implementation.

public void testCustomNumberEquality() throws Exception {
    Assert.assertTrue(OgnlOps.equal(new MyNumber(1.234), new MyNumber(1.234)));
}

class MyNumber extends Number {
    private Double d;

    public MyNumber(Double d) {
        super();
        this.d = d;
    }

    @Override
    public int intValue() {
        return d.intValue();
    }

    @Override
    public long longValue() {
        return d.longValue();
    }

    @Override
    public float floatValue() {
        return d.floatValue();
    }

    @Override
    public double doubleValue() {
        return d.doubleValue();
    }
}

[aleksandr-m]

@harawata You need to override equals method as well.

[harawata]

@aleksandr-m ,
If that's required from now on, I'm totally fine with that.
I just couldn't think of any other intent in the original code. :)

[JCgH4164838Gh792C124B5]

Hi. Thanks to @aleksandr-m for advising of the issue and proposing a potential fix PR. 😄

Also, thanks to @lukaszlenart for the heads-up for the discussion.

One concern might be that, according to the repository history, OgnlOps.equal() and OgnlOps.isEqual() have been around since at least 2007 with their logic mostly unchanged, and changes could have unintended side-effects.

The proposed change: https://github.com/jkuhnert/ognl/blob/c496b654c4683829b397b4a3c6d8de5db724c42b/src/main/java/ognl/OgnlOps.java#L811-L814 appears to remove the "fall-back" logic that attempts to compare the two parameters (if Number types) as double: https://github.com/jkuhnert/ognl/blob/703059882a3d6e55ffd36dd9d1722496d6cd853c/src/main/java/ognl/OgnlOps.java#L811-L818

The original logic already called OgnlOps.isEqual() in an initial attempt to compare for equality, with the additional check in the event the result of the first check was false (and the two parameters were both Number types).

I guess the question is why the original "fall-back" logic was present for OgnlOps.equal() in the first place (along with the null and direct-equality checks) ?

If someone knows the history of why OgnlOps.equal() was structured that way originally, maybe that would help.

Removing the "fall-back" check might fix the reported issue, but it might also change the behaviour unexpectedly for some other existing OGNL usage. Maybe the original OgnlOps.equal() structure should be maintained due to that uncertainty, but perhaps modified to use some better "fuzzy numeric type-checking" to resolve the reported issue ?

One possible alternative implementation that passes @aleksandr-m 's new unit tests could be something like:

    public static boolean equal(Object v1, Object v2)
    {
        if (v1 == null) return v2 == null;
        if (v1 == v2 || isEqual(v1, v2)) return true;
        if (v1 instanceof Number && v2 instanceof Number) return equalAsNumbers((Number) v1, (Number) v2);
        return false;
    }

    /**
     * Older logic used by {@link OgnlOps#equal(java.lang.Object, java.lang.Object)} always compared {@link Number} types as {@link Double}.
     * Now it calls this method which applies more precise numeric comparisons for known types, with fallback to {@link Object#equals(java.lang.Object)}
     * for high-precision or custom {@link Number} types.
     * 
     * @param v1 first {@link Number} to compare.
     * @param v2 second {@link Number} to compare.
     * @return true if both {@link Number} parameters have an equal numeric value, false otherwise.
     */
    public static boolean equalAsNumbers(Number v1, Number v2)
    {
        if (v1 == null) return v2 == null;
        if (v2 == null) return false;  // v1 not null at this point.
        final int bestMatchNumericType = getNumericType(v1, v2);
        switch (bestMatchNumericType) {
            case BOOL:
            case BYTE:
            case CHAR:
            case SHORT:
                return v1.shortValue() == v2.shortValue();
            case INT:
                return v1.intValue() == v2.intValue();
            case LONG:
                return v1.longValue() == v2.longValue();
            case FLOAT:
                return v1.floatValue() == v2.floatValue();
            case DOUBLE:
                return v1.doubleValue() == v2.doubleValue();
            case BIGINT:
            case BIGDEC:
            default:
                return v1.equals(v2);  // No direct conversion
        }
    }

There may be logic flaws in the above alternative (it does not address the scenario @harawata identified for instance), but something like it might be less risky than completely removing the old logic.

Maybe a smaller incremental change that keeps some of the "fuzzy numeric logic" would be safer ?

[aleksandr-m]

The original logic already called OgnlOps.isEqual() in an initial attempt to compare for equality,

This check doesn't work if result is false.

but perhaps modified to use some better "fuzzy numeric type-checking" to resolve the reported issue ?

isEqual does something similar, see
https://github.com/jkuhnert/ognl/blob/703059882a3d6e55ffd36dd9d1722496d6cd853c/src/main/java/ognl/OgnlOps.java#L69

And what about comparing different objects? Currently String can be compared with int, for example, and etc. With some caveats of course, but still.

it does not address the scenario @harawata identified for instance

If someone wants to compare objects by properties the equals and hashCode must be overridden, it is a Java way.

[harawata]

If someone wants to compare objects by properties the equals and hashCode must be overridden, it is a Java way.

I think OgnlOps.equal() (or isEqual()) is useful because they can compare objects that are not comparable in Java way.
I would even suggest adding some tests that emphasize that point. e.g.

public void testLongVsString() throws Exception {
    final Long v1 = 100L;
    final String v2 = "100";
    assertTrue(OgnlOps.equal(v1, v2));
}

And the fall-back logic made it possible to compare two different Number subclasses (e.g. NumA and NumB).
It may not work with the new code even if equals() and hashCode() are implemented.
(again, I honestly am not sure if this usage is really important)

Not that I recommend it, a trivial patch like the following might be safer if you're worried about backward compatibility.

diff --git a/src/main/java/ognl/OgnlOps.java b/src/main/java/ognl/OgnlOps.java
index 7e990df..cbadafa 100644
--- a/src/main/java/ognl/OgnlOps.java
+++ b/src/main/java/ognl/OgnlOps.java
@@ -813,7 +813,8 @@ public abstract class OgnlOps implements NumericTypes
        ​if (v1 == null) return v2 == null;
        ​if (v1 == v2 || isEqual(v1, v2)) return true;
        ​if (v1 instanceof Number && v2 instanceof Number)
-            return ((Number) v1).doubleValue() == ((Number) v2).doubleValue();
+            return ((Number) v1).longValue() == ((Number) v2).longValue()
+                && ((Number) v1).doubleValue() == ((Number) v2).doubleValue();
        ​return false;
    ​}
​

[JCgH4164838Gh792C124B5]

Hi. There are fair points raised by both @aleksandr-m and @harawata .

The old "fall-back" logic for Number type comparisons is/was a bit strange. Removing it for 3.2.x is probably not a big deal, I was just a little concerned with not understanding why it was there in the first place (as it might impact someone's logic).

A larger concern would probably be preserving the additional checks in OgnlOps.equal(), since they look like they might be there for performance reasons. In that case it the change could be:

 public static boolean equal(Object v1, Object v2) 
 { 
     if (v1 == null) return v2 == null; 
     if (v1 == v2 || isEqual(v1, v2)) return true; 
     return false; 
 } 

If someone needs to do specialized Number equality comparisons after such a change, they should still have an option to write their own comparison method and call it from OGNL ...

[lukaszlenart]

I built a SNAPSHOT version of OGNL based on this PR and I have ran all the Struts tests based on that - everything looks good. So I assume the change is fine, but I think @JCgH4164838Gh792C124B5 has raised a valid concern that this code was here for performance reason. I would use @harawata or @JCgH4164838Gh792C124B5 latest proposal, @aleksandr-m wdyt?

[aleksandr-m]

Used @JCgH4164838Gh792C124B5 version. Thanks.

[lukaszlenart]

Nice, if no more objections I can prepare a release. @aleksandr-m do you want to port this change to OGNL 3.1.x to be used with Struts 2.5 or maybe it is not needed?

[aleksandr-m]

@lukaszlenart Done.

[lukaszlenart]

3.2.20 released