-
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
Add a type implementing both IConfiguration and IConfigurationBuilder #51770
Comments
Tagging subscribers to this area: @maryamariyan, @safern Issue DetailsBackground and MotivationWhen developing a new minimal host for ASP.NET Core ( For this reason, we've temporarily introduced a Configuration type to Moving this type will be a breaking change in ASP.NET Core between preview4 and preview5, but we're okay this. Proposed APInamespace Microsoft.Extensions.Configuration
{
+ public sealed class Configuration : IConfigurationRoot, IConfigurationBuilder
+ {
+ public void ChangeBasePath(string path)
+ public void ChangeFileProvider(IFileProvider fileProvider)
+ }
Usage Examples
var config = new Configuration();
config.AddEnvironmentVariables(prefix: "MyCustomPrefix_");
if (config["FileConfig"] == "enabled")
{
config.AddJsonFile("MyConfig.json", optional: true, reloadOnChange: true);
} Alternative DesignsIn the current implementation of I was thinking an alternative might be an interface or an abstract base class we implement in ASP.NET Core. Perhaps RisksIt might be confusing to developers if and when this should be used over a plain old
|
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"); |
Can we turn the ConfigurationBuilder into this type? Just make the IConfiguration members lazy? |
That's a good point. If "kvuri" comes from a reloadable source, the keyvault source wouldn't be updated when "kvuri" updates after startup.
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. |
Yeah, that comment was unrelated to the first one. I'm just proposing to reuse the existing type instead of adding a new one. |
That's a very interesting idea. I think it would work really well actually. |
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 APII 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
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.
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.
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. |
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. |
If renaming type is possible, perhaps |
Yea, I'm not a fan of Config, plus it violates the naming guidelines 😄 |
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();
}
} |
What happens when System.Configuration is in the same project? |
One other alternate design possibility: implement namespace Microsoft.Extensions.Configuration
{
public sealed class ConfigurationRoot : IConfigurationRoot, IConfigurationBuilder, IDisposable
{
public ConfigurationRoot();
public string? this[string key] { get; set; }
public IConfigurationSection GetSection(string key);
public void Dispose();
}
} using var config = new ConfigurationRoot();
config.AddEnvironmentVariables(prefix: "MyCustomPrefix_");
if (config["FileConfig"] == "enabled")
{
config.AddJsonFile("MyConfig.json", optional: true, reloadOnChange: true);
}
string myValueFromJson = config["JsonConfigValue"];
// ... |
That feels slightly risky as it changes the behavior quite a bit and introduces some potentially undesirable side effects |
Can you elaborate on that? My thinking is |
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
andDOTNET_
/ASPNETCORE_
environment variables) while still being able to add new configuration sources. Every time a source is added via theIConfigurationBuilder
interface, theIConfiguration
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 inMicrosoft.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
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 fromappsettings.json
andDOTNET_
/ASPNETCORE_
while configuring the host'sIServiceCollection
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.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 theIConfigurationBuilder
methods as normal methods instead of explicit interface implementations like in the proposal.In the current implementation ofConfiguration
inMicrosoft.AspNetCore.Builder
, theChangeBasePath
andChangeFileProvider
methods are internal. Consumers ofWebApplicationBuilder
should generally callbuilder.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. PerhapsIUpdateConfiguration: 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 callChangeBasePath
orChangeFileProvider
methods instead of theSetBasePath
andSetFileProvider
methods extension with this new type. The answer is you should.@maryamariyan @eerhardt @safern @tarekgh @pranavkm @davidfowl @Tratcher
The text was updated successfully, but these errors were encountered: