Treat empty string value from chained configuration as present - #131480
Treat empty string value from chained configuration as present#131480rosebyte wants to merge 1 commit into
Conversation
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Updates ChainedConfigurationProvider.TryGet so that an empty string ("") from the chained configuration is treated as a present value (only null remains “not found”). This brings chained configuration behavior in line with the provider model where “found” is independent of “non-empty”.
Changes:
- Change
ChainedConfigurationProvider.TryGetto returntruewhen the underlying configuration indexer returns a non-nullvalue (including empty string). - Add tests covering empty-string shadowing behavior and parity with directly-added in-memory sources, plus a test pinning existing
nullbehavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/libraries/Microsoft.Extensions.Configuration/src/ChainedConfigurationProvider.cs | Adjusts TryGet presence semantics from “non-empty” to “non-null” and updates its XML doc. |
| src/libraries/Microsoft.Extensions.Configuration/tests/ChainedConfigurationProviderTests.cs | Adds coverage for empty-string presence/shadowing and null non-contribution scenarios. |
Comments suppressed due to low confidence (2)
src/libraries/Microsoft.Extensions.Configuration/tests/ChainedConfigurationProviderTests.cs:313
TryGetmay assignnullto theoutparameter on failure; usingout string valuehere can trigger nullability warnings. Useout string?to match the contract.
Assert.False(provider.TryGet("MissingKey", out string value));
Assert.Null(value);
src/libraries/Microsoft.Extensions.Configuration/tests/ChainedConfigurationProviderTests.cs:396
IConfiguration.this[string]is nullable (string?). Implementing it as non-nullablestringcan cause nullability mismatch warnings (and makes the wrapper incorrect for missing keys). Make the indexerstring?to match the interface contract.
public string this[string key]
{
get => _inner[key];
set => _inner[key] = value;
}
| /// <param name="key">The key.</param> | ||
| /// <param name="value">When this method returns, contains the value.</param> | ||
| /// <returns><see langword="true"/> if a value for the specified key was found, otherwise <see langword="false"/>.</returns> | ||
| /// <returns><see langword="true"/> if the chained configuration has a value for the specified key, otherwise <see langword="false"/>.</returns> |
| .AddInMemoryCollection(new Dictionary<string, string> { { "Key", value } }) | ||
| .Build()); | ||
|
|
||
| Assert.True(provider.TryGet("Key", out string actual)); |
| var inner = new ConfigurationBuilder() | ||
| .AddInMemoryCollection(new Dictionary<string, string> { { "Key", null }, { "Section:Key", null } }) | ||
| .Build(); |
|
Added When you commit this breaking change:
Tagging @dotnet/compat for awareness of the breaking change. |
|
@rosebyte I have marked this PR with the breaking change label. It is better to have breaking change doc for that and can be linked to the .NET 10 breaking change doc. |
|
One thing worth deciding before merge: the PR says Since the core scenario in #65594 (respecting
Either way is fine, just want to make sure the null limitation stays tracked rather than being closed out silently. |
|
Building on the
Suggested implementation: public bool TryGet(string key, out string? value) => _config switch
{
IConfigurationRoot root => root.TryGetConfiguration(key, out value),
ConfigurationSection section => section.TryGetValue(key, out value),
_ => (value = _config[key]) is not null
};
Points to address if we take this:
|
Fixes #65594
Summary
ChainedConfigurationProvider.TryGetused!string.IsNullOrEmpty(value), so a key present in the chained configuration with an empty value was reported as not found. It therefore failed to shadow values from providers registered earlier in the outer builder, and a chained configuration disagreed with a plainConfigurationBuildergiven the same data.Continuation of the .NET 10 work
.NET 10 converged the interpretations of
""andnullin #116677 ("Support Null configuration", fixing #116700 and #36510). Before that pass the two were used more or less interchangeably to mean "nothing here". That PR separated them:nullrather than converting it to"";nulland onto"";nullas a value to bind rather than a value to skip;ConfigurationSection.TryGetValuewas added so callers can tell an absent key from an explicit null.The rule that fell out of it is that an empty string is a value and
nullis an absence.ChainedConfigurationProviderwas not part of that pass and still conflates the two. This PR applies the same rule to it.Null is deliberately left alone
The null rows still disagree, and this PR does not change that.
The wrapped
IConfigurationexposes only an indexer, which cannot tell a null value apart from a missing key. Closing the gap would mean downcasting to the in-box configuration types and walking their providers, which turns an implementation detail into a contract: it would commitConfigurationRootandConfigurationManagerto never growing lookup logic of their own. That is too high a price for the remaining sliver.There is also a reading on which the current behaviour is right. A chained
IConfigurationis a merged unit rather than a single provider, and a unit reports the absence of a value as null, so "null means nothing to contribute" is defensible on its own terms.ChainedConfiguration_NullValueIsNotContributedpins it so the limit reads as deliberate rather than as an oversight.Breaking change
This is a behavioural breaking change, and it is the counterpart to the .NET 10 one documented as Null values preserved in configuration (dotnet/docs#46890). That change stopped providers treating null as missing; this one stops chained configuration treating empty as missing.
Requesting the
breaking-changelabel, and a docs issue to sit alongside the .NET 10 article.