From 18f6739be48f51dcae40b6b97a8e16b50ba7b707 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Thu, 2 May 2019 17:27:49 +0200 Subject: [PATCH 1/3] 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 ebb046f91ce..80f96b1b7cf 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 1a50ed55330..c08bf4fa99c 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); + } + } + } From 8158b6fd8605a9f19b999ac93fa04ed7dbf329df Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Thu, 2 May 2019 17:28:11 +0200 Subject: [PATCH 2/3] Update postProcessBeforeInstantiation comment on factory methods Closes gh-22867 --- .../config/InstantiationAwareBeanPostProcessor.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/config/InstantiationAwareBeanPostProcessor.java b/spring-beans/src/main/java/org/springframework/beans/factory/config/InstantiationAwareBeanPostProcessor.java index e9ba8b46f85..c4e21122ed8 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/config/InstantiationAwareBeanPostProcessor.java +++ b/spring-beans/src/main/java/org/springframework/beans/factory/config/InstantiationAwareBeanPostProcessor.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. @@ -54,8 +54,9 @@ public interface InstantiationAwareBeanPostProcessor extends BeanPostProcessor { * will be short-circuited. The only further processing applied is the * {@link #postProcessAfterInitialization} callback from the configured * {@link BeanPostProcessor BeanPostProcessors}. - *

This callback will only be applied to bean definitions with a bean class. - * In particular, it will not be applied to beans with a factory method. + *

This callback will be applied to bean definitions with their bean class, + * as well as to factory-method definitions in which case the returned bean type + * will be passed in here. *

Post-processors may implement the extended * {@link SmartInstantiationAwareBeanPostProcessor} interface in order * to predict the type of the bean object that they are going to return here. @@ -66,7 +67,8 @@ public interface InstantiationAwareBeanPostProcessor extends BeanPostProcessor { * or {@code null} to proceed with default instantiation * @throws org.springframework.beans.BeansException in case of errors * @see #postProcessAfterInstantiation - * @see org.springframework.beans.factory.support.AbstractBeanDefinition#hasBeanClass + * @see org.springframework.beans.factory.support.AbstractBeanDefinition#getBeanClass() + * @see org.springframework.beans.factory.support.AbstractBeanDefinition#getFactoryMethodName() */ @Nullable default Object postProcessBeforeInstantiation(Class beanClass, String beanName) throws BeansException { From f359c117d316789fc57b3746c645a0001b027ea1 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Thu, 2 May 2019 17:28:32 +0200 Subject: [PATCH 3/3] Polishing --- .../transaction/annotation/Propagation.java | 6 +++--- .../support/AbstractPlatformTransactionManager.java | 6 ++++-- .../support/TransactionSynchronizationManager.java | 7 ++++--- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/spring-tx/src/main/java/org/springframework/transaction/annotation/Propagation.java b/spring-tx/src/main/java/org/springframework/transaction/annotation/Propagation.java index 95cc22fc9e2..1e235ce8770 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/annotation/Propagation.java +++ b/spring-tx/src/main/java/org/springframework/transaction/annotation/Propagation.java @@ -87,11 +87,11 @@ public enum Propagation { /** * Execute within a nested transaction if a current transaction exists, - * behave like {@code REQUIRED} else. There is no analogous feature in EJB. + * behave like {@code REQUIRED} otherwise. There is no analogous feature in EJB. *

Note: Actual creation of a nested transaction will only work on specific * transaction managers. Out of the box, this only applies to the JDBC - * DataSourceTransactionManager when working on a JDBC 3.0 driver. - * Some JTA providers might support nested transactions as well. + * DataSourceTransactionManager. Some JTA providers might support nested + * transactions as well. * @see org.springframework.jdbc.datasource.DataSourceTransactionManager */ NESTED(TransactionDefinition.PROPAGATION_NESTED); diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/AbstractPlatformTransactionManager.java b/spring-tx/src/main/java/org/springframework/transaction/support/AbstractPlatformTransactionManager.java index ee75c9aea34..809a854aad2 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/AbstractPlatformTransactionManager.java +++ b/spring-tx/src/main/java/org/springframework/transaction/support/AbstractPlatformTransactionManager.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. @@ -553,7 +553,7 @@ public abstract class AbstractPlatformTransactionManager implements PlatformTran if (definition.getTimeout() != TransactionDefinition.TIMEOUT_DEFAULT) { return definition.getTimeout(); } - return this.defaultTimeout; + return getDefaultTimeout(); } @@ -1099,6 +1099,8 @@ public abstract class AbstractPlatformTransactionManager implements PlatformTran * @param definition a TransactionDefinition instance, describing propagation * behavior, isolation level, read-only flag, timeout, and transaction name * @throws TransactionException in case of creation or system errors + * @throws org.springframework.transaction.NestedTransactionNotSupportedException + * if the underlying transaction does not support nesting */ protected abstract void doBegin(Object transaction, TransactionDefinition definition) throws TransactionException; diff --git a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionSynchronizationManager.java b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionSynchronizationManager.java index f2aeba57c6f..19f20f8571e 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/support/TransactionSynchronizationManager.java +++ b/spring-tx/src/main/java/org/springframework/transaction/support/TransactionSynchronizationManager.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2017 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. @@ -292,10 +292,11 @@ public abstract class TransactionSynchronizationManager { throws IllegalStateException { Assert.notNull(synchronization, "TransactionSynchronization must not be null"); - if (!isSynchronizationActive()) { + Set synchs = synchronizations.get(); + if (synchs == null) { throw new IllegalStateException("Transaction synchronization is not active"); } - synchronizations.get().add(synchronization); + synchs.add(synchronization); } /**