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]

Do you mean you defined
myConfig=

in config source, you will get empty string instead of null. Do you want to get null instead? If not, can you elaborate your use case?

[rmannibucau]

I'm not sure about myConfig=[nothing] case but more than myConfig doesn't exists and cdi bean uses the snippet I put in the first message, by default I expect it to be null instead of fail (required by tck today). I don't want (also can't in several cases) inject Optional cause it is not serializable and Provider is slow compared to have null.

Something like @ConfigProperty(name="...", nullAllowed=true) would be nice.

[struberg]

or optional=true

[rmannibucau]

sounds weird with Optional class no: "optional=true will set null and you can inject Optional which will not be null" :s? But agree a better phrasing can be found.

[Emily-Jiang]

-1 on optional=true, as it contradicts with the contract of "the presence of defaultValue" means optional.

How about to add a new constant on ConfigProperty?

String NULL_VALUE="org.eclipse.microprofile.config.configproperty.nullvalue";

Usage:

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

[rmannibucau]

I had that in mind as well so +1

[dmlloyd]

The currently proposed solution does not seem correct. One alternative discussed in #512 was to add a separate nullable annotation property indicating that the injected field should be treated as if it were optional with an orElse(null) qualification. This property would not be allowed to be true if the injection target is not nullable (that is, it is primitive).

[radcortez]

+1

[rmannibucau]

Hmm, why isnt it correct? Having a flag or a flag is the same. One is likely more elegant than the other but both are strictly equivalent.
Now, for the primitive case, you must consider the magic string NULL_VALUE means "the configured value is null but passthrough" so the output takes its default.
Overall point is that duplicating this notion just adds noise to the spec.
Note, however, this is already outside the spec - like geronimo proxy support - since in practise using a default of NULL_VALUE is only valid in cdi injections so not primitives so we are safe anyway.

[dmlloyd]

Hmm, why isnt it correct? Having a flag or a flag is the same. One is likely more elegant than the other but both are strictly equivalent.

They are not strictly equivalent. How would you then specify the default value for a nullable property? If default values are implemented using a low priority configuration source (as we do in Quarkus), how do you represent this special behavior?

Bear in mind that we already have a recommended approach for optional values, and that is Optional.

Now, for the primitive case, you must consider the magic string NULL_VALUE means "the configured value is null but passthrough" so the output takes its default.

That is a bit of a stretch, I'd say. Anyway I believe I already proposed in #476 that primitive types should already implicitly default to their Java default values, which I think is probably a cleaner approach. It doesn't make these properties optional, but it does remove boilerplate.

Note, however, this is already outside the spec - like geronimo proxy support - since in practise using a default of NULL_VALUE is only valid in cdi injections so not primitives so we are safe anyway.

I'm not sure what you mean by this.

[rmannibucau]

How would you then specify the default value for a nullable property?

This is the whole point, you can't be both at the same time. If you have a default then you are not nullable so at the end it is just about requesting to accept null as a default. The NULL_VALUE (mp-config 1.4) is exactly there for this case.

Bear in mind that we already have a recommended approach for optional values, and that is Optional.

As explained before it goes partially against CDI and fully against java recommendation so it can't last IMHO.

The last part which was likely unclear is that the coercing of defaults for primitive is not needed in the spec, there is no way to inject a primitive with CDI so no need to spec how to do it. The only existing case it is useful is the case the impl supports configuration proxies (see an example here https://github.com/apache/geronimo-config/blob/trunk/impl/src/test/java/org/apache/geronimo/config/test/internal/ProxyTest.java#L104) but it is out of the spec for now.

[dmlloyd]

How would you then specify the default value for a nullable property?

This is the whole point, you can't be both at the same time. If you have a default then you are not nullable so at the end it is just about requesting to accept null as a default. The NULL_VALUE (mp-config 1.4) is exactly there for this case.

This is factually incorrect. Users can have a property which is optional/emptiable but also has a default value. If they want to explicitly clear the property, they may do so. At least, this works fine on SmallRye/Quarkus.

Bear in mind that we already have a recommended approach for optional values, and that is Optional.

As explained before it goes partially against CDI and fully against java recommendation so it can't last IMHO.

This should be tackled in a separate issue, since Optional is used extensively by this API for this purpose. Using this issue as a proxy for that discussion is counter-productive.

The last part which was likely unclear is that the coercing of defaults for primitive is not needed in the spec, there is no way to inject a primitive with CDI so no need to spec how to do it. The only existing case it is useful is the case the impl supports configuration proxies (see an example here https://github.com/apache/geronimo-config/blob/trunk/impl/src/test/java/org/apache/geronimo/config/test/internal/ProxyTest.java#L104) but it is out of the spec for now.

OK.

[rmannibucau]

Users can have a property which is optional/emptiable but also has a default value.

No, the meaning of defaultValue is "use that if not set". There is no way to set null anywhere in the configuration (getValue in a ConfigSource does not return an optional so no way to know it is null or not set at all).
There is also no reason to set a default value if you support null.
So at the end both cases are exactly the same: what value do you use if you get null from config sources.

What you are speaking about is another topic: how to specify null when there is a default value. In other words it is how to set explicitly null in/from the config source.
This is another topic for the "core" API, not the CDI integration which uses NULL_VALUE as a marker in the @ConfigProperty annotation - as unconfigured marker which is not supported in the getValue/getOptionalValue methods.
I don't think we want to use the constants of @ConfigProperty in Config to use them as marker + in the core API this distinction is not needed because you can set a default using getOptionalValue().orElse as easily as having a marker.
So at the end, the injection API is covered now, and the standalone one too IMHO.

Using this issue as a proxy for that discussion is counter-productive.

Not sure I followed this one. Factually Optional fields do not work in passivable CDI beans so it prevents to use the spec and it is where this issue is coming from.

[emattheis]

There is no way to set null anywhere in the configuration (getValue in a ConfigSource does not return an optional so no way to know it is null or not set at all).

This issue has been raised separately and needs to be resolved, IMHO. Personally, I would like to see the empty string "" used to indicate the presence of a property with an absent value in a config source, as distinct from a null which means the property is not present and other sources should be considered.

The last part which was likely unclear is that the coercing of defaults for primitive is not needed in the spec, there is no way to inject a primitive with CDI so no need to spec how to do it.

Primitives types are valid beans and injectable per CDI. CDI also specifies that null gets coerced to a primitive's default at injection time. It's true they are not proxyable, but that should not be a concern for MicroProfile Config.

[emattheis]

The debate about how to indicate nullability in the annotation is largely driving by limitations in the expressiveness of annotation structures. I think the guiding principle here is that we need to avoid further complicating the interpretation of value strings by overloading the use of the defaultValue annotation field to convey addition meaning.

It can certainly be argued that this is ambiguous at a glance:

@ConfigProperty(name = "foo", nullable = true, defaultValue = "bar")
String foo;

But so is this:

@ConfigProperty(name = "foo", defaultValue = "bar")
Optional<String> foo;

It is easy enough to specify that defaultValue must be empty when nullable is true, and CDI gives us a convenient way to detect that condition at bootstrap time and provide meaningful feedback.

[emattheis]

Maybe the real issue here is the defaultValue support itself. Config doesn't have

<T> T getValue(String propertyName, String defaultValue, Class<T> propertyType);

So why should we provide such convenience during injection? As recommended in the javadocs, a developer is almost certainly better off simple bundling the default values in META-INF/microprofile-config.properties.

[dmlloyd]

The debate about how to indicate nullability in the annotation is largely driving by limitations in the expressiveness of annotation structures. I think the guiding principle here is that we need to avoid further complicating the interpretation of value strings by overloading the use of the defaultValue annotation field to convey addition meaning.

+1 here

It can certainly be argued that this is ambiguous at a glance:

@ConfigProperty(name = "foo", nullable = true, defaultValue = "bar")
String foo;

But so is this:

@ConfigProperty(name = "foo", defaultValue = "bar")
Optional<String> foo;

It is easy enough to specify that defaultValue must be empty when nullable is true, and CDI gives us a convenient way to detect that condition at bootstrap time and provide meaningful feedback.

We support this case. Having a default is not the same as making a property be required (if you assume the reverse, all kinds of ambiguous situations appear, as previously discussed).

Maybe the real issue here is the defaultValue support itself. Config doesn't have

<T> T getValue(String propertyName, String defaultValue, Class<T> propertyType);

This has been proposed. But using a config source is more future-proof (calling back to the property expansion discussion).

[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.