AspNet Core Settings - Basic implementation

[StefanKert]

The title is pretty self-explaining. Please take a look at the current implementation and add things if you find something that makes no sense, or tell me to change it :).

There are still some points that need to be discussed:

• Should we register a Method that refreshes the Settings on each file change?
• Should we add a validation for the settings file? Especially if someone adds an errorstore that is not supported it would be better to fail early. Maybe there are further settings that need to be validated.
• I haven´t found a similar event for a post deserialize for the configuration implementation in asp.net core but maybe you have an idea? Currently, I am calling all initialize methods after the Binding happened.

I really tried to add up to the current ConfigSettings class in StackExchange.Exceptional so maybe we can share some code here, but this would need some workarounds, so I think it is a good solution to use a separate class structure for this.

Thanks for your feedback. Really appreciate any input :)

[StefanKert]

Thanks for the feedback Nick! Really appreciate it. Fixed most of the issues. I need to do the rebase, but therefore i need a little time to get my git skills straight ;)

There is just the ApplicationBuilder exception thing that we need to perform some steps on. I added a proposal in the comments section, but just to be safe:

This is not the best approach. Agreed. One of the possible solutions that I can think of is instantiating ConfigSettings in ctor of ExceptionalMiddleWare.

So basically we would add this call in the Startup class if the user likes to use the Configuration for exceptional:

services.Configure<ConfigSettings>(Configuration.GetSection("Exceptional")

We could then inject ConfigSections in ExceptionMiddleware ctor and call Populate(settings) in there.

[StefanKert]

Another possible solution would be to extend the IServiceCollection:

public static IServiceCollection ConfigureExceptionalSettings(this IServiceCollection services, IConfiguration config)
{
 //nullchecks
  var configSettings = new ConfigSettings(configuration);
  configSettings.Populate(Settings.Current);
  // add to di container
 return services;
}

This way we would instantiate the current settings with the give configuration. The user has to add a call to this in his ConfigureServices Method similar to this:

services.ConfigureExceptionalSettings(Configuration.GetSection("Exceptional");

We could also add it to the DI Container so that we can retrieve it by adding IOptions<ConfigSettings> to any ctor.

[StefanKert]

I´ve already created an example file that I used for testing, need to get some comments in it!

Just another thing regarding IgnoreSettings and probably also LogFilters. If We call Populate again any time during the app/webapp is running, we would add up all Filters and IgnoreSettings to the current list in Settings. Shouldn´t we add a check if these items already exist in the list? I don´t want to clear the list at all because if somebody has already added some items they get also cleared out so I think here it would be better to check if these items already exist. Maybe you got different thoughts on this? Please let me know, but I think this could be a problem if we don´t check what to do now.

[NickCraver]

@StefanKert That's why the underlying implementations are de-duped collection types ;) IgnoreSettings.Regexes as the exception to this (just changed), so shouldn't be an issue anymore :)

[StefanKert]

@NickCraver Awesome :) try to take a look if we could use the whole ConfigSettings class as internal. What are you thinking about providing a separate Configure Method?

public static IServiceCollection ConfigureExceptionalSettings(this IServiceCollection services, IConfiguration config)
{
 //nullchecks
  var configSettings = new ConfigSettings(configuration);
  configSettings.Populate(Settings.Current);
  // add to di container
 return services;
}

Could be the replacement for the IConfiguration parameter.

[NickCraver]

I think what you have is better, it's not about being clever here, but about user expectations. ASP.NET Core has certain patterns, the .GetSection() approach is the normal pattern...so we go with the flow. I'll just add some overloads when we're done without the param, no big deal.

[StefanKert]

Ok. I can totally add up to this, but I thought about using similar approach to the recommended way as it is in aspnet core. For example if you´d like to load your configuration into a setings class you would implement it like this:

    public void ConfigureServices(IServiceCollection services)
    {
        // Adds services required for using options.
        services.AddOptions();

        // Register the IConfiguration instance which MyOptions binds against.
        services.Configure<MySubOptions>(Configuration.GetSection("subsection"));

        // Add framework services.
        services.AddMvc();
    }

So we got the Configure Method here that populates the MyOptions class and adds it to the DI Container. So the call for our method would probably look like this:

    public void ConfigureServices(IServiceCollection services)
    {
        // Adds services required for using options.
        services.AddOptions();

        // Register the IConfiguration instance which MyOptions binds against.
        services.ConfigureExceptionalSettings(Configuration.GetSection("Exceptional"));

        // Add framework services.
        services.AddMvc();
    }

Just wanted to throw this in so you get what I mean with that :) I am totally fine with the overload approach, just wanted to leave this in here :)

[StefanKert]

@NickCraver I tested if we could make the ConfigSettings class internal only, but right now this is not working. aspnet/Configuration#394 and if I understand the comments correctly this will come in 2.1? There is also another package that lets you set all settings even if they have no setter, but this would introduce another dependency on a package that is out of our control and even not in control of Microsoft. So I suggest that we use public for all the properties that need them and wait for the new Configuration release.

[NickCraver]

@StefanKert whelp, that sucks. I appreciate the linked issue, I'll help track that down and see if we can get the change in. I'm torn on whether to use a custom binder, or waste even more time and duplication in restoring the comments...I'm leaning towards the binder, but I can take that separately. Let me look around for options.

[NickCraver]

@StefanKert the internal class seems to be working here, I'll test out more tweaks but from my reading of the default binder only non-public properties (or constructors) would have any issues. I think we're all good.

Merged into master and moved to .Internal to cleanup, same with extension changes, thank you!!

[StefanKert]

@NickCraver I guess I should have tested a little bit more :) only tested all internal. Awesome that this is working now.

Great to hear :) if you need any further help just ping me here or on Twitter.