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

Expression NO_$$ mode #344

Merged
merged 1 commit into from
Oct 1, 2024
Merged

Expression NO_$$ mode #344

merged 1 commit into from
Oct 1, 2024

Conversation

@dmlloyd
Copy link
Collaborator

dmlloyd commented Sep 17, 2024

There are some subtle problems with this approach, but before I get into it, I want to ask: is there a reason why we don't change this line in ExpressionConfigSourceInterceptor to add the ESCAPES flag?

        Expression expression = Expression.compile(escapeDollarIfExists(configValue.getValue()), LENIENT_SYNTAX, NO_TRIM,
                NO_SMART_BRACES, DOUBLE_COLON);

@radcortez
Copy link
Member Author

One of the reasons was that it doesn't play well with the comma escape for multiple elements in arrays, like array=cat,dog,${mouse},sea\\,turtle

@dmlloyd
Copy link
Collaborator

dmlloyd commented Sep 17, 2024

Sure, I can see that. However it is not possible to unambiguously discriminate in this case, so I think we may just have to "eat it" and require double-escaping for lists in order to truly fix the problem, or else come up with a single expression escaping scheme that also covers the list case (which is much harder than it would seem, since lists of lists are a thing).

@radcortez
Copy link
Member Author

That is something we can explore.

Still, since we mandate the brackets to define the expression, I think the $ escape should not be applied when a random $$ is found in the middle of a nonexpression. From my understanding, the $ escape is there for cases where you have single letter expressions with MINI_EXPRS, but that is not our case, so it feels weird.

On another note, what are the subtle issues with this approach? :)

@dmlloyd
Copy link
Collaborator

dmlloyd commented Sep 18, 2024

Basically, this is a state machine (a DFA), and each state is well-defined (basically meaning, every character path is evaluated and tested for each state). By adding a boolean flag which sometimes changes the code path, the number of states has doubled. Unless every new state is tested (or proven not to be enterable) then there is a lacking of test coverage.

@dmlloyd
Copy link
Collaborator

dmlloyd commented Sep 18, 2024

I think you're right about $$ - that should be refactored to be a part of MINI_EXPRS, and having it be separate along with the lack of ESCAPES really causes confusion and problems. It'll break some things but I think the end result will be more consistent and easier to explain.

@dmlloyd
Copy link
Collaborator

dmlloyd commented Sep 18, 2024

Of course then I'll have to find all the places where I said that $$ is the official correct way to escape $ and update that. 😢

@radcortez
Copy link
Member Author

Basically, this is a state machine (a DFA), and each state is well-defined (basically meaning, every character path is evaluated and tested for each state). By adding a boolean flag which sometimes changes the code path, the number of states has doubled. Unless every new state is tested (or proven not to be enterable) then there is a lacking of test coverage.

I added the flag make the checks easiers, but I think it could be writtent without it.

I think you're right about $$ - that should be refactored to be a part of MINI_EXPRS, and having it be separate along with the lack of ESCAPES really causes confusion and problems. It'll break some things but I think the end result will be more consistent and easier to explain.

Ok, so we have two options:

Either we go with the new expression mode and cover our current expected cases

or

We turn on ESCAPES and make $$ escape only for MINI_EXPRS. We know it breaks escaping commas for lists, which probably has a shallow usage (if any). We need to check if there are other cases in which it breaks.

@radcortez
Copy link
Member Author

Of course then I'll have to find all the places where I said that $$ is the official correct way to escape $ and update that. 😢

Or we could still use it if $ is immediately followed by a { with an ending }, and ignore it in all the other cases.

@radcortez
Copy link
Member Author

I've made a small change to remove the flag.

@radcortez radcortez force-pushed the strict-escapes branch 2 times, most recently from 498f49e to 25912c6 Compare September 25, 2024 12:41
@radcortez radcortez requested a review from dmlloyd September 25, 2024 12:50
@radcortez radcortez changed the title Expression STRICT_ESCAPES mode Expression NO_SS mode Sep 25, 2024
@radcortez radcortez requested a review from dmlloyd September 25, 2024 21:39
@radcortez radcortez changed the title Expression NO_SS mode Expression NO_$$ mode Sep 25, 2024
@radcortez radcortez force-pushed the strict-escapes branch 2 times, most recently from 6b0bc75 to c8fddf8 Compare September 30, 2024 16:48
@radcortez radcortez requested a review from dmlloyd September 30, 2024 16:54
Copy link
Collaborator

@dmlloyd dmlloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, nice and minimal now. Thanks for your patience!

@radcortez
Copy link
Member Author

All good! Thank you for your patience! :)

@radcortez radcortez merged commit 9aff0b4 into smallrye:main Oct 1, 2024
4 checks passed
@github-actions github-actions bot added this to the 2.7.0 milestone Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants