aaubry / YamlDotNet

YamlDotNet is a .NET library for YAML
MIT License
2.48k stars 466 forks source link

`WithDuplicateChecking()` conflicts with `MergingParser` operation #888

Open dougbu opened 5 months ago

dougbu commented 5 months ago

Describe the bug

As far as I can tell, YamlDotNet correctly handles duplicate key checking with YAML-spec-compliant anchors and aliases. However, the MergingParser is unable to deserialize YAML using the proposed merge key when DeserializerBuilder.WithDuplicateKeyChecking() has been called.

My initial thought for a potential fix would be tracking the anchor from which a key was previously set. While expanding a higher-level alias or a new map, duplicate keys should be ignored and handled as they are today when using MergingParser. I see YamlNode.Anchor exists but am not sure the MergingParser, DictionaryDeserializer, and ObjectNodeDeserializer can use this property to handle keys during merges correctly.  

To Reproduce

Add a test like SerializationTests.ExampleFromSpecificationIsHandledCorrectly() but with a new name and using

var deserializer = DeserializerBuilder.WithDuplicateKeyChecking().Build();

instead of the inherited Deserializer property. Execute the new test.

Expected

The test should work perfectly.

Actual

The test fails. In my branch, the error is

[xUnit.net 00:00:03.91]     ExampleFromSpecificationIsHandledCorrectlyWithDuplicateChecking [FAIL]
  Failed ExampleFromSpecificationIsHandledCorrectlyWithDuplicateChecking [6 ms]
  Error Message:
   YamlDotNet.Core.YamlException : Encountered duplicate key r
  Stack Trace:
     at YamlDotNet.Serialization.NodeDeserializers.DictionaryDeserializer.TryAssign(IDictionary result, Object key, Object value, MappingStart propertyName) in C:\dd\dnx\other\YamlDotNet\YamlDotNet\Serialization\NodeDeserializers\DictionaryDeserializer.cs:line 44
...

Debugging shows the incorrect detection occurs at the << : [ *BIG, *LEFT, *SMALL ] line in the sample. I'm not positive (because I'm not familiar enough w/ the code) but suspect it's currently expanding the *BIG alias.

dougbu commented 5 months ago

@aaubry, this one is important in something I'd like to change in .NET build infrastructure. if you have a minute to suggest an approach, I'm happy to put some time into it :grin:

EdwardCooke commented 5 months ago

I’d have to double check but if the merging parser uses yamlstream under the hood then that makes sense because the stream calls dictionary.add without checking for duplicate keys. My suspicion is so that it will follow the spec. It was requested not to long ago to have that feature in the stream where duplicate keys would override the previous value, pretty sure that’s what it was.

EdwardCooke commented 5 months ago

Ignore my last comment. Way off base.