From a96e445e4dcca014be473a1ebb42806d2f917565 Mon Sep 17 00:00:00 2001 From: RoosterDragon Date: Fri, 7 Jul 2023 19:35:02 +0100 Subject: [PATCH] Handle duplicate nodes key checks in MiniYaml in a better place. Moving the key duplication check allows a redundant check on top-level nodes to be avoided. Add tests to ensure key checks are functioning as expected. --- OpenRA.Game/MiniYaml.cs | 6 +- OpenRA.Test/OpenRA.Game/MiniYamlTest.cs | 147 +++++++++++++++++++++++- 2 files changed, 144 insertions(+), 9 deletions(-) diff --git a/OpenRA.Game/MiniYaml.cs b/OpenRA.Game/MiniYaml.cs index 851742da11..2bee5fb23f 100644 --- a/OpenRA.Game/MiniYaml.cs +++ b/OpenRA.Game/MiniYaml.cs @@ -416,6 +416,9 @@ namespace OpenRA static MiniYaml MergePartial(MiniYaml existingNodes, MiniYaml overrideNodes) { + existingNodes?.Nodes.ToDictionaryWithConflictLog(x => x.Key, "MiniYaml.Merge", null, x => $"{x.Key} (at {x.Location})"); + overrideNodes?.Nodes.ToDictionaryWithConflictLog(x => x.Key, "MiniYaml.Merge", null, x => $"{x.Key} (at {x.Location})"); + if (existingNodes == null) return overrideNodes; @@ -435,9 +438,6 @@ namespace OpenRA var ret = new List(existingNodes.Count + overrideNodes.Count); - var existingDict = existingNodes.ToDictionaryWithConflictLog(x => x.Key, "MiniYaml.Merge", null, x => $"{x.Key} (at {x.Location})"); - var overrideDict = overrideNodes.ToDictionaryWithConflictLog(x => x.Key, "MiniYaml.Merge", null, x => $"{x.Key} (at {x.Location})"); - foreach (var node in existingNodes.Concat(overrideNodes)) { // Append Removal nodes to the result. diff --git a/OpenRA.Test/OpenRA.Game/MiniYamlTest.cs b/OpenRA.Test/OpenRA.Game/MiniYamlTest.cs index 1dc2dfc447..1f6d5557a5 100644 --- a/OpenRA.Test/OpenRA.Game/MiniYamlTest.cs +++ b/OpenRA.Test/OpenRA.Game/MiniYamlTest.cs @@ -18,7 +18,10 @@ namespace OpenRA.Test [TestFixture] public class MiniYamlTest { - readonly string yamlTabStyle = @" + [TestCase(TestName = "Mixed tabs & spaces indents")] + public void TestIndents() + { + var yamlTabStyle = @" Root1: Child1: Attribute1: Test @@ -31,7 +34,7 @@ Root2: Attribute1: Test "; - readonly string yamlMixedStyle = @" + var yamlMixedStyle = @" Root1: Child1: Attribute1: Test @@ -43,10 +46,6 @@ Root2: Child1: Attribute1: Test "; - - [TestCase(TestName = "Mixed tabs & spaces indents")] - public void TestIndents() - { var tabs = MiniYaml.FromString(yamlTabStyle, "yamlTabStyle").WriteToString(); Console.WriteLine(tabs); var mixed = MiniYaml.FromString(yamlMixedStyle, "yamlMixedStyle").WriteToString(); @@ -300,6 +299,142 @@ Test: Assert.That(mergeNode.Nodes[0].Value.Value, Is.EqualTo("override"), "Merge node Child value should be 'override', but is not"); } + [TestCase(TestName = "Duplicated nodes across multiple sources are correctly merged")] + public void TestSelfMergingMultiSource() + { + var firstYaml = @" +Test: + Merge: original + Child: original + Original: +"; + var secondYaml = @" +Test: + Merge: original + Child: original + Original: +Test: + Merge: override + Child: override + Override: +"; + + var result = MiniYaml.Merge(new[] { firstYaml, secondYaml }.Select(s => MiniYaml.FromString(s, ""))); + Assert.That(result.Count(n => n.Key == "Test"), Is.EqualTo(1), "Result should have exactly one Test node."); + + var testNodes = result.First(n => n.Key == "Test").Value.Nodes; + Assert.That(testNodes.Select(n => n.Key), Is.EqualTo(new[] { "Merge", "Original", "Override" }), "Merged Test node has incorrect child nodes."); + + var mergeNode = testNodes.First(n => n.Key == "Merge").Value; + Assert.That(mergeNode.Value, Is.EqualTo("override"), "Merge node has incorrect value."); + Assert.That(mergeNode.Nodes[0].Value.Value, Is.EqualTo("override"), "Merge node Child value should be 'override', but is not"); + } + + [TestCase(TestName = "Duplicated child nodes do not throw if parent does not require merging")] + public void TestMergeConflictsNoMerge() + { + var baseYaml = @" +Test: + Merge: + Child: + Child: +"; + + var result = MiniYaml.Merge(new[] { baseYaml }.Select(s => MiniYaml.FromString(s, ""))); + var testNodes = result.First(n => n.Key == "Test").Value.Nodes; + var mergeNode = testNodes.First(n => n.Key == "Merge").Value; + Assert.That(mergeNode.Nodes.Count, Is.EqualTo(2)); + } + + [TestCase(TestName = "Duplicated child nodes throw merge error if first parent requires merging")] + public void TestMergeConflictsFirstParent() + { + var baseYaml = @" +Test: + Merge: + Child1: + Child1: + Merge: +"; + + void Merge() => MiniYaml.Merge(new[] { baseYaml }.Select(s => MiniYaml.FromString(s, "test-filename"))); + Assert.That(Merge, Throws.Exception.TypeOf().And.Message.EqualTo( + "MiniYaml.Merge, duplicate values found for the following keys: Child1: [Child1 (at test-filename:4),Child1 (at test-filename:5)]")); + } + + [TestCase(TestName = "Duplicated child nodes throw merge error if second parent requires merging")] + public void TestMergeConflictsSecondParent() + { + var baseYaml = @" +Test: + Merge: + Merge: + Child2: + Child2: +"; + void Merge() => MiniYaml.Merge(new[] { baseYaml }.Select(s => MiniYaml.FromString(s, "test-filename"))); + Assert.That(Merge, Throws.Exception.TypeOf().And.Message.EqualTo( + "MiniYaml.Merge, duplicate values found for the following keys: Child2: [Child2 (at test-filename:5),Child2 (at test-filename:6)]")); + } + + [TestCase(TestName = "Duplicated child nodes across multiple sources do not throw")] + public void TestMergeConflictsMultiSourceMerge() + { + var firstYaml = @" +Test: + Merge: + Child: +"; + var secondYaml = @" +Test: + Merge: + Child: +"; + + var result = MiniYaml.Merge(new[] { firstYaml, secondYaml }.Select(s => MiniYaml.FromString(s, ""))); + var testNodes = result.First(n => n.Key == "Test").Value.Nodes; + var mergeNode = testNodes.First(n => n.Key == "Merge").Value; + Assert.That(mergeNode.Nodes.Count, Is.EqualTo(1)); + } + + [TestCase(TestName = "Duplicated child nodes across multiple sources throw merge error if first parent requires merging")] + public void TestMergeConflictsMultiSourceFirstParent() + { + var firstYaml = @" +Test: + Merge: + Child1: + Child1: +"; + var secondYaml = @" +Test: + Merge: +"; + + void Merge() => MiniYaml.Merge(new[] { firstYaml, secondYaml }.Select(s => MiniYaml.FromString(s, "test-filename"))); + Assert.That(Merge, Throws.Exception.TypeOf().And.Message.EqualTo( + "MiniYaml.Merge, duplicate values found for the following keys: Child1: [Child1 (at test-filename:4),Child1 (at test-filename:5)]")); + } + + [TestCase(TestName = "Duplicated child nodes across multiple sources throw merge error if second parent requires merging")] + public void TestMergeConflictsMultiSourceSecondParent() + { + var firstYaml = @" +Test: + Merge: +"; + var secondYaml = @" +Test: + Merge: + Child2: + Child2: +"; + + void Merge() => MiniYaml.Merge(new[] { firstYaml, secondYaml }.Select(s => MiniYaml.FromString(s, "test-filename"))); + Assert.That(Merge, Throws.Exception.TypeOf().And.Message.EqualTo( + "MiniYaml.Merge, duplicate values found for the following keys: Child2: [Child2 (at test-filename:4),Child2 (at test-filename:5)]")); + } + [TestCase(TestName = "Comments are correctly separated from values")] public void TestEscapedHashInValues() {