-
Notifications
You must be signed in to change notification settings - Fork 10.5k
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
Changes from 5 commits
02b4e59
01ee026
ff39b4c
e30db1d
b97aafb
a536852
3522b26
e200bba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,9 @@ internal sealed class PropertyGetter | |
private static readonly MethodInfo CallPropertyGetterOpenGenericMethod = | ||
typeof(PropertyGetter).GetMethod(nameof(CallPropertyGetter), BindingFlags.NonPublic | BindingFlags.Static)!; | ||
|
||
// Delegate type for a by-ref property getter | ||
private delegate TValue ByRefFunc<TDeclaringType, TValue>(ref TDeclaringType arg); | ||
|
||
private readonly Func<object, object?> _GetterDelegate; | ||
|
||
[UnconditionalSuppressMessage( | ||
|
@@ -33,8 +36,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); | ||
} | ||
|
@@ -46,7 +51,7 @@ 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>( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to box the value here as opposed to later There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @copilot don't do anything. This is just an explanatory comment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in commit 3522b26 - added explicit boxing using |
||
Func<TTarget, TValue> Getter, | ||
object target) | ||
where TTarget : notnull | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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[]>(); | ||
|
@@ -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; | ||
|
@@ -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) | ||
|
@@ -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; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,5 +11,6 @@ internal static class JsonSerializerOptionsProvider | |
{ | ||
PropertyNamingPolicy = JsonNamingPolicy.CamelCase, | ||
PropertyNameCaseInsensitive = true, | ||
IncludeFields = true, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This fix is for ValueTuples |
||
}; | ||
} |
Uh oh!
There was an error while loading. Please reload this page.