soenneker / soenneker.utils.autobogus

The .NET Autogenerator
https://soenneker.com
MIT License
22 stars 2 forks source link

Private readonly fields - are overwritten/generated automatically #175

Closed rizi closed 1 month ago

rizi commented 1 month ago

Hi,

With this fix: #143, private readonly fields are now also set automatically, is there a way to prevent/controll this with some kind of configuration (per type)?

This can cause issues when the field is initilized via constructor, because afterwards it's getting overwritten bei Autobogus.

Here is a very simple sample.

public class AnotherObjectToFake
{
    private readonly string _key;

    public AnotherObjectToFake(string key)
    {
        _key = key;
    }

    public string GetKey()
    {
        return _key;
    }
}

//test class
 [TestMethod]
 public void PrivateReadOnlyField_Should_Not_Be_Overwritten()
 {
     //arrange
     const string key = "someKey";

     //act
     AnotherObjectToFake objectToFake = new AutoFaker<AnotherObjectToFake>()
         .CustomInstantiator(_ => new AnotherObjectToFake(key));

     //assert
     objectToFake.GetKey().Should().Be(key);
 } 

image

Br

soenneker commented 1 month ago

I looked into this... there doesn't seem to be a straightforward way of solving this.

CustomInstantiator builds a 'CreateAction' which is for the 'Create' action within Bogus, and 'Finalize' is used to populate after the variables have been created with AutoBogus.

Since it's private, a rule can't be created within Faker. An override could be a solution that works now.

Other options:

rizi commented 1 month ago

I looked into this... there doesn't seem to be a straightforward way of solving this.

CustomInstantiator builds a 'CreateAction' which is for the 'Create' action within Bogus, and 'Finalize' is used to populate after the variables have been created with AutoBogus.

Since it's private, a rule can't be created within Faker. An override could be a solution that works now.

Other options:

  • Create a toggle for disabling private variable population
  • Create a string lookup type of ignore list

Thx for your answer, the override is working, but I couldn't find a way (tried it the last two days) to create a general AutoFakerOverride that can be used to ingore all fields (regardless of the type) --> maybe you could point me in the right direction (if there's a way).

So I think the best way would be to:

How much effort do you think this would be?

I think it would also be great solution, that when a CustomInstantiator is used that no fields are initialized from AutoBogus, in other words if a CustomInstantiator is used the the toogle should be set to "ignore fields" automatically --> don't know if this is possible, what do you think?

Br

soenneker commented 1 month ago

@rizi

Create a toggle for disabling private variable population

Do you want to make this change? The main changes would need to be done in the reflection cache library. Probably should just have a bool excluding private fields or just pass in the reflection flags into the cache.

Then in this library add a config option and then configure the cache.

I think it would also be great solution, that when a CustomInstantiator is used that no fields are initialized from AutoBogus

I am not certain that this actually is possible without some changes within Bogus.. I remember looking into this but want to check again.

rizi commented 1 month ago

@rizi

Create a toggle for disabling private variable population

Do you want to make this change? The main changes would need to be done in the reflection cache library. Probably should just have a bool excluding private fields or just pass in the reflection flags into the cache.

Then in this library add a config option and then configure the cache.

I think it would also be great solution, that when a CustomInstantiator is used that no fields are initialized from AutoBogus

I am not certain that this actually is possible without some changes within Bogus.. I remember looking into this but want to check again.

@soenneker I would happily make the change if you tell me how I get the adapted version (after my change) of your cache library in your AutoFaker Library --> is there NuGet involved or something like that?

About what kind of config do you think, a config per type or a global one?

Br

soenneker commented 1 month ago

is there NuGet involved or something like that?

Yes, check soenneker.reflection.cache

About what kind of config do you think, a config per type or a global one?

I think a global one would be good to start out with

rizi commented 1 month ago

is there NuGet involved or something like that?

Yes, check soenneker.reflection.cache

About what kind of config do you think, a config per type or a global one?

I think a global one would be good to start out with

@soenneker here is a first draft to add the possibility to configure which properties/fields should be taken into account. https://github.com/soenneker/soenneker.reflection.cache/pull/94

Could you please also tell me how to push the latest version of soenneker.reflection.cache to nuget so I can use it once you are happy with the solution.

br

rizi commented 1 month ago

@soenneker I found a few places where ReflectionCache is used (see the list below): https://github.com/soenneker/soenneker.utils.autobogus/blob/c04651bcde47d69b771eb42e3cfc6b80217b6d86/src/Services/CacheService.cs#L10 https://github.com/soenneker/soenneker.utils.autobogus/blob/c04651bcde47d69b771eb42e3cfc6b80217b6d86/src/Services/CachedTypeService.cs#L12 https://github.com/soenneker/soenneker.utils.autobogus/blob/c04651bcde47d69b771eb42e3cfc6b80217b6d86/src/Services/StaticCacheService.cs#L10 https://github.com/soenneker/soenneker.utils.autobogus/blob/c04651bcde47d69b771eb42e3cfc6b80217b6d86/src/AutoFaker.cs#L45

I think the correct place to pass the config to the Reflection Cache is within the AutoFaker class

What do you think, is this the correct place to pass the config to the Reflection Cache?

What should I do with the other usages (of Reflection Cache), pass the config there as well?

I would add the new option(s) to the AutoFakerConfig, do you think that's O.K?

br

soenneker commented 1 month ago

@rizi

I appreciate you calling out the various usages of ReflectionCache... I was able to consolidate that nicely in this commit: https://github.com/soenneker/soenneker.utils.autobogus/commit/e724a457871cbab1f4c607e6640470d518208695

I then added the ReflectionCacheOptions to AutoFakerConfig. Along with that I implemented delayed initialization so that Config can be set either within the constructor or just before Generate.

I'll also add that the example in your first comment, since the variable is set from the constructor (and that's public), it will still get set from Autobogus.

Let me know how it works. Thanks again for your help, please keep at it!

rizi commented 1 month ago

@soenneker it's woking fine, thx for your help!