Configuration data allows nullable values, but ChainedConfigurationProvider.TryGet doesn't work correctly with them

[eerhardt]

See the conversation here: dotnet/core#7211 (comment).

IConfigurationProvider.TryGet(string key, out string? value); is nullable annotated as

runtime/src/libraries/Microsoft.Extensions.Configuration.Abstractions/ref/Microsoft.Extensions.Configuration.Abstractions.cs

Line 66 in cf6dbde

bool TryGet(string key, out string? value);

This means that a ConfigurationProvider can have a null value for a key. So it would return true and the value == null.

However, ChainedConfigurationProvider doesn't seem to support this:

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ChainedConfigurationProvider.cs

Lines 39 to 42 in cf6dbde

	public bool TryGet(string key, out string? value)
	{
	value = _config[key];
	return !string.IsNullOrEmpty(value);

If the inner configuration returned null, ChainedConfigurationProvider would return false, even though the it is possible the inner configuration provider returned true and value == null.

This feels like an issue with our API design here. Can null be a valid configuration value or not? If it can, should ChainedConfigurationProvider respect it?

cc @maxkoshevoi @halter73 @HaoK

[ghost]

Tagging subscribers to this area: @dotnet/area-extensions-configuration
See info in area-owners.md if you want to be subscribed.

Issue Details

---

See the conversation here: dotnet/core#7211 (comment).

IConfigurationProvider.TryGet(string key, out string? value); is nullable annotated as

runtime/src/libraries/Microsoft.Extensions.Configuration.Abstractions/ref/Microsoft.Extensions.Configuration.Abstractions.cs

Line 66 in cf6dbde

bool TryGet(string key, out string? value);

This means that a ConfigurationProvider can have a null value for a key. So it would return true and the value == null.

However, ChainedConfigurationProvider doesn't seem to support this:

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ChainedConfigurationProvider.cs

Lines 39 to 42 in cf6dbde

	public bool TryGet(string key, out string? value)
	{
	value = _config[key];
	return !string.IsNullOrEmpty(value);

If the inner configuration returned null, ChainedConfigurationProvider would return false, even though the it is possible the inner configuration provider returned true and value == null.

This feels like an issue with our API design here. Can null be a valid configuration value or not? If it can, should ChainedConfigurationProvider respect it?

cc @maxkoshevoi @halter73 @HaoK

Author:	eerhardt
Assignees:	-
Labels:	area-Extensions-Configuration
Milestone:	7.0.0

[maxkoshevoi]

Found the reason of why out string? value wasn't annotated with [NotNullWhen(true)]: #57414 (comment)

[halter73]

I think the right behavior is for TryGet to return false if _config[key] is null but return true if its "". I'm not how much code out there today relies on falling back from chained config sources that return "". Hopefully not a lot.

[cschulznethaus]

I just ran into this behavior of the ChainedConfigurationProvider.

We have a configuration system that get its values from a database, but all values can be overwritten per environment or secrets. Environment and secrets come from the ChainedConfigurationProvider, while the database values have their own IConfigurationProvider implementation.

I wanted to overwrite one value with "" in the environment, but that did not happen due to the string.IsNullOrEmpty(value)check of theChainedConfigurationProvider`.

I agree with @halter73 that "" should return true, but we also had an internal discussion that this would probably not be fixed in the near future as it could harm existing code. 🙄

[jamescrosswell]

Just adding a vote here.

The current design results in a discrepancy between how Web Host and Generic Host read configuration data from appsettings.json files (presumably Generic Host uses ChainedConfigurationProvider). Web Host supports/returns empty strings verbatim as valid configuration values whereas Generic host interprets these as null.

This breaks the Sentry SDK when using Generic Host (which allows you to set the URL of your sentry server to string.Empty to disable the SDK):

• Cannot pass empty string as Dsn from appsettings.json when using generic host builder getsentry/sentry-dotnet#3875

Arguably the current behavioural inconsistencies are a bug then.