From 18f6739be48f51dcae40b6b97a8e16b50ba7b707 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Thu, 2 May 2019 17:27:49 +0200 Subject: [PATCH] Allow null operands in compiled SpEL numeric operator expressions Closes gh-22358 --- .../expression/spel/ast/Operator.java | 97 ++++++++++- .../spel/SpelCompilationCoverageTests.java | 153 +++++++++++++++++- 2 files changed, 241 insertions(+), 9 deletions(-) diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/Operator.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/Operator.java index ebb046f91c..80f96b1b7c 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/Operator.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/Operator.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. @@ -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) { - CodeFlow.insertUnboxInsns(mv, targetType, leftDesc); + if (CodeFlow.isPrimitive(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: + // 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: + // 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: + 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); 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 1a50ed5533..c08bf4fa99 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 @@ -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 ); + + 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 ); + + 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 ); + + 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 ); + } + + 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); @@ -5039,7 +5124,7 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { } - // helper methods + // Helper methods private SpelNodeImpl getAst() { SpelExpression spelExpression = (SpelExpression) expression; @@ -5111,7 +5196,7 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { } - // nested types + // Nested types public interface Message { @@ -6129,4 +6214,66 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { public Long someLong = 3L; } + + 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); + } + } + }