Tynamix / ObjectFiller.NET

The .NET ObjectFiller fills the properties of your .NET objects with random data
http://objectfiller.net
MIT License
76 stars 35 forks source link

Impossible to fill abstract properties inherited from an abstract parent #143

Closed cguyonnet closed 2 months ago

cguyonnet commented 3 months ago

I'm having trouble filling a property that is inherited from an abstract base class. Whatever the setup I try, it seems the property gets ignored by the filler.

In the example below, abstract class EntityBase defines an abstract property AbstractName. Then CustomEntity inherits EntityBase and overrides the property. (I use this approach to have different validation attributes on the property in each inheritor of EntityBase.)

🔍 Investigation

From my investigations, it looks like the issue comes from FillProperties method, more precisely in GetPropertyFromProperties when we check if a property from the target type has a corresponding setup in the filler.

Basically the property does not have the same DeclaringType on both sides and therefore gets ignored :

property (context) DeclaringType in FillerSetup DeclaringType extracted from target type
Name (defined by abstract parent class) EntityBase EntityBase
AbstractName (defined abstract in parent, overridden in concrete child) EntityBase CustomEntity
DerivedName (defined by concrete child) CustomEntity CustomEntity

Properties in FillerSetup:

Properties here are extracted from the body of the Expression provided to OnProperty() methods.

Properties extracted from target type:

Done in NetTypeApiExtensions.GetProperties method. This will recursively extract properties from the target type and its ancestors (unless ignoreInheritance is set), and ignore duplicates.

This strategy makes perfect sense, especially for instance when a property is declared by the parent but re-declared with new keyword on the child. But it appears for inherited abstract properties it does not... For sure the child overrides it, but it is still declared by the parent.

❓Proposition

In my opinion the strategy for extracting properties from the target type should mimick the behavior of properties from the Expression used in OnProperty methods. The GetDeclaredPropertyInfosRecursive method could be improved a little bit : when extracting an abstract property it should not check if a property with the same name has already been extracted as the abstract declaration should take precedence over the override from the concrete type.

ℹ️ I'm open to feedback on this issue and proposition. I'll try it on my side soon and might propose a PR if it works and no objections are raised in the mean time


Full example:

using System.Text.Json;
using Tynamix.ObjectFiller;

var customEntity = CustomEntityBuilder.New().Create();
Console.WriteLine(JsonSerializer.Serialize(customEntity));
// {"AbstractName":null,"DerivedName":"derived name","Name":"Jane"

abstract class EntityBase
{
    public string? Name { get; set; }
    public abstract string? AbstractName { get; set; }
}

class CustomEntity : EntityBase
{
    public override string? AbstractName { get; set; }
    public string? DerivedName { get; set; }
}

abstract class EntityBaseBuilder<T> where T : EntityBase
{
    private readonly Filler<T> _filler = new();
    protected FluentFillerApi<T> Setup { get; }

    protected EntityBaseBuilder()
    {
        Setup = _filler.Setup(true);

        Setup
            .IgnoreAllUnknownTypes()
            .OnProperty(x => x.Name).Use("Jane")
            .OnProperty(x => x.AbstractName).Use("abstract name");
    }

    //[WithName(), ...]

    public T Create()
    {
        return _filler.Create();
    }
}

class CustomEntityBuilder : EntityBaseBuilder<CustomEntity>
{
    private CustomEntityBuilder() : base()
    {
        Setup
            .OnProperty(x => x.AbstractName).Use("overridden abstract name")
            .OnProperty(x => x.DerivedName).Use("derived name");
    }

    //[WithDerivedName(), ...]

    public static CustomEntityBuilder New() => new();
}
Tynamix commented 3 months ago

Thank you very much for this contribution. Will take a look on this the next days. 👍

cguyonnet commented 3 months ago

Hey @Tynamix Any chance of giving this PR a look soon ?

evan-godec commented 2 months ago

I have got the same issue, will the fix come soon ? Thanks