Null must be injectable

[rmannibucau]

@Inject
@ConfigProperty(name="...")
String value;

It must be allowed to inject that and get null, a workaround is to use an optional but it is not recommanded (even the opposite) to use optionals as fields so this is not a solution. Injecting null is fine from all point of views and compatible with mainstream config solution so it should probably make its path in mp-config asap.

[Emily-Jiang]

Also, for the record, I don't view this thread as overly contentious and I understand the problem space much better having participated. If my comments seem contentious to other participants, I do apologize, and thanks for everyone for sticking with it!

@emattheis no worries. I have not seen anything being contentious. Everyone brings different perspective, which is why collaboration matters. Sometimes, it seems take a long time to get the resolution. I think it worthes it as we covered many angles. Config is a center piece and it is very crucial that we get it right.

[Emily-Jiang]

We can just specify that the default is optional=false but it will be implicitly set to true if the injection target is an optional type.

There is no need to introduce optional=false, as the absence of defaultValue means the property is not optional. Otherwise, it is optional.

[rmannibucau]

@Emily-Jiang do you remove the validation then and consider defaultValue=unconfigured implies null, or do you keep NULL_VALUE? Not sure i got your last message right.

[ljnelson]

In case this is helpful, it would seem to me that CDI-validation-time of configuration injection points is effectively impossible, since it is (apparently?) deliberately (currently) unspecified whether ConfigSources may come and go at runtime. So maybe focus on the null-versus-Optional problem and accept that validation is problematic at best, impossible at worst?

[rmannibucau]

Hmm, i think it is specified by the api: configsources are constant for a config instance - we still dont have a way to add/remove it even if a custom source can facade others. However it is also specified that their properties can change and that config is dynamically recalling sources when needed - no cache. Combined with cdi which can recreate bean instances - including appscope ones - it means all is dynamic so validation only makes sense if the app creates a "business" instance at startup without reload. In such a case, the validation is likely done at that time. Concretely the presence only validation is rarely a good validation (url, query, bounds for ints etc...) so it is revalidated anyway by the runtime. All that to say that you are right on the validation point which should be dropped. Now if it is removed the question is: does the required constraint moves to the runtime? This issue was about to not let it move and let passthrough the "unset" value (default value of the type). I guess we can maybe "open our mind" to other specs and assumes bean validation would be used in config instances so we drop any validation from here?

[dmlloyd]

I think exploring integration with bean validation would be a good idea for 2.0.

[radcortez]

Bean Validation is something that should be explored overall in MP, not only with Config, but with other specs, for instances OpenAPI / Bean Validation. I see no reason why we need to duplicate BVal annotations and OpenAPI annotations to declare validations in our openapi document.

[rmannibucau]

Side note: bena validation being already cdi integrated it already works ;)

[emattheis]

+1 for potentially adopting bean validation as the mechanism, but we still have to decide if injecting to a non-optional type implies @NotNull.

I'm still a little unclear on whether the motivation of raising this issue was to avoid validation or to actually support null injection.

I think if null injection is to be supported, then we must also allow Config::getValue to return null instead of throwing an exception.

Per @dmlloyd 's notes above, we don't want to start an Optional vs null flame war, but we should be clear about MicroProfile Config's stance. The current position implied by the Config API and injection facility, if not formally stated anywhere, is that we're on team Optional.

My personal opinion is that low-level APIs like Config should match the lowest common denominator of the platform. Like it or not, Java has null. Config should too. Having convenient accessors to avoid excessive Optional.ofNullable() boilerplate for those who prefer Optional, seems fine, but forcing everyone who prefers null to unwrap a throwaway Optional seems wasteful.

[rmannibucau]

The original issue was just to enable an unconfigured value to end up as null in the injection point. This is why NULL_VALUE was doing the job. But reworking the getValue() to get a ValueRequest { key, defaultString, canBeNull, overridenConverters, ... } makes sense too to me and would enable to clean up the CDI integration based on a "core" with the same features.

[dmlloyd]

+1 for potentially adopting bean validation as the mechanism, but we still have to decide if injecting to a non-optional type implies @NotNull.

I'm still a little unclear on whether the motivation of raising this issue was to avoid validation or to actually support null injection.

I think if null injection is to be supported, then we must also allow Config::getValue to return null instead of throwing an exception.

Per @dmlloyd 's notes above, we don't want to start an Optional vs null flame war, but we should be clear about MicroProfile Config's stance. The current position implied by the Config API and injection facility, if not formally stated anywhere, is that we're on team Optional.

+1, this is clearly the stance of the API.

My personal opinion is that low-level APIs like Config should match the lowest common denominator of the platform. Like it or not, Java has null. Config should too. Having convenient accessors to avoid excessive Optional.ofNullable() boilerplate for those who prefer Optional, seems fine, but forcing everyone who prefers null to unwrap a throwaway Optional seems wasteful.

Changing a method that previously never returned null to begin returning null is a pretty nasty compatibility breaker because it will cause previously-working code to start throwing NPEs. Given the nature of this breakage I don't think it's OK to make such a change, even in a 2.0 release. So if we do decide to have a variant of getValue which can return null, it should be a third variant, beyond getValue and getOptionalValue.

[dmlloyd]

BTW my personal opinion is to stick with "team Optional". In the end it really doesn't make that big a difference and it's nice to stay consistent. I'm no fan of Optional but I can honestly say that it hasn't caused a single problem outside the context of CDI. But I think maybe the CDI problems are more indicative of a systemic issue, which also has other indications (such as the necessity for things like this in order to support injecting basic types - does it really make sense, or should we be injecting configuration object which in turn have properties managed by MP config?).

[rmannibucau]

@dmlloyd as a side note, please note this is NOT a breaking change because before the deployment was invalid so the configs are there for all working apps, it will just enable new configs to be null.

[dmlloyd]

It is breaking in cases where failing deployment validation is the desired behavior (this would be the definition of "required configuration": such configuration is required and the application should not start up if it is missing).

Today any configuration that is not Optional is required. And before we start up again with the default value argument, adding a default value doesn't necessarily make a property optional, it just means there is a value by default, which is a necessary interpretation to clean up the mess that is empty value handling, and again I direct all argument on this point to #446.

[rmannibucau]

Hmm, not sure I see your point about:

It is breaking in cases where failing deployment validation is the desired behavior

Means it is breaking broken cases? Sounds quite acceptable to me. Concretely there is not real case like that in real life otherwise you could have never deployed your application so this can't create regressions.

That said it brings another argument to remove any validation from the spec: the moment it is validated is more complex than just the startup or instantiation, a whole bunch of code is validated lazily or just not validated in applications due to the libraries layers and condition it encounters. I just met it about a HTTP client, JDBC pool and LDap client configurations.

[radcortez]

I can see the argument of a breaking change.

I've seen teams that use this to validate that deployments have everything set in their containers, to force DevOps or admins to set the correct values (you may agree or not with the use case).

We need to weight things like these before making the decision.

[dmlloyd]

Hmm, not sure I see your point about:

It is breaking in cases where failing deployment validation is the desired behavior

Means it is breaking broken cases? Sounds quite acceptable to me.

You seem to be directly saying that you think that requiring a configuration property is broken behavior. Is that correct?

Concretely there is not real case like that in real life otherwise you could have never deployed your application so this can't create regressions.

Given the config may in some cases be external to the application, I believe this is a real possibility, and starting up the application without a required configuration property (leading to possibly undefined behavior) is demonstrably worse than failing the deployment.

That said it brings another argument to remove any validation from the spec: the moment it is validated is more complex than just the startup or instantiation, a whole bunch of code is validated lazily or just not validated in applications due to the libraries layers and condition it encounters. I just met it about a HTTP client, JDBC pool and LDap client configurations.

OK but surely you must agree that removing all validation from the specification is clearly a bigger discussion? Essentially you're positing use cases which are substantially different from those which have been posited before. Those use cases may be valid and even important but we can't derail every discussion for incremental improvement because of them, otherwise the spec is essentially dead.

[rmannibucau]

You seem to be directly saying that you think that requiring a configuration property is broken behavior. Is that correct?

No, that today it is indeed correctly configured so that it will not introduce a breaking change to relax this. The feature by itself makes sense and can already be covered by any validation interceptor.

About if the discussion is bigger or not, I'm not sure but it sounds like a good exit door for all this thread to me. No issue on my side if it requests more discussion, PoC or tests on your side. I'm just trying to make MP matching real life use cases without having to use workarounds.

[dmlloyd]

About if the discussion is bigger or not, I'm not sure but it sounds like a good exit door for all this thread to me. No issue on my side if it requests more discussion, PoC or tests on your side. I'm just trying to make MP matching real life use cases without having to use workarounds.

I think we need to collect these use cases, for 2.0 if possible, so I expect this will resurface again soon.

[emattheis]

If the ability to inject null, to skirt validation or otherwise, is no longer a concern, we should just close this as "works as designed".

Otherwise, I think the non-breaking way forward is a new config method to retrieve potentially null values:

String foo = getNullableValue("foo", String.class);

and an equivalent injection mechanism:

@Inject
@Nullable
@ConfigProperty(name="foo")
String foo;

In contrast to adding a property to the existing annotation, introducing a new annotation makes it clear that nullability applies only to the injection site, instead of potentially confusing the issue of whether or not to apply a default value when retrieving the config value.

[rmannibucau]

Hmm, and then we will need default value handling (to use converter chain) in Config so we'll add getValueWithConverters and getNullableValueWithConverters etc? This is why i proposed a getValue(ValueRequest) method.

I don't know if @Nullable is a good name since some tools like findbugs use that simple name to detect things - and they are generally broken with IoC code - but I agree splitting in multiple annotation is more promishing than putting it all in a single one.

[dmlloyd]

This discussion seems to have run out of steam, and there are so many comments and directions of the discussion that it is no longer useful for the issue to remain open.

If we want to introduce any ideas from this issue, please either open a new issue labeled use case or else link to an existing open issue with that label.