From 5bc79e84498e2159ecc2e5c0430f69653d723ae6 Mon Sep 17 00:00:00 2001 From: Pranav K Date: Fri, 16 Aug 2019 17:53:55 -0700 Subject: [PATCH] Ignore overriden properties in component parameters (#13198) * Ignore overriden properties in component parameters * Modify MemberAssignment to return shadowed properties, but not inherited properties * Modify ComponentProperties to throw if a Parameter property is non-public Fixes https://github.com/aspnet/AspNetCore/issues/13162 --- .../src/Reflection/ComponentProperties.cs | 8 +- .../src/Reflection/MemberAssignment.cs | 19 ++++- .../test/ParameterViewTest.Assignment.cs | 83 +++++++++++++++++-- .../test/RenderTreeDiffBuilderTest.cs | 2 +- .../Components/test/RendererTest.cs | 8 +- 5 files changed, 108 insertions(+), 12 deletions(-) diff --git a/src/Components/Components/src/Reflection/ComponentProperties.cs b/src/Components/Components/src/Reflection/ComponentProperties.cs index 04f2f8d0c1..0b85a66a84 100644 --- a/src/Components/Components/src/Reflection/ComponentProperties.cs +++ b/src/Components/Components/src/Reflection/ComponentProperties.cs @@ -238,9 +238,15 @@ namespace Microsoft.AspNetCore.Components.Reflection continue; } + var propertyName = propertyInfo.Name; + if (parameterAttribute != null && (propertyInfo.SetMethod == null || !propertyInfo.SetMethod.IsPublic)) + { + throw new InvalidOperationException( + $"The type '{targetType.FullName}' declares a parameter matching the name '{propertyName}' that is not public. Parameters must be public."); + } + var propertySetter = MemberAssignment.CreatePropertySetter(targetType, propertyInfo, cascading: cascadingParameterAttribute != null); - var propertyName = propertyInfo.Name; if (WritersByName.ContainsKey(propertyName)) { throw new InvalidOperationException( diff --git a/src/Components/Components/src/Reflection/MemberAssignment.cs b/src/Components/Components/src/Reflection/MemberAssignment.cs index b59d7d2ed7..ab0cf6b1e7 100644 --- a/src/Components/Components/src/Reflection/MemberAssignment.cs +++ b/src/Components/Components/src/Reflection/MemberAssignment.cs @@ -13,17 +13,34 @@ namespace Microsoft.AspNetCore.Components.Reflection public static IEnumerable GetPropertiesIncludingInherited( Type type, BindingFlags bindingFlags) { + var dictionary = new Dictionary>(); + while (type != null) { var properties = type.GetProperties(bindingFlags) .Where(prop => prop.DeclaringType == type); foreach (var property in properties) { - yield return property; + if (!dictionary.TryGetValue(property.Name, out var others)) + { + others = new List(); + dictionary.Add(property.Name, others); + } + + if (others.Any(other => other.GetMethod?.GetBaseDefinition() == property.GetMethod?.GetBaseDefinition())) + { + // This is an inheritance case. We can safely ignore the value of property since + // we have seen a more derived value. + continue; + } + + others.Add(property); } type = type.BaseType; } + + return dictionary.Values.SelectMany(p => p); } public static IPropertySetter CreatePropertySetter(Type targetType, PropertyInfo property, bool cascading) diff --git a/src/Components/Components/test/ParameterViewTest.Assignment.cs b/src/Components/Components/test/ParameterViewTest.Assignment.cs index bf2f38af5a..9f677ef50a 100644 --- a/src/Components/Components/test/ParameterViewTest.Assignment.cs +++ b/src/Components/Components/test/ParameterViewTest.Assignment.cs @@ -6,7 +6,6 @@ using System.Collections; using System.Collections.Generic; using System.Linq; using Microsoft.AspNetCore.Components.Rendering; -using Microsoft.AspNetCore.Components.RenderTree; using Xunit; namespace Microsoft.AspNetCore.Components @@ -71,6 +70,24 @@ namespace Microsoft.AspNetCore.Components Assert.Equal(456, target.DerivedClassIntProp); } + [Fact] + public void IncomingParameterMatchesOverridenParameter_ThatDoesNotHasAttribute() + { + // Test for https://github.com/aspnet/AspNetCore/issues/13162 + // Arrange + var parameters = new ParameterViewBuilder + { + { nameof(DerivedType.VirtualProp), 123 }, + }.Build(); + var target = new DerivedType(); + + // Act + parameters.SetParameterProperties(target); + + // Assert + Assert.Equal(123, target.VirtualProp); + } + [Fact] public void NoIncomingParameterMatchesDeclaredParameter_LeavesValueUnchanged() { @@ -172,6 +189,43 @@ namespace Microsoft.AspNetCore.Components ex.Message); } + [Fact] + public void IncomingParameterMatchesPropertyNotPublic_Throws() + { + // Arrange + var target = new HasNonPublicPropertyWithParameterAttribute(); + var parameters = new ParameterViewBuilder + { + { nameof(HasNonPublicPropertyWithParameterAttribute.IntProp), 123 }, + }.Build(); + + // Act + var ex = Assert.Throws( + () => parameters.SetParameterProperties(target)); + + // Assert + Assert.Equal(default, target.IntProp); + Assert.Equal( + $"The type '{typeof(HasNonPublicPropertyWithParameterAttribute).FullName}' declares a parameter matching the name '{nameof(HasNonPublicPropertyWithParameterAttribute.IntProp)}' that is not public. Parameters must be public.", + ex.Message); + } + + [Fact] + public void IncomingCascadingParameterMatchesPropertyNotPublic_Works() + { + // Arrange + var target = new HasNonPublicCascadingParameter(); + var builder = new ParameterViewBuilder(); + builder.Add("Cascading", "Test", cascading: true); + var parameters = builder.Build(); + + // Act + parameters.SetParameterProperties(target); + + // Assert + Assert.Equal("Test", target.GetCascadingValue()); + } + [Fact] public void IncomingNonCascadingValueMatchesCascadingParameter_Throws() { @@ -497,13 +551,9 @@ namespace Microsoft.AspNetCore.Components class HasInstanceProperties { - // "internal" to show we're not requiring public accessors, but also - // to keep the assertions simple in the tests - [Parameter] public int IntProp { get; set; } [Parameter] public string StringProp { get; set; } - // Also a truly private one to show there's nothing special about 'internal' [Parameter] public object ObjectProp { get; set; } public static string ObjectPropName => nameof(ObjectProp); @@ -521,6 +571,12 @@ namespace Microsoft.AspNetCore.Components class HasPropertyWithoutParameterAttribute { + public int IntProp { get; set; } + } + + class HasNonPublicPropertyWithParameterAttribute + { + [Parameter] internal int IntProp { get; set; } } @@ -539,6 +595,16 @@ namespace Microsoft.AspNetCore.Components [Parameter] public int DerivedClassIntProp { get; set; } } + class BaseType + { + [Parameter] public virtual int VirtualProp { get; set; } + } + + class DerivedType : BaseType + { + public override int VirtualProp { get; set; } + } + class HasParametersVaryingOnlyByCase { [Parameter] public object MyValue { get; set; } @@ -575,6 +641,13 @@ namespace Microsoft.AspNetCore.Components [Parameter(CaptureUnmatchedValues = true)] public KeyValuePair[] CaptureUnmatchedValuesProp { get; set; } } + class HasNonPublicCascadingParameter + { + [CascadingParameter] private string Cascading { get; set; } + + public string GetCascadingValue() => Cascading; + } + class ParameterViewBuilder : IEnumerable { private readonly List<(string Name, object Value, bool Cascading)> _keyValuePairs diff --git a/src/Components/Components/test/RenderTreeDiffBuilderTest.cs b/src/Components/Components/test/RenderTreeDiffBuilderTest.cs index 503d45155f..28f617eba2 100644 --- a/src/Components/Components/test/RenderTreeDiffBuilderTest.cs +++ b/src/Components/Components/test/RenderTreeDiffBuilderTest.cs @@ -2231,7 +2231,7 @@ namespace Microsoft.AspNetCore.Components.Test public object ObjectProperty { get; set; } [Parameter] - public string ReadonlyProperty { get; private set; } + public string ReadonlyProperty { get; set; } [Parameter] public string PrivateProperty { get; set; } diff --git a/src/Components/Components/test/RendererTest.cs b/src/Components/Components/test/RendererTest.cs index 71ebf86351..e734ecef7c 100644 --- a/src/Components/Components/test/RendererTest.cs +++ b/src/Components/Components/test/RendererTest.cs @@ -3791,10 +3791,10 @@ namespace Microsoft.AspNetCore.Components.Test private class FakeComponent : IComponent { [Parameter] - public int IntProperty { get; private set; } + public int IntProperty { get; set; } [Parameter] - public string StringProperty { get; private set; } + public string StringProperty { get; set; } [Parameter] public object ObjectProperty { get; set; } @@ -3927,7 +3927,7 @@ namespace Microsoft.AspNetCore.Components.Test private class ReRendersParentComponent : AutoRenderComponent { [Parameter] - public TestComponent Parent { get; private set; } + public TestComponent Parent { get; set; } private bool _isFirstTime = true; @@ -4057,7 +4057,7 @@ namespace Microsoft.AspNetCore.Components.Test public bool Disposed { get; private set; } [Parameter] - public Action DisposeAction { get; private set; } + public Action DisposeAction { get; set; } public void Dispose() {