Inconsistent behaviour between Config.getValue() and Config.getOptionalValue()

[manovotn]

I stumbled upon the different behaviour of Config.getValue() and Config.getOptionalValue() method.

When you define a configuration and give it an empty value, e.g. something like org.foo.bar= (expected value here is String[], then getValue() will (IMO correctly) return empty string, whereas getOptionalValue() gives me empty optional which I cannot retrieve.

Now, I don't really see spec mentioning this in detail, but those two methods should behave equally.

[manovotn]

So I was about to send a PR but now I noticed that there is already a test that assumes this to be incorrect.
So maybe I will wait for some input from people who know Mp Conf spec better than I do :)

[mkouba]

e.g. something like org.foo.bar= (expected value here is String[], then getValue() will (IMO correctly) return empty string

I'd say that the expected value should be an empty array. And getOptionalValue() should return an Optional holding an empty array.

The spec would deserve some clarification in this area.

[manovotn]

I meant to say that it should return an empty String[] (not just empty string).

And yes, spec seems to be lenient here.

[manovotn]

@kenfinnigan @jmesnil what are your thoughts on this?

[kenfinnigan]

Is this around determining the difference between a non set value and a set to nothing value?

It does make sense for getValue() and getOptionalValue() to behave in a similar fashion.

I'm not familiar with the spec intricacies, so not sure if this would need spec clarification

[manovotn]

Is this around determining the difference between a non set value and a set to nothing value?

It's more so about getValue() and getOptionalValue() behaviour in case you pass it "nothing value".
getOptionalValue() always treats such a scenario as if it were "not set value".

MP Concurrency is currently trying to use similar setup where it is valid to provide a "nothing value".
For instance when you define a String[] of PROPAGATED contexts via MP config, you can pass in something like org.eclipse.microprofile.example.MyBean/contextPropagator/ThreadContextConfig/propagated= which effectively means no contexts are propagated (when using given ThreadContext).

[manovotn]

Related spec pr is eclipse/microprofile-config#407
And issue - eclipse/microprofile-config#397

[jmesnil]

as @manovotn commented, I've opened a PR to clarify mp-config specification.
Rule of thumb is that a config property with an empty value must be returned as an empty "thing" (empty string or empty collection)

For optional values, an empty config property MUST return an Optional of an empty thing (e.g. an Optional of a empty array).
^ there is a bug in smallrye config where an Optional of an empty value returns Optional.empty().
That is not correct: the property does exist but its value happens to be empty. That's logically different from Optional.empty() which means the property does not exist.
Once the spec clarifies the behaviour, I'll fix it accordingly in SmallRye.

[dmlloyd]

This would be fixed by #102 and is basically the same problem as #84.

[manovotn]

@dmlloyd @kenfinnigan I don't think #102 PR solves this issue. Sorry I couldn't review earlier, was on PTO.
The problem here is that when you have a configuration of org.bar.Foo=, then getValue() [assuming String as expected type] returns you an empty String whereas getOptionalValue() gives you Optional.empty() which is logically different. And this is still the case even with the PR.

[dmlloyd]

Hmm I didn't notice that behavior. Easy enough to fix it though, one second :)

[dmlloyd]

Here's a wrinkle though: I can't find anything in the spec that clarifies empty string behavior. So it looks like there is going to be some legwork here. I'm on it though

[manovotn]

@dmlloyd yeah, I think we mentioned it in previous comments here on this issue.
There is also eclipse/microprofile-config#407
But our current behaviour is just weird in a way that you can get two varying results based on what method you choose.

[manovotn]

In the meantime, reopening this issue.

[dmlloyd]

Bad news. The TCK tests that the result of an empty string is an empty string rather than a missing value. This creates a big inconsistency in the way things work. @jmesnil and/or @kenfinnigan do you mind if I raise a spec issue? Or would you rather do so? OK I see that the issue you linked covers this, I'll follow up there.

[manovotn]

That's actually how I would expect it to work though.
Empty string is an actual value and surely doesn't equals absence of that config property

[dmlloyd]

It must though. If an empty string translates to an empty array, then it must also translate to a non-present value. If an empty string is not a non-present value, it becomes impossible to override a present value with a non-present one, and in addition, it must translate to a single-element array containing an empty string otherwise translating the empty string versus "," is inconsistent.

[dmlloyd]

In other words, if ",," is three empty strings, and "," is two empty strings, then "" must be one empty string - obviously undesirable.

[kenfinnigan]

@dmlloyd I've no issues with you raising it at spec level

[manovotn]

@dmlloyd @kenfinnigan Uf, so with this change, how are you supposed to figure out if there is a configuration key, but it has an empty value?
Because in MP context propagation you can set for instance default of what is to be propagated as shown here - https://github.com/eclipse/microprofile-context-propagation/blob/master/spec/src/main/asciidoc/mpconfig.asciidoc
Namely the ThreadContextconfig example shows: ThreadContext/propagated=
This means you want to, by default, not propagate any context. Without this settings, a non-empty value would be present. Hence we check the MP config for this value and if not present, we use defaults. However, with this latest change, I can no longer tell if there is a key with empty value. What am I missing?

BTW the aforementioned setting is OFC tested in MP CP TCK hence will cause failures in SR impl on SR config version bump.

EDIT: apparently using Config.getPropertynames() still gives you all the properties, even if they are empty. This can be used to workaround what I asked about above, but it feel like cheating it more then solving because we now basically have a situation where asking directly for a key tell you there is none whereas asking for all keys you can see it present.

[jmesnil]

This change treats an empty value as a missing property by design.

It's not clear to the user that writes the configuration what ThreadContext/propagated= actually means.

According to https://github.com/eclipse/microprofile-context-propagation/blob/7c7ac8558b2a8890aacc323e5df009a9a3f0b4f5/api/src/main/java/org/eclipse/microprofile/context/ManagedExecutor.java#L227, propagated accepts a list of String.
Can you pass an empty string to it?

With that change, you should be able to fix https://github.com/smallrye/smallrye-context-propagation/blob/2d434f7739457c133c39304faa65c0c57bdc52b9/core/src/main/java/io/smallrye/context/impl/DefaultValues.java#L44-L47 by using Config.getOptionalValue, right?

[manovotn]

@jmesnil ThreadContext/propagated= means no context is propagated, that's how spec defines it. It's actually an array of Strings you pass into it and it is perfectly fine to have that empty.

Using getOptionalValue() with the current fix doesn't solve a thing, you will get an optional that is empty - same result that you would get in case the config key wasn't present at all. The comment you are referring to is what I wrote there when I originally created this issue and thought the solution will be pretty much the exact opposite :D

[dmlloyd]

@manovotn my question is, what does a missing value signify if an empty string means "don't propagate anything"?

BTW I've found it useful to represent default values as a "bottom priority" config source, if that helps. This is an idea that might be worth formalizing in the spec.

Just adding a clarification. If you're using a missing value to determine whether to use a default, you're definitely in for some pain no matter what. By putting the defaults in as a config source, an empty value can be always safely treated the same as a missing value, and can have whatever semantic meaning is associated with such a value.

[kenfinnigan]

@manovotn @jmesnil @dmlloyd so what's the path forward here?

[manovotn]

@kenfinnigan I already went over it with Jeff on Zulip and created a MP CP issue (eclipse/microprofile-context-propagation#160) to introduce some constant basically meaning "nothing". So long as the spec change goes through, we can keep this closed and I'll try to mend this on MP CP side.

But to explain it, @dmlloyd , the thing is that every implementation has its default values for propagated/cleared/unchanged (for ThreadContext). Each of them may or may not be empty string array. Users can then override these defaults via MP config (one way to have portability over several impls). Hence the empty value being valid (non-default) settings.

[dmlloyd]

But to explain it, @dmlloyd , the thing is that every implementation has its default values for propagated/cleared/unchanged (for ThreadContext). Each of them may or may not be empty string array. Users can then override these defaults via MP config (one way to have portability over several impls). Hence the empty value being valid (non-default) settings.

As I said, using not-present to indicate a default doesn't really work (for anyone). If the default value (which could be impl-defined, for sure) was established in a bottom priority config source, then you don't have to do anything at all: not-present can still mean "no context", and if it is present, then either the user specified a value or it fell through to the default value. Does this make sense?

[mkouba]

Hm, so I might be late to the party but if I have a config property prefix with the default value foo_ and let's say we use this property to prefix(String value) -> foo_value. If I need to set the prefix to be an empty string so that prefix(String value) returns name the only solution would be to use some constant like EMPTY and handle this constant in a special way, right?

[dmlloyd]

Hm, so I might be late to the party but if I have a config property prefix with the default value foo_ and let's say we use this property to prefix(String value) -> foo_value. If I need to set the prefix to be an empty string so that prefix(String value) returns name the only solution would be to use some constant like EMPTY and handle this constant in a special way, right?

No, you don't need to do anything like that. You'd use getOptionalValue(String.class) and if it's empty then you don't have a prefix.

[jmesnil]

@mkouba you would use an optional value:

prefix = config.getOptionalValue("prefix", String.class).orElse("");

[mkouba]

So in other words I can't use the @Inject @ConfigProperty(name="prefix", defaultValue = "_foo") String myPrefix but always work with Optional...

[jmesnil]

@mkouba or you can do:

@Inject @ConfigProperty(name="prefix", defaultValue = "") 
String myPrefix

and specifies prefix=_foo in a low ordinal config source.

• if the user does nothing, _foo is used
• if the user specifies prefix= in a high ordinal config source, the property will be considered missing and the default "" will be used.
• if the user specifies prefix=_bar, _bar_ will be used

[mkouba]

Well, that's not very obvious. Also a regular user does not care about config sources at all.

[jmesnil]

"low ordinal config source" can a microprofile-config.properties in your jar (that's not seen by the user)
"high ordinal config source" can be a sys prop or an env var that the user knows how to set.

[dmlloyd]

So in other words I can't use the @Inject @ConfigProperty(name="prefix", defaultValue = "_foo") String myPrefix but always work with Optional...

Why not? The CDI integration can do the optional part for you. But you do have to know what an empty value means.

[dmlloyd]

@mkouba or you can do:

@Inject @ConfigProperty(name="prefix", defaultValue = "") 
String myPrefix

and specifies prefix=_foo in a low ordinal config source.

• if the user does nothing, _foo is used
• if the user specifies prefix= in a high ordinal config source, the property will be considered missing and the default "" will be used.
• if the user specifies prefix=_bar, _bar_ will be used

As I said, the downside to using a config source with a low priority is that all your defaults need to be known up front. With the CDI integration at least, I think the default value can vary by use site, so already this doesn't work. So a ConfigAccessor should be used here.

[mkouba]

Why not? The CDI integration can do the optional part for you. But you do have to know what an empty value means

I'm really sorry for my ignorance but I don't understand. @Inject @ConfigProperty(name="prefix", defaultValue = "_foo") -> I thought that I will currently get _foo if a user specifies prefix=
(empty = absent value) in the properties file? But I would like to get an empty string instead.

[dmlloyd]

Why not? The CDI integration can do the optional part for you. But you do have to know what an empty value means

I'm really sorry for my ignorance but I don't understand. @Inject @ConfigProperty(name="prefix", defaultValue = "_foo") -> I thought that I will currently get _foo if a user specifies prefix=
(empty = absent value) in the properties file? But I would like to get an empty string instead.

You would get the default value if the value was not specified in the properties file. You would get an empty string/missing value if the user specifies prefix=. But you would need some way to indicate that a missing value should be a string and not a no-such-element exception. For that you'd normally use Optional and map a missing value to "".

[manovotn]

I think Martin's problem is very similar to what I tried to describe.
E.g. he needs to distinguish between a missing key (for which he uses some defined default) and an existing key with empty value (here you do not use default and instead use the empty value).

And in this case using Config.getOptional("prefix", String.class).orElse("??") doesn't cut it because within the orElse() bit you actually don't know whether to use default or empty string as you cannot detect presence of a key with empty value. And even if you had lower-order config source with default, then you would never be able to set empty string as you would always find that default config instead.

[mkouba]

But you would need some way to indicate that a missing value should be a string and not a no-such-element exception. For that you'd normally use Optional and map a missing value to "".

If so then I see no other way then to inject Optional instead of String.

[dmlloyd]

I think Martin's problem is very similar to what I tried to describe.
E.g. he needs to distinguish between a missing key (for which he uses some defined default) and an existing key with empty value (here you do not use default and instead use the empty value).

It already does this though. You pass the default you want into the config accessor. If the value is not specified, you get the default back. If the value is specified as empty, you get an empty value.

[manovotn]

But then you get to a point where for one configuration, say, foo=:

• Config.getValue("foo", String.class) - you blow up with NoSuchElementException
• Config.getOptional("foo", String.class) - gives you empty Optional
• config.access("foo", String.class).withStringDefault("bar").getValue() - you actually give me back empty String?

That doesn't sounds consistent.

[dmlloyd]

But then you get to a point where for one configuration, say, foo=:

• Config.getValue("foo", String.class) - you blow up with NoSuchElementException
• Config.getOptional("foo", String.class) - gives you empty Optional
• config.access("foo", String.class).withStringDefault("bar").getValue() - you actually give me back empty String?

That doesn't sounds consistent.

No, under the present proposal, in your third case you'd get NoSuchElementException unless you used getOptionalValue() so it's consistent.

[manovotn]

Uff, we are running in circles here. Lemme reformulate. Assuming a config file with key foo and empty value (foo=) and an expected output type of String[].
What code do I need to use in order to get back an empty string array - String[]{}?

[dmlloyd]

You have the following options, as it is currently specified:

    // there's no default value
    String[] array = config.getOptionalValue("my.key", String[].class).orElse(EMPTY_ARRAY);
    // there's a default value
    String[] array = config.access("my.key", String[].class).withStringDefault("foo,bar").getOptionalValue().orElse(EMPTY_ARRAY);

I think it still might be worth introducing a shortcut for the second one, like this:

    String[] array = config.getOptionalValue("my.key", String[].class, "foo,bar");

But in this case I don't know if the default should normally be String or T, or if we'd want two methods.