nickdodd79 / AutoBogus

A C# library complementing the Bogus generator by adding auto creation and population capabilities.
MIT License
431 stars 50 forks source link

ImmutableDictionary regression in 2.7 #26

Closed nycdotnet closed 4 years ago

nycdotnet commented 4 years ago

Hello,

In 2.7, AutoBogus throws an exception when using AutoFaker.Generate<T>() with classes that expose an ImmutableDictionary<T,U> as a public property. In 2.6, such classes used to be able to generate without having to skip these properties - they would just be null.

This also seems to happen with ReadOnlyDictionary<T,U>, which is a regression - in 2.6, a ReadOnlyDictionary would work and be faked correctly.

The regression can be worked-around by calling WithSkip in AutoFaker.Configure.

The below console app demonstrates the issue. If you install the 2.6.0 AutoBogus package, it runs and produces the below output (the immutable dictionary is left as null). If you update to 2.7.0 or beyond, it crashes with one of the exceptions below. Skipping both the ReadOnlyDictionary and ImmutableDictionary works around the problem.

SomeStringProperty = JSON
SomeReadOnlyDictionary = System.Collections.ObjectModel.ReadOnlyDictionary`2[System.String,System.String]
SomeReadOnlyDictionary Count = 3
SomeImmutableDictionary =
SomeImmutableDictionary Count =

In 2.7.0 and after, it crashes with an exception. Even if you skip the immutable dictionary property, it still crashes when trying to call Add.

Example of exception for converting to ReadOnlyDictionary<T,U>:

System.ArgumentException
  HResult=0x80070057
  Message=Object of type 'System.Collections.Generic.Dictionary`2[System.String,System.String]' cannot be converted to type 'System.Collections.ObjectModel.ReadOnlyDictionary`2[System.String,System.String]'.
  Source=System.Private.CoreLib
  StackTrace:
   at System.RuntimeType.TryChangeType(Object value, Binder binder, CultureInfo culture, Boolean needsSpecialCast)
   at System.RuntimeType.CheckValue(Object value, Binder binder, CultureInfo culture, BindingFlags invokeAttr)
   at System.Reflection.MethodBase.CheckArguments(Object[] parameters, Binder binder, BindingFlags invokeAttr, CultureInfo culture, Signature sig)
   at System.Reflection.RuntimeMethodInfo.InvokeArgumentsCheck(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Reflection.RuntimePropertyInfo.SetValue(Object obj, Object value, BindingFlags invokeAttr, Binder binder, Object[] index, CultureInfo culture)
   at System.Reflection.RuntimePropertyInfo.SetValue(Object obj, Object value, Object[] index)
   at AutoBogus.AutoMember.<>c__DisplayClass0_0.<.ctor>b__1(Object obj, Object value)
   at AutoBogus.AutoBinder.PopulateInstance[TType](Object instance, AutoGenerateContext context, IEnumerable`1 members)
   at AutoBogus.Generators.TypeGenerator`1.AutoBogus.IAutoGenerator.Generate(AutoGenerateContext context)
   at AutoBogus.AutoGenerateContextExtensions.Generate[TType](AutoGenerateContext context)
   at AutoBogus.AutoFaker.AutoBogus.IAutoFaker.Generate[TType](Action`1 configure)
   at AutoBogus.AutoFaker.Generate[TType](Action`1 configure)
   at AutoBogusImmutableIssue.Program.Main(String[] args) in C:\Users\SteveOgnibene\source\repos\AutoBogusImmutableIssue\AutoBogusImmutableIssue\Program.cs:line 21

Example of exception for converting to ImmutableDictionary<T,U>:

System.ArgumentException
  HResult=0x80070057
  Message=Object of type 'System.Collections.Generic.Dictionary`2[System.String,System.String]' cannot be converted to type 'System.Collections.Immutable.ImmutableDictionary`2[System.String,System.String]'.
  Source=System.Private.CoreLib
  StackTrace:
   at System.RuntimeType.TryChangeType(Object value, Binder binder, CultureInfo culture, Boolean needsSpecialCast)
   at System.RuntimeType.CheckValue(Object value, Binder binder, CultureInfo culture, BindingFlags invokeAttr)
   at System.Reflection.MethodBase.CheckArguments(Object[] parameters, Binder binder, BindingFlags invokeAttr, CultureInfo culture, Signature sig)
   at System.Reflection.RuntimeMethodInfo.InvokeArgumentsCheck(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Reflection.RuntimePropertyInfo.SetValue(Object obj, Object value, BindingFlags invokeAttr, Binder binder, Object[] index, CultureInfo culture)
   at System.Reflection.RuntimePropertyInfo.SetValue(Object obj, Object value, Object[] index)
   at AutoBogus.AutoMember.<>c__DisplayClass0_0.<.ctor>b__1(Object obj, Object value)
   at AutoBogus.AutoBinder.PopulateInstance[TType](Object instance, AutoGenerateContext context, IEnumerable`1 members)
   at AutoBogus.Generators.TypeGenerator`1.AutoBogus.IAutoGenerator.Generate(AutoGenerateContext context)
   at AutoBogus.AutoGenerateContextExtensions.Generate[TType](AutoGenerateContext context)
   at AutoBogus.AutoFaker.AutoBogus.IAutoFaker.Generate[TType](Action`1 configure)
   at AutoBogus.AutoFaker.Generate[TType](Action`1 configure)
   at AutoBogusImmutableIssue.Program.Main(String[] args) in C:\Users\SteveOgnibene\source\repos\AutoBogusImmutableIssue\AutoBogusImmutableIssue\Program.cs:line 11

Demo program:

using AutoBogus;
using System;
using System.Collections.Immutable;
using System.Collections.ObjectModel;

namespace AutoBogusImmutableIssue
{
    class Program
    {
        static void Main(string[] args)
        {
            //AutoFaker.Configure(builder =>
            //{
              //  builder
              //  .WithSkip<MyClass>(mc => mc.SomeImmutableDictionary)
              //  .WithSkip<MyClass>(mc => mc.SomeReadOnlyDictionary)
             //   ;
            //});

            var myInstance = AutoFaker.Generate<MyClass>();
            Console.WriteLine($"{nameof(MyClass.SomeStringProperty)} = {myInstance.SomeStringProperty}");
            Console.WriteLine($"{nameof(MyClass.SomeReadOnlyDictionary)} = {myInstance.SomeReadOnlyDictionary}");
            Console.WriteLine($"{nameof(MyClass.SomeReadOnlyDictionary)} Count = {myInstance.SomeReadOnlyDictionary?.Count}");
            Console.WriteLine($"{nameof(MyClass.SomeImmutableDictionary)} = {myInstance.SomeImmutableDictionary}");
            Console.WriteLine($"{nameof(MyClass.SomeImmutableDictionary)} Count = {myInstance.SomeImmutableDictionary?.Count}");

        }
    }

    public class MyClass
    {
        public string SomeStringProperty { get; set; }
        public ReadOnlyDictionary<string, string> SomeReadOnlyDictionary { get; set; }
        public ImmutableDictionary<string, string> SomeImmutableDictionary { get; set; }
    }
}

I'm willing to give it a shot to fix this if you can come up with a good idea. I can try bisecting if you think that would be valuable. Thanks for this great library.

nickdodd79 commented 4 years ago

Hi @nycdotnet

Thanks for the details above. I am surprised you are seeing an issue with ReadOnlyDictionary. That type should be handled, so I will need to investigate further.

With the ImmutableDictionary that could possibly be a special case that doesn't follow the supported collection interfaces. Again, I will need to investigate to see why.

Watch this space :smile:

Nick.

nycdotnet commented 4 years ago

Hi @nickdodd79 - this is still an issue for us. Did you like the behavior of how ReadOnlyDictionary was faked in 2.6? If so, I can take a crack at restoring that and making it work with ImmutableDictionary and friends.

nickdodd79 commented 4 years ago

Hey @nycdotnet

Apologies for not getting a fix in place. There have been several events, commitments and deadlines I have had that have stolen my time. I have some spare time over the next few weeks, so I am hoping to resolve this for you.

Thanks for your patience. Nick.

nycdotnet commented 4 years ago

No worries at all. I totally understand. We have reverted back to 2.6 on all our projects for now, and it’s meeting our needs very well. Thanks again for this great library. Happy Holidays.

kevin-david commented 4 years ago

Just wanted to point out that this same regression exists with IReadOnlyDictionary<T,U> members as well. Hopefully, the fix is the same, if not very similar.

nickdodd79 commented 4 years ago

Hey @kevin-david

I have now got IReadOnlyDictionary<T,U> working again. Turns out some of my tests weren't executing which would have highlighted the issue. A new version will be available shortly.

Hey @nycdotnet

I can't get ImmutableDictionary to work as expected because the AutoBogus code runs on .NET Standard, but ImmutableDictionary seems to only be available in .NET Core.

https://docs.microsoft.com/en-us/dotnet/api/system.collections.immutable.immutabledictionary-2?view=netcore-2.0

I do have plans for a big refactor of AutoBogus (v3) to cater for this, which I am currently working on.

However, I did wonder how you managed to get it working in v2.6, so I checked out that version of the code to run some tests. Do your properties populate as expected, because when I run my test code, the ImmutableDictionary property is always null. As far as I can tell ImmutableDictionary instances need to be created using static factory methods on the class (which AutoBogus doesn't know about to create an instance).

Nick.

nycdotnet commented 4 years ago

@nickdodd79 it works in 2.6 precisely because this version does not attempt to populate them. We slap another abstraction on top of AutoBogus where we generate a bogus object and then add the immutable properties ourselves after.

You're correct about the immutable factories, but one of the refinements in recent versions is the extension methods in System.Collections.Immutable on the cousin mutable collections - so you can call .ToImmutableDictionary() on a Dictionary<T,U> and get the same effect as using the static builder at a minor performance cost but way simpler code, or .ToImmutableList(), .ToImmutableHashSet(), etc.

Thanks again for this great library and effort, and happy new year.

nycdotnet commented 4 years ago

Just to say out loud, I was thinking about this issue the other day - and I was expecting that the immutable collections being in their own separate package to actually be a problem. To add to the complexity, there are even multiple versions of the immutable libraries and they're continuing to refine them, so I think it might be worse if AutoBogus were to pick/require a specific version. I was actually wondering if there might be some sort of way to use reflection to call this stuff without requiring a reference? Given that immutable collections are CPU intensive already for steady-state ops, and that AutoBogus is probably mostly a library used for testing (though not specifically limited to that) I wonder if this might be a possible solution since for this library it seems to me that performance is important, but flexibility/functionality is probably more important.

nickdodd79 commented 4 years ago

OK. In that case I think I have a quick fix for 2.7 where we can wrap the binding operations in the try..catch. That will prevent the error from occurring, but will mean the ImmutableDictionary property has a value of null.

I am currently creating v3 which will handle stuff like this better.

nycdotnet commented 4 years ago

@nickdodd79 that would be fine and would make the fixed 2.7 have the same behavior as 2.6 does for immutables.

nickdodd79 commented 4 years ago

Hey all,

I have now released v2.7.4 which should address this issue.

Nick.

nickdodd79 commented 4 years ago

Hey @nycdotnet

Just wanted to follow up and see how the new version is going? Can this issue be closed?

Nick.

nycdotnet commented 4 years ago

we can close and will reopen if not. Thank you!!