diff --git a/src/GameCult.Caching/CultElementIds.cs b/src/GameCult.Caching/CultElementIds.cs index 77ba8501..34541f34 100644 --- a/src/GameCult.Caching/CultElementIds.cs +++ b/src/GameCult.Caching/CultElementIds.cs @@ -136,7 +136,7 @@ namespace GameCult.Caching return; foreach (var (member, memberType) in KeyedMembers(type)) VisitMemberType(documentType, type, member, memberType, Array.Empty(), false, seen, survey, graph); - foreach (var (member, memberType) in NonPublicKeyedMembers(type)) + foreach (var (member, memberType) in UnreadKeyedMembers(type)) VisitMemberType(documentType, type, member, memberType, Array.Empty(), true, seen, survey, graph); foreach (var union in type.GetCustomAttributes(true)) { @@ -276,7 +276,10 @@ namespace GameCult.Caching private static string? IdProblem(Type element) { - var ids = MembersOf(element).Where(member => Declares(member, typeof(CultElementIdAttribute))).ToArray(); + // Candidates come from every instance member up the base chain, public or not, so an id MessagePack does not write + // is named as such instead of being reported missing. An override or a hiding member is one member, the most derived. + var ids = AllInstanceMembers(element).Where(member => Declares(member, typeof(CultElementIdAttribute))) + .GroupBy(member => member.Name).Select(group => group.First()).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) @@ -288,11 +291,16 @@ namespace GameCult.Caching return $"declares [CultElementId] on {id.Name}, which is not a string."; if (KeyOf(id) is not { } idKey) return $"declares [CultElementId] on {id.Name}, which has no integer [Key], so it is never persisted."; + if (MembersOf(element).FirstOrDefault(member => member.Name == id.Name && member.MemberType == id.MemberType) is not { } written) + return $"declares [CultElementId] on {id.Name}, which MessagePack does not write ([IgnoreMember], non-public, or a property whose getter is not public); the id would never reach disk."; + id = written; var sharing = KeyedMembers(element).FirstOrDefault(entry => entry.Member != id && KeyOf(entry.Member) == idKey); if (sharing.Member != null) return $"declares [CultElementId] on {id.Name} with [Key({idKey})], which {sharing.Member.Name} also uses; an id needs a key of its own."; if (id is PropertyInfo { SetMethod: null } or FieldInfo { IsInitOnly: true }) return $"declares [CultElementId] on {id.Name}, which cannot be assigned; the cache mints into it."; + if (id is PropertyInfo { SetMethod.IsPublic: false }) + return $"declares [CultElementId] on {id.Name}, whose setter is not public: MessagePack writes the id but does not read it back, so every load would mint a different one. Make the setter public (init is fine)."; var source = id.GetCustomAttribute(true)!.DerivedFrom; if (source != null) { @@ -533,9 +541,33 @@ namespace GameCult.Caching return shape; } + // The members MessagePack's StandardResolver writes for an integer-keyed type: public instance fields, and public + // instance properties with a public getter, none of them [IgnoreMember]. TheWalkReadsTheMembersMessagePackWrites checks + // this set against MessagePack's own member discovery. private static IEnumerable MembersOf(Type type) => type.GetFields(BindingFlags.Instance | BindingFlags.Public).Cast() - .Concat(type.GetProperties(BindingFlags.Instance | BindingFlags.Public).Where(property => property.GetIndexParameters().Length == 0 && property.GetMethod != null)); + .Concat(type.GetProperties(BindingFlags.Instance | BindingFlags.Public) + .Where(property => property.GetIndexParameters().Length == 0 && property.GetMethod is { IsPublic: true })) + .Where(member => !Declares(member, typeof(IgnoreMemberAttribute))); + + // Every instance field and indexer-free property declared on the type or any base, most derived first. + private static IEnumerable AllInstanceMembers(Type type) + { + const BindingFlags flags = BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.DeclaredOnly; + for (var level = type; level != null && level != typeof(object); level = level.BaseType) + { + foreach (var field in level.GetFields(flags)) + yield return field; + foreach (var property in level.GetProperties(flags).Where(property => property.GetIndexParameters().Length == 0)) + yield return property; + } + } + + // The members the walk reads, as MessagePack writes them, for the oracle test that checks both against MessagePack. + internal static IEnumerable WrittenKeyedMembers(Type type) => KeyedMembers(type).Select(entry => entry.Member); + + // The [Key] members MessagePack writes only under AllowPrivate, for the same test. + internal static IEnumerable UnreadMembers(Type type) => UnreadKeyedMembers(type).Select(entry => entry.Member); private static IEnumerable<(MemberInfo Member, Type Type)> KeyedMembers(Type type) => MembersOf(type).Where(member => KeyOf(member) != null).Select(member => (member, TypeOf(member))); @@ -544,16 +576,18 @@ namespace GameCult.Caching // 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); - // [Key] members MessagePack writes only under AllowPrivate: non-public fields and properties, up the base chain. - private static IEnumerable<(MemberInfo Member, Type Type)> NonPublicKeyedMembers(Type type) + // [Key] members MessagePack writes only under AllowPrivate: non-public fields and properties, and public properties whose + // getter is not public, up the base chain. An [IgnoreMember] member is never written, so it is not one of them. + private static IEnumerable<(MemberInfo Member, Type Type)> UnreadKeyedMembers(Type type) { for (var level = type; level != null && level != typeof(object); level = level.BaseType) { - const BindingFlags flags = BindingFlags.Instance | BindingFlags.NonPublic | BindingFlags.DeclaredOnly; - foreach (var member in level.GetFields(flags).Cast() - .Concat(level.GetProperties(flags).Where(property => property.GetIndexParameters().Length == 0))) + const BindingFlags flags = BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.DeclaredOnly; + foreach (var member in level.GetFields(flags).Where(field => !field.IsPublic).Cast() + .Concat(level.GetProperties(flags).Where(property => + property.GetIndexParameters().Length == 0 && property.GetMethod is { IsPublic: false }))) { - if (KeyOf(member) != null) + if (KeyOf(member) != null && !Declares(member, typeof(IgnoreMemberAttribute))) yield return (member, TypeOf(member)); } } diff --git a/tests/GameCult.Caching.Tests/ElementIdEngineTests.cs b/tests/GameCult.Caching.Tests/ElementIdEngineTests.cs index 1a4417b8..3f6b084e 100644 --- a/tests/GameCult.Caching.Tests/ElementIdEngineTests.cs +++ b/tests/GameCult.Caching.Tests/ElementIdEngineTests.cs @@ -511,7 +511,165 @@ namespace GameCult.Caching.Tests Assert.That(CultElementIds.Holds(typeof(KBagIded), bag), Is.True); } + // The walk reads the members MessagePack writes: a public field or a property with a public getter, no [IgnoreMember]. + // Anything else that carries a [Key] is MessagePack's AllowPrivate business and, on the way to an object list, is refused. + [Test] + public void TheWalkReadsTheMembersMessagePackWrites() + { + Assert.That(Problems(typeof(PubIgn)), Is.Empty, "an ignored member is never written, so its list is nobody's business"); + Assert.That(Problems(typeof(ApIgn)), Is.Empty, "an ignored private member is not a hidden list"); + var held = Problems(typeof(PgHold)).Single(); + Assert.That(held, Does.Contain("Items").And.Contain("non-public")); + + foreach (var type in new[] { typeof(EIded), typeof(HHiddenLeaf), typeof(PubIgn), typeof(ApIgn), typeof(PgHold), typeof(MemberMix) }) + { + var written = MessagePackMembers(type, allowPrivate: false); + Assert.That(CultElementIds.WrittenKeyedMembers(type).Select(member => member.Name), Is.EquivalentTo(written), type.Name); + var readable = MessagePackMembers(type, allowPrivate: true); + Assert.That(CultElementIds.UnreadMembers(type).Select(member => member.Name), Is.EquivalentTo(readable.Except(written)), type.Name); + } + + Assert.That(CultElementIds.UnreadMembers(typeof(MemberMix)).Select(member => member.Name), Is.EquivalentTo(new[] { "priv" })); + Assert.That(CultElementIds.UnreadMembers(typeof(PgHold)).Select(member => member.Name), Is.EquivalentTo(new[] { "Items" })); + } + + // The names of the members MessagePack's own discovery reads for a type, by reflection into its internal type: the + // oracle the walk's member set is pinned to. + private static List MessagePackMembers(Type type, bool allowPrivate) + { + const string moved = "MessagePack moved its member discovery; re-derive MembersOf"; + const System.Reflection.BindingFlags all = System.Reflection.BindingFlags.Static | System.Reflection.BindingFlags.Instance | + System.Reflection.BindingFlags.Public | System.Reflection.BindingFlags.NonPublic; + var info = typeof(MessagePackSerializer).Assembly.GetType("MessagePack.Internal.ObjectSerializationInfo"); + var create = info?.GetMethod("CreateOrNull", all); + if (create == null || create.GetParameters().Length != 4) + { + Assert.Fail(moved); + return new List(); + } + + var result = create.Invoke(null, new object[] { type, false, false, allowPrivate }); + var members = result?.GetType().GetProperty("Members", all)?.GetValue(result) as System.Collections.IEnumerable; + if (members == null) + { + Assert.Fail(moved); + return new List(); + } + + var names = new List(); + foreach (var member in members) + { + var readable = member.GetType().GetProperty("IsReadable", all)?.GetValue(member); + var name = member.GetType().GetProperty("Name", all)?.GetValue(member) as string; + if (readable is not bool || name == null) + Assert.Fail(moved); + if ((bool)readable!) + names.Add(name!); + } + + return names; + } + + // An id MessagePack writes but cannot read back would be re-minted differently on every load; one it does not write + // never reaches disk. + [Test] + public void AnIdMessagePackCannotReadBackIsRefused() + { + Assert.That(Problems(typeof(HPrivSet)).Single(), Does.Contain("setter is not public").And.Contain("Id")); + Assert.That(Problems(typeof(HIgnId)).Single(), Does.Contain("does not write").And.Contain("Id")); + Assert.That(Problems(typeof(HPrivateId)).Single(), Does.Contain("does not write").And.Contain("id")); + Assert.That(Problems(typeof(HInit)), Is.Empty); + Assert.That(Problems(typeof(HCtorId)), Is.Empty); + + foreach (var holder in new object[] + { + new HInit { Items = { new EInit(), new EInit() } }, + new HCtorId { Items = { new ECtorId(null), new ECtorId(null) } }, + }) + { + Assert.That(Assign(holder, "r", deterministic: false), Is.EqualTo(2), holder.GetType().Name); + var bytes = MessagePackSerializer.Serialize(holder.GetType(), holder, GameCult.Caching.MessagePack.CultDocumentMessagePackSerialization.Options); + var back = MessagePackSerializer.Deserialize(holder.GetType(), bytes, GameCult.Caching.MessagePack.CultDocumentMessagePackSerialization.Options); + Assert.That(Ids(back), Is.EqualTo(Ids(holder)), holder.GetType().Name); + Assert.That(Ids(back), Has.All.Match("^[0-9a-f]{12}$"), holder.GetType().Name); + } + } + + private static string?[] Ids(object holder) => holder switch + { + HInit init => init.Items.Select(item => item.Id).ToArray(), + HCtorId ctor => ctor.Items.Select(item => item.Id).ToArray(), + _ => throw new ArgumentException(holder.GetType().Name), + }; + + // A subclass that redeclares the id member does not change which member the declared element writes. + [Test] + public void AnElementIsReadByItsDeclaredTypeNotItsRuntimeType() + { + Assert.That(Problems(typeof(HDoc2)), Is.Empty); + static HE Hd(string? declared = null, string? redeclared = null) + { + var item = new HD { Id = redeclared }; + ((HE)item).Id = declared; + return item; + } + + var minted = new HDoc2 { Items = { Hd() } }; + Assert.That(Assign(minted, "r", deterministic: false), Is.EqualTo(1)); + Assert.That(minted.Items[0].Id, Does.Match("^[0-9a-f]{12}$")); + Assert.That(((HD)minted.Items[0]).Id, Is.Null, "the redeclared member is never written, so it is not minted"); + + Assert.That(() => CultElementIds.Plan(typeof(HDoc2), new HDoc2 { Items = { Hd(declared: "not-hex") } }, "r", false), + Throws.TypeOf(), "the declared member is checked"); + Assert.That(() => CultElementIds.Plan(typeof(HDoc2), new HDoc2 { Items = { Hd(redeclared: "not-hex") } }, "r", false), + Throws.Nothing, "the redeclared member is never read"); + + Assert.That(CultElementIds.Holds(typeof(HDoc2), new HDoc2 { Items = { Hd(declared: "0123456789ab") } }), Is.True); + Assert.That(CultElementIds.Holds(typeof(HDoc2), new HDoc2 { Items = { Hd(redeclared: "0123456789ab") } }), Is.False); + } + // ---- shapes ---- + [MessagePackObject] public class PubIgn { [Key(0)] [IgnoreMember] public List L { get; set; } = new(); } +#pragma warning disable CS0169, CS0414, CS0649 + [MessagePackObject] public class ApIgn { [Key(0)] [IgnoreMember] private List l = new(); } +#pragma warning restore CS0169, CS0414, CS0649 + [MessagePackObject] public class PgHold { [Key(0)] public List Items { private get; set; } = new(); } + [MessagePackObject] + public class MemberMix + { + [Key(0)] public int A; + [Key(1)] [IgnoreMember] public List? Ign; + [Key(2)] public string? PS { get; private set; } + [Key(3)] public string? GetOnly { get; } +#pragma warning disable CS0169, CS0414, CS0649 + [Key(4)] private int priv; +#pragma warning restore CS0169, CS0414, CS0649 + [Key(5)] public string? InitOnly { get; init; } + [IgnoreMember] public int Priv => priv; + } + + [MessagePackObject] public class EPrivSet { [Key(0)] [CultElementId] public string? Id { get; private set; } } + [MessagePackObject] public class HPrivSet { [Key(0)] public List Items { get; set; } = new(); } + [MessagePackObject] public class EIgnId { [Key(0)] [IgnoreMember] [CultElementId] public string? Id { get; set; } } + [MessagePackObject] public class HIgnId { [Key(0)] public List Items { get; set; } = new(); } + #pragma warning disable CS0169, CS0649 + [MessagePackObject] public class EPrivateId { [Key(0)] [CultElementId] private string? id; } +#pragma warning restore CS0169, CS0649 + [MessagePackObject] public class HPrivateId { [Key(0)] public List Items { get; set; } = new(); } + [MessagePackObject] public class EInit { [Key(0)] [CultElementId] public string? Id { get; init; } } + [MessagePackObject] public class HInit { [Key(0)] public List Items { get; set; } = new(); } + [MessagePackObject] + public class ECtorId + { + [SerializationConstructor] public ECtorId(string? id) { Id = id; } + [Key(0)] [CultElementId] public string? Id { get; set; } + } + [MessagePackObject] public class HCtorId { [Key(0)] public List Items { get; set; } = new(); } + + [MessagePackObject] public class HE { [Key(0)] [CultElementId] public string? Id { get; set; } } + [MessagePackObject] public class HD : HE { [Key(2)] [CultElementId] public new string? Id { get; set; } } + [MessagePackObject] public class HDoc2 { [Key(0)] public List Items { get; set; } = new(); } + [MessagePackObject] public class KNode : IEnumerable {