Skip to content

ConfigurationBinder should support binding private properties from ba… #71039

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.

Sign up for GitHub

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

Merged
eerhardt merged 3 commits into from
Jul 14, 2022
Merged

ConfigurationBinder should support binding private properties from ba… #71039

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d0cecde
ConfigurationBinder should support binding private properties from ba…
lateapexearlyspeed Jun 21, 2022
03d1d1d
Fix comment: consider binding 'Object' instance directly.
lateapexearlyspeed Jun 21, 2022
1db4e96
Fix comment: directly use base-most methodInfo for properties; Add te…
lateapexearlyspeed Jul 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 29 additions & 5 deletions src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ public static void Bind(this IConfiguration configuration, object? instance, Act
[RequiresUnreferencedCode(PropertyTrimmingWarningMessage)]
private static void BindNonScalar(this IConfiguration configuration, object instance, BinderOptions options)
{
PropertyInfo[] modelProperties = GetAllProperties(instance.GetType());
List<PropertyInfo> modelProperties = GetAllProperties(instance.GetType());

if (options.ErrorOnUnknownConfiguration)
{
Expand Down Expand Up @@ -451,7 +451,7 @@ private static object CreateInstance(
}


PropertyInfo[] properties = GetAllProperties(type);
List<PropertyInfo> properties = GetAllProperties(type);

if (!DoAllParametersHaveEquivalentProperties(parameters, properties, out string nameOfInvalidParameters))
{
Expand Down Expand Up @@ -482,7 +482,7 @@ private static object CreateInstance(
}

private static bool DoAllParametersHaveEquivalentProperties(ParameterInfo[] parameters,
PropertyInfo[] properties, out string missing)
List<PropertyInfo> properties, out string missing)
{
HashSet<string> propertyNames = new(StringComparer.OrdinalIgnoreCase);
foreach (PropertyInfo prop in properties)
Expand Down Expand Up @@ -752,8 +752,32 @@ private static bool IsArrayCompatibleReadOnlyInterface(Type type)
return null;
}

private static PropertyInfo[] GetAllProperties([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] Type type)
=> type.GetProperties(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.Static);
private static List<PropertyInfo> GetAllProperties([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] Type type)
Copy link
Member

@eerhardt eerhardt Jun 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this just be:

    internal static List<PropertyInfo> GetAllProperties(Type type)
    {
        List<PropertyInfo> allProperties = new();

        Type baseType = type;
        while (baseType != typeof(object))
        {
            PropertyInfo[] properties = baseType.GetProperties(DeclaredOnlyLookup);

            foreach (PropertyInfo property in properties)
            {
                // if the property is virtual, only add the base-most definition so
                // overriden properties aren't duplicated in the list.
                MethodInfo? setMethod = property.GetSetMethod(nonPublic: true);
                if (setMethod == null || !setMethod.IsVirtual || setMethod.GetBaseDefinition().DeclaringType == baseType)
                {
                    allProperties.Add(property);
                }
            }

            baseType = baseType.BaseType!;
        }

        return allProperties;
    }

Copy link
Contributor Author

@lateapexearlyspeed lateapexearlyspeed Jul 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, I just realize Reflection use dynamic lookup so that base-most virtual properties can also be ok, thanks .

{
var allProperties = new List<PropertyInfo>();

Type baseType = type;
while (baseType != typeof(object))
{
PropertyInfo[] properties = baseType.GetProperties(DeclaredOnlyLookup);

foreach (PropertyInfo property in properties)
{
// if the property is virtual, only add the base-most definition so
// overriden properties aren't duplicated in the list.
MethodInfo? setMethod = property.GetSetMethod(true);

if (setMethod is null || !setMethod.IsVirtual || setMethod == setMethod.GetBaseDefinition())
{
allProperties.Add(property);
}
}

baseType = baseType.BaseType!;
}

return allProperties;
}

[RequiresUnreferencedCode(PropertyTrimmingWarningMessage)]
private static object? BindParameter(ParameterInfo parameter, Type type, IConfiguration config,
Expand Down
60 changes: 58 additions & 2 deletions src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1751,20 +1751,45 @@ public void CanBindNullableNestedStructProperties()
}

[Fact]
public void CanBindVirtualPropertiesWithoutDuplicates()
public void CanBindVirtualProperties()
{
ConfigurationBuilder configurationBuilder = new();
configurationBuilder.AddInMemoryCollection(new Dictionary<string, string>
{
{ "Test:0", "1" }
{ $"{nameof(BaseClassWithVirtualProperty.Test)}:0", "1" },
{ $"{nameof(BaseClassWithVirtualProperty.TestGetSetOverriden)}", "2" },
{ $"{nameof(BaseClassWithVirtualProperty.TestGetOverriden)}", "3" },
{ $"{nameof(BaseClassWithVirtualProperty.TestSetOverriden)}", "4" },
{ $"{nameof(BaseClassWithVirtualProperty.TestNoOverriden)}", "5" },
{ $"{nameof(BaseClassWithVirtualProperty.TestVirtualSet)}", "6" }
});
IConfiguration config = configurationBuilder.Build();

var test = new ClassOverridingVirtualProperty();
config.Bind(test);

Assert.Equal("1", Assert.Single(test.Test));
Assert.Equal("2", test.TestGetSetOverriden);
Assert.Equal("3", test.TestGetOverriden);
Assert.Equal("4", test.TestSetOverriden);
Assert.Equal("5", test.TestNoOverriden);
Assert.Null(test.ExposeTestVirtualSet());
Copy link
Member

@eerhardt eerhardt Jul 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this Null instead of 6?

Copy link
Contributor Author

@lateapexearlyspeed lateapexearlyspeed Jul 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because set-only property was not bound, which was existing behavior (it is not about code in GetAllProperties()). I added this test case to make sure all get/set combinations covered and behavior not break. @eerhardt

}

[Fact]
public void CanBindPrivatePropertiesFromBaseClass()
{
ConfigurationBuilder configurationBuilder = new();
configurationBuilder.AddInMemoryCollection(new Dictionary<string, string>
{
{ "PrivateProperty", "a" }
});
IConfiguration config = configurationBuilder.Build();

var test = new ClassOverridingVirtualProperty();
config.Bind(test, b => b.BindNonPublicProperties = true);
Assert.Equal("a", test.ExposePrivatePropertyValue());
}

private interface ISomeInterface
{
Expand Down Expand Up @@ -1845,12 +1870,43 @@ public struct DeeplyNested

public class BaseClassWithVirtualProperty
{
private string? PrivateProperty { get; set; }

public virtual string[] Test { get; set; } = System.Array.Empty<string>();

public virtual string? TestGetSetOverriden { get; set; }
public virtual string? TestGetOverriden { get; set; }
public virtual string? TestSetOverriden { get; set; }

private string? _testVirtualSet;
public virtual string? TestVirtualSet
{
set => _testVirtualSet = value;
}

public virtual string? TestNoOverriden { get; set; }

public string? ExposePrivatePropertyValue() => PrivateProperty;
Copy link
Member

@eerhardt eerhardt Jun 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also add tests with different get/set combinations. For example:

class A 
{
   public virtual string TestString { get; set; }
}

class B : A
{
   public override string TestString
   {
      get { return ...; }
   }
}

Copy link
Contributor Author

@lateapexearlyspeed lateapexearlyspeed Jul 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks.

}

public class ClassOverridingVirtualProperty : BaseClassWithVirtualProperty
{
public override string[] Test { get => base.Test; set => base.Test = value; }

public override string? TestGetSetOverriden { get; set; }
public override string? TestGetOverriden => base.TestGetOverriden;
public override string? TestSetOverriden
{
set => base.TestSetOverriden = value;
}

private string? _testVirtualSet;
public override string? TestVirtualSet
{
set => _testVirtualSet = value;
}

public string? ExposeTestVirtualSet() => _testVirtualSet;
}
}
}