ConfigurationBinder should support binding private properties from ba… #71039
Conversation
ghost
added
community-contribution
|
Tagging subscribers to this area: @dotnet/area-extensions-configuration Issue Details…se classes.
|
|
|
||
| baseType = baseType.BaseType!; | ||
| } | ||
| while (baseType != typeof(object)); |
There was a problem hiding this comment.
I would do an additional null check here just in case someone binds to object directly :)
There was a problem hiding this comment.
Fixed, thanks !
|
|
||
| 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) |
There was a problem hiding this comment.
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;
}There was a problem hiding this comment.
Fixed, I just realize Reflection use dynamic lookup so that base-most virtual properties can also be ok, thanks .
| private string? PrivateProperty { get; set; } | ||
| public virtual string[] Test { get; set; } = System.Array.Empty<string>(); | ||
|
|
||
| public string? ExposePrivatePropertyValue() => PrivateProperty; |
There was a problem hiding this comment.
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 ...; }
}
}
There was a problem hiding this comment.
Fixed, thanks.
…st cases for more get set combinations.
eerhardt
left a comment
eerhardt
left a comment
There was a problem hiding this comment.
This looks good to me. Thanks for the contribution @lateapexearlyspeed!
|
Hi @eerhardt , thanks ! Just a friendly reminder: don't forget to merge if it can be :) |
| Assert.Equal("3", test.TestGetOverriden); | ||
| Assert.Equal("4", test.TestSetOverriden); | ||
| Assert.Equal("5", test.TestNoOverriden); | ||
| Assert.Null(test.ExposeTestVirtualSet()); |
There was a problem hiding this comment.
Why is this Null instead of 6?
There was a problem hiding this comment.
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
ghost
locked as resolved and limited conversation to collaborators
3 participants
…se classes.
Fix #71040
I checked code change history, private property binding issue may be introduced by improving virtual properties handling. So this PR changes logic to handle both of these.