From 73b4b47451cffdfb410cdb14dcbcdcfa4562dc85 Mon Sep 17 00:00:00 2001 From: Meta Vens Date: Thu, 15 Oct 2026 00:26:52 +0200 Subject: [PATCH] CultElementIds: collection types join the id path, member attributes read through overrides, each unkeyed element type refused once Graph edges and lists carry the collection types between member and child; Settle adds them to the path so [MessagePackFormatter] on a collection class and a resolver claim on List are judged. Declares replaces the four MemberInfo.IsDefined member reads, which ignore inherit for properties. Survey.Judged dedupes the unkeyed-element refusal. Co-Authored-By: Claude Sonnet 5.5 --- .../Contracts/cultcache-persistence-format.md | 2 +- src/GameCult.Caching/CultElementIds.cs | 41 ++++--- .../ElementIdEngineTests.cs | 108 +++++++++++++++++- 3 files changed, 135 insertions(+), 16 deletions(-) diff --git a/src/GameCult.Caching/Contracts/cultcache-persistence-format.md b/src/GameCult.Caching/Contracts/cultcache-persistence-format.md index 4048eaf8..f1335e8f 100644 --- a/src/GameCult.Caching/Contracts/cultcache-persistence-format.md +++ b/src/GameCult.Caching/Contracts/cultcache-persistence-format.md @@ -434,7 +434,7 @@ elements retargets nothing. keyed member of the element. A list whose element type is not keyed (`[MessagePackObject]` or `[Union]`) is a value list and is replaced whole. For an object list, C# refuses at registration a concrete element type that is not keyed and - `[MessagePackFormatter]` on any type or member on the way to the list or on the id + `[MessagePackFormatter]` on any type, collection type or member on the way to the list or on the id member, and refuses at the first encode or decode of the document a formatter resolver that claims one of those types. diff --git a/src/GameCult.Caching/CultElementIds.cs b/src/GameCult.Caching/CultElementIds.cs index 000bed40..de457f8a 100644 --- a/src/GameCult.Caching/CultElementIds.cs +++ b/src/GameCult.Caching/CultElementIds.cs @@ -100,14 +100,17 @@ namespace GameCult.Caching // element types and every concrete type under them. A serialization codec must write all of these by their [Key] // layout, so it checks them against the formatters it composed. public List IdPath { get; } = new(); + + // The concrete element types already judged in this document, so each is refused at most once however many lists hold it. + internal HashSet Judged { get; } = new(); } // What the walk saw, settled once it is complete: a type reaches a list only through types reached later, and a type // can sit in a cycle, so the path is a fixed point over these edges and not a stack. private sealed class Graph { - public List<(Type Owner, MemberInfo? Member, Type Child)> Edges { get; } = new(); - public List<(Type Owner, MemberInfo Member, Type Element, List Concrete)> Lists { get; } = new(); + public List<(Type Owner, MemberInfo? Member, Type Child, Type[] Through)> Edges { get; } = new(); + public List<(Type Owner, MemberInfo Member, Type Element, List Concrete, Type[] Through)> Lists { get; } = new(); } private static readonly ConcurrentDictionary IdCapable = new(); @@ -130,22 +133,22 @@ namespace GameCult.Caching if (!seen.Add(type)) return; foreach (var (member, memberType) in KeyedMembers(type)) - VisitMemberType(documentType, type, member, memberType, seen, survey, graph); + VisitMemberType(documentType, type, member, memberType, Array.Empty(), seen, survey, graph); foreach (var union in type.GetCustomAttributes(true)) { - graph.Edges.Add((type, null, union.SubType)); + graph.Edges.Add((type, null, union.SubType, Array.Empty())); VisitObject(documentType, union.SubType, seen, survey, graph); } } - private static void VisitMemberType(Type documentType, Type owner, MemberInfo member, Type memberType, HashSet seen, Survey survey, Graph graph) + private static void VisitMemberType(Type documentType, Type owner, MemberInfo member, Type memberType, Type[] through, HashSet seen, Survey survey, Graph graph) { memberType = Nullable.GetUnderlyingType(memberType) ?? memberType; if (IsLeaf(memberType)) return; if (DictionaryValueType(memberType) is { } valueType) { - VisitMemberType(documentType, owner, member, valueType, seen, survey, graph); + VisitMemberType(documentType, owner, member, valueType, through.Append(memberType).ToArray(), seen, survey, graph); return; } @@ -154,9 +157,11 @@ namespace GameCult.Caching if (IsObjectType(element)) { var concretes = ConcreteTypes(element).ToList(); - graph.Lists.Add((owner, member, element, concretes)); + graph.Lists.Add((owner, member, element, concretes, through.Append(memberType).ToArray())); foreach (var concrete in concretes) { + if (!survey.Judged.Add(concrete)) + continue; var problem = IsObjectType(concrete) ? IdProblem(concrete) : UnkeyedElementProblem; if (problem != null) survey.Problems.Add($"Cult document {documentType.Name}: list element type {concrete.Name} (in {owner.Name}.{member.Name}) {problem}"); @@ -165,13 +170,13 @@ namespace GameCult.Caching VisitObject(documentType, element, seen, survey, graph); } else - VisitMemberType(documentType, owner, member, element, seen, survey, graph); + VisitMemberType(documentType, owner, member, element, through.Append(memberType).ToArray(), seen, survey, graph); return; } if (IsObjectType(memberType)) { - graph.Edges.Add((owner, member, memberType)); + graph.Edges.Add((owner, member, memberType, through)); VisitObject(documentType, memberType, seen, survey, graph); } } @@ -186,15 +191,17 @@ namespace GameCult.Caching { var reaching = new HashSet(graph.Lists.Select(list => list.Owner)); var members = new List(graph.Lists.Select(list => list.Member)); + var through = new List(graph.Lists.SelectMany(list => list.Through)); for (var grew = true; grew;) { grew = false; - foreach (var (owner, member, child) in graph.Edges) + foreach (var (owner, member, child, via) in graph.Edges) { if (!reaching.Contains(child)) continue; if (member != null && !members.Contains(member)) members.Add(member); + through.AddRange(via); grew |= reaching.Add(owner); } } @@ -208,6 +215,8 @@ namespace GameCult.Caching foreach (var type in reaching) Add(type); + foreach (var type in through) + Add(type); foreach (var list in graph.Lists) { Add(list.Element); @@ -224,7 +233,7 @@ namespace GameCult.Caching foreach (var member in members) { - if (member.IsDefined(typeof(MessagePackFormatterAttribute), true)) + if (Declares(member, typeof(MessagePackFormatterAttribute))) survey.Problems.Add($"Cult document {documentType.Name}: {member.DeclaringType!.Name}.{member.Name} leads to a list whose elements carry ids and declares [MessagePackFormatter], so its element ids would never reach disk; remove the attribute."); } } @@ -252,13 +261,13 @@ namespace GameCult.Caching private static string? IdProblem(Type element) { - var ids = MembersOf(element).Where(member => member.IsDefined(typeof(CultElementIdAttribute), true)).ToArray(); + var ids = MembersOf(element).Where(member => Declares(member, typeof(CultElementIdAttribute))).ToArray(); if (ids.Length == 0) return "has no [CultElementId] member; add a public string member with [Key] and [CultElementId] so each element has an identity."; if (ids.Length > 1) return $"has two [CultElementId] members ({ids[0].Name}, {ids[1].Name}); an element has one identity."; var id = ids[0]; - if (id.IsDefined(typeof(MessagePackFormatterAttribute), true)) + if (Declares(id, typeof(MessagePackFormatterAttribute))) return $"declares [MessagePackFormatter] on its [CultElementId] member {id.Name}, so the id would not be written by its [Key]."; if (TypeOf(id) != typeof(string)) return $"declares [CultElementId] on {id.Name}, which is not a string."; @@ -475,7 +484,7 @@ namespace GameCult.Caching private static Shape Build(Type type) { var shape = new Shape(); - var idMember = MembersOf(type).FirstOrDefault(member => member.IsDefined(typeof(CultElementIdAttribute), true)); + var idMember = MembersOf(type).FirstOrDefault(member => Declares(member, typeof(CultElementIdAttribute))); if (idMember != null) { shape.GetId = idMember is FieldInfo field ? o => (string?)field.GetValue(o) : o => (string?)((PropertyInfo)idMember).GetValue(o); @@ -504,6 +513,10 @@ namespace GameCult.Caching private static IEnumerable<(MemberInfo Member, Type Type)> KeyedMembers(Type type) => MembersOf(type).Where(member => KeyOf(member) != null).Select(member => (member, TypeOf(member))); + // An attribute a member declares, or inherits through an override: MemberInfo.IsDefined ignores inherit for properties, so an + // override of a base virtual member would not see its base's attribute. + private static bool Declares(MemberInfo member, Type attribute) => Attribute.IsDefined(member, attribute, inherit: true); + private static int? KeyOf(MemberInfo member) => member.GetCustomAttribute(true)?.IntKey; private static Type TypeOf(MemberInfo member) => member is FieldInfo field ? field.FieldType : ((PropertyInfo)member).PropertyType; diff --git a/tests/GameCult.Caching.Tests/ElementIdEngineTests.cs b/tests/GameCult.Caching.Tests/ElementIdEngineTests.cs index dd7be0b5..551faada 100644 --- a/tests/GameCult.Caching.Tests/ElementIdEngineTests.cs +++ b/tests/GameCult.Caching.Tests/ElementIdEngineTests.cs @@ -313,6 +313,52 @@ namespace GameCult.Caching.Tests Assert.That(Problems(typeof(HFormattedDerived)).Single(), Does.Contain("FormattedDerived").And.Contain("[MessagePackFormatter]"), "the attribute is inherited"); } + // The collection types between a member and its object list are written by some formatter too, so they sit on the id path. + // A value list (of a struct, a primitive or a non-keyed class) never reaches an object, so it does not. + [Test] + public void ACollectionTypeOnTheWayToAnObjectListIsOnTheIdPath() + { + var path = CultElementIds.Inspect(typeof(HCollections)).IdPath; + Assert.That(path, Is.EquivalentTo(new[] + { + typeof(HCollections), typeof(EIded), typeof(HHasIds), + typeof(List), typeof(EIded[]), typeof(Dictionary), typeof(List>) + })); + + var valueLists = CultElementIds.Inspect(typeof(HValueListsBesideObjects)).IdPath; + Assert.That(valueLists, Is.EquivalentTo(new[] { typeof(HValueListsBesideObjects), typeof(List), typeof(EIded) })); + } + + [Test] + public void AFormatterOnACollectionClassOfAnObjectListIsRefused() + { + Assert.That(Problems(typeof(HRoles)).Single(), Does.Contain("Roles").And.Contain("[MessagePackFormatter]")); + Assert.That(Problems(typeof(HFormattedValueCollection)), Is.Empty, "a collection class of a non-keyed element is a value list"); + } + + // MemberInfo.IsDefined ignores inherit for properties: an override sees its base's attribute only through Declares. + [Test] + public void AnInheritedFormatterOnAnOverriddenMemberIsRefused() + { + Assert.That(Problems(typeof(HOverFormattedId)).Single(), Does.Contain("Id").And.Contain("[MessagePackFormatter]")); + Assert.That(Problems(typeof(OwnerOverFormatted)).Single(), Does.Contain("Items").And.Contain("[MessagePackFormatter]")); + + Assert.That(Problems(typeof(HIdOverride)), Is.Empty, "a base [CultElementId] is the override's id"); + var root = new HIdOverride { Items = { new IdOverride() } }; + Assert.That(Assign(root, "r", deterministic: false), Is.EqualTo(1)); + Assert.That(root.Items[0].Id, Has.Length.EqualTo(12)); + } + + // The union arm has a valid id, so IdProblem passes it and only the rule that an element is written by its [Key] layout + // can refuse it; it is refused once per document however many lists hold it. + [Test] + public void ANonKeyedArmWithAValidIdIsRefusedOncePerType() + { + Assert.That(Problems(typeof(HUnkeyedArmList)).Single(), Does.Contain("UnkeyedArm").And.Contain("formatter the cache does not own")); + Assert.That(Problems(typeof(HUnkeyedTwoLists)).Single(), Does.Contain("UnkeyedArm")); + Assert.That(Problems(typeof(HUnkeyedArmField)), Is.Empty); + } + // The id path is a fixed point, not the stack at the moment a list is found: a type in a cycle with the list's owner, and // a type reached twice, are both on the way to it. [Test] @@ -336,11 +382,71 @@ namespace GameCult.Caching.Tests [MessagePackObject] public class EBareNest { [Key(0)] [CultElementId] public string Id { get; set; } = ""; [Key(1)] public List Sub { get; set; } = new(); } public sealed class NoFormatter { } - public class UnkeyedArm : ArmBase { [Key(0)] public string Id { get; set; } = ""; } + public class UnkeyedArm : ArmBase { [Key(0)] [CultElementId] public string Id { get; set; } = ""; } [Union(0, typeof(KeyedArm)), Union(1, typeof(UnkeyedArm))] public abstract class ArmBase { } [MessagePackObject] public class KeyedArm : ArmBase { [Key(0)] [CultElementId] public string Id { get; set; } = ""; } [MessagePackObject] public class HUnkeyedArmList { [Key(0)] public List Items { get; set; } = new(); } [MessagePackObject] public class HUnkeyedArmField { [Key(0)] public ArmBase? Root { get; set; } } + [MessagePackObject] + public class HCollections + { + [Key(0)] public List A { get; set; } = new(); + [Key(1)] public EIded[] B { get; set; } = Array.Empty(); + [Key(2)] public Dictionary C { get; set; } = new(); + [Key(3)] public List> D { get; set; } = new(); + } + + [MessagePackObject] + public class HValueListsBesideObjects + { + [Key(0)] public List Objects { get; set; } = new(); + [Key(1)] public List Numbers { get; set; } = new(); + [Key(2)] public List Plain { get; set; } = new(); + [Key(3)] public List Points { get; set; } = new(); + } + + [MessagePackFormatter(typeof(NoFormatter))] public class Roles : List { } + [MessagePackObject] public class HRoles { [Key(0)] public Roles Items { get; set; } = new(); } + [MessagePackFormatter(typeof(NoFormatter))] public class PlainValues : List { } + [MessagePackObject] public class HFormattedValueCollection { [Key(0)] public List Objects { get; set; } = new(); [Key(1)] public PlainValues Plain { get; set; } = new(); } + + [MessagePackObject] + public abstract class FormattedIdBase + { + [Key(0)] [MessagePackFormatter(typeof(NoFormatter))] public virtual string Id { get; set; } = ""; + } + + [MessagePackObject] + public class FormattedIdOverride : FormattedIdBase + { + [Key(0)] [CultElementId] public override string Id { get; set; } = ""; + } + + [MessagePackObject] public class HOverFormattedId { [Key(0)] public List Items { get; set; } = new(); } + + [MessagePackObject] + public class OwnerBaseFormatted + { + [Key(0)] [MessagePackFormatter(typeof(NoFormatter))] public virtual List Items { get; set; } = new(); + } + + [MessagePackObject] + public class OwnerOverFormatted : OwnerBaseFormatted + { + [Key(0)] public override List Items { get; set; } = new(); + } + + [MessagePackObject] + public class IdVirtual + { + [Key(0)] public string X { get; set; } = ""; + [Key(1)] [CultElementId] public virtual string Id { get; set; } = ""; + } + + [MessagePackObject] public class IdOverride : IdVirtual { [Key(1)] public override string Id { get; set; } = ""; } + [MessagePackObject] public class HIdOverride { [Key(0)] public List Items { get; set; } = new(); } + + [MessagePackObject] public class HUnkeyedTwoLists { [Key(0)] public List A { get; set; } = new(); [Key(1)] public List B { get; set; } = new(); } public class PlainValue { public string X { get; set; } = ""; } [MessagePackObject] public class HValueList { [Key(0)] public List Items { get; set; } = new(); }