From d178eafc11f6576188df8bbb569d82e9d5569bc2 Mon Sep 17 00:00:00 2001 From: Tin Pavlinic Date: Wed, 1 Nov 2017 14:32:40 +1100 Subject: [PATCH] Fix SpEL comparison operator for comparable types Prior to this commit, the SpEL relational operators for comparison would have the following problem: * their implementation would claim that incompatible types could be compared and later fail during comparison * not delegate the comparison to the actual `TypeComparator` implementation but rely on operator specifics This commit ensures that the `TypeComparator` implementation is used for both `canCompare` and `compare` calls in the operators. See gh-1581 --- .../expression/spel/ast/Operator.java | 7 +-- .../spel/support/StandardTypeComparator.java | 6 +- .../spel/AlternativeComparatorTests.java | 62 +++++++++++++++++++ .../spel/DefaultComparatorUnitTests.java | 2 +- 4 files changed, 69 insertions(+), 8 deletions(-) create mode 100644 spring-expression/src/test/java/org/springframework/expression/spel/AlternativeComparatorTests.java 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 11150a7c9bf..8ae4b06de2b 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 @@ -309,11 +309,8 @@ public abstract class Operator extends SpelNodeImpl { return true; } - if (left instanceof Comparable && right instanceof Comparable) { - Class ancestor = ClassUtils.determineCommonAncestor(left.getClass(), right.getClass()); - if (ancestor != null && Comparable.class.isAssignableFrom(ancestor)) { - return (context.getTypeComparator().compare(left, right) == 0); - } + if (context.getTypeComparator().canCompare(left, right)) { + return context.getTypeComparator().compare(left, right) == 0; } return false; diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/support/StandardTypeComparator.java b/spring-expression/src/main/java/org/springframework/expression/spel/support/StandardTypeComparator.java index fc15f6b3ec0..a27fbe6ceee 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/support/StandardTypeComparator.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/support/StandardTypeComparator.java @@ -23,6 +23,7 @@ import org.springframework.expression.TypeComparator; import org.springframework.expression.spel.SpelEvaluationException; import org.springframework.expression.spel.SpelMessage; import org.springframework.lang.Nullable; +import org.springframework.util.ClassUtils; import org.springframework.util.NumberUtils; /** @@ -44,8 +45,9 @@ public class StandardTypeComparator implements TypeComparator { if (left instanceof Number && right instanceof Number) { return true; } - if (left instanceof Comparable) { - return true; + if (left instanceof Comparable && right instanceof Comparable) { + Class ancestor = ClassUtils.determineCommonAncestor(left.getClass(), right.getClass()); + return ancestor != null && Comparable.class.isAssignableFrom(ancestor); } return false; } diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/AlternativeComparatorTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/AlternativeComparatorTests.java new file mode 100644 index 00000000000..34f417c7646 --- /dev/null +++ b/spring-expression/src/test/java/org/springframework/expression/spel/AlternativeComparatorTests.java @@ -0,0 +1,62 @@ +/* + * Copyright 2002-2014 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.expression.spel; + +import org.junit.Test; +import org.springframework.lang.Nullable; +import org.springframework.expression.Expression; +import org.springframework.expression.TypeComparator; +import org.springframework.expression.EvaluationException; +import org.springframework.expression.ExpressionParser; +import org.springframework.expression.spel.standard.SpelExpressionParser; +import org.springframework.expression.spel.support.StandardEvaluationContext; + +import static org.junit.Assert.*; + +/** + * Test construction of arrays. + * + * @author Andy Clement + */ +public class AlternativeComparatorTests { + private ExpressionParser parser = new SpelExpressionParser(); + + // A silly comparator declaring everything to be equal + private TypeComparator customComparator = new TypeComparator() { + @Override + public boolean canCompare(@Nullable Object firstObject, @Nullable Object secondObject) { + return true; + } + + @Override + public int compare(@Nullable Object firstObject, @Nullable Object secondObject) throws EvaluationException { + return 0; + } + + }; + + @Test + public void customComparatorWorksWithEquality() { + final StandardEvaluationContext ctx = new StandardEvaluationContext(); + ctx.setTypeComparator(customComparator); + + Expression expr = parser.parseExpression("'1' == 1"); + + assertEquals(true, expr.getValue(ctx, Boolean.class)); + + } +} diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/DefaultComparatorUnitTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/DefaultComparatorUnitTests.java index eeb23b4d415..f27a0ed42d4 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/DefaultComparatorUnitTests.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/DefaultComparatorUnitTests.java @@ -116,7 +116,7 @@ class DefaultComparatorUnitTests { assertThat(comparator.canCompare(2,1)).isTrue(); assertThat(comparator.canCompare("abc","def")).isTrue(); - assertThat(comparator.canCompare("abc",3)).isTrue(); + assertThat(comparator.canCompare("abc",3)).isFalse(); assertThat(comparator.canCompare(String.class,3)).isFalse(); }