Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Jun 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion src/Components/Components/src/ParameterView.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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>());
Expand Down Expand Up @@ -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.
Expand Down
35 changes: 25 additions & 10 deletions src/Components/Components/src/Reflection/ComponentProperties.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
//
Expand All @@ -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);
}
Expand All @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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;
}
Expand All @@ -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))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
67 changes: 65 additions & 2 deletions src/Components/Components/test/ParameterViewTest.Assignment.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand All @@ -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()
{
Expand Down Expand Up @@ -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();
Expand Down