Skip to content

Commit 2e7489d

Browse files
jhoellerjhoeller
committed
Allow null operands in compiled SpEL numeric operator expressions
Closes gh-22358
1 parent 8d0b72b commit 2e7489d

File tree

2 files changed

+247
-13
lines changed

2 files changed

+247
-13
lines changed

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

Lines changed: 97 additions & 10 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.
@@ -106,32 +106,119 @@ protected boolean isCompilableOperatorUsingNumerics() {
106106
* two comparison instructions.
107107
*/
108108
protected void generateComparisonCode(MethodVisitor mv, CodeFlow cf, int compInstruction1, int compInstruction2) {
109-
String leftDesc = getLeftOperand().exitTypeDescriptor;
110-
String rightDesc = getRightOperand().exitTypeDescriptor;
111-
109+
SpelNodeImpl left = getLeftOperand();
110+
SpelNodeImpl right = getRightOperand();
111+
String leftDesc = left.exitTypeDescriptor;
112+
String rightDesc = right.exitTypeDescriptor;
113+
Label elseTarget = new Label();
114+
Label endOfIf = new Label();
112115
boolean unboxLeft = !CodeFlow.isPrimitive(leftDesc);
113116
boolean unboxRight = !CodeFlow.isPrimitive(rightDesc);
114117
DescriptorComparison dc = DescriptorComparison.checkNumericCompatibility(
115118
leftDesc, rightDesc, this.leftActualDescriptor, this.rightActualDescriptor);
116119
char targetType = dc.compatibleType; // CodeFlow.toPrimitiveTargetDesc(leftDesc);
117120

118121
cf.enterCompilationScope();
119-
getLeftOperand().generateCode(mv, cf);
122+
left.generateCode(mv, cf);
120123
cf.exitCompilationScope();
121-
if (unboxLeft) {
122-
CodeFlow.insertUnboxInsns(mv, targetType, leftDesc);
124+
if (CodeFlow.isPrimitive(leftDesc)) {
125+
CodeFlow.insertBoxIfNecessary(mv, leftDesc);
126+
unboxLeft = true;
123127
}
124128

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

132221
// assert: SpelCompiler.boxingCompatible(leftDesc, rightDesc)
133-
Label elseTarget = new Label();
134-
Label endOfIf = new Label();
135222
if (targetType == 'D') {
136223
mv.visitInsn(DCMPG);
137224
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.
@@ -4943,6 +4943,91 @@ public void elvisOperator_SPR17214() throws Exception {
49434943
assertNull(expression.getValue(rh));
49444944
}
49454945

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

50205105

5021-
// helper methods
5106+
// Helper methods
50225107

50235108
private SpelNodeImpl getAst() {
50245109
SpelExpression spelExpression = (SpelExpression) expression;
@@ -5090,7 +5175,7 @@ private void assertIsCompiled(Expression expression) {
50905175
}
50915176

50925177

5093-
// nested types
5178+
// Nested types
50945179

50955180
public interface Message<T> {
50965181

@@ -6108,4 +6193,66 @@ public static class LongHolder {
61086193
public Long someLong = 3L;
61096194
}
61106195

6196+
6197+
public class Reg {
6198+
6199+
private Integer _value,_value2;
6200+
private Long _valueL,_valueL2;
6201+
private Double _valueD,_valueD2;
6202+
private Float _valueF,_valueF2;
6203+
6204+
public Reg(int v) {
6205+
this._value = v;
6206+
this._valueL = new Long(v);
6207+
this._valueD = new Double(v);
6208+
this._valueF = new Float(v);
6209+
}
6210+
6211+
public Integer getValue() {
6212+
return _value;
6213+
}
6214+
6215+
public Long getValueL() {
6216+
return _valueL;
6217+
}
6218+
6219+
public Double getValueD() {
6220+
return _valueD;
6221+
}
6222+
6223+
public Float getValueF() {
6224+
return _valueF;
6225+
}
6226+
6227+
public Integer getValue2() {
6228+
return _value2;
6229+
}
6230+
6231+
public Long getValueL2() {
6232+
return _valueL2;
6233+
}
6234+
6235+
public Double getValueD2() {
6236+
return _valueD2;
6237+
}
6238+
6239+
public Float getValueF2() {
6240+
return _valueF2;
6241+
}
6242+
6243+
public void setValue(Integer value) {
6244+
_value = value;
6245+
_valueL = value==null?null:new Long(value);
6246+
_valueD = value==null?null:new Double(value);
6247+
_valueF = value==null?null:new Float(value);
6248+
}
6249+
6250+
public void setValue2(Integer value) {
6251+
_value2 = value;
6252+
_valueL2 = value==null?null:new Long(value);
6253+
_valueD2 = value==null?null:new Double(value);
6254+
_valueF2 = value==null?null:new Float(value);
6255+
}
6256+
}
6257+
61116258
}

0 commit comments

Comments
 (0)