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.

Sign up for GitHub

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

Jump to bottom

FullObjectGraphTraversalStrategy.TraverseObject<TContext> makes its dictionary checks in (what appears to be) the wrong order #656

Closed
airbreather opened this issue Dec 8, 2021 · 1 comment · Fixed by #904
Closed

FullObjectGraphTraversalStrategy.TraverseObject<TContext> makes its dictionary checks in (what appears to be) the wrong order #656

airbreather opened this issue Dec 8, 2021 · 1 comment · Fixed by #904
Labels
question

Comments

@airbreather
Copy link
Contributor

airbreather commented Dec 8, 2021 •
edited

Shorter description: .EnsureRoundTrip() serializers fail to handle Dictionary<TKey, TValue> properties nested in other objects, because the check for IDictionary happens first and assigns the static type of object for both the key and value, before the check for IDictionary<TKey, TValue> that would assign the static types of TKey and TValue.

Longer description, with more specifics for why this happens, a minimal repro, and a workaround:

This bit here traverses an object that implements IDictionary<TKey, TValue>:

YamlDotNet/YamlDotNet/Serialization/ObjectGraphTraversalStrategies/FullObjectGraphTraversalStrategy.cs

Lines 176 to 183 in 665b182

var genericDictionaryType = ReflectionUtility.GetImplementedGenericInterface(value.Type, typeof(IDictionary<,>));
if (genericDictionaryType != null)
{
var genericArguments = genericDictionaryType.GetGenericArguments();
var adaptedDictionary = Activator.CreateInstance(typeof(GenericDictionaryToNonGenericAdapter<,>).MakeGenericType(genericArguments), value.Value)!;
TraverseDictionary(new ObjectDescriptor(adaptedDictionary, value.Type, value.StaticType, value.ScalarStyle), visitor, genericArguments[0], genericArguments[1], context, path);
return;
}

And this bit here traverses an object that implements the non-generic IDictionary:

YamlDotNet/YamlDotNet/Serialization/ObjectGraphTraversalStrategies/FullObjectGraphTraversalStrategy.cs

Lines 170 to 174 in 665b182

if (typeof(IDictionary).IsAssignableFrom(value.Type))
{
TraverseDictionary(value, visitor, typeof(object), typeof(object), context, path);
return;
}

Which looks like a fantastic idea to me... except that it actually checks the non-generic IDictionary first, and so it thinks that the key and value types for a Dictionary<TKey, TValue> are statically object instead of what they actually are, which causes problems for a .EnsureRoundTrip() serializer.

Here's a full repro, with a workaround below.

ConsoleApp0.csproj

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net6.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="YamlDotNet" Version="11.2.1" />
  </ItemGroup>

</Project>

Program.cs

using YamlDotNet.Serialization;

var serializer = new SerializerBuilder()
    .EnsureRoundtrip()
    .Build();

using StringWriter stringWriter = new();
serializer.Serialize(stringWriter, new MyOuterThing(), typeof(MyOuterThing));
Console.WriteLine(stringWriter);

public sealed class MyOuterThing
{
    public Dictionary<int, Payload> Lookups { get; set; } = new() { [1] = new() { I = 1 } };
}

public sealed class Payload
{
    public int I { get; set; }
}

Expected: should work
Actual: doesn't, exception:

YamlDotNet.Core.YamlException: (Line: 1, Col: 1, Idx: 0) - (Line: 1, Col: 1, Idx: 0): Cannot serialize type 'Payload' where a 'System.Object' was expected because no tag mapping has been registered for 'Payload', which means that it won't be possible to deserialize the document.
Register a tag mapping using the SerializerBuilder.WithTagMapping method.

E.g: builder.WithTagMapping("!Payload", typeof(Payload));
   at YamlDotNet.Serialization.EventEmitters.TypeAssigningEventEmitter.AssignTypeIfNeeded(ObjectEventInfo eventInfo)
   at YamlDotNet.Serialization.EventEmitters.TypeAssigningEventEmitter.Emit(MappingStartEventInfo eventInfo, IEmitter emitter)
   at YamlDotNet.Serialization.ObjectGraphVisitors.AnchorAssigningObjectGraphVisitor.VisitMappingStart(IObjectDescriptor mapping, Type keyType, Type valueType, IEmitter context)
   at YamlDotNet.Serialization.ObjectGraphVisitors.ChainedObjectGraphVisitor.VisitMappingStart(IObjectDescriptor mapping, Type keyType, Type valueType, IEmitter context)
   at YamlDotNet.Serialization.ObjectGraphTraversalStrategies.FullObjectGraphTraversalStrategy.TraverseProperties[TContext](IObjectDescriptor value, IObjectGraphVisitor`1 visitor, TContext context, Stack`1 path)
   at YamlDotNet.Serialization.ObjectGraphTraversalStrategies.RoundtripObjectGraphTraversalStrategy.TraverseProperties[TContext](IObjectDescriptor value, IObjectGraphVisitor`1 visitor, TContext context, Stack`1 path)
   at YamlDotNet.Serialization.ObjectGraphTraversalStrategies.FullObjectGraphTraversalStrategy.TraverseObject[TContext](IObjectDescriptor value, IObjectGraphVisitor`1 visitor, TContext context, Stack`1 path)
   at YamlDotNet.Serialization.ObjectGraphTraversalStrategies.FullObjectGraphTraversalStrategy.Traverse[TContext](Object name, IObjectDescriptor value, IObjectGraphVisitor`1 visitor, TContext context, Stack`1 path)
   at YamlDotNet.Serialization.ObjectGraphTraversalStrategies.FullObjectGraphTraversalStrategy.TraverseDictionary[TContext](IObjectDescriptor dictionary, IObjectGraphVisitor`1 visitor, Type keyType, Type valueType, TContext context, Stack`1 path)
   at YamlDotNet.Serialization.ObjectGraphTraversalStrategies.FullObjectGraphTraversalStrategy.TraverseObject[TContext](IObjectDescriptor value, IObjectGraphVisitor`1 visitor, TContext context, Stack`1 path)
   at YamlDotNet.Serialization.ObjectGraphTraversalStrategies.FullObjectGraphTraversalStrategy.Traverse[TContext](Object name, IObjectDescriptor value, IObjectGraphVisitor`1 visitor, TContext context, Stack`1 path)
   at YamlDotNet.Serialization.ObjectGraphTraversalStrategies.FullObjectGraphTraversalStrategy.TraverseProperties[TContext](IObjectDescriptor value, IObjectGraphVisitor`1 visitor, TContext context, Stack`1 path)
   at YamlDotNet.Serialization.ObjectGraphTraversalStrategies.RoundtripObjectGraphTraversalStrategy.TraverseProperties[TContext](IObjectDescriptor value, IObjectGraphVisitor`1 visitor, TContext context, Stack`1 path)
   at YamlDotNet.Serialization.ObjectGraphTraversalStrategies.FullObjectGraphTraversalStrategy.TraverseObject[TContext](IObjectDescriptor value, IObjectGraphVisitor`1 visitor, TContext context, Stack`1 path)
   at YamlDotNet.Serialization.ObjectGraphTraversalStrategies.FullObjectGraphTraversalStrategy.Traverse[TContext](Object name, IObjectDescriptor value, IObjectGraphVisitor`1 visitor, TContext context, Stack`1 path)
   at YamlDotNet.Serialization.ObjectGraphTraversalStrategies.FullObjectGraphTraversalStrategy.YamlDotNet.Serialization.IObjectGraphTraversalStrategy.Traverse[TContext](IObjectDescriptor graph, IObjectGraphVisitor`1 visitor, TContext context)
   at YamlDotNet.Serialization.SerializerBuilder.ValueSerializer.SerializeValue(IEmitter emitter, Object value, Type type)
   at YamlDotNet.Serialization.Serializer.EmitDocument(IEmitter emitter, Object graph, Type type)
   at YamlDotNet.Serialization.Serializer.Serialize(IEmitter emitter, Object graph, Type type)
   at YamlDotNet.Serialization.Serializer.Serialize(TextWriter writer, Object graph, Type type)
   at Program.<Main>$(String[] args) in C:\REDACTED\Program.cs:line 8

I'm able to work around it by using a subclass that does the generic type check first before falling back to the base, which looks like this:

Program.cs

using YamlDotNet.Serialization;
using YamlDotNet.Serialization.NamingConventions;
using YamlDotNet.Serialization.ObjectGraphTraversalStrategies;

var serializer = new SerializerBuilder()
    .EnsureRoundtrip()
    .WithObjectGraphTraversalStrategyFactory((typeInspector, typeResolver, typeConverters, maximumRecursion) => new CustomFullObjectGraphTraversalStrategy(typeInspector, typeResolver, maximumRecursion, CamelCaseNamingConvention.Instance))
    .Build();

using StringWriter stringWriter = new();
serializer.Serialize(stringWriter, new MyOuterThing(), typeof(MyOuterThing));
Console.WriteLine(stringWriter);

public sealed class MyOuterThing
{
    public Dictionary<int, Payload> Lookups { get; set; } = new() { [1] = new() { I = 1 } };
}

public sealed class Payload
{
    public int I { get; set; }
}

public sealed class CustomFullObjectGraphTraversalStrategy : FullObjectGraphTraversalStrategy
{
    public CustomFullObjectGraphTraversalStrategy(ITypeInspector typeDescriptor, ITypeResolver typeResolver, int maxRecursion, INamingConvention namingConvention)
        : base(typeDescriptor, typeResolver, maxRecursion, namingConvention)
    {
    }

    protected override void TraverseObject<TContext>(IObjectDescriptor value, IObjectGraphVisitor<TContext> visitor, TContext context, Stack<ObjectPathSegment> path)
    {
        if (value.Type.IsGenericType && value.Type.GetInterfaces().Any(i => i.IsGenericType && i.GetGenericTypeDefinition() == typeof(IDictionary<,>)))
        {
            var genericArguments = value.Type.GetGenericArguments();
            TraverseDictionary(new ObjectDescriptor(value.Value, value.Type, value.StaticType, value.ScalarStyle), visitor, genericArguments[0], genericArguments[1], context, path);
        }
        else
        {
            base.TraverseObject(value, visitor, context, path);
        }
    }
}
@aaubry aaubry mentioned this issue Apr 29, 2022
Closed
@airbreather airbreather mentioned this issue Feb 9, 2024
Merged
EdwardCooke added a commit that referenced this issue May 11, 2024
@EdwardCooke
Merge pull request #904 from airbreather/fix-656
22070fd 
Add a regression test for #656

+semver:fix
@aaubry
Copy link
Owner

aaubry commented May 11, 2024

A fix for this issue has been released in version 15.1.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
question
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add a regression test for #656 airbreather/YamlDotNet
3 participants
@aaubry @airbreather @EdwardCooke