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] Take cascading parameter attribute type into account when supplying cascading values #48554

Merged
merged 14 commits into from
Jun 16, 2023
Merged
21 changes: 15 additions & 6 deletions src/Components/Components/src/Binding/CascadingModelBinder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ namespace Microsoft.AspNetCore.Components;
/// <summary>
/// Defines the binding context for data bound from external sources.
/// </summary>
public sealed class CascadingModelBinder : IComponent, ICascadingValueComponent, IDisposable
public sealed class CascadingModelBinder : IComponent, ICascadingValueSupplier, IDisposable
{
private RenderHandle _handle;
private ModelBindingContext? _bindingContext;
Expand Down Expand Up @@ -42,6 +42,8 @@ public sealed class CascadingModelBinder : IComponent, ICascadingValueComponent,

[Inject] internal IFormValueSupplier FormValueSupplier { get; set; } = null!;

internal ModelBindingContext? BindingContext => _bindingContext;

void IComponent.Attach(RenderHandle renderHandle)
{
_handle = renderHandle;
Expand Down Expand Up @@ -142,8 +144,15 @@ void IDisposable.Dispose()
Navigation.LocationChanged -= HandleLocationChanged;
}

bool ICascadingValueComponent.CanSupplyValue(Type valueType, string? valueName)
bool ICascadingValueSupplier.CanSupplyValue(in CascadingParameterInfo parameterInfo)
{
if (parameterInfo.Attribute is not IFormValueCascadingParameterAttribute formCascadingParameterAttribute)
{
return false;
}

var valueType = parameterInfo.PropertyType;
var valueName = formCascadingParameterAttribute.Name;
var formName = string.IsNullOrEmpty(valueName) ?
(_bindingContext?.Name) :
ModelBindingContext.Combine(_bindingContext, valueName);
Expand Down Expand Up @@ -174,21 +183,21 @@ bool ICascadingValueComponent.CanSupplyValue(Type valueType, string? valueName)
return false;
}

void ICascadingValueComponent.Subscribe(ComponentState subscriber)
void ICascadingValueSupplier.Subscribe(ComponentState subscriber)
{
throw new InvalidOperationException("Form values are always fixed.");
}

void ICascadingValueComponent.Unsubscribe(ComponentState subscriber)
void ICascadingValueSupplier.Unsubscribe(ComponentState subscriber)
{
throw new InvalidOperationException("Form values are always fixed.");
}

object? ICascadingValueComponent.CurrentValue => _bindingInfo == null ?
object? ICascadingValueSupplier.GetCurrentValue(in CascadingParameterInfo parameterInfo) => _bindingInfo == null ?
throw new InvalidOperationException("Tried to access form value before it was bound.") :
_bindingInfo.BoundValue;

bool ICascadingValueComponent.CurrentValueIsFixed => true;
bool ICascadingValueSupplier.CurrentValueIsFixed => true;

private record BindingInfo(string? FormName, Type ValueType, bool BindingResult, object? BoundValue);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

namespace Microsoft.AspNetCore.Components.Binding;

internal interface IFormValueCascadingParameterAttribute : ICascadingParameterAttribute
{
}
MackinnonBuck marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ namespace Microsoft.AspNetCore.Components;
/// supplies values with a compatible type and name.
/// </summary>
[AttributeUsage(AttributeTargets.Property, AllowMultiple = false, Inherited = true)]
public sealed class CascadingParameterAttribute : Attribute
public sealed class CascadingParameterAttribute : Attribute, ICascadingParameterAttribute
{
/// <summary>
/// If specified, the parameter value will be supplied by the closest
Expand Down
11 changes: 11 additions & 0 deletions src/Components/Components/src/CascadingParameterInfo.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

namespace Microsoft.AspNetCore.Components;

internal readonly struct CascadingParameterInfo(ICascadingParameterAttribute attribute, string propertyName, Type propertyType)
{
public ICascadingParameterAttribute Attribute { get; } = attribute;
public string PropertyName { get; } = propertyName;
public Type PropertyType { get; } = propertyType;
}
73 changes: 22 additions & 51 deletions src/Components/Components/src/CascadingParameterState.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,21 +13,21 @@ namespace Microsoft.AspNetCore.Components;

internal readonly struct CascadingParameterState
{
private static readonly ConcurrentDictionary<Type, ReflectedCascadingParameterInfo[]> _cachedInfos = new();
private static readonly ConcurrentDictionary<Type, CascadingParameterInfo[]> _cachedInfos = new();

public string LocalValueName { get; }
public ICascadingValueComponent ValueSupplier { get; }
public CascadingParameterInfo ParameterInfo { get; }
public ICascadingValueSupplier ValueSupplier { get; }

public CascadingParameterState(string localValueName, ICascadingValueComponent valueSupplier)
public CascadingParameterState(in CascadingParameterInfo parameterInfo, ICascadingValueSupplier valueSupplier)
{
LocalValueName = localValueName;
ParameterInfo = parameterInfo;
ValueSupplier = valueSupplier;
}

public static IReadOnlyList<CascadingParameterState> FindCascadingParameters(ComponentState componentState)
{
var componentType = componentState.Component.GetType();
var infos = GetReflectedCascadingParameterInfos(componentType);
var infos = GetCascadingParameterInfos(componentType);

// For components known not to have any cascading parameters, bail out early
if (infos.Length == 0)
Expand All @@ -48,23 +48,21 @@ public static IReadOnlyList<CascadingParameterState> FindCascadingParameters(Com
{
// Although not all parameters might be matched, we know the maximum number
resultStates ??= new List<CascadingParameterState>(infos.Length - infoIndex);

resultStates.Add(new CascadingParameterState(info.ConsumerValueName, supplier));
resultStates.Add(new CascadingParameterState(info, supplier));
}
}

return resultStates ?? (IReadOnlyList<CascadingParameterState>)Array.Empty<CascadingParameterState>();
}

private static ICascadingValueComponent? GetMatchingCascadingValueSupplier(in ReflectedCascadingParameterInfo info, ComponentState componentState)
private static ICascadingValueSupplier? GetMatchingCascadingValueSupplier(in CascadingParameterInfo info, ComponentState componentState)
{
var candidate = componentState;
do
{
if (candidate.Component is ICascadingValueComponent candidateSupplier
&& candidateSupplier.CanSupplyValue(info.ValueType, info.SupplierValueName))
if (candidate.Component is ICascadingValueSupplier valueSupplier && valueSupplier.CanSupplyValue(info))
{
return candidateSupplier;
return valueSupplier;
}

candidate = candidate.ParentComponentState;
Expand All @@ -74,64 +72,37 @@ public static IReadOnlyList<CascadingParameterState> FindCascadingParameters(Com
return null;
}

private static ReflectedCascadingParameterInfo[] GetReflectedCascadingParameterInfos(
private static CascadingParameterInfo[] GetCascadingParameterInfos(
[DynamicallyAccessedMembers(Component)] Type componentType)
{
if (!_cachedInfos.TryGetValue(componentType, out var infos))
{
infos = CreateReflectedCascadingParameterInfos(componentType);
infos = CreateCascadingParameterInfos(componentType);
_cachedInfos[componentType] = infos;
}

return infos;
}

private static ReflectedCascadingParameterInfo[] CreateReflectedCascadingParameterInfos(
private static CascadingParameterInfo[] CreateCascadingParameterInfos(
[DynamicallyAccessedMembers(Component)] Type componentType)
{
List<ReflectedCascadingParameterInfo>? result = null;
List<CascadingParameterInfo>? result = null;
var candidateProps = ComponentProperties.GetCandidateBindableProperties(componentType);
foreach (var prop in candidateProps)
{
var attribute = prop.GetCustomAttribute<CascadingParameterAttribute>();
if (attribute != null)
var cascadingParameterAttribute = prop.GetCustomAttributes()
.OfType<ICascadingParameterAttribute>().SingleOrDefault();
if (cascadingParameterAttribute != null)
Copy link
Member

@javiercn javiercn Jun 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to think about what happens when you have [SupplyParameterFromQuery] and [SupplyParameterFromFrom] attribute. There are some cases that might be "incompatible", but there are other cases where we might want both attributes to apply. For example, if we do [SupplyParameterFromPrerenderedState]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, true. I wonder if it would be fine to modify this code slightly to add a ReflectedCascadingParameterInfo for each attribute on the parameter property. Maybe we could also add something like an Order property to ICascadingParameterAttribute so that, for example, if [SupplyParameterFromQuery] and [SupplyParameterFromForm] are both present, we prioritize supplying a value from a form when possible, but fall back to supplying a parameter from the query if form data is not available.

Is this something you think should be addressed by this PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great points to raise. It would definitely be consistent with other parts of the framework if we had an Order property.

I don't think you should feel required to implement that in this PR but if there's broad agreement about the direction, it would be good to file an issue summarizing the design then we can hopefully get on and implement that soon.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW we likely end up with a fairly clear, straightforwards design if:

  • CascadingParameterAttribute is unsealed
  • ... and gains a new Order property
  • ... and we change it to AllowMultiple = true

Then we can cheaply have form/query subclasses in a different assembly, and the core doesn't need to know or care about anything but the base class.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CascadingParameterAttribute I'd rather not open up the ability for third-parties to create their own definitions of Cascading values by extending/overriding CascadingParameterAttribute.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to address that in #48554 (comment). Can you give more details about what you're concerned about?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we can cheaply have form/query subclasses in a different assembly, and the core doesn't need to know or care about anything but the base class.

I like this, but wouldn't CascadingModelBinder and CascadingQueryValueProvider still need to know what attribute type was used so that, in the case of only one attribute being present, they don't supply values for each other (or for CascadingValue) by accident? Or would we be okay with, for example, a parameter annotated with [CascadingValue] receiving values from the query or a form request?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect CascadingModelBinder and CascadingQueryValueProvider to be in layers where they are able to know about the corresponding CascadingParameterAttribute subclass and hence can do the same type check that you already have.

{
result ??= new List<ReflectedCascadingParameterInfo>();

result.Add(new ReflectedCascadingParameterInfo(
prop.Name,
prop.PropertyType,
attribute.Name));
}

var hostParameterAttribute = prop.GetCustomAttributes()
.OfType<IHostEnvironmentCascadingParameter>().SingleOrDefault();
if (hostParameterAttribute != null)
{
result ??= new List<ReflectedCascadingParameterInfo>();

result.Add(new ReflectedCascadingParameterInfo(
result ??= new List<CascadingParameterInfo>();
result.Add(new CascadingParameterInfo(
cascadingParameterAttribute,
prop.Name,
prop.PropertyType,
hostParameterAttribute.Name));
prop.PropertyType));
}
}

return result?.ToArray() ?? Array.Empty<ReflectedCascadingParameterInfo>();
}

readonly struct ReflectedCascadingParameterInfo
{
public string ConsumerValueName { get; }
public string? SupplierValueName { get; }
public Type ValueType { get; }

public ReflectedCascadingParameterInfo(
string consumerValueName, Type valueType, string? supplierValueName)
{
ConsumerValueName = consumerValueName;
SupplierValueName = supplierValueName;
ValueType = valueType;
}
return result?.ToArray() ?? Array.Empty<CascadingParameterInfo>();
}
}
108 changes: 108 additions & 0 deletions src/Components/Components/src/CascadingQueryValueProvider.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using Microsoft.AspNetCore.Components.Rendering;
using Microsoft.AspNetCore.Components.Routing;

namespace Microsoft.AspNetCore.Components;

internal sealed class CascadingQueryValueProvider : IComponent, ICascadingValueSupplier, IDisposable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have expected this to be done directly within CascadingModelBinder and not as a separate component. CascadingModelBinder in my view, is responsible for all the binding operations related to [SupplyParameterFromXXX]

{
private readonly QueryParameterValueSupplier _queryParameterValueSupplier = new();

private HashSet<ComponentState>? _subscribers;
private RenderHandle _handle;
private bool _hasSetInitialParameters;

[Inject] private NavigationManager Navigation { get; set; } = default!;

[Parameter] public RenderFragment? ChildContent { get; set; }

void IComponent.Attach(RenderHandle renderHandle)
{
_handle = renderHandle;
}

Task IComponent.SetParametersAsync(ParameterView parameters)
{
if (!_hasSetInitialParameters)
{
_hasSetInitialParameters = true;

UpdateQueryParameterValues();

Navigation.LocationChanged += HandleLocationChanged;
}

parameters.SetParameterProperties(this);

_handle.Render(Render);

return Task.CompletedTask;
}

private void HandleLocationChanged(object? sender, LocationChangedEventArgs e)
=> UpdateQueryParameterValues();

private void UpdateQueryParameterValues()
{
var query = GetQueryString(Navigation.Uri);

_queryParameterValueSupplier.ReadParametersFromQuery(query);

if (_subscribers is null)
{
return;
}

foreach (var subscriber in _subscribers)
{
subscriber.NotifyCascadingValueChanged(ParameterViewLifetime.Unbound);
}

static ReadOnlyMemory<char> GetQueryString(string url)
{
var queryStartPos = url.IndexOf('?');

if (queryStartPos < 0)
{
return default;
}

var queryEndPos = url.IndexOf('#', queryStartPos);
return url.AsMemory(queryStartPos..(queryEndPos < 0 ? url.Length : queryEndPos));
}
}

private void Render(RenderTreeBuilder builder)
{
builder.AddContent(0, ChildContent);
}

bool ICascadingValueSupplier.CanSupplyValue(in CascadingParameterInfo parameterInfo)
=> parameterInfo.Attribute is SupplyParameterFromQueryAttribute;

bool ICascadingValueSupplier.CurrentValueIsFixed => false;

object? ICascadingValueSupplier.GetCurrentValue(in CascadingParameterInfo parameterInfo)
{
var queryParameterName = parameterInfo.Attribute.Name ?? parameterInfo.PropertyName;
return _queryParameterValueSupplier.GetQueryParameterValue(parameterInfo.PropertyType, queryParameterName);
}

void ICascadingValueSupplier.Subscribe(ComponentState subscriber)
{
_subscribers ??= new();
_subscribers.Add(subscriber);
}

void ICascadingValueSupplier.Unsubscribe(ComponentState subscriber)
{
_subscribers?.Remove(subscriber);
}

void IDisposable.Dispose()
{
Navigation.LocationChanged -= HandleLocationChanged;
}
}
26 changes: 16 additions & 10 deletions src/Components/Components/src/CascadingValue.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ namespace Microsoft.AspNetCore.Components;
/// <summary>
/// A component that provides a cascading value to all descendant components.
/// </summary>
public class CascadingValue<TValue> : ICascadingValueComponent, IComponent
public class CascadingValue<TValue> : ICascadingValueSupplier, IComponent
{
private RenderHandle _renderHandle;
private HashSet<ComponentState>? _subscribers; // Lazily instantiated
Expand Down Expand Up @@ -41,9 +41,7 @@ public class CascadingValue<TValue> : ICascadingValueComponent, IComponent
/// </summary>
[Parameter] public bool IsFixed { get; set; }

object? ICascadingValueComponent.CurrentValue => Value;

bool ICascadingValueComponent.CurrentValueIsFixed => IsFixed;
bool ICascadingValueSupplier.CurrentValueIsFixed => IsFixed;

/// <inheritdoc />
public void Attach(RenderHandle renderHandle)
Expand Down Expand Up @@ -130,18 +128,26 @@ public Task SetParametersAsync(ParameterView parameters)
return Task.CompletedTask;
}

bool ICascadingValueComponent.CanSupplyValue(Type requestedType, string? requestedName)
bool ICascadingValueSupplier.CanSupplyValue(in CascadingParameterInfo parameterInfo)
{
if (!requestedType.IsAssignableFrom(typeof(TValue)))
if (parameterInfo.Attribute is not CascadingParameterAttribute cascadingParameterAttribute || !parameterInfo.PropertyType.IsAssignableFrom(typeof(TValue)))
{
return false;
}

return (requestedName == null && Name == null) // Match on type alone
|| string.Equals(requestedName, Name, StringComparison.OrdinalIgnoreCase); // Also match on name
// We only consider explicitly specified names, not the property name.
var parameterSpecifiedName = cascadingParameterAttribute.Name;

return (parameterSpecifiedName == null && Name == null) || // Match on type alone
string.Equals(parameterSpecifiedName, Name, StringComparison.OrdinalIgnoreCase); // Also match on name
}

object? ICascadingValueSupplier.GetCurrentValue(in CascadingParameterInfo parameterInfo)
{
return Value;
}

void ICascadingValueComponent.Subscribe(ComponentState subscriber)
void ICascadingValueSupplier.Subscribe(ComponentState subscriber)
{
#if DEBUG
if (IsFixed)
Expand All @@ -160,7 +166,7 @@ void ICascadingValueComponent.Subscribe(ComponentState subscriber)
_subscribers.Add(subscriber);
}

void ICascadingValueComponent.Unsubscribe(ComponentState subscriber)
void ICascadingValueSupplier.Unsubscribe(ComponentState subscriber)
{
_subscribers?.Remove(subscriber);
}
Expand Down
Loading