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 MP mode #354

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Expression MP mode #354

wants to merge 1 commit into from

Conversation

radcortez
Copy link
Member

@radcortez radcortez commented Oct 3, 2024

Adds a new Expression mode to support MP escape \$ and keeps compatibility with $$ escape only in the presence of an expression, allowing to escape $${exp}, but keeping exp$$exp as a plain literal.

This allows the removal of the SR Config workaround:
This also allows the removal of the workaround for MP Config:
https://github.com/smallrye/smallrye-config/blob/d447b0c346e39a09c6d07c7d5ce8186bd4154037/implementation/src/main/java/io/smallrye/config/ExpressionConfigSourceInterceptor.java#L102-L123

@radcortez radcortez requested a review from dmlloyd October 3, 2024 22:59
@radcortez
Copy link
Member Author

@dmlloyd let me know if you are ok with this new mode.

@dmlloyd
Copy link
Collaborator

dmlloyd commented Oct 7, 2024

We would never use this mode, would we?

@radcortez
Copy link
Member Author

Well, until we find a better alternative, this would be my proposal based on our discussions:
https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/Expression.20Expansion

We would add this new flag to the list of flags that SR Config already uses: LENIENT_SYNTAX,NO_TRIM, NO_SMART_BRACES, DOUBLE_COLON.

@dmlloyd
Copy link
Collaborator

dmlloyd commented Oct 7, 2024

Wouldn't it be better to just not comply with the MP config spec in this regard? It's clearly wrong. People using \$ would have to change to using $$, but other than that it seems to me we'd be uniformly better off than we are today. Otherwise we're in the definitely worse situation of \ having a variety of effects (or no effects) depending on where it is used.

@radcortez
Copy link
Member Author

People using \$ would have to change to using $$,

No, this mode supports both styles; you can use one or the other to escape an expression.

Otherwise we're in the definitely worse situation of \ having a variety of effects (or no effects) depending on where it is used.

I believe the situation is pretty much the same. It does remove the current workaround in SR Config (so it is the same) and allows you to use $$ anywhere of the expression without being interpreted as an escape, except if $$ proceeds a {.

Is it the ideal world? I agree it isn't, but we discussed other directions which cause a greater impact. I'm pretty sure we can change the MP spec, but until then we need to rely on subpar implementations, unfortunately.

@dmlloyd
Copy link
Collaborator

dmlloyd commented Oct 7, 2024

I disagree; we did discuss impact but in this case I think the greater consideration is the long-term effect. $$ having a special meaning sometimes and not other times results in more special cases. Same with \. The only acceptable outcomes are ones where there are consistent rules which apply consistently. So far the only options that meet this criterion are: \ escapes always, and \ does not escape under any circumstances (but $$ always does).

The impact of switching to a consistent solution is short-term. Once everyone has adapted to the new reality, it will be a stable solution always with no new bug reports. The impact of switching to an inconsistent solution is long-term. As people discover problems, we add more exceptional cases to cover them, and then as people discover problems with those, we add yet more exceptional cases. The code gets more complex, and compatibility becomes more and more difficult.

@radcortez
Copy link
Member Author

I'm fine to remove the MP escape \$.

But I do think that the $$ escape must be context-aware. While it is not common to have values with $$ (these can happen, namely in passwords), users don't expect to have $$ to turn to $ if not in the presence of an expression. As examples:

While documented, I would say most users are unaware of this and only look for it when they need to escape an actual expression. This leads to hard-to-debug problems, especially with passwords, because you get a wrong password problem and have no idea why.

@dmlloyd
Copy link
Collaborator

dmlloyd commented Oct 8, 2024

It cannot be context aware though. Otherwise you have an ambiguous parsing rule for $$${ for example. Would this be $ followed by expression, or $${, or something else? And, why?

@dmlloyd
Copy link
Collaborator

dmlloyd commented Oct 8, 2024

This is to say, we need to solve the password issue some other way IMO.

@radcortez
Copy link
Member Author

It cannot be context aware though. Otherwise you have an ambiguous parsing rule for $$${ for example. Would this be $ followed by expression, or $${, or something else? And, why?

My proposal is to be $${} (which is implemented). Any number of $ followed by { will always become $ - 1 followed by {. I know that it is not ideal, but it is less likely to happen.

This is to say, we need to solve the password issue some other way IMO.

Ok, what are the options?

@dmlloyd
Copy link
Collaborator

dmlloyd commented Oct 8, 2024

We could have a literal meta-expression. Or, a no-expression quoting scheme. Or, better yet, don't encode passwords in cleartext. We could aggressively reject syntax errors in expressions so that we don't start up if there's an invalid $ sequence.

@radcortez
Copy link
Member Author

Or, better yet, don't encode passwords in cleartext.

Well, you know that it is pretty much impossible to force users to do that :) With the new Secret type, we can force it into that direction (which I planned to do), but it won't force users with their own configuration.

So, considering a value as foo$$bar, currently the only option is to double escape $ as foo$$$$bar to get the intended value. How are you thinking about the literal or quoting scheme? Won't both options require changing the original value in someway?

@dmlloyd
Copy link
Collaborator

dmlloyd commented Oct 8, 2024

Yes, the original value must be changed no matter what if it has an unintended expression string or an unintended escape in it.

@radcortez
Copy link
Member Author

Then I feel it is not going to improve the situation.

I would like to avoid users having to jump through additional hoops to get what they expect. Modifying the value with an additional escape, quoting scheme, or literal is pretty much the same situation. It may improve how the issue is resolved but will not help identify the problem.

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