Skip to content

Commit 18f6739

Browse files
jhoellerjhoeller
committed
Allow null operands in compiled SpEL numeric operator expressions
Closes gh-22358
1 parent b398216 commit 18f6739

File tree

2 files changed

+241
-9
lines changed

2 files changed

+241
-9
lines changed

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

Lines changed: 91 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2018 the original author or authors.
2+
* Copyright 2002-2019 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.
@@ -113,7 +113,8 @@ protected void generateComparisonCode(MethodVisitor mv, CodeFlow cf, int compIns
113113
SpelNodeImpl right = getRightOperand();
114114
String leftDesc = left.exitTypeDescriptor;
115115
String rightDesc = right.exitTypeDescriptor;
116-
116+
Label elseTarget = new Label();
117+
Label endOfIf = new Label();
117118
boolean unboxLeft = !CodeFlow.isPrimitive(leftDesc);
118119
boolean unboxRight = !CodeFlow.isPrimitive(rightDesc);
119120
DescriptorComparison dc = DescriptorComparison.checkNumericCompatibility(
@@ -123,20 +124,104 @@ protected void generateComparisonCode(MethodVisitor mv, CodeFlow cf, int compIns
123124
cf.enterCompilationScope();
124125
left.generateCode(mv, cf);
125126
cf.exitCompilationScope();
126-
if (unboxLeft) {
127-
CodeFlow.insertUnboxInsns(mv, targetType, leftDesc);
127+
if (CodeFlow.isPrimitive(leftDesc)) {
128+
CodeFlow.insertBoxIfNecessary(mv, leftDesc);
129+
unboxLeft = true;
128130
}
129131

130132
cf.enterCompilationScope();
131133
right.generateCode(mv, cf);
132134
cf.exitCompilationScope();
135+
if (CodeFlow.isPrimitive(rightDesc)) {
136+
CodeFlow.insertBoxIfNecessary(mv, rightDesc);
137+
unboxRight = true;
138+
}
139+
140+
// This code block checks whether the left or right operand is null and handles
141+
// those cases before letting the original code (that only handled actual numbers) run
142+
Label rightIsNonNull = new Label();
143+
mv.visitInsn(DUP); // stack: left/right/right
144+
mv.visitJumpInsn(IFNONNULL, rightIsNonNull); // stack: left/right
145+
// here: RIGHT==null LEFT==unknown
146+
mv.visitInsn(SWAP); // right/left
147+
Label leftNotNullRightIsNull = new Label();
148+
mv.visitJumpInsn(IFNONNULL, leftNotNullRightIsNull); // stack: right
149+
// here: RIGHT==null LEFT==null
150+
mv.visitInsn(POP); // stack: <nothing>
151+
// load 0 or 1 depending on comparison instruction
152+
switch (compInstruction1) {
153+
case IFGE: // OpLT
154+
case IFLE: // OpGT
155+
mv.visitInsn(ICONST_0); // false - null is not < or > null
156+
break;
157+
case IFGT: // OpLE
158+
case IFLT: // OpGE
159+
mv.visitInsn(ICONST_1); // true - null is <= or >= null
160+
break;
161+
default:
162+
throw new IllegalStateException("Unsupported: " + compInstruction1);
163+
}
164+
mv.visitJumpInsn(GOTO, endOfIf);
165+
mv.visitLabel(leftNotNullRightIsNull); // stack: right
166+
// RIGHT==null LEFT!=null
167+
mv.visitInsn(POP); // stack: <nothing>
168+
// load 0 or 1 depending on comparison instruction
169+
switch (compInstruction1) {
170+
case IFGE: // OpLT
171+
case IFGT: // OpLE
172+
mv.visitInsn(ICONST_0); // false - something is not < or <= null
173+
break;
174+
case IFLE: // OpGT
175+
case IFLT: // OpGE
176+
mv.visitInsn(ICONST_1); // true - something is > or >= null
177+
break;
178+
default:
179+
throw new IllegalStateException("Unsupported: " + compInstruction1);
180+
}
181+
mv.visitJumpInsn(GOTO, endOfIf);
182+
183+
mv.visitLabel(rightIsNonNull); // stack: left/right
184+
// here: RIGHT!=null LEFT==unknown
185+
mv.visitInsn(SWAP); // stack: right/left
186+
mv.visitInsn(DUP); // stack: right/left/left
187+
Label neitherRightNorLeftAreNull = new Label();
188+
mv.visitJumpInsn(IFNONNULL, neitherRightNorLeftAreNull); // stack: right/left
189+
// here: RIGHT!=null LEFT==null
190+
mv.visitInsn(POP2); // stack: <nothing>
191+
switch (compInstruction1) {
192+
case IFGE: // OpLT
193+
case IFGT: // OpLE
194+
mv.visitInsn(ICONST_1); // true - null is < or <= something
195+
break;
196+
case IFLE: // OpGT
197+
case IFLT: // OpGE
198+
mv.visitInsn(ICONST_0); // false - null is not > or >= something
199+
break;
200+
default:
201+
throw new IllegalStateException("Unsupported: " + compInstruction1);
202+
}
203+
mv.visitJumpInsn(GOTO, endOfIf);
204+
mv.visitLabel(neitherRightNorLeftAreNull); // stack: right/left
205+
// neither were null so unbox and proceed with numeric comparison
206+
if (unboxLeft) {
207+
CodeFlow.insertUnboxInsns(mv, targetType, leftDesc);
208+
}
209+
// What we just unboxed might be a double slot item (long/double)
210+
// so can't just use SWAP
211+
// stack: right/left(1or2slots)
212+
if (targetType == 'D' || targetType == 'J') {
213+
mv.visitInsn(DUP2_X1);
214+
mv.visitInsn(POP2);
215+
}
216+
else {
217+
mv.visitInsn(SWAP);
218+
}
219+
// stack: left(1or2)/right
133220
if (unboxRight) {
134221
CodeFlow.insertUnboxInsns(mv, targetType, rightDesc);
135222
}
136223

137224
// assert: SpelCompiler.boxingCompatible(leftDesc, rightDesc)
138-
Label elseTarget = new Label();
139-
Label endOfIf = new Label();
140225
if (targetType == 'D') {
141226
mv.visitInsn(DCMPG);
142227
mv.visitJumpInsn(compInstruction1, elseTarget);

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

Lines changed: 150 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2018 the original author or authors.
2+
* Copyright 2002-2019 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.
@@ -4945,6 +4945,91 @@ public void elvisOperator_SPR17214() throws Exception {
49454945
assertNull(expression.getValue(rh));
49464946
}
49474947

4948+
@Test
4949+
public void testNullComparison_SPR22358() {
4950+
SpelParserConfiguration configuration = new SpelParserConfiguration(SpelCompilerMode.OFF, null);
4951+
SpelExpressionParser parser = new SpelExpressionParser(configuration);
4952+
StandardEvaluationContext ctx = new StandardEvaluationContext();
4953+
ctx.setRootObject(new Reg(1));
4954+
verifyCompilationAndBehaviourWithNull("value>1", parser, ctx );
4955+
verifyCompilationAndBehaviourWithNull("value<1", parser, ctx );
4956+
verifyCompilationAndBehaviourWithNull("value>=1", parser, ctx );
4957+
verifyCompilationAndBehaviourWithNull("value<=1", parser, ctx );
4958+
4959+
verifyCompilationAndBehaviourWithNull2("value>value2", parser, ctx );
4960+
verifyCompilationAndBehaviourWithNull2("value<value2", parser, ctx );
4961+
verifyCompilationAndBehaviourWithNull2("value>=value2", parser, ctx );
4962+
verifyCompilationAndBehaviourWithNull2("value<=value2", parser, ctx );
4963+
4964+
verifyCompilationAndBehaviourWithNull("valueD>1.0d", parser, ctx );
4965+
verifyCompilationAndBehaviourWithNull("valueD<1.0d", parser, ctx );
4966+
verifyCompilationAndBehaviourWithNull("valueD>=1.0d", parser, ctx );
4967+
verifyCompilationAndBehaviourWithNull("valueD<=1.0d", parser, ctx );
4968+
4969+
verifyCompilationAndBehaviourWithNull2("valueD>valueD2", parser, ctx );
4970+
verifyCompilationAndBehaviourWithNull2("valueD<valueD2", parser, ctx );
4971+
verifyCompilationAndBehaviourWithNull2("valueD>=valueD2", parser, ctx );
4972+
verifyCompilationAndBehaviourWithNull2("valueD<=valueD2", parser, ctx );
4973+
4974+
verifyCompilationAndBehaviourWithNull("valueL>1L", parser, ctx );
4975+
verifyCompilationAndBehaviourWithNull("valueL<1L", parser, ctx );
4976+
verifyCompilationAndBehaviourWithNull("valueL>=1L", parser, ctx );
4977+
verifyCompilationAndBehaviourWithNull("valueL<=1L", parser, ctx );
4978+
4979+
verifyCompilationAndBehaviourWithNull2("valueL>valueL2", parser, ctx );
4980+
verifyCompilationAndBehaviourWithNull2("valueL<valueL2", parser, ctx );
4981+
verifyCompilationAndBehaviourWithNull2("valueL>=valueL2", parser, ctx );
4982+
verifyCompilationAndBehaviourWithNull2("valueL<=valueL2", parser, ctx );
4983+
4984+
verifyCompilationAndBehaviourWithNull("valueF>1.0f", parser, ctx );
4985+
verifyCompilationAndBehaviourWithNull("valueF<1.0f", parser, ctx );
4986+
verifyCompilationAndBehaviourWithNull("valueF>=1.0f", parser, ctx );
4987+
verifyCompilationAndBehaviourWithNull("valueF<=1.0f", parser, ctx );
4988+
4989+
verifyCompilationAndBehaviourWithNull("valueF>valueF2", parser, ctx );
4990+
verifyCompilationAndBehaviourWithNull("valueF<valueF2", parser, ctx );
4991+
verifyCompilationAndBehaviourWithNull("valueF>=valueF2", parser, ctx );
4992+
verifyCompilationAndBehaviourWithNull("valueF<=valueF2", parser, ctx );
4993+
}
4994+
4995+
private void verifyCompilationAndBehaviourWithNull(String expressionText, SpelExpressionParser parser, StandardEvaluationContext ctx) {
4996+
Reg r = (Reg)ctx.getRootObject().getValue();
4997+
r.setValue2(1); // having a value in value2 fields will enable compilation to succeed, then can switch it to null
4998+
SpelExpression fast = (SpelExpression) parser.parseExpression(expressionText);
4999+
SpelExpression slow = (SpelExpression) parser.parseExpression(expressionText);
5000+
fast.getValue(ctx);
5001+
assertTrue(fast.compileExpression());
5002+
r.setValue2(null);
5003+
// try the numbers 0,1,2,null
5004+
for (int i=0;i<4;i++) {
5005+
r.setValue(i<3?i:null);
5006+
boolean slowResult = (Boolean)slow.getValue(ctx);
5007+
boolean fastResult = (Boolean)fast.getValue(ctx);
5008+
// System.out.println("Trying "+expressionText+" with value="+r.getValue()+" result is "+slowResult);
5009+
assertEquals(" Differing results: expression="+expressionText+
5010+
" value="+r.getValue()+" slow="+slowResult+" fast="+fastResult,
5011+
slowResult,fastResult);
5012+
}
5013+
}
5014+
5015+
private void verifyCompilationAndBehaviourWithNull2(String expressionText, SpelExpressionParser parser, StandardEvaluationContext ctx) {
5016+
SpelExpression fast = (SpelExpression) parser.parseExpression(expressionText);
5017+
SpelExpression slow = (SpelExpression) parser.parseExpression(expressionText);
5018+
fast.getValue(ctx);
5019+
assertTrue(fast.compileExpression());
5020+
Reg r = (Reg)ctx.getRootObject().getValue();
5021+
// try the numbers 0,1,2,null
5022+
for (int i=0;i<4;i++) {
5023+
r.setValue(i<3?i:null);
5024+
boolean slowResult = (Boolean)slow.getValue(ctx);
5025+
boolean fastResult = (Boolean)fast.getValue(ctx);
5026+
// System.out.println("Trying "+expressionText+" with value="+r.getValue()+" result is "+slowResult);
5027+
assertEquals(" Differing results: expression="+expressionText+
5028+
" value="+r.getValue()+" slow="+slowResult+" fast="+fastResult,
5029+
slowResult,fastResult);
5030+
}
5031+
}
5032+
49485033
@Test
49495034
public void ternaryOperator_SPR15192() {
49505035
SpelParserConfiguration configuration = new SpelParserConfiguration(SpelCompilerMode.IMMEDIATE, null);
@@ -5039,7 +5124,7 @@ public void repeatedCompilation() throws Exception {
50395124
}
50405125

50415126

5042-
// helper methods
5127+
// Helper methods
50435128

50445129
private SpelNodeImpl getAst() {
50455130
SpelExpression spelExpression = (SpelExpression) expression;
@@ -5111,7 +5196,7 @@ private void assertIsCompiled(Expression expression) {
51115196
}
51125197

51135198

5114-
// nested types
5199+
// Nested types
51155200

51165201
public interface Message<T> {
51175202

@@ -6129,4 +6214,66 @@ public static class LongHolder {
61296214
public Long someLong = 3L;
61306215
}
61316216

6217+
6218+
public class Reg {
6219+
6220+
private Integer _value,_value2;
6221+
private Long _valueL,_valueL2;
6222+
private Double _valueD,_valueD2;
6223+
private Float _valueF,_valueF2;
6224+
6225+
public Reg(int v) {
6226+
this._value = v;
6227+
this._valueL = new Long(v);
6228+
this._valueD = new Double(v);
6229+
this._valueF = new Float(v);
6230+
}
6231+
6232+
public Integer getValue() {
6233+
return _value;
6234+
}
6235+
6236+
public Long getValueL() {
6237+
return _valueL;
6238+
}
6239+
6240+
public Double getValueD() {
6241+
return _valueD;
6242+
}
6243+
6244+
public Float getValueF() {
6245+
return _valueF;
6246+
}
6247+
6248+
public Integer getValue2() {
6249+
return _value2;
6250+
}
6251+
6252+
public Long getValueL2() {
6253+
return _valueL2;
6254+
}
6255+
6256+
public Double getValueD2() {
6257+
return _valueD2;
6258+
}
6259+
6260+
public Float getValueF2() {
6261+
return _valueF2;
6262+
}
6263+
6264+
public void setValue(Integer value) {
6265+
_value = value;
6266+
_valueL = value==null?null:new Long(value);
6267+
_valueD = value==null?null:new Double(value);
6268+
_valueF = value==null?null:new Float(value);
6269+
}
6270+
6271+
public void setValue2(Integer value) {
6272+
_value2 = value;
6273+
_valueL2 = value==null?null:new Long(value);
6274+
_valueD2 = value==null?null:new Double(value);
6275+
_valueF2 = value==null?null:new Float(value);
6276+
}
6277+
}
6278+
61326279
}

0 commit comments

Comments
 (0)