Quarkus: Wrong Property Expressions evaluation

[provcristianmaluenda]

Describe the bug
When an application.properties has an expression with the character {, Quarkus evaluates the expression wrong.

Expected behavior
If I have the property:

key = ${TOKEN:111111{11}

Using the annotation @ConfigProperty(name = "key"), I would expect to get the value:

• key: 111111{11

Actual behavior
The problem is, the configuration doesn't understand the chart { in the middle of an expression, and it doesn't close the expression correctly. The result of the previous case is:
key: 111111{11} -> This includes the curly brace that closes the expression }.

I tried to scape the curly brace in the expression:

key = ${TOKEN:111111\{11}

But, I got the same result: 111111{11}.
The walkaround that I found is not to close the expression:

key = ${TOKEN:111111{11

That looks wrong to me.

To Reproduce
Steps to reproduce the behavior:

1. Add an expression in the application.properties file with a curly brace key=${SOMETHING:0000{0000}

Configuration

key=${SOMETHING:0000{0000}

Environment (please complete the following information):

• Output of uname -a or ver:

Darwin WALT952837.local 18.0.0 Darwin Kernel Version 18.0.0: Wed Aug 22 20:13:40 PDT 2018; root:xnu-4903.201.2~1/RELEASE_X86_64 x86_64

• Output of java -version:

openjdk version "11.0.1" 2018-10-16
OpenJDK Runtime Environment 18.9 (build 11.0.1+13)
OpenJDK 64-Bit Server VM 18.9 (build 11.0.1+13, mixed mode)

• GraalVM version (if different from Java):
• Quarkus version or git rev:
  I tested on Quarkus 1.5.2.Final
• Build tool (ie. output of mvnw --version or gradlew --version):

------------------------------------------------------------
Gradle 6.4.1
------------------------------------------------------------

Build time:   2020-05-15 19:43:40 UTC
Revision:     1a04183c502614b5c80e33d603074e0b4a2777c5

Kotlin:       1.3.71
Groovy:       2.5.10
Ant:          Apache Ant(TM) version 1.10.7 compiled on September 1 2019
JVM:          11.0.1 (Oracle Corporation 11.0.1+13)
OS:           Mac OS X 10.14 x86_64

Thanks

[gastaldi]

It looks like something to be fixed in smallrye-common: https://github.com/smallrye/smallrye-common

[gastaldi]

/cc @radcortez

[radcortez]

We may need to be careful with this. There are a few flags that can be enabled / disabled in the Expression Expansion:
https://github.com/smallrye/smallrye-common/blob/0b59733491ff936808cd26a4b300f11fe3f2a5f0/expression/src/main/java/io/smallrye/common/expression/Expression.java#L653-L692

They are not exposed to Quarkus, but we could add them. Setting NO_SMART_BRACES would provide the desired behaviour. My concern, is that Quarkus was always set up to not use that, meaning that if we switch that on, we may break someone else configuration.

@provcristianmaluenda as a work around, you could set up your own ExpressionInterceptor like described here: https://smallrye.io/docs/smallrye-config/interceptors/interceptors.html#expression-interceptor, and use the NO_SMART_BRACES flag. As long as the new interceptor runs before the one that ships with Quarkus, it should do the replace first and then the Quarkus one does not do any replacement.

In the meanwhile, we need to decide if we just activate the flag in our current implementation (might be a breaking change) or if we provide a way to configure it (keeping the current configuration, but allowing to modify the flags).

@gastaldi @geoand any thoughts about this?

[geoand]

I would personally be in favor of keeping things as they are now, but _maybe_ allow advanced users a way to configure their own interceptors

[gsmet]

Hmmm, I think the described behavior is incorrect. We should either not consider the close bracket or provide a way to escape the open one but there's definitely something wrong.

[gastaldi]

IMHO we should provide a way to escape it, since generated passwords may contain braces, so asking the developer to perform changes in the code to support that looks weird to me :)

[radcortez]

Just to confirm, before moving this to SmallRye Config, Quarkus was doing expansion here:
https://github.com/quarkusio/quarkus/commit/2a95fc3400a30af061c73a1cf43b870232afdb4e#r40422799

In SmallRye Config we use the same config:
https://github.com/smallrye/smallrye-config/blob/c0a1a95f5cfc31b937a1135a1478960960b8aa9b/implementation/src/main/java/io/smallrye/config/ExpressionConfigSourceInterceptor.java#L36

There is a difference: Quarkus was using the Wildly Common Lib, which we started to migrate to SmallRye Common, the Expression source was copied as is, but I can verify if it is indeed the same.

[provcristianmaluenda]

We may need to be careful with this. There are a few flags that can be enabled / disabled in the Expression Expansion:
https://github.com/smallrye/smallrye-common/blob/0b59733491ff936808cd26a4b300f11fe3f2a5f0/expression/src/main/java/io/smallrye/common/expression/Expression.java#L653-L692

They are not exposed to Quarkus, but we could add them. Setting NO_SMART_BRACES would provide the desired behaviour. My concern, is that Quarkus was always set up to not use that, meaning that if we switch that on, we may break someone else configuration.

@provcristianmaluenda as a work around, you could set up your own ExpressionInterceptor like described here: https://smallrye.io/docs/smallrye-config/interceptors/interceptors.html#expression-interceptor, and use the NO_SMART_BRACES flag. As long as the new interceptor runs before the one that ships with Quarkus, it should do the replace first and then the Quarkus one does not do any replacement.

In the meanwhile, we need to decide if we just activate the flag in our current implementation (might be a breaking change) or if we provide a way to configure it (keeping the current configuration, but allowing to modify the flags).

@gastaldi @geoand any thoughts about this?

I found another walkaround. I can define a key with the token and refer it in the expression:

token=1234{567
key=${TOKEN:${token}}

[provcristianmaluenda]

IMHO we should provide a way to escape it, since generated passwords may contain braces, so asking the developer to perform changes in the code to support that looks weird to me :)

This is my case. I had an API key with braces. My walkaround was adding a key with the token and use it in the expression key=${TOKEN:${token}}

[dmlloyd]

I think smart braces should still be OK, but I would have expected the escape \ to prevent it from being "counted". IMO that's the real bug, if there is one.

[radcortez]

Maybe because the switch statements picks up braces before the escape?

{- https://github.com/smallrye/smallrye-common/blob/master/expression/src/main/java/io/smallrye/common/expression/Expression.java#L516

} - https://github.com/smallrye/smallrye-common/blob/master/expression/src/main/java/io/smallrye/common/expression/Expression.java#L524

\\ - https://github.com/smallrye/smallrye-common/blob/master/expression/src/main/java/io/smallrye/common/expression/Expression.java#L544

[dmlloyd]

Switch is unordered; but perhaps https://github.com/smallrye/smallrye-common/blob/master/expression/src/main/java/io/smallrye/common/expression/Expression.java#L562 should have picked it up, I would have thought (though I haven't had time to examine the code in detail).

[radcortez]

I'll try to have a look.

[radcortez]

Nevermind. We were looking into this in the wrong way.

I just realized that we didn't have the ESCAPES flag on. I did copy the config from the old ExpandingConfigSource:
https://github.com/quarkusio/quarkus/commit/2a95fc3400a30af061c73a1cf43b870232afdb4e#r40422799

Which only had LENIENT_SYNTAX and NO_TRIM. Adding ESCAPES will fix it.

[radcortez]

On the other hand, adding the ESCAPES flags, does remove all escapes completely, meaning that if you want to pass escapes to a Converter (for instance to parse an array), you are required to double double escape.

Now I remember that I think I've tried that configuration and then reverted it back due to that problem.