fix: report a command with no parameterless constructor as a load failure [patch]

[matt-edmondson]

Fixes #71.

JsonUndoRedoSerializer.ConvertFromSerializableCommand reconstructs commands with Activator.CreateInstance(commandType), which requires a public parameterless constructor. Real ICommand implementations take their target and the values being changed as constructor arguments, and nothing on ISerializableCommand documented the requirement.

The resulting MissingMethodException is not in LoadStateAsync's exception filter:

catch (Exception ex) when (ex is JsonException or InvalidOperationException or NotSupportedException)
{
	return false;
}

So it escaped a method whose whole Task<bool> contract is to report deserialization failure as false. Saving such a command worked fine (SerializeAsync constructs nothing); loading it back threw.

The change

Two parts, both small:

1. Translate at the source. The Activator.CreateInstance call is wrapped, and MissingMethodException becomes an InvalidOperationException naming the offending type, with the original kept as InnerException. Translating here rather than widening the filter in LoadStateAsync means direct DeserializeAsync callers also get a diagnosable error instead of a bare reflection failure, and LoadStateAsync returns false through the filter it already has.

2. Document the requirement on ISerializableCommand itself, so implementers learn it from the interface rather than at load time. The issue raised this as the longer-term half; enforcement would need an analyzer or a generic constraint, and neither belongs in a patch, so this is documentation only.

MissingMethodException is also what Activator.CreateInstance raises for an abstract type, so that case is covered by the same translation.

Tests

Both verified to fail before the change and pass after it (by stashing the JsonUndoRedoSerializer.cs change and re-running):

Test	Fails without the fix with
UndoRedoService_LoadStateCommandHasNoParameterlessConstructor_ReturnsFalse	MissingMethodException escapes LoadStateAsync instead of it returning false
JsonSerializer_DeserializeCommandHasNoParameterlessConstructor_ThrowsInvalidOperationException	MissingMethodException where InvalidOperationException is the contract

The test command ConstructorOnlySerializableCommand deliberately has only a value-taking constructor — the shape the issue describes as the realistic case. The existing TestSerializableCommand has a parameterless constructor, which is why the existing round-trip tests never saw this.

Full suite: 48 passed, 0 failed, 0 skipped.

🤖 Generated with Claude Code

https://claude.ai/code/session_01T1ntzPwg732TrH7D5vWpSV

---

Generated by Claude Code

[sonarqubecloud]

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[matt-edmondson]

SonarQube's quality gate passed here (100% coverage on new code, 0 duplication, 0 hotspots). Recording what I did with the 4 new issues it counted, since the summary links to them rather than listing them.

All four are the same rule, MSTEST0049 (INFO / maintainability), on the four awaits in the tests this PR adds:

Consider using the overload that accepts a CancellationToken and pass 'TestContext.CancellationToken'

These stay as they are. Not because they're INFO — because adopting them here would make this PR inconsistent with the file it's in:

• TestContext is used nowhere in UndoRedo.Test today, so there's no convention to follow and no partial migration to join.
• The same no-token pattern appears in 16 sibling calls in SerializationTests.cs, including pre-existing ones at lines 21, 28, 53, 54, 72, 95, 129 and 153. Sonar doesn't flag those only because they aren't new code.
• Adopting it would mean adding TestContext plumbing to the class — test-infrastructure work unrelated to the MissingMethodException bug this PR fixes.

If the repo wants MSTEST0049 honoured, it's worth doing as its own sweep across all of UndoRedo.Test rather than in the two tests that happen to be newest. Happy to open that separately if you'd like it.

For contrast, I did act on the lint findings in the sibling PRs where the repo had already established the newer convention: #72 (MSTEST0037 → Assert.HasCount, which the rest of the suite already uses) and ktsu-dev/BlastMerge#34 (MSTEST0068 → Assert.AreSequenceEqual, already used in that repo's newest test files).

---

Generated by Claude Code