Compare non-comparable objects by equals only

[peteruhnak]

When comparing non-numeric objects that do not implement Comparable interface, OGNL would throw an exception if they are not equal (so a == b is either true or an exception for these objects)
https://github.com/jkuhnert/ognl/blob/master/src/main/java/ognl/OgnlOps.java#L93

e.g.

    Map<String, Object> map = new HashMap<String, Object>();
    map.put("a", new Object());
    map.put("b", new Object());

    // same object, result is true
    assertTrue((Boolean)Ognl.getValue("a == a", map));
    // different object, throws an exception instead of returning false
    assertFalse((Boolean)Ognl.getValue("a == b", map));

This PR addresses that.

Note that it doesn't make sense to make the check inside OgnlOps::compareWithConversion, because that method is used by OgnlOps::less, in which case it still makes sense to throw up. (because a < b on non-comparable objects doesn't make sense).

[lukaszlenart]

Looks good to me, LGTM 👍 - @harawata @quaff @JCgH4164838Gh792C124B5 @danielfernandez do you have any doubts?

[harawata]

I happen to know that OGNL 2.6.9 worked like that (i.e. returning false instead of throwing IllegalArgumentException).

compareWithConversion() and isEqual() in 2.6.9

    /**
     * Compares two objects for equality, even if it has to convert
     * one of them to the other type.  If both objects are numeric
     * they are converted to the widest type and compared.  If
     * one is non-numeric and one is numeric the non-numeric is
     * converted to double and compared to the double numeric
     * value.  If both are non-numeric and Comparable and the
     * types are compatible (i.e. v1 is of the same or superclass
     * of v2's type) they are compared with Comparable.compareTo().
     * If both values are non-numeric and not Comparable or of
     * incompatible classes this will throw and IllegalArgumentException.
     *
     * @param v1    First value to compare
     * @param v2    second value to compare
     *
     * @return integer describing the comparison between the two objects.
     *          A negative number indicates that v1 < v2.  Positive indicates
     *          that v1 > v2.  Zero indicates v1 == v2.
     *
     * @throws IllegalArgumentException if the objects are both non-numeric
     *              yet of incompatible types or do not implement Comparable.
     */
    public static int compareWithConversion( Object v1, Object v2, boolean equals )
    {
        int     result;

        if (v1 == v2) {
            result = 0;
        } else {
            int     t1 = getNumericType(v1),
                    t2 = getNumericType(v2),
                    type = getNumericType(t1, t2, true);

            switch (type) {
                case BIGINT:
                    result = bigIntValue(v1).compareTo(bigIntValue(v2));
                    break;

                case BIGDEC:
                    result = bigDecValue(v1).compareTo(bigDecValue(v2));
                    break;

                case NONNUMERIC:
                    if ((t1 == NONNUMERIC) && (t2 == NONNUMERIC)) {
                        if ((v1 == null) || (v2 == null)) {
                            result = (v1 == v2) ? 0 : 1;
                        } else {
                            if (v1.getClass().isAssignableFrom(v2.getClass()) || v2.getClass().isAssignableFrom(v1.getClass())) {
                                if (v1 instanceof Comparable) {
                                    result = ((Comparable)v1).compareTo(v2);
                                    break;
                                } else {
                                    if (equals) {
                                        result = v1.equals(v2) ? 0 : 1;
                                        break;
                                    }
                                }
                            }
                            if (equals) {
                                // Equals comparison between non-numerics that are not of a common
                                // superclass return not equal
                                result = 1;
                                break;
                            } else {
                                throw new IllegalArgumentException("invalid comparison: " + v1.getClass().getName() + " and " + v2.getClass().getName());
                            }
                        }
                    }
                    // else fall through
                case FLOAT:
                case DOUBLE:
                    double      dv1 = doubleValue(v1),
                                dv2 = doubleValue(v2);

                    return (dv1 == dv2) ? 0 : ((dv1 < dv2) ? -1 : 1);

                default:
                    long        lv1 = longValue(v1),
                                lv2 = longValue(v2);

                    return (lv1 == lv2) ? 0 : ((lv1 < lv2) ? -1 : 1);
              }
        }
        return result;
    }

    /**
     * Returns true if object1 is equal to object2 in either the
     * sense that they are the same object or, if both are non-null
     * if they are equal in the <CODE>equals()</CODE> sense.
     *
     * @param v1 First object to compare
     * @param v2 Second object to compare
     *
     * @return true if v1 == v2
     */
    public static boolean isEqual(Object object1, Object object2)
    {
        boolean     result = false;

          if (object1 == object2) {
              result = true;
          } else {
              if ((object1 != null) && (object2 != null)) {
                  if (object1.getClass().isArray() && object2.getClass().isArray() && (object2.getClass() == object1.getClass())) {
                      result = (Array.getLength(object1) == Array.getLength(object2));
                    if (result) {
                        for (int i = 0, icount = Array.getLength(object1); result && (i < icount); i++) {
                            result = isEqual(Array.get(object1, i), Array.get(object2, i));
                        }
                    }
                  } else {
                      if ((object1 != null) && (object2 != null)) {
                          // Check for converted equivalence first, then equals() equivalence
                        result = (compareWithConversion(object1, object2, true) == 0) || object1.equals(object2);
                    }
                }
            }
        }
        return result;
    }

I couldn't find why it was changed, though (there must have been a reason, right?).
The change was made before the initial import of this repo, so there is not even a commit log.
@jkuhnert might remember something...?

[jkuhnert]

I thought I imported with svn history in-tact? In either case no unfortunately I don't remember why I did it if it was me. Probably a Tapestry-related bug if it was. I think I always tried to be good about adding a test-case for every change, I'm sure there must be one asserting the illegalargument case that gives a clue behind reasoning if it was one of my changes. I don't know.

[quaff]

LGTM 👍

[harawata]

Regarding the change, it feels a little bit inconsistent that the result depends on the order of variables. For example:

{ "#a = new java.lang.Object(), #a == ''", Boolean.FALSE}, // PASS
{ "#a = new java.lang.Object(), '' == #a", Boolean.FALSE}, // IllegalArgumentException

@jkuhnert ,
Thank you for your swift reply!
It might be a good idea to involve Tapestry folks if we proceed.

[peteruhnak]

@harawata thanks for the catch!

Do you want me to rebase the commits?

[harawata]

Thanks for the update, @peteruhnak !
I just added a minor suggestion by SonarLint and that's all from me. :)

[harawata]

SonarLint says this assignment is redundant.
It can be...

} else if (object1 != null && object2 != null) {

[JCgH4164838Gh792C124B5]

I think line 131 would still need to remain:
} else if (object1 == null || object2 == null) {
otherwise wouldn't the rest of the comparison logic below get skipped ?
Line 132 could still be removed if desired.

[harawata]

Sorry, I wasn't clear enough.
What I meant was ...

        if (object1 == object2) {
            result = true;
        } else if (object1 != null && object2 != null) {
            if (object1.getClass().isArray()) {
                if (object2.getClass().isArray() && (object2.getClass() == object1.getClass())) {
                    result = (Array.getLength(object1) == Array.getLength(object2));

[peteruhnak]

Hmm, I wanted to avoid adding additional nesting and make it more explicit, but I'll make the change.

[harawata]

Maybe I still wasn't clear enough...

--- a/src/main/java/ognl/OgnlOps.java
+++ b/src/main/java/ognl/OgnlOps.java
@@ -128,9 +128,7 @@ public abstract class OgnlOps implements NumericTypes
 
         if (object1 == object2) {
             result = true;
-        } else if (object1 == null || object2 == null) {
-            result = false;
-        } else {
+        } else if (object1 != null && object2 != null) {
             if (object1.getClass().isArray()) {
                 if (object2.getClass().isArray() && (object2.getClass() == object1.getClass())) {
                     result = (Array.getLength(object1) == Array.getLength(object2));

There is no additional nesting. :)

[peteruhnak]

Yeah, I understood. I just confused myself. :) I've pushed the change.

[JCgH4164838Gh792C124B5]

Hi.

The latest change (as suggested by @harawata) seems to be similar to the logic in OGNL 2.6.9. I tried to locate any changelog information for why it changed in OGNL 2.7, but that information does not seem to be around (only an announcement mentioning the release of Tapestry 4.1.2 / OGNL 2.7 in June 2007).

[JCgH4164838Gh792C124B5]

Hi.
Two minor concerns to mention:

1. There could be a project using OGNL 3.2.x that is expecting the IllegalArgumentException under certain conditions (probably not, but you never know).
2. There could be comparison expressions that failed evaluation before which now succeed (with a false return) or return a different result with the new logic.
   Hopefully the unit tests for other projects will detect if the change produces different behaviour.

Even so, the latest changes still LGTM. 👍

[lukaszlenart]

LGTM 👍

[lukaszlenart]

OGNL 3.2.11 is under way to the Central, thanks!