From 4aa24916f0f0ab1320420c33ef0c76505b7381d6 Mon Sep 17 00:00:00 2001 From: remi_bourgarel Date: Thu, 13 Dec 2018 15:05:17 +0000 Subject: [PATCH 1/5] Change AssignToProperties to clear all unset bindables --- .../CascadingValue.cs | 2 +- .../ComponentBase.cs | 2 +- .../Layouts/LayoutDisplay.cs | 4 +-- .../ParameterCollectionExtensions.cs | 27 +++++++++------- .../Reflection/IPropertySetter.cs | 4 ++- .../Reflection/MemberAssignment.cs | 10 +++++- .../Routing/Router.cs | 2 +- ...meterCollectionAssignmentExtensionsTest.cs | 32 +++++++++---------- .../RenderTreeDiffBuilderTest.cs | 2 +- .../RendererTest.cs | 4 +-- .../test/shared/AutoRenderComponent.cs | 2 +- 11 files changed, 52 insertions(+), 39 deletions(-) diff --git a/src/Components/src/Microsoft.AspNetCore.Components/CascadingValue.cs b/src/Components/src/Microsoft.AspNetCore.Components/CascadingValue.cs index 006fca448f63..53e356ae2eee 100644 --- a/src/Components/src/Microsoft.AspNetCore.Components/CascadingValue.cs +++ b/src/Components/src/Microsoft.AspNetCore.Components/CascadingValue.cs @@ -58,7 +58,7 @@ public void Init(RenderHandle renderHandle) public void SetParameters(ParameterCollection parameters) { // Implementing the parameter binding manually, instead of just calling - // parameters.AssignToProperties(this), is just a very slight perf optimization + // parameters.SetParameterProperties(this), is just a very slight perf optimization // and makes it simpler impose rules about the params being required or not. var hasSuppliedValue = false; diff --git a/src/Components/src/Microsoft.AspNetCore.Components/ComponentBase.cs b/src/Components/src/Microsoft.AspNetCore.Components/ComponentBase.cs index 1ff2b60b7b96..58a23ee546da 100644 --- a/src/Components/src/Microsoft.AspNetCore.Components/ComponentBase.cs +++ b/src/Components/src/Microsoft.AspNetCore.Components/ComponentBase.cs @@ -151,7 +151,7 @@ void IComponent.Init(RenderHandle renderHandle) /// The parameters to apply. public virtual void SetParameters(ParameterCollection parameters) { - parameters.AssignToProperties(this); + parameters.SetParameterProperties(this); if (!_hasCalledInit) { diff --git a/src/Components/src/Microsoft.AspNetCore.Components/Layouts/LayoutDisplay.cs b/src/Components/src/Microsoft.AspNetCore.Components/Layouts/LayoutDisplay.cs index c9b26ea897ca..afd205c40dc2 100644 --- a/src/Components/src/Microsoft.AspNetCore.Components/Layouts/LayoutDisplay.cs +++ b/src/Components/src/Microsoft.AspNetCore.Components/Layouts/LayoutDisplay.cs @@ -1,4 +1,4 @@ -// Copyright (c) .NET Foundation. All rights reserved. +// Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; @@ -42,7 +42,7 @@ public void Init(RenderHandle renderHandle) /// public void SetParameters(ParameterCollection parameters) { - parameters.AssignToProperties(this); + parameters.SetParameterProperties(this); Render(); } diff --git a/src/Components/src/Microsoft.AspNetCore.Components/ParameterCollectionExtensions.cs b/src/Components/src/Microsoft.AspNetCore.Components/ParameterCollectionExtensions.cs index 2319708751c0..eb255223eb27 100644 --- a/src/Components/src/Microsoft.AspNetCore.Components/ParameterCollectionExtensions.cs +++ b/src/Components/src/Microsoft.AspNetCore.Components/ParameterCollectionExtensions.cs @@ -5,6 +5,7 @@ using System; using System.Collections.Concurrent; using System.Collections.Generic; +using System.Linq; using System.Reflection; namespace Microsoft.AspNetCore.Components @@ -16,10 +17,7 @@ public static class ParameterCollectionExtensions { private const BindingFlags _bindablePropertyFlags = BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.IgnoreCase; - private delegate void WriteParameterAction(object target, object parameterValue); - - private readonly static IDictionary> _cachedParameterWriters - = new ConcurrentDictionary>(); + private readonly static IDictionary> _cachedParameterWriters = new ConcurrentDictionary>(); /// /// Iterates through the , assigning each parameter @@ -27,7 +25,7 @@ private readonly static IDictionary /// The . /// An object that has a public writable property matching each parameter's name and type. - public static void AssignToProperties( + public static void SetParameterProperties( in this ParameterCollection parameterCollection, object target) { @@ -43,6 +41,8 @@ public static void AssignToProperties( _cachedParameterWriters[targetType] = parameterWriters; } + var localParameterWriter = parameterWriters.Values.ToList(); + foreach (var parameter in parameterCollection) { var parameterName = parameter.Name; @@ -53,7 +53,8 @@ public static void AssignToProperties( try { - parameterWriter(target, parameter.Value); + parameterWriter.SetValue(target, parameter.Value); + localParameterWriter.Remove(parameterWriter); } catch (Exception ex) { @@ -62,14 +63,19 @@ public static void AssignToProperties( $"type '{target.GetType().FullName}'. The error was: {ex.Message}", ex); } } + + foreach (var nonUsedParameter in localParameterWriter) + { + nonUsedParameter.SetValue(target, nonUsedParameter.GetDefaultValue()); + } } internal static IEnumerable GetCandidateBindableProperties(Type targetType) => MemberAssignment.GetPropertiesIncludingInherited(targetType, _bindablePropertyFlags); - private static IDictionary CreateParameterWriters(Type targetType) + private static IDictionary CreateParameterWriters(Type targetType) { - var result = new Dictionary(StringComparer.OrdinalIgnoreCase); + var result = new Dictionary(StringComparer.OrdinalIgnoreCase); foreach (var propertyInfo in GetCandidateBindableProperties(targetType)) { @@ -90,10 +96,7 @@ private static IDictionary CreateParameterWriters( $"name '{propertyName.ToLowerInvariant()}'. Parameter names are case-insensitive and must be unique."); } - result.Add(propertyName, (object target, object parameterValue) => - { - propertySetter.SetValue(target, parameterValue); - }); + result.Add(propertyName, propertySetter); } return result; diff --git a/src/Components/src/Microsoft.AspNetCore.Components/Reflection/IPropertySetter.cs b/src/Components/src/Microsoft.AspNetCore.Components/Reflection/IPropertySetter.cs index 061580f2ca84..62bab3467fb6 100644 --- a/src/Components/src/Microsoft.AspNetCore.Components/Reflection/IPropertySetter.cs +++ b/src/Components/src/Microsoft.AspNetCore.Components/Reflection/IPropertySetter.cs @@ -1,4 +1,4 @@ -// Copyright (c) .NET Foundation. All rights reserved. +// Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; @@ -8,5 +8,7 @@ namespace Microsoft.AspNetCore.Components.Reflection internal interface IPropertySetter { void SetValue(object target, object value); + + object GetDefaultValue(); } } diff --git a/src/Components/src/Microsoft.AspNetCore.Components/Reflection/MemberAssignment.cs b/src/Components/src/Microsoft.AspNetCore.Components/Reflection/MemberAssignment.cs index eff055a1c7c8..5c9ed16e2fa5 100644 --- a/src/Components/src/Microsoft.AspNetCore.Components/Reflection/MemberAssignment.cs +++ b/src/Components/src/Microsoft.AspNetCore.Components/Reflection/MemberAssignment.cs @@ -1,4 +1,4 @@ -// Copyright (c) .NET Foundation. All rights reserved. +// Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; @@ -43,15 +43,23 @@ public static IPropertySetter CreatePropertySetter(Type targetType, PropertyInfo class PropertySetter : IPropertySetter { private readonly Action _setterDelegate; + private object _defaultValue; public PropertySetter(MethodInfo setMethod) { _setterDelegate = (Action)Delegate.CreateDelegate( typeof(Action), setMethod); + var propertyType = typeof(TValue); + _defaultValue = propertyType.IsValueType ? Activator.CreateInstance(propertyType) : null; } public void SetValue(object target, object value) => _setterDelegate((TTarget)target, (TValue)value); + + public object GetDefaultValue() + { + return _defaultValue; + } } } } diff --git a/src/Components/src/Microsoft.AspNetCore.Components/Routing/Router.cs b/src/Components/src/Microsoft.AspNetCore.Components/Routing/Router.cs index 8ccbfc119bc0..82b15251c8b6 100644 --- a/src/Components/src/Microsoft.AspNetCore.Components/Routing/Router.cs +++ b/src/Components/src/Microsoft.AspNetCore.Components/Routing/Router.cs @@ -45,7 +45,7 @@ public void Init(RenderHandle renderHandle) /// public void SetParameters(ParameterCollection parameters) { - parameters.AssignToProperties(this); + parameters.SetParameterProperties(this); var types = ComponentResolver.ResolveComponents(AppAssembly); Routes = RouteTable.Create(types); Refresh(); diff --git a/src/Components/test/Microsoft.AspNetCore.Components.Test/ParameterCollectionAssignmentExtensionsTest.cs b/src/Components/test/Microsoft.AspNetCore.Components.Test/ParameterCollectionAssignmentExtensionsTest.cs index cc8d62191113..a3c4ba3d767d 100644 --- a/src/Components/test/Microsoft.AspNetCore.Components.Test/ParameterCollectionAssignmentExtensionsTest.cs +++ b/src/Components/test/Microsoft.AspNetCore.Components.Test/ParameterCollectionAssignmentExtensionsTest.cs @@ -27,7 +27,7 @@ public void IncomingParameterMatchesAnnotatedPrivateProperty_SetsValue() var target = new HasInstanceProperties(); // Act - parameterCollection.AssignToProperties(target); + parameterCollection.SetParameterProperties(target); // Assert Assert.Equal(123, target.IntProp); @@ -46,7 +46,7 @@ public void IncomingParameterMatchesDeclaredParameterCaseInsensitively_SetsValue var target = new HasInstanceProperties(); // Act - parameterCollection.AssignToProperties(target); + parameterCollection.SetParameterProperties(target); // Assert Assert.Equal(123, target.IntProp); @@ -64,7 +64,7 @@ public void IncomingParameterMatchesInheritedDeclaredParameter_SetsValue() var target = new HasInheritedProperties(); // Act - parameterCollection.AssignToProperties(target); + parameterCollection.SetParameterProperties(target); // Assert Assert.Equal(123, target.IntProp); @@ -72,7 +72,7 @@ public void IncomingParameterMatchesInheritedDeclaredParameter_SetsValue() } [Fact] - public void NoIncomingParameterMatchesDeclaredParameter_LeavesValueUnchanged() + public void NoIncomingParameterMatchesDeclaredParameter_SetValuesDefault() { // Arrange var existingObjectValue = new object(); @@ -86,12 +86,12 @@ public void NoIncomingParameterMatchesDeclaredParameter_LeavesValueUnchanged() var parameterCollection = new ParameterCollectionBuilder().Build(); // Act - parameterCollection.AssignToProperties(target); + parameterCollection.SetParameterProperties(target); // Assert - Assert.Equal(456, target.IntProp); - Assert.Equal("Existing value", target.StringProp); - Assert.Same(existingObjectValue, target.ObjectPropCurrentValue); + Assert.Equal(0, target.IntProp); + Assert.Null(target.StringProp); + Assert.Null(target.ObjectPropCurrentValue); } [Fact] @@ -106,7 +106,7 @@ public void IncomingParameterMatchesNoDeclaredParameter_Throws() // Act var ex = Assert.Throws( - () => parameterCollection.AssignToProperties(target)); + () => parameterCollection.SetParameterProperties(target)); // Assert Assert.Equal( @@ -127,7 +127,7 @@ public void IncomingParameterMatchesPropertyNotDeclaredAsParameter_Throws() // Act var ex = Assert.Throws( - () => parameterCollection.AssignToProperties(target)); + () => parameterCollection.SetParameterProperties(target)); // Assert Assert.Equal(default, target.IntProp); @@ -150,7 +150,7 @@ public void IncomingParameterValueMismatchesDeclaredParameterType_Throws() // Act var ex = Assert.Throws( - () => parameterCollection.AssignToProperties(target)); + () => parameterCollection.SetParameterProperties(target)); // Assert Assert.Equal( @@ -171,7 +171,7 @@ public void PropertyExplicitSetterException_Throws() // Act var ex = Assert.Throws( - () => parameterCollection.AssignToProperties(target)); + () => parameterCollection.SetParameterProperties(target)); // Assert Assert.Equal( @@ -189,7 +189,7 @@ public void DeclaredParametersVaryOnlyByCase_Throws() // Act var ex = Assert.Throws(() => - parameterCollection.AssignToProperties(target)); + parameterCollection.SetParameterProperties(target)); // Assert Assert.Equal( @@ -205,14 +205,14 @@ public void DeclaredParameterClashesWithInheritedParameter_Throws() // an allowed scenario because there would be no way for the consumer to specify // both property values, and it's no good leaving the shadowed one unset because the // base class can legitimately depend on it for correct functioning. - + // Arrange var parameterCollection = new ParameterCollectionBuilder().Build(); var target = new HasParameterClashingWithInherited(); // Act var ex = Assert.Throws(() => - parameterCollection.AssignToProperties(target)); + parameterCollection.SetParameterProperties(target)); // Assert Assert.Equal( @@ -226,7 +226,7 @@ class HasInstanceProperties { // "internal" to show we're not requiring public accessors, but also // to keep the assertions simple in the tests - + [Parameter] internal int IntProp { get; set; } [Parameter] internal string StringProp { get; set; } diff --git a/src/Components/test/Microsoft.AspNetCore.Components.Test/RenderTreeDiffBuilderTest.cs b/src/Components/test/Microsoft.AspNetCore.Components.Test/RenderTreeDiffBuilderTest.cs index 53dd093061dc..4866be353f56 100644 --- a/src/Components/test/Microsoft.AspNetCore.Components.Test/RenderTreeDiffBuilderTest.cs +++ b/src/Components/test/Microsoft.AspNetCore.Components.Test/RenderTreeDiffBuilderTest.cs @@ -1557,7 +1557,7 @@ private class FakeComponent : IComponent public void Init(RenderHandle renderHandle) { } public void SetParameters(ParameterCollection parameters) { - parameters.AssignToProperties(this); + parameters.SetParameterProperties(this); } } diff --git a/src/Components/test/Microsoft.AspNetCore.Components.Test/RendererTest.cs b/src/Components/test/Microsoft.AspNetCore.Components.Test/RendererTest.cs index 6d84c590cb58..5fe8b7909753 100644 --- a/src/Components/test/Microsoft.AspNetCore.Components.Test/RendererTest.cs +++ b/src/Components/test/Microsoft.AspNetCore.Components.Test/RendererTest.cs @@ -1202,7 +1202,7 @@ public void Init(RenderHandle renderHandle) => RenderHandle = renderHandle; public void SetParameters(ParameterCollection parameters) - => parameters.AssignToProperties(this); + => parameters.SetParameterProperties(this); } private class EventComponent : AutoRenderComponent, IComponent, IHandleEvent @@ -1310,7 +1310,7 @@ public void Init(RenderHandle renderHandle) public void SetParameters(ParameterCollection parameters) { - parameters.AssignToProperties(this); + parameters.SetParameterProperties(this); Render(); } diff --git a/src/Components/test/shared/AutoRenderComponent.cs b/src/Components/test/shared/AutoRenderComponent.cs index b28d045a8565..05e7ad289575 100644 --- a/src/Components/test/shared/AutoRenderComponent.cs +++ b/src/Components/test/shared/AutoRenderComponent.cs @@ -17,7 +17,7 @@ public void Init(RenderHandle renderHandle) public virtual void SetParameters(ParameterCollection parameters) { - parameters.AssignToProperties(this); + parameters.SetParameterProperties(this); TriggerRender(); } From f69c79e14bb481cabc328fe9d8d6c07a1c1796c4 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Thu, 13 Dec 2018 15:14:05 +0000 Subject: [PATCH 2/5] Avoid unnecessary boxing of default values --- .../ParameterCollectionExtensions.cs | 2 +- .../Reflection/IPropertySetter.cs | 4 +--- .../Reflection/MemberAssignment.cs | 8 ++------ 3 files changed, 4 insertions(+), 10 deletions(-) diff --git a/src/Components/src/Microsoft.AspNetCore.Components/ParameterCollectionExtensions.cs b/src/Components/src/Microsoft.AspNetCore.Components/ParameterCollectionExtensions.cs index eb255223eb27..65acb071a2fc 100644 --- a/src/Components/src/Microsoft.AspNetCore.Components/ParameterCollectionExtensions.cs +++ b/src/Components/src/Microsoft.AspNetCore.Components/ParameterCollectionExtensions.cs @@ -66,7 +66,7 @@ public static void SetParameterProperties( foreach (var nonUsedParameter in localParameterWriter) { - nonUsedParameter.SetValue(target, nonUsedParameter.GetDefaultValue()); + nonUsedParameter.SetDefaultValue(target); } } diff --git a/src/Components/src/Microsoft.AspNetCore.Components/Reflection/IPropertySetter.cs b/src/Components/src/Microsoft.AspNetCore.Components/Reflection/IPropertySetter.cs index 62bab3467fb6..8cb862433f59 100644 --- a/src/Components/src/Microsoft.AspNetCore.Components/Reflection/IPropertySetter.cs +++ b/src/Components/src/Microsoft.AspNetCore.Components/Reflection/IPropertySetter.cs @@ -1,14 +1,12 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. -using System; - namespace Microsoft.AspNetCore.Components.Reflection { internal interface IPropertySetter { void SetValue(object target, object value); - object GetDefaultValue(); + void SetDefaultValue(object target); } } diff --git a/src/Components/src/Microsoft.AspNetCore.Components/Reflection/MemberAssignment.cs b/src/Components/src/Microsoft.AspNetCore.Components/Reflection/MemberAssignment.cs index 5c9ed16e2fa5..ede570059599 100644 --- a/src/Components/src/Microsoft.AspNetCore.Components/Reflection/MemberAssignment.cs +++ b/src/Components/src/Microsoft.AspNetCore.Components/Reflection/MemberAssignment.cs @@ -43,23 +43,19 @@ public static IPropertySetter CreatePropertySetter(Type targetType, PropertyInfo class PropertySetter : IPropertySetter { private readonly Action _setterDelegate; - private object _defaultValue; public PropertySetter(MethodInfo setMethod) { _setterDelegate = (Action)Delegate.CreateDelegate( typeof(Action), setMethod); var propertyType = typeof(TValue); - _defaultValue = propertyType.IsValueType ? Activator.CreateInstance(propertyType) : null; } public void SetValue(object target, object value) => _setterDelegate((TTarget)target, (TValue)value); - public object GetDefaultValue() - { - return _defaultValue; - } + public void SetDefaultValue(object target) + => _setterDelegate((TTarget)target, default); } } } From b35a7e45469dbcf5e91e50e2a827b87df3875337 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Thu, 13 Dec 2018 16:39:48 +0000 Subject: [PATCH 3/5] Make SetParameterProperties free of allocations --- .../Microsoft.AspNetCore.Components.csproj | 3 +- .../ParameterCollectionExtensions.cs | 117 ++++++++++++------ 2 files changed, 78 insertions(+), 42 deletions(-) diff --git a/src/Components/src/Microsoft.AspNetCore.Components/Microsoft.AspNetCore.Components.csproj b/src/Components/src/Microsoft.AspNetCore.Components/Microsoft.AspNetCore.Components.csproj index 8c211bdda190..bf2746fc612d 100644 --- a/src/Components/src/Microsoft.AspNetCore.Components/Microsoft.AspNetCore.Components.csproj +++ b/src/Components/src/Microsoft.AspNetCore.Components/Microsoft.AspNetCore.Components.csproj @@ -1,8 +1,9 @@ - + netstandard2.0 Components feature for ASP.NET Core. + true diff --git a/src/Components/src/Microsoft.AspNetCore.Components/ParameterCollectionExtensions.cs b/src/Components/src/Microsoft.AspNetCore.Components/ParameterCollectionExtensions.cs index 65acb071a2fc..436f8f6d29dd 100644 --- a/src/Components/src/Microsoft.AspNetCore.Components/ParameterCollectionExtensions.cs +++ b/src/Components/src/Microsoft.AspNetCore.Components/ParameterCollectionExtensions.cs @@ -5,7 +5,6 @@ using System; using System.Collections.Concurrent; using System.Collections.Generic; -using System.Linq; using System.Reflection; namespace Microsoft.AspNetCore.Components @@ -17,15 +16,16 @@ public static class ParameterCollectionExtensions { private const BindingFlags _bindablePropertyFlags = BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.IgnoreCase; - private readonly static IDictionary> _cachedParameterWriters = new ConcurrentDictionary>(); + private readonly static ConcurrentDictionary _cachedWritersByType + = new ConcurrentDictionary(); /// - /// Iterates through the , assigning each parameter - /// to a property of the same name on . + /// For each parameter property on , updates its value to + /// match the corresponding entry in the . /// /// The . /// An object that has a public writable property matching each parameter's name and type. - public static void SetParameterProperties( + public unsafe static void SetParameterProperties( in this ParameterCollection parameterCollection, object target) { @@ -35,26 +35,36 @@ public static void SetParameterProperties( } var targetType = target.GetType(); - if (!_cachedParameterWriters.TryGetValue(targetType, out var parameterWriters)) + if (!_cachedWritersByType.TryGetValue(targetType, out var writers)) { - parameterWriters = CreateParameterWriters(targetType); - _cachedParameterWriters[targetType] = parameterWriters; + writers = new WritersForType(targetType); + _cachedWritersByType[targetType] = writers; } - var localParameterWriter = parameterWriters.Values.ToList(); + // We only want to iterate through the parameterCollection once, and by the end of it, + // need to have tracked which of the parameter properties haven't yet been written. + // To avoid allocating any list/dictionary to track that, here we stackalloc an array + // of flags and set them based on the indices of the writers we use. + var numWriters = writers.WritersByIndex.Count; + var numUsedWriters = 0; + + // TODO: Once we're able to move to netstandard2.1, this can be changed to be + // a Span and then the enclosing method no longer needs to be 'unsafe' + bool* usageFlags = stackalloc bool[numWriters]; foreach (var parameter in parameterCollection) { var parameterName = parameter.Name; - if (!parameterWriters.TryGetValue(parameterName, out var parameterWriter)) + if (!writers.WritersByName.TryGetValue(parameterName, out var writerWithIndex)) { ThrowForUnknownIncomingParameterName(targetType, parameterName); } try { - parameterWriter.SetValue(target, parameter.Value); - localParameterWriter.Remove(parameterWriter); + writerWithIndex.Writer.SetValue(target, parameter.Value); + usageFlags[writerWithIndex.Index] = true; + numUsedWriters++; } catch (Exception ex) { @@ -64,44 +74,27 @@ public static void SetParameterProperties( } } - foreach (var nonUsedParameter in localParameterWriter) - { - nonUsedParameter.SetDefaultValue(target); - } - } - - internal static IEnumerable GetCandidateBindableProperties(Type targetType) - => MemberAssignment.GetPropertiesIncludingInherited(targetType, _bindablePropertyFlags); - - private static IDictionary CreateParameterWriters(Type targetType) - { - var result = new Dictionary(StringComparer.OrdinalIgnoreCase); - - foreach (var propertyInfo in GetCandidateBindableProperties(targetType)) + // Now we can determine whether any writers have not been used, and if there are + // some unused ones, find them. + for (var index = 0; numUsedWriters < numWriters; index++) { - var shouldCreateWriter = propertyInfo.IsDefined(typeof(ParameterAttribute)) - || propertyInfo.IsDefined(typeof(CascadingParameterAttribute)); - if (!shouldCreateWriter) + if (index >= numWriters) { - continue; + // This should not be possible + throw new InvalidOperationException("Ran out of writers before marking them all as used."); } - var propertySetter = MemberAssignment.CreatePropertySetter(targetType, propertyInfo); - - var propertyName = propertyInfo.Name; - if (result.ContainsKey(propertyName)) + if (!usageFlags[index]) { - throw new InvalidOperationException( - $"The type '{targetType.FullName}' declares more than one parameter matching the " + - $"name '{propertyName.ToLowerInvariant()}'. Parameter names are case-insensitive and must be unique."); + writers.WritersByIndex[index].SetDefaultValue(target); + numUsedWriters++; } - - result.Add(propertyName, propertySetter); } - - return result; } + internal static IEnumerable GetCandidateBindableProperties(Type targetType) + => MemberAssignment.GetPropertiesIncludingInherited(targetType, _bindablePropertyFlags); + private static void ThrowForUnknownIncomingParameterName(Type targetType, string parameterName) { // We know we're going to throw by this stage, so it doesn't matter that the following @@ -129,5 +122,47 @@ private static void ThrowForUnknownIncomingParameterName(Type targetType, string $"matching the name '{parameterName}'."); } } + + class WritersForType + { + public Dictionary WritersByName { get; } + public List WritersByIndex { get; } + + public WritersForType(Type targetType) + { + var propertySettersByName = new Dictionary(StringComparer.OrdinalIgnoreCase); + foreach (var propertyInfo in GetCandidateBindableProperties(targetType)) + { + var shouldCreateWriter = propertyInfo.IsDefined(typeof(ParameterAttribute)) + || propertyInfo.IsDefined(typeof(CascadingParameterAttribute)); + if (!shouldCreateWriter) + { + continue; + } + + var propertySetter = MemberAssignment.CreatePropertySetter(targetType, propertyInfo); + + var propertyName = propertyInfo.Name; + if (propertySettersByName.ContainsKey(propertyName)) + { + throw new InvalidOperationException( + $"The type '{targetType.FullName}' declares more than one parameter matching the " + + $"name '{propertyName.ToLowerInvariant()}'. Parameter names are case-insensitive and must be unique."); + } + + propertySettersByName.Add(propertyName, propertySetter); + } + + // Now we know all the entries, construct the resulting list/dictionary + // with well-defined indices + WritersByIndex = new List(); + WritersByName = new Dictionary(StringComparer.OrdinalIgnoreCase); + foreach (var pair in propertySettersByName) + { + WritersByName.Add(pair.Key, (WritersByIndex.Count, pair.Value)); + WritersByIndex.Add(pair.Value); + } + } + } } } From 41755173c9cf88fa7f8e7aafa1ca9305b7b23865 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Thu, 13 Dec 2018 16:48:11 +0000 Subject: [PATCH 4/5] E2E test showing the new parameter removal capability works --- .../Tests/RoutingTest.cs | 10 ++++++++++ .../test/testapps/BasicTestApp/RouterTest/Links.cshtml | 3 ++- .../BasicTestApp/RouterTest/WithParameters.cshtml | 1 + 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/Components/test/Microsoft.AspNetCore.Components.E2ETest/Tests/RoutingTest.cs b/src/Components/test/Microsoft.AspNetCore.Components.E2ETest/Tests/RoutingTest.cs index f7d0f69e633c..146b629f9be3 100644 --- a/src/Components/test/Microsoft.AspNetCore.Components.E2ETest/Tests/RoutingTest.cs +++ b/src/Components/test/Microsoft.AspNetCore.Components.E2ETest/Tests/RoutingTest.cs @@ -203,7 +203,17 @@ public void CanFollowLinkToPageWithParameters() var app = MountTestComponent(); app.FindElement(By.LinkText("With parameters")).Click(); + WaitAssert.Equal("Your full name is Abc .", () => app.FindElement(By.Id("test-info")).Text); + AssertHighlightedLinks("With parameters"); + + // Can add more parameters while remaining on same page + app.FindElement(By.LinkText("With more parameters")).Click(); WaitAssert.Equal("Your full name is Abc McDef.", () => app.FindElement(By.Id("test-info")).Text); + AssertHighlightedLinks("With parameters", "With more parameters"); + + // Can remove parameters while remaining on same page + app.FindElement(By.LinkText("With parameters")).Click(); + WaitAssert.Equal("Your full name is Abc .", () => app.FindElement(By.Id("test-info")).Text); AssertHighlightedLinks("With parameters"); } diff --git a/src/Components/test/testapps/BasicTestApp/RouterTest/Links.cshtml b/src/Components/test/testapps/BasicTestApp/RouterTest/Links.cshtml index 10c401898f76..e849ba75f7f0 100644 --- a/src/Components/test/testapps/BasicTestApp/RouterTest/Links.cshtml +++ b/src/Components/test/testapps/BasicTestApp/RouterTest/Links.cshtml @@ -10,7 +10,8 @@
  • Other with base-relative URL (matches all)
  • Other with query
  • Other with hash
    • -
  • With parameters
    • +
  • With parameters
    • +
  • With more parameters