Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IConfiguration binding behave unexpectedly when binding empty strings from nested configurations #62532

Open
FynZ opened this issue Dec 8, 2021 · 4 comments

Comments

@FynZ
Copy link

FynZ commented Dec 8, 2021

Describe the bug

Binding configuration with empty string as value ("") will result in null if the section it binds to comes from a nested configuration.

To Reproduce

// code

using Microsoft.AspNetCore;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Hosting;

// we build the configuration upfront to configure stuff (logging, ..)
var configuration = new ConfigurationBuilder()
    .AddJsonFile("appsettings.NotIncluedByDefault.json")
    .AddEnvironmentVariables()
    .AddCommandLine(args)
    .Build();

var host = WebHost.CreateDefaultBuilder(args)
    .UseStartup<Startup>()
    .UseUrls(configuration["Urls"])
    .UseConfiguration(configuration)
    // we add the previously built configuration manually
    .ConfigureAppConfiguration(builder => builder.AddConfiguration(configuration));

await host.Build().RunAsync();

public class Startup
{
    private readonly IConfiguration _configuration;

    public Startup(IConfiguration configuration)
    {
        _configuration = configuration;
    }

    /// <summary>
    ///     This method gets called by the runtime. Use this method to add services to the container.
    /// </summary>
    /// <param name="services">Collection of service descriptors.</param>
    public void ConfigureServices(IServiceCollection services)
    {
        // This configuration binds from the main settings files
        var ok = new WorkingConfiguration();
        _configuration.GetSection("BindingOk").Bind(ok);

        // This configuration binds from a settings files added "manually"
        var faulted = new FaultedConfiguration();
        _configuration.GetSection("BindingNotOk").Bind(faulted);

        services.AddSingleton(ok);
        services.AddSingleton(faulted);
    }

    /// <summary>
    ///     Application configuration.
    /// </summary>
    /// <param name="app">The application builder.</param>
    public void Configure(IApplicationBuilder app)
    {
    }
}

public class WorkingConfiguration
{
    public string ThisBindingWillBindToString { get; set; }
}

public class FaultedConfiguration
{
    public string ThisBindingWillBindToNull { get; set; }
}

// appsettings.json
{
  "Logging": {
    "LogLevel": {
      "Default": "Information",
      "Microsoft.Hosting.Lifetime": "Information"
    }
  },
  "BindingOk": {
    "ThisBindingWillBindToString": ""
  }
}

// appsettings.NotIncluedByDefault.json
{
  "BindingNotOk": {
    "ThisBindingWillBindToNull": "" 
  }
}

Further technical details

  • ASP.NET Core version: 6.0.100
  • The IDE (VS / VS Code/ VS4Mac) you're running on, and its version: VisualStudioProfessional 2022 17.0.2, also observed in Rider from a colleague (dont know the version) and in production (base image is mcr.microsoft.com/dotnet/aspnet:6.0)
  • Include the output of dotnet --info:
dotnet --info Output
.NET SDK (reflecting any global.json):
 Version:   6.0.100
 Commit:    9e8b04bbff

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.19043
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\6.0.100\

Host (useful for support):
  Version: 6.0.0
  Commit:  4822e3c3aa

.NET SDKs installed:
  5.0.403 [C:\Program Files\dotnet\sdk]
  6.0.100 [C:\Program Files\dotnet\sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.App 3.1.21 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 5.0.12 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 6.0.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 3.1.21 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.12 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.WindowsDesktop.App 3.1.21 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 5.0.12 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 6.0.0 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

To install additional .NET runtimes or SDKs:
  https://aka.ms/dotnet-download
@pranavkm pranavkm transferred this issue from dotnet/aspnetcore Dec 8, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-Extensions-Configuration untriaged New issue has not been triaged by the area owner labels Dec 8, 2021
@ghost
Copy link

ghost commented Dec 8, 2021

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

Issue Details

Describe the bug

Binding configuration with empty string as value ("") will result in null if the section it binds to comes from a nested configuration.

To Reproduce

// code

using Microsoft.AspNetCore;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Hosting;

// we build the configuration upfront to configure stuff (logging, ..)
var configuration = new ConfigurationBuilder()
    .AddJsonFile("appsettings.NotIncluedByDefault.json")
    .AddEnvironmentVariables()
    .AddCommandLine(args)
    .Build();

var host = WebHost.CreateDefaultBuilder(args)
    .UseStartup<Startup>()
    .UseUrls(configuration["Urls"])
    .UseConfiguration(configuration)
    // we add the previously built configuration manually
    .ConfigureAppConfiguration(builder => builder.AddConfiguration(configuration));

await host.Build().RunAsync();

public class Startup
{
    private readonly IConfiguration _configuration;

    public Startup(IConfiguration configuration)
    {
        _configuration = configuration;
    }

    /// <summary>
    ///     This method gets called by the runtime. Use this method to add services to the container.
    /// </summary>
    /// <param name="services">Collection of service descriptors.</param>
    public void ConfigureServices(IServiceCollection services)
    {
        // This configuration binds from the main settings files
        var ok = new WorkingConfiguration();
        _configuration.GetSection("BindingOk").Bind(ok);

        // This configuration binds from a settings files added "manually"
        var faulted = new FaultedConfiguration();
        _configuration.GetSection("BindingNotOk").Bind(faulted);

        services.AddSingleton(ok);
        services.AddSingleton(faulted);
    }

    /// <summary>
    ///     Application configuration.
    /// </summary>
    /// <param name="app">The application builder.</param>
    public void Configure(IApplicationBuilder app)
    {
    }
}

public class WorkingConfiguration
{
    public string ThisBindingWillBindToString { get; set; }
}

public class FaultedConfiguration
{
    public string ThisBindingWillBindToNull { get; set; }
}

// appsettings.json
{
  "Logging": {
    "LogLevel": {
      "Default": "Information",
      "Microsoft.Hosting.Lifetime": "Information"
    }
  },
  "BindingOk": {
    "ThisBindingWillBindToString": ""
  }
}

// appsettings.NotIncluedByDefault.json
{
  "BindingNotOk": {
    "ThisBindingWillBindToNull": "" 
  }
}

Further technical details

  • ASP.NET Core version: 6.0.100
  • The IDE (VS / VS Code/ VS4Mac) you're running on, and its version: VisualStudioProfessional 2022 17.0.2, also observed in Rider from a colleague (dont know the version) and in production (base image is mcr.microsoft.com/dotnet/aspnet:6.0)
  • Include the output of dotnet --info:
dotnet --info Output
.NET SDK (reflecting any global.json):
 Version:   6.0.100
 Commit:    9e8b04bbff

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.19043
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\6.0.100\

Host (useful for support):
  Version: 6.0.0
  Commit:  4822e3c3aa

.NET SDKs installed:
  5.0.403 [C:\Program Files\dotnet\sdk]
  6.0.100 [C:\Program Files\dotnet\sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.App 3.1.21 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 5.0.12 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 6.0.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 3.1.21 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.12 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.WindowsDesktop.App 3.1.21 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 5.0.12 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 6.0.0 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

To install additional .NET runtimes or SDKs:
  https://aka.ms/dotnet-download
Author: FynZ
Assignees: -
Labels:

untriaged, area-Extensions-Configuration

Milestone: -

@maryamariyan maryamariyan added the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Jan 11, 2022
@maryamariyan maryamariyan added this to the 7.0.0 milestone Jan 11, 2022
@maryamariyan maryamariyan removed untriaged New issue has not been triaged by the area owner needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration labels Jan 11, 2022
@maryamariyan maryamariyan modified the milestones: 7.0.0, Future Jan 11, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Mar 6, 2022
@halter73
Copy link
Member

halter73 commented Mar 17, 2022

Here's a simpler repro:

test.json

{
  "NullIfChained": "" 
}

Program.cs

var config1 = new ConfigurationBuilder().AddJsonFile("test.json").Build();
var config2 = new ConfigurationBuilder().AddConfiguration(config1).Build();

Console.WriteLine($"config1[\"NullIfChained\"] = \"{config1["NullIfChained"] ?? "(null)"}\"");
Console.WriteLine($"config2[\"NullIfChained\"] = \"{config2["NullIfChained"] ?? "(null)"}\"");

Output

config1["NullIfChained"] = ""
config2["NullIfChained"] = "(null)"

This appears to be caused by this logic in ChainedConfigurationProvider:

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

Most ConfigurationProviders will just look for the presence of a string in the Dictionary<string, string?> Data and return true even if the string? is null as long as an entry exists in the dictionary.

public virtual bool TryGet(string key, out string? value)
=> Data.TryGetValue(key, out value);

ChainedConfigurationProvider doesn't have access to a dictionary, just the nested IConfiguration and it assumes an empty string is equivalent to null. I'm not sure why. This seems wrong, so I'm very tempted to change this. Unfortunately, this could be pretty breaking because it would stop Configuration and ConfigurationManager from falling back to the next config source if a chained IConfiguration returns an empty string like it does today.

@halter73 halter73 removed the in-pr There is an active PR which will close this issue when it is merged label Mar 17, 2022
@eerhardt
Copy link
Member

ChainedConfigurationProvider doesn't have access to a dictionary, just the nested IConfiguration and it assumes an empty string is equivalent to null. I'm not sure why.

See also #65594

@maryamariyan
Copy link
Member

If we decided it is an OK breaking change, it would make sense to fix as long as the behavior between Configuration and ConfigurationManager stays consistent.

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

Successfully merging a pull request may close this issue.

4 participants