From 9e10dc8d1391cb46c43bd534ad4dd722301fec4b Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 26 Sep 2026 19:29:54 +0000 Subject: [PATCH] fix: report every unloadable saved command as a failed load [patch] LoadStateAsync promises to return false for data it cannot load, but a malformed assembly-qualified type name threw FileLoadException, a type that implements ISerializableCommand without ICommand threw InvalidCastException, and a command whose DeserializeData rejected its data let that exception escape. Translate each into InvalidOperationException with the original as the inner exception, the way the missing-constructor case already is, and check for ICommand before constructing the command. Fixes #93 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01N4m2HJkrsY88XBtTqTKAkE --- UndoRedo.Test/SerializationTests.cs | 105 ++++++++++++++++++++ UndoRedo/Services/JsonUndoRedoSerializer.cs | 38 ++++++- 2 files changed, 141 insertions(+), 2 deletions(-) diff --git a/UndoRedo.Test/SerializationTests.cs b/UndoRedo.Test/SerializationTests.cs index 7d8b1c5..0086740 100644 --- a/UndoRedo.Test/SerializationTests.cs +++ b/UndoRedo.Test/SerializationTests.cs @@ -467,6 +467,111 @@ public void UndoRedoService_RestoreFromStateNullSaveBoundaries_ReturnsFalseAndKe Assert.HasCount(1, stack.SaveBoundaries, "A failed restore should keep the existing save boundaries"); } + private const string MalformedAssemblyName = "malformed assembly name"; + private const string InvalidVersion = "invalid assembly version"; + private const string NotACommand = "serializable type that is not a command"; + private const string DataParseFailure = "command data its parser rejects"; + + private static async Task SerializeWithCommandTypeAsync(string caseName) + { + string type = caseName switch + { + MalformedAssemblyName => "Foo, =bad", + InvalidVersion => "Foo, Bar, Version=abc", + NotACommand => typeof(SerializableNonCommand).AssemblyQualifiedName!, + DataParseFailure => typeof(IntParsingSerializableCommand).AssemblyQualifiedName!, + _ => throw new ArgumentOutOfRangeException(nameof(caseName)), + }; + + JsonUndoRedoSerializer serializer = new(); + byte[] data = await serializer.SerializeAsync([new TestSerializableCommand("saved")], 0, []).ConfigureAwait(false); + System.Text.Json.Nodes.JsonNode root = System.Text.Json.Nodes.JsonNode.Parse(data)!; + System.Text.Json.Nodes.JsonObject command = root["commands"]![0]!.AsObject(); + string typeKey = command.Single(p => p.Key.Equals("type", StringComparison.OrdinalIgnoreCase)).Key; + string dataKey = command.Single(p => p.Key.Equals("data", StringComparison.OrdinalIgnoreCase)).Key; + command[typeKey] = type; + command[dataKey] = "abc"; + return System.Text.Encoding.UTF8.GetBytes(root.ToJsonString()); + } + + [TestMethod] + [DataRow(MalformedAssemblyName)] + [DataRow(InvalidVersion)] + [DataRow(NotACommand)] + [DataRow(DataParseFailure)] + public async Task JsonSerializer_DeserializeUnloadableCommand_ThrowsInvalidOperationException(string caseName) + { + // Arrange + byte[] data = await SerializeWithCommandTypeAsync(caseName).ConfigureAwait(false); + JsonUndoRedoSerializer serializer = new(); + + // Act & Assert: every way a command can fail to load is reported through the deserialization + // contract rather than as the raw reflection, cast or parse error + await Assert.ThrowsExactlyAsync( + () => serializer.DeserializeAsync(data)).ConfigureAwait(false); + } + + [TestMethod] + [DataRow(MalformedAssemblyName)] + [DataRow(InvalidVersion)] + [DataRow(NotACommand)] + [DataRow(DataParseFailure)] + public async Task UndoRedoService_LoadStateUnloadableCommand_ReturnsFalseAndKeepsHistory(string caseName) + { + // Arrange + byte[] data = await SerializeWithCommandTypeAsync(caseName).ConfigureAwait(false); + UndoRedoService stack = CreateService(); + stack.SetSerializer(new JsonUndoRedoSerializer()); + stack.Execute(new DelegateCommand("A", () => { }, () => { })); + + // Act + bool success = await stack.LoadStateAsync(data).ConfigureAwait(false); + + // Assert + Assert.IsFalse(success, "LoadStateAsync should return false when a command cannot be loaded"); + Assert.AreEqual(1, stack.CommandCount, "A failed load should keep the existing commands"); + Assert.AreEqual("A", stack.Commands[0].Description); + } + +#pragma warning disable CA1812 // Instantiated by reflection during deserialization + private sealed class SerializableNonCommand : ISerializableCommand + { + public string SerializeData() => string.Empty; + + public void DeserializeData(string data) + { + // Nothing to restore + } + } +#pragma warning restore CA1812 + +#pragma warning disable CA1812 // Instantiated by reflection during deserialization + private sealed class IntParsingSerializableCommand : BaseCommand, ISerializableCommand + { + public IntParsingSerializableCommand() : base(ChangeType.Modify, ["test"]) + { + } + + public int Value { get; private set; } + + public override string Description => $"Int command with value: {Value}"; + + public override void Execute() + { + // Test implementation + } + + public override void Undo() + { + // Test implementation + } + + public string SerializeData() => Value.ToString(System.Globalization.CultureInfo.InvariantCulture); + + public void DeserializeData(string data) => Value = int.Parse(data, System.Globalization.CultureInfo.InvariantCulture); + } +#pragma warning restore CA1812 + private sealed class ConstructorOnlySerializableCommand(string value) : BaseCommand(ChangeType.Modify, ["test"]), ISerializableCommand { diff --git a/UndoRedo/Services/JsonUndoRedoSerializer.cs b/UndoRedo/Services/JsonUndoRedoSerializer.cs index 8a0d0d4..fd6a895 100644 --- a/UndoRedo/Services/JsonUndoRedoSerializer.cs +++ b/UndoRedo/Services/JsonUndoRedoSerializer.cs @@ -145,9 +145,15 @@ private static ICommand ConvertFromSerializableCommand(SerializableCommand seria } // For commands that implement ISerializableCommand, try to reconstruct them - Type? commandType = Type.GetType(serializableCommand.Type); + Type? commandType = ResolveCommandType(serializableCommand.Type); if (commandType != null && typeof(ISerializableCommand).IsAssignableFrom(commandType)) { + if (!typeof(ICommand).IsAssignableFrom(commandType)) + { + throw new InvalidOperationException( + $"Cannot reconstruct command type '{commandType.FullName}': it implements {nameof(ISerializableCommand)} but not {nameof(ICommand)}."); + } + ISerializableCommand? instance; try { @@ -163,7 +169,21 @@ private static ICommand ConvertFromSerializableCommand(SerializableCommand seria ex); } - instance?.DeserializeData(serializableCommand.Data!); + try + { + instance?.DeserializeData(serializableCommand.Data!); + } +#pragma warning disable CA1031 // Do not catch general exception types + catch (Exception ex) when (ex is not OperationCanceledException) +#pragma warning restore CA1031 // Do not catch general exception types + { + // DeserializeData is the command's own parser, so it can throw anything. Report it + // through the deserialization contract so LoadStateAsync returns false. + throw new InvalidOperationException( + $"Cannot reconstruct command type '{commandType.FullName}': its {nameof(ISerializableCommand.DeserializeData)} rejected the saved data.", + ex); + } + return (ICommand)instance!; } @@ -171,6 +191,20 @@ private static ICommand ConvertFromSerializableCommand(SerializableCommand seria return new PlaceholderCommand(serializableCommand.Description, serializableCommand.NavigationContext, serializableCommand.Metadata); } + private static Type? ResolveCommandType(string typeName) + { + try + { + return Type.GetType(typeName); + } + catch (Exception ex) when (ex is IOException or BadImageFormatException or ArgumentException or TypeLoadException) + { + // Type.GetType returns null for a type it cannot find, but still throws for a malformed + // assembly-qualified name or an assembly that fails to load. + throw new InvalidOperationException($"Cannot resolve command type '{typeName}'.", ex); + } + } + /// /// Serializable representation of a command ///