Add support for repeated XML elements without a name attribute

[amoerie]

New PR because #43722 was out of date and CI was broken at the time. See that PR for initial review.

Fixes #36541
Fixes #36561

This pull request adds support in Microsoft.Extensions.Configuration.Xml for repeated XML elements without requiring a Name attribute.
This solves a particularly subtle bug when configuring Serilog from an XML configuration source. For a full description, see #36541

The original implementation of the XmlStreamConfigurationProvider has been modified to do the following:

Maintain a stack of encountered XML elements while traversing the XML source. This is needed to detect siblings.
When siblings are detected, automatically append an index to the generated configuration keys. This makes it work exactly the same as the JSON configuration provider with JSON arrays.
Tests are updated to reflect the new behavior:

Tests that verified an exception occurs when entering duplicate keys have been removed. Duplicate keys are supported now.
Add tests that verify duplicate keys result in the correct configuration keys, with all the lower/upper case variants and with support for the special "Name" attribute handling.
Thank you for your consideration.

[ghost]

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.

---

Issue meta data

Issue content:
	New PR because #43722 was out of date and CI was broken at the time. See that PR for initial review. Fixes #36541 Fixes #36561 This pull request adds support in Microsoft.Extensions.Configuration.Xml for repeated XML elements without requiring a Name attribute. This solves a particularly subtle bug when configuring Serilog from an XML configuration source. For a full description, see #36541 The original implementation of the XmlStreamConfigurationProvider has been modified to do the following: Maintain a stack of encountered XML elements while traversing the XML source. This is needed to detect siblings. When siblings are detected, automatically append an index to the generated configuration keys. This makes it work exactly the same as the JSON configuration provider with JSON arrays. Tests are updated to reflect the new behavior: Tests that verified an exception occurs when entering duplicate keys have been removed. Duplicate keys are supported now. Add tests that verify duplicate keys result in the correct configuration keys, with all the lower/upper case variants and with support for the special "Name" attribute handling. Thank you for your consideration. </td>
Issue author:		amoerie
Assignees:	-
Labels:	`area-Extensions-Configuration` </td>
Milestone:	-

[maryamariyan]

Configuration tests failing shown in https://dev.azure.com/dnceng/public/_build/results?buildId=884531&view=ms.vss-test-web.build-test-results-tab

[amoerie]

@maryamariyan CI is green now. The tests that verify the configuration with duplicate keys in them are not compatible with the way the repeated elements work in XML configuration.

To be more specific, the 'DuplicatesTestConfig' provides the following test data:

public static TestSection DuplicatesTestConfig { get; }
                = new TestSection
                {
                    Values = new[]
                    {
                        ("Key1", (TestKeyValue)"Value1"),
                        ("Key1", (TestKeyValue)"Value1")
                    },
                    ...
                };

Later, this is verified:

protected virtual void AssertConfig(
            IConfigurationRoot config,
            bool expectNulls = false,
            string nullValue = null)
        {
            var value1 = expectNulls ? nullValue : "Value1";
            ...

            Assert.Equal(value1, config["Key1"], StringComparer.InvariantCultureIgnoreCase);
            ...
        }

But this fails, because "Key1" is not in the output. This is because the XML config provider will produce two entries, "Key1:0" and "Key1:1", both with value "Value1".
This is not a breaking change, because previously the XML provider simply threw an exception in this scenario.
However, it does mean we cannot simply reuse the duplicates tests from ConfigurationProviderTestBase, because the output does not match.

[amoerie]

Any update on this?

[maryamariyan]

Any update on this?

We'll be reviewing this week,
cc: @safern

[amoerie]

@maryamariyan I took a look at your comments, and almost all of them stem from the same issue: the "Key" property was lazily computed in various places, because you cannot compute it once in the constructor. This is because the value of "IsMultiple" might change, because a new element is encountered during traversal that shares the same name. This triggers the new behavior where the "sibling index" is then added to the key.

To address your concerns, I slightly reworked the implementation, and I would be interested to hear your thoughts.

I've removed all "Key" and "Value" computation logic from the new classes, they are now extremely simple POCOs. Instead, all of the heavy lifting now happens in XmlStreamConfigurationProvider.

The "algorithm" now works as follows:

1. Traverse the XML document using XmlReader. While traversing, build an object tree of XmlConfigurationElement. Each element has a reference to its attributes, text content and children. This traversal happens in XmlStreamConfigurationProvider.Read.
2. While traversing, detect siblings. Siblings are elements that share the same parent and element name. (If a Name attribute is present, that must also be the same)
3. After the XML document has been traversed, process the XmlConfigurationElement tree (starting from the root) and produce keys and values for the configuration dictionary. This happens in XmlStreamConfigurationProvider.ProvideConfiguration

[amoerie]

@maryamariyan Any chance this PR could get another review?

[maryamariyan]

Thanks for the update, I'll do a comprehensive review within next week,

[maryamariyan]

Hey @amoerie, I haven't forgotten about this one, I'll make sure this keeps moving and totally appreciate how you kept the fix up to date.

[amoerie]

Thanks! On the bright side, I guess I can say I've been contributing to Microsoft Open Source for 3 years already. 😉

[safern]

@amoerie thanks for running the benchmarks. I don't think we have any benchmarks for Microsoft.Extensions.Configuration yet: https://github.com/dotnet/performance/tree/master/src/benchmarks/micro/libraries

based on the results and your investigation, I think taking a 5-20% regression is not acceptable, specially since we could try and improve your current implementation with the info you got from the profiling.

GetLineInfo : previously, line info was only read when an error occurred. Now, we must call it for every element and store it, in case we need to throw an exception later. One might question if this line info is really such an important feature.

Is there a way to get the line info only if an exception is thrown? Removing it from the error might be breaking and I think having it in the error is valuable as it helps you know where the error in your configuration file is.

[amoerie]

@safern I've pushed some optimizations, if you would find the time again to review that.

GetLineInfo : previously, line info was only read when an error occurred. Now, we must call it for every element and store it, in case we need to throw an exception later. One might question if this line info is really such an important feature.

Is there a way to get the line info only if an exception is thrown? Removing it from the error might be breaking and I think having it in the error is valuable as it helps you know where the error in your configuration file is.

Avoiding the line info reading would require completely revising the algorithm, which I'm hesitant to do right now. But I found that the most time was spent on SR.Format which uses the LineNumber and LinePosition, so now I'm delaying that string format as long as possible.

This is a summary of the improvements I've made:

• Store LineNumber and LinePosition instead of LineInfo. This avoids a call to string.Format in most cases.
• Use a helper class to build the configuration keys. This uses StringBuilder instead of string.Join internally, which is much faster.
• Keep a dictionary of XML children, keyed by their "SiblingName" (= ElementName + Name). This allows for a much more efficient lookup of siblings under the current parent.
• Do not create this dictionary for XML elements with a single child. This is quite common, and avoiding it in this scenario is more efficient in terms of memory and execution.
• Use Dictionary.TryAdd instead of Dictionary.ContainsKey + Dictionary.Add if we can (.NET Standard 2.1)

All of this amounts to an implementation that is even slightly faster than the original one! 🎉

These are the performance results of these modifications:

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19042
Intel Core i7-8700K CPU 3.70GHz (Coffee Lake), 1 CPU, 12 logical and 6 physical cores
.NET Core SDK=5.0.103
  [Host]     : .NET Core 5.0.3 (CoreCLR 5.0.321.7212, CoreFX 5.0.321.7212), X64 RyuJIT
  DefaultJob : .NET Core 5.0.3 (CoreCLR 5.0.321.7212, CoreFX 5.0.321.7212), X64 RyuJIT

Method	TestFile	Mean	Error	StdDev	Ratio	RatioSD	Gen 0	Gen 1	Gen 2	Allocated
Before	deep.xml	125.49 μs	2.507 μs	2.682 μs	1.00	0.00	14.6484	1.9531	-	92048 B
After	deep.xml	130.67 μs	1.721 μs	1.609 μs	1.04	0.03	15.8691	1.9531	-	99800 B
Improved	deep.xml	124.07 μs	2.414 μs	2.965 μs	0.99	0.03	14.4043	1.8311	-	90648 B
Before	names.xml	185.49 μs	2.378 μs	2.442 μs	1.00	0.00	24.1699	4.6387	-	151912 B
After	names.xml	231.85 μs	4.546 μs	7.962 μs	1.27	0.05	27.3438	6.1035	-	172088 B
Improved	names.xml	181.91 μs	1.792 μs	1.399 μs	0.98	0.02	25.8789	5.6152	-	163224 B
Before	repeated.xml	NA	NA	NA	?	?	-	-	-	-
After	repeated.xml	183.69 μs	2.673 μs	2.501 μs	?	?	22.9492	4.1504	-	144720 B
Improved	repeated.xml	156.03 μs	2.841 μs	3.157 μs	?	?	20.7520	4.1504	-	131392 B
Before	simple.xml	92.73 μs	1.156 μs	0.903 μs	1.00	0.00	11.5967	1.2207	-	72864 B
After	simple.xml	111.59 μs	1.996 μs	1.770 μs	1.21	0.02	12.5732	1.3428	-	79096 B
Improved	simple.xml	92.49 μs	1.412 μs	1.179 μs	1.00	0.02	11.7188	1.3428	-	74232 B

Benchmarks with issues:
XmlConfigurationBenchmarks.Before: DefaultJob [TestFile=repeated.xml]

[safern]

Thanks, @amoerie. I'm happy that you were able to find a way that it even improves the perf. Now that you wrote some benchmarks, can we add them to: https://github.com/dotnet/performance/tree/master/src/benchmarks/micro/libraries ? That way our infra will catch if there are any regressions in the future.

I left a couple of comments, but overall looks good.

[amoerie]

Thanks, @amoerie. I'm happy that you were able to find a way that it even improves the perf. Now that you wrote some benchmarks, can we add them to: https://github.com/dotnet/performance/tree/master/src/benchmarks/micro/libraries ? That way our infra will catch if there are any regressions in the future.

I left a couple of comments, but overall looks good.

Thanks, I'll see if I can add the benchmarks there.

[safern]

Timeout is: #49309

[safern]

@amoerie thanks for the effort to get this fix in 🎉

[amoerie]

@safern

You're very welcome! It's been quite the experience, I learned quite a few things during this endeavor. 💪

Thank you (and @maryamariyan too) for taking time out of your schedules to review and discuss the work of a total stranger. From my own work experience, I know this is not trivial.

For me personally, I consider it a very big deal to have contributed to .NET itself. So thanks for making that possible. 👍

[safern]

For me personally, I consider it a very big deal to have contributed to .NET itself. So thanks for making that possible.

Anytime! Feel free to browse for more up-for-grabs issues and help us make .NET better and reach out for any help if needed, we really appreciate and like the community help!

[eerhardt]

I have created dotnet/docs#30660 to document this breaking change.