From bc8e4d36802fabe6999c79db88ce1b105ce2e4e1 Mon Sep 17 00:00:00 2001 From: Andy Clement Date: Tue, 21 Oct 2014 08:43:51 -0700 Subject: [PATCH] Fix SpEL varargs handling and usage of other getValue() methods Building on the initial work for SPR-12326, this commit addresses three problems: Firstly the ReflectiveMethodResolver is modified to consider a direct parameter match more important than a varargs match. Also in that same type when there are a number of close matches, the first one is taken rather than the last one. Secondly more testcases and better support have been added for the case of passing a single parameter to a varargs accepting method. Finally it is possible to set the root context object indirectly and not pass it on getValue() calls to the expression objects but not all variants of getValue() were handling that. This is now fixed. Issue: SPR-12326 --- .../spel/standard/SpelExpression.java | 8 +- .../spel/support/ReflectionHelper.java | 81 ++++++- .../support/ReflectiveMethodResolver.java | 17 +- .../expression/spel/EvaluationTests.java | 30 +++ .../spel/SpelCompilationCoverageTests.java | 218 +++++++++++++++++- 5 files changed, 346 insertions(+), 8 deletions(-) diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/standard/SpelExpression.java b/spring-expression/src/main/java/org/springframework/expression/spel/standard/SpelExpression.java index eba325e415..4be3506bc0 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/standard/SpelExpression.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/standard/SpelExpression.java @@ -109,7 +109,8 @@ public class SpelExpression implements Expression { Object result; if (this.compiledAst != null) { try { - return this.compiledAst.getValue(null,null); + TypedValue contextRoot = evaluationContext == null ? null : evaluationContext.getRootObject(); + return this.compiledAst.getValue(contextRoot == null ? null : contextRoot.getValue(), evaluationContext); } catch (Throwable ex) { // If running in mixed mode, revert to interpreted @@ -134,7 +135,7 @@ public class SpelExpression implements Expression { Object result; if (this.compiledAst != null) { try { - return this.compiledAst.getValue(rootObject,null); + return this.compiledAst.getValue(rootObject, evaluationContext); } catch (Throwable ex) { // If running in mixed mode, revert to interpreted @@ -159,7 +160,8 @@ public class SpelExpression implements Expression { public T getValue(Class expectedResultType) throws EvaluationException { if (this.compiledAst != null) { try { - Object result = this.compiledAst.getValue(null,null); + TypedValue contextRoot = evaluationContext == null ? null : evaluationContext.getRootObject(); + Object result = this.compiledAst.getValue(contextRoot == null ? null : contextRoot.getValue(), evaluationContext); if (expectedResultType == null) { return (T)result; } diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectionHelper.java b/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectionHelper.java index 245228e555..f9858f88e1 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectionHelper.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectionHelper.java @@ -253,8 +253,11 @@ public class ReflectionHelper { if (varargsPosition == arguments.length - 1) { TypeDescriptor targetType = new TypeDescriptor(methodParam); Object argument = arguments[varargsPosition]; - arguments[varargsPosition] = converter.convertValue(argument, TypeDescriptor.forObject(argument), targetType); - conversionOccurred |= (argument != arguments[varargsPosition]); + TypeDescriptor sourceType = TypeDescriptor.forObject(argument); + arguments[varargsPosition] = converter.convertValue(argument, sourceType, targetType); + if (!looksLikeSimpleArrayPackaging(sourceType, targetType)) { + conversionOccurred |= (argument != arguments[varargsPosition]); + } } else { TypeDescriptor targetType = new TypeDescriptor(methodParam).getElementTypeDescriptor(); @@ -268,6 +271,80 @@ public class ReflectionHelper { return conversionOccurred; } + /** + * Check if the target type simply represents the array (possibly boxed/unboxed) form of sourceType. + * @param sourceType the type of the original argument + * @param actualType the type of the converted argument + * @return + */ + private static boolean looksLikeSimpleArrayPackaging(TypeDescriptor sourceType, TypeDescriptor targetType) { + TypeDescriptor td = targetType.getElementTypeDescriptor(); + if (td != null) { + if (td.equals(sourceType)) { + return true; + } + else { // check for boxing + if (td.isPrimitive() || sourceType.isPrimitive()) { + Class targetElementClass = td.getType(); + Class sourceElementClass = sourceType.getType(); + if (targetElementClass.isPrimitive()) { + if (targetElementClass == Boolean.TYPE) { + return sourceElementClass == Boolean.class; + } + else if (targetElementClass == Double.TYPE) { + return sourceElementClass == Double.class; + } + else if (targetElementClass == Float.TYPE) { + return sourceElementClass == Float.class; + } + else if (targetElementClass == Integer.TYPE) { + return sourceElementClass == Integer.class; + } + else if (targetElementClass == Long.TYPE) { + return sourceElementClass == Long.class; + } + else if (targetElementClass == Short.TYPE) { + return sourceElementClass == Short.class; + } + else if (targetElementClass == Character.TYPE) { + return sourceElementClass == Character.class; + } + else if (targetElementClass == Byte.TYPE) { + return sourceElementClass == Byte.class; + } + } + else if (sourceElementClass.isPrimitive()) { + if (sourceElementClass == Boolean.TYPE) { + return targetElementClass == Boolean.class; + } + else if (sourceElementClass == Double.TYPE) { + return targetElementClass == Double.class; + } + else if (sourceElementClass == Float.TYPE) { + return targetElementClass == Float.class; + } + else if (sourceElementClass == Integer.TYPE) { + return targetElementClass == Integer.class; + } + else if (sourceElementClass == Long.TYPE) { + return targetElementClass == Long.class; + } + else if (sourceElementClass == Character.TYPE) { + return targetElementClass == Character.class; + } + else if (sourceElementClass == Short.TYPE) { + return targetElementClass == Short.class; + } + else if (sourceElementClass == Byte.TYPE) { + return targetElementClass == Byte.class; + } + } + } + } + } + return false; + } + /** * Convert a supplied set of arguments into the requested types. If the parameterTypes are related to * a varargs method then the final entry in the parameterTypes array is going to be an array itself whose diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectiveMethodResolver.java b/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectiveMethodResolver.java index 39067a0cd9..7987f405b5 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectiveMethodResolver.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectiveMethodResolver.java @@ -123,6 +123,18 @@ public class ReflectiveMethodResolver implements MethodResolver { public int compare(Method m1, Method m2) { int m1pl = m1.getParameterTypes().length; int m2pl = m2.getParameterTypes().length; + // varargs methods go last + if (m1pl == m2pl) { + if (!m1.isVarArgs() && m2.isVarArgs()) { + return -1; + } + else if (m1.isVarArgs() && !m2.isVarArgs()) { + return 1; + } + else { + return 0; + } + } return (new Integer(m1pl)).compareTo(m2pl); } }); @@ -163,7 +175,10 @@ public class ReflectiveMethodResolver implements MethodResolver { } else if (matchInfo.isCloseMatch()) { if (!this.useDistance) { - closeMatch = method; + // Take this as a close match if there isn't one already + if (closeMatch == null) { + closeMatch = method; + } } else { int matchDistance = ReflectionHelper.getTypeDifferenceWeight(paramDescriptors, argumentTypes); diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/EvaluationTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/EvaluationTests.java index c77ccd5682..209224a016 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/EvaluationTests.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/EvaluationTests.java @@ -18,6 +18,7 @@ package org.springframework.expression.spel; import java.lang.reflect.Method; import java.math.BigDecimal; +import java.math.BigInteger; import java.util.ArrayList; import java.util.List; import java.util.Map; @@ -464,6 +465,35 @@ public class EvaluationTests extends AbstractExpressionTests { assertEquals("T(java.lang.String)", expr.toStringAST()); assertEquals(String.class, expr.getValue(Class.class)); } + + @Test + public void operatorVariants() throws Exception { + SpelExpression expr = (SpelExpression)parser.parseExpression("#a < #b"); + EvaluationContext ctx = new StandardEvaluationContext(); + ctx.setVariable("a", (short)3); + ctx.setVariable("b", (short)6); + assertTrue(expr.getValue(ctx, Boolean.class)); + ctx.setVariable("b", (byte)6); + assertTrue(expr.getValue(ctx, Boolean.class)); + ctx.setVariable("a", (byte)9); + ctx.setVariable("b", (byte)6); + assertFalse(expr.getValue(ctx, Boolean.class)); + ctx.setVariable("a", 10L); + ctx.setVariable("b", (short)30); + assertTrue(expr.getValue(ctx, Boolean.class)); + ctx.setVariable("a", (byte)3); + ctx.setVariable("b", (short)30); + assertTrue(expr.getValue(ctx, Boolean.class)); + ctx.setVariable("a", (byte)3); + ctx.setVariable("b", 30L); + assertTrue(expr.getValue(ctx, Boolean.class)); + ctx.setVariable("a", (byte)3); + ctx.setVariable("b", 30f); + assertTrue(expr.getValue(ctx, Boolean.class)); + ctx.setVariable("a", new BigInteger("10")); + ctx.setVariable("b", new BigInteger("20")); + assertTrue(expr.getValue(ctx, Boolean.class)); + } @Test public void testTypeReferencesPrimitive() { diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java index c1a6eac36d..7c8d4ac317 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java @@ -1685,6 +1685,21 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { assertEquals(1.0f,expression.getValue()); } + @Test + public void failsWhenSettingContextForExpression_SPR12326() { + SpelExpressionParser parser = new SpelExpressionParser(new SpelParserConfiguration(SpelCompilerMode.IMMEDIATE, this + .getClass().getClassLoader())); + Person3 person = new Person3("foo", 1); + SpelExpression expression = parser.parseRaw("#it?.age?.equals([0])"); + StandardEvaluationContext context = new StandardEvaluationContext(new Object[] { 1 }); + context.setVariable("it", person); + expression.setEvaluationContext(context); + assertTrue(expression.getValue(Boolean.class)); + assertTrue(expression.getValue(Boolean.class)); + assertCanCompile(expression); + assertTrue(expression.getValue(Boolean.class)); + } + @Test public void constructorReference_SPR12326() { String type = this.getClass().getName(); @@ -1806,6 +1821,23 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { this.age = age; } } + + public class Person3 { + + private int age; + + public Person3(String name, int age) { + this.age = age; + } + + public int getAge() { + return age; + } + + public void setAge(int age) { + this.age = age; + } + } @Test public void constructorReference() throws Exception { @@ -1851,6 +1883,57 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { assertCantCompile(expression); } + @Test + public void methodReferenceReflectiveMethodSelectionWithVarargs() throws Exception { + TestClass10 tc = new TestClass10(); + + // Should call the non varargs version of concat + // (which causes the '::' prefix in test output) + expression = parser.parseExpression("concat('test')"); + assertCantCompile(expression); + expression.getValue(tc); + assertEquals("::test",tc.s); + assertCanCompile(expression); + tc.reset(); + expression.getValue(tc); + assertEquals("::test",tc.s); + tc.reset(); + + // This will call the varargs concat with an empty array + expression = parser.parseExpression("concat()"); + assertCantCompile(expression); + expression.getValue(tc); + assertEquals("",tc.s); + assertCanCompile(expression); + tc.reset(); + expression.getValue(tc); + assertEquals("",tc.s); + tc.reset(); + + // Should call the non varargs version of concat + // (which causes the '::' prefix in test output) + expression = parser.parseExpression("concat2('test')"); + assertCantCompile(expression); + expression.getValue(tc); + assertEquals("::test",tc.s); + assertCanCompile(expression); + tc.reset(); + expression.getValue(tc); + assertEquals("::test",tc.s); + tc.reset(); + + // This will call the varargs concat with an empty array + expression = parser.parseExpression("concat2()"); + assertCantCompile(expression); + expression.getValue(tc); + assertEquals("",tc.s); + assertCanCompile(expression); + tc.reset(); + expression.getValue(tc); + assertEquals("",tc.s); + tc.reset(); + } + @Test public void methodReferenceVarargs() throws Exception { TestClass5 tc = new TestClass5(); @@ -1866,6 +1949,17 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { assertEquals("",tc.s); tc.reset(); + // varargs string + expression = parser.parseExpression("eleven('aaa')"); + assertCantCompile(expression); + expression.getValue(tc); + assertEquals("aaa",tc.s); + assertCanCompile(expression); + tc.reset(); + expression.getValue(tc); + assertEquals("aaa",tc.s); + tc.reset(); + // varargs string expression = parser.parseExpression("eleven(stringArray)"); assertCantCompile(expression); @@ -1898,6 +1992,16 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { expression.getValue(tc); assertEquals(6,tc.i); tc.reset(); + + expression = parser.parseExpression("twelve(1)"); + assertCantCompile(expression); + expression.getValue(tc); + assertEquals(1,tc.i); + assertCanCompile(expression); + tc.reset(); + expression.getValue(tc); + assertEquals(1,tc.i); + tc.reset(); // one string then varargs string expression = parser.parseExpression("thirteen('aaa','bbb','ccc')"); @@ -1954,6 +2058,16 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { assertEquals("truetruefalse",tc.s); tc.reset(); + expression = parser.parseExpression("arrayz(true)"); + assertCantCompile(expression); + expression.getValue(tc); + assertEquals("true",tc.s); + assertCanCompile(expression); + tc.reset(); + expression.getValue(tc); + assertEquals("true",tc.s); + tc.reset(); + // varargs short expression = parser.parseExpression("arrays(s1,s2,s3)"); assertCantCompile(expression); @@ -1965,6 +2079,16 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { assertEquals("123",tc.s); tc.reset(); + expression = parser.parseExpression("arrays(s1)"); + assertCantCompile(expression); + expression.getValue(tc); + assertEquals("1",tc.s); + assertCanCompile(expression); + tc.reset(); + expression.getValue(tc); + assertEquals("1",tc.s); + tc.reset(); + // varargs double expression = parser.parseExpression("arrayd(1.0d,2.0d,3.0d)"); assertCantCompile(expression); @@ -1976,6 +2100,16 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { assertEquals("1.02.03.0",tc.s); tc.reset(); + expression = parser.parseExpression("arrayd(1.0d)"); + assertCantCompile(expression); + expression.getValue(tc); + assertEquals("1.0",tc.s); + assertCanCompile(expression); + tc.reset(); + expression.getValue(tc); + assertEquals("1.0",tc.s); + tc.reset(); + // varargs long expression = parser.parseExpression("arrayj(l1,l2,l3)"); assertCantCompile(expression); @@ -1986,7 +2120,17 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { expression.getValue(tc); assertEquals("123",tc.s); tc.reset(); - + + expression = parser.parseExpression("arrayj(l1)"); + assertCantCompile(expression); + expression.getValue(tc); + assertEquals("1",tc.s); + assertCanCompile(expression); + tc.reset(); + expression.getValue(tc); + assertEquals("1",tc.s); + tc.reset(); + // varargs char expression = parser.parseExpression("arrayc(c1,c2,c3)"); assertCantCompile(expression); @@ -1997,7 +2141,17 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { expression.getValue(tc); assertEquals("abc",tc.s); tc.reset(); - + + expression = parser.parseExpression("arrayc(c1)"); + assertCantCompile(expression); + expression.getValue(tc); + assertEquals("a",tc.s); + assertCanCompile(expression); + tc.reset(); + expression.getValue(tc); + assertEquals("a",tc.s); + tc.reset(); + // varargs byte expression = parser.parseExpression("arrayb(b1,b2,b3)"); assertCantCompile(expression); @@ -2008,6 +2162,16 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { expression.getValue(tc); assertEquals("656667",tc.s); tc.reset(); + + expression = parser.parseExpression("arrayb(b1)"); + assertCantCompile(expression); + expression.getValue(tc); + assertEquals("65",tc.s); + assertCanCompile(expression); + tc.reset(); + expression.getValue(tc); + assertEquals("65",tc.s); + tc.reset(); // varargs float expression = parser.parseExpression("arrayf(f1,f2,f3)"); @@ -2019,6 +2183,16 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { expression.getValue(tc); assertEquals("1.02.03.0",tc.s); tc.reset(); + + expression = parser.parseExpression("arrayf(f1)"); + assertCantCompile(expression); + expression.getValue(tc); + assertEquals("1.0",tc.s); + assertCanCompile(expression); + tc.reset(); + expression.getValue(tc); + assertEquals("1.0",tc.s); + tc.reset(); } @Test @@ -3307,6 +3481,46 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { public boolean getB() { return b; } } + public static class TestClass10 { + public String s = null; + + public void reset() { + s = null; + } + + public void concat(String arg) { + s = "::"+arg; + } + + public void concat(String... vargs) { + if (vargs==null) { + s = ""; + } + else { + s = ""; + for (String varg: vargs) { + s+=varg; + } + } + } + + public void concat2(Object arg) { + s = "::"+arg; + } + + public void concat2(Object... vargs) { + if (vargs==null) { + s = ""; + } + else { + s = ""; + for (Object varg: vargs) { + s+=varg; + } + } + } + } + public static class TestClass5 { public int i = 0; public String s = null;