SpEL: OpEQ should use equals() [SPR-9194]

[spring-projects-issues]

Oliver Becker opened SPR-9194 and commented

The implementation of the SpEL == operator in org.springframework.expression.spel.ast.OpEQ does some special handling for Number types and Comparables, but finally uses left == right.
I think it should always call left.equals(right) - or for null-safety left == null ? right == null : left.equals(right)
This would be consistent with Scala or Groovy, for example.

As for the number types: a float 1.2 would be equal to a short 1 (see also https://jira.springsource.org/browse/SPR-9164)

---

Affects: 3.1.1

Issue Links:

• ClassCastException when evaluating the eq/ne operator in the EL [SPR-11708] #16330 ClassCastException when evaluating the eq/ne operator in the EL
• SpEL: Compiled OpNE should favor equals() to != [SPR-14863] #19429 SpEL: Compiled OpNE should favor equals() to !=
• Support BigDecimals with SpEL [SPR-9164] #13802 Support BigDecimals with SpEL

2 votes, 6 watchers

[spring-projects-issues]

Oliver Becker commented

I just noted that at least all of the standard Java Number subclasses implement Comparable.
So the special code for Number might be completely dropped.

[spring-projects-issues]

Chris Beams commented

Added Andy Clement as a watcher. @Andy please take a look at this an comment if you have a moment.

[spring-projects-issues]

Chris Beams commented

ping for comments from @Andy

[spring-projects-issues]

Andy Clement commented

The change sounds fine to me. But the pull request in that other issue to add a floatliteral will likely also fix the 'a float 1.2 would be equal to a short 1' situation.

[spring-projects-issues]

Chris Beams commented

Thanks, Andy. Linked to #13802 and slating for the general backlog then.

[spring-projects-issues]

Oliver Becker commented

I recently fell again in the trap described in this issue (with database entity objects instead of numbers).
Since Spring 4.0 aims at a stronger Groovy integration I think it is time to bring this issue back on the table. Thanks.

[spring-projects-issues]

Martin Frey commented

Definately vote up. This is a common pitfall which is not necessary. Jboss EL implements it too, as does thymeleaf.

[spring-projects-issues]

Andy Clement commented

@Oliver: I'm taking a look at this. I just want to understand your proposal - you want to remove everything in the implementation of OpEq and replace it with:

Object left = getLeftOperand().getValueInternal(state).getValue();
Object right = getRightOperand().getValueInternal(state).getValue();
return BooleanTypedValue.forValue(left==null?right==null:left.equals(right));

This removes the ability for a SpEL consumer to plug in their own type comparator - not necessarily a problem. But if it were removed from here it makes sense to also remove it from the other mathematical operators (otherwise we could get into some weirdness where == returns one answer but is contradicted by the <,<=,>=,> answer for the same operands). We are then left with: if the user needs objects to be comparable they should ensure the classes for those objects implement Comparable.

[spring-projects-issues]

Andy Clement commented

Or were you just proposing to change the == line to use .equals?

[spring-projects-issues]

Andy Clement commented

Removing the whole body of OpEq won't do the right thing for numbers. They may implement Comparable but they don't do the widening for us. Without the special handling will mean checks like this fail "2.0d == 2" which currently work. So I propose under this bug simply to change the == to use .equals in the final clause of the comparison method.

[spring-projects-issues]

Martin Frey commented

Why not do the most obvious tests upfront?

    Object left = ...
    Object right = ...

    if (left == right)
      return true;
    else if (left != null && left instanceof Object && left.equals(right))
      return true;
    else
      restofthetests;

After that I would put this code into an utility class and reuse the code from the "not equals" processor and just negate the outcome.

What do you think?

[spring-projects-issues]

Andy Clement commented

My only hesitation with that is that it disturbs the ordering where right now Comparable is consulted ahead of falling back on equals. Someone may have built a system that depends on it being like that. Maybe they implemented Comparable but didn't wire up equals to use compareTo so it does something different... It may be a bug in their app but the important thing is that the behaviour will change when they update to a new spring level and it may not be obvious why.

Of course you could argue a similar point about switching from == to equals...

If you need the expressions to go fast, you want the JIT that I have in a branch which compiles SpEL expressions to bytecode.

[spring-projects-issues]

Andy Clement commented

Pull request submitted to Spring-Framework: #406

I made the change I proposed but also did as Martin suggested and pulled it up into a common utility method in the superclass for use in == and !=

[spring-projects-issues]

Martin Frey commented

Thanks for the work on it.
I still dont see the point on not calling == and .equals upfront. A comparable should still have a valid equals implementation, right? Also from the performance point of view this would be better i would say.

In the end this pull request should solve the pitfall.

Thank you

[spring-projects-issues]

Oliver Becker commented

Looks fine to with one exception: the order of Number class checks. BigDecimals won't be compared correctly. I suppose new BigDecimal("1.2") == new BigDecimal("1.3") would return true (fallback to int).

I wonder if both are numbers then why not simply compare double values (for all number types). Would this break anything?
Do you have a unit test that fails with this simplification?

if (left instanceof Number && right instanceof Number) {
    Number op1 = (Number) left;
    Number op2 = (Number) right;
    return (op1.doubleValue() == op2.doubleValue());
}