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

fix: escapes $$ prior to escaping \$ #1059

Closed
wants to merge 1 commit into from
Closed

Conversation

shawkins
Copy link
Contributor

@shawkins shawkins commented Dec 4, 2023

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
Copy link
Contributor

dmlloyd commented Dec 4, 2023

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
Copy link
Contributor Author

shawkins commented Dec 4, 2023

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

* MicroProfile Config defines the backslash escape for dollar to retrieve the raw expression. We don't want to
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
Copy link
Contributor

dmlloyd commented Dec 4, 2023

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

* MicroProfile Config defines the backslash escape for dollar to retrieve the raw expression. We don't want to

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
Copy link
Contributor

dmlloyd commented Dec 4, 2023

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
Copy link
Contributor Author

shawkins commented Dec 4, 2023

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 keycloak/keycloak#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 \$\$ keycloak/keycloak#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.

@radcortez
Copy link
Member

@shawkins, thank you for the PR.

We are now proposing a new alternative to fix the issue: smallrye/smallrye-common#344

@radcortez radcortez closed this Sep 13, 2024
@shawkins
Copy link
Contributor Author

We are now proposing a new alternative to fix the issue: smallrye/smallrye-common#344

Great, hopefully we'll be able to adopt that in Keycloak.

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.

Handling of $$ in value
3 participants