Specify handling of empty config property

[jmesnil]

• [Spec] add section explaining how the Config API handles a config
  property with missing or empty value
• [Spec] mention that it is possible to set an empty value to reset a
  property from a lower-ordinal config source.
• [TCK] add EmptyConfigPropertyTest to test all cases of empty config
  property

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

[dmlloyd]

I disagree strongly with this behavior. This makes it impossible to have a configuration source which overrides a present value with a non-present value.

[struberg]

yes, this is against the possibility to detect 'nils'. This would break other features, like to 'unconfigure' values.

[jmesnil]

The spec never mentions anything about "unconfiguring" the properties.
My understanding of the spec is that the configsource only allows to overrides values, not to "delete/unconfigure" them.

If a lower configsource defines a=foo and I override that with a higher configsource to be a= (empty value). I expect the Config API to take my value and returns ``.

[struberg]

Probably we never wrote this down but I only explained this in the past. The original handing in DeltaSpike was to have an empty string 'erase' any configured value in a lower ordinal ConfigSource. This turned out to be quite useful. Especially now with Optional<T> it is otherwise possible to come back to the orElse. Do you have a better idea?

[dmlloyd]

The spec never mentions anything about "unconfiguring" the properties.
My understanding of the spec is that the configsource only allows to overrides values, not to "delete/unconfigure" them.

This to me is a mistake and we should amend the specification.

If a lower configsource defines a=foo and I override that with a higher configsource to be a= (empty value). I expect the Config API to take my value and returns ``.

As I stated elsewhere, there is no possible meaningful distinction between empty and missing. At best it encourages poor behavior and UX.

[jmesnil]

I agree with @dmlloyd proposal to modify the Optional case:

Input string	Output type	Current rules	Better rules
""	Optional	Optional.of("")	Optional.empty()

This would make it possible to use the Optional<T> to "unconfigure" the value and use the orElse.

Edit: I agree with david's latest change to treat an empty string as a missing config value.

However, I'm not convinced and need to think more how changing the behaviour for the plain `getValue`.

If I understand you correctly, If I have a lower config source with a=foo that I want to configure, I'd have to provide a higher config source with a=.

In my code, I then have to take into account all the use cases:

• a is not configured anywhere, Config.getValue(String) throws NoSuchElementException
• a is configured in a config source, Config.getValue(String) return foo
• a is configured in a config source and "unconfigured" in a higher config source with an empty string. What does Config.getValue() return?
  • `` (empty string that I specified)
  • null as David is proposing
  • throws a NoSuchElementException (current rule and it is consistent with "unconfiguring" the property)

[dmlloyd]

Having an empty string be an empty string is implicitly contradictory with this requirement. If , returns an array of [ "", "" ] then the empty string must return an array of [ "" ]. The only logical and consistent solution is to have arrays (and lists) ignore empty strings and have an empty string be identical to "not present".

[dmlloyd]

Then this should be Optional.empty().

[dmlloyd]

And this should also be Optional.empty().

[dmlloyd]

Here is my basic problem with the current and proposed rules. An empty string is sometimes considered non-present, and sometimes considered a present empty string. This inconsistency causes a few problems:

• Inconsistency of array element counting: the size of the array is n + 1 (where n is the number of commas) - except when n is zero, meaning an array or list can never be produced which contains exactly one empty string if this rule is followed in this way, which is pretty surprising.
• It becomes impossible for a higher priority configuration source to override a lower value with "not present", because the only "not present" value is null which indicates that a lower priority source must be consulted.
• It is not and should not be meaningful to applications to differentiate between a non-present value and an empty string; even with the best of intentions this would lead to user confusion. It would always be better to use a separate boolean configuration switch in cases where one would be tempted to make such a distinction.

It would be far, far more sensible to always treat all empty values as an explicit "not present", which allows us to have much more consistent rules.

Id	Input string	Output type	Method	Current rules	Better rules
1	missing	String[]	getValue	throws NoSuchElementException	throws NoSuchElementException
2	""	String[]	getValue	{ }	throws NoSuchElementException
3	","	String[]	getValue	{ "", "" }	throws NoSuchElementException
4	",,"	String[]	getValue	{ "", "", "" }	throws NoSuchElementException
5	"foo,bar"	String[]	getValue	{ "foo", "bar" }	{ "foo", "bar" }
6	"foo,"	String[]	getValue	{ "foo", "" }	{ "foo" }
7	",bar"	String[]	getValue	{ "", "bar" }	{ "bar" }
8	" "	String[]	getValue	{ " " }	{ " " }
9	"foo"	String	getValue	"foo"	"foo"
10	""	String	getValue	""	throws NoSuchElementException
11	","	String	getValue	","	","
12	missing	String	getValue	throws NoSuchElementException	throws NoSuchElementException
13	"foo"	String	getOptionalValue	Optional.of("foo")	Optional.of("foo")
14	""	String	getOptionalValue	Optional.of("")	Optional.empty()
15	missing	String	getOptionalValue	Optional.empty()	Optional.empty()
16	""	String[]	getOptionalValue	Optional.of({})	Optional.empty()
17	","	String[]	getOptionalValue	Optional.of({ "", "" })	Optional.empty()
18	",,"	String[]	getOptionalValue	Optional.of({ "", "", "" })	Optional.empty()
19	missing	String[]	getOptionalValue	Optional.empty	Optional.empty()

Updated with the method name

[jmesnil]

@dmlloyd thanks for the table, it clearly identifies all the use cases (and should serve as the basic of a TCK test)

I thinks the following ones are not accurate wrt to the current rules (and disagree with the "" better rules)

Input string	Output type	Current rules	Better rules
missing	String[]	throws NoSuchElementException	{ }
""	String	""	""
missing	String	throws NoSuchElementException	throws NoSuchElementException

I have no issue with changing the rule so that a missing String[] returns an empty array instead of throwing an exception.
However I still think than empty string should be returned as an empty string and not null.
Likewise a missing String should still throw NoSuchElementException and not return null.

[dmlloyd]

@dmlloyd thanks for the table, it clearly identifies all the use cases (and should serve as the basic of a TCK test)

I have no issue with changing the rule so that a missing String[] returns an empty array instead of throwing an exception.
However I still think than empty string should be returned as an empty string and not null.

Unfortunately this just cannot be made to work consistently, especially when custom converters are involved. If an empty string is an empty string, then arrays created from empty strings must be { "" } which is nonsense. Otherwise we get inconsistent values.

Likewise a missing String should still throw NoSuchElementException and not return null.

Ah you're right, that was my mistake. I'll update the table accordingly.

[jmesnil]

@dmlloyd with your latest changes, I approved your changes.

It was bothering me to treat an "" string as null but I have no issue with having an empty string throws a NoSuchElementException... Optics :)

This also is consistent with @struberg comment about unconfiguring properties.
With this change, an empty string value is treated as a missing config and can be used to unconfigure properties from lower config source.

However, this will change the behaviour of existing applications and must bump the MicroProfile Configuration to 2.0.

[jmesnil]

@dmlloyd I've integrated your table in the spec: https://github.com/eclipse/microprofile-config/pull/407/files#diff-8faa438f11f150bc9d369db23f9044a1R120

@struberg I've added a note about resetting a property by setting it to an empty string in a higher-ordinal config source: https://github.com/eclipse/microprofile-config/pull/407/files#diff-8faa438f11f150bc9d369db23f9044a1R113

and I've based the TCK tests on the spec tables to check all these use cases

[jmesnil]

This PR break compatibility as it now treats an empty string as a missing property and must require to bump the major version of the spec to 2.0

[dmlloyd]

Since people are definitely using a missing value to signify that a default should be used, I think it may be worth adding a provision to the spec for making a "default values" configuration source like we do in Quarkus which has bottom priority. It might also be a good idea to make it non-enumerable. But this is something that can be discussed (there are pros and cons).

[njr-11]

In your table, you have,

Input string	Output type	Method	Current rules	Better rules
"foo,"	String[]	getValue	{ "foo", "" }	{ "foo" }
",bar"	String[]	getValue	{ "", "bar" }	{ "bar" }

But what happens when the user specifies,

Input string	Output type	Method	Current rules	Better rules
"foo,,bar"	String[]	getValue	{ "foo", "", "bar" }	???

It seems that for consistency with the previous two example, you would need to remove the empty string value from the middle of this list as well. However, removing empty strings that the user has explicitly specified, whether at the beginning, end, or middle, seems wrong to me. If you do that, how do they go about specifying an empty string when they actually want one?

Also, I want to point out that we're currently trying to write a spec that lets the user specify an empty list value in MicroProfile Config. I opened #397 specifically for this because the MP Config spec was unclear on whether to interpret
my.config.property=
as String[] { "" } or String[] {}. Due to the uncertainty, we ended up writing our spec to state that both of these values must interpreted as meaning the user wants a size 0 (empty) list.
However, If I understand the table correctly, this change makes the problem even worse, because it is saying that,
my.config.property=
will no longer have a value at all and will now be treated the same as if the property were completely missing from config. To put it more succinctly, the proposed changes here do not resolve #397 - they actually make the problem worse by getting rid of the ability for the user to indicate that wanted a size 0 (empty) list.

[dmlloyd]

In your table, you have,

Input string Output type Method Current rules Better rules
"foo," String[] getValue { "foo", "" } { "foo" }
",bar" String[] getValue { "", "bar" } { "bar" }
But what happens when the user specifies,

Input string Output type Method Current rules Better rules
"foo,,bar" String[] getValue { "foo", "", "bar" } ???
It seems that for consistency with the previous two example, you would need to remove the empty string value from the middle of this list as well.

Exactly, you'd get a two-element list. This is also useful for expression cases like this:

foo.my.property=something,${foo.other.property:},something-else

However, removing empty strings that the user has explicitly specified, whether at the beginning, end, or middle, seems wrong to me. If you do that, how do they go about specifying an empty string when they actually want one?

They can't have one under the current rules. And note that under the current SmallRye implementation, trailing empty segments were already ignored and have been all along.

If a user wants to explicitly have empty strings, then we need a quoting mechanism. But I think this would be a design/UX error in 90% of cases.

Also, I want to point out that we're currently trying to write a spec that lets the user specify an empty list value in MicroProfile Config. I opened #397 specifically for this because the MP Config spec was unclear on whether to interpret
my.config.property=
as String[] { "" } or String[] {}. Due to the uncertainty, we ended up writing our spec to state that both of these values must interpreted as meaning the user wants a size 0 (empty) list.
However, If I understand the table correctly, this change makes the problem even worse, because it is saying that,
my.config.property=
will no longer have a value at all and will now be treated the same as if the property were completely missing from config. To put it more succinctly, the proposed changes here do not resolve #397 - they actually make the problem worse by getting rid of the ability for the user to indicate that wanted a size 0 (empty) list.

The user would always get an empty list for an empty value; this would not get rid of that. This gets rid of the logic that would result in a String[] { "" } array or list, which is not useful most of the time.

[njr-11]

The user would always get an empty list for an empty value; this would not get rid of that. This gets rid of the logic that would result in a String[] { "" } array or list, which is not useful most of the time.

That means I misinterpreted the meaning of { } in your table. Sorry about that.
However, your table still has missing and "" resolving to the same thing, which means we cannot disambiguate between the user's attempt to specify a size 0 list/array vs not specifying any override at all, and this will be a problem for our spec because we need to make that distinction,

Input string	Output type	Method	Current rules	Better rules
missing	String[]	getValue	{ }	{ }
""	String[]	getValue	{ }	{ }

Or, maybe I don't understand what you mean by missing. I've been assuming that you mean that the key/value pair is missing entirely. But if you only meant that the value is missing but the key is present, then this is fine.

[dmlloyd]

The user would always get an empty list for an empty value; this would not get rid of that. This gets rid of the logic that would result in a String[] { "" } array or list, which is not useful most of the time.

That means I misinterpreted the meaning of { } in your table. Sorry about that.
However, your table still has missing and "" resolving to the same thing, which means we cannot disambiguate between the user's attempt to specify a size 0 list/array vs not specifying any override at all, and this will be a problem for our spec because we need to make that distinction,

Right. But I don't think you do need to make that distinction - I suspect what you're really trying to determine is whether the user explicitly wants no contexts to be propagated, or whether the default should be used (whatever that may be) - if I'm wrong about that, please correct me.

The spec still has a hole here that I want to fix as a part of this change. In Quarkus for example the default configuration values exist as a config source. So if the user does not give a key, calling getValue() on that key will return the default value. The disadvantage to this approach though is that the default values have to be known at the time the configuration is created - this is something we can do in Quarkus but it's not practical for other scenarios; also it ignores the fact that, as a practical matter, default configuration values are really supported by use site today and making them all be declared up front is probably not even possible. So I would propose a new API method, something like this:

    /**
     * Return the resolved property value with the specified type for the
     * specified property name from the underlying {@link ConfigSource ConfigSources},
     * returning a default value if the property name is not found in any of them.
     * 
     * If this method gets used very often then consider to locally store the configured value.
     *
     * @param <T>
     *             The property type
     * @param propertyName
     *             The configuration propertyName.
     * @param propertyType
     *             The type into which the resolve property value should get converted
     * @param defVal
     *             The default value to use if no configuration source contains the configuration property name, or {@code null} to indicate that the property should be treated as non-present
     * @return the resolved property value as an object of the requested type.
     * @throws java.lang.IllegalArgumentException if the property cannot be converted to the specified type.
     * @throws java.util.NoSuchElementException if the property isn't present in the configuration.
     */
    <T> T getValue(String name, Class<T> aClass, T defVal);

This could also be extended to getOptionalValue. Alternatively, defVal could be typed as a String.

This basically solves the problem of needing to fall back to a default if the key is not specified, while also allowing the user to specify no value explicitly which would override the default.

[dmlloyd]

@jmesnil tells me that the new ConfigAccessor API already does this exact thing, so I think that this problem is already solved.

[manovotn]

@njr-11 @dmlloyd I still feel you are talking about two slightly different scenarios.

So assuming I have a config foo.bar= and I invoke getValue("foo.bar", String[].class), what do I get with the changes proposed here?
Because Smallrye currently gives me NoSuchElementException - treating this the same as if the key wasn't present at all.

From MP CP perspective (as it is designed now), having a config key equals empty value is perfectly fine and it doesn't mean "unconfigure" or "use default"; it translates into an empty string array.

[dmlloyd]

@njr-11 @dmlloyd I still feel you are talking about two slightly different scenarios.

So assuming I have a config foo.bar= and I invoke getValue("foo.bar", String[].class), what do I get with the changes proposed here?

I would propose that you get an empty array, as shown in the chart, but OTOH with optional values I think it's probably better to get an empty, which is inconsistent, so... 🤷‍♂️

Because Smallrye currently gives me NoSuchElementException - treating this the same as if the key wasn't present at all.

You're right.

From MP CP perspective (as it is designed now), having a config key equals empty value is perfectly fine and it doesn't mean "unconfigure" or "use default"; it translates into an empty string array.

So I guess it's a technical question about getValue. We could detect arrays and treat them specially (though this arguably sucks, and wouldn't cover cases like custom collections). We could go back to the old behavior which was to make the Converter instance decide what to do (though this is error prone). Or we could go with a behavior where empty values always translate into non-present which means NoSuchElementException for getValue and empty for getOptionalValue.

We would have to beef up the language around converters and what they are expected to do if we go with the second option, to make it clear that they should return null if they are singular or an empty collection if they are collection-oriented. This would imply that getOptionalValue on collection-oriented values would always return an Optional with a value though, otherwise we're back to the original problem.

[njr-11]

We could detect arrays and treat them specially (though this arguably sucks, and wouldn't cover cases like custom collections). We could go back to the old behavior which was to make the Converter instance decide what to do (though this is error prone). Or we could go with a behavior where empty values always translate into non-present which means NoSuchElementException for getValue and empty for getOptionalValue

Regardless of which approach is standardized here, we have a requirement for the user to be able to specify a size 0 (empty) array, the purpose of which is well summarized in your earlier comment,

I suspect what you're really trying to determine is whether the user explicitly wants no contexts to be propagated, or whether the default should be used (whatever that may be) - if I'm wrong about that, please correct me.

If there is a conflicting request asking for the ability to 'unconfigure' a property back to its defaults and there is trouble deciding between whether an empty string value should be used for that vs the size 0 (empty) array, would it be possible to solve the problem by introducing a constant to be used instead for one of the these? For example, the UNCONFIGURED_VALUE constant in org.eclipse.microprofile.config.inject.ConfigProperty which is currently available but lacks JavaDoc explaining its purpose? Then you could do,

foo.bar=org.eclipse.microprofile.config.configproperty.unconfigureddvalue

as a very natural way of specifying that you want to override a config property such that is no longer configured and thus ends up with its default value.

[dmlloyd]

We could detect arrays and treat them specially (though this arguably sucks, and wouldn't cover cases like custom collections). We could go back to the old behavior which was to make the Converter instance decide what to do (though this is error prone). Or we could go with a behavior where empty values always translate into non-present which means NoSuchElementException for getValue and empty for getOptionalValue

Regardless of which approach is standardized here, we have a requirement for the user to be able to specify a size 0 (empty) array, the purpose of which is well summarized in your earlier comment,

I suspect what you're really trying to determine is whether the user explicitly wants no contexts to be propagated, or whether the default should be used (whatever that may be) - if I'm wrong about that, please correct me.

If there are others asking for the ability to 'unconfigure' a property back to its defaults and there is trouble deciding between whether an empty string value should be used for that vs the size 0 (empty) array, would it be possible to solve the problem introducing a constant to instead be used for one of the these? For example, the UNCONFIGURED_VALUE constant in org.eclipse.microprofile.config.inject.ConfigProperty which is currently available but lacks JavaDoc explaining its purpose? Then you could do,

foo.bar=org.eclipse.microprofile.config.configproperty.unconfigureddvalue

as a very natural way of specifying that you want to override a config property such that is no longer configured and thus ends up with its default value.

The problem I'm trying to solve here is that when a property has a default value, the user can't configure it to an empty or missing value. In other words, I think the concept of a default value could be decoupled completely from the concept of a missing value.

So here's another table covering just these problem cases:

I want to	Using the existing approach	Using the proposed approach
Have a value in my config for an array	it's fine	it's fine
Have an array containing empty values	inconsistent for one-element arrays	not possible*
Have a default value for an array	it's fine	it's fine
Override an earlier value with another value	it's fine	it's fine
Override a default value with another value	it's fine	it's fine
Override a default value with an empty value	depends on the converter	it's fine
Override a default value with a non-present value	not possible	it's fine (same as empty value)
Override a specified value with an empty value	depends on the converter	it's fine
Override a specified value with a non-present value	not possible	it's fine (same as empty value)
Override a specified value with the default value	not possible*	not possible*

The * indicates that you can change the rules with custom converters.

[Emily-Jiang]

wow. This is a long conversation. I don't agree with the suggestion though. I would like to discuss this in more details on a hangout.
At the moment:
my.property=
MP Config interpret it exists with an empty value.

In the past we were required to differentiate empty value with nulls. Now, some of you suggested it is wrong to differentiate them. I am not convinced.

By the way, the spec does not state an empty string means erasing a value. It is DeltaSpike feature not MP Config. When we try to support erasing a property, we might use a different approach.

By the way, the issue was raised to clarify the behavior on how to convert to an array.
For me:

my.property=
will get an empty array.
my.property=,
will get an 1-size array with the empty as the first value.
What is the use case for the full table expansion?

[dmlloyd]

wow. This is a long conversation. I don't agree with the suggestion though. I would like to discuss this in more details on a hangout.
At the moment:
my.property=
MP Config interpret it exists with an empty value.

Correct.

In the past we were required to differentiate empty value with nulls. Now, some of you suggested it is wrong to differentiate them. I am not convinced.

To turn this around, can you come up with a valid reason to make them be different? I contend that such reasons either result in usability deficits or else are hypothetical (i.e. not real-world), or else simply don't necessitate differentiation. On the other hand there are definite concrete real-world advantages to keep them the same, as I've outlined above.

How would you decide which approach is best, if not by comparing use cases?

By the way, the spec does not state an empty string means erasing a value. It is DeltaSpike feature not MP Config. When we try to support erasing a property, we might use a different approach.

Correct, I'm proposing changing the spec in a slightly incompatible way, based on our experiences with Quarkus.

By the way, the issue was raised to clarify the behavior on how to convert to an array.
For me:

my.property=
will get an empty array.
my.property=,
will get an 1-size array with the empty as the first value.

The current implementation gives two empty values in this case AFAICT.

What is the use case for the full table expansion?

What do you mean by "full table expansion"?

[dmlloyd]

Sorry, I'm confused about what you're proposing because I cannot figure out what suggestion by @jmesnil you are referring to. Are you suggesting that the proposal can be accepted as it currently stands? Or is there some case you would add or change?

[Emily-Jiang]

What I mentioned is not in this PR. It was in one of @jmesnil comments.
I discussed this again at JCrete with Dimitris Andreadis, Ivar Grimstad and Werner Keil. The general consensus for the use case of my.users= specified in a config source is:
The retrieved value is empty string
@Inject @ConfigProperty(name="my.users") String names; -> empty string

if a different type such as int or Integer is specified, a Conversion Exception should be thrown.

I think we have discussed this long enough. I suggest to validate this PR to reflect the above.
As for the following:
@Inject @ConfigProperty(name="my.users") String[] names; -> array size 1 with empty value in the first element.
I think this will solve the original issue raised, which was to seek clarification for the above statement.
If my.users=,
@Inject @ConfigProperty(name="my.users") String[] names; -> array size 1 with empty value in the first element.

[dmlloyd]

Unfortunately this introduces quite a lot of chaos into the implementation.

Proposed rules:

• If the value is empty, it is missing
• If a list value is empty, it is missing
• Users can remove a lower-priority config value with a new value or missing

Your rules:

• If the value is empty,
  • If the value has a natural empty representation, use that (Strings for example)
  • If not, give either a conversion exception or a missing exception
• If a list contains an empty value,
  • If the value has a natural empty representation, preserve it as a blank spot in the list unless that value happens to be the last value in the list, in which case it's left off
  • If not, give either a conversion exception or a missing exception
• Users cannot cause a value present in a lower-priority source to be missing

Can you explain why these rules are better? It seems like every time we talk, you agree that the proposed rules make sense, yet you keep coming back to these rules which are more complex and yet cover fewer use cases AFAICT.

[dmlloyd]

Also you keep talking about consensus, but have you noticed that we have reached many (conflicting) consensuses? It all depends on which subset of people you have in the group, and whether they understand what is being proposed. Those opposed have largely assumed that the proposal doesn't solve some use case, whereas the use case chart above would easily have answered these questions. The way it is continually described consistently has conflated "what" (adding support for overriding values with empty, without sacrificing any known use case) with "how" (the specific treatment of optional and empty string, which changes the way some use cases are executed). So, imagine my frustration when you say a decision has been reached.

If you are expressing that you've reached a point of frustration because of the length of this process, I can only say that it's a natural result of the combination of a general lack of interest (one way or another) and confusion about what is being proposed. It is frankly necessary that both of these problems be solved in order to proceed rationally.

[OndroMih]

The distinction for 1) only makes sense if the config sources can make the distinction. And if they can do it, they either have to do the conversion themselves or they need to pass this information to upper levels. If we do the conversion without knowing whether the value is set to empty or not set at all, the non-default conversion can be done in a custom converter.

If we expect that ConfigSources do the conversion, then it's up to each ConfigSource how the conversion is done. Though we can specify what it means when a ConfigSource returns null and "". We can also specify how the default ConfigSources treat my.users= values, e.g. whether they return null or "".

If we want to cover most cases with a simple solution and allow extending it to all other cases, we should provide all the information to converters so that a custom converter can decide what to do with an empty/null value.

A possible solution:

• specify that when a ConfigSource returns null, it means the value doesn't exist. If it returns "" then the value exists but is empty. Some sources would return null even for empty values, that's up to the config source. I believe there are very few custom sources that would return "" for unspecified values so we can ignore that case.

• then each converter would either receive an empty value or null and could decide what to do with it, regardless of the default behavior we agree on.

If we go with the simpler default (an empty value leads to "missing") as described by @dmlloyd in #407 (comment), a simple converter could return empty strings instead:

String convert(String value) {
    if (value == null) {
        return null;
    } else if (value.isEmpty()) {
        return "";
    } else {
        return value;
    }
}

Alternatively, we can specify a special confg prefix which would trigger this behavior automatically even just for specific config keys. E.g. if the app specifies mp-config-empty-value-to-empy-string.my.users=true then my.users= would result in "", otherwise in null. However, I think this is too complicated to specify and should be left implementation specific. We can just provide a hint for implementations how to provide more flexibility to users in a recommended way.

[OndroMih]

So, my question is: regardless of the final default solution, is there something that prevents using a custom converter to switch to the other behavior or even customize it completely?

If not, I prefer the behavior that is simpler to specify. Here I agree with @dmlloyd (#407 (comment)) that returning null is easier to specify than returning "" and throwing exceptions for incompatible types.

[dmlloyd]

Ondro, there's a lot of confusion in this thread so let me lay it out simply. The solution you propose is very close to what I have proposed.

This proposal is really about two things: adding the ability to unset properties, and aligning on exactly one representation for empty values for all specification-provided converters. All other use cases which are supported today would remain possible and supported.

In terms of UX it is proposed to do it this way:

• Users would unset properties by setting them to the empty string
• The value Optional.empty() is used to represent all empty values, even for types which do not have a "natural" empty type.

The following additional behavioral changes are necessary:

• In arrays and lists, empty initial and middle values will be left out. Today, AFAIK only empty trailing values are left out.
• All config sources would return "" for present-but-empty and null for missing like you say.
• All spec-provided converters would return null when the input is an empty value.
• User provided converters would, as you suggest, be able to return alternative "empty" values, of course, because ultimately they have control over what they return.

I would also suggest that to ensure a uniform behavior, the converter for a given type should always be interrogated to determine what to do when a value is empty by passing it an empty string. This would give user-provided converters complete control over what they return. However many converters today will fail with an IllegalArgumentException when presented with an empty string. This complication would either have to be accepted as a compatibility change, or else implementations should capture this exception when it is thrown in response to an empty string and automatically use a null/empty value in this case.

Converters should be specified to never be given a null value for any reason.

One thing that is not yet clear is how to apply the new consistent rules to @Inject+@ConfigProperty configuration injections. A simple solution would be to require that injected values which can be empty simply should be of type Optional<X>. However this is also more impacting in terms of compatibility. It is also possible to specify that injected values should use special converters for strings, arrays, sets, and lists which yield their "native" empty representations. But the disadvantage is that it would then be impossible to automatically require a value for these cases.

[njr-11]

In terms of UX it is proposed to do it this way:

* Users would unset properties by setting them to the empty string
* The value `Optional.empty()` is used to represent all empty values, even for types which do not have a "natural" empty type.

I agree that looking at this from the perspective of user experience is a good approach to take. The part of the user experience that stands out as missing here is how the user sets properties to an empty list. I have a MicroProfile spec that instructs users to specify an empty list as the property value in MP Config when they want to override the implementation's default built-in value with empty. It is a completely different meaning from 'missing', which would mean to accept the default built-in value. In current versions of MP Config, this is possible with the empty string value. The proposal under this issue which seems to be gaining consensus involves repurposing the empty string value, but without any clear way for the user to specify that they really wanted an empty list value. I suspect the intent here is that when you want empty list, write your own converter that re-interprets the value in that way, but that is an awful user experience. The user who configures the property and the 'user' which is the code that accepts the configured value are two completely different users. A well-designed solution shouldn't require that they have prearranged knowledge of eachother's interpretation of values to make this work. Even if that approach were considered acceptable, it involves swapping away one provided function (ability to unset) to get another (set to empty list). If unset is as important as those behind this issue are claiming it to be, then those ought to be two completely distinct things, so that a user who has the ability to specify an empty list value for one property simultaneously has the ability to unset a different property of the same type.

[dmlloyd]

In terms of UX it is proposed to do it this way:

* Users would unset properties by setting them to the empty string
* The value `Optional.empty()` is used to represent all empty values, even for types which do not have a "natural" empty type.

I agree that looking at this from the perspective of user experience is a good approach to take. The part of the user experience that stands out as missing here is how the user sets properties to an empty list. I have a MicroProfile spec that instructs users to specify an empty list as the property value in MP Config when they want to override the implementation's default built-in value with empty. It is a completely different meaning from 'missing', which would mean to accept the default built-in value.

I'm going to stop you right there because this is the point where people seem to have confusion. You've made an implementation assumption that detecting a missing value as empty is how one would know to use a default value, but that is not correct. Using the config accessor API you should specify what the default value is in the case (either as a string or as the actual target object type) where the default value is something other than empty. This way you would simply read the property and use the result as-is: if it's empty, that's because the user intended it to be empty. If it's the default value, either the user didn't specify the property or else the default was used (both cases should be treated with one code path if possible). If it's some other value, then that value should be used. No branching is necessary.

In the less common event that the default value is not representable, you would have to use (and test for) a sentinel default value object instead in the implementation. But I expect this case to be less common than a simple default value.

[njr-11]

You've made an implementation assumption that detecting a missing value as empty is how one would know to use a default value, but that is not correct. Using the config accessor API you should specify what the default value is in the case (either as a string or as the actual target object type) where the default value is something other than empty.

I'm not intending to make any implementation assumptions, especially not the one stated. Not making implementation assumptions means that we don't know if the user who consumes the value will do so via the ConfigAccessor or other MP Config API methods like Config.getValue or Config.getOptionalValue.

When the user invokes config.getValue(propName, String[].class) on an empty-value property, they can only get back one result: either a NoSuchElementException overriding the lower priority config source due to the property being 'unset' in a higher priority one, or they can get back the empty array value representing the empty value that they configured. (Whatever value they get back I hope will not be implementation-specific and vary across implementations - that would be the worst experience for the user). Similarly, with config.getOptionalValue(propName, String[].class) on an empty value property, the user can only be given a single result. Is it an Optional.empty() overriding the lower priority config source due to the property being 'unset' in a higher priority one? Or is it the empty array? How can we not be trading off one for the other here?

I know this discussion has gone on for a while, and I'm torn between continuing to provide helpful - if that's even the right word - feedback on the user perspective here, or whether to just remain quiet until a solution complete with JavaDoc and everything else is finished, and then assess how it fits into our spec usage in MicroProfile.

[dmlloyd]

You've made an implementation assumption that detecting a missing value as empty is how one would know to use a default value, but that is not correct. Using the config accessor API you should specify what the default value is in the case (either as a string or as the actual target object type) where the default value is something other than empty.

I'm not intending to make any implementation assumptions, especially not the one stated. Not making implementation assumptions means that we don't know if the user who consumes the value will do so via the ConfigAccessor or other MP Config API methods like Config.getValue or Config.getOptionalValue.

Right, but those would be the wrong methods to call if you have a default value, unless you follow the (recommended) practice of using a low-priority configuration source for your default values. (The reason this is recommended is due to property expansion: most of our users seem to expect that their expressions can refer to values that have defined defaults, and get the default value if the value isn't defined, but this is only achievable using a configuration source for default values).

When the user invokes config.getValue(propName, String[].class) on an empty-value property, they can only get back one result: either a NoSuchElementException overriding the lower priority config source due to the property being 'unset' in a higher priority one, or they can get back the empty array value representing the empty value that they configured. (Whatever value they get back I hope will not be implementation-specific and vary across implementations - that would be the worst experience for the user).

Agreed that making this dependent on implementation would be a bad user experience. Users would call this method if empty is not an acceptable value.

Similarly, with config.getOptionalValue(propName, String[].class) on an empty value property, the user can only be given a single result. Is it an Optional.empty() overriding the lower priority config source due to the property being 'unset' in a higher priority one? Or is it the empty array? How can we not be trading off one for the other here?

Saying that the user must call one of these two methods is in fact assuming an implementation which cannot meet the use cases. To meet these kinds of use cases, the config accessor API becomes useful. However I also would propose adding methods to Config such as:

<T> T getValueWithDefault(String key, Class<T> type, T defaultValue);
<T> T getValueWithStringDefault(String key, Class<T> type, String defaultValue);
<T> T getValueWithDefault(String key, Converter<T> converter, T defaultValue);
<T> T getValueWithStringDefault(String key, Converter<T> converter, String defaultValue);
<T> Optional<T> getOptionalValueWithDefault(String key, Class<T> type, T defaultValue);
<T> Optional<T> getOptionalValueWithStringDefault(String key, Class<T> type, String defaultValue);
<T> Optional<T> getOptionalValueWithDefault(String key, Converter<T> converter, T defaultValue);
<T> Optional<T> getOptionalValueWithStringDefault(String key, Converter<T> converter, String defaultValue);

Note that by allowing the converter to provide a custom empty behavior, all of the getOptional* methods can be implemented using a single generic Optional converter.

I know this discussion has gone on for a while, and I'm torn between continuing to provide helpful - if that's even the right word - feedback on the user perspective here, or whether to just remain quiet until a solution complete with JavaDoc and everything else is finished, and then assess how it fits into our spec usage in MicroProfile.

It's always worth the one-time investment to get things right IMO. However I will say that I'm on vacation right now so my responses may be slow.

[dmlloyd]

This has been superceded by the issue #446 and two options #444 and #445. @jmesnil would you please close this PR?