From bff36fb1456ff498354960a725f63f9116ee5b74 Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Mon, 14 Jan 2013 14:07:26 -0800 Subject: [PATCH] Improve exceptions for multi-operand expressions Fix SpEL expression parser and tokenizer to provide better exceptions when dealing with operations that expect two operands. For example, prior to this commit the expression '/foo' would throw a NPE due to missing operands to the left of '/'. Issue: SPR-10146 --- .../expression/spel/SpelMessage.java | 5 +-- .../InternalSpelExpressionParser.java | 26 +++++++++++----- .../expression/spel/standard/Tokenizer.java | 17 +++++++--- .../expression/spel/SpelReproTests.java | 31 +++++++++++++++++-- 4 files changed, 62 insertions(+), 17 deletions(-) diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/SpelMessage.java b/spring-expression/src/main/java/org/springframework/expression/spel/SpelMessage.java index a7f457e147..d9ae1a2efb 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/SpelMessage.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/SpelMessage.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2012 the original author or authors. + * Copyright 2002-2013 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. @@ -108,7 +108,8 @@ public enum SpelMessage { OPERAND_NOT_INCREMENTABLE(Kind.ERROR,1066,"the expression component ''{0}'' does not support increment"), // OPERAND_NOT_DECREMENTABLE(Kind.ERROR,1067,"the expression component ''{0}'' does not support decrement"), // NOT_ASSIGNABLE(Kind.ERROR,1068,"the expression component ''{0}'' is not assignable"), // - ; + MISSING_CHARACTER(Kind.ERROR,1069,"missing expected character ''{0}''"), + LEFT_OPERAND_PROBLEM(Kind.ERROR,1070, "Problem parsing left operand"); private Kind kind; private int code; diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/standard/InternalSpelExpressionParser.java b/spring-expression/src/main/java/org/springframework/expression/spel/standard/InternalSpelExpressionParser.java index be8e988157..c243b5d7f4 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/standard/InternalSpelExpressionParser.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/standard/InternalSpelExpressionParser.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2012 the original author or authors. + * Copyright 2002-2013 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. @@ -37,6 +37,7 @@ import org.springframework.util.StringUtils; * Hand written SpEL parser. Instances are reusable but are not thread safe. * * @author Andy Clement + * @author Phillip Webb * @since 3.0 */ class InternalSpelExpressionParser extends TemplateAwareExpressionParser { @@ -104,8 +105,8 @@ class InternalSpelExpressionParser extends TemplateAwareExpressionParser { Token t = peekToken(); if (t.kind==TokenKind.ASSIGN) { // a=b if (expr==null) { - expr = new NullLiteral(toPos(t.startpos-1,t.endpos-1)); - } + expr = new NullLiteral(toPos(t.startpos-1,t.endpos-1)); + } nextToken(); SpelNodeImpl assignedValue = eatLogicalOrExpression(); return new Assign(toPos(t),expr,assignedValue); @@ -139,7 +140,7 @@ class InternalSpelExpressionParser extends TemplateAwareExpressionParser { while (peekIdentifierToken("or") || peekToken(TokenKind.SYMBOLIC_OR)) { Token t = nextToken(); //consume OR SpelNodeImpl rhExpr = eatLogicalAndExpression(); - checkRightOperand(t,rhExpr); + checkOperands(t,expr,rhExpr); expr = new OpOr(toPos(t),expr,rhExpr); } return expr; @@ -151,7 +152,7 @@ class InternalSpelExpressionParser extends TemplateAwareExpressionParser { while (peekIdentifierToken("and") || peekToken(TokenKind.SYMBOLIC_AND)) { Token t = nextToken();// consume 'AND' SpelNodeImpl rhExpr = eatRelationalExpression(); - checkRightOperand(t,rhExpr); + checkOperands(t,expr,rhExpr); expr = new OpAnd(toPos(t),expr,rhExpr); } return expr; @@ -164,7 +165,7 @@ class InternalSpelExpressionParser extends TemplateAwareExpressionParser { if (relationalOperatorToken != null) { Token t = nextToken(); //consume relational operator token SpelNodeImpl rhExpr = eatSumExpression(); - checkRightOperand(t,rhExpr); + checkOperands(t,expr,rhExpr); TokenKind tk = relationalOperatorToken.kind; if (relationalOperatorToken.isNumericRelationalOperator()) { int pos = toPos(t); @@ -217,7 +218,7 @@ class InternalSpelExpressionParser extends TemplateAwareExpressionParser { while (peekToken(TokenKind.STAR,TokenKind.DIV,TokenKind.MOD)) { Token t = nextToken(); // consume STAR/DIV/MOD SpelNodeImpl rhExpr = eatPowerIncDecExpression(); - checkRightOperand(t,rhExpr); + checkOperands(t,expr,rhExpr); if (t.kind==TokenKind.STAR) { expr = new OpMultiply(toPos(t),expr,rhExpr); } else if (t.kind==TokenKind.DIV) { @@ -836,6 +837,17 @@ class InternalSpelExpressionParser extends TemplateAwareExpressionParser { } } + private void checkOperands(Token token, SpelNodeImpl left, SpelNodeImpl right) { + checkLeftOperand(token, left); + checkRightOperand(token, right); + } + + private void checkLeftOperand(Token token, SpelNodeImpl operandExpression) { + if (operandExpression==null) { + raiseInternalException(token.startpos,SpelMessage.LEFT_OPERAND_PROBLEM); + } + } + private void checkRightOperand(Token token, SpelNodeImpl operandExpression) { if (operandExpression==null) { raiseInternalException(token.startpos,SpelMessage.RIGHT_OPERAND_PROBLEM); diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/standard/Tokenizer.java b/spring-expression/src/main/java/org/springframework/expression/spel/standard/Tokenizer.java index 396c8f17ff..4223a40542 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/standard/Tokenizer.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/standard/Tokenizer.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2012 the original author or authors. + * Copyright 2002-2013 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. @@ -29,6 +29,7 @@ import org.springframework.util.Assert; * Lex some input data into a stream of tokens that can then be parsed. * * @author Andy Clement + * @author Phillip Webb * @since 3.0 */ class Tokenizer { @@ -137,14 +138,20 @@ class Tokenizer { } break; case '&': - if (isTwoCharToken(TokenKind.SYMBOLIC_AND)) { - pushPairToken(TokenKind.SYMBOLIC_AND); + if (!isTwoCharToken(TokenKind.SYMBOLIC_AND)) { + throw new InternalParseException(new SpelParseException( + expressionString, pos, + SpelMessage.MISSING_CHARACTER, "&")); } + pushPairToken(TokenKind.SYMBOLIC_AND); break; case '|': - if (isTwoCharToken(TokenKind.SYMBOLIC_OR)) { - pushPairToken(TokenKind.SYMBOLIC_OR); + if (!isTwoCharToken(TokenKind.SYMBOLIC_OR)) { + throw new InternalParseException(new SpelParseException( + expressionString, pos, + SpelMessage.MISSING_CHARACTER, "|")); } + pushPairToken(TokenKind.SYMBOLIC_OR); break; case '?': if (isTwoCharToken(TokenKind.SELECT)) { diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/SpelReproTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/SpelReproTests.java index 9cb0a58253..01ff05220e 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/SpelReproTests.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/SpelReproTests.java @@ -16,6 +16,11 @@ package org.springframework.expression.spel; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + import java.lang.reflect.Field; import java.lang.reflect.Method; import java.util.ArrayList; @@ -26,8 +31,9 @@ import java.util.Map; import java.util.Properties; import org.junit.Ignore; +import org.junit.Rule; import org.junit.Test; - +import org.junit.rules.ExpectedException; import org.springframework.core.convert.TypeDescriptor; import org.springframework.expression.AccessException; import org.springframework.expression.BeanResolver; @@ -48,8 +54,6 @@ import org.springframework.expression.spel.support.StandardEvaluationContext; import org.springframework.expression.spel.support.StandardTypeLocator; import org.springframework.expression.spel.testresources.le.div.mod.reserved.Reserver; -import static org.junit.Assert.*; - /** * Reproduction tests cornering various SpEL JIRA issues. * @@ -60,6 +64,9 @@ import static org.junit.Assert.*; */ public class SpelReproTests extends ExpressionTestCase { + @Rule + public ExpectedException thrown = ExpectedException.none(); + @Test public void testNPE_SPR5661() { evaluate("joinThreeStrings('a',null,'c')", "anullc", String.class); @@ -1694,6 +1701,24 @@ public class SpelReproTests extends ExpressionTestCase { Object value = parser.parseExpression("primitiveProperty").getValue(evaluationContext); } + @Test + public void SPR_10146_malformedExpressions() throws Exception { + doTestSpr10146("/foo", "EL1070E:(pos 0): Problem parsing left operand"); + doTestSpr10146("*foo", "EL1070E:(pos 0): Problem parsing left operand"); + doTestSpr10146("%foo", "EL1070E:(pos 0): Problem parsing left operand"); + doTestSpr10146("foo", "EL1070E:(pos 0): Problem parsing left operand"); + doTestSpr10146("&&foo", "EL1070E:(pos 0): Problem parsing left operand"); + doTestSpr10146("||foo", "EL1070E:(pos 0): Problem parsing left operand"); + doTestSpr10146("&foo", "EL1069E:(pos 0): missing expected character '&'"); + doTestSpr10146("|foo", "EL1069E:(pos 0): missing expected character '|'"); + } + + private void doTestSpr10146(String expression, String expectedMessage) { + thrown.expect(SpelParseException.class); + thrown.expectMessage(expectedMessage); + new SpelExpressionParser().parseExpression(expression); + } public static class BooleanHolder {