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

Empty objects loaded via ConfigurationBuilder.AddJsonFile() are ignored #40218

Closed
halter73 opened this issue Jul 31, 2020 · 19 comments · Fixed by #40829
Closed

Empty objects loaded via ConfigurationBuilder.AddJsonFile() are ignored #40218

halter73 opened this issue Jul 31, 2020 · 19 comments · Fixed by #40829
Labels
area-Extensions-Configuration help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@halter73
Copy link
Member

halter73 commented Jul 31, 2020

Description

Empty objects loaded via ConfigurationBuilder.AddJsonFile() are ignored.


Repro

config.json
{
    "a": "b",
    "c": {
        "d": "e"
    },
    "f": "",
    "g": null,
    "h": {},
    "i": {
        "k": {}
    } 
}
Program.cs
using System;
using Microsoft.Extensions.Configuration;

namespace EmptyConfigSection
{
    class Program
    {
        static void Main(string[] args)
        {
            var config = new ConfigurationBuilder()
                .AddJsonFile("config.json")
                .Build();

            Console.WriteLine("{");

            foreach (var child in config.GetChildren())
            {
                PrintConfigSection(child, padding: "    ");
            }

            Console.WriteLine("}");
        }

        static void PrintConfigSection(IConfigurationSection configSection, string padding)
        {
            if (configSection.Value is null)
            {
                Console.WriteLine($@"{padding}""{configSection.Key}"": {{");

                foreach (var child in configSection.GetChildren())
                {
                    PrintConfigSection(child, $"    {padding}");
                }

                Console.WriteLine($"{padding}}},");
            }
            else
            {
                Console.WriteLine($@"{padding}""{configSection.Key}"": ""{configSection.Value}"",");
            }
        }
    }
}
EmptyConfigSection.csproj
<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net5.0</TargetFramework>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.Extensions.Configuration.Json" Version="5.0.0-preview.7.20364.11" />
  </ItemGroup>

  <ItemGroup>
    <Content Include="config.json">
      <CopyToOutputDirectory>Always</CopyToOutputDirectory>
    </Content>
  </ItemGroup>

</Project>

Expected output

{
    "a": "b",
    "c": {
        "d": "e",
    },
    "f": "",
    "g": "",
    "h": {
    },
    "i": {
        "k": {
        },
    },
}

Actual output

{
    "a": "b",
    "c": {
        "d": "e",
    },
    "f": "",
    "g": "",
}

"h", "i" and "k" are not present in the actual output even though they should be.

The top-level call to IConfiguration.GetChildren() in Program.Main() should return an IEnumerable containing 6 top-level IConfigurationSections: "a", "c", "f", "g", "h" and "k". Instead it returns 4 top-level IConfiguration sections omitting both "h" and "i".

The repro program never even makes a call to GetChildren() that should return the "k" subsection, because it never sees "k"'s parent "i" in the first place.

Configuration

F:\dev> dotnet --info
.NET SDK (reflecting any global.json):
 Version:   5.0.100-preview.7.20330.3
 Commit:    eeb77e1a55

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.20161
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   F:\dev\aspnet\AspNetCore\.dotnet\sdk\5.0.100-preview.7.20330.3\

Host (useful for support):
  Version: 5.0.0-rc.1.20370.4
  Commit:  0e0e648770

I do not think this issue is specific to my configuration however.

Regression?

  • I don't think this is a regression, but I cannot say for sure.
@ghost
Copy link

ghost commented Jul 31, 2020

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Jul 31, 2020
@davidfowl
Copy link
Member

@HaoK do you remember if this was intentional?

@HaoK
Copy link
Member

HaoK commented Jul 31, 2020

I don't think this is intentional, I'd imagine this is going to depend on the behavior of the json parser code: https://github.com/dotnet/runtime/blob/master/src/libraries/Microsoft.Extensions.Configuration.Json/src/JsonConfigurationFileParser.cs

One thing to try @halter73 , compare against the old Newtonsoft provider:

https://github.com/dotnet/extensions/blob/master/src/Configuration/Config.NewtonsoftJson/src/NewtonsoftJsonConfigurationExtensions.cs#L37

@HaoK
Copy link
Member

HaoK commented Jul 31, 2020

Looking at the code: https://github.com/dotnet/runtime/blob/master/src/libraries/Microsoft.Extensions.Configuration.Json/src/JsonConfigurationFileParser.cs#L47

Doesn't look like empty objects are handled, so might just be a bug?

@HaoK
Copy link
Member

HaoK commented Jul 31, 2020

Basically maybe there needs to be a special case in there that inserts an empty child section, right now looks like empty objects are just dropped with no entry into config (maybe the old json provider did the same thing though...) as the code is similar: https://github.com/dotnet/extensions/blob/master/src/Configuration/Config.NewtonsoftJson/src/NewtonsoftJsonConfigurationFileParser.cs#L41

@HaoK
Copy link
Member

HaoK commented Jul 31, 2020

I could see how maybe this is intentional, as the only real functional difference between omitting empty sections would be for code that is looking at the hierarchy of config, as you would never get anything out of empty sections...

@HaoK
Copy link
Member

HaoK commented Jul 31, 2020

I wasn't involved in the initial designs of config, so the actual design/intent for this code was before my time

@halter73
Copy link
Member Author

I could see how maybe this is intentional, as the only real functional difference between omitting empty sections would be for code that is looking at the hierarchy of config, as you would never get anything out of empty sections...

And that's exactly why I noticed this now. See dotnet/aspnetcore#24286.

@HaoK
Copy link
Member

HaoK commented Jul 31, 2020

Yeah, seems reasonable to fix the behavior so the config hierarchy gets empty sections so you don't have to do wacky stuff like add random junk to your sections for them to exist

@maryamariyan maryamariyan added this to the 5.0.0 milestone Jul 31, 2020
@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label Jul 31, 2020
@alefranz
Copy link
Contributor

alefranz commented Aug 1, 2020

If this is up from grabs, I could try to look into it.

What is the expected behaviour?
Should it only add an empty child section when it is not already there?
If it already exists, it will not be clearing out the section from all the properties, right?

@halter73
Copy link
Member Author

halter73 commented Aug 3, 2020

What's the normal behavior when multiple sources define the same config section?

I assume we'd want to do what we normally do when multiple sources define the same section. If sections with the same path from multiple sources normally get merged, then clearing out sections because the JSON source defined those sections as empty objects seems wrong. If it's the last source wins, and a JSON file defines sections as empty objects, then clearing out those sections seems right.

I don't know if this will be up for grabs. It hasn't been labeled that way yet.

@HaoK
Copy link
Member

HaoK commented Aug 3, 2020

I believe you can think of sections as a union of key/values. So an empty section is basically a no-op, any other section that defines key/values would win in an union. But when it comes to a specific value for a key, that's where overriding comes into play, so foo.bar="baz" and foo.bar="", the last one will win, so you can 'delete' values.

@HaoK
Copy link
Member

HaoK commented Aug 3, 2020

This is just a natural result of how configuration works. Each provider can define different configuration values for a section, i.e. json defines foo.bar = "yes", xml defines foo.baz = "no". The end result is foo.bar and foo.baz show up. That's just how the config provider model is intended to work.

@halter73
Copy link
Member Author

halter73 commented Aug 3, 2020

That makes a lot of sense. The alternative would be each top-level section only being defined by one config source which doesn't seem very useful.

@alefranz
Copy link
Contributor

alefranz commented Aug 4, 2020

I agree this makes sense and it match my expectations. It is also the smaller change as it will not change the current behaviour when a section already exists, as it will only add it if missing.

I was asking as currently there is no easy way to clean up a config section, unless you add an extra json file to clear the section (setting it to null) before your actual json file the set the only properties you want. A good example would be to have a environment specific json settings override where you want to clean up any sort of logging overrides in the app specific appsettings.json. But I guess that is a different issue to discuss.

Going back on topic, if this gets added, I think it would make sense to add support to create empty sections in all configuration providers.
Currently a way to achieve something close to this behaviour that is compatible with all providers is to add a property named empty string, however then you currently need logic to exclude empty string keys if you enumerate the children. Maybe an option would be to exclude the ability to have a empty string property name and use this to syntax to express the empty block in the other providers.

e.g.
environment variable: foo__=1
cli: --foo:=1
json: { "foo": { "" : 1 } }
All these currently lead to a section foo with a child "":"1".

@davidfowl
Copy link
Member

cc @ericstj @maryamariyan

@maryamariyan maryamariyan added the help wanted [up-for-grabs] Good issue for external contributors label Aug 4, 2020
@maryamariyan
Copy link
Member

@alefranz were you still up to taking a PR up for this?

@alefranz
Copy link
Contributor

Sure, I'd like to! I've missed it was up for grabs, I guess github doesn't send notifications when labels are added.

@alefranz
Copy link
Contributor

Created PR #40829

@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Configuration help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants