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

Configproperty value containing '$$' will be '$' as string #41883

Open
appiepollo14 opened this issue Jul 14, 2024 · 11 comments
Open

Configproperty value containing '$$' will be '$' as string #41883

appiepollo14 opened this issue Jul 14, 2024 · 11 comments
Assignees
Labels
area/config kind/bug Something isn't working

Comments

@appiepollo14
Copy link
Contributor

appiepollo14 commented Jul 14, 2024

Describe the bug

When injecting a configproperty, containing '$$' , the string value will contain just one dollarsign: '$' See my attached reproducer. I've not found any other special character with this behavior. Is this expected behavior, from the microprofile spec which I've found or is this a bug?

Run the tests and see the one which is failing for a reproducer.

Expected behavior

I would like to be able to use the correct value of a configproperty with a value containing: '$$'

Actual behavior

The value of a configproperty with a value containing: '$$' is changed to: '$'

How to Reproduce?

Run the test in: https://github.com/appiepollo14/quarkus-reproducer/tree/configproperty

and see the following diff:

Scherm­afbeelding 2024-07-14 om 18 23 13

Output of uname -a or ver

Darwin MacBook-Pro.local 23.5.0 Darwin Kernel Version 23.5.0: Wed May 1 20:12:58 PDT 2024; root:xnu-10063.121.3~5/RELEASE_ARM64_T6000 arm64

Output of java -version

openjdk version "21.0.2" 2024-01-16

Quarkus version or git rev

3.12.2

Build tool (ie. output of mvnw --version or gradlew --version)

Apache Maven 3.9.1 (2e178502fcdbffc201671fb2537d0cb4b4cc58f8)

Additional information

It doesn't matter whether the value is set via an environment variable or for example in the applications.properties file

@appiepollo14 appiepollo14 added the kind/bug Something isn't working label Jul 14, 2024
Copy link

quarkus-bot bot commented Jul 14, 2024

/cc @radcortez (config)

@dmlloyd
Copy link
Member

dmlloyd commented Jul 15, 2024

The property expression syntax uses $ as the prefix for a property expansion, e.g. ${name}. $$ is an escape syntax for $. So, if you want a $$ in your final property, you could give $$$$. Alternatively, you could instead give \$\$.

@radcortez
Copy link
Member

We do $$ escape even if there is no opening curly brace in the value. I guess this is because of MINI_EXPRS, which we don't have on.

We probably shouldn't be making the $$ escape when MINI_EXPRS is off and there is no opening curly brace immediately after a $$. What do you think?

@appiepollo14
Copy link
Contributor Author

@radcortez thanks for your reply. Exactly that's what I expect. I would like to be able to use the raw value: $$ within a Configproperty. My use-case is as follows:

Use a secret holding a key, in this case containing $$. Mount this secret as en env var in both a quarkus and a go service. Expect the same value to be used in both. The go service uses the raw value, the Quarkus does something fishy.

@dmlloyd
Copy link
Member

dmlloyd commented Jul 16, 2024

We do $$ escape even if there is no opening curly brace in the value. I guess this is because of MINI_EXPRS, which we don't have on.

We probably shouldn't be making the $$ escape when MINI_EXPRS is off and there is no opening curly brace immediately after a $$. What do you think?

This would cause yet another grammatical ambiguity that could be very hard to untangle. If there is a curly brace right after $$, that has a real meaning to day, which is to not expand the expression. $$ is an escape now, and it should either remain that way or else be changed to no longer be an escape under any circumstances (which will require release notes etc. because it's an incompatible change). To put a literal value into the configuration where we already have numerous syntactic rules, we need some other uniform solution. Putting a bandage over every case where it surprises someone will end up with a parser that is more bandage than logic, and will become (more) difficult to debug and maintain.

@radcortez
Copy link
Member

If there is a curly brace right after $$, that has a real meaning to day, which is to not expand the expression.

Correct. but that is not currently the case. A value like $$ without the curly brance transforms to $ directly. Do you see a reason for that? I was thinking it was because of the MINI_EXPRS, but more interesting if you use $$ with MINI_EXPRS, then the $$ does not work as an escape and $ is the expansion key:

https://github.com/smallrye/smallrye-common/blob/eb96399f98f40f88b2e0cf50f0fe48e1b90b8824/expression/src/test/java/io/smallrye/common/expression/ExpressionTestCase.java#L307-L314

(we don't have that flag on btw)

@dmlloyd
Copy link
Member

dmlloyd commented Jul 19, 2024

The reason is just that this has historically been the escape for $, AFAIK. This property syntax has a fairly long lineage and has gathered some baggage over time. But $$ simply means "emit $". There's not a lot more to it. It's not more problematic than \$ for example (in fact it's less so, because of the number of syntaxes for which \ must also be escaped, resulting in \\$ or \\\\$ or whatever).

@radcortez
Copy link
Member

What if?

# old 
${foo} -> expand
$${foo} -> ${foo}
$$ -> $

# new
${foo} -> expand
$${foo} -> ${foo}
$$ -> $$

@appiepollo14
Copy link
Contributor Author

What if?

# old 
${foo} -> expand
$${foo} -> ${foo}
$$ -> $

# new
${foo} -> expand
$${foo} -> ${foo}
$$ -> $$

Hmm I believe it should be:

# old 
${foo} -> expand
$${foo} -> ${foo}
$$ -> $

# new
${foo} -> expand
$${foo} -> $expand
$$ -> $$

There should just be a check for ${..} if available, then expand. Else just use: $

@radcortez
Copy link
Member

You still want to be able to provide an escape to not expand, and have a ${foo} in your value.

@dmlloyd
Copy link
Member

dmlloyd commented Jul 22, 2024

What if?

# old 
${foo} -> expand
$${foo} -> ${foo}
$$ -> $

# new
${foo} -> expand
$${foo} -> ${foo}
$$ -> $$

This causes complexity in the grammar because now $$ has to look ahead one character to see whether it expands or not, and it also has the problem of creating ambiguity in the single case. You might be tempted to solve this by preprocessing the string somehow before passing it to the expression parser. Don't do this though, it will make the syntax become essentially indescribable, which is a guaranteed source of infinite bugs. For example, when enabling single-character expressions, now you have to figure out what to do with $$X where X is a valid single character expression.

The solution to a syntax inconvenience is almost never to patch the syntax, unless you have examined the syntax description (the original BNF or the DFA) and can show that the change does not introduce an ambiguity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/config kind/bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants