From 408b4f7a7ca5bffa27436264f5bf3b81cf251ce7 Mon Sep 17 00:00:00 2001 From: Kevin Hahn Date: Thu, 30 Jul 2026 09:09:09 +0700 Subject: [PATCH 1/2] Make OpaqueChange fallback opt-in and EntityType nullable Unknown IChange $type handling is now configurable via HarmonyConfig.UnknownChangeHandling, defaulting to Throw. Callers opt into OpaqueChange fallback with UnknownChangeHandling.Fallback. OpaqueChange.EntityType now returns null instead of throwing, and IChange.EntityType is nullable to reflect that. Co-Authored-By: Claude Opus 4.8 --- src/SIL.Harmony.Tests/ChangeConverterTests.cs | 18 +++++++++++- src/SIL.Harmony/Changes/Change.cs | 2 +- src/SIL.Harmony/Changes/OpaqueChange.cs | 3 +- .../PeekThenConcreteChangeConverter.cs | 12 ++++++-- src/SIL.Harmony/Config/HarmonyConfig.cs | 28 ++++++++++++++----- .../Config/UnknownChangeHandling.cs | 13 +++++++++ 6 files changed, 63 insertions(+), 13 deletions(-) create mode 100644 src/SIL.Harmony/Config/UnknownChangeHandling.cs diff --git a/src/SIL.Harmony.Tests/ChangeConverterTests.cs b/src/SIL.Harmony.Tests/ChangeConverterTests.cs index 039252b..9f413e8 100644 --- a/src/SIL.Harmony.Tests/ChangeConverterTests.cs +++ b/src/SIL.Harmony.Tests/ChangeConverterTests.cs @@ -1,6 +1,7 @@ using System.Text.Json; using Microsoft.Extensions.DependencyInjection; using SIL.Harmony.Changes; +using SIL.Harmony.Config; using SIL.Harmony.Sample; using SIL.Harmony.Sample.Changes; @@ -8,9 +9,10 @@ namespace SIL.Harmony.Tests; public class ChangeConverterTests { - private static JsonSerializerOptions SampleOptions() => + private static JsonSerializerOptions SampleOptions(UnknownChangeHandling handling = UnknownChangeHandling.Fallback) => new ServiceCollection() .AddCrdtDataSample(":memory:") + .Configure(c => c.UnknownChangeHandling = handling) .BuildServiceProvider() .GetRequiredService(); @@ -45,6 +47,20 @@ public void Unknown_type_deserializes_to_OpaqueChange() opaque.RawJson.GetProperty("Priority").GetInt32().Should().Be(7); opaque.SupportsNewEntity().Should().BeFalse(); opaque.SupportsApplyChange().Should().BeFalse(); + opaque.EntityType.Should().BeNull(); + } + + [Fact] + public void Unknown_type_throws_when_fallback_disabled() + { + var options = SampleOptions(UnknownChangeHandling.Throw); + var json = """ + {"$type":"SetWordPriorityChange","EntityId":"aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee","Priority":7} + """; + + var act = () => JsonSerializer.Deserialize(json, options); + + act.Should().Throw().WithMessage("*SetWordPriorityChange*"); } [Fact] diff --git a/src/SIL.Harmony/Changes/Change.cs b/src/SIL.Harmony/Changes/Change.cs index c816403..68ecc5b 100644 --- a/src/SIL.Harmony/Changes/Change.cs +++ b/src/SIL.Harmony/Changes/Change.cs @@ -15,7 +15,7 @@ public interface IChange Guid EntityId { get; set; } [JsonIgnore] - Type EntityType { get; } + Type? EntityType { get; } ValueTask ApplyChange(IObjectBase entity, IChangeContext context); ValueTask NewEntity(Commit commit, IChangeContext context); diff --git a/src/SIL.Harmony/Changes/OpaqueChange.cs b/src/SIL.Harmony/Changes/OpaqueChange.cs index 2a33314..c09a44b 100644 --- a/src/SIL.Harmony/Changes/OpaqueChange.cs +++ b/src/SIL.Harmony/Changes/OpaqueChange.cs @@ -13,8 +13,7 @@ public sealed class OpaqueChange : IChange public Guid EntityId { get; set; } - public Type EntityType => - throw new NotSupportedException($"Opaque change '{TypeName}' has no known entity type."); + public Type? EntityType => null; public ValueTask ApplyChange(IObjectBase entity, IChangeContext context) => default; diff --git a/src/SIL.Harmony/Changes/PeekThenConcreteChangeConverter.cs b/src/SIL.Harmony/Changes/PeekThenConcreteChangeConverter.cs index b129b07..0c13bed 100644 --- a/src/SIL.Harmony/Changes/PeekThenConcreteChangeConverter.cs +++ b/src/SIL.Harmony/Changes/PeekThenConcreteChangeConverter.cs @@ -2,6 +2,7 @@ using System.Text.Json; using System.Text.Json.Serialization; using System.Text.Json.Serialization.Metadata; +using SIL.Harmony.Config; namespace SIL.Harmony.Changes; @@ -9,17 +10,20 @@ namespace SIL.Harmony.Changes; /// Owns discrimination. Requires $type as the first JSON property /// (matching synthetic write order from ). /// Known discriminators deserialize via cached concrete ; -/// unknown → preserving the raw payload. +/// an unknown discriminator either throws or falls back to +/// (preserving the raw payload), depending on . /// internal sealed class PeekThenConcreteChangeConverter : JsonConverter { private readonly KnownType[] _known; private readonly byte[] _discriminatorPropertyUtf8; + private readonly UnknownChangeHandling _unknownChangeHandling; - public PeekThenConcreteChangeConverter(IReadOnlyDictionary known) + public PeekThenConcreteChangeConverter(IReadOnlyDictionary known, UnknownChangeHandling unknownChangeHandling) { _discriminatorPropertyUtf8 = Encoding.UTF8.GetBytes(CrdtConstants.ChangeDiscriminatorProperty); _known = known.Select(kv => new KnownType(Encoding.UTF8.GetBytes(kv.Key), kv.Value)).ToArray(); + _unknownChangeHandling = unknownChangeHandling; } public override IChange Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) @@ -42,6 +46,10 @@ public override IChange Read(ref Utf8JsonReader reader, Type typeToConvert, Json if (!TryFindKnown(ref reader, out var knownIndex, out var unknownTypeName)) { + if (_unknownChangeHandling != UnknownChangeHandling.Fallback) + throw new JsonException( + $"Unknown IChange \"{CrdtConstants.ChangeDiscriminatorProperty}\" discriminator '{unknownTypeName}'"); + reader = checkpoint; return ReadOpaque(ref reader, unknownTypeName!); } diff --git a/src/SIL.Harmony/Config/HarmonyConfig.cs b/src/SIL.Harmony/Config/HarmonyConfig.cs index 65556aa..1e34850 100644 --- a/src/SIL.Harmony/Config/HarmonyConfig.cs +++ b/src/SIL.Harmony/Config/HarmonyConfig.cs @@ -21,6 +21,12 @@ public class HarmonyConfig /// after adding any commit validate the commit history, not great for performance but good for testing. /// public bool AlwaysValidateCommits { get; set; } = true; + /// + /// Controls how an unknown $type is handled during deserialization. + /// Defaults to ; set to + /// to preserve unknown changes as . + /// + public UnknownChangeHandling UnknownChangeHandling { get; set; } = UnknownChangeHandling.Throw; public ChangeTypeListBuilder ChangeTypeListBuilder { get; } = new(); public IReadOnlyList ChangeTypes => ChangeTypeListBuilder.Types; public ObjectTypeListBuilder ObjectTypeListBuilder { get; } = new(); @@ -38,17 +44,25 @@ public HarmonyConfig() private JsonSerializerOptions CreateJsonSerializerOptions() { - var changeDiscriminators = _lazyChangeDiscriminatorMaps.Value; - var options = new JsonSerializerOptions(JsonSerializerDefaults.General) - { - TypeInfoResolver = MakeJsonTypeResolver() - }; - options.Converters.Add(new PeekThenConcreteChangeConverter(changeDiscriminators.ByDiscriminator)); - _jsonOptionsBuilder.ApplyTo(options); + var options = new JsonSerializerOptions(JsonSerializerDefaults.General); + ConfigureExternalJsonOptions(options); return options; } + /// + /// Configures for Harmony's serialization of and types. + /// Also applies any callbacks registered via . + /// + public void ConfigureExternalJsonOptions(JsonSerializerOptions options) + { + var changeDiscriminators = _lazyChangeDiscriminatorMaps.Value; + options.TypeInfoResolver = options.TypeInfoResolver?.WithAddedModifier(MakeJsonTypeModifier()) + ?? MakeJsonTypeResolver(); + options.Converters.Add(new PeekThenConcreteChangeConverter(changeDiscriminators.ByDiscriminator, UnknownChangeHandling)); + _jsonOptionsBuilder.ApplyTo(options); + } + /// /// Registers a callback to customize before they are frozen. /// Callbacks run after Harmony's type resolver and change converter are configured. diff --git a/src/SIL.Harmony/Config/UnknownChangeHandling.cs b/src/SIL.Harmony/Config/UnknownChangeHandling.cs new file mode 100644 index 0000000..925349c --- /dev/null +++ b/src/SIL.Harmony/Config/UnknownChangeHandling.cs @@ -0,0 +1,13 @@ +namespace SIL.Harmony.Config; + +/// +/// Controls how PeekThenConcreteChangeConverter handles an +/// whose $type is not registered on this client. +/// +public enum UnknownChangeHandling +{ + /// Throw a on an unknown $type (default). + Throw, + /// Fall back to , preserving the raw JSON so it round-trips. + Fallback, +} From fc5f8fb7faa0e5e63e6f872e4bcde5fb6f00da2f Mon Sep 17 00:00:00 2001 From: Kevin Hahn Date: Thu, 30 Jul 2026 09:11:13 +0700 Subject: [PATCH 2/2] skip perf test in debug rather than fail --- src/SIL.Harmony.Tests/DataModelPerformanceTests.cs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/SIL.Harmony.Tests/DataModelPerformanceTests.cs b/src/SIL.Harmony.Tests/DataModelPerformanceTests.cs index 5eca65c..8a76489 100644 --- a/src/SIL.Harmony.Tests/DataModelPerformanceTests.cs +++ b/src/SIL.Harmony.Tests/DataModelPerformanceTests.cs @@ -17,12 +17,13 @@ namespace SIL.Harmony.Tests; [Trait("Category", "Performance")] public class DataModelPerformanceTests(ITestOutputHelper output) { - [Fact] - public void AddingChangePerformance() - { + [Fact( #if DEBUG - Assert.Fail("This test is disabled in debug builds, not reliable"); + Skip = "This test is disabled in debug builds, not reliable" #endif + )] + public void AddingChangePerformance() + { var summary = BenchmarkRunner.Run( ManualConfig.CreateEmpty()