smallrye / smallrye-common

Common utilities for SmallRye
Apache License 2.0
21 stars 24 forks source link

Expression MP mode #354

Open radcortez opened 2 weeks ago

radcortez commented 2 weeks ago

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 commented 1 week ago

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

dmlloyd commented 1 week ago

We would never use this mode, would we?

radcortez commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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

radcortez commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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

radcortez commented 1 week ago

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.