smallrye-common icon indicating copy to clipboard operation
smallrye-common copied to clipboard

Expression MP mode

Open radcortez opened this issue 1 year ago • 14 comments

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 avatar Oct 03 '24 22:10 radcortez

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

radcortez avatar Oct 07 '24 15:10 radcortez

We would never use this mode, would we?

dmlloyd avatar Oct 07 '24 15:10 dmlloyd

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.

radcortez avatar Oct 07 '24 15:10 radcortez

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.

dmlloyd avatar Oct 07 '24 16:10 dmlloyd

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.

radcortez avatar Oct 07 '24 17:10 radcortez

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.

dmlloyd avatar Oct 07 '24 18:10 dmlloyd

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:

  • https://github.com/quarkusio/quarkus/issues/41883
  • https://github.com/smallrye/smallrye-config/issues/1056

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.

radcortez avatar Oct 08 '24 12:10 radcortez

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 avatar Oct 08 '24 12:10 dmlloyd

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

dmlloyd avatar Oct 08 '24 12:10 dmlloyd

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?

radcortez avatar Oct 08 '24 12:10 radcortez

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.

dmlloyd avatar Oct 08 '24 12:10 dmlloyd

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?

radcortez avatar Oct 08 '24 13:10 radcortez

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

dmlloyd avatar Oct 08 '24 14:10 dmlloyd

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.

radcortez avatar Oct 09 '24 10:10 radcortez