Clarify when defaultValue is used on ConfigProperty

[Emily-Jiang]

Clarify when defaultValue is used
@ConfigProperty(name="my.property", defaultValue="123") int myProp;
In the javadoc of ConfigProperty, it has the following:
The default value if the configured property value does not exist.

What if in a config source, the following config property is defined.
my.property=abc.

Normally you will get IllegalArgumentException if the converter throws that. However, what if a converter did not throw an exception, it returns null. Will defaultValue or null be assigned to the variable?

• Option 1: defaultValue is not used and null -> myProp. Add a tck to verify this.
  Pros: honour the converter. In this way, converter has an option to assign null.

• Option 2: defaultValue is used. Javadoc needs to be updated to say: if converter return null, this value will be used.
  Pros: non null is returned.
  Cons: behaviour change. Not possible to return a null. Could be confusing if a property is defined with a value and the defaultValue is still used.

Thoughts?

[Joseph-Cass]

I agree this needs a little clarification.

Personally, I prefer option 1. Although it is worth noting that the con from option 2 can be used as a counter-argument for option 1: "It could be confusing if a property is resolved to null (with the Converter) and the defaultValue is not used."

[jbee]

Given a default value often is put in to make sure that there is a workable fallback I'd think that there are cases where one would want the default to be used. On the contrary discussions in the past have revealed that different vendors or application developers use different styles. Some use fail fast, other rather have a fallback behaviour. I feel there is no right answer. One of the groups will find the behaviour not to their liking. The reason I personally prefer option 2 is that I would not want converters to be able to alter the notion of present or not present. I think we have tried to define it along these lines in the past and I would consider it more consistent with that if the default would kick in.

[radcortez]

I also prefer option 1, for the sole reason that you may not even notice that you are using the default because the conversion failed.

[Emily-Jiang]

I also prefer option 1 for the same reason @radcortez mentioned besides there is no behaviour change. I'll create a TCK to try the option 1 first to see whether we hit any issues.

[Emily-Jiang]

Based on the discussion on the PR #611 , it is a bit confusing with Option 1. Let me create another PR for Option 2 to see whether it settles better.

[Joseph-Cass]

It seems that the PR that was merged in the end (#618) was neither of these options- Is that correct?

It looks like the defaultValue is ignored if a Converter returns null and a NoSuchElementException should be thrown (if the property is "required"). So for clarification, in our example, should "requiring" my.property throw a NoSuchElementException?

[Emily-Jiang]

@Joseph-Cass correct. Based on the discussion and experiment, the conclusion is to throw NoSuchElementException when converter returns null. DefaultValue is only a lowest config source.

[Joseph-Cass]

Based on the outcomes of this discussion, should the following throw a DeploymentExeption when empty.string= exists in a ConfigSource?

@Inject
@ConfigProperty(name = "empty.string", defaultValue = "a default value")
String emptyString;

If so, should there be a TCK for this? It seems like it could be a relatively common use-case. The closest I can find is EmptyValuesTest.java, however emptyValuesBean also defines a defaultValue of "" which under some implementations could be the underlying cause of the DeploymentException being tested for.

[Emily-Jiang]

Here is the TCK test for this.

[Joseph-Cass]

Yes, that's the one I linked to. However, the test is different to my example. The test also supplies an invalid defaultValue, of value "" meaning any implementations which "fallback" to a defaultValue rather than use it as "only a lowest config source" would pass the test.

From my understanding of the conclusion of this conversation, in EmptyValuesTest.java the stringValue should never even try to use the defaultValue. Is that correct?

[radcortez]

Based on the outcomes of this discussion, should the following throw a DeploymentExeption when empty.string= exists in a ConfigSource?

Yes, because the idea was to provide a way to empty values in higher ordinals sources.

[Emily-Jiang]

@Joseph-Cass Please open a new issue if further discussion is needed as this issue was closed and released. Any further discussions might confuse people.