Skip to content

Treat empty string value from chained configuration as present - #131480

Open
rosebyte wants to merge 1 commit into
dotnet:mainfrom
rosebyte:rosebyte-fix-chained-config-empty-values
Open

Treat empty string value from chained configuration as present#131480
rosebyte wants to merge 1 commit into
dotnet:mainfrom
rosebyte:rosebyte-fix-chained-config-empty-values

Conversation

@rosebyte

Copy link
Copy Markdown
Member

Fixes #65594

Summary

ChainedConfigurationProvider.TryGet used !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 plain ConfigurationBuilder given the same data.

Continuation of the .NET 10 work

.NET 10 converged the interpretations of "" and null in #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:

  • the JSON provider stores a real null rather than converting it to "";
  • an empty JSON array moved off null and onto "";
  • the binder treats null as a value to bind rather than a value to skip;
  • ConfigurationSection.TryGetValue was 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 null is an absence.

ChainedConfigurationProvider was 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 IConfiguration exposes 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 commit ConfigurationRoot and ConfigurationManager to 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 IConfiguration is 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_NullValueIsNotContributed pins 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-change label, and a docs issue to sit alongside the .NET 10 article.

Copilot AI review requested due to automatic review settings July 28, 2026 16:52
@azure-pipelines

Copy link
Copy Markdown
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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.TryGet to return true when the underlying configuration indexer returns a non-null value (including empty string).
  • Add tests covering empty-string shadowing behavior and parity with directly-added in-memory sources, plus a test pinning existing null behavior.

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

  • TryGet may assign null to the out parameter on failure; using out string value here can trigger nullability warnings. Use out 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-nullable string can cause nullability mismatch warnings (and makes the wrapper incorrect for missing keys). Make the indexer string? 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));
Comment on lines +288 to +290
var inner = new ConfigurationBuilder()
.AddInMemoryCollection(new Dictionary<string, string> { { "Key", null }, { "Section:Key", null } })
.Build();
@tarekgh tarekgh added this to the 11.0.0 milestone Jul 28, 2026
@tarekgh tarekgh added the breaking-change Issue or PR that represents a breaking API or functional change over a previous release. label Jul 28, 2026
@dotnet-policy-service dotnet-policy-service Bot added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Jul 28, 2026
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

  1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
  2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

@tarekgh

tarekgh commented Jul 28, 2026

Copy link
Copy Markdown
Member

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

@tarekgh

tarekgh commented Jul 28, 2026

Copy link
Copy Markdown
Member

One thing worth deciding before merge: the PR says Fixes #65594, which will auto-close that issue on merge. But #65594 is specifically about the null case (an inner provider returning true with value == null being swallowed by the chained provider). This PR intentionally addresses only the empty-string case and deliberately leaves null as-is, which is a reasonable call given the IConfiguration indexer cannot distinguish a null-valued key from a missing key.

Since the core scenario in #65594 (respecting null) remains unaddressed, auto-closing it would lose track of that gap. Suggest either:

Either way is fine, just want to make sure the null limitation stays tracked rather than being closed out silently.

@tarekgh

tarekgh commented Jul 28, 2026

Copy link
Copy Markdown
Member

Building on the Fixes #65594 note above: the null case (an inner provider that returns true with value == null) is the actual subject of that issue, and it can be handled for the in-box configuration types instead of being dropped.

_config[key] goes through the ConfigurationRoot indexer, which returns null both when the key is absent and when it is present with a null value, so the two are indistinguishable. But InternalConfigurationRootExtensions.TryGetConfiguration runs the same reverse provider scan and returns the bool instead of collapsing to null, so it can report true + null. This is not a new contract: ConfigurationSection.TryGetValue already uses _root.TryGetConfiguration(...) for exactly this purpose (added in the .NET 10 null work this PR cites).

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
};

TryGetConfiguration is internal and in the same assembly, and ConfigurationSection.TryGetValue is public, so both are callable. This behaves identically to the current change for non-null and empty values; the only difference is that a wrapped root/manager/section with a present-null value now correctly returns true + null, matching the base provider and a directly added source. The residual is limited to custom IConfiguration implementations, which expose only the indexer and genuinely cannot distinguish present-null from absent.

Points to address if we take this:

  1. Behavior for the in-box types should respect null rather than drop it, so ChainedConfiguration_NullValueIsNotContributed needs to be updated: null is now contributed for the root/manager/section kinds, and the "not contributed" assertion stays only for the custom IConfiguration kind.
  2. The breaking-change doc should cover both cases: chained configuration no longer treats empty as missing, and no longer treats a present null from an in-box configuration as missing.
  3. Add test coverage for the null-shadowing case across ConfigurationRoot, ConfigurationManager, and ConfigurationSection, mirroring the existing empty-string theories, so the new null behavior is pinned the same way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-Extensions-Configuration breaking-change Issue or PR that represents a breaking API or functional change over a previous release. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

3 participants