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

Using Faker[T] with private constructor and private property setter #213

Closed
hkakutalua opened this issue Mar 24, 2019 · 8 comments
Closed

Comments

@hkakutalua
Copy link

Currently is not possible to generate data on types with a private constructor and/or private properties.

It would be cool if Bogus use .NET reflection to solve this.
I usually create types that are not supposed to be instantiated and modified in client code, because i delegate this process to Entity Framework.

But for the purposes of unit/integration tests is very important to generate data on those types without compromising on the object-oriented design around them.

I eager to contribute in this if possible.
Best regards.

@bchavez
Copy link
Owner

bchavez commented Mar 24, 2019

Hi Henrick,

Thank you for your question. As you might know, working with private properties via reflection is a brittle endeavor, so Bogus out of the box (at least right now) doesn't have any plans to support this scenario anytime soon. However, that shouldn't stop you from digging around the internals of Bogus and pushing Bogus to make it work the way you want!

Here's how I did it. The following unit test passes:

class Thing
{
   private int Bar { get; set; } = 0;
   private int Qux { get; set; } = 0;

   private Thing()
   {
   }

   public int GetBar() => this.Bar;
   public int GetQux() => this.Qux;
}

public class PrivateFaker<T> : Faker<T> where T : class 
{
   public PrivateFaker<T> UsePrivateConstructor()
   {
      return base.CustomInstantiator(f => Activator.CreateInstance(typeof(T), nonPublic: true) as T)
         as PrivateFaker<T>;
   }

   public PrivateFaker<T> RuleForPrivate<TProperty>(string propertyName, Func<Faker, TProperty> setter)
   {
      base.RuleFor(propertyName, setter);
      return this;
   }
}

[Fact]
public void Test()
{
   var faker = new PrivateFaker<Thing>()
      .UsePrivateConstructor()
      .RuleForPrivate("Bar", f => f.Random.Int())
      .RuleForPrivate("Qux", f => f.Random.Int());

   var fakeThing = faker.Generate();

   fakeThing.GetBar().Should().NotBe(0);
   fakeThing.GetQux().Should().NotBe(0);
   console.Dump(fakeThing.GetBar());
   console.Dump(fakeThing.GetQux());
}

//OUTPUT:
1296054233
-1749342366

Let me know if that works for you.

Also, if there is anything I can do to make your process easier to use with EF without compromising the over arching design goals and principals inside Bogus, please let me know so we can discuss how to make those changes.

Thanks,
Brian

@hkakutalua
Copy link
Author

That's really an awesome way to achieve what i want. it would be really cool if Bogus directly gave this option through something quite similar with your example.

I wanted to write "generate data on private setters, but anyway, the code should work in this scenario too.

Which kind of brittleness are your referring when working with reflection? We can always avoid with static typing anyway.

Thanks

@bchavez
Copy link
Owner

bchavez commented Mar 25, 2019

Ah, okay, if we are only dealing with private setters, then the solution gets even easier. The following example removes the need for PrivateFaker<T> sub-classing inheritance. Check it out:

class Thing
{
   public int Bar { get; private set; } = 0;
   public int Qux { get; private set; } = 0;

   private Thing()
   {
   }
}

public static class CustomFakerExtensions{
   public static Faker<T> UsePrivateConstructor<T>(this Faker<T> faker) where T : class
   {
      return faker.CustomInstantiator(f => Activator.CreateInstance(typeof(T), nonPublic: true) as T);
   }
}

[Fact]
public void Test()
{
   var faker = new Faker<Thing>()
                    .UsePrivateConstructor()
                    .RuleFor(t => t.Bar, f => f.Random.Int())
                    .RuleFor(t => t.Qux, f => f.Random.Int());

   var fakeThing = faker.Generate();

   fakeThing.Bar.Should().NotBe(0);
   fakeThing.Qux.Should().NotBe(0);
}

Originally, I was referring to the non-typed "magic strings" for property names. For example, .RuleForPrivate("Bar", f => f.Random.Int()) in my last example. Magic strings like "Bar" can get out of sync with models pretty quickly. This was the brittle process I was referring to.

🚗 🚙 "Let the good times roll..."

@bchavez
Copy link
Owner

bchavez commented Mar 25, 2019

Hey Henrick,

I'm going to close the issue for now. I think the custom extension method approach is a decent solution for this particular scenario.

If I get more requests to support this scenario out-of-the-box, I'll certainly consider adding it to the official API. However, right now I'm a little OCD w.r.t the fluent API surface. Now-a-days, I don't change the fluent API unless there are repeated requests for such a change.

I will keep your scenario in mind for future requests.

Also, thanks again for submitting your question/feedback!
Brian

💼 👔 "Taking care of business every day... Taking care of business every way..."

@bchavez bchavez closed this as completed Mar 25, 2019
@bchavez bchavez changed the title Use reflection to generate data Using Faker[T] with private constructor and private property setter Mar 25, 2019
@lonix1
Copy link

lonix1 commented May 3, 2019

@bchavez +1 this would be great functionality, because:

  • agree with OP that it's common when using private ctors and EF
  • when using Domain Driven Design the norm is to never expose public ctors as that leaves a domain object in an unvalidated state (objects should be valid, always)
  • old-school OOP / defensive programming / best practices have said for decades that we only expose ctors when absolutely necessary

So I use private classes, private ctors, and "private all the things" - and only expose when necessary.

BUT, I agree with you that reflection done poorly is very brittle. However we need not worry about that in this case, as there are no magic strings in this solution.

The use case from the discussion above: a (DDD style) class with a private default ctor:

public class User {
  private User() { }
  public User(string name, string phone) { Name = name; Phone = phone; }
  public string Name { get; }
  public string Phone { get; }
  public void update(string name, string phone) { /* ... */ }
}

I used your example and some stuff I found in the source, for this much simpler approach (please let me know if you spot a problem with it, I'm not familiar with your code):

using System;

namespace Bogus {

  public class PrivateFaker<T> : Faker<T> where T : class {

    public PrivateFaker() {
      this.CreateActions[Default] = fakerOfT =>
        base.CustomInstantiator(f => Activator.CreateInstance(typeof(T), nonPublic: true) as T)
        as PrivateFaker<T>;
    }

  }
}

Which makes it easier to use:

new PrivateFake<User>()
  .StrictMode(true)
  .RuleFor(p => p.Name, r => r.Person.FullName)
  .RuleFor(p => p.Phone, r => r.Person.Phone)
  .Generate();

If you one day decide to allow private constructors, the best approach would be to just add a constructor overload to Faker<T>:

public Faker(bool usePrivateConstructor = false) {
  if (!usePrivateConstructor) throw new Exception("Use the default constructor.");
  this.CreateActions[Default] = fakerOfT =>
    base.CustomInstantiator(f => Activator.CreateInstance(typeof(T), nonPublic: true) as T)
    as Faker<T>;
}

This API ensures that it is opt-in and would fail-fast if not used properly.

There are ways to make it so the User class doesn't even have to define a private default ctor. But that's a topic for discussion when/if such functionality is added. I'm using that approach BTW.

@bchavez
Copy link
Owner

bchavez commented May 3, 2019

Hi @lonix1,

Thanks for the feedback!

In general, I don't see a problem with your PrivateFaker<T> per se. In this case, I think sub-classing Faker<T> fine if you need to encapsulate logic that should get executed before the other chained calls.

By design, I've tried to expose much of the internals of Faker<T> to inheritors as much as I can (without affecting public APIs) to allow those who need custom behavior to easily override the internal workings of Faker<T>.

However, strictly speaking, the code samples provided above might not work with User as-is because the User's properties only have getters. By default, read-only properties won't be set by Bogus using the default Binder. I ran your code in LINQPad and here are the results I got:

void Main()
{
   var faker = new PrivateFaker<User>()
        .StrictMode(true)
        .RuleFor(p => p.Name, r => r.Person.FullName)
        .RuleFor(p => p.Phone, r => r.Person.Phone);

   faker.Generate(3).Dump();;
}

public class PrivateFaker<T> : Faker<T> where T : class
{
   public PrivateFaker()
   {
      this.CreateActions[Default] = fakerOfT => 
         base.CustomInstantiator(f => Activator.CreateInstance(typeof(T), nonPublic: true) as T);
   }
}

public class User
{
   private User() { }
   public User(string name, string phone) {
      Name = name;
      Phone = phone;
   }
   public string Name { get; }
   public string Phone { get; }
   public void update(string name, string phone) { /* ... */ }
}

LINQPad_3033

You'll have to provide Name and Phone with internal set;ers if we really want this to work.

If internal is not an option, IIRC, you'll need a custom IBinder that reflects deeper into User's member backing fields to get this to work. Even then, I'm not sure it could work because backing fields could have some weird names generated by the compiler like <Phone>k__BackingField. You'd have to override the internal .RuleFor mapping so that p => p.Phone ("Phone" property name) gets mutated into <Phone>k__BackingField for reflection to work. The customizations required to get Faker<T> working with User if we go down this path is do-able, but eh... I have some reservations about it because I think there could be a simpler way using a factory method (as I describe later in this post).

⚠️ Slight digression: Now, in specific cases like this, it could be interesting if a Faker<T>'s .RuleFor(...) rules could be mapped into the constructor following the same naming convention as defined in the constructor when creating T. 💭 💭 💭

image

This is just a neat idea. Little automagical, but still pretty neat.


So, for strongly well-defined types like User whose property values are fully injected in the constructor, let's not forget, it could be easier to use a creational factory method to create User objects as shown below:

void Main()
{
   var f = new Faker();
   var user = CreateUser(f);
   user.Dump();
}

public static User CreateUser(Faker f){
   return new User(f.Name.FullName(), f.Phone.PhoneNumber());
}

public class User
{
   private User() { }
   public User(string name, string phone)
   {
      Name = name;
      Phone = phone;
   }
   public string Name { get; }
   public string Phone { get; }
   public void update(string name, string phone) { /* ... */ }
}

Simple, but it works! StrictMode is implicitly enforced by the constructor and compiler too! 😄

Again, thanks for the feedback and your use-case examples. I really appreciate it. It does help me think about the design of everything.

⚡ 🏃 "I was struck by lighting. Walkin' down the street..."

@lonix1
Copy link

lonix1 commented May 3, 2019

Yeah those non-public setters are the problem... just checked my code and indeed they are not private... Oh well there are a few good approaches in this thread!

I take your point about exposing as much as possible, I've been pleasantly surprised how easy it is to extend this library because of all the extensibility points you've allowed.

That digression you posted is very interesting, and neat. I'ma look into that.


That last example: YEEHAW! Now that's what I'm talking about! I was trying so hard to make it work that I forgot you offer a non-fluent API too. So easy.

I've been having much fun playing with this library... thanks for making+maintaining it! 😄

@jonathh21
Copy link

So has anyone come up with a neat solution to this?

We use immutable types all over the show and have used faker as a base class in the form for Faker (using a little magic so that we can have a non public constructor) - I have never been hugely averse to make a constructor protected rather than private to facilitate this.

However as we moved to System.text.json we are trying to move to constructor initialisation, and with it no setters - the goal here is to remove the temptation to add methods to the request/command/et al that can mutate state - no setter - no temptation.

I typically tackle Testdtos by derrving from them, and am prepared to consider anything really in that derrived or wrapping backclass to give me what i need.

eg:

public class TestAccountDetailsDto : FakerThatAllowsNonPublicCtors<AccountDetailsDto>
{
    public TestAccountDetailsDto()
    {
        RuleFor(r => r.InvestorAccounts,
            () => new[] {new InvestorAccountsDto(InvestorAccountsDto.AccountStatus.Active)});
    }
}

then i just 'new' it up.

AccountDetailsDto = new TestAccountDetailsDto()

These days InvestorAccounts has no property set... so it doesn't work anymore.

Any suggestions for clean encapsulation appreciated.

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

No branches or pull requests

4 participants