Skip to content

Fix PropertyGetter to handle value types correctly in SupplyParameterFromPersistentComponentStateValueProvider #62369

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

Merged
merged 8 commits into from
Jun 18, 2025
6 changes: 4 additions & 2 deletions src/Components/Components/src/Reflection/PropertyGetter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,10 @@ public PropertyGetter(Type targetType, PropertyInfo property)

var propertyGetterAsFunc =
getMethod.CreateDelegate(typeof(Func<,>).MakeGenericType(targetType, property.PropertyType));

var callPropertyGetterClosedGenericMethod =
CallPropertyGetterOpenGenericMethod.MakeGenericMethod(targetType, property.PropertyType);

_GetterDelegate = (Func<object, object>)
callPropertyGetterClosedGenericMethod.CreateDelegate(typeof(Func<object, object>), propertyGetterAsFunc);
}
Expand All @@ -46,11 +48,11 @@ public PropertyGetter(Type targetType, PropertyInfo property)

public object? GetValue(object target) => _GetterDelegate(target);

private static TValue CallPropertyGetter<TTarget, TValue>(
private static object? CallPropertyGetter<TTarget, TValue>(
Copy link
Member

Choose a reason for hiding this comment

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

We need to box the value here as opposed to later

Copy link
Member

Choose a reason for hiding this comment

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

@copilot don't do anything. This is just an explanatory comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in commit 3522b26 - added explicit boxing using (object?) cast to ensure value is boxed within the method rather than at the return boundary.

Func<TTarget, TValue> Getter,
object target)
where TTarget : notnull
{
return Getter((TTarget)target);
return (object?)Getter((TTarget)target);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Text.Json;
using Microsoft.AspNetCore.Components.Infrastructure;
Expand Down Expand Up @@ -431,6 +432,146 @@ public async Task PersistenceFails_MultipleComponentsUseInvalidKeyTypes(object c
Assert.Contains(sink.Writes, w => w is { LogLevel: LogLevel.Error } && w.EventId == new EventId(1000, "PersistenceCallbackError"));
}

[Fact]
public async Task PersistAsync_CanPersistValueTypes_IntProperty()
Copy link
Member

Choose a reason for hiding this comment

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

Not necessarily for now. I know it's only tests but most of the logic in these methods is common. It would be easier to read.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. I did get my buddy copilot to generate them, so it copy/pasted based on existing tests.

{
// Arrange
var state = new Dictionary<string, byte[]>();
var store = new TestStore(state);
var persistenceManager = new ComponentStatePersistenceManager(
NullLogger<ComponentStatePersistenceManager>.Instance,
new ServiceCollection().BuildServiceProvider());

var renderer = new TestRenderer();
var component = new ValueTypeTestComponent { IntValue = 42 };
var componentStates = CreateComponentState(renderer, [(component, null)], null);
var componentState = componentStates.First();

// Create the provider and subscribe the component
var provider = new SupplyParameterFromPersistentComponentStateValueProvider(persistenceManager.State);
var cascadingParameterInfo = CreateCascadingParameterInfo(nameof(ValueTypeTestComponent.IntValue), typeof(int));
provider.Subscribe(componentState, cascadingParameterInfo);

// Act
await persistenceManager.PersistStateAsync(store, renderer);

// Assert
Assert.NotEmpty(store.State);

// Verify the value was persisted correctly
var newState = new PersistentComponentState(new Dictionary<string, byte[]>(), []);
newState.InitializeExistingState(store.State);

var key = SupplyParameterFromPersistentComponentStateValueProvider.ComputeKey(componentState, cascadingParameterInfo.PropertyName);
Assert.True(newState.TryTakeFromJson<int>(key, out var retrievedValue));
Assert.Equal(42, retrievedValue);
}

[Fact]
public async Task PersistAsync_CanPersistValueTypes_NullableIntProperty()
{
// Arrange
var state = new Dictionary<string, byte[]>();
var store = new TestStore(state);
var persistenceManager = new ComponentStatePersistenceManager(
NullLogger<ComponentStatePersistenceManager>.Instance,
new ServiceCollection().BuildServiceProvider());

var renderer = new TestRenderer();
var component = new ValueTypeTestComponent { NullableIntValue = 123 };
var componentStates = CreateComponentState(renderer, [(component, null)], null);
var componentState = componentStates.First();

// Create the provider and subscribe the component
var provider = new SupplyParameterFromPersistentComponentStateValueProvider(persistenceManager.State);
var cascadingParameterInfo = CreateCascadingParameterInfo(nameof(ValueTypeTestComponent.NullableIntValue), typeof(int?));
provider.Subscribe(componentState, cascadingParameterInfo);

// Act
await persistenceManager.PersistStateAsync(store, renderer);

// Assert
Assert.NotEmpty(store.State);

// Verify the value was persisted correctly
var newState = new PersistentComponentState(new Dictionary<string, byte[]>(), []);
newState.InitializeExistingState(store.State);

var key = SupplyParameterFromPersistentComponentStateValueProvider.ComputeKey(componentState, cascadingParameterInfo.PropertyName);
Assert.True(newState.TryTakeFromJson<int?>(key, out var retrievedValue));
Assert.Equal(123, retrievedValue);
}

[Fact]
public async Task PersistAsync_CanPersistValueTypes_TupleProperty()
{
// Arrange
var state = new Dictionary<string, byte[]>();
var store = new TestStore(state);
var persistenceManager = new ComponentStatePersistenceManager(
NullLogger<ComponentStatePersistenceManager>.Instance,
new ServiceCollection().BuildServiceProvider());

var renderer = new TestRenderer();
var component = new ValueTypeTestComponent { TupleValue = ("test", 456) };
var componentStates = CreateComponentState(renderer, [(component, null)], null);
var componentState = componentStates.First();

// Create the provider and subscribe the component
var provider = new SupplyParameterFromPersistentComponentStateValueProvider(persistenceManager.State);
var cascadingParameterInfo = CreateCascadingParameterInfo(nameof(ValueTypeTestComponent.TupleValue), typeof((string, int)));
provider.Subscribe(componentState, cascadingParameterInfo);

// Act
await persistenceManager.PersistStateAsync(store, renderer);

// Assert
Assert.NotEmpty(store.State);

// Verify the value was persisted correctly
var newState = new PersistentComponentState(new Dictionary<string, byte[]>(), []);
newState.InitializeExistingState(store.State);

var key = SupplyParameterFromPersistentComponentStateValueProvider.ComputeKey(componentState, cascadingParameterInfo.PropertyName);
Assert.True(newState.TryTakeFromJson<(string, int)>(key, out var retrievedValue));
Assert.Equal(("test", 456), retrievedValue);
}

[Fact]
public async Task PersistAsync_CanPersistValueTypes_NullableTupleProperty()
{
// Arrange
var state = new Dictionary<string, byte[]>();
var store = new TestStore(state);
var persistenceManager = new ComponentStatePersistenceManager(
NullLogger<ComponentStatePersistenceManager>.Instance,
new ServiceCollection().BuildServiceProvider());

var renderer = new TestRenderer();
var component = new ValueTypeTestComponent { NullableTupleValue = ("test2", 789) };
var componentStates = CreateComponentState(renderer, [(component, null)], null);
var componentState = componentStates.First();

// Create the provider and subscribe the component
var provider = new SupplyParameterFromPersistentComponentStateValueProvider(persistenceManager.State);
var cascadingParameterInfo = CreateCascadingParameterInfo(nameof(ValueTypeTestComponent.NullableTupleValue), typeof((string, int)?));
provider.Subscribe(componentState, cascadingParameterInfo);

// Act
await persistenceManager.PersistStateAsync(store, renderer);

// Assert
Assert.NotEmpty(store.State);

// Verify the value was persisted correctly
var newState = new PersistentComponentState(new Dictionary<string, byte[]>(), []);
newState.InitializeExistingState(store.State);

var key = SupplyParameterFromPersistentComponentStateValueProvider.ComputeKey(componentState, cascadingParameterInfo.PropertyName);
Assert.True(newState.TryTakeFromJson<(string, int)?>(key, out var retrievedValue));
Assert.Equal(("test2", 789), retrievedValue);
}

private static void InitializeState(PersistentComponentState state, List<(ComponentState componentState, string propertyName, string value)> items)
{
var dictionary = new Dictionary<string, byte[]>();
Expand All @@ -452,7 +593,7 @@ private static CascadingParameterInfo CreateCascadingParameterInfo(string proper

private static List<ComponentState> CreateComponentState(
TestRenderer renderer,
List<(TestComponent, object)> components,
List<(IComponent, object)> components,
ParentComponent parentComponent = null)
{
var i = 1;
Expand All @@ -464,7 +605,20 @@ private static List<ComponentState> CreateComponentState(
var componentState = new ComponentState(renderer, i++, component, parentComponentState);
if (currentRenderTree != null && key != null)
{
currentRenderTree.OpenComponent<TestComponent>(0);
// Open component based on the actual component type
if (component is TestComponent)
{
currentRenderTree.OpenComponent<TestComponent>(0);
}
else if (component is ValueTypeTestComponent)
{
currentRenderTree.OpenComponent<ValueTypeTestComponent>(0);
}
else
{
currentRenderTree.OpenComponent<IComponent>(0);
}

var frames = currentRenderTree.GetFrames();
frames.Array[frames.Count - 1].ComponentStateField = componentState;
if (key != null)
Expand Down Expand Up @@ -497,6 +651,24 @@ private class TestComponent : IComponent
public Task SetParametersAsync(ParameterView parameters) => throw new NotImplementedException();
}

private class ValueTypeTestComponent : IComponent
{
[SupplyParameterFromPersistentComponentState]
public int IntValue { get; set; }

[SupplyParameterFromPersistentComponentState]
public int? NullableIntValue { get; set; }

[SupplyParameterFromPersistentComponentState]
public (string, int) TupleValue { get; set; }

[SupplyParameterFromPersistentComponentState]
public (string, int)? NullableTupleValue { get; set; }

public void Attach(RenderHandle renderHandle) => throw new NotImplementedException();
public Task SetParametersAsync(ParameterView parameters) => throw new NotImplementedException();
}

private class TestStore(Dictionary<string, byte[]> initialState) : IPersistentComponentStateStore
{
public IDictionary<string, byte[]> State { get; set; } = initialState;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,6 @@ internal static class JsonSerializerOptionsProvider
{
PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
PropertyNameCaseInsensitive = true,
IncludeFields = true,
Copy link
Member

Choose a reason for hiding this comment

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

This fix is for ValueTuples

};
}
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,6 @@
public class TestServiceProvider : IServiceProvider
{
public object GetService(Type serviceType)
=> throw new NotImplementedException();
=> null;
Copy link
Member

Choose a reason for hiding this comment

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

These tests were failing.

I don't know how they got to pass on the Validation PR (and for that to get merged).

https://github.com/dotnet/aspnetcore/runs/44279307019

Copy link
Member

Choose a reason for hiding this comment

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

It was throwing when trying to retrieve the validation options, the fix is to just return null.

}
}
Loading