Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WithDuplicateChecking() conflicts with MergingParser operation #888

Open
dougbu opened this issue Jan 7, 2024 · 4 comments
Open

WithDuplicateChecking() conflicts with MergingParser operation #888

dougbu opened this issue Jan 7, 2024 · 4 comments

Comments

@dougbu
Copy link
Contributor

dougbu commented Jan 7, 2024

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
Copy link
Contributor Author

dougbu commented Jan 23, 2024

@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 😁

@EdwardCooke
Copy link
Collaborator

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
Copy link
Collaborator

Ignore my last comment. Way off base.

@EdwardCooke
Copy link
Collaborator

I’m going to work on fixing this in the next little bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants