Add a type implementing both IConfiguration and IConfigurationBuilder

[halter73]

Background and Motivation

When developing a new minimal host for ASP.NET Core (WebApplication, WebApplicationBuilder), we found that it would be useful to be able to read from configuration (e.g. appsettings.json and DOTNET_/ASPNETCORE_ environment variables) while still being able to add new configuration sources. Every time a source is added via the IConfigurationBuilder interface, the IConfiguration updates automatically and immediately.

For this reason, we've temporarily introduced a Configuration type to Microsoft.AspNetCore.Builder that does exactly this. This type belongs in Microsoft.Extensions.Configuration though.

Moving this type will be a breaking change in ASP.NET Core between preview6 and preview7, but we're okay this.

Proposed API

namespace Microsoft.Extensions.Configuration
{
+    public sealed class ConfigurationManager : IConfigurationRoot, IConfigurationBuilder, IDisposable
+    {
+        public ConfigurationManager();
+        public string? this[string key] { get; set; }
+        public IConfigurationSection GetSection(string key);
+        public void Dispose();
+    }

The members that are required to implement IConfigurationBuilder will be implemented explicitly, so members like IList<IConfigurationSource> IConfigurationBuilder.Sources don't pollute intellisense. Extension methods are generally used to add configuration sources.

Usage Examples

WebApplicationBuilder essentially already exposes this type as a public property. The problem it is trying to solve is being able to read config from appsettings.json and DOTNET_/ASPNETCORE_ while configuring the host's IServiceCollection while at the same time being able to add new configuration sources or even change the content root without having to introduce additional build stages.

    using var config = new Config();

    config.AddEnvironmentVariables(prefix: "MyCustomPrefix_");

    if (config["FileConfig"] == "enabled")
    {
        config.AddJsonFile("MyConfig.json", optional: true, reloadOnChange: true);
    } 

    string myValueFromJson = config["JsonConfigValue"];
    // ...

We have docs where we demonstrate manually building an IConfigurationBuilder, reading from the built IConfiguration, and then throwing that away to add a new config source. This is an alternative to that.

Alternative Designs

Update ConfigurationBuilder to implement IConfigurationRoot and basically become this type. It would leave the IConfigurationBuilder methods as normal methods instead of explicit interface implementations like in the proposal.

In the current implementation of Configuration in Microsoft.AspNetCore.Builder, the ChangeBasePath and ChangeFileProvider methods are internal. Consumers of WebApplicationBuilder should generally call builder.WebHost.SetContentRoot() instead. That, in turn, calls these currently-internal methods.

I was thinking an alternative might be an interface or an abstract base class we implement in ASP.NET Core. Perhaps IUpdateConfiguration: IConfigurationRoot, IConfigurationBuilder. I imagine we'd want at least one default implementation in Microsoft.Extensions.Configuration though.

All sources are now reloaded when the IConfigurationBuilder.Properties["FileProvider"] changes.

Risks

It might be confusing to developers if and when this should be used over a plain old ConfigurationBuilder. It also might not be clear if you should call ChangeBasePath or ChangeFileProvider methods instead of the SetBasePath and SetFileProvider methods extension with this new type. The answer is you should.

@maryamariyan @eerhardt @safern @tarekgh @pranavkm @davidfowl @Tratcher

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

Issue Details

---

Background and Motivation

When developing a new minimal host for ASP.NET Core (WebApplication, WebApplicationBuilder), we found that it would be useful to be able to read from configuration (appsettings.json and DOTNET_/ASPNETCORE_) environment variables in particular while still being able to add new configuration sources. Every time the configurati

For this reason, we've temporarily introduced a Configuration type to Microsoft.AspNetCore.Builder that does exactly this. This type belongs in Microsoft.Extensions.Configuration though.

Moving this type will be a breaking change in ASP.NET Core between preview4 and preview5, but we're okay this.

Proposed API

namespace Microsoft.Extensions.Configuration
{
+    public sealed class Configuration : IConfigurationRoot, IConfigurationBuilder
+    {
+        public void ChangeBasePath(string path)
+        public void ChangeFileProvider(IFileProvider fileProvider)
+    }

Usage Examples

WebApplicationBuilder essentially already exposes this type as a public property. The problem it is trying to solve is being able to read config from appsettings.json and DOTNET_/ASPNETCORE_ while configuring the host's IServiceCollection while at the same time being able to add new configuration sources or even change the content root without having to introduce additional build stages.

    var config = new Configuration();

    config.AddEnvironmentVariables(prefix: "MyCustomPrefix_");

    if (config["FileConfig"] == "enabled")
    {
        config.AddJsonFile("MyConfig.json", optional: true, reloadOnChange: true);
    } 

Alternative Designs

In the current implementation of Configuration in Microsoft.AspNetCore.Builder, the ChangeBasePath and ChangeFileProvider methods are internal. For WebApplicationBuilder, we would prefer builder.WebHost.SetContentRoot() was called instead, but that in turn call these currently-internal methods.

I was thinking an alternative might be an interface or an abstract base class we implement in ASP.NET Core. Perhaps IUpdateConfiguration: IConfigurationRoot, IConfigurationBuilder. I imagine we'd want at least one default implementation in Microsoft.Extensions.Configuration though.

Risks

It might be confusing to developers if and when this should be used over a plain old ConfigurationBuilder. It also might not be clear if you should call ChangeBasePath or ChangeFileProvider methods instead of the SetBasePath and SetFileProvider methods with this new type. The answer is you should.

Author:	halter73
Assignees:	-
Labels:	api-suggestion, area-Extensions-Configuration
Milestone:	-

[pakrym]

How does reloading work when one config source is based on another one? I.e. appconfig.json contains a KeyVault uri and we reload app config.

Example:

var config = new Configuration();
config.AddEnvironmentVariables(prefix: "MyCustomPrefix_"); 
config.AddJsonFile("MyConfig.json", optional: true, reloadOnChange: true);
config.AddKeyVault(config["kvuri");

[pakrym]

Can we turn the ConfigurationBuilder into this type? Just make the IConfiguration members lazy?

[halter73]

That's a good point. If "kvuri" comes from a reloadable source, the keyvault source wouldn't be updated when "kvuri" updates after startup.

Just make the IConfiguration members lazy?

I don't think it's possible to make the above example "just work" though. How does AddKeyVault know that it's argument came from config? If AddKeyVault is called in Program.Main, we cannot just rerun it.

Maybe we can add a helper method that let's you register a callback that reruns on reload and re-adds dependent config sources.

[pakrym]

I don't think it's possible to make the above example "just work" though. How does AddKeyVault know that it's argument came from config? If AddKeyVault is called in Program.Main, we cannot just rerun it.

Yeah, that comment was unrelated to the first one. I'm just proposing to reuse the existing type instead of adding a new one.

[davidfowl]

Can we turn the ConfigurationBuilder into this type? Just make the IConfiguration members lazy?

That's a very interesting idea. I think it would work really well actually.

[KevinCathcart]

In terms of overall value, I see the basic concept as a good idea. As proof, I offer that implementation linked to in the first post is the second implementation of this in ASP.NET Core.

However I do have real concerns with both the API as proposed here, and especially with the "reference implementation" of this type shipped by ASP.NET Core in Preview 4

For the API

I object to the new methods, they are not necessary.

First of all, SetBasePath is just a helper over SetFileProvider, and all that really does is set a property in the builder's Property dictionary.

But the implementation's Property IDictionary<,> can be built to monitor values being set just like sources list does so these are simply not needed. Since any change to these properties could result in a configuration source provider generating a configuration source with different config values, you really want to trigger rebuild on that anyway.

For the implementation

1. It would probably be better to actually implement the IConfigurationBuilder directly rather than delegate to the existing ConfigurationBuilder class. It is only 24 lines of nearly trivial non-whitespace non-comment code. Your current implementation would probably come within one or two lines of breaking-even with this change, and it can completely avoid concerns like the sources lists getting out of sync. (Of course, if this does become just enhancing the existing ConfigurationBuilder, then you get this for free!)

2. You really do want the build/rebuilds to be lazy. Building a new ConfigurationRoot after each change even if not queried between the changes is not great, since the load action of the configuration provider can potentially be expensive (e.g. network IO). Having to do that 3 times because you just added three more file sources seems silly. Being lazy, and rebuilding only when you try to fetch a configuration value or the providers property is much better.

Even better would be to also cache the providers, and only build the new sources when ones are added. However for property changes, you would need to do a full rebuild. Only problem is that that approach would prevent delegating to the existing ConfigurationRoot class. I'd suggest NOT doing this approach at first, as it will make proper ChangeToken handling rather complicated. The lazy building/rebuilding is probably good enough, unless people complain.

3. In the real implementation, GetSections and GetChildren should wrap this hybrid type. This will mean that if you are holding onto an IConfigurationSection, you will see updates from newly added sources. If implemented in the main Microsoft.Extensions.Configuration assembly, these would both be literal one liners:

public IConfigurationSection GetSection(string key) => new ConfigurationSection(this, key);
public IEnumerable<IConfigurationSection> GetChildren() => this.GetChildrenImplementation(null);

The second one is an internal static shared helper that works for any IConfigurationRoot, used to share the implementation of GetChildren between RootConfiguration and ConfigurationSection.

4. You need to implement the Reload token properly. It needs to be a separate token that triggers whenever a (lazy) rebuild is triggered, as well as whenever the delegated ConfigurationRoot's token fires. Look at how the existing ConfigurationRoot class handles this. Its token is triggered when any provider's is triggered, or when you call it's reload method. The only real difference here is that we have a single ConfigurationRoot, as opposed to multiple providers, and there are a few minor twists stemming from laziness.

5. Having the build method returning this is dangerous. It needs to return something that is thread safe because the result becomes a DI singleton, and this combined builder is not thread-safe. Returning this._configuration is also not good, since that instance could get disposed. Returning a freshly build ConfigurationRoot is the safe option.

I've posted a gist with my proposed alternative reference implementation that addresses all the above points. (It is a derivative of the one I'm criticizing. This implementation is offered under the terms of the .NET CLA, which you can verify I have signed by looking at my pull request(s) in this repo.).

Hope this helps.

[halter73]

I object to the new methods, they are not necessary.

Agreed. I need to update this proposal. @pakrym pointed out many the same things. Thanks for the gist! The reference implementation will need to be rewritten.