Revisit configuration API (design flaws)

[artganify]

Alright, here's a big one. I played around with the current state of the configuration API a little while (both the Microsoft.Extensions.Configuration.Abstractions as well as the Microsoft.Extensions.Configuration implementations) and I noticed various inconsistencies, design flaws, not really obvious behaviors etc. etc. A lot of the things I'm going to mention here have already been discussed or submitted by other issues, but I also got some things that haven't been mentioned yet that I think they are worth addressing.

Please note that this is not a demand to go back to the drawing board again, additionally some of this highly depends on whether you consider the Microsoft.Extensions.Configuration package just a framework (but then refrain to use it as a basis for other stuff within ASP.NET 5 and related frameworks) or a full replacement of the old System.Configuration on which many frameworks and platforms heavily rely on nowadays (and thus need a solid replacement!).

Persisting configuration

As already mentioned in #283, #385, and #404, the current IConfiguration contract allows to set configuration values, however they are only persisted within memory and lost whenever a refresh occurs. There is an issue #386 which demands support for persisting configurations, but it has been put on the backlog while I think this is a big problem that really needs more attention, mostly because the contract implies a 'feature' that isn't supported at all. This can be solved in two ways:

• Add support for persisting configuration: While this sounds like an obvious feature, it isn't as easily implemented at all, mainly because of 1) the possibility to have multiple configuration providers and b) some configuration providers simply can't have support for persisting configuration (e.g. command line)
• Drop persistence support completely: Simply remove the setters on the IConfiguration contract.

Multiple configuration providers

I really can't think of any good reason why the configuration API supports having multiple configuration providers at all! At first I thought it was pretty clever because that way, one can e.g. have a default configuration (stored in a file) which easily can be overwritten by e.g. the command line. However since the individiual configuration providers are stored within a simple list in Microsoft.Extensions.Configuration, there's really no way to tell in what order the configuration providers are being looped through when retrieving a configuration value. One either requires looking at the source of his IConfigurationBuilder or look at a documentation.

var configurationBuilder = new ConfigurationBuilder()
                                                .AddJsonFile("file1.json")
                                                .AddJsonFile("file2.json")

... can result in something completely different than...

var configurationBuilder = new ConfigurationBuilder()
                                                .AddJsonFile("file2.json")
                                                .AddJsonFile("file1.json")

Value in IConfigurationSection

What's the deal with this? I can see it's being used by some extension method to create a copy of the current configuration section for returning it as IEnumerable, but other than that it doesn't make sense at all. What does the Value of a configuration section contain? A serialized representation of the section? A list of keys for configuration values and subsections? This again highly depends on the implementation and isn't obvious at all.

Change token

As far as I understand this, it's for notifying a client about a change in configuration, which again implies that there is a fixed set of configuration data that can actually change, e.g. from a file system etc. But maybe there will once be configuration providers that fetch their values from a remote server - how is this supposed to be working then, if the server isn't able to e.g. push change notifications or the client constantly pulling the server in order to watch for changes?

This again implies a feature that simply can't be provided by all providers, which goes against the idea to provide abstractions 'for everything'.

Other things to consider (optional)

• Configuration is not only a thing to read for machines. It's also a thing for humans to edit. Because of that, many configuration frameworks support 'comments'. I know this wouldn't work with many providers, such as the command line or JSON, but would you also like to use your kitchen as a configuration provider? Not every tool is suitable for each task.
• Command line configuration provider: Does that one even make sense? The command line is so different from an application configuration, I think it would make way more sense to provide it as an extension for Microsoft.Extensions.Options (just like the options can be built from the configuration). This would also fix a lot of problems like e.g. storing configuraiton.
• Key/value configuration? It's 2016! Why not make a neat little framework which uses strongly-typed configuration models? Then you can utilize cool stuff like configuration model annotations, configuration converters etc. The configuration binder in Microsoft.Extensions.Configuration.Binder will probably be the most used feature anyway.

I hope this issue doesn't sound like ranting or anything. I'm mostly confused because it's still not clear to me if the Microsoft.Extensions.Configuration namespace is going to be a replacement for System.Configuration (which is not perfect, but has everything a decent configuration framework needs - except e.g. the support for different providers). But even if not, many frameworks will be built on this in the future which means it should transparent, easy to use and obvious with its intentions and contracts.

[herecydev]

I hope this issue doesn't sound like ranting or anything

It totally does; would be much better having multiple issues that can be addresses individually.

Regarding the persisting configuration, I'm not sure I agree with it being a good idea. I think having a mutable configuration is, without caution, a dangerous precedent. Often, I'm relying on immutable deployments and versioning of code to guarantee I'm running what I expect. Having code potentially manipulate the runtime behavior scares me.

I really can't think of any good reason why the configuration API supports having multiple configuration providers at all

• Transitioning large programs from an older configuration (xml) to a more desired one (Json) without going for a big bang approach.
• Multiple files to have a more "Single responsibility" approach

What's the deal with this?

Doesn't it return a section of the configuration? Useful for when you have a deeply nested section, preventing `Azure:Storage:Table:TableName' instead you can do 'TableName' from the section.

[artganify]

Hello @herecydev.

Yeah, I initially thought about creating multiple issues for the individual parts of the current one, however the reason I ended up creating a single issue is simply because the main 'issue' is that the Microsoft.Extensions.Configuration.Abstractions namespace contains contracts with inconsistencies, not self-evident methods etc. The individual 'issues' I mentioned are just examples, or results of, in my opinion, design flaws.

Regarding the persisting configuration, I'm not sure I agree with it being a good idea.

I don't like it either. And I never mentioned that I'm favoring this. That would be just one possible way to solve the problem of having a contract which allows to set configuration values (as it's currently the case). As mentioned the current contracts allow to set configuration values (in memory) however with no way to actually persist them. That's confusing and inconsistent. Either removing the setters completely and/or allowing configuration to be persisted in general.

x Transitioning large programs from an older configuration (xml) to a more desired one (Json) without going for a big bang approach.
x Multiple files to have a more "Single responsibility" approach

Those points doesn't sound like reasons to me. More 'features to further cause problems'. I see why one might separate configuration values into mutliple files, but in the end you have just a single configuration root with a list of key/value pairs. If for some reason you end up having identical keys in multiple configuration sources, it entirely depends on the configuration builder to handle this and you can't really tell from the outside which configuration values make it into the list of your configuration root and in what order.

Doesn't it return a section of the configuration? Useful for when you have a deeply nested section, preventing `Azure:Storage:Table:TableName' instead you can do 'TableName' from the section.

It does. But it does it as a 'string'. A string can honestly be anything and if you have a structured concept like a 'section' (which is a collection of individual parts), then representing those as string is really a bad idea because without documentation, it's not really obvious what the contents and the structure of the value are going to be. This field is somehow only used for returning an IEnumerable of the current configuration section anyway, so what's the point of it?

[herecydev]

I see why one might separate configuration values into mutliple files, but in the end you have just a single configuration root with a list of key/value pairs.

Doesn't mean you shouldn't split it out logically. It's no different to splitting classes into single responsibility, purpose built entities. Makes it more flexible, easier to understand and maintain.

If for some reason you end up having identical keys in multiple configuration sources, it entirely depends on the configuration builder to handle this and you can't really tell from the outside which configuration values make it into the list of your configuration root and in what order.

It's simply the value that was registered last. Could raise a separate issue to add that to the XML docs in order to clarify the behavior. Though I would imagine, it's a discourageable practice anyway. Other than that, throw some sort of DuplicateKeyException. Perhaps this is better when configuration is pulled from a persisted source (Json/Xml) but not so much fun when you are intentionally overwriting it through a command line for example. Or return an IEnumerable<string> with all the values, but this dilutes the simplicity for every other scenario.

It does. But it does it as a 'string'.

It's uses are limited when you can do Get<AzureTableOptions>("Azure:Storage:Table"). @divega What are the intended uses of this?

[Mertsch]

I would like to add to the discussion the fact that Config and DI setup works basically the same way by using a "builder", but they have been named two completely different things

Consider the following code to see the differences

IConfigurationBuilder builder = new ConfigurationBuilder();
this.ConfigureConfiguration(builder);
this.configuration = builder.Build();

ServiceCollection serviceCollection = new ServiceCollection();
this.ConfigureServices(serviceCollection);
this.services = serviceCollection.BuildServiceProvider();

Functionality is basically the same, naming is completely different between the two.

What do you guys say? Should I open an issue for that?

[herecydev]

Functionality is basically the same

I could see why you think that, they are both providers of something. However, that's where it kind of ends. Configuration is responsible for managing IConfigurationProviders and provides a sort of giant list. The IServiceProvider manages all the ServiceDescriptors but has more responsibilities around lifetimes of objects, dependency graphs and how to build them.

It wouldn't make sense to merge them if that's what you're suggesting.

[Mertsch]

@herecydev No, that's not what I was trying to say, it was more about the pure naming and use. The systems themselves should be separate.
To make it more clear what I mean I'll list the points

1. Both "build" a usable instance of something one is called ConfigurationBuilder the other one ServiceCollection why not ConfigurationCollection or ServiceBuilder
2. You call .Build(); and .BuildServiceProvider(); both with the same "build some instance"
3. By default config is "build" in Startup() while DI is in ConfigureServices(IServiceCollection services). Again same principal, but completely different places. For example a bug in config (lets say mandatory config file is missing) leads to ctor crash which is much harder to locate from a logging/debugging standpoint, then in a specialized method like ConfigureServices

Thanks for your feedback

[herecydev]

Both "build" a usable instance of something one is called ConfigurationBuilder the other one ServiceCollection why not ConfigurationCollection or ServiceBuilder

The ConfigurationBuilder takes multiple sources/providers and doesn't have any collections of configuration in itself; it simply builds a ConfigurationRoot that has the providers. The ServiceCollection contains a collection of ServiceDescriptors and you can add/remove directly to this collection. With these key differences, you can see the naming actually makes sense.

You call .Build(); and .BuildServiceProvider(); both with the same "build some instance"

This probably could be .BuildConfiguration(). @divega for consistency?

By default config is "build" in Startup() while DI is in ConfigureServices(IServiceCollection services). Again same principal, but completely different places.

That's just the template that has this in, because it's often used first. This isn't mandatory though, and you're encouraged to move things around to suit your needs.

For example a bug in config (lets say mandatory config file is missing) leads to ctor crash which is much harder to locate from a logging/debugging standpoint, then in a specialized method like ConfigureServices

You can get an ILoggerFactory injected into the startup constructor and log this out:

public Startup(IHostingEnvironment env, ILoggerFactory loggerFactory)
{
    try
    {
        var builder = new ConfigurationBuilder().AddJsonFile("appsettings.json");
        //some exception happens here
    }
    catch(Exception ex)
    {
        var logger = loggerFactory.CreateLogger<Startup>();
        logger.LogError("Startup error!", ex);
    }
}

[ajcvickers]

The parts of this that might make sense to do are tracked by other issues. Closing this discussion thread.