Skip to content

Commit aae2ee0

Browse files
hahn-kev-bothahn-kevcursoragentclaudemyieye
authored
Tolerate unknown IChange $type via PeekThenConcreteChangeConverter (#80)
* Wire PeekThenConcreteChangeConverter for unknown $type tolerance. CrdtConfig owns IChange discrimination with synthetic $type on write; SnapshotWorker skips opaque creates. --------- Co-authored-by: Kevin Hahn <kevin_hahn@sil.org> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Tim Haasdyk <myieye@gmail.com>
1 parent a14c5bb commit aae2ee0

6 files changed

Lines changed: 325 additions & 8 deletions

File tree

‎src/SIL.Harmony.Tests/ChangeConverterTests.cs‎

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
using System.Text.Json;
2+
using Microsoft.Extensions.DependencyInjection;
3+
using SIL.Harmony.Changes;
4+
using SIL.Harmony.Sample;
5+
using SIL.Harmony.Sample.Changes;
6+
7+
namespace SIL.Harmony.Tests;
8+
9+
public class ChangeConverterTests
10+
{
11+
private static JsonSerializerOptions SampleOptions() =>
12+
new ServiceCollection()
13+
.AddCrdtDataSample(":memory:")
14+
.BuildServiceProvider()
15+
.GetRequiredService<JsonSerializerOptions>();
16+
17+
[Fact]
18+
public void Happy_path_deserializes_to_concrete_change()
19+
{
20+
var options = SampleOptions();
21+
var entityId = Guid.Parse("aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee");
22+
IChange change = new SetWordTextChange(entityId, "hello");
23+
24+
var json = JsonSerializer.Serialize(change, options);
25+
var roundTripped = JsonSerializer.Deserialize<IChange>(json, options);
26+
27+
roundTripped.Should().BeOfType<SetWordTextChange>()
28+
.Which.Text.Should().Be("hello");
29+
roundTripped!.EntityId.Should().Be(entityId);
30+
json.Should().StartWith("{\"$type\":\"SetWordTextChange\"");
31+
}
32+
33+
[Fact]
34+
public void Unknown_type_deserializes_to_OpaqueChange()
35+
{
36+
var options = SampleOptions();
37+
var json = """
38+
{"$type":"SetWordPriorityChange","EntityId":"aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee","Priority":7}
39+
""";
40+
41+
var change = JsonSerializer.Deserialize<IChange>(json, options);
42+
43+
var opaque = change.Should().BeOfType<OpaqueChange>().Subject;
44+
opaque.TypeName.Should().Be("SetWordPriorityChange");
45+
opaque.RawJson.GetProperty("Priority").GetInt32().Should().Be(7);
46+
opaque.SupportsNewEntity().Should().BeFalse();
47+
opaque.SupportsApplyChange().Should().BeFalse();
48+
}
49+
50+
[Fact]
51+
public void OpaqueChange_round_trips_original_discriminator()
52+
{
53+
var options = SampleOptions();
54+
var json = """
55+
{"$type":"SetWordPriorityChange","EntityId":"aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee","Priority":7}
56+
""";
57+
58+
var change = JsonSerializer.Deserialize<IChange>(json, options)!;
59+
var rewritten = JsonSerializer.Serialize(change, options);
60+
61+
rewritten.Should().Contain("\"$type\":\"SetWordPriorityChange\"");
62+
rewritten.Should().Contain("\"Priority\":7");
63+
rewritten.Should().NotContain("OpaqueChange");
64+
}
65+
66+
[Fact]
67+
public void Mixed_commit_round_trips_known_and_opaque_changes()
68+
{
69+
var options = SampleOptions();
70+
var entityId = Guid.Parse("aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee");
71+
var commit = new Commit
72+
{
73+
ClientId = Guid.NewGuid(),
74+
HybridDateTime = new HybridDateTime(DateTimeOffset.UtcNow, 0),
75+
};
76+
commit.ChangeEntities.Add(new ChangeEntity<IChange>
77+
{
78+
Index = 0,
79+
CommitId = commit.Id,
80+
EntityId = entityId,
81+
Change = new SetWordTextChange(entityId, "hello")
82+
});
83+
84+
var json = JsonSerializer.Serialize(commit, options);
85+
// Inject an unknown change as if from a newer client.
86+
json = json.Replace(
87+
"\"ChangeEntities\":[",
88+
"""
89+
"ChangeEntities":[{"Index":1,"CommitId":"00000000-0000-0000-0000-000000000000","EntityId":"aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee","Change":{"$type":"SetWordPriorityChange","Priority":3}},
90+
""");
91+
92+
var roundTripped = JsonSerializer.Deserialize<Commit>(json, options)!;
93+
roundTripped.ChangeEntities.Should().HaveCount(2);
94+
roundTripped.ChangeEntities.Select(c => c.Change.GetType())
95+
.Should().BeEquivalentTo([typeof(OpaqueChange), typeof(SetWordTextChange)]);
96+
}
97+
98+
[Fact]
99+
public void Requires_type_as_first_property()
100+
{
101+
var options = SampleOptions();
102+
var json = """
103+
{"Text":"hello","EntityId":"aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee","$type":"SetWordTextChange"}
104+
""";
105+
106+
var act = () => JsonSerializer.Deserialize<IChange>(json, options);
107+
act.Should().Throw<JsonException>().WithMessage("*first property*");
108+
}
109+
}

‎src/SIL.Harmony/Changes/Change.cs‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,12 @@
33

44
namespace SIL.Harmony.Changes;
55

6-
[JsonPolymorphic(TypeDiscriminatorPropertyName = CrdtConstants.ChangeDiscriminatorProperty)]
6+
/// <summary>
7+
/// Polymorphic JSON for <see cref="IChange"/> is owned by
8+
/// <c>PeekThenConcreteChangeConverter</c> (via <see cref="CrdtConfig"/>), not
9+
/// <see cref="JsonPolymorphicAttribute"/>. Unknown <c>$type</c> values become
10+
/// <see cref="OpaqueChange"/>.
11+
/// </summary>
712
public interface IChange
813
{
914
[JsonIgnore]

‎src/SIL.Harmony/Changes/OpaqueChange.cs‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
using System.Text.Json;
2+
3+
namespace SIL.Harmony.Changes;
4+
5+
/// <summary>
6+
/// An <see cref="IChange"/> whose <c>$type</c> was not registered on this client.
7+
/// Preserves the original JSON so it can round-trip and be applied once the type is known.
8+
/// </summary>
9+
public sealed class OpaqueChange : IChange
10+
{
11+
public required string TypeName { get; init; }
12+
public required JsonElement RawJson { get; init; }
13+
14+
public Guid EntityId { get; set; }
15+
16+
public Type EntityType =>
17+
throw new NotSupportedException($"Opaque change '{TypeName}' has no known entity type.");
18+
19+
public ValueTask ApplyChange(IObjectBase entity, IChangeContext context) => default;
20+
21+
public ValueTask<IObjectBase> NewEntity(Commit commit, IChangeContext context) =>
22+
throw new NotSupportedException(
23+
$"Opaque change '{TypeName}' cannot create entities on this client. CommitId: {commit.Id}, EntityId: {EntityId}");
24+
25+
public bool SupportsApplyChange() => false;
26+
public bool SupportsNewEntity() => false;
27+
}

‎src/SIL.Harmony/Changes/PeekThenConcreteChangeConverter.cs‎

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
using System.Text;
2+
using System.Text.Json;
3+
using System.Text.Json.Serialization;
4+
using System.Text.Json.Serialization.Metadata;
5+
6+
namespace SIL.Harmony.Changes;
7+
8+
/// <summary>
9+
/// Owns <see cref="IChange"/> discrimination. Requires <c>$type</c> as the first JSON property
10+
/// (matching synthetic write order from <see cref="CrdtConfig"/>).
11+
/// Known discriminators deserialize via cached concrete <see cref="JsonTypeInfo"/>;
12+
/// unknown → <see cref="OpaqueChange"/> preserving the raw payload.
13+
/// </summary>
14+
internal sealed class PeekThenConcreteChangeConverter : JsonConverter<IChange>
15+
{
16+
private readonly KnownType[] _known;
17+
private readonly byte[] _discriminatorPropertyUtf8;
18+
19+
public PeekThenConcreteChangeConverter(IReadOnlyDictionary<string, Type> known)
20+
{
21+
_discriminatorPropertyUtf8 = Encoding.UTF8.GetBytes(CrdtConstants.ChangeDiscriminatorProperty);
22+
_known = known.Select(kv => new KnownType(Encoding.UTF8.GetBytes(kv.Key), kv.Value)).ToArray();
23+
}
24+
25+
public override IChange Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
26+
{
27+
if (reader.TokenType != JsonTokenType.StartObject)
28+
throw new JsonException("Expected StartObject");
29+
30+
// Checkpoint: restore for full-object deserialize / opaque capture after peeking $type.
31+
var checkpoint = reader;
32+
33+
if (!reader.Read() || reader.TokenType != JsonTokenType.PropertyName)
34+
throw new JsonException("Expected property name");
35+
36+
if (!reader.ValueTextEquals(_discriminatorPropertyUtf8))
37+
throw new JsonException(
38+
$"IChange requires \"{CrdtConstants.ChangeDiscriminatorProperty}\" as the first property");
39+
40+
if (!reader.Read() || reader.TokenType != JsonTokenType.String)
41+
throw new JsonException($"Expected string {CrdtConstants.ChangeDiscriminatorProperty} discriminator");
42+
43+
if (!TryFindKnown(ref reader, out var knownIndex, out var unknownTypeName))
44+
{
45+
reader = checkpoint;
46+
return ReadOpaque(ref reader, unknownTypeName!);
47+
}
48+
49+
ref var known = ref _known[knownIndex];
50+
var typeInfo = known.EnsureTypeInfo(options);
51+
52+
// Real change types use parameterized constructors / get-only props — let STJ materialize.
53+
reader = checkpoint;
54+
return (IChange)(JsonSerializer.Deserialize(ref reader, typeInfo)
55+
?? throw new JsonException($"null {known.ClrType.Name}"));
56+
}
57+
58+
public override void Write(Utf8JsonWriter writer, IChange value, JsonSerializerOptions options)
59+
{
60+
if (value is OpaqueChange opaque)
61+
{
62+
opaque.RawJson.WriteTo(writer);
63+
return;
64+
}
65+
66+
// Concrete runtime type: synthetic $type comes from JsonTypeInfo modifier.
67+
JsonSerializer.Serialize(writer, value, value.GetType(), options);
68+
}
69+
70+
private static OpaqueChange ReadOpaque(ref Utf8JsonReader reader, string typeName)
71+
{
72+
using var doc = JsonDocument.ParseValue(ref reader);
73+
var element = doc.RootElement.Clone();
74+
return new OpaqueChange
75+
{
76+
TypeName = typeName,
77+
EntityId = element.TryGetProperty(nameof(IChange.EntityId), out var id) && id.ValueKind == JsonValueKind.String
78+
? id.GetGuid()
79+
: default,
80+
RawJson = element
81+
};
82+
}
83+
84+
private bool TryFindKnown(ref Utf8JsonReader reader, out int index, out string? unknownTypeName)
85+
{
86+
for (var i = 0; i < _known.Length; i++)
87+
{
88+
if (reader.ValueTextEquals(_known[i].Utf8Discriminator))
89+
{
90+
index = i;
91+
unknownTypeName = null;
92+
return true;
93+
}
94+
}
95+
96+
index = -1;
97+
unknownTypeName = reader.GetString();
98+
return false;
99+
}
100+
101+
private struct KnownType
102+
{
103+
public KnownType(byte[] utf8Discriminator, Type clrType)
104+
{
105+
Utf8Discriminator = utf8Discriminator;
106+
ClrType = clrType;
107+
}
108+
109+
public byte[] Utf8Discriminator { get; }
110+
public Type ClrType { get; }
111+
private JsonTypeInfo? _typeInfo;
112+
113+
public JsonTypeInfo EnsureTypeInfo(JsonSerializerOptions options)
114+
{
115+
if (_typeInfo is not null && ReferenceEquals(_typeInfo.Options, options))
116+
return _typeInfo;
117+
118+
_typeInfo = options.GetTypeInfo(ClrType);
119+
return _typeInfo;
120+
}
121+
}
122+
}

‎src/SIL.Harmony/CrdtConfig.cs‎

Lines changed: 55 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -29,15 +29,49 @@ public class CrdtConfig
2929
public IEnumerable<Type> ObjectTypes => ObjectTypeListBuilder.AdapterProviders.SelectMany(p => p.GetRegistrations().Select(r => r.ObjectDbType));
3030
public JsonSerializerOptions JsonSerializerOptions => _lazyJsonSerializerOptions.Value;
3131
private readonly Lazy<JsonSerializerOptions> _lazyJsonSerializerOptions;
32+
private readonly Lazy<ChangeDiscriminatorMaps> _lazyChangeDiscriminatorMaps;
3233

3334
public CrdtConfig()
3435
{
35-
_lazyJsonSerializerOptions = new Lazy<JsonSerializerOptions>(() => new JsonSerializerOptions(JsonSerializerDefaults.General)
36+
_lazyChangeDiscriminatorMaps = new Lazy<ChangeDiscriminatorMaps>(BuildChangeDiscriminatorMaps);
37+
_lazyJsonSerializerOptions = new Lazy<JsonSerializerOptions>(CreateJsonSerializerOptions);
38+
}
39+
40+
private JsonSerializerOptions CreateJsonSerializerOptions()
41+
{
42+
var changeDiscriminators = _lazyChangeDiscriminatorMaps.Value;
43+
44+
var options = new JsonSerializerOptions(JsonSerializerDefaults.General)
3645
{
3746
TypeInfoResolver = MakeJsonTypeResolver()
38-
});
47+
};
48+
options.Converters.Add(new PeekThenConcreteChangeConverter(changeDiscriminators.ByDiscriminator));
49+
return options;
3950
}
4051

52+
private ChangeDiscriminatorMaps BuildChangeDiscriminatorMaps()
53+
{
54+
ChangeTypeListBuilder.Freeze();
55+
56+
var knownChanges = new Dictionary<string, Type>(ChangeTypeListBuilder.Types.Count);
57+
var discriminators = new Dictionary<Type, string>(ChangeTypeListBuilder.Types.Count);
58+
foreach (var derived in ChangeTypeListBuilder.Types)
59+
{
60+
if (derived.TypeDiscriminator is not string discriminator)
61+
throw new InvalidOperationException(
62+
$"Change type {derived.DerivedType} must use a string $type discriminator");
63+
64+
knownChanges.Add(discriminator, derived.DerivedType);
65+
discriminators.Add(derived.DerivedType, discriminator);
66+
}
67+
68+
return new ChangeDiscriminatorMaps(knownChanges, discriminators);
69+
}
70+
71+
private sealed record ChangeDiscriminatorMaps(
72+
IReadOnlyDictionary<string, Type> ByDiscriminator,
73+
IReadOnlyDictionary<Type, string> ByType);
74+
4175
public Action<JsonTypeInfo> MakeJsonTypeModifier()
4276
{
4377
return JsonTypeModifier;
@@ -55,12 +89,13 @@ private void JsonTypeModifier(JsonTypeInfo typeInfo)
5589
{
5690
ChangeTypeListBuilder.Freeze();
5791
ObjectTypeListBuilder.Freeze();
58-
if (typeInfo.Type == typeof(IChange))
92+
var changeTypeDiscriminators = _lazyChangeDiscriminatorMaps.Value.ByType;
93+
94+
// IChange polymorphism is owned by PeekThenConcreteChangeConverter — do not set PolymorphismOptions.
95+
if (typeInfo.Kind == JsonTypeInfoKind.Object
96+
&& changeTypeDiscriminators.TryGetValue(typeInfo.Type, out var discriminator))
5997
{
60-
foreach (var type in ChangeTypeListBuilder.Types)
61-
{
62-
typeInfo.PolymorphismOptions!.DerivedTypes.Add(type);
63-
}
98+
AddSyntheticTypeDiscriminator(typeInfo, discriminator);
6499
}
65100

66101
if (ObjectTypeListBuilder.JsonTypes?.TryGetValue(typeInfo.Type, out var types) == true)
@@ -73,6 +108,19 @@ private void JsonTypeModifier(JsonTypeInfo typeInfo)
73108
}
74109
}
75110

111+
/// <summary>
112+
/// Serialize-only <c>$type</c> on concrete change types so write stays a plain concrete serialize
113+
/// (converter Write does not inject the discriminator). Order forces <c>$type</c> first for the read path.
114+
/// </summary>
115+
private static void AddSyntheticTypeDiscriminator(JsonTypeInfo typeInfo, string discriminator)
116+
{
117+
var typeName = discriminator;
118+
var prop = typeInfo.CreateJsonPropertyInfo(typeof(string), CrdtConstants.ChangeDiscriminatorProperty);
119+
prop.Get = _ => typeName;
120+
prop.Order = int.MinValue;
121+
typeInfo.Properties.Add(prop);
122+
}
123+
76124
public bool RemoteResourcesEnabled { get; private set; }
77125
public Type? RemoteResourceMetadataType { get; private set; }
78126
public string LocalResourceCachePath { get; set; } = Path.GetFullPath("./localResourceCache");

‎src/SIL.Harmony/SnapshotWorker.cs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,12 @@ private async ValueTask ApplyCommitChanges(SortedSet<Commit> commits)
7474

7575
if (prevSnapshot is null)
7676
{
77+
if (commitChange.Change is OpaqueChange)
78+
{
79+
// Keep unknown changes in history until this client understands how to apply them.
80+
continue;
81+
}
82+
7783
// create brand new entity - this will (and should) throw if the change doesn't support NewEntity
7884
entity = await commitChange.Change.NewEntity(commit, changeContext);
7985
}

0 commit comments

Comments
 (0)