Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SpEL ternary and Elvis expressions are missing enclosing parentheses in toStringAST() #29463

Closed
MisterRnobe opened this issue Nov 10, 2022 · 3 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: bug A general bug
Milestone

Comments

@MisterRnobe
Copy link

Affects: 5.2.23


Hi

Today I faced with the following problem: ternary operator loses parenthesis after I call .toStringAST(). I implemented the test reproducing the bug:

        var spelExpressionParser = new SpelExpressionParser();
        var expression = (SpelExpression) spelExpressionParser.parseExpression("(a ? 1 : 0) * 10");

        assertEquals("((a ? 1 : 0) * 10)", expression.getAST().toStringAST());

And it fails with the error

org.opentest4j.AssertionFailedError: expected: <((a ? 1 : 0) * 10)> but was: <(a ? 1 : 0 * 10)>

In fact, the actual expression is different since it multiplies 0 to 10 first and then applies ternary operator

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Nov 10, 2022
@sbrannen sbrannen changed the title Ternary operator loses parentheses on .toStringAST() SpEL ternary operator loses closing parenthesis in toStringAST() Nov 10, 2022
@sbrannen sbrannen added type: bug A general bug in: core Issues in core modules (aop, beans, core, context, expression) and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Nov 10, 2022
@sbrannen
Copy link
Member

sbrannen commented Nov 10, 2022

Hi @MisterRnobe,

Thanks for bringing this to our attention, and congratulations on creating your first issue for the Spring Framework.

In fact, the actual expression is different since it multiplies 0 to 10 first and then applies ternary operator

The following test demonstrates that the expression is evaluated successfully, but the assertion for toStringAST() indeed fails as you pointed out.

@Test
void ternaryExpressionEnclosedInParentheses() {
    SpelExpressionParser parser = new SpelExpressionParser();
    SpelExpression expression =
        (SpelExpression) parser.parseExpression("((4 % 2 == 0) ? 2 : 1) * 10");

    assertThat(expression.getValue()).isEqualTo(20);
    assertThat(expression.getAST().toStringAST()).isEqualTo("(((4 % 2) == 0) ? 2 : 1) * 10");
}

So the issue seems to only be the output of toStringAST().

We will look into it.

@sbrannen sbrannen added this to the 5.3.24 milestone Nov 10, 2022
@sbrannen sbrannen self-assigned this Nov 10, 2022
@sbrannen sbrannen changed the title SpEL ternary operator loses closing parenthesis in toStringAST() SpEL ternary an elvis expressions are missing enclosing parentheses in toStringAST() Nov 11, 2022
@sbrannen sbrannen changed the title SpEL ternary an elvis expressions are missing enclosing parentheses in toStringAST() SpEL ternary and Elvis expressions are missing enclosing parentheses in toStringAST() Nov 11, 2022
@sbrannen
Copy link
Member

sbrannen commented Nov 11, 2022

It turns our that Elvis expressions are also missing enclosing parentheses in the generated AST string representation, so I'll address that as well.

sbrannen added a commit to sbrannen/spring-framework that referenced this issue Nov 11, 2022
… in toStringAST()

Prior to this commit, ternary and Elvis expressions enclosed in
parentheses (to account for operator precedence) were properly parsed
and evaluated; however, the corresponding toStringAST() implementations
did not enclose the results in parentheses. Consequently, the string
representation of the ASTs did not reflect the original semantics of
such expressions.

For example, given "(4 % 2 == 0 ? 1 : 0) * 10" as the expression to
parse and evaluate, the result of toStringAST() was previously
"(((4 % 2) == 0) ? 1 : 0 * 10)" instead of
"((((4 % 2) == 0) ? 1 : 0) * 10)", implying that 0 should be multiplied
by 10 instead of multiplying the result of the ternary expression by 10.

This commit addresses this by ensuring that SpEL ternary and Elvis
expressions are enclosed in parentheses in toStringAST().

Closes spring-projectsgh-29463
@sbrannen
Copy link
Member

This has been addressed in 27f3fee for inclusion in 5.3.24 and 6.0 GA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: bug A general bug
Projects
None yet
Development

No branches or pull requests

3 participants