aspnet / MicrosoftConfigurationBuilders

Microsoft.Configuration.Builders
MIT License
118 stars 61 forks source link

KeyValueConfigBuilders are broken when upgrading #196

Closed philipwolfe closed 1 year ago

philipwolfe commented 2 years ago

Here is a sample project that runs with the current version. It is a simple example using the Json file config builder but this problem is with all new config builders.

https://github.com/philipwolfe/ConfigBuildersTester

When you upgrade the project to 3.0 and change the 'Expand' value to 'Token', the exception is thrown about not finding an ISectionHandler. (Which I have reported previously as a problem.) This will prevent us from upgrading.

We are moving away from using appSettings and connectionStrings because of traceability. When you have a project that references 20 other libraries that all have their own config settings, it is not practical to tell which settings belong to which library. When settings are shared, one does not know when to remove a setting, so no one cleans up settings. With applicationSettings sections, each library declares what settings it needs and "is noisy" about it when they are missing. We are hoping this can get resolved before release. Thanks.

philipwolfe commented 1 year ago

@StephenMolloy or @terrajobst Can you comment on this before releasing V3?

StephenMolloy commented 1 year ago

Unfortunately, there are some cases where Expand mode worked that are just left out by the transition to Token mode. Expand worked on a full raw-text view of an entire section before it was turned into a ConfigurationSection, and as such, it could be applied to any section regardless of type. Token mode OTOH is limited to a key/value enumeration of sections after they have been objectified by the config system, which means there needs to be a section handler available to help turn that particular section type into a key/value enumeration that can be manipulated.

The trade-off with the newly restrictive model is that we don't have the confusing two-pass application of builders to each section anymore, as this suite of builders no longer bothers with ProcessRawXml. Having that two-pass model was confusing at best, and could easily result in some unintended recursion issues if not careful. Buildling config based on the results of builders earlier in the chain was a nightmare to keep track of, especially when builders were going into appSettings to retrieve their own settings that might or might not be available until the next stage.

Anecdotally, I haven't really seen anyone taking full advantage of the raw xml string capabilities. Even if that was the implementation of Expand, in practice, folks still seem to be keeping their tokens within the mentally formed boundaries of 'key' or 'value.' And indeed this is even the case with your sample.

So the good news is there is hope. For your basic sample, all you need to do is add a section handler for the section in question. More good news is that I've already got a basic sample handler written for ClientSettingsSections. The bad news is that I have intentionally left it a sample and not included it in the base package because "ClientSettings" are a bit of a unique beast that travels a slightly different road than most configuration sections - owing to the fact that it is both a ".Net Configuration System" section as well as a magical interactive config object intended to be used by client applications and surfaced through an API which is separate from the traditional config system. I can't guarantee that our basic config handler gets run at the right time to get your expected values depending on what you're doing with the fancy app-facing settings object.

It does work with your simple sample though. You just need to add a class like ClientSettingsSectionHandler.cs to your project, and then register the section handler in your app config like lines 6 and 23-27 of Sample App.config.

StephenMolloy commented 1 year ago

One more thing. If your real-case scenario can't leverage Token/SectionHandler's, then you could always try to bring back the old Expand behavior with your own derived KVConfigBuilder, or something like this this wrapper approach as is demonstrated in the SampleConsoleApp.