From 2b0a1686c3e66c3e267454e3d1c9094bd06859b5 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Tue, 6 Aug 2019 07:37:49 -0700 Subject: [PATCH] Improve SetParametersAsync docs (#12826) * Improve SetParametersAsync docs Fixes: #12820 * Clarify semantics of clashing cascading parameters This change attempts to lock down the semantics of how cascading parameters behave inside ParameterView.SetParameterProperties. Previously a cascading value could be used to set a non-cascading parameter, and vice-versa. We were just missing tests in general for how cascading values are interpreted by the parameter set logic. --- .../Components/src/ComponentBase.cs | 19 +- .../Components/src/ComponentFactory.cs | 2 +- src/Components/Components/src/IComponent.cs | 5 + .../src/Reflection/ComponentProperties.cs | 61 ++++- .../src/Reflection/IPropertySetter.cs | 2 + .../src/Reflection/MemberAssignment.cs | 10 +- .../test/ParameterViewTest.Assignment.cs | 208 ++++++++++++++++-- 7 files changed, 279 insertions(+), 28 deletions(-) diff --git a/src/Components/Components/src/ComponentBase.cs b/src/Components/Components/src/ComponentBase.cs index baa91fd458..2ea6d38532 100644 --- a/src/Components/Components/src/ComponentBase.cs +++ b/src/Components/Components/src/ComponentBase.cs @@ -171,10 +171,25 @@ namespace Microsoft.AspNetCore.Components _renderHandle = renderHandle; } + /// - /// Method invoked to apply initial or updated parameters to the component. + /// Sets parameters supplied by the component's parent in the render tree. /// - /// The parameters to apply. + /// The parameters. + /// A that completes when the component has finished updating and rendering itself. + /// + /// + /// The method should be passed the entire set of parameter values each + /// time is called. It not required that the caller supply a parameter + /// value for all parameters that are logically understood by the component. + /// + /// + /// The default implementation of will set the value of each property + /// decorated with or that has + /// a corresponding value in the . Parameters that do not have a corresponding value + /// will be unchanged. + /// + /// public virtual Task SetParametersAsync(ParameterView parameters) { parameters.SetParameterProperties(this); diff --git a/src/Components/Components/src/ComponentFactory.cs b/src/Components/Components/src/ComponentFactory.cs index aebf96bc06..bf5de30d2a 100644 --- a/src/Components/Components/src/ComponentFactory.cs +++ b/src/Components/Components/src/ComponentFactory.cs @@ -61,7 +61,7 @@ namespace Microsoft.AspNetCore.Components ( propertyName: property.Name, propertyType: property.PropertyType, - setter: MemberAssignment.CreatePropertySetter(type, property) + setter: MemberAssignment.CreatePropertySetter(type, property, cascading: false) )).ToArray(); return Initialize; diff --git a/src/Components/Components/src/IComponent.cs b/src/Components/Components/src/IComponent.cs index ec1059b1e6..936cd37944 100644 --- a/src/Components/Components/src/IComponent.cs +++ b/src/Components/Components/src/IComponent.cs @@ -21,6 +21,11 @@ namespace Microsoft.AspNetCore.Components /// /// The parameters. /// A that completes when the component has finished updating and rendering itself. + /// + /// The method should be passed the entire set of parameter values each + /// time is called. It not required that the caller supply a parameter + /// value for all parameters that are logically understood by the component. + /// Task SetParametersAsync(ParameterView parameters); } } diff --git a/src/Components/Components/src/Reflection/ComponentProperties.cs b/src/Components/Components/src/Reflection/ComponentProperties.cs index 5caeab20b1..04f2f8d0c1 100644 --- a/src/Components/Components/src/Reflection/ComponentProperties.cs +++ b/src/Components/Components/src/Reflection/ComponentProperties.cs @@ -14,6 +14,9 @@ namespace Microsoft.AspNetCore.Components.Reflection { private const BindingFlags _bindablePropertyFlags = BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.IgnoreCase; + // Right now it's not possible for a component to define a Parameter and a Cascading Parameter with + // the same name. We don't give you a way to express this in code (would create duplicate properties), + // and we don't have the ability to represent it in our data structures. private readonly static ConcurrentDictionary _cachedWritersByType = new ConcurrentDictionary(); @@ -44,6 +47,24 @@ namespace Microsoft.AspNetCore.Components.Reflection ThrowForUnknownIncomingParameterName(targetType, parameterName); throw null; // Unreachable } + else if (writer.Cascading && !parameter.Cascading) + { + // We don't allow you to set a cascading parameter with a non-cascading value. Put another way: + // cascading parameters are not part of the public API of a component, so it's not reasonable + // for someone to set it directly. + // + // If we find a strong reason for this to work in the future we can reverse our decision since + // this throws today. + ThrowForSettingCascadingParameterWithNonCascadingValue(targetType, parameterName); + throw null; // Unreachable + } + else if (!writer.Cascading && parameter.Cascading) + { + // We're giving a more specific error here because trying to set a non-cascading parameter + // with a cascading value is likely deliberate (but not supported), or is a bug in our code. + ThrowForSettingParameterWithCascadingValue(targetType, parameterName); + throw null; // Unreachable + } SetProperty(target, writer, parameterName, parameter.Value); } @@ -62,7 +83,24 @@ namespace Microsoft.AspNetCore.Components.Reflection } var isUnmatchedValue = !writers.WritersByName.TryGetValue(parameterName, out var writer); - if (isUnmatchedValue) + + if ((isUnmatchedValue && parameter.Cascading) || (writer != null && !writer.Cascading && parameter.Cascading)) + { + // Don't allow an "extra" cascading value to be collected - or don't allow a non-cascading + // parameter to be set with a cascading value. + // + // This is likely a bug in our infrastructure or an attempt to deliberately do something unsupported. + ThrowForSettingParameterWithCascadingValue(targetType, parameterName); + throw null; // Unreachable + + } + else if (isUnmatchedValue || + + // Allow unmatched parameters to collide with the names of cascading parameters. This is + // valid because cascading parameter names are not part of the public API. There's no + // way for the user of a component to know what the names of cascading parameters + // are. + (writer.Cascading && !parameter.Cascading)) { unmatched ??= new Dictionary(StringComparer.OrdinalIgnoreCase); unmatched[parameterName] = parameter.Value; @@ -138,6 +176,20 @@ namespace Microsoft.AspNetCore.Components.Reflection } } + private static void ThrowForSettingCascadingParameterWithNonCascadingValue(Type targetType, string parameterName) + { + throw new InvalidOperationException( + $"Object of type '{targetType.FullName}' has a property matching the name '{parameterName}', " + + $"but it does not have [{nameof(ParameterAttribute)}] applied."); + } + + private static void ThrowForSettingParameterWithCascadingValue(Type targetType, string parameterName) + { + throw new InvalidOperationException( + $"The property '{parameterName}' on component type '{targetType.FullName}' cannot be set " + + $"using a cascading value."); + } + private static void ThrowForCaptureUnmatchedValuesConflict(Type targetType, string parameterName, Dictionary unmatched) { throw new InvalidOperationException( @@ -179,13 +231,14 @@ namespace Microsoft.AspNetCore.Components.Reflection foreach (var propertyInfo in GetCandidateBindableProperties(targetType)) { var parameterAttribute = propertyInfo.GetCustomAttribute(); - var isParameter = parameterAttribute != null || propertyInfo.IsDefined(typeof(CascadingParameterAttribute)); + var cascadingParameterAttribute = propertyInfo.GetCustomAttribute(); + var isParameter = parameterAttribute != null || cascadingParameterAttribute != null; if (!isParameter) { continue; } - var propertySetter = MemberAssignment.CreatePropertySetter(targetType, propertyInfo); + var propertySetter = MemberAssignment.CreatePropertySetter(targetType, propertyInfo, cascading: cascadingParameterAttribute != null); var propertyName = propertyInfo.Name; if (WritersByName.ContainsKey(propertyName)) @@ -213,7 +266,7 @@ namespace Microsoft.AspNetCore.Components.Reflection ThrowForInvalidCaptureUnmatchedValuesParameterType(targetType, propertyInfo); } - CaptureUnmatchedValuesWriter = MemberAssignment.CreatePropertySetter(targetType, propertyInfo); + CaptureUnmatchedValuesWriter = MemberAssignment.CreatePropertySetter(targetType, propertyInfo, cascading: false); CaptureUnmatchedValuesPropertyName = propertyInfo.Name; } } diff --git a/src/Components/Components/src/Reflection/IPropertySetter.cs b/src/Components/Components/src/Reflection/IPropertySetter.cs index 575b2e669b..d6a60e2395 100644 --- a/src/Components/Components/src/Reflection/IPropertySetter.cs +++ b/src/Components/Components/src/Reflection/IPropertySetter.cs @@ -5,6 +5,8 @@ namespace Microsoft.AspNetCore.Components.Reflection { internal interface IPropertySetter { + bool Cascading { get; } + void SetValue(object target, object value); } } diff --git a/src/Components/Components/src/Reflection/MemberAssignment.cs b/src/Components/Components/src/Reflection/MemberAssignment.cs index 0ab288cedc..b59d7d2ed7 100644 --- a/src/Components/Components/src/Reflection/MemberAssignment.cs +++ b/src/Components/Components/src/Reflection/MemberAssignment.cs @@ -26,7 +26,7 @@ namespace Microsoft.AspNetCore.Components.Reflection } } - public static IPropertySetter CreatePropertySetter(Type targetType, PropertyInfo property) + public static IPropertySetter CreatePropertySetter(Type targetType, PropertyInfo property, bool cascading) { if (property.SetMethod == null) { @@ -37,19 +37,23 @@ namespace Microsoft.AspNetCore.Components.Reflection return (IPropertySetter)Activator.CreateInstance( typeof(PropertySetter<,>).MakeGenericType(targetType, property.PropertyType), - property.SetMethod); + property.SetMethod, + cascading); } class PropertySetter : IPropertySetter { private readonly Action _setterDelegate; - public PropertySetter(MethodInfo setMethod) + public PropertySetter(MethodInfo setMethod, bool cascading) { _setterDelegate = (Action)Delegate.CreateDelegate( typeof(Action), setMethod); + Cascading = cascading; } + public bool Cascading { get; } + public void SetValue(object target, object value) { if (value == null) diff --git a/src/Components/Components/test/ParameterViewTest.Assignment.cs b/src/Components/Components/test/ParameterViewTest.Assignment.cs index 7c2056683a..bf2f38af5a 100644 --- a/src/Components/Components/test/ParameterViewTest.Assignment.cs +++ b/src/Components/Components/test/ParameterViewTest.Assignment.cs @@ -5,9 +5,8 @@ using System; using System.Collections; using System.Collections.Generic; using System.Linq; -using System.Threading.Tasks; +using Microsoft.AspNetCore.Components.Rendering; using Microsoft.AspNetCore.Components.RenderTree; -using Microsoft.AspNetCore.Components.Test.Helpers; using Xunit; namespace Microsoft.AspNetCore.Components @@ -96,23 +95,58 @@ namespace Microsoft.AspNetCore.Components } [Fact] - public void IncomingParameterMatchesNoDeclaredParameter_Throws() + public void IncomingCascadingValueMatchesCascadingParameter_SetsValue() { // Arrange - var target = new HasPropertyWithoutParameterAttribute(); - var parameters = new ParameterViewBuilder - { - { "AnyOtherKey", 123 }, - }.Build(); + var builder = new ParameterViewBuilder(); + builder.Add(nameof(HasCascadingParameter.Cascading), "hi", cascading: true); + var parameters = builder.Build(); + + var target = new HasCascadingParameter(); // Act - var ex = Assert.Throws( - () => parameters.SetParameterProperties(target)); + parameters.SetParameterProperties(target); + + // Assert + Assert.Equal("hi", target.Cascading); + } + + [Fact] + public void NoIncomingCascadingValueMatchesDeclaredCascadingParameter_LeavesValueUnchanged() + { + // Arrange + var builder = new ParameterViewBuilder(); + var parameters = builder.Build(); + + var target = new HasCascadingParameter() + { + Cascading = "bye", + }; + + // Act + parameters.SetParameterProperties(target); + + // Assert + Assert.Equal("bye", target.Cascading); + } + + [Fact] + public void IncomingCascadingValueMatchesNoDeclaredParameter_Throws() + { + // Arrange + var builder = new ParameterViewBuilder(); + builder.Add("SomethingElse", "hi", cascading: true); + var parameters = builder.Build(); + + var target = new HasCascadingParameter(); + + // Act + var ex = Assert.Throws(() => parameters.SetParameterProperties(target)); // Assert Assert.Equal( - $"Object of type '{typeof(HasPropertyWithoutParameterAttribute).FullName}' does not have a property " + - $"matching the name 'AnyOtherKey'.", + $"Object of type '{typeof(HasCascadingParameter).FullName}' does not have a property " + + $"matching the name 'SomethingElse'.", ex.Message); } @@ -138,6 +172,45 @@ namespace Microsoft.AspNetCore.Components ex.Message); } + [Fact] + public void IncomingNonCascadingValueMatchesCascadingParameter_Throws() + { + // Arrange + var target = new HasCascadingParameter(); + var parameters = new ParameterViewBuilder + { + { nameof(HasCascadingParameter.Cascading), 123 }, + }.Build(); + + // Act + var ex = Assert.Throws(() => parameters.SetParameterProperties(target)); + + // Assert + Assert.Equal( + $"Object of type '{typeof(HasCascadingParameter).FullName}' has a property matching the name '{nameof(HasCascadingParameter.Cascading)}', " + + $"but it does not have [{nameof(ParameterAttribute)}] applied.", + ex.Message); + } + + [Fact] + public void IncomingCascadingValueMatchesNonCascadingParameter_Throws() + { + // Arrange + var target = new HasInstanceProperties(); + var builder = new ParameterViewBuilder(); + builder.Add(nameof(HasInstanceProperties.IntProp), 16, cascading: true); + var parameters = builder.Build(); + + // Act + var ex = Assert.Throws(() => parameters.SetParameterProperties(target)); + + // Assert + Assert.Equal( + $"The property '{nameof(HasInstanceProperties.IntProp)}' on component type '{typeof(HasInstanceProperties).FullName}' " + + $"cannot be set using a cascading value.", + ex.Message); + } + [Fact] public void SettingCaptureUnmatchedValuesParameterExplicitlyWorks() { @@ -274,6 +347,51 @@ namespace Microsoft.AspNetCore.Components ex.Message); } + [Fact] + public void IncomingNonCascadingValueMatchesCascadingParameter_WithCaptureUnmatchedValues_DoesNotThrow() + { + // Arrange + var target = new HasCaptureUnmatchedValuesPropertyAndCascadingParameter() + { + Cascading = "bye", + }; + var parameters = new ParameterViewBuilder + { + { nameof(HasCaptureUnmatchedValuesPropertyAndCascadingParameter.Cascading), "hi" }, + }.Build(); + + // Act + parameters.SetParameterProperties(target); + + Assert.Collection( + target.CaptureUnmatchedValues, + kvp => + { + Assert.Equal(nameof(HasCaptureUnmatchedValuesPropertyAndCascadingParameter.Cascading), kvp.Key); + Assert.Equal("hi", kvp.Value); + }); + Assert.Equal("bye", target.Cascading); + } + + [Fact] + public void IncomingCascadingValueMatchesNonCascadingParameter_WithCaptureUnmatchedValues_Throws() + { + // Arrange + var target = new HasCaptureUnmatchedValuesProperty(); + var builder = new ParameterViewBuilder(); + builder.Add(nameof(HasInstanceProperties.IntProp), 16, cascading: true); + var parameters = builder.Build(); + + // Act + var ex = Assert.Throws(() => parameters.SetParameterProperties(target)); + + // Assert + Assert.Equal( + $"The property '{nameof(HasCaptureUnmatchedValuesProperty.IntProp)}' on component type '{typeof(HasCaptureUnmatchedValuesProperty).FullName}' " + + $"cannot be set using a cascading value.", + ex.Message); + } + [Fact] public void IncomingParameterValueMismatchesDeclaredParameterType_Throws() { @@ -396,6 +514,11 @@ namespace Microsoft.AspNetCore.Components } } + class HasCascadingParameter + { + [CascadingParameter] public string Cascading { get; set; } + } + class HasPropertyWithoutParameterAttribute { internal int IntProp { get; set; } @@ -435,6 +558,12 @@ namespace Microsoft.AspNetCore.Components [Parameter(CaptureUnmatchedValues = true)] public IReadOnlyDictionary CaptureUnmatchedValues { get; set; } } + class HasCaptureUnmatchedValuesPropertyAndCascadingParameter + { + [CascadingParameter] public string Cascading { get; set; } + [Parameter(CaptureUnmatchedValues = true)] public IReadOnlyDictionary CaptureUnmatchedValues { get; set; } + } + class HasDupliateCaptureUnmatchedValuesProperty { [Parameter(CaptureUnmatchedValues = true)] public Dictionary CaptureUnmatchedValuesProp1 { get; set; } @@ -448,11 +577,11 @@ namespace Microsoft.AspNetCore.Components class ParameterViewBuilder : IEnumerable { - private readonly List<(string Name, object Value)> _keyValuePairs - = new List<(string, object)>(); + private readonly List<(string Name, object Value, bool Cascading)> _keyValuePairs + = new List<(string, object, bool)>(); - public void Add(string name, object value) - => _keyValuePairs.Add((name, value)); + public void Add(string name, object value, bool cascading = false) + => _keyValuePairs.Add((name, value, cascading)); public IEnumerator GetEnumerator() => throw new NotImplementedException(); @@ -460,13 +589,56 @@ namespace Microsoft.AspNetCore.Components public ParameterView Build() { var builder = new RenderTreeBuilder(); + builder.OpenComponent(0); foreach (var kvp in _keyValuePairs) { - builder.AddAttribute(1, kvp.Name, kvp.Value); + if (!kvp.Cascading) + { + builder.AddAttribute(1, kvp.Name, kvp.Value); + } } builder.CloseComponent(); - return new ParameterView(builder.GetFrames().Array, ownerIndex: 0); + + var view = new ParameterView(builder.GetFrames().Array, ownerIndex: 0); + + var cascadingParameters = new List(); + foreach (var kvp in _keyValuePairs) + { + if (kvp.Cascading) + { + cascadingParameters.Add(new CascadingParameterState(kvp.Name, new TestCascadingValueProvider(kvp.Value))); + } + } + + return view.WithCascadingParameters(cascadingParameters); + } + } + + private class TestCascadingValueProvider : ICascadingValueComponent + { + public TestCascadingValueProvider(object value) + { + CurrentValue = value; + } + + public object CurrentValue { get; } + + public bool CurrentValueIsFixed => throw new NotImplementedException(); + + public bool CanSupplyValue(Type valueType, string valueName) + { + throw new NotImplementedException(); + } + + public void Subscribe(ComponentState subscriber) + { + throw new NotImplementedException(); + } + + public void Unsubscribe(ComponentState subscriber) + { + throw new NotImplementedException(); } } }