CDI @ConfigProperty default value consistency should be enforced

[dmlloyd]

If the same configuration property is given more than once via the @ConfigProperty annotation, they should be enforced to have the same default value string, or else an error should result.

This is a necessary prerequisite to #405; if multiple default values are allowed for a given property, then it becomes impossible to define which value would be returned for an expression string which references that property.

The specification text for @ConfigProperty should be updated to this effect.

[dmlloyd]

An alternative is to explicitly specify that default values given in @ConfigProperty are only scoped to that access and will not appear in variable expansions if/when #405 is ratified.

[dmlloyd]

One challenge here is that the only way that equivalence can be enforced would be to cause the container to fail to deploy the application. But we don't really have any way to test that this happens in the TCK. Should we go ahead with the spec change, with wording like "The container may cause the application to fail" or something like that?

[Azquelt]

I don't think we should do this, primarily because I don't think that an annotation on one injection point should affect a value injected at another injection point.

@Inject @ConfigProperty(value="foo", defaultValue="bar")
private String baz;

should be seen as the CDI equivalent of

String baz = Config.getConfig().getOptionalValue("foo", String.class).orElse("bar");

If you need to set the default for a config value across your application, this can be done in the microprofile-config.properties file within the application.

[radcortez]

Yes, that works and aligns with the proposal of default values being scoped to the injection point, but then these should be excluded from property expansion. Consider:

@Inject
@ConfigProperty(value="foo", defaultValue="bar")
private String bar
@Inject
@ConfigProperty(value="foo", defaultValue="baz")
private String baz

expand=${foo}
Config.getConfig().getOptionalValue("expand", String.class);

Which one gets expanded into expand?

[Azquelt]

Assuming "foo" is not defined, neither.

Edit: So yes, would also say that the default value in @ConfigProperty should never be used for property expansion.

[Azquelt]

If you want to assign a default value to foo so that it gets expanded, doing this makes more sense:

@Inject
@ConfigProperty(value="foo")
private String bar

@Inject
@ConfigProperty(value="foo")
private String baz

foo=bar
expand=${foo}
Config.getConfig().getOptionalValue("expand", String.class);

[Azquelt]

Alternatively, you could define an expansion syntax which allows for a default value to be provided, but I'm not sure this provides much of an advantage over just defining the default in a config source with a low ordinal.

E.g. expand=${foo:fooDefaultValue}

With this syntax, we'd not be defining a global default, we'd just be setting the value to use in this location if no value exists for the given property.

[radcortez]

Assuming "foo" is not defined, neither.

Correct.

Yes, all of your suggestions are possible and we should probably document them / add them, but we cannot prevent the user for making the described mistake, so the spec needs to specify what should happen in that case.

The easiest is to just say that default values cannot be used for expansion.

[dmlloyd]

@Azquelt I originally had the same opinion, however IIRC Quarkus users almost immediately complained about it - the expectation was really that a default value gets expanded when a property expression references the corresponding property. So I've since revised my view. I think it is logically an error to define the same property at two points with different default values, and I've come to think of the default value as applying to the configuration as a whole; I think this world view is a lot more practical for most users.

Also it is my opinion that single value injection in CDI was a design error, which is something we're exploring in SmallRye right now (smallrye/smallrye-config#279 to see that discussion). I'm feeling more and more that:

• The config API should have a means to globally declare default values (this reminds me to open a SmallRye issue for this...)
• The injection mechanism (either before or after being modified as described above) should use the same mechanism as the config API to define the defaults so that there is only one consistent view
• The config API's default values should be treated as a lowest-priority config source like we do in Quarkus

We'll be exploring these ideas in SmallRye and we will report back our findings as we have them.

[Azquelt]

the expectation was really that a default value gets expanded when a property expression references the corresponding property

If that's actually the case, then possibly we should consider removing the defaultValue attribute?

My expectation would be that it's tied directly to the specific injection point and I'd be very surprised if it caused a change in behaviour anywhere else. @ConfigProperty is used to reference a config variable, so I wouldn't expect it to also be used to globally declare a default value for that variable.

The config API should have a means to globally declare default values

How would this be different from saying that default values should be stored in microprofile-config.properties?

[dmlloyd]

the expectation was really that a default value gets expanded when a property expression references the corresponding property

If that's actually the case, then possibly we should consider removing the defaultValue attribute?

My expectation would be that it's tied directly to the specific injection point and I'd be very surprised if it caused a change in behaviour anywhere else. @ConfigProperty is used to reference a config variable, so I wouldn't expect it to also be used to globally declare a default value for that variable.

My opinion on that is only that (as I said earlier) I feel that the CDI mechanism was mis-designed from the start. If we have a "higher" notion of a configuration object that aggregates the various configuration properties for an application, then suddenly we're referencing configuration rather than a configuration property, and the configuration object itself now logically better aligns with the notion of declaring what properties are expected to be present (and what are optional), along with their default values.

But failing that, at least making default values be uniform moves the needle in the right direction (as far as I can see it).

The config API should have a means to globally declare default values

How would this be different from saying that default values should be stored in microprofile-config.properties?

This depends on what you're configuring and how. MP Config is open-ended: configuration sources may be higher or lower than the JAR's microprofile-config.properties. Frameworks may intend to configure themselves or other frameworks, or they may intend for users to configure them. If a given JAR specifies a default value for its own configuration in its own microprofile-config.properties, there's really no way to know whether or not that value will take precedence over some other value in another microprofile-config.properties in another JAR on the same class path, or whether the container's class loading model will behave in a way that precludes this possibility, let alone values from other configuration sources.

So I don't think that microprofile-config.properties is really suitable for this purpose overall. Maybe if there were another file specifically for defaults, which gets aggregated at a very low priority, that would be more suitable, with advice that frameworks and applications should use it to supply defaults only for the configuration properties that are used by that framework or application. Additionally it should be error-checked because conflicting defaults can cause run time problems. But in any event it makes sense to back such a mechanism with an API method.

[Emily-Jiang]

    the expectation was really that a default value gets expanded when a property expression references the corresponding property

If that's actually the case, then possibly we should consider removing the defaultValue attribute?

My expectation would be that it's tied directly to the specific injection point and I'd be very surprised if it caused a change in behaviour anywhere else. @ConfigProperty is used to reference a config variable, so I wouldn't expect it to also be used to globally declare a default value for that variable.

My opinion on that is only that (as I said earlier) I feel that the CDI mechanism was mis-designed from the start. If we have a "higher" notion of a configuration object that aggregates the various configuration properties for an application, then suddenly we're referencing configuration rather than a configuration property, and the configuration object itself now logically better aligns with the notion of declaring what properties are expected to be present (and what are optional), along with their default values.

But failing that, at least making default values be uniform moves the needle in the right direction (as far as I can see it).

By the way, the design of defaultValue on the injection is only scope to that injection point. I argue what the attemption to use it for variable expansion is not a good design. The variable expansion should ignore the defaultValue. There is no problem as yet. We can clearly spell out when we add variable replacement. The variable replacement should only work on config source defined properties. If the property does not exist, no expansion should happen.

    The config API should have a means to globally declare default values

How would this be different from saying that default values should be stored in microprofile-config.properties?

This depends on what you're configuring and how. MP Config is open-ended: configuration sources may be higher or lower than the JAR's microprofile-config.properties. Frameworks may intend to configure themselves or other frameworks, or they may intend for users to configure them. If a given JAR specifies a default value for its own configuration in its own microprofile-config.properties, there's really no way to know whether or not that value will take precedence over some other value in another microprofile-config.properties in another JAR on the same class path, or whether the container's class loading model will behave in a way that precludes this possibility, let alone values from other configuration sources.

So I don't think that microprofile-config.properties is really suitable for this purpose overall. Maybe if there were another file specifically for defaults, which gets aggregated at a very low priority, that would be more suitable, with advice that frameworks and applications should use it to supply defaults only for the configuration properties that are used by that framework or application. Additionally it should be error-checked because conflicting defaults can cause run time problems. But in any event it makes sense to back such a mechanism with an API method.

If you want to set some default with multiple jars potentially using the same config, it is best to put that config outside the jar, e.g. system properties, environment variables. This is design principles though.

[jbee]

Stumbled upon this when preparing a bit for yesterdays weekly meeting and just want to share a thought.

This is a necessary prerequisite to #405; if multiple default values are allowed for a given property, then it becomes impossible to define which value would be returned for an expression string which references that property.

Doesn't this also imply that values would not be allowed to change?

[dmlloyd]

Stumbled upon this when preparing a bit for yesterdays weekly meeting and just want to share a thought.

This is a necessary prerequisite to #405; if multiple default values are allowed for a given property, then it becomes impossible to define which value would be returned for an expression string which references that property.

Doesn't this also imply that values would not be allowed to change?

No, because even if a value for a property changes, there's still only one single value. Not that I'm in favor of config source mutability (I'm not) but it's essentially a reality today.

But I'd say that default values should not be allowed to change, just for the sake of the user's sanity.

[jbee]

I'm still unravelling the statement and its consequences in my head.

The way I understand it is that if there would be multiple defaults you would not know which one to pick when resolving the substitution. While I can follow this thought I don't see how it applies as defaults are passed by callers, so if a caller resolves a property which then causes a substitution with a value which itself is a property that property would be resolved with no default (as you can't even pass one as caller) so it would not matter if there are call sides for this property which do provide different defaults.

[dmlloyd]

I don't think defaults should be allowed to be passed by callers because it confuses the situation immensely. Also I think users will be very confused about when to pass a default vs when to use Optional.orElse.

[jbee]

After discussing this in yesterdays weekly meeting I first grasped the entirety of this proposal. I try to summarised it again in my own words so that it might help others in getting an understanding.

The issue: The same property can be injected multiple times using @ConfigProperty where each occurrence could state a different defaultValue. Looking at each injection point alone this is fine. When looking at it in context of other features (caching, var expansion, ...) the ambiguity causes issues.

The proposal is this: On deployment CDI

1. verifies that all defaultValue for any configuration property use the same default value or otherwise throws an exception
2. makes the defaultValue from annotations generally available by virtually putting them in a map and wrapping that map in a new ConfigSource with lowest priority so that these defaults become effective even for lookups where the default isn't directly supplied.

The goal is to make defaults apply consistently independent of where and how the lookup was made.

[radcortez]

Additionally, we can consider to add an API to provide the default values and deprecate @ConfigProperty#defaultValue. But that can be done in a separate issue / PR in 2.1 or forward.

[Emily-Jiang]

@jbee and @Emily-Jiang discussed this a bit more now. @jbee suggested to create an inmemory config source with a lowest config_ordinal, which is used when the config properties are not specified in other config sources. In this way, it can force the value consistence. If the same config property has multiple different values, which one wins depends on which injection point is scanned first. This suggests the defaultValue is global as the value will be used by other injection even though defaultValue is not specified as well as config.getValue() look up. Thoughts?

[radcortez]

Does that mean:

@Inject
@ConfigProperty(value="foo", defaultValue="foo")
private String bar

@Inject
@ConfigProperty(value="foo")
private String baz

That:

bar=foo and baz=foo? In essence, a default value configured in another injection point may be used to set the value of the same property name in another injection point without a default value?

[jbee]

yes

[radcortez]

Ok, I'm fine with this.

In SmallRye, we added a specific API at the Builder level to contribute default values.