Add support for repeated XML elements without a name attribute

[amoerie]

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.

[amoerie]

I could use a little help with the failing build, I don't see how it's related to my changes.

[maryamariyan]

The failing build

  Discovering: Microsoft.Extensions.Hosting.Unit.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  Microsoft.Extensions.Hosting.Unit.Tests (found 84 test cases)
  Starting:    Microsoft.Extensions.Hosting.Unit.Tests (parallel test collections = on, max threads = 2)
    Microsoft.Extensions.Hosting.Internal.HostTests.BackgroundServiceAsyncExceptionGetsLogged [FAIL]
      'BackgroundService failed' did not get logged
      Expected: True
      Actual:   False
      Stack Trace:
        /_/src/libraries/Microsoft.Extensions.Hosting/tests/UnitTests/Internal/HostTests.cs(1256,0): at Microsoft.Extensions.Hosting.Internal.HostTests.BackgroundServiceAsyncExceptionGetsLogged()
�[40m�[32minfo�[39m�[22m�[49m: Microsoft.Extensions.Hosting.Tests.HostTests[0]
      Request starting
  Finished:    Microsoft.Extensions.Hosting.Unit.Tests
=== TEST EXECUTION SUMMARY ===
   Microsoft.Extensions.Hosting.Unit.Tests  Total: 84, Errors: 0, Failed: 1, Skipped: 0, Time: 15.317s

is tracked by #43389

[maryamariyan]

Thanks @amoerie for porting over this change to dotnet/runtime.

We'll need to do a full review on the changes as it seems like there is a considerable change being introduced.
This changes has been upvoted by community and @ajcvickers mentioned in earlier PRs that the repo consolidations were part of the blocker for this fix to getting merged in.

Also adding @HaoK to this PR. Do you have any major hesitations on this kind of enhancement getting introduced into extensions configuration or it sounds good to you?

[amoerie]

We'll need to do a full review on the changes as it seems like there is a considerable change being introduced.

I just want to add a little note to this sentence: the internal changes are indeed considerable, but the external compatibility has not been broken. I did not modify any existing tests (except for 2 tests that verified duplicate keys are not supported - now they are) and only added tests to verify the new supported behavior.

In any case, I'm very interested to hear your feedback on my code.

[HaoK]

I don't have any particular concerns, other than the general one around accidental breaking changes. This behavior in particular has been asked for frequently, and it was always near the top of the list of PRs we were considering, so I think its a good candidate to be worth the risk.

[HaoK]

One thing worth mentioning is that this change appears only to be made in the XmlStreamProvider, it would be strange for there to be a big behavior difference between the stream and file providers?

[amoerie]

One thing worth mentioning is that this change appears only to be made in the XmlStreamProvider, it would be strange for there to be a big behavior difference between the stream and file providers?

Are you saying there's other config provider implementations for XML files somewhere? Could you point me to them? I was operating under the assumption that everything would use the classes that I modified.

[amoerie]

@HaoK

Small update: I went digging a little and found no other implementations for XML files. In fact, I am convinced now that XML files also simply pass through the XmlStreamConfigurationProvider. See this class:

runtime/src/libraries/Microsoft.Extensions.Configuration.Xml/src/XmlConfigurationProvider.cs

Lines 8 to 29 in f3d84ca

	/// <summary>
	/// Represents an XML file as an <see cref="IConfigurationSource"/>.
	/// </summary>
	public class XmlConfigurationProvider : FileConfigurationProvider
	{
	/// <summary>
	/// Initializes a new instance with the specified source.
	/// </summary>
	/// <param name="source">The source settings.</param>
	public XmlConfigurationProvider(XmlConfigurationSource source) : base(source) { }
	internal XmlDocumentDecryptor Decryptor { get; set; } = XmlDocumentDecryptor.Instance;
	/// <summary>
	/// Loads the XML data from a stream.
	/// </summary>
	/// <param name="stream">The stream to read.</param>
	public override void Load(Stream stream)
	{
	Data = XmlStreamConfigurationProvider.Read(stream, Decryptor);
	}
	}

If I'm overlooking anything (this is a huge repo) please let me know and I'll make as many additions as necessary.

[amoerie]

/azp run runtime

[azure-pipelines]

Commenter does not have sufficient privileges for PR 43722 in repo dotnet/runtime

[maryamariyan]

/azp run runtime

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[amoerie]

/azp run runtime

Thanks @maryamariyan

Perhaps the PR guide could use an update, because I didn't see any option aside from pushing more commits or closing and reopening this PR.

[amoerie]

I'll close and reopen this PR to avoid merge commits.

[maryamariyan]

@amoerie how about the "rerun all checks" tab?
I think it refers to the "rerun" links in https://github.com/dotnet/runtime/pull/43722/checks?check_run_id=1302103237

[safern]

I believe that is because that is not available for contributors, only for collaborators. I believe write permissions are needed in order to use /azp run... and to see the rerun buttons on the checks tab.

[maryamariyan]

One thing worth mentioning is that this change appears only to be made in the XmlStreamProvider, it would be strange for there to be a big behavior difference between the stream and file providers?

Small update: I went digging a little and found no other implementations for XML files. In fact, I am convinced now that XML files also simply pass through the XmlStreamConfigurationProvider.

We have three file providers, and XmlConfigurationProvider is one of them, the other two are JsonConfigurationProvider and IniConfigurationProvider.

If I understood correctly @HaoK recommended if we are going to accept this breaking change in behavior that should also be done for the other two.

[HaoK]

Actually I wasn't talking about the other types of provider, I was referring to the old XmlConfigurationProvider, but indeed its just routing calls through the same code path so we should be good.

https://github.com/amoerie/runtime/blob/88e4f545e95d6952fbb34926f1dc9156b4e0baf6/src/libraries/Microsoft.Extensions.Configuration.Xml/src/XmlConfigurationProvider.cs#L27

[amoerie]

I've recreated my pull request here: #44608

This cleans up the commit history (i've squashed my commits into one) and properly retries the CI.