Skip to content

Commit afb378a

Browse files
jhoellerjhoeller
committed
Consistently throw ParseException instead of IllegalStateException
Closes gh-31097
1 parent a4fc7d3 commit afb378a

File tree

2 files changed

+61
-44
lines changed

2 files changed

+61
-44
lines changed

‎spring-expression/src/main/java/org/springframework/expression/spel/standard/InternalSpelExpressionParser.java

Lines changed: 45 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,6 @@
7777
import org.springframework.expression.spel.ast.TypeReference;
7878
import org.springframework.expression.spel.ast.VariableReference;
7979
import org.springframework.lang.Nullable;
80-
import org.springframework.util.Assert;
8180
import org.springframework.util.StringUtils;
8281

8382
/**
@@ -137,12 +136,13 @@ protected SpelExpression doParseExpression(String expressionString, @Nullable Pa
137136
this.tokenStreamPointer = 0;
138137
this.constructedNodes.clear();
139138
SpelNodeImpl ast = eatExpression();
140-
Assert.state(ast != null, "No node");
139+
if (ast == null) {
140+
throw new SpelParseException(this.expressionString, 0, SpelMessage.OOD);
141+
}
141142
Token t = peekToken();
142143
if (t != null) {
143-
throw new SpelParseException(t.startPos, SpelMessage.MORE_INPUT, toString(nextToken()));
144+
throw new SpelParseException(this.expressionString, t.startPos, SpelMessage.MORE_INPUT, toString(nextToken()));
144145
}
145-
Assert.isTrue(this.constructedNodes.isEmpty(), "At least one node expected");
146146
return new SpelExpression(expressionString, ast, this.configuration);
147147
}
148148
catch (InternalParseException ex) {
@@ -254,20 +254,20 @@ private SpelNodeImpl eatRelationalExpression() {
254254
if (tk == TokenKind.EQ) {
255255
return new OpEQ(t.startPos, t.endPos, expr, rhExpr);
256256
}
257-
Assert.isTrue(tk == TokenKind.NE, "Not-equals token expected");
258-
return new OpNE(t.startPos, t.endPos, expr, rhExpr);
257+
if (tk == TokenKind.NE) {
258+
return new OpNE(t.startPos, t.endPos, expr, rhExpr);
259+
}
259260
}
260261

261262
if (tk == TokenKind.INSTANCEOF) {
262263
return new OperatorInstanceof(t.startPos, t.endPos, expr, rhExpr);
263264
}
264-
265265
if (tk == TokenKind.MATCHES) {
266266
return new OperatorMatches(this.patternCache, t.startPos, t.endPos, expr, rhExpr);
267267
}
268-
269-
Assert.isTrue(tk == TokenKind.BETWEEN, "Between token expected");
270-
return new OperatorBetween(t.startPos, t.endPos, expr, rhExpr);
268+
if (tk == TokenKind.BETWEEN) {
269+
return new OperatorBetween(t.startPos, t.endPos, expr, rhExpr);
270+
}
271271
}
272272
return expr;
273273
}
@@ -304,8 +304,7 @@ private SpelNodeImpl eatProductExpression() {
304304
else if (t.kind == TokenKind.DIV) {
305305
expr = new OpDivide(t.startPos, t.endPos, expr, rhExpr);
306306
}
307-
else {
308-
Assert.isTrue(t.kind == TokenKind.MOD, "Mod token expected");
307+
else if (t.kind == TokenKind.MOD) {
309308
expr = new OpModulus(t.startPos, t.endPos, expr, rhExpr);
310309
}
311310
}
@@ -335,26 +334,31 @@ private SpelNodeImpl eatPowerIncDecExpression() {
335334
// unaryExpression: (PLUS^ | MINUS^ | BANG^ | INC^ | DEC^) unaryExpression | primaryExpression ;
336335
@Nullable
337336
private SpelNodeImpl eatUnaryExpression() {
338-
if (peekToken(TokenKind.PLUS, TokenKind.MINUS, TokenKind.NOT)) {
337+
if (peekToken(TokenKind.NOT, TokenKind.PLUS, TokenKind.MINUS)) {
339338
Token t = takeToken();
340339
SpelNodeImpl expr = eatUnaryExpression();
341-
Assert.state(expr != null, "No node");
340+
if (expr == null) {
341+
throw internalException(t.startPos, SpelMessage.OOD);
342+
}
342343
if (t.kind == TokenKind.NOT) {
343344
return new OperatorNot(t.startPos, t.endPos, expr);
344345
}
345346
if (t.kind == TokenKind.PLUS) {
346347
return new OpPlus(t.startPos, t.endPos, expr);
347348
}
348-
Assert.isTrue(t.kind == TokenKind.MINUS, "Minus token expected");
349-
return new OpMinus(t.startPos, t.endPos, expr);
349+
if (t.kind == TokenKind.MINUS) {
350+
return new OpMinus(t.startPos, t.endPos, expr);
351+
}
350352
}
351353
if (peekToken(TokenKind.INC, TokenKind.DEC)) {
352354
Token t = takeToken();
353355
SpelNodeImpl expr = eatUnaryExpression();
354356
if (t.getKind() == TokenKind.INC) {
355357
return new OpInc(t.startPos, t.endPos, false, expr);
356358
}
357-
return new OpDec(t.startPos, t.endPos, false, expr);
359+
if (t.kind == TokenKind.DEC) {
360+
return new OpDec(t.startPos, t.endPos, false, expr);
361+
}
358362
}
359363
return eatPrimaryExpression();
360364
}
@@ -414,7 +418,6 @@ private SpelNodeImpl eatDottedNode() {
414418
return pop();
415419
}
416420
if (peekToken() == null) {
417-
// unexpectedly ran out of data
418421
throw internalException(t.startPos, SpelMessage.OOD);
419422
}
420423
else {
@@ -460,8 +463,7 @@ private SpelNodeImpl[] maybeEatMethodArgs() {
460463

461464
private void eatConstructorArgs(List<SpelNodeImpl> accumulatedArguments) {
462465
if (!peekToken(TokenKind.LPAREN)) {
463-
throw new InternalParseException(new SpelParseException(this.expressionString,
464-
positionOf(peekToken()), SpelMessage.MISSING_CONSTRUCTOR_ARGS));
466+
throw internalException(positionOf(peekToken()), SpelMessage.MISSING_CONSTRUCTOR_ARGS);
465467
}
466468
consumeArguments(accumulatedArguments);
467469
eatToken(TokenKind.RPAREN);
@@ -472,7 +474,9 @@ private void eatConstructorArgs(List<SpelNodeImpl> accumulatedArguments) {
472474
*/
473475
private void consumeArguments(List<SpelNodeImpl> accumulatedArguments) {
474476
Token t = peekToken();
475-
Assert.state(t != null, "Expected token");
477+
if (t == null) {
478+
return;
479+
}
476480
int pos = t.startPos;
477481
Token next;
478482
do {
@@ -575,8 +579,7 @@ else if (peekToken(TokenKind.LITERAL_STRING)) {
575579
private boolean maybeEatTypeReference() {
576580
if (peekToken(TokenKind.IDENTIFIER)) {
577581
Token typeName = peekToken();
578-
Assert.state(typeName != null, "Expected token");
579-
if (!"T".equals(typeName.stringValue())) {
582+
if (typeName == null || !"T".equals(typeName.stringValue())) {
580583
return false;
581584
}
582585
// It looks like a type reference but is T being used as a map key?
@@ -605,8 +608,7 @@ private boolean maybeEatTypeReference() {
605608
private boolean maybeEatNullReference() {
606609
if (peekToken(TokenKind.IDENTIFIER)) {
607610
Token nullToken = peekToken();
608-
Assert.state(nullToken != null, "Expected token");
609-
if (!"null".equalsIgnoreCase(nullToken.stringValue())) {
611+
if (nullToken == null || !"null".equalsIgnoreCase(nullToken.stringValue())) {
610612
return false;
611613
}
612614
nextToken();
@@ -619,12 +621,13 @@ private boolean maybeEatNullReference() {
619621
//projection: PROJECT^ expression RCURLY!;
620622
private boolean maybeEatProjection(boolean nullSafeNavigation) {
621623
Token t = peekToken();
622-
if (!peekToken(TokenKind.PROJECT, true)) {
624+
if (t == null || !peekToken(TokenKind.PROJECT, true)) {
623625
return false;
624626
}
625-
Assert.state(t != null, "No token");
626627
SpelNodeImpl expr = eatExpression();
627-
Assert.state(expr != null, "No node");
628+
if (expr == null) {
629+
throw internalException(t.startPos, SpelMessage.OOD);
630+
}
628631
eatToken(TokenKind.RSQUARE);
629632
this.constructedNodes.push(new Projection(nullSafeNavigation, t.startPos, t.endPos, expr));
630633
return true;
@@ -634,15 +637,13 @@ private boolean maybeEatProjection(boolean nullSafeNavigation) {
634637
// map = LCURLY (key ':' value (COMMA key ':' value)*) RCURLY
635638
private boolean maybeEatInlineListOrMap() {
636639
Token t = peekToken();
637-
if (!peekToken(TokenKind.LCURLY, true)) {
640+
if (t == null || !peekToken(TokenKind.LCURLY, true)) {
638641
return false;
639642
}
640-
Assert.state(t != null, "No token");
641643
SpelNodeImpl expr = null;
642644
Token closingCurly = peekToken();
643-
if (peekToken(TokenKind.RCURLY, true)) {
645+
if (closingCurly != null && peekToken(TokenKind.RCURLY, true)) {
644646
// empty list '{}'
645-
Assert.state(closingCurly != null, "No token");
646647
expr = new InlineList(t.startPos, closingCurly.endPos);
647648
}
648649
else if (peekToken(TokenKind.COLON, true)) {
@@ -695,23 +696,23 @@ else if (peekToken(TokenKind.COLON, true)) { // map!
695696

696697
private boolean maybeEatIndexer() {
697698
Token t = peekToken();
698-
if (!peekToken(TokenKind.LSQUARE, true)) {
699+
if (t == null || !peekToken(TokenKind.LSQUARE, true)) {
699700
return false;
700701
}
701-
Assert.state(t != null, "No token");
702702
SpelNodeImpl expr = eatExpression();
703-
Assert.state(expr != null, "No node");
703+
if (expr == null) {
704+
throw internalException(t.startPos, SpelMessage.MISSING_SELECTION_EXPRESSION);
705+
}
704706
eatToken(TokenKind.RSQUARE);
705707
this.constructedNodes.push(new Indexer(t.startPos, t.endPos, expr));
706708
return true;
707709
}
708710

709711
private boolean maybeEatSelection(boolean nullSafeNavigation) {
710712
Token t = peekToken();
711-
if (!peekSelectToken()) {
713+
if (t == null || !peekSelectToken()) {
712714
return false;
713715
}
714-
Assert.state(t != null, "No token");
715716
nextToken();
716717
SpelNodeImpl expr = eatExpression();
717718
if (expr == null) {
@@ -889,9 +890,14 @@ else if (t.kind == TokenKind.LITERAL_STRING) {
889890
//parenExpr : LPAREN! expression RPAREN!;
890891
private boolean maybeEatParenExpression() {
891892
if (peekToken(TokenKind.LPAREN)) {
892-
nextToken();
893+
Token t = nextToken();
894+
if (t == null) {
895+
return false;
896+
}
893897
SpelNodeImpl expr = eatExpression();
894-
Assert.state(expr != null, "No node");
898+
if (expr == null) {
899+
throw internalException(t.startPos, SpelMessage.OOD);
900+
}
895901
eatToken(TokenKind.RPAREN);
896902
push(expr);
897903
return true;

‎spring-expression/src/test/java/org/springframework/expression/spel/standard/SpelParserTests.java

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,9 @@
3939
import static org.springframework.expression.spel.SpelMessage.NON_TERMINATING_QUOTED_STRING;
4040
import static org.springframework.expression.spel.SpelMessage.NOT_AN_INTEGER;
4141
import static org.springframework.expression.spel.SpelMessage.NOT_A_LONG;
42+
import static org.springframework.expression.spel.SpelMessage.OOD;
4243
import static org.springframework.expression.spel.SpelMessage.REAL_CANNOT_BE_LONG;
44+
import static org.springframework.expression.spel.SpelMessage.RIGHT_OPERAND_PROBLEM;
4345
import static org.springframework.expression.spel.SpelMessage.RUN_OUT_OF_ARGUMENTS;
4446
import static org.springframework.expression.spel.SpelMessage.UNEXPECTED_DATA_AFTER_DOT;
4547
import static org.springframework.expression.spel.SpelMessage.UNEXPECTED_ESCAPE_CHAR;
@@ -76,8 +78,8 @@ void blankExpressionIsRejected() {
7678

7779
private static void assertNullOrEmptyExpressionIsRejected(ThrowingCallable throwingCallable) {
7880
assertThatIllegalArgumentException()
79-
.isThrownBy(throwingCallable)
80-
.withMessage("'expressionString' must not be null or blank");
81+
.isThrownBy(throwingCallable)
82+
.withMessage("'expressionString' must not be null or blank");
8183
}
8284

8385
@Test
@@ -152,7 +154,13 @@ void parseExceptions() {
152154
assertParseException(() -> parser.parseRaw("new String(3"), RUN_OUT_OF_ARGUMENTS, 10);
153155
assertParseException(() -> parser.parseRaw("new String("), RUN_OUT_OF_ARGUMENTS, 10);
154156
assertParseException(() -> parser.parseRaw("\"abc"), NON_TERMINATING_DOUBLE_QUOTED_STRING, 0);
157+
assertParseException(() -> parser.parseRaw("abc\""), NON_TERMINATING_DOUBLE_QUOTED_STRING, 3);
155158
assertParseException(() -> parser.parseRaw("'abc"), NON_TERMINATING_QUOTED_STRING, 0);
159+
assertParseException(() -> parser.parseRaw("abc'"), NON_TERMINATING_QUOTED_STRING, 3);
160+
assertParseException(() -> parser.parseRaw("("), OOD, 0);
161+
assertParseException(() -> parser.parseRaw(")"), OOD, 0);
162+
assertParseException(() -> parser.parseRaw("+"), OOD, 0);
163+
assertParseException(() -> parser.parseRaw("1+"), RIGHT_OPERAND_PROBLEM, 1);
156164
}
157165

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

378386
private void checkNumberError(String expression, SpelMessage expectedMessage) {
379387
assertParseExceptionThrownBy(() -> parser.parseRaw(expression))
380-
.satisfies(ex -> assertThat(ex.getMessageCode()).isEqualTo(expectedMessage));
388+
.satisfies(ex -> assertThat(ex.getMessageCode()).isEqualTo(expectedMessage));
381389
}
382390

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

387395
private static void assertParseException(ThrowingCallable throwingCallable, SpelMessage expectedMessage, int expectedPosition) {
388396
assertParseExceptionThrownBy(throwingCallable)
389-
.satisfies(parseExceptionRequirements(expectedMessage, expectedPosition));
397+
.satisfies(parseExceptionRequirements(expectedMessage, expectedPosition));
390398
}
391399

392400
private static <E extends SpelParseException> Consumer<E> parseExceptionRequirements(
393401
SpelMessage expectedMessage, int expectedPosition) {
402+
394403
return ex -> {
395404
assertThat(ex.getMessageCode()).isEqualTo(expectedMessage);
396405
assertThat(ex.getPosition()).isEqualTo(expectedPosition);
397-
assertThat(ex.getMessage()).contains(ex.getExpressionString());
406+
if (ex.getExpressionString() != null) {
407+
assertThat(ex.getMessage()).contains(ex.getExpressionString());
408+
}
398409
};
399410
}
400411

0 commit comments

Comments
 (0)