From 2a05e6afa116ab56378521b5e8c834ba92c25b85 Mon Sep 17 00:00:00 2001 From: Andy Clement Date: Thu, 7 Nov 2013 13:50:40 -0800 Subject: [PATCH] Change SpEL equality operators to use .equals Prior to this commit the SpEL operators `==` and `!=` were using the Java `==` comparison operator as part of their equality checking. It is more flexible to use the equals() method on Object. Under this commit the change to .equals() has been made and the equality checking code has been pushed into a common method in the Operator superclass. This commit also makes some tweaks to the other operator classes - the Float case was missing from OpGT. Issue: SPR-9194 --- .../expression/spel/ast/OpEQ.java | 25 ++------------- .../expression/spel/ast/OpGT.java | 4 +++ .../expression/spel/ast/OpLT.java | 3 +- .../expression/spel/ast/OpNE.java | 29 ++--------------- .../expression/spel/ast/Operator.java | 31 ++++++++++++++++++- .../expression/spel/SpelReproTests.java | 29 +++++++++++++++++ 6 files changed, 69 insertions(+), 52 deletions(-) diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpEQ.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpEQ.java index 1121cc840a..027d4e41cf 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpEQ.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpEQ.java @@ -28,6 +28,7 @@ import org.springframework.expression.spel.support.BooleanTypedValue; */ public class OpEQ extends Operator { + public OpEQ(int pos, SpelNodeImpl... operands) { super("==", pos, operands); } @@ -38,29 +39,7 @@ public class OpEQ extends Operator { throws EvaluationException { Object left = getLeftOperand().getValueInternal(state).getValue(); Object right = getRightOperand().getValueInternal(state).getValue(); - if (left instanceof Number && right instanceof Number) { - Number op1 = (Number) left; - Number op2 = (Number) right; - if (op1 instanceof Double || op2 instanceof Double) { - return BooleanTypedValue.forValue(op1.doubleValue() == op2.doubleValue()); - } - else if (op1 instanceof Float || op2 instanceof Float) { - return BooleanTypedValue.forValue(op1.floatValue() == op2.floatValue()); - } - else if (op1 instanceof Long || op2 instanceof Long) { - return BooleanTypedValue.forValue(op1.longValue() == op2.longValue()); - } - else { - return BooleanTypedValue.forValue(op1.intValue() == op2.intValue()); - } - } - if (left != null && (left instanceof Comparable)) { - return BooleanTypedValue.forValue(state.getTypeComparator().compare(left, - right) == 0); - } - else { - return BooleanTypedValue.forValue(left == right); - } + return BooleanTypedValue.forValue(equalityCheck(state, left, right)); } } diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpGT.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpGT.java index 3d751c976d..769cc8d11a 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpGT.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpGT.java @@ -28,6 +28,7 @@ import org.springframework.expression.spel.support.BooleanTypedValue; */ public class OpGT extends Operator { + public OpGT(int pos, SpelNodeImpl... operands) { super(">", pos, operands); } @@ -43,6 +44,9 @@ public class OpGT extends Operator { if (leftNumber instanceof Double || rightNumber instanceof Double) { return BooleanTypedValue.forValue(leftNumber.doubleValue() > rightNumber.doubleValue()); } + else if (leftNumber instanceof Float || rightNumber instanceof Float) { + return BooleanTypedValue.forValue(leftNumber.floatValue() > rightNumber.floatValue()); + } else if (leftNumber instanceof Long || rightNumber instanceof Long) { return BooleanTypedValue.forValue(leftNumber.longValue() > rightNumber.longValue()); } diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpLT.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpLT.java index dc8916dda8..1027a0c026 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpLT.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpLT.java @@ -28,6 +28,7 @@ import org.springframework.expression.spel.support.BooleanTypedValue; */ public class OpLT extends Operator { + public OpLT(int pos, SpelNodeImpl... operands) { super("<", pos, operands); } @@ -38,7 +39,7 @@ public class OpLT extends Operator { throws EvaluationException { Object left = getLeftOperand().getValueInternal(state).getValue(); Object right = getRightOperand().getValueInternal(state).getValue(); - // TODO could leave all of these to the comparator - just seems quicker to do some here + if (left instanceof Number && right instanceof Number) { Number leftNumber = (Number) left; Number rightNumber = (Number) right; diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpNE.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpNE.java index fdf65c24d7..70b2ce4b8e 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpNE.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpNE.java @@ -28,6 +28,7 @@ import org.springframework.expression.spel.support.BooleanTypedValue; */ public class OpNE extends Operator { + public OpNE(int pos, SpelNodeImpl... operands) { super("!=", pos, operands); } @@ -35,35 +36,9 @@ public class OpNE extends Operator { @Override public BooleanTypedValue getValueInternal(ExpressionState state) throws EvaluationException { - Object left = getLeftOperand().getValueInternal(state).getValue(); Object right = getRightOperand().getValueInternal(state).getValue(); - - if (left instanceof Number && right instanceof Number) { - Number op1 = (Number) left; - Number op2 = (Number) right; - - if (op1 instanceof Double || op2 instanceof Double) { - return BooleanTypedValue.forValue(op1.doubleValue() != op2.doubleValue()); - } - - if (op1 instanceof Float || op2 instanceof Float) { - return BooleanTypedValue.forValue(op1.floatValue() != op2.floatValue()); - } - - if (op1 instanceof Long || op2 instanceof Long) { - return BooleanTypedValue.forValue(op1.longValue() != op2.longValue()); - } - - return BooleanTypedValue.forValue(op1.intValue() != op2.intValue()); - } - - if (left != null && (left instanceof Comparable)) { - return BooleanTypedValue.forValue(state.getTypeComparator().compare(left, - right) != 0); - } - - return BooleanTypedValue.forValue(left != right); + return BooleanTypedValue.forValue(!equalityCheck(state, left, right)); } } diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/Operator.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/Operator.java index 637ba71185..9f5e00364f 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/Operator.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/Operator.java @@ -16,6 +16,8 @@ package org.springframework.expression.spel.ast; +import org.springframework.expression.spel.ExpressionState; + /** * Common supertype for operators that operate on either one or two operands. In the case * of multiply or divide there would be two operands, but for unary plus or minus, there @@ -26,7 +28,7 @@ package org.springframework.expression.spel.ast; */ public abstract class Operator extends SpelNodeImpl { - String operatorName; + private final String operatorName; public Operator(String payload,int pos,SpelNodeImpl... operands) { @@ -63,4 +65,31 @@ public abstract class Operator extends SpelNodeImpl { return sb.toString(); } + protected boolean equalityCheck(ExpressionState state, Object left, Object right) { + if (left instanceof Number && right instanceof Number) { + Number op1 = (Number) left; + Number op2 = (Number) right; + + if (op1 instanceof Double || op2 instanceof Double) { + return (op1.doubleValue() == op2.doubleValue()); + } + + if (op1 instanceof Float || op2 instanceof Float) { + return (op1.floatValue() == op2.floatValue()); + } + + if (op1 instanceof Long || op2 instanceof Long) { + return (op1.longValue() == op2.longValue()); + } + + return (op1.intValue() == op2.intValue()); + } + + if (left != null && (left instanceof Comparable)) { + return (state.getTypeComparator().compare(left, right) == 0); + } + + return (left == null ? right == null : left.equals(right)); + } + } diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/SpelReproTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/SpelReproTests.java index a68dfda68d..cc6a36af06 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/SpelReproTests.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/SpelReproTests.java @@ -1839,6 +1839,19 @@ public class SpelReproTests extends ExpressionTestCase { equalTo((Object) "name")); } + @Test + public void testOperatorEq_SPR9194() { + TestClass2 one = new TestClass2("abc"); + TestClass2 two = new TestClass2("abc"); + Map map = new HashMap(); + map.put("one",one); + map.put("two",two); + + SpelExpressionParser parser = new SpelExpressionParser(); + Expression classNameExpression = parser.parseExpression("['one'] == ['two']"); + assertTrue(classNameExpression.getValue(map,Boolean.class)); + } + private static enum ABC {A, B, C} @@ -1922,4 +1935,20 @@ public class SpelReproTests extends ExpressionTestCase { } } + + static class TestClass2 { // SPR-9194 + String string; + + public TestClass2(String string) { + this.string = string; + } + + public boolean equals(Object o) { + if (o instanceof TestClass2) { + return string.equals(((TestClass2)o).string); + } + return false; + } + + } }