Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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
Original file line number Diff line number Diff line change
Expand Up @@ -231,8 +231,24 @@ private static PropertyGetter PropertyGetterFactory((Type type, string propertyN
var propertyInfo = GetPropertyInfo(type, propertyName);
if (propertyInfo == null)
{
// Check if the property exists but is not public to provide a better error message
var nonPublicProperty = type.GetProperty(propertyName, ComponentProperties.BindablePropertyFlags);
if (nonPublicProperty != null)
{
throw new InvalidOperationException(
$"The property '{propertyName}' on component type '{type.FullName}' cannot be used with {nameof(PersistentStateAttribute)} because it is not public. Properties with {nameof(PersistentStateAttribute)} must have a public getter.");
}
Copy link
Member

Choose a reason for hiding this comment

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

Just combine all the checks into a single one with a single error message for all cases. A public property X on component type Y with a public getter wasn't found.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@copilot address the remaining feedback

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Combined all error checks into a single condition with the simplified error message as requested. The new message "A public property '{propertyName}' on component type '{type.FullName}' with a public getter wasn't found" covers all failure scenarios. Tests updated accordingly and pass. (commit edc2fcf)


throw new InvalidOperationException($"Property {propertyName} not found on type {type.FullName}");
}

// Check if the property is public
if (propertyInfo.GetMethod == null || !propertyInfo.GetMethod.IsPublic)
{
throw new InvalidOperationException(
$"The property '{propertyName}' on component type '{type.FullName}' cannot be used with {nameof(PersistentStateAttribute)} because it is not public. Properties with {nameof(PersistentStateAttribute)} must have a public getter.");
}

return new PropertyGetter(type, propertyInfo);

static PropertyInfo? GetPropertyInfo([DynamicallyAccessedMembers(LinkerFlags.Component)] Type type, string propertyName)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -623,4 +623,90 @@ private class TestStore(IDictionary<string, byte[]> state) : IPersistentComponen
public Task<IDictionary<string, byte[]>> GetPersistedStateAsync() => Task.FromResult(state);
public Task PersistStateAsync(IReadOnlyDictionary<string, byte[]> state) => throw new NotImplementedException();
}

private class ComponentWithPrivateProperty : IComponent
{
[PersistentState]
private string PrivateValue { get; set; } = "initial";

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

private class ComponentWithPrivateGetter : IComponent
{
[PersistentState]
public string PropertyWithPrivateGetter { private get; set; } = "initial";

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

[Fact]
public void Constructor_ThrowsClearException_ForPrivateProperty()
{
// Arrange
var state = new PersistentComponentState(new Dictionary<string, byte[]>(), [], []);
state.InitializeExistingState(new Dictionary<string, byte[]>(), RestoreContext.InitialValue);
var renderer = new TestRenderer();
var component = new ComponentWithPrivateProperty();
var componentState = CreateComponentState(renderer, component, null, null);
var cascadingParameterInfo = CreateCascadingParameterInfo("PrivateValue", typeof(string));
var serviceProvider = new ServiceCollection().BuildServiceProvider();
var logger = NullLogger.Instance;

// Act & Assert
var exception = Assert.Throws<InvalidOperationException>(() =>
new PersistentValueProviderComponentSubscription(
state, componentState, cascadingParameterInfo, serviceProvider, logger));

// Should throw a clear error about non-public properties, not "Property not found"
Assert.Contains("not public", exception.Message);
Assert.Contains("PersistentState", exception.Message);
Assert.DoesNotContain("not found", exception.Message);
}

[Fact]
public void Constructor_ThrowsClearException_ForPrivateGetter()
{
// Arrange
var state = new PersistentComponentState(new Dictionary<string, byte[]>(), [], []);
state.InitializeExistingState(new Dictionary<string, byte[]>(), RestoreContext.InitialValue);
var renderer = new TestRenderer();
var component = new ComponentWithPrivateGetter();
var componentState = CreateComponentState(renderer, component, null, null);
var cascadingParameterInfo = CreateCascadingParameterInfo(nameof(ComponentWithPrivateGetter.PropertyWithPrivateGetter), typeof(string));
var serviceProvider = new ServiceCollection().BuildServiceProvider();
var logger = NullLogger.Instance;

// Act & Assert
var exception = Assert.Throws<InvalidOperationException>(() =>
new PersistentValueProviderComponentSubscription(
state, componentState, cascadingParameterInfo, serviceProvider, logger));

// Should throw a clear error about non-public getter
Assert.Contains("not public", exception.Message);
Assert.Contains("PersistentState", exception.Message);
}

[Fact]
public void Constructor_WorksCorrectly_ForPublicProperty()
{
// Arrange
var state = new PersistentComponentState(new Dictionary<string, byte[]>(), [], []);
state.InitializeExistingState(new Dictionary<string, byte[]>(), RestoreContext.InitialValue);
var renderer = new TestRenderer();
var component = new TestComponent { State = "test-value" };
var componentState = CreateComponentState(renderer, component, null, null);
var cascadingParameterInfo = CreateCascadingParameterInfo(nameof(TestComponent.State), typeof(string));
var serviceProvider = new ServiceCollection().BuildServiceProvider();
var logger = NullLogger.Instance;

// Act & Assert - Should not throw
var subscription = new PersistentValueProviderComponentSubscription(
state, componentState, cascadingParameterInfo, serviceProvider, logger);

Assert.NotNull(subscription);
subscription.Dispose();
}
}
Loading