fix: escapes $$ prior to escaping \$

[shawkins]

This seems to be the most consistent handling for $ escaping - escape existing instances of $$ before replacing $ with $$. This is not of course completely compatible with the existing handling.

closes: #1056

[dmlloyd]

I think this trades one problem for another. If there's a specific parsing mode that we need for expression strings then it should be implemented directly in smallrye-common-expression. The optimization that is attempted by this existing code is problematic at best IMO.

[shawkins]

@dmlloyd it seems like there isn't a great way to do this. The comment https://github.com/smallrye/smallrye-config/blob/889fa9ffb91cd44d3d4b77e82b39c9cb81fc7750/implementation/src/main/java/io/smallrye/config/ExpressionConfigSourceInterceptor.java#L94 seems to make it clear that the more general handling of escape will not happen, so this workaround will remain in place for a while. Any new feature flag for just the handling of $$ can't be added without affecting this workaround.

Are you thinking that something new like Flag.ESCAPES_COMPAT is needed to handle both \$ and not automatically treat $$ as an escaped $?

Or @radcortez based upon your issue comment, are you wanting to further limit where the case and not do the $$ to $ substitution as long as Flag.MINI_EXPRS is not enabled and/or the next character is not {?

[dmlloyd]

@dmlloyd it seems like there isn't a great way to do this. The comment

https://github.com/smallrye/smallrye-config/blob/889fa9ffb91cd44d3d4b77e82b39c9cb81fc7750/implementation/src/main/java/io/smallrye/config/ExpressionConfigSourceInterceptor.java#L94

seems to make it clear that the more general handling of escape will not happen, so this workaround will remain in place for a while. Any new feature flag for just the handling of $$ can't be added without affecting this workaround.

The existing workaround is already concerning IMO. What if it hits \$$? or \\$ or \\\$ or \\\$$... etc. I don't think all of these cases are well-defined in the existing code.

Are you thinking that something new like Flag.ESCAPES_COMPAT is needed to handle both $ and treat not automatically treat $$ as an escaped $?

Exactly. But MP Config itself also does a very poor job of defining the behavior of \\ in a general sense. Once you involve lists or any converter which uses \\ in any other way, you end up with a big mess of confusing semantics.

[dmlloyd]

Adding on to that, I think MP Config's treatment of \$ is an error as well, precisely because of how it interferes with traditional expression syntax, converters, and the properties file spec. How many \\ do you need? It's difficult to know for any given situation.

[shawkins]

The existing workaround is already concerning IMO. What if it hits $$? or \$ or \$ or \$$... etc. I don't think all of these cases are well-defined in the existing code.

I agree it's not well defined.

My interest here was primarily the $$ case to resolve the downstream issue https://github.com/keycloak/keycloak/issues/19831 - but obviously someone will hit the issue of additional or combined escapes eventually.

Downstream I'm proposing that we simply document for now that $$ needs to be escaped as \$\$ https://github.com/keycloak/keycloak/pull/25218 - seems like it should work regardless of the resolution here. If that's all that's worth doing at this time, I'm also fine just with closing this pr.