Support variable expansion

[jmesnil]

• Spec
  • Update specification with Variable Expansion chapter
  • Add a note about backwards compatibilty
• API
  • rename evaluateVariables() to expandVariables()
• TCK
  • Add VariableExpansionTest to the TCK to validate variable expansion

Signed-off-by: Jeff Mesnil jmesnil@redhat.com

[jmesnil]

The feature breaks backward compatibility (as explained in the spec) but this feature is too useful to be disabled by default.
We can mandate that implementation provides backwards compatibility by disabling variable expansion (with a configuration property mp-config.expand-variables).

With that requirement, could we add this feature to MP Config 1.4 (without having to bump the major version)?

[Emily-Jiang]

Not sure we are ready to do this PR as yet. I would like to ask the community first for this backward incompatibility changes first.

[jmesnil]

TODO: rename it to mp.config.expandVariables to be consistent with other MP specs

[dmlloyd]

Could this be implemented as a default method on ConfigSource, whose default implementation is to evaluate that property? This would allow for config source implementations that always or never expand properties. For example, we've already run into trouble with property expansion in environment variables which contain ${..} sequences.

[jmesnil]

The intent of this property is a global switch that applies at the Config level (either try to expand all properties or not).

If I follow you, you want to be able to configure variable expansion on a ConfigSource level.
E.g.

# config_source_1
mp.config.source.expandVariables=true
host=localhost
port=8080
server=http://${host}:${port}

# config_source_2
mp.config.source.expandVariables=false
server=http://${host}:${port}

If config_source_1 has higher ordinal, config.getValue("server", String.class) would return http://localhost:8080 (as its variables are expanded).
If config_source_2 has higher ordinal, config.getValue("server", String.class) would return http://${host}:${port} (as its variables are not expanded).

@dmlloyd is that the behaviour you are expecting?

[dmlloyd]

No, I had just misinterpreted the description of the functionality. I thought the property applied to each config source but the specification states that it must be configured in one of the default config sources to apply (globally).

That said, we did internally talk about the fact that we probably do not want to allow expansion within environment variable values as this could cause issues with certain environments.

[pasumarthivijaykumar]

The intent of this property is a global switch that applies at the Config level (either try to expand all properties or not).

If I follow you, you want to be able to configure variable expansion on a ConfigSource level.
E.g.

# config_source_1
mp.config.source.expandVariables=true
host=localhost
port=8080
server=http://${host}:${port}

# config_source_2
mp.config.source.expandVariables=false
server=http://${host}:${port}

If config_source_1 has higher ordinal, config.getValue("server", String.class) would return http://localhost:8080 (as its variables are expanded).
If config_source_2 has higher ordinal, config.getValue("server", String.class) would return http://${host}:${port} (as its variables are not expanded).

@dmlloyd is that the behaviour you are expecting?

Is this implemented already and can you please let us know how to use it and details.?

[Emily-Jiang]

-1 on specifying the location of the property. It should be allowed to be specified anywhere as a config, as long as it is available on application starting

[dmlloyd]

If I understand correctly, the property is supposed to apply on a per-source basis only, so that some sources can have expressions and some not.

[jmesnil]

@dmlloyd no, that's not the intent.

The intent is to support the mp.config.evaluateVariables property to configure globally variable evaluation on a Config level.
The wording is poor. I just wanted to restrict the usage of this property to the 3 default config sources so that a ConfigBuilder will only look in these 3 config sources to determine the behaviour of the built Config.

[jmesnil]

@emilyjiang we can not specify this property that is using to configure the Config itself on any ConfigSource or we end up with a chicken-and-egg situation.

Let's say we have a 3rd-party Zookeeper ConfigSource that calls ConfigProvider.getConfig() to get its own configuration (to be able to connect to ZK).

If we look for any ConfigSource to get this mp.config.evaluateVariables, we will call ZooKeep ConfigSource that will call ConfigProvider.getConfig() to get its configuration (to be able to fetch its properties). In turn this will call the method responsible to get the mp.config.evaluateVariables property, etc.

Restricting to the 3 default ConfigSource avoid this situation. This property is a special case: it it used to configure the ConfigBuilder itself, not the application. It seems ok to me to limit its use.

[Emily-Jiang]

Not per source but per application

[dmlloyd]

Not per source but per application

I guess this was meant to be in reply to #405 (comment) ?

There are two points where expansion could be decidable. One is per-source (for example, the environment often can contain things that look like expressions but would fail on expansion), and the other is the application. I believe the specification allows both. But the paragraph in question that you responded to seems to be referring to controlling expansion by source, not by application.

[gunnarmorling]

One tiny typo fix. Also I was wondering: is escaping handled somewhere? I.e. how can I have a config var to return ${name} literally instead of resolving it?

[gunnarmorling]

Suggested change

	The value of a configuration property can contains a variable corresponding to another configuration property.
	The value of a configuration property can contain a variable corresponding to another configuration property.

[svenhaag]

Hi folks, why can't we have a more generic approach to this? I think of invoking (custom) interceptors/filters before returning a value. Each interceptor should be provided with a context that e.g. contains all (unresolved) config properties from ConfigProvider#getConfig().

With this, a variable expansion would be just an implementation of this interface.

[dmlloyd]

In SmallRye, we provide an API to allow a ConfigSource to act as a wrapper for another ConfigSource, which is interceptor-like. This is, as you suggest, exactly how we implement variable expansion in Quarkus, and multiple other features as well. We could do something similar in the spec.

[radcortez]

This should be a mandatory feature for 2.0.

The question is, how to implement it.

At this time, I believe we should expose an ahead of time declaration of behaviour (either with hooks, interceptors, filters) to make this more flexible and also has suggested by @svenhaag and discussed in #524.

[dmlloyd]

I think we should close this old PR and open one or more new Use Case issue(s) to cover this functionality. WDYT?

[dmlloyd]

Closing this PR, as we discussed previously.