Should Config.getValue() to support variable replacement

[Emily-Jiang]

To address the issue #118, we have this PR #392 introducing a way on variable replacement.

In the class ConfigAccessorBuilder.evaluateVariable(boolean evaluateVariables), we would like to suggest the evaluateVariable is on by default.

However, the above only impacts the variable values obtained via ConfigAccess. In ConfigProperty annotation, we also introduced a property evaluateVariables with the default value of true.

Base on this, it might make sense to support evaluateVaribles by default with Config.getValue(). Basically, if a config contains a variable replacement, the variable will be evaluated.

server.host=localhost
server.port=9080
server.url=http://${server.host}:${server.port}

If this issue is accepted, a backward incompatible change will be introduced. This will lead MP Config release to bump major version.

when calling config.getValue(server.url, String.class) will return

http://localhost:9080

If this issue is accepted, a backward incompatible change will be introduced. Hence, this issue will lead MP Config release to bump major version.

[jmesnil]

I'd really like to avoid having a breaking change due to this feature.

As discussed during the last mp config meeting, we could move on to support variable evaluation and specifies that implementation MUST provide a way to support backwards compatibility.

My suggestion for this would be:

• starting with next release, variable evaluation is enabled by default
• implementation must disable variable evaluation if a well-known property (e.g. mp-config.evaluate-variables.enabled is configured on any of the default config sources (env vars, sys props & microprofile-config.properties)

This way an existing application could run without changing its code by adding the sys prop -Dmp-config.evaluate-variables.enabled=false or the env var MP_CONFIG_EVALUATE_VARIABLES_ENABLED=false and preserve its behaviour.

[jmesnil]

We also need to have a proper spec for what variable evaluation.

I've starting working on a prototype and these are the questions I need to answer to move forward:

• Behaviour when a variable can not be evaluated
• Support for default value
• Support for recursiveness

Starting with your example server.url=http://${server.host}:${server.port}.

What should happen when all variables can be evaluated?

server.host=localhost
server.port=9080
server.url=http://${server.host}:${server.port}

=> the expected value will be http://localhost:9080

What should happen when some variables can not be evaluated?

server.host=localhost
# server.port is not defined
server.url=http://${server.host}:${server.port}

=> The API should throw a NoSuchElementException (or return an Optional.empty()).

Should we support default values?

server.host=localhost
# server.port is not defined but the expression as a default value
server.url=http://${server.host}:${server.port:9080}

=> The expected value will be http://locahost:9080

I really like having default value as it avoid having to create and configure "transitive configuration properties" for such use case.

Having:

server.url=http://${server.host:localhost}:${server.port:9080}${server.path:/mystuff}

is much simpler to maintain and configure than:

server.host=localhost
server.port=9080
server.path=/mystuff
server.url=http://${server.host}:${server.port}${server.path}

(and we still support the latter for complex configuration where many other configuration properties could reference server.host and server.port)

Should we support expression in the default values?

This one makes sense only if we support default values

enabled=${outer.enabled:{$inner.enabled:false}}

would evaluate to:

• false if neither outer.enabled nor inner.enabled are configured
• the value of inner.enabled if it is configured and outer.enabled is not configured
• the value or outer.enabled if it is configured (and the configuration of inner.enabled is not taken into account)

My personal opinion is that we should support only raw "unevaluated" default value and not expression in the default values.

[jmesnil]

I've opened #405 to add variable expansion to the spec. The PR contains spec and TCK tests that will make it simpler to agree on the feature.

[radcortez]

What are the plans for variable replacement in MP Config API?

We just implemented a possible solution in SmallRye Config, but I'm not sure if it is the right time to move it here or even think about something different in the context of MP Config.

I'm wondering if we should give more time for the implementations to come up with their solutions and field test them before deciding something here. So maybe, we should postpone this for 2.1?

[Emily-Jiang]

I would like this to be part of ConfigValue interface.

[radcortez]

Which ConfigValue interface? We did create a ConfigValue object in SmallRye Config, but this imo also is not field proven yet, so I'm wondering that we should experiment with this a little more.

If you want to have a look, here is some of the work we have done:
smallrye/smallrye-config#249

Property Expansion Support:
smallrye/smallrye-config#266

[Emily-Jiang]

There was a ConfigValue prior to Config 1.4 release, but we deleted it to get 1.4 release. We need to bring it back.

[Emily-Jiang]

I took at your PRs @radcortez ! I think there is the common understanding that ConfigValue consists of name, value. I am thinking add another method to expandValue. I am not sure why you need any config source ordinal, config source name, etc. By the way, the InterceptorContext should be implementation details, right?

[radcortez]

We are not sure what we need in ConfigValue. For instance, ConfigSource name and ordinal were added so you can retrieve some information from the ConfigSource used to load a that particular configuration. Here is another sample:
smallrye/smallrye-config#262

Well the idea with the interceptor was to provide an easy way to extend and implement many of the features we are discussing here, so I would like to see a similar approach done in the spec. For instance, with the interceptor chain, it would also be easy to implement something like Configuration Name Relocation:
smallrye/smallrye-config#268

[dmlloyd]

I took at your PRs @radcortez ! I think there is the common understanding that ConfigValue consists of name, value. I am thinking add another method to expandValue. I am not sure why you need any config source ordinal, config source name, etc.

Remember, we are driving the implementation by use cases, not the other way around. One of the stated use cases for having a ConfigValue API was so that we can explain what configuration source a configuration value came from. So that's where the name comes in.

Another use case is to be able to support profiles in the manner of Quarkus. For this, an interceptor is required; if interceptors use ConfigValue (which is the "obvious" implementation) then the interceptor needs access to the ordinal of the config source to determine which property takes priority (the per-profile property name only takes effect when it comes from the same or higher ordinal config source).

[dmlloyd]

I would like this to be part of ConfigValue interface.

The use case is twofold: to be able to read properties with expanded values, and to be able to selectively enable or disable property variable expansion.

Property expansion is best achieved using interceptors. This allows the user to disable property expansion globally and optionally substitute their own implementation or even implement expansion on a per-property basis if they choose. So it doesn't necessarily make sense to tie in property expansion, which is effectively an orthogonal concern, to the config value API. So far everything on ConfigValue has been a static property of the configuration value, without program logic. To me it is not a good design choice to change that when we can meet the corresponding use case in a similarly orthogonal API.

[svenhaag]

Sound great! How do you handle empty or missing values from a high order config source? Currently the list of ConfigSources is iterated until a value is perhaps present, see: io.smallrye.config.SmallRyeConfig#getRawValue.
I think this has to be reconsidered?

[dmlloyd]

Empty values are addressed by #541. An explicit empty value is to be considered an intent to have an empty value for the property, which is not the same thing as undefining the property (such that it falls through to the next configuration source). If a property expression expands to an empty string, then the property is considered to be explicitly empty.

[Emily-Jiang]

If a property expression expands to an empty string, then the property is considered to be explicitly empty.

If it is empty, with #541, it means the property value does not exist. After the expansion, it does not exist, an exception should be thrown.
With the interceptor approach, how to retrieve the raw value per config?

[radcortez]

There is a final interceptor in the chain that performs the regular lookup. So, if the property doesn't exist, you get the exception.

[Emily-Jiang]

With the interceptor approach, how to retrieve the raw value per config?

[radcortez]

The same thing. The final chain will retrieve the raw value, but when going up the chain, that value can be modified by any interceptor. Something like:

Chain 
  Expand
    GetRawValue