-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Comments
Tagging subscribers to this area: @maryamariyan |
@HaoK do you remember if this was intentional? |
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: |
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? |
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 |
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... |
I wasn't involved in the initial designs of config, so the actual design/intent for this code was before my time |
And that's exactly why I noticed this now. See dotnet/aspnetcore#24286. |
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 |
If this is up from grabs, I could try to look into it. What is the expected behaviour? |
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. |
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. |
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. |
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. |
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. e.g. |
@alefranz were you still up to taking a PR up for this? |
Sure, I'd like to! I've missed it was up for grabs, I guess github doesn't send notifications when labels are added. |
Created PR #40829 |
Description
Empty objects loaded via ConfigurationBuilder.AddJsonFile() are ignored.
Repro
config.json
Program.cs
EmptyConfigSection.csproj
Expected output
Actual output
"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
I do not think this issue is specific to my configuration however.
Regression?
The text was updated successfully, but these errors were encountered: