Allow null operands in compiled SpEL numeric operator expressions

Prior to this when SpEL compiled an expression using the numeric
operators <, >, <= or >= then it would not create code that handled
nulls. Nulls can occur if a boxed numeric operand is used prior
to compilation, then it is nulled out. SpEL now creates null
handling bytecode.

Closes gh-22358

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

@@ -113,7 +113,8 @@ public abstract class Operator extends SpelNodeImpl {
 		SpelNodeImpl right = getRightOperand();
 		String leftDesc = left.exitTypeDescriptor;
 		String rightDesc = right.exitTypeDescriptor;
-
+		Label elseTarget = new Label();
+		Label endOfIf = new Label();
 		boolean unboxLeft = !CodeFlow.isPrimitive(leftDesc);
 		boolean unboxRight = !CodeFlow.isPrimitive(rightDesc);
 		DescriptorComparison dc = DescriptorComparison.checkNumericCompatibility(
@@ -123,20 +124,104 @@ public abstract class Operator extends SpelNodeImpl {
 		cf.enterCompilationScope();
 		left.generateCode(mv, cf);
 		cf.exitCompilationScope();
-		if (unboxLeft) {
+		if (CodeFlow.isPrimitive(leftDesc)) {
-			CodeFlow.insertUnboxInsns(mv, targetType, leftDesc);
+			CodeFlow.insertBoxIfNecessary(mv, leftDesc);
+			unboxLeft = true;
 		}
 
 		cf.enterCompilationScope();
 		right.generateCode(mv, cf);
 		cf.exitCompilationScope();
+		if (CodeFlow.isPrimitive(rightDesc)) {
+			CodeFlow.insertBoxIfNecessary(mv, rightDesc);
+			unboxRight = true;
+		}
+
+		// This code block checks whether the left or right operand is null and handles
+		// those cases before letting the original code (that only handled actual numbers) run
+		Label rightIsNonNull = new Label();
+		mv.visitInsn(DUP); // stack: left/right/right
+		mv.visitJumpInsn(IFNONNULL, rightIsNonNull); // stack: left/right
+		// here: RIGHT==null LEFT==unknown
+		mv.visitInsn(SWAP); // right/left
+		Label leftNotNullRightIsNull = new Label();
+		mv.visitJumpInsn(IFNONNULL, leftNotNullRightIsNull); // stack: right
+		// here: RIGHT==null LEFT==null
+		mv.visitInsn(POP); // stack: <nothing>
+		// load 0 or 1 depending on comparison instruction
+		switch (compInstruction1) {
+		case IFGE: // OpLT
+		case IFLE: // OpGT
+			mv.visitInsn(ICONST_0); // false - null is not < or > null
+			break;
+		case IFGT: // OpLE
+		case IFLT: // OpGE
+			mv.visitInsn(ICONST_1); // true - null is <= or >= null
+			break;
+		default:
+			throw new IllegalStateException("Unsupported: "+compInstruction1);
+		}
+		mv.visitJumpInsn(GOTO, endOfIf);
+		mv.visitLabel(leftNotNullRightIsNull); // stack: right
+		// RIGHT==null LEFT!=null
+		mv.visitInsn(POP); // stack: <nothing>
+		// load 0 or 1 depending on comparison instruction
+		switch (compInstruction1) {
+		case IFGE: // OpLT
+		case IFGT: // OpLE
+			mv.visitInsn(ICONST_0); // false - something is not < or <= null
+			break;
+		case IFLE: // OpGT
+		case IFLT: // OpGE
+			mv.visitInsn(ICONST_1); // true - something is > or >= null
+			break;
+		default:
+			throw new IllegalStateException("Unsupported: "+compInstruction1);
+		}
+		mv.visitJumpInsn(GOTO, endOfIf);
+
+		mv.visitLabel(rightIsNonNull); // stack: left/right
+		// here: RIGHT!=null LEFT==unknown
+		mv.visitInsn(SWAP); // stack: right/left
+		mv.visitInsn(DUP); // stack: right/left/left
+		Label neitherRightNorLeftAreNull = new Label();
+		mv.visitJumpInsn(IFNONNULL, neitherRightNorLeftAreNull); // stack: right/left
+		// here: RIGHT!=null LEFT==null
+		mv.visitInsn(POP2); // stack: <nothing>
+		switch (compInstruction1) {
+		case IFGE: // OpLT
+		case IFGT: // OpLE
+			mv.visitInsn(ICONST_1); // true - null is < or <= something
+			break;
+		case IFLE: // OpGT
+		case IFLT: // OpGE
+			mv.visitInsn(ICONST_0); // false - null is not > or >= something
+			break;
+		default:
+			throw new IllegalStateException("Unsupported: "+compInstruction1);
+		}
+		mv.visitJumpInsn(GOTO, endOfIf);
+		mv.visitLabel(neitherRightNorLeftAreNull); // stack: right/left
+		// neither were null so unbox and proceed with numeric comparison
+		if (unboxLeft) {
+			CodeFlow.insertUnboxInsns(mv, targetType, leftDesc);
+		}
+		// What we just unboxed might be a double slot item (long/double)
+		// so can't just use SWAP
+		// stack: right/left(1or2slots)
+		if (targetType == 'D' || targetType == 'J') {
+			mv.visitInsn(DUP2_X1);
+			mv.visitInsn(POP2);
+		}
+		else {
+			mv.visitInsn(SWAP);
+		}
+		// stack: left(1or2)/right
 		if (unboxRight) {
 			CodeFlow.insertUnboxInsns(mv, targetType, rightDesc);
 		}
 
 		// assert: SpelCompiler.boxingCompatible(leftDesc, rightDesc)
-		Label elseTarget = new Label();
-		Label endOfIf = new Label();
 		if (targetType == 'D') {
 			mv.visitInsn(DCMPG);
 			mv.visitJumpInsn(compInstruction1, elseTarget);

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2018 the original author or authors.
+ * Copyright 2002-2019 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.
@@ -4945,6 +4945,91 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 		assertNull(expression.getValue(rh));
 	}
 
+	@Test
+	public void testNullComparison_SPR22358() {
+		SpelParserConfiguration configuration = new SpelParserConfiguration(SpelCompilerMode.OFF, null);
+		SpelExpressionParser parser = new SpelExpressionParser(configuration);
+		StandardEvaluationContext ctx = new StandardEvaluationContext();
+		ctx.setRootObject(new Reg(1));
+		verifyCompilationAndBehaviourWithNull("value>1", parser, ctx );
+		verifyCompilationAndBehaviourWithNull("value<1", parser, ctx );
+		verifyCompilationAndBehaviourWithNull("value>=1", parser, ctx );
+		verifyCompilationAndBehaviourWithNull("value<=1", parser, ctx );
+
+		verifyCompilationAndBehaviourWithNull2("value>value2", parser, ctx );
+		verifyCompilationAndBehaviourWithNull2("value<value2", parser, ctx );
+		verifyCompilationAndBehaviourWithNull2("value>=value2", parser, ctx );
+		verifyCompilationAndBehaviourWithNull2("value<=value2", parser, ctx );
+
+		verifyCompilationAndBehaviourWithNull("valueD>1.0d", parser, ctx );
+		verifyCompilationAndBehaviourWithNull("valueD<1.0d", parser, ctx );
+		verifyCompilationAndBehaviourWithNull("valueD>=1.0d", parser, ctx );
+		verifyCompilationAndBehaviourWithNull("valueD<=1.0d", parser, ctx );
+
+		verifyCompilationAndBehaviourWithNull2("valueD>valueD2", parser, ctx );
+		verifyCompilationAndBehaviourWithNull2("valueD<valueD2", parser, ctx );
+		verifyCompilationAndBehaviourWithNull2("valueD>=valueD2", parser, ctx );
+		verifyCompilationAndBehaviourWithNull2("valueD<=valueD2", parser, ctx );
+
+		verifyCompilationAndBehaviourWithNull("valueL>1L", parser, ctx );
+		verifyCompilationAndBehaviourWithNull("valueL<1L", parser, ctx );
+		verifyCompilationAndBehaviourWithNull("valueL>=1L", parser, ctx );
+		verifyCompilationAndBehaviourWithNull("valueL<=1L", parser, ctx );
+
+		verifyCompilationAndBehaviourWithNull2("valueL>valueL2", parser, ctx );
+		verifyCompilationAndBehaviourWithNull2("valueL<valueL2", parser, ctx );
+		verifyCompilationAndBehaviourWithNull2("valueL>=valueL2", parser, ctx );
+		verifyCompilationAndBehaviourWithNull2("valueL<=valueL2", parser, ctx );
+
+		verifyCompilationAndBehaviourWithNull("valueF>1.0f", parser, ctx );
+		verifyCompilationAndBehaviourWithNull("valueF<1.0f", parser, ctx );
+		verifyCompilationAndBehaviourWithNull("valueF>=1.0f", parser, ctx );
+		verifyCompilationAndBehaviourWithNull("valueF<=1.0f", parser, ctx );
+
+		verifyCompilationAndBehaviourWithNull("valueF>valueF2", parser, ctx );
+		verifyCompilationAndBehaviourWithNull("valueF<valueF2", parser, ctx );
+		verifyCompilationAndBehaviourWithNull("valueF>=valueF2", parser, ctx );
+		verifyCompilationAndBehaviourWithNull("valueF<=valueF2", parser, ctx );
+	}
+
+	private void verifyCompilationAndBehaviourWithNull(String expressionText, SpelExpressionParser parser, StandardEvaluationContext ctx) {
+		Reg r = (Reg)ctx.getRootObject().getValue();
+		r.setValue2(1); // having a value in value2 fields will enable compilation to succeed, then can switch it to null
+		SpelExpression fast = (SpelExpression) parser.parseExpression(expressionText);
+		SpelExpression slow = (SpelExpression) parser.parseExpression(expressionText);
+		fast.getValue(ctx);
+		assertTrue(fast.compileExpression());
+		r.setValue2(null);
+		// try the numbers 0,1,2,null
+		for (int i=0;i<4;i++) {
+			r.setValue(i<3?i:null);
+			boolean slowResult = (Boolean)slow.getValue(ctx);
+			boolean fastResult = (Boolean)fast.getValue(ctx);
+			// System.out.println("Trying "+expressionText+" with value="+r.getValue()+" result is "+slowResult);
+			assertEquals(" Differing results: expression="+expressionText+
+					" value="+r.getValue()+" slow="+slowResult+" fast="+fastResult,
+					slowResult,fastResult);
+		}
+	}
+
+	private void verifyCompilationAndBehaviourWithNull2(String expressionText, SpelExpressionParser parser, StandardEvaluationContext ctx) {
+		SpelExpression fast = (SpelExpression) parser.parseExpression(expressionText);
+		SpelExpression slow = (SpelExpression) parser.parseExpression(expressionText);
+		fast.getValue(ctx);
+		assertTrue(fast.compileExpression());
+		Reg r = (Reg)ctx.getRootObject().getValue();
+		// try the numbers 0,1,2,null
+		for (int i=0;i<4;i++) {
+			r.setValue(i<3?i:null);
+			boolean slowResult = (Boolean)slow.getValue(ctx);
+			boolean fastResult = (Boolean)fast.getValue(ctx);
+			// System.out.println("Trying "+expressionText+" with value="+r.getValue()+" result is "+slowResult);
+			assertEquals(" Differing results: expression="+expressionText+
+					" value="+r.getValue()+" slow="+slowResult+" fast="+fastResult,
+					slowResult,fastResult);
+		}
+	}
+
 	@Test
 	public void ternaryOperator_SPR15192() {
 		SpelParserConfiguration configuration = new SpelParserConfiguration(SpelCompilerMode.IMMEDIATE, null);
@@ -5113,6 +5198,68 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 
 	// nested types
 
+	public class Reg {
+		private Integer _value,_value2;
+		private Long _valueL,_valueL2;
+		private Double _valueD,_valueD2;
+		private Float _valueF,_valueF2;
+
+
+		public Reg(int v) {
+			this._value  = v;
+			this._valueL = new Long(v);
+			this._valueD = new Double(v);
+			this._valueF = new Float(v);
+		}
+
+		public Integer getValue() {
+			return _value;
+		}
+
+		public Long getValueL() {
+			return _valueL;
+		}
+		
+		public Double getValueD() {
+			return _valueD;
+		}
+		
+		public Float getValueF() {
+			return _valueF;
+		}
+
+		public Integer getValue2() {
+			return _value2;
+		}
+
+		public Long getValueL2() {
+			return _valueL2;
+		}
+		
+		public Double getValueD2() {
+			return _valueD2;
+		}
+		
+		public Float getValueF2() {
+			return _valueF2;
+		}
+
+		public void setValue(Integer value) {
+			_value  = value;
+			_valueL = value==null?null:new Long(value);
+			_valueD = value==null?null:new Double(value);
+			_valueF = value==null?null:new Float(value);
+		}
+
+		public void setValue2(Integer value) {
+			_value2  = value;
+			_valueL2 = value==null?null:new Long(value);
+			_valueD2 = value==null?null:new Double(value);
+			_valueF2 = value==null?null:new Float(value);
+		}
+	}
+
+
 	public interface Message<T> {
 
 		MessageHeaders getHeaders();