-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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 all 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 |
---|---|---|
|
@@ -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 |
||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -107,6 +107,6 @@ | |
public class TestServiceProvider : IServiceProvider | ||
{ | ||
public object GetService(Type serviceType) | ||
=> throw new NotImplementedException(); | ||
=> null; | ||
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. These tests were failing. I don't know how they got to pass on the Validation PR (and for that to get merged). 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. It was throwing when trying to retrieve the validation options, the fix is to just return null. |
||
} | ||
} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.