Cover change converter and config/adapter guard paths

src/SIL.Harmony.Tests/ChangeConverterTests.cs

@@ -9,12 +9,16 @@ namespace SIL.Harmony.Tests;
 
 public class ChangeConverterTests
 {
-    private static JsonSerializerOptions SampleOptions(UnknownChangeHandling handling = UnknownChangeHandling.Fallback) =>
-        new ServiceCollection()
+    private static JsonSerializerOptions SampleOptions(UnknownChangeHandling handling = UnknownChangeHandling.Fallback)
+    {
+        //dispose the provider once the options are built; JsonSerializerOptions is self-contained
+        //(the change converter holds its own type maps) so it stays valid after disposal
+        using var services = new ServiceCollection()
             .AddCrdtDataSample(":memory:")
             .Configure<HarmonyConfig>(c => c.UnknownChangeHandling = handling)
-            .BuildServiceProvider()
-            .GetRequiredService<JsonSerializerOptions>();
+            .BuildServiceProvider();
+        return services.GetRequiredService<JsonSerializerOptions>();
+    }
 
     [Fact]
     public void Happy_path_deserializes_to_concrete_change()
@@ -122,4 +126,116 @@ public void Requires_type_as_first_property()
         var act = () => JsonSerializer.Deserialize<IChange>(json, options);
         act.Should().Throw<JsonException>().WithMessage("*first property*");
     }
+
+    [Fact]
+    public void Deserialize_NonObjectToken_ThrowsExpectedStartObject()
+    {
+        var options = SampleOptions();
+
+        var act = () => JsonSerializer.Deserialize<IChange>("[1,2,3]", options);
+        act.Should().Throw<JsonException>().WithMessage("*StartObject*");
+    }
+
+    [Fact]
+    public void Deserialize_EmptyObject_ThrowsExpectedPropertyName()
+    {
+        var options = SampleOptions();
+
+        var act = () => JsonSerializer.Deserialize<IChange>("{}", options);
+        act.Should().Throw<JsonException>().WithMessage("*property name*");
+    }
+
+    [Fact]
+    public void Deserialize_NonStringDiscriminator_Throws()
+    {
+        var options = SampleOptions();
+        var json = """{"$type":5,"EntityId":"aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"}""";
+
+        var act = () => JsonSerializer.Deserialize<IChange>(json, options);
+        act.Should().Throw<JsonException>().WithMessage("*string*discriminator*");
+    }
+
+    [Fact]
+    public void OpaqueChange_ParsesEntityIdFromRawJson()
+    {
+        var options = SampleOptions();
+        var entityId = Guid.Parse("aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee");
+        var json = $$"""{"$type":"SetWordPriorityChange","EntityId":"{{entityId}}","Priority":7}""";
+
+        var change = JsonSerializer.Deserialize<IChange>(json, options);
+
+        change.Should().BeOfType<OpaqueChange>().Which.EntityId.Should().Be(entityId);
+    }
+
+    [Fact]
+    public void OpaqueChange_MissingEntityId_DefaultsToEmpty()
+    {
+        var options = SampleOptions();
+        var json = """{"$type":"SetWordPriorityChange","Priority":7}""";
+
+        var change = JsonSerializer.Deserialize<IChange>(json, options);
+
+        change.Should().BeOfType<OpaqueChange>().Which.EntityId.Should().Be(Guid.Empty);
+    }
+
+    [Fact]
+    public void OpaqueChange_MalformedEntityId_DefaultsToEmpty()
+    {
+        var options = SampleOptions();
+        //a newer client could send a non-GUID EntityId; the opaque fallback must not throw
+        var json = """{"$type":"SetWordPriorityChange","EntityId":"not-a-guid","Priority":7}""";
+
+        var change = JsonSerializer.Deserialize<IChange>(json, options);
+
+        change.Should().BeOfType<OpaqueChange>().Which.EntityId.Should().Be(Guid.Empty);
+    }
+
+    [Fact]
+    public void OpaqueChange_PreservesNestedJson_OnRoundTrip()
+    {
+        var options = SampleOptions();
+        var json = """
+            {"$type":"SetWordPriorityChange","EntityId":"aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee","Data":{"nested":[1,2],"flag":true}}
+            """;
+
+        var change = JsonSerializer.Deserialize<IChange>(json, options)!;
+        var opaque = change.Should().BeOfType<OpaqueChange>().Subject;
+        opaque.RawJson.GetProperty("Data").GetProperty("flag").GetBoolean().Should().BeTrue();
+
+        var rewritten = JsonSerializer.Serialize(change, options);
+        rewritten.Should().Contain("\"nested\":[1,2]");
+        rewritten.Should().Contain("\"flag\":true");
+    }
+
+    private static OpaqueChange NewOpaque() => new()
+    {
+        TypeName = "UnknownChange",
+        RawJson = JsonDocument.Parse("{}").RootElement.Clone(),
+    };
+
+    [Fact]
+    public void OpaqueChange_EntityType_IsNull()
+    {
+        //an unknown change has no known entity type on this client
+        NewOpaque().EntityType.Should().BeNull();
+    }
+
+    [Fact]
+    public async Task OpaqueChange_NewEntity_Throws()
+    {
+        var commit = new Commit
+        {
+            ClientId = Guid.NewGuid(),
+            HybridDateTime = new HybridDateTime(DateTimeOffset.UtcNow, 0),
+        };
+        var act = async () => await NewOpaque().NewEntity(commit, null!);
+        await act.Should().ThrowAsync<NotSupportedException>();
+    }
+
+    [Fact]
+    public async Task OpaqueChange_ApplyChange_IsNoOp()
+    {
+        //an unknown change applied via sync must not mutate anything or throw
+        await NewOpaque().ApplyChange(null!, null!);
+    }
 }

src/SIL.Harmony.Tests/ConfigTests.cs

@@ -69,4 +69,98 @@ public void ConfigureJsonOptions_throws_after_freeze()
         act.Should().Throw<InvalidOperationException>()
             .WithMessage("*JsonOptionsBuilder* frozen*");
     }
+
+    [Fact]
+    public void ChangeTypeListBuilder_DuplicateAdd_IsIdempotent()
+    {
+        var config = new HarmonyConfig();
+        config.ChangeTypeListBuilder.Add<SetWordTextChange>();
+        config.ChangeTypeListBuilder.Add<SetWordTextChange>();
+
+        config.ChangeTypes.Should().ContainSingle(t => t.Type == typeof(SetWordTextChange));
+    }
+
+    [Fact]
+    public void ChangeTypeListBuilder_AddAfterFreeze_Throws()
+    {
+        var config = new HarmonyConfig();
+        //building the serializer options freezes the change type builder
+        _ = config.JsonSerializerOptions;
+
+        var act = () => config.ChangeTypeListBuilder.Add<SetWordTextChange>();
+
+        act.Should().Throw<InvalidOperationException>().WithMessage("*ChangeTypeListBuilder*frozen*");
+    }
+
+    [Fact]
+    public void ObjectTypeListBuilder_AddAfterFreeze_Throws()
+    {
+        var config = new HarmonyConfig();
+        //capture the adapter before freezing so we can exercise Add (not DefaultAdapter) after freeze
+        var adapter = config.ObjectTypeListBuilder.DefaultAdapter();
+        adapter.Add<Word>();
+        config.ObjectTypeListBuilder.Freeze(); //happens during EF model build in a real setup
+
+        var act = () => adapter.Add<Definition>();
+
+        act.Should().Throw<InvalidOperationException>().WithMessage("*ObjectTypeListBuilder*frozen*");
+    }
+
+    [Fact]
+    public void ObjectTypeListBuilder_DefaultAdapterAfterFreeze_Throws()
+    {
+        var config = new HarmonyConfig();
+        config.ObjectTypeListBuilder.DefaultAdapter().Add<Word>();
+        config.ObjectTypeListBuilder.Freeze();
+
+        var act = () => config.ObjectTypeListBuilder.DefaultAdapter();
+
+        act.Should().Throw<InvalidOperationException>().WithMessage("*ObjectTypeListBuilder*frozen*");
+    }
+
+    [Fact]
+    public void DefaultAdapter_CalledTwice_ReturnsSameInstance()
+    {
+        var config = new HarmonyConfig();
+        var first = config.ObjectTypeListBuilder.DefaultAdapter();
+        var second = config.ObjectTypeListBuilder.DefaultAdapter();
+
+        second.Should().BeSameAs(first);
+    }
+
+    [Fact]
+    public void Adapt_ObjectImplementingIObjectBase_ReturnsIt()
+    {
+        var config = new HarmonyConfig();
+        config.ObjectTypeListBuilder.DefaultAdapter().Add<Word>();
+        var word = new Word { Id = Guid.NewGuid(), Text = "hello" };
+
+        config.ObjectTypeListBuilder.Adapt(word).Should().BeSameAs(word);
+    }
+
+    [Fact]
+    public void Adapt_NonObjectBase_Throws()
+    {
+        var config = new HarmonyConfig();
+        config.ObjectTypeListBuilder.DefaultAdapter().Add<Word>();
+
+        var act = () => config.ObjectTypeListBuilder.Adapt("not an entity");
+
+        act.Should().Throw<ArgumentException>().WithMessage("*does not implement*IObjectBase*");
+    }
+
+    [Fact]
+    public void Adapt_NoProviderMatches_Throws()
+    {
+        var config = new HarmonyConfig();
+        //two providers, so Adapt takes the multi-provider dispatch path rather than the single-adapter fast path
+        config.ObjectTypeListBuilder.DefaultAdapter().Add<Word>();
+        config.ObjectTypeListBuilder
+            .CustomAdapter<CustomObjectAdapterTests.IMyCustomInterface, CustomObjectAdapterTests.MyClassAdapter>()
+            .Add<CustomObjectAdapterTests.MyClass>();
+
+        var act = () => config.ObjectTypeListBuilder.Adapt(new object());
+
+        act.Should().Throw<ArgumentException>().WithMessage("*Unable to adapt*");
+    }
 }

src/SIL.Harmony.Tests/Helpers/DerivedTypeHelperTests.cs

@@ -0,0 +1,42 @@
+using System.Text.Json.Serialization.Metadata;
+using SIL.Harmony.Entities;
+using SIL.Harmony.Helpers;
+using SIL.Harmony.Sample.Models;
+
+namespace SIL.Harmony.Tests.Helpers;
+
+public class DerivedTypeHelperTests
+{
+    [Fact]
+    public void AddDerivedType_Duplicate_Throws()
+    {
+        var types = new Dictionary<Type, List<JsonDerivedType>>();
+        types.AddDerivedType(typeof(IObjectBase), typeof(Word), "Word");
+
+        //different discriminator, same DerivedType: proves the guard rejects the duplicate type, not the discriminator
+        var act = () => types.AddDerivedType(typeof(IObjectBase), typeof(Word), "WordAgain");
+
+        act.Should().Throw<InvalidOperationException>().WithMessage("*already added*");
+    }
+
+    [Fact]
+    public void AddDerivedType_DifferentTypesUnderSameBase_Succeeds()
+    {
+        var types = new Dictionary<Type, List<JsonDerivedType>>();
+        types.AddDerivedType(typeof(IObjectBase), typeof(Word), "Word");
+        types.AddDerivedType(typeof(IObjectBase), typeof(Definition), "Definition");
+
+        types[typeof(IObjectBase)].Should().BeEquivalentTo([
+            new JsonDerivedType(typeof(Word), "Word"),
+            new JsonDerivedType(typeof(Definition), "Definition"),
+        ]);
+    }
+
+    [Fact]
+    public void GetEntityDiscriminator_WhenInstanceTypeIsNotAssignableToBase_Throws()
+    {
+        var act = () => DerivedTypeHelper.GetEntityDiscriminator<IObjectBase>(typeof(string));
+
+        act.Should().Throw<ArgumentException>().WithMessage("*must implement IObjectBase*");
+    }
+}

src/SIL.Harmony/Changes/PeekThenConcreteChangeConverter.cs

@@ -82,8 +82,12 @@ private static OpaqueChange ReadOpaque(ref Utf8JsonReader reader, string typeNam
         return new OpaqueChange
         {
             TypeName = typeName,
-            EntityId = element.TryGetProperty(nameof(IChange.EntityId), out var id) && id.ValueKind == JsonValueKind.String
-                ? id.GetGuid()
+            //use TryGetGuid so a malformed (non-GUID) EntityId from a newer client defaults to empty
+            //instead of throwing FormatException, which would defeat the opaque fallback
+            EntityId = element.TryGetProperty(nameof(IChange.EntityId), out var id)
+                       && id.ValueKind == JsonValueKind.String
+                       && id.TryGetGuid(out var entityId)
+                ? entityId
                 : default,
             RawJson = element
         };