[Blazor] Allow properties marked with both [Parameter] and [SupplyParameterFromQuery] to receive values directly (#48954) (#49038)

Author: MackinnonBuck

src/Components/Components/src/ParameterView.cs

@@ -16,7 +16,7 @@ public readonly struct ParameterView
 {
     private static readonly RenderTreeFrame[] _emptyFrames = new RenderTreeFrame[]
     {
-            RenderTreeFrame.Element(0, string.Empty).WithComponentSubtreeLength(1)
+        RenderTreeFrame.Element(0, string.Empty).WithComponentSubtreeLength(1)
     };
 
     private static readonly ParameterView _empty = new ParameterView(ParameterViewLifetime.Unbound, _emptyFrames, 0, Array.Empty<CascadingParameterState>());
@@ -131,6 +131,20 @@ internal ParameterView Clone()
     internal ParameterView WithCascadingParameters(IReadOnlyList<CascadingParameterState> cascadingParameters)
         => new ParameterView(_lifetime, _frames, _ownerIndex, cascadingParameters);
 
+    internal bool HasDirectParameter(string parameterName)
+    {
+        var directParameterEnumerator = new RenderTreeFrameParameterEnumerator(_frames, _ownerIndex);
+        while (directParameterEnumerator.MoveNext())
+        {
+            if (string.Equals(directParameterEnumerator.Current.Name, parameterName, StringComparison.Ordinal))
+            {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
     // It's internal because there isn't a known use case for user code comparing
     // ParameterView instances, and even if there was, it's unlikely it should
     // use these equality rules which are designed for their effect on rendering.

src/Components/Components/src/Reflection/ComponentProperties.cs

@@ -45,9 +45,9 @@ public static void SetProperties(in ParameterView parameters, object target)
                     ThrowForUnknownIncomingParameterName(targetType, parameterName);
                     throw null; // Unreachable
                 }
-                else if (writer.Cascading && !parameter.Cascading)
+                else if (!writer.AcceptsDirectParameters && !parameter.Cascading)
                 {
-                    // We don't allow you to set a cascading parameter with a non-cascading value. Put another way:
+                    // We don't allow you to set a cascading parameter with a non-cascading (direct) 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.
                     //
@@ -56,13 +56,23 @@ public static void SetProperties(in ParameterView parameters, object target)
                     ThrowForSettingCascadingParameterWithNonCascadingValue(targetType, parameterName);
                     throw null; // Unreachable
                 }
-                else if (!writer.Cascading && parameter.Cascading)
+                else if (!writer.AcceptsCascadingParameters && 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
                 }
+                else if (parameter.Cascading && writer.AcceptsDirectParameters && writer.AcceptsCascadingParameters)
+                {
+                    // Today, the only case where this is possible is when a property is annotated with both
+                    // ParameterAttribute and SupplyParameterFromQueryAttribute. If that happens, we want to
+                    // prefer the directly supplied value over the cascading value.
+                    if (parameters.HasDirectParameter(parameterName))
+                    {
+                        continue;
+                    }
+                }
 
                 SetProperty(target, writer, parameterName, parameter.Value);
             }
@@ -82,7 +92,7 @@ public static void SetProperties(in ParameterView parameters, object target)
 
                 if (writers.TryGetValue(parameterName, out var writer))
                 {
-                    if (!writer.Cascading && parameter.Cascading)
+                    if (!writer.AcceptsCascadingParameters && 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.
@@ -91,7 +101,7 @@ public static void SetProperties(in ParameterView parameters, object target)
                         ThrowForSettingParameterWithCascadingValue(targetType, parameterName);
                         throw null; // Unreachable
                     }
-                    else if (writer.Cascading && !parameter.Cascading)
+                    else if (writer.AcceptsCascadingParameters && !parameter.Cascading)
                     {
                         // 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
@@ -196,8 +206,8 @@ private static void ThrowForUnknownIncomingParameterName([DynamicallyAccessedMem
     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.");
+            $"The property '{parameterName}' on component type '{targetType.FullName}' cannot be set " +
+            $"explicitly because it only accepts cascading values.");
     }
 
     [DoesNotReturn]
@@ -279,8 +289,12 @@ public WritersForType([DynamicallyAccessedMembers(Component)] Type targetType)
                     }
                 }
 
-                var isParameter = parameterAttribute != null || cascadingParameterAttribute != null;
-                if (!isParameter)
+                // A property cannot accept direct parameters if it's annotated with a cascading value attribute, unless it's a
+                // SupplyParameterFromQueryAttribute. This is to retain backwards compatibility with previous versions of the
+                // SupplyParameterFromQuery feature that did not utilize cascading values, and thus did not have this limitation.
+                var acceptsDirectParameters = parameterAttribute is not null && cascadingParameterAttribute is null or SupplyParameterFromQueryAttribute;
+                var acceptsCascadingParameters = cascadingParameterAttribute is not null;
+                if (!acceptsDirectParameters && !acceptsCascadingParameters)
                 {
                     continue;
                 }
@@ -294,7 +308,8 @@ public WritersForType([DynamicallyAccessedMembers(Component)] Type targetType)
 
                 var propertySetter = new PropertySetter(targetType, propertyInfo)
                 {
-                    Cascading = cascadingParameterAttribute != null,
+                    AcceptsDirectParameters = acceptsDirectParameters,
+                    AcceptsCascadingParameters = acceptsCascadingParameters,
                 };
 
                 if (_underlyingWriters.ContainsKey(propertyName))

src/Components/Components/src/Reflection/PropertySetter.cs

@@ -36,7 +36,9 @@ public PropertySetter(Type targetType, PropertyInfo property)
             callPropertySetterClosedGenericMethod.CreateDelegate(typeof(Action<object, object>), propertySetterAsAction);
     }
 
-    public bool Cascading { get; init; }
+    public bool AcceptsDirectParameters { get; init; }
+
+    public bool AcceptsCascadingParameters { get; init; }
 
     public void SetValue(object target, object value) => _setterDelegate(target, value);
 

src/Components/Components/test/ParameterViewTest.Assignment.cs

@@ -237,8 +237,8 @@ public void IncomingNonCascadingValueMatchesCascadingParameter_Throws()
 
         // 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.",
+            $"The property '{nameof(HasCascadingParameter.Cascading)}' on component type '{typeof(HasCascadingParameter).FullName}' " +
+            $"cannot be set explicitly because it only accepts cascading values.",
             ex.Message);
     }
 
@@ -261,6 +261,59 @@ public void IncomingCascadingValueMatchesNonCascadingParameter_Throws()
             ex.Message);
     }
 
+    [Fact]
+    public void IncomingNonCascadingValueMatchesParameterThatIsBothCascadingAndNonCascading_Throws()
+    {
+        // Arrange
+        var target = new HasPropertyWithParameterAndCascadingParameterAttributes();
+        var builder = new ParameterViewBuilder();
+        builder.Add(nameof(HasPropertyWithParameterAndCascadingParameterAttributes.Parameter), "Hello", cascading: false);
+        var parameters = builder.Build();
+
+        // Act
+        var ex = Assert.Throws<InvalidOperationException>(() => parameters.SetParameterProperties(target));
+
+        // Assert
+        Assert.Equal(
+            $"The property '{nameof(HasPropertyWithParameterAndCascadingParameterAttributes.Parameter)}' on component type " +
+            $"'{typeof(HasPropertyWithParameterAndCascadingParameterAttributes).FullName}' cannot be set explicitly because it " +
+            $"only accepts cascading values.",
+            ex.Message);
+    }
+
+    [Fact]
+    public void IncomingCascadingValueMatchesParameterThatIsBothCascadingAndNonCascading_Works()
+    {
+        // Arrange
+        var target = new HasPropertyWithParameterAndCascadingParameterAttributes();
+        var builder = new ParameterViewBuilder();
+        builder.Add(nameof(HasPropertyWithParameterAndCascadingParameterAttributes.Parameter), "Hello", cascading: true);
+        var parameters = builder.Build();
+
+        // Act
+        parameters.SetParameterProperties(target);
+
+        // Assert
+        Assert.Equal("Hello", target.Parameter);
+    }
+
+    [Fact]
+    public void ParameterThatCanBeSuppliedFromQueryOrNonCascadingValue_PrefersNonCascadingValue()
+    {
+        // Arrange
+        var target = new HasPropertyWithParameterAndSupplyParameterFromQueryAttributes();
+        var builder = new ParameterViewBuilder();
+        builder.Add(nameof(HasPropertyWithParameterAndSupplyParameterFromQueryAttributes.Parameter), "Non-cascading", cascading: false);
+        builder.Add(nameof(HasPropertyWithParameterAndSupplyParameterFromQueryAttributes.Parameter), "Cascading", cascading: true);
+        var parameters = builder.Build();
+
+        // Act
+        parameters.SetParameterProperties(target);
+
+        // Assert
+        Assert.Equal("Non-cascading", target.Parameter);
+    }
+
     [Fact]
     public void SettingCaptureUnmatchedValuesParameterExplicitlyWorks()
     {
@@ -644,6 +697,16 @@ class HasNonPublicCascadingParameter
         public string GetCascadingValue() => Cascading;
     }
 
+    class HasPropertyWithParameterAndCascadingParameterAttributes
+    {
+        [Parameter, CascadingParameter] public string Parameter { get; set; }
+    }
+
+    class HasPropertyWithParameterAndSupplyParameterFromQueryAttributes
+    {
+        [Parameter, SupplyParameterFromQuery] public string Parameter { get; set; }
+    }
+
     class ParameterViewBuilder : IEnumerable
     {
         private readonly List<(string Name, object Value, bool Cascading)> _parameters = new();