dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.27k stars 4.73k forks source link

[API Proposal]: Use user-provided `IDictionary<TK, TV>` instance in `Microsoft.Extensions.Configuration.Xml.XmlConfigurationProvider` #107251

Open ZzZombo opened 2 months ago

ZzZombo commented 2 months ago

Background and motivation

In our case we'd like to use an ordered dictionary for storing the parsed configuration. However, it's currently not feasible as the underlying implementation manages this entirely behind the scenes w/ no clean way to influence it. We ended up subclassing XmlConfigurationProvider in order to override the Load() family of methods and copy-paste the code from .NET source, with the only change to populate an ordered dictionary instead of the regular one. But much of the needed code is not exposed to outside assemblies, meaning that we had to find a way to access the internals from our code. We for now settled on selectively publicizing the members we needed (11 entries with varying degrees of granularity). The most problematic were the SR string constants used in the localized error messages, as there are several different source definitions of the symbol, causing immense confusion w/o a clear recourse. We ended up copying them too where we couldn't resolve the conflicts.

The order is important because some of the configuration settings may trigger side effects when set and so it matters for correct program execution. The customer has also asked to keep the configuration files largely intact when writing modified configuration back to disk, so we needed to preserve the order of existing entries.

API Proposal

We do not have a clear API in mind. I personally think that it should be possible to use the concrete instance of ConfigurationProvider.Data when populating the entries within XmlStreamConfigurationProvider.Read(), possibly it could take an additional IDictionary<,> parameter. Then our subclass could just instantiate the proper concrete dictionary instance in the constructor once. The rest of the code should also preserve the custom provided instance instead of the generic one, as there are several places where it initializes a brand new Dictionary<,> instance.

XmlStreamConfigurationProvider.Read(Stream stream, XmlDocumentDecryptor decryptor, IDictionary<string, string?> data)

API Usage

class OrderedXmlProvider: XmlConfigurationProvider {
    OrderedXmlProvider() => this.Data = new OrderedDictionary<string, string>();
}

// The underlying `XmlStreamConfigurationProvider.Read()` is passed the value of `this.Data` as the dictionary to use. If it is `null` then it may instantiate `Dictionary<string, string>` instead.

Alternative Designs

With the fresh addition of a build-in ordered dictionary implementation in .NET 9, this issue could be solved by using it as the backing store of parsed configuration; I doubt it may have any implications because nobody should rely on any order with the currently used Dictionary<,>; I think it has merit outside of our use case to make it the default, at least I can't think of downside for doing so for other user code. Another possibility is to open up the involved classes for easier subclassing in user code, although it would still also require a certain level of refactoring because as it stands currently, XmlStreamConfigurationProvider.Read() will not be going to cooperate with the goal of populating the user-provided dictionary type/instance as is.

Risks

dotnet-policy-service[bot] commented 2 months ago

Tagging subscribers to this area: @dotnet/area-extensions-configuration See info in area-owners.md if you want to be subscribed.