Skip to content

Commit

Permalink
Consistently throw ParseException instead of IllegalStateException
Browse files Browse the repository at this point in the history 
Closes gh-31097
  • Loading branch information
@jhoeller
jhoeller committed Aug 23, 2023
1 parent a4fc7d3 commit afb378a
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 44 deletions.
84 changes: 45 additions & 39 deletions .../main/java/org/springframework/expression/spel/standard/InternalSpelExpressionParser.java
View file
Expand Up @@ -77,7 +77,6 @@
import org.springframework.expression.spel.ast.TypeReference;
import org.springframework.expression.spel.ast.VariableReference;
import org.springframework.lang.Nullable;
import org.springframework.util.Assert;
import org.springframework.util.StringUtils;

/**
Expand Down Expand Up @@ -137,12 +136,13 @@ protected SpelExpression doParseExpression(String expressionString, @Nullable Pa
this.tokenStreamPointer = 0;
this.constructedNodes.clear();
SpelNodeImpl ast = eatExpression();
Assert.state(ast != null, "No node");
if (ast == null) {
throw new SpelParseException(this.expressionString, 0, SpelMessage.OOD);
}
Token t = peekToken();
if (t != null) {
throw new SpelParseException(t.startPos, SpelMessage.MORE_INPUT, toString(nextToken()));
throw new SpelParseException(this.expressionString, t.startPos, SpelMessage.MORE_INPUT, toString(nextToken()));
}
Assert.isTrue(this.constructedNodes.isEmpty(), "At least one node expected");
return new SpelExpression(expressionString, ast, this.configuration);
}
catch (InternalParseException ex) {
Expand Down Expand Up @@ -254,20 +254,20 @@ private SpelNodeImpl eatRelationalExpression() {
if (tk == TokenKind.EQ) {
return new OpEQ(t.startPos, t.endPos, expr, rhExpr);
}
Assert.isTrue(tk == TokenKind.NE, "Not-equals token expected");
return new OpNE(t.startPos, t.endPos, expr, rhExpr);
if (tk == TokenKind.NE) {
return new OpNE(t.startPos, t.endPos, expr, rhExpr);
}
}

if (tk == TokenKind.INSTANCEOF) {
return new OperatorInstanceof(t.startPos, t.endPos, expr, rhExpr);
}

if (tk == TokenKind.MATCHES) {
return new OperatorMatches(this.patternCache, t.startPos, t.endPos, expr, rhExpr);
}

Assert.isTrue(tk == TokenKind.BETWEEN, "Between token expected");
return new OperatorBetween(t.startPos, t.endPos, expr, rhExpr);
if (tk == TokenKind.BETWEEN) {
return new OperatorBetween(t.startPos, t.endPos, expr, rhExpr);
}
}
return expr;
}
Expand Down Expand Up @@ -304,8 +304,7 @@ private SpelNodeImpl eatProductExpression() {
else if (t.kind == TokenKind.DIV) {
expr = new OpDivide(t.startPos, t.endPos, expr, rhExpr);
}
else {
Assert.isTrue(t.kind == TokenKind.MOD, "Mod token expected");
else if (t.kind == TokenKind.MOD) {
expr = new OpModulus(t.startPos, t.endPos, expr, rhExpr);
}
}
Expand Down Expand Up @@ -335,26 +334,31 @@ private SpelNodeImpl eatPowerIncDecExpression() {
// unaryExpression: (PLUS^ | MINUS^ | BANG^ | INC^ | DEC^) unaryExpression | primaryExpression ;
@Nullable
private SpelNodeImpl eatUnaryExpression() {
if (peekToken(TokenKind.PLUS, TokenKind.MINUS, TokenKind.NOT)) {
if (peekToken(TokenKind.NOT, TokenKind.PLUS, TokenKind.MINUS)) {
Token t = takeToken();
SpelNodeImpl expr = eatUnaryExpression();
Assert.state(expr != null, "No node");
if (expr == null) {
throw internalException(t.startPos, SpelMessage.OOD);
}
if (t.kind == TokenKind.NOT) {
return new OperatorNot(t.startPos, t.endPos, expr);
}
if (t.kind == TokenKind.PLUS) {
return new OpPlus(t.startPos, t.endPos, expr);
}
Assert.isTrue(t.kind == TokenKind.MINUS, "Minus token expected");
return new OpMinus(t.startPos, t.endPos, expr);
if (t.kind == TokenKind.MINUS) {
return new OpMinus(t.startPos, t.endPos, expr);
}
}
if (peekToken(TokenKind.INC, TokenKind.DEC)) {
Token t = takeToken();
SpelNodeImpl expr = eatUnaryExpression();
if (t.getKind() == TokenKind.INC) {
return new OpInc(t.startPos, t.endPos, false, expr);
}
return new OpDec(t.startPos, t.endPos, false, expr);
if (t.kind == TokenKind.DEC) {
return new OpDec(t.startPos, t.endPos, false, expr);
}
}
return eatPrimaryExpression();
}
Expand Down Expand Up @@ -414,7 +418,6 @@ private SpelNodeImpl eatDottedNode() {
return pop();
}
if (peekToken() == null) {
// unexpectedly ran out of data
throw internalException(t.startPos, SpelMessage.OOD);
}
else {
Expand Down Expand Up @@ -460,8 +463,7 @@ private SpelNodeImpl[] maybeEatMethodArgs() {

private void eatConstructorArgs(List<SpelNodeImpl> accumulatedArguments) {
if (!peekToken(TokenKind.LPAREN)) {
throw new InternalParseException(new SpelParseException(this.expressionString,
positionOf(peekToken()), SpelMessage.MISSING_CONSTRUCTOR_ARGS));
throw internalException(positionOf(peekToken()), SpelMessage.MISSING_CONSTRUCTOR_ARGS);
}
consumeArguments(accumulatedArguments);
eatToken(TokenKind.RPAREN);
Expand All @@ -472,7 +474,9 @@ private void eatConstructorArgs(List<SpelNodeImpl> accumulatedArguments) {
*/
private void consumeArguments(List<SpelNodeImpl> accumulatedArguments) {
Token t = peekToken();
Assert.state(t != null, "Expected token");
if (t == null) {
return;
}
int pos = t.startPos;
Token next;
do {
Expand Down Expand Up @@ -575,8 +579,7 @@ else if (peekToken(TokenKind.LITERAL_STRING)) {
private boolean maybeEatTypeReference() {
if (peekToken(TokenKind.IDENTIFIER)) {
Token typeName = peekToken();
Assert.state(typeName != null, "Expected token");
if (!"T".equals(typeName.stringValue())) {
if (typeName == null || !"T".equals(typeName.stringValue())) {
return false;
}
// It looks like a type reference but is T being used as a map key?
Expand Down Expand Up @@ -605,8 +608,7 @@ private boolean maybeEatTypeReference() {
private boolean maybeEatNullReference() {
if (peekToken(TokenKind.IDENTIFIER)) {
Token nullToken = peekToken();
Assert.state(nullToken != null, "Expected token");
if (!"null".equalsIgnoreCase(nullToken.stringValue())) {
if (nullToken == null || !"null".equalsIgnoreCase(nullToken.stringValue())) {
return false;
}
nextToken();
Expand All @@ -619,12 +621,13 @@ private boolean maybeEatNullReference() {
//projection: PROJECT^ expression RCURLY!;
private boolean maybeEatProjection(boolean nullSafeNavigation) {
Token t = peekToken();
if (!peekToken(TokenKind.PROJECT, true)) {
if (t == null || !peekToken(TokenKind.PROJECT, true)) {
return false;
}
Assert.state(t != null, "No token");
SpelNodeImpl expr = eatExpression();
Assert.state(expr != null, "No node");
if (expr == null) {
throw internalException(t.startPos, SpelMessage.OOD);
}
eatToken(TokenKind.RSQUARE);
this.constructedNodes.push(new Projection(nullSafeNavigation, t.startPos, t.endPos, expr));
return true;
Expand All @@ -634,15 +637,13 @@ private boolean maybeEatProjection(boolean nullSafeNavigation) {
// map = LCURLY (key ':' value (COMMA key ':' value)*) RCURLY
private boolean maybeEatInlineListOrMap() {
Token t = peekToken();
if (!peekToken(TokenKind.LCURLY, true)) {
if (t == null || !peekToken(TokenKind.LCURLY, true)) {
return false;
}
Assert.state(t != null, "No token");
SpelNodeImpl expr = null;
Token closingCurly = peekToken();
if (peekToken(TokenKind.RCURLY, true)) {
if (closingCurly != null && peekToken(TokenKind.RCURLY, true)) {
// empty list '{}'
Assert.state(closingCurly != null, "No token");
expr = new InlineList(t.startPos, closingCurly.endPos);
}
else if (peekToken(TokenKind.COLON, true)) {
Expand Down Expand Up @@ -695,23 +696,23 @@ else if (peekToken(TokenKind.COLON, true)) { // map!

private boolean maybeEatIndexer() {
Token t = peekToken();
if (!peekToken(TokenKind.LSQUARE, true)) {
if (t == null || !peekToken(TokenKind.LSQUARE, true)) {
return false;
}
Assert.state(t != null, "No token");
SpelNodeImpl expr = eatExpression();
Assert.state(expr != null, "No node");
if (expr == null) {
throw internalException(t.startPos, SpelMessage.MISSING_SELECTION_EXPRESSION);
}
eatToken(TokenKind.RSQUARE);
this.constructedNodes.push(new Indexer(t.startPos, t.endPos, expr));
return true;
}

private boolean maybeEatSelection(boolean nullSafeNavigation) {
Token t = peekToken();
if (!peekSelectToken()) {
if (t == null || !peekSelectToken()) {
return false;
}
Assert.state(t != null, "No token");
nextToken();
SpelNodeImpl expr = eatExpression();
if (expr == null) {
Expand Down Expand Up @@ -889,9 +890,14 @@ else if (t.kind == TokenKind.LITERAL_STRING) {
//parenExpr : LPAREN! expression RPAREN!;
private boolean maybeEatParenExpression() {
if (peekToken(TokenKind.LPAREN)) {
nextToken();
Token t = nextToken();
if (t == null) {
return false;
}
SpelNodeImpl expr = eatExpression();
Assert.state(expr != null, "No node");
if (expr == null) {
throw internalException(t.startPos, SpelMessage.OOD);
}
eatToken(TokenKind.RPAREN);
push(expr);
return true;
Expand Down
21 changes: 16 additions & 5 deletions ...xpression/src/test/java/org/springframework/expression/spel/standard/SpelParserTests.java
View file
Expand Up @@ -39,7 +39,9 @@
import static org.springframework.expression.spel.SpelMessage.NON_TERMINATING_QUOTED_STRING;
import static org.springframework.expression.spel.SpelMessage.NOT_AN_INTEGER;
import static org.springframework.expression.spel.SpelMessage.NOT_A_LONG;
import static org.springframework.expression.spel.SpelMessage.OOD;
import static org.springframework.expression.spel.SpelMessage.REAL_CANNOT_BE_LONG;
import static org.springframework.expression.spel.SpelMessage.RIGHT_OPERAND_PROBLEM;
import static org.springframework.expression.spel.SpelMessage.RUN_OUT_OF_ARGUMENTS;
import static org.springframework.expression.spel.SpelMessage.UNEXPECTED_DATA_AFTER_DOT;
import static org.springframework.expression.spel.SpelMessage.UNEXPECTED_ESCAPE_CHAR;
Expand Down Expand Up @@ -76,8 +78,8 @@ void blankExpressionIsRejected() {

private static void assertNullOrEmptyExpressionIsRejected(ThrowingCallable throwingCallable) {
assertThatIllegalArgumentException()
.isThrownBy(throwingCallable)
.withMessage("'expressionString' must not be null or blank");
.isThrownBy(throwingCallable)
.withMessage("'expressionString' must not be null or blank");
}

@Test
Expand Down Expand Up @@ -152,7 +154,13 @@ void parseExceptions() {
assertParseException(() -> parser.parseRaw("new String(3"), RUN_OUT_OF_ARGUMENTS, 10);
assertParseException(() -> parser.parseRaw("new String("), RUN_OUT_OF_ARGUMENTS, 10);
assertParseException(() -> parser.parseRaw("\"abc"), NON_TERMINATING_DOUBLE_QUOTED_STRING, 0);
assertParseException(() -> parser.parseRaw("abc\""), NON_TERMINATING_DOUBLE_QUOTED_STRING, 3);
assertParseException(() -> parser.parseRaw("'abc"), NON_TERMINATING_QUOTED_STRING, 0);
assertParseException(() -> parser.parseRaw("abc'"), NON_TERMINATING_QUOTED_STRING, 3);
assertParseException(() -> parser.parseRaw("("), OOD, 0);
assertParseException(() -> parser.parseRaw(")"), OOD, 0);
assertParseException(() -> parser.parseRaw("+"), OOD, 0);
assertParseException(() -> parser.parseRaw("1+"), RIGHT_OPERAND_PROBLEM, 1);
}

@Test
Expand Down Expand Up @@ -377,7 +385,7 @@ private void checkNumber(String expression, Object value, Class<?> type) {

private void checkNumberError(String expression, SpelMessage expectedMessage) {
assertParseExceptionThrownBy(() -> parser.parseRaw(expression))
.satisfies(ex -> assertThat(ex.getMessageCode()).isEqualTo(expectedMessage));
.satisfies(ex -> assertThat(ex.getMessageCode()).isEqualTo(expectedMessage));
}

private static ThrowableAssertAlternative<SpelParseException> assertParseExceptionThrownBy(ThrowingCallable throwingCallable) {
Expand All @@ -386,15 +394,18 @@ private static ThrowableAssertAlternative<SpelParseException> assertParseExcepti

private static void assertParseException(ThrowingCallable throwingCallable, SpelMessage expectedMessage, int expectedPosition) {
assertParseExceptionThrownBy(throwingCallable)
.satisfies(parseExceptionRequirements(expectedMessage, expectedPosition));
.satisfies(parseExceptionRequirements(expectedMessage, expectedPosition));
}

private static <E extends SpelParseException> Consumer<E> parseExceptionRequirements(
SpelMessage expectedMessage, int expectedPosition) {

return ex -> {
assertThat(ex.getMessageCode()).isEqualTo(expectedMessage);
assertThat(ex.getPosition()).isEqualTo(expectedPosition);
assertThat(ex.getMessage()).contains(ex.getExpressionString());
if (ex.getExpressionString() != null) {
assertThat(ex.getMessage()).contains(ex.getExpressionString());
}
};
}

Expand Down

0 comments on commit afb378a

Please sign in to comment.