Revert #388 pending further discussion

[dmlloyd]

Revert #388. This methodology of injecting null introduces inconsistencies with the base API:

• It becomes impossible to implement injectable default values with a config source
• It becomes impossible to support a default value with a nullable property
• It introduces another problem case with respect to empty values
• It introduces a problem where injection produces a different result that cannot be replicated on the base API

We need to solve this problem another way. Optional is the preferred mechanism, but if we must support nullability on injection, then we should have a separate annotation or annotation property which indicates that the value is optional and there should be an implicit .orElse(null).

[Emily-Jiang]

I don't understand what you meant. Please provide an example. By the way, Open Liberty implemented this without any issues.

[Emily-Jiang]

-1 on reverting this!

[dmlloyd]

I've explained clearly and concisely. Let me pose a question: if someone wanted to implement default values using a configuration source, what value would they use for null?

[dmlloyd]

By the way, Open Liberty implemented this without any issues.

This is a nonsense argument. If there was a problem implementing it, I would have said so. The problem has nothing to do with implementation. The problem is as I explained: it cuts off solutions to things we already know need to be solved.

If we are going to implement property expansion, for example, then every default value has to be consistent (#475) and representable within a configuration source. Adding special sentinel values which only apply to CDI causes a layering problem (CDI layers above the base API). What happens when you get such a default value, represented within a config source, from the base API? You get something weird or unspecified.

This change was not well-thought-out. Please carefully re-read my comments. I believe I've explained clearly the issues.

[dmlloyd]

I don't understand what you meant.

Please read again more carefully! I believe my phrasing is fairly exact.

[struberg]

-1 on the revert as well

[struberg]

I've explained clearly and concisely. Let me pose a question: if someone wanted to implement default values using a configuration source, what value would they use for null?

ConfigSources are not meant to provide default values. As simple as that. That's like defining the database should provide an own logic to serve values if a column entry is null.

[dmlloyd]

I've explained clearly and concisely. Let me pose a question: if someone wanted to implement default values using a configuration source, what value would they use for null?

ConfigSources are not meant to provide default values. As simple as that. That's like defining the database should provide an own logic to serve values if a column entry is null.

I do not think this comparison makes any sense; if you're going to draw an analogy then please explain it in detail. Let's say you implement a config source using a database (bringing this analogy as close as possible to the actual case). You would not use the database for the default values; they would be a distinct config source with a lower priority.

We in fact do use config sources today to provide default values in this way; in fact, it is necessary to robustly support property expansion (#405) because otherwise expressions cannot reference properties with default values with any expectation that the expansion will work correctly (and we learned almost immediately that this is something they will expect). It works well in practice also. I believe we discussed this on a call a while back, and you indicated that you understand this and I believe you agreed that this is in fact the best way to solve the problem of property expansion with default values.

To me it looks like the implementation choice of using a sentinel default value is being used to justify the design decision. The requirement (by my understanding) is that the user requested a way for a configuration property to be optional without using Optional. There is nothing about that requirement that indicates that using defaultValue for this purpose is necessary.

Given that the chosen solution is not necessary to solve the initial requirement, and that the chosen solution has clear problems, it should be reverted and solved another way.

[radcortez]

Original issue here: #365

I didn't want to raise any issue at first, since I thought this has already been discussed and accepted.

I do find the use of a sentinel value in default property a little weird, especially because it is a String value and it forces you to look into the API to know about it, or else you will never know it is there. Apart for any other considerations, if we are revisiting this, I would prefer to add an additional field to ConfigProperty like nullable to represent this. I believe this will be clear in terms of API and also address @dmlloyd concerns.

[dmlloyd]

A weakness of the original approach can be shown quite simply: how, with the original approach, does the user specify that a property may be nullable, but by default it is not null? Answer: they cannot.

[Emily-Jiang]

This is a nonsense argument.

@dmlloyd I found the above comment is hard to take. Is it ok to discuss the issue politely?

[emattheis]

Personally, I would like to avoid the whole issue of indicating nullability at the injection point by simply making that the default behavior. The current behavior assumes that the configuration a CDI bean relies on will be stable at CDI initialization time and thus it is appropriate to fail initialization by default when a property is missing. In reality, bean instances will be constructed depending on their scopes and the presence of a relied upon property cannot be guaranteed by a config check when the container is initialized.

When a property is missing, we should simply inject a null value unless a default is provided at the injection point.

Further, if we already assume that an empty string is the same as a null value, we can remove the existing NOT_DEFINED sentinel string and just use an empty string as the default for defaultValue. Guarding against the presence of such sentinel strings in an actual config source is not someone anyone should have to deal with.

[dmlloyd]

This is a nonsense argument.

@dmlloyd I found the above comment is hard to take. Is it ok to discuss the issue politely?

I'm sorry, it wasn't meant in a personal way. I should have chosen a better way to communicate this idea.

My point is that the argument is not logically cohesive. It is necessary for a specification change to be implementable, but it is self-evident that the implementability of a specification change is not sufficient to demonstrate its correctness or validity. I find such arguments to be disingenuous and also detrimental to our task.

[dmlloyd]

Personally, I would like to avoid the whole issue of indicating nullability at the injection point by simply making that the default behavior. The current behavior assumes that the configuration a CDI bean relies on will be stable at CDI initialization time and thus it is appropriate to fail initialization by default when a property is missing. In reality, bean instances will be constructed depending on their scopes and the presence of a relied upon property cannot be guaranteed by a config check when the container is initialized.

This is true only when viewed through the lens of config mutability - a controversial topic indeed.

When a property is missing, we should simply inject a null value unless a default is provided at the injection point.

This would cause some difficulties for those who wish to indicate that a given property is required to have a value, would it not?

Further, if we already assume that an empty string is the same as a null value, we can remove the existing NOT_DEFINED sentinel string and just use an empty string as the default for defaultValue. Guarding against the presence of such sentinel strings in an actual config source is not someone anyone should have to deal with.

I agree with this point. But I do think that it is necessary to be able to indicate whether an injected property is expected/required to have a non-empty value.

[emattheis]

This is true only when viewed through the lens of config mutability - a controversial topic indeed.

Given that we support system properties, which are inherently mutable, this seems like a moot point.

Regardless, it also assumes that the config used to retrieve the injected value is itself created and available at CDI instantiation time. This precludes the use of enlisting CDI beans themselves as config sources or converters as proposed in #125.

This would cause some difficulties for those who wish to indicate that a given property is required to have a value, would it not?

But I do think that it is necessary to be able to indicate whether an injected property is expected/required to have a non-empty value.

Yeah, that's fair. In my use cases I always have a default value or allow null, but for those who want a non-null guarantee I like @radcortez 's proposal to use a new field - e.g. nullable = false.

[smillidge]

Given this is currently specified behaviour you wish to remove why don't you raise an issue first where you properly explain the issue before you implement a solution?

[kenfinnigan]

Though it's currently specified in master, I don't believe the change has been part of a release to date, other than 1.4 RCs.

From a quick look, could this be a change that should've been removed when the ConfigAccessor pieces were removed, as it appears to have been added as part of the same large change?

[emattheis]

How about we re-open #365? @rmannibucau 's issue is still valid - it would be nice to be able to explicitly inject null instead of failing at CDI instantiation time.

[smillidge]

It's a long time since a config release. I thought it must be released given the 2018 on the original PR.
I'm for reopening #365.

[dmlloyd]

+1 on reopening #365.

[dmlloyd]

I've reopened #365, and at least one feasible alternative solution has been proposed. Can this be merged?

[radcortez]

+1

[GedMarc]

+1 Nullity in configuration sources is mandatory, they are very different from db, or business logic requirements

[dmlloyd]

#365 (comment) - we must merge this because the original change was merged without consensus. Let's get this merged, and then we can continue the discussion about whether we need two different ways to specify optionality for CDI config properties within #365.

[radcortez]

+1

[Emily-Jiang]

we must merge this because the original change was merged without consensus.

@dmlloyd I disagree with the above statement. The PR was merged ages ago and was approved by @OndroMih and Romain!
The correct reason for reverting is that the current group found some potential issues with this and no better approach has been identified in a timely manner.

[Emily-Jiang]

After some further discussion, there is potential issue with the previous design but no better solution was found in a timely manner. Hence, the consensus is to revert the original support and put the issue back to backlog for further discussion.

[struberg]

A weakness of the original approach can be shown quite simply: how, with the original approach, does the user specify that a property may be nullable, but by default it is not null? Answer: they cannot.

It is actually not a question of the config subsystem at all whether a value is optional or mandatory. This is solely up to the application. And it might even be mandatory for one app but not for the other.

Very early in the design discussion we also talked about metadata for config and dismissed it as too complex.

[dmlloyd]

A weakness of the original approach can be shown quite simply: how, with the original approach, does the user specify that a property may be nullable, but by default it is not null? Answer: they cannot.

It is actually not a question of the config subsystem at all whether a value is optional or mandatory. This is solely up to the application. And it might even be mandatory for one app but not for the other.

Very early in the design discussion we also talked about metadata for config and dismissed it as too complex.

The existing implementation already provides two mechanisms for the application to specify whether a property should be required or optional. The first mechanism is on Config itself by way of getValue and getOptionalValue. The second mechanism is indirect by way of the CDI integration, where an application developer can type a property as Optional, thus requesting that the configuration system make the property optional.

In neither case is the user using or providing metadata about the configuration property; rather, they're providing metadata (if you want to call it that) about the actual usage of that property in a specific context.

I'm not arguing for the config system to start doing anything that it was not already doing before; quite the opposite in fact.

[svenhaag]

Combining CDI and Optional doesn't make sense to me, as, at least in our case, in most cases, we are dealing with passivation capable beans (Serializable). I now went the route of javax.enterprise.inject.Specializes the ConfigProducer in combination with a custom annotation like ConfigPropertyNullable. The producer then return null instead of NoSuchElementException, if the annotation is present.