bchavez / Bogus

:card_index: A simple fake data generator for C#, F#, and VB.NET. Based on and ported from the famed faker.js.
Other
8.81k stars 500 forks source link

Support c# 9 records #334

Closed Trolldemorted closed 3 years ago

Trolldemorted commented 3 years ago

Faker only works for types with a parameterless contructor (throwing No parameterless constructor defined for type 'WASP.Models.Attack'. (System.MissingMethodException) otherwise).

This makes it impossible to use it with record types which don't have a default constructor. Other dotnet libraries (json parsers, ef core, ...) are able to use the full constructors, so there must be a decent way to do so, could you adapt that?

bchavez commented 3 years ago

Hi @Trolldemorted,

Thank you for your issue. What kind of C# 9 record support are you specifically looking for from Bogus? Seems C# 9 records work reasonably well. See below:

void Main()
{
   var dogFaker = new Faker<Dog>()
                  .CustomInstantiator( f =>
                     new Dog(
                        f.Name.FirstName(),
                        f.Name.LastName()
                      )
                  );

   dogFaker.Generate(5).Dump(nameof(dogFaker));

   var catFaker = new Faker<Cat>()
                  .CustomInstantiator(f =>
                     new Cat{
                        FirstName = f.Name.FirstName(),
                        LastName = f.Name.LastName()
                     });

   catFaker.Generate(5).Dump(nameof(catFaker));

   var catFaker2 = new Faker<Cat>()
               .RuleFor(u => u.FirstName, f => f.Name.FirstName())
               .RuleFor(u => u.LastName, f => f.Name.LastName());

   catFaker2.Generate(5).Dump(nameof(catFaker2));
}

public record Dog(string FirstName, string LastName, string Sound = "bark! bark!");
public record Cat
{
   public string FirstName {get; init;}
   public string LastName {get; init;}
   public string Sound {get; } = "prrr! meow!";
}

image

Trolldemorted commented 3 years ago

It is nice to see that it works with CustomInstantiators! I tried with RuleFor and it didn't:

using Bogus;
using System;

namespace BogusRecords
{
    record Dog(string Name);

    class Program
    {
        static void Main()
        {
            var dog = new Faker<Dog>("de")
                .RuleFor(d => d.Name, (f, d) => f.Name.FindName())
                .Generate();
            Console.WriteLine(dog);
        }
    }
}

This code throws System.MissingMethodException: 'No parameterless constructor defined for type 'BogusRecords.Dog'.'

bchavez commented 3 years ago

Hi @Trolldemorted,

The issue is a little more complex for C# positional record types with .RuleFor because according to Mads the C# language designer:

We deliberately do not support an official way to distinguish records from non-records, within the language or with reflection. ... Ideally records should just be very intelligent sugar for classes that people could have written themselves. This is 90% true in C# 9.0 and may well be 100% true in C# 10. Exposing a runtime way to check what syntax someone chose for their implementation just invites brittleness. https://twitter.com/Hamed__Fathi/status/1326809779319214080

So, there's no official way in .NET to ascertain if a given type T is a record or not via .NET reflection. This means, there's no official way for Bogus to determine if type T is a record or not. This is the crux of the issue for Bogus. Without this determination, there isn't much I can do to make your example work under-the-hood with Faker<T>:

record Dog(string Name);

var dog = new Faker<Dog>()
   .RuleFor(d => d.Name, f => f.Name.FindName())
   .Generate();

The good news is, there is some discussion going on at Microsoft to include a .IsRecord property in .NET's reflection metadata, (see here and here, SO here), but until that is officially implemented; for the time being, you'll have to rely on some hacks to get C# positional record types to work with .RuleFor without calling .CustomInstantiator explicitly.

Now, on to the workarounds...

Technically, there are some hacky ways to detect if type T is a record. One way is probing for a compiler-generated method called <Clone>$. The Roslyn compiler lowering of a C# positional record for Cat in .NET 5.0 is shown below:

public record Cat(string Name)

image

~Unfortunately, there is a very small edge case where probing for <Clone>$ might not work. For example, LinqPad 6 generates a record with a <>Clone method name as shown below:~

LINQPad6_4067

~So keeping in mind this hack,...~ (:point_up: this was because LINQPad6 used a preview version of the Roslyn compiler.)

The following workarounds rely on extending Bogus for your particular situation. You'll have to choose the best that fits your implementation style:

Option A: Using an .WithRecord extension method

void Main()
{   
   var catFaker = new Faker<Cat>()
                      .WithRecord()
                      .RuleFor( c => c.Name, f => f.Name.FindName());

   catFaker.Generate(5).Dump();
}

public record Cat(string Name);

public static class ExtensionsForBogus
{
   public static Faker<T> WithRecord<T>(this Faker<T> faker) where T : class
   {
      faker.CustomInstantiator( _ => RuntimeHelpers.GetUninitializedObject(typeof(T)) as T );
      return faker;
   }
}

image


Option B: Using an explicit RecordFaker<T> for only record types

void Main()
{
   var catFaker = new RecordFaker<Cat>()
                      .RuleFor(c => c.Name, f => f.Name.FindName());

   catFaker.Generate(5).Dump();
}

public record Cat(string Name);

public class RecordFaker<T> : Faker<T> where T : class
{
   public RecordFaker()
   {
      this.CustomInstantiator(_ => RuntimeHelpers.GetUninitializedObject(typeof(T)) as T);
   }
}

image


Option C: Using automatic detection with Faker2

void Main()
{
   var catFaker = new Faker2<Cat>()
                      .RuleFor(c => c.Name, f => f.Name.FindName());

   catFaker.Generate(5).Dump();
}

public record Cat(string Name);

public class Faker2<T> : Faker<T> where T : class
{
   public Faker2()
   {
      if (this.IsRecord())
      {
         this.CustomInstantiator(_ => RuntimeHelpers.GetUninitializedObject(typeof(T)) as T);
      }
   }

   private bool IsRecord(){
      var methods = typeof(T).GetMethods().Select( m => m.Name).ToArray();

      foreach (var name in methods)
      {
         if( name.Contains('<') && name.Contains('>') && name.Contains("clone", StringComparison.OrdinalIgnoreCase))
            return true;
      }
      return false;
   }
}

image


As for "official" support in Bogus, we'll have to wait for .NET to carry a proper .IsRecord flag in reflection. The very best I can do is maybe provide some kind of extension method similar to Option A for edgy cases like this.

I hope this helps.

Thanks, Brian

Trolldemorted commented 3 years ago

Thanks for the detailed response! For now, using CustomInstantiator will work nicely for us.

Do you know how System.Text.Json and EF Core are handling this? They can construct those records, my first guess was they are assuming that the public properties can be set in a constructor which takes one argument per property.

bchavez commented 3 years ago

Hi @Trolldemorted,

No problem. I'm glad I could help.

I'm not sure how System.Text.Json or EF Core do it. I think you're correct, I'm guessing they probably have some generalized creational object factory that injects values into an object's or record's constructor since constructor parameter argument names match up nicely with property names.

Basyras commented 3 years ago

I would like to add my 5 cents here.

I think this is not related just for new record feature but any class / struct without parameterless constructor (where we can't add parameterless constructor for many reasons, eg. not having access to the class).

We can use CustomInstantiator() as workaround but we loose almost every feature that Bogus provides to us (+ it gets quite annoying to write .CustomInstantiator(x=>new MyClass(null,default,null,default)) over and over again)

I would probably suggest creating new method in Faker<T> called SkipContructor() wich would set CreateActions[Default] to FormatterServices.GetUninitializedObject.

One may argue that skipping constructor is not best practice in normal world (or can even break stuff) but Bogus is already kinda hacking when fieldInfo.SetValue() is used for every RuleFor() (PopulateAction) wich use hidden setters (for example every setter of property in records 😉)

I would keep throwing No parameterless constructor defined for type as default behaivor (By throwing i mean using Activator.CreateInstance() as default CreateActions[Default] ) because constructor could have additional steps necessary for creating a class

@bchavez If you don't have any issues with my suggestions i can make PR for that (probably today).

bchavez commented 3 years ago

@Basyras,

I think adding a method .SkipConstructor() probably makes sense. I would probably accept a PR for this. Just make sure there are no changes to the TFMs or any uses of preprocessor directives. Please make as few changes as possible.

Also, at some point (not right now), I'd like to deprecate .CustomInstantiator() and rename it to .UseConstructor() . In essence, if we think about it:

However, what I would probably not accept is automatically calling .SkipConstructor() when T is a record. I would need to think about this situation more. As I mentioned earlier, there is no .NET .IsRecord in Reflection; so we'd have to rely on a <Clone>$ hack. I'm very wary of making too much "automagic" in Faker<T>, especially when using a hack for detecting record types.

Sharaf-Mansour commented 3 years ago

@Trolldemorted I think an easy way to fix this is just by implementing the default constructor yourself like this

public record Dog(string Name)
{
    public Dog() : this(Name: default)
    {

    }
}

and that is it. This will just work fine..

var dog = new Faker<Dog>()
    .RuleFor(d => d.Name, f => f.Name.FindName())
    .Generate();

Extra..

public record Person(string Name, int Age)
{
    public Person() : this(default, default) { }
}

and to allow setters

public record Person(string Name, int Age)
{
    public Person() : this(default, default) { }
    public string Name { get; set; } = Name;
    public int Age { get; set; } = Age;
}

Hope this helps.

PureKrome commented 1 year ago

Just stumbled onto this.. :(

public record Foo(int Id);

var foo = new Faker<Foo>().Generate();

System.MissingMethodException : Cannot dynamically create an instance of type 'Foo'. Reason: No parameterless constructor defined.

I have nearly everything defined as record ...

Crying.A.Lot. 😿

EDIT 1:

also, I don't know how @bchavez got it to work in his first reply (2nd post, above) with .RuleFor example.

   var catFaker2 = new Faker<Cat>()
               .RuleFor(u => u.FirstName, f => f.Name.FirstName())
               .RuleFor(u => u.LastName, f => f.Name.LastName());

   catFaker2.Generate(5).Dump(nameof(catFaker2));

I tried:

var foo = new Faker<Foo>()
    .RuleFor(x => x.Id, f => f.Random.Int())
    .Generate(); 

and that still threw an exception.

davidandersen commented 1 year ago

Could it be that the single line declaration/constructor of Foo removes the parameterless constructor? If something like below is used, it should work.

public record Foo
{
    public int Id { get; set; }
}

Thanks!