From 57cc63798975daa6a9fe9e9dca8d7446722db9d7 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 23 Sep 2026 18:28:09 +0000 Subject: [PATCH] fix: report a command with no parameterless constructor as a load failure [patch] JsonUndoRedoSerializer.ConvertFromSerializableCommand reconstructs commands with Activator.CreateInstance(Type), which needs a public parameterless constructor. Real ICommand implementations take their target and values as constructor arguments, and nothing on ISerializableCommand documented the requirement. The resulting MissingMethodException was not in LoadStateAsync's exception filter (JsonException, InvalidOperationException, NotSupportedException), so it escaped a method whose Task contract is to report deserialization failure as false. Saving worked, loading threw. Translate MissingMethodException into an InvalidOperationException naming the offending type, which the existing filter already covers, and preserve the original as InnerException. Also document the parameterless-constructor requirement on ISerializableCommand so implementers learn it from the interface rather than at load time. Adds two tests, both failing before the change: - UndoRedoService_LoadStateCommandHasNoParameterlessConstructor_ReturnsFalse - JsonSerializer_DeserializeCommandHasNoParameterlessConstructor_ThrowsInvalidOperationException Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01T1ntzPwg732TrH7D5vWpSV --- UndoRedo.Test/SerializationTests.cs | 65 +++++++++++++++++++++ UndoRedo/Services/JsonUndoRedoSerializer.cs | 21 ++++++- 2 files changed, 85 insertions(+), 1 deletion(-) diff --git a/UndoRedo.Test/SerializationTests.cs b/UndoRedo.Test/SerializationTests.cs index 7af3f92..7c3237d 100644 --- a/UndoRedo.Test/SerializationTests.cs +++ b/UndoRedo.Test/SerializationTests.cs @@ -323,6 +323,71 @@ public void UndoRedoStackState_Properties_CalculateCorrectly() Assert.IsFalse(state.CanRedo, "State should not allow redo when at end of command stack"); } + [TestMethod] + public async Task UndoRedoService_LoadStateCommandHasNoParameterlessConstructor_ReturnsFalse() + { + // Arrange: a command type whose only constructor takes the value it changes, which is what a + // real ISerializableCommand implementation looks like + UndoRedoService stack = CreateService(); + stack.SetSerializer(new JsonUndoRedoSerializer()); + stack.Execute(new ConstructorOnlySerializableCommand("saved")); + + byte[] data = await stack.SaveStateAsync().ConfigureAwait(false); + + UndoRedoService newStack = CreateService(); + newStack.SetSerializer(new JsonUndoRedoSerializer()); + + // Act: LoadStateAsync reports failure rather than letting MissingMethodException escape + bool success = await newStack.LoadStateAsync(data).ConfigureAwait(false); + + // Assert + Assert.IsFalse(success, "LoadStateAsync should return false when a command cannot be reconstructed"); + Assert.AreEqual(0, newStack.CommandCount, "A failed load should not leave partial state on the stack"); + } + + [TestMethod] + public async Task JsonSerializer_DeserializeCommandHasNoParameterlessConstructor_ThrowsInvalidOperationException() + { + // Arrange + JsonUndoRedoSerializer serializer = new(); + ConstructorOnlySerializableCommand command = new("saved"); + byte[] data = await serializer.SerializeAsync([command], 0, []).ConfigureAwait(false); + + // Act & Assert: the failure is reported as part of the deserialization contract, not as the + // raw reflection error + InvalidOperationException ex = await Assert.ThrowsExactlyAsync( + () => serializer.DeserializeAsync(data)).ConfigureAwait(false); + + Assert.Contains(nameof(ConstructorOnlySerializableCommand), ex.Message, "The message should name the type that could not be reconstructed"); + Assert.IsInstanceOfType(ex.InnerException, "The underlying reflection failure should be preserved"); + } + + private sealed class ConstructorOnlySerializableCommand(string value) + : BaseCommand(ChangeType.Modify, ["test"]), ISerializableCommand + { + public string Value { get; private set; } = value; + + public override string Description => $"Constructor-only command with value: {Value}"; + + public override void Execute() + { + // Test implementation + } + + public override void Undo() + { + // Test implementation + } + + public string SerializeData() => JsonSerializer.Serialize(new { Value }); + + public void DeserializeData(string data) + { + JsonElement element = JsonSerializer.Deserialize(data); + Value = element.GetProperty(nameof(Value)).GetString() ?? string.Empty; + } + } + private sealed class TestSerializableCommand : BaseCommand, ISerializableCommand { public string Value { get; private set; } = string.Empty; diff --git a/UndoRedo/Services/JsonUndoRedoSerializer.cs b/UndoRedo/Services/JsonUndoRedoSerializer.cs index 0f408f8..cb327b2 100644 --- a/UndoRedo/Services/JsonUndoRedoSerializer.cs +++ b/UndoRedo/Services/JsonUndoRedoSerializer.cs @@ -109,7 +109,21 @@ private static ICommand ConvertFromSerializableCommand(SerializableCommand seria Type? commandType = Type.GetType(serializableCommand.Type); if (commandType != null && typeof(ISerializableCommand).IsAssignableFrom(commandType)) { - ISerializableCommand? instance = Activator.CreateInstance(commandType) as ISerializableCommand; + ISerializableCommand? instance; + try + { + instance = Activator.CreateInstance(commandType) as ISerializableCommand; + } + catch (MissingMethodException ex) + { + // Activator.CreateInstance needs a public parameterless constructor, which most real + // command types do not have. Translate it into an exception the deserialization + // contract already covers, so LoadStateAsync reports false instead of throwing. + throw new InvalidOperationException( + $"Cannot reconstruct command type '{commandType.FullName}': {nameof(ISerializableCommand)} implementations must have a public parameterless constructor for DeserializeData to populate.", + ex); + } + instance?.DeserializeData(serializableCommand.Data!); return (ICommand)instance!; } @@ -146,6 +160,11 @@ private sealed class SerializableStackState /// /// Interface for commands that can serialize their data /// +/// +/// Implementations must also provide a public parameterless constructor. Deserialization creates the +/// instance before it has any data to work from, then populates it through . +/// Without one, reconstructing the command fails and LoadStateAsync reports . +/// public interface ISerializableCommand { ///