Skip to content

Commit 7879bdf

Browse files
aclementjhoeller
aclement
authored and
jhoeller
committed
Fix compilation of SpEL elvis/ternary expressions
Without this fix the compiled version of elvis actual behaved differently to the interpreted version if the value being queried was an empty string. This is now fixed. It also now correctly handles the query value being a primitive and addresses the findings of SPR-15192 where some type inferencing logic was trying to be too clever, that code has been deleted. Issue: SPR-15192 (cherry picked from commit d41d28f)
1 parent dfa8a7c commit 7879bdf

File tree

3 files changed

+200
-19
lines changed

3 files changed

+200
-19
lines changed

‎spring-expression/src/main/java/org/springframework/expression/spel/ast/Elvis.java

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2014 the original author or authors.
2+
* Copyright 2002-2017 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -35,7 +35,7 @@
3535
public class Elvis extends SpelNodeImpl {
3636

3737
public Elvis(int pos, SpelNodeImpl... args) {
38-
super(pos,args);
38+
super(pos, args);
3939
}
4040

4141

@@ -49,6 +49,7 @@ public Elvis(int pos, SpelNodeImpl... args) {
4949
@Override
5050
public TypedValue getValueInternal(ExpressionState state) throws EvaluationException {
5151
TypedValue value = this.children[0].getValueInternal(state);
52+
// If this check is changed, the generateCode method will need changing too
5253
if (!StringUtils.isEmpty(value.getValue())) {
5354
return value;
5455
}
@@ -77,11 +78,17 @@ public void generateCode(MethodVisitor mv, CodeFlow cf) {
7778
// exit type descriptor can be null if both components are literal expressions
7879
computeExitTypeDescriptor();
7980
this.children[0].generateCode(mv, cf);
81+
CodeFlow.insertBoxIfNecessary(mv, cf.lastDescriptor().charAt(0));
8082
Label elseTarget = new Label();
8183
Label endOfIf = new Label();
8284
mv.visitInsn(DUP);
8385
mv.visitJumpInsn(IFNULL, elseTarget);
84-
mv.visitJumpInsn(GOTO, endOfIf);
86+
// Also check if empty string, as per the code in the interpreted version
87+
mv.visitInsn(DUP);
88+
mv.visitLdcInsn("");
89+
mv.visitInsn(SWAP);
90+
mv.visitMethodInsn(INVOKEVIRTUAL, "java/lang/String", "equals", "(Ljava/lang/Object;)Z",false);
91+
mv.visitJumpInsn(IFEQ, endOfIf); // if not empty, drop through to elseTarget
8592
mv.visitLabel(elseTarget);
8693
mv.visitInsn(POP);
8794
this.children[1].generateCode(mv, cf);
@@ -100,12 +107,6 @@ private void computeExitTypeDescriptor() {
100107
if (conditionDescriptor.equals(ifNullValueDescriptor)) {
101108
this.exitTypeDescriptor = conditionDescriptor;
102109
}
103-
else if (conditionDescriptor.equals("Ljava/lang/Object") && !CodeFlow.isPrimitive(ifNullValueDescriptor)) {
104-
this.exitTypeDescriptor = ifNullValueDescriptor;
105-
}
106-
else if (ifNullValueDescriptor.equals("Ljava/lang/Object") && !CodeFlow.isPrimitive(conditionDescriptor)) {
107-
this.exitTypeDescriptor = conditionDescriptor;
108-
}
109110
else {
110111
// Use the easiest to compute common super type
111112
this.exitTypeDescriptor = "Ljava/lang/Object";

‎spring-expression/src/main/java/org/springframework/expression/spel/ast/Ternary.java

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2014 the original author or authors.
2+
* Copyright 2002-2017 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -35,7 +35,7 @@
3535
public class Ternary extends SpelNodeImpl {
3636

3737
public Ternary(int pos, SpelNodeImpl... args) {
38-
super(pos,args);
38+
super(pos, args);
3939
}
4040

4141

@@ -71,12 +71,6 @@ private void computeExitTypeDescriptor() {
7171
if (leftDescriptor.equals(rightDescriptor)) {
7272
this.exitTypeDescriptor = leftDescriptor;
7373
}
74-
else if (leftDescriptor.equals("Ljava/lang/Object") && !CodeFlow.isPrimitive(rightDescriptor)) {
75-
this.exitTypeDescriptor = rightDescriptor;
76-
}
77-
else if (rightDescriptor.equals("Ljava/lang/Object") && !CodeFlow.isPrimitive(leftDescriptor)) {
78-
this.exitTypeDescriptor = leftDescriptor;
79-
}
8074
else {
8175
// Use the easiest to compute common super type
8276
this.exitTypeDescriptor = "Ljava/lang/Object";

‎spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java

Lines changed: 188 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2016 the original author or authors.
2+
* Copyright 2002-2017 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -17,8 +17,10 @@
1717
package org.springframework.expression.spel;
1818

1919
import java.io.IOException;
20+
import java.lang.reflect.Field;
2021
import java.lang.reflect.Method;
2122
import java.util.ArrayList;
23+
import java.util.Collections;
2224
import java.util.HashMap;
2325
import java.util.List;
2426
import java.util.Map;
@@ -4626,6 +4628,164 @@ public void indexerMapAccessor_12045() throws Exception {
46264628
assertEquals(3, expression.getValue(root));
46274629
assertEquals(3, expression.getValue(root));
46284630
}
4631+
4632+
@Test
4633+
public void elvisOperator_SPR15192() {
4634+
SpelParserConfiguration configuration = new SpelParserConfiguration(SpelCompilerMode.IMMEDIATE, null);
4635+
Expression exp;
4636+
4637+
exp = new SpelExpressionParser(configuration).parseExpression("bar()");
4638+
assertEquals("BAR", exp.getValue(new Foo(), String.class));
4639+
assertCanCompile(exp);
4640+
assertEquals("BAR", exp.getValue(new Foo(), String.class));
4641+
assertIsCompiled(exp);
4642+
4643+
exp = new SpelExpressionParser(configuration).parseExpression("bar('baz')");
4644+
assertEquals("BAZ", exp.getValue(new Foo(), String.class));
4645+
assertCanCompile(exp);
4646+
assertEquals("BAZ", exp.getValue(new Foo(), String.class));
4647+
assertIsCompiled(exp);
4648+
4649+
StandardEvaluationContext context = new StandardEvaluationContext();
4650+
context.setVariable("map", Collections.singletonMap("foo", "qux"));
4651+
4652+
exp = new SpelExpressionParser(configuration).parseExpression("bar(#map['foo'])");
4653+
assertEquals("QUX", exp.getValue(context, new Foo(), String.class));
4654+
assertCanCompile(exp);
4655+
assertEquals("QUX", exp.getValue(context, new Foo(), String.class));
4656+
assertIsCompiled(exp);
4657+
4658+
exp = new SpelExpressionParser(configuration).parseExpression("bar(#map['foo'] ?: 'qux')");
4659+
assertEquals("QUX", exp.getValue(context, new Foo(), String.class));
4660+
assertCanCompile(exp);
4661+
assertEquals("QUX", exp.getValue(context, new Foo(), String.class));
4662+
assertIsCompiled(exp);
4663+
4664+
// When the condition is a primitive
4665+
exp = new SpelExpressionParser(configuration).parseExpression("3?:'foo'");
4666+
assertEquals("3", exp.getValue(context, new Foo(), String.class));
4667+
assertCanCompile(exp);
4668+
assertEquals("3", exp.getValue(context, new Foo(), String.class));
4669+
assertIsCompiled(exp);
4670+
4671+
// When the condition is a double slot primitive
4672+
exp = new SpelExpressionParser(configuration).parseExpression("3L?:'foo'");
4673+
assertEquals("3", exp.getValue(context, new Foo(), String.class));
4674+
assertCanCompile(exp);
4675+
assertEquals("3", exp.getValue(context, new Foo(), String.class));
4676+
assertIsCompiled(exp);
4677+
4678+
// When the condition is an empty string
4679+
exp = new SpelExpressionParser(configuration).parseExpression("''?:4L");
4680+
assertEquals("4", exp.getValue(context, new Foo(), String.class));
4681+
assertCanCompile(exp);
4682+
assertEquals("4", exp.getValue(context, new Foo(), String.class));
4683+
assertIsCompiled(exp);
4684+
4685+
// null condition
4686+
exp = new SpelExpressionParser(configuration).parseExpression("null?:4L");
4687+
assertEquals("4", exp.getValue(context, new Foo(), String.class));
4688+
assertCanCompile(exp);
4689+
assertEquals("4", exp.getValue(context, new Foo(), String.class));
4690+
assertIsCompiled(exp);
4691+
4692+
// variable access returning primitive
4693+
exp = new SpelExpressionParser(configuration).parseExpression("#x?:'foo'");
4694+
context.setVariable("x",50);
4695+
assertEquals("50", exp.getValue(context, new Foo(), String.class));
4696+
assertCanCompile(exp);
4697+
assertEquals("50", exp.getValue(context, new Foo(), String.class));
4698+
assertIsCompiled(exp);
4699+
4700+
exp = new SpelExpressionParser(configuration).parseExpression("#x?:'foo'");
4701+
context.setVariable("x",null);
4702+
assertEquals("foo", exp.getValue(context, new Foo(), String.class));
4703+
assertCanCompile(exp);
4704+
assertEquals("foo", exp.getValue(context, new Foo(), String.class));
4705+
assertIsCompiled(exp);
4706+
4707+
// variable access returning array
4708+
exp = new SpelExpressionParser(configuration).parseExpression("#x?:'foo'");
4709+
context.setVariable("x",new int[]{1,2,3});
4710+
assertEquals("1,2,3", exp.getValue(context, new Foo(), String.class));
4711+
assertCanCompile(exp);
4712+
assertEquals("1,2,3", exp.getValue(context, new Foo(), String.class));
4713+
assertIsCompiled(exp);
4714+
}
4715+
4716+
@Test
4717+
public void ternaryOperator_SPR15192() {
4718+
SpelParserConfiguration configuration = new SpelParserConfiguration(SpelCompilerMode.IMMEDIATE, null);
4719+
Expression exp;
4720+
StandardEvaluationContext context = new StandardEvaluationContext();
4721+
context.setVariable("map", Collections.singletonMap("foo", "qux"));
4722+
4723+
exp = new SpelExpressionParser(configuration).parseExpression("bar(#map['foo'] != null ? #map['foo'] : 'qux')");
4724+
assertEquals("QUX", exp.getValue(context, new Foo(), String.class));
4725+
assertCanCompile(exp);
4726+
assertEquals("QUX", exp.getValue(context, new Foo(), String.class));
4727+
assertIsCompiled(exp);
4728+
4729+
exp = new SpelExpressionParser(configuration).parseExpression("3==3?3:'foo'");
4730+
assertEquals("3", exp.getValue(context, new Foo(), String.class));
4731+
assertCanCompile(exp);
4732+
assertEquals("3", exp.getValue(context, new Foo(), String.class));
4733+
assertIsCompiled(exp);
4734+
exp = new SpelExpressionParser(configuration).parseExpression("3!=3?3:'foo'");
4735+
assertEquals("foo", exp.getValue(context, new Foo(), String.class));
4736+
assertCanCompile(exp);
4737+
assertEquals("foo", exp.getValue(context, new Foo(), String.class));
4738+
assertIsCompiled(exp);
4739+
4740+
// When the condition is a double slot primitive
4741+
exp = new SpelExpressionParser(configuration).parseExpression("3==3?3L:'foo'");
4742+
assertEquals("3", exp.getValue(context, new Foo(), String.class));
4743+
assertCanCompile(exp);
4744+
assertEquals("3", exp.getValue(context, new Foo(), String.class));
4745+
assertIsCompiled(exp);
4746+
exp = new SpelExpressionParser(configuration).parseExpression("3!=3?3L:'foo'");
4747+
assertEquals("foo", exp.getValue(context, new Foo(), String.class));
4748+
assertCanCompile(exp);
4749+
assertEquals("foo", exp.getValue(context, new Foo(), String.class));
4750+
assertIsCompiled(exp);
4751+
4752+
// When the condition is an empty string
4753+
exp = new SpelExpressionParser(configuration).parseExpression("''==''?'abc':4L");
4754+
assertEquals("abc", exp.getValue(context, new Foo(), String.class));
4755+
assertCanCompile(exp);
4756+
assertEquals("abc", exp.getValue(context, new Foo(), String.class));
4757+
assertIsCompiled(exp);
4758+
4759+
// null condition
4760+
exp = new SpelExpressionParser(configuration).parseExpression("3==3?null:4L");
4761+
assertEquals(null, exp.getValue(context, new Foo(), String.class));
4762+
assertCanCompile(exp);
4763+
assertEquals(null, exp.getValue(context, new Foo(), String.class));
4764+
assertIsCompiled(exp);
4765+
4766+
// variable access returning primitive
4767+
exp = new SpelExpressionParser(configuration).parseExpression("#x==#x?50:'foo'");
4768+
context.setVariable("x",50);
4769+
assertEquals("50", exp.getValue(context, new Foo(), String.class));
4770+
assertCanCompile(exp);
4771+
assertEquals("50", exp.getValue(context, new Foo(), String.class));
4772+
assertIsCompiled(exp);
4773+
4774+
exp = new SpelExpressionParser(configuration).parseExpression("#x!=#x?50:'foo'");
4775+
context.setVariable("x",null);
4776+
assertEquals("foo", exp.getValue(context, new Foo(), String.class));
4777+
assertCanCompile(exp);
4778+
assertEquals("foo", exp.getValue(context, new Foo(), String.class));
4779+
assertIsCompiled(exp);
4780+
4781+
// variable access returning array
4782+
exp = new SpelExpressionParser(configuration).parseExpression("#x==#x?'1,2,3':'foo'");
4783+
context.setVariable("x",new int[]{1,2,3});
4784+
assertEquals("1,2,3", exp.getValue(context, new Foo(), String.class));
4785+
assertCanCompile(exp);
4786+
assertEquals("1,2,3", exp.getValue(context, new Foo(), String.class));
4787+
assertIsCompiled(exp);
4788+
}
46294789

46304790

46314791
// helper methods
@@ -4687,6 +4847,20 @@ private void assertGetValueFail(Expression expression) {
46874847
}
46884848
}
46894849

4850+
private void assertIsCompiled(Expression expression) {
4851+
try {
4852+
Field field = SpelExpression.class.getDeclaredField("compiledAst");
4853+
field.setAccessible(true);
4854+
Object object = field.get(expression);
4855+
assertNotNull(object);
4856+
}
4857+
catch (Exception ex) {
4858+
fail(ex.toString());
4859+
}
4860+
}
4861+
4862+
4863+
// nested types
46904864

46914865
public interface Message<T> {
46924866

@@ -4803,7 +4977,7 @@ public void generateCode(String propertyName, MethodVisitor mv,CodeFlow cf) {
48034977
try {
48044978
method = Payload2.class.getDeclaredMethod("getField", String.class);
48054979
}
4806-
catch (Exception e) {
4980+
catch (Exception ex) {
48074981
}
48084982
}
48094983
String descriptor = cf.lastDescriptor();
@@ -5661,4 +5835,16 @@ public Map<String, String> getData() {
56615835
}
56625836
}
56635837

5838+
5839+
public static class Foo {
5840+
5841+
public String bar() {
5842+
return "BAR";
5843+
}
5844+
5845+
public String bar(String arg) {
5846+
return arg.toUpperCase();
5847+
}
5848+
}
5849+
56645850
}

0 commit comments

Comments
 (0)