diff --git a/src/GameCult.Caching/Contracts/cultcache-schema-compatibility.md b/src/GameCult.Caching/Contracts/cultcache-schema-compatibility.md index b3cd9f43..9669044c 100644 --- a/src/GameCult.Caching/Contracts/cultcache-schema-compatibility.md +++ b/src/GameCult.Caching/Contracts/cultcache-schema-compatibility.md @@ -33,15 +33,24 @@ A member's slot is its MessagePack `[Key(n)]` integer. MessagePack's `[Key]` and `MessagePack.Annotations`. Payloads are written by `MessagePackSerializer`, so the registry refuses a document type without `[MessagePackObject]` and a non-public document type without `[MessagePackObject(AllowPrivate = true)]`. -`AllowPrivate` makes MessagePack read non-public members too, so every -non-public instance field or property of such a type must be `[IgnoreMember]`; -the registry persists public members only. A -member's type name is the CLR full name; a nested type is written `Outer+Inner`. +A member's type name is the CLR full name; a nested type is written `Outer+Inner`. The registry also refuses a persisted member without `[Key]`, a string `[Key]`, two members sharing a slot (including `new`-hidden members), a hidden persisted member, an override whose `[Key]` or `[IgnoreMember]` differs from its -base declaration's (MessagePack reads the base declaration), and a persisted -member without a public or internal setter. +base declaration's (MessagePack reads the base declaration), and a readonly +persisted field. + +Visibility follows MessagePack's rule: the registry accepts what MessagePack +round-trips and refuses what it would silently lose. The registry persists +public fields and properties only. Without `AllowPrivate`, a persisted property +needs a public setter, and a `[Key]` on a non-public member is refused because +MessagePack skips that member. With `AllowPrivate`, private, internal and +init-only setters are all writable. But MessagePack then also reads non-public +members, so every non-public instance field or property must be +`[IgnoreMember]`. A class also needs a constructor MessagePack can call. That +is a parameterless one (public unless `AllowPrivate`), or one whose parameters +take the members at `[Key(0)]`, `[Key(1)]`, ... in order. A primary constructor +usually has neither. ## Which schema a record carries diff --git a/src/GameCult.Caching/CultCache.cs b/src/GameCult.Caching/CultCache.cs index 8f0a5c80..47440394 100644 --- a/src/GameCult.Caching/CultCache.cs +++ b/src/GameCult.Caching/CultCache.cs @@ -838,7 +838,11 @@ namespace GameCult.Caching private static IReadOnlyList DiscoverMembers(Type type) { const BindingFlags declared = BindingFlags.DeclaredOnly | BindingFlags.Instance | BindingFlags.Public; + var messagePackObject = type.GetCustomAttribute(true); + var allowPrivate = messagePackObject?.AllowPrivate == true; var candidates = new List<(MemberInfo Member, Type MemberType, int Depth, bool Writable, PropertyInfo? DivergentRoot)>(); + // Keyed get-only properties: MessagePack writes them and fills them through a constructor parameter only. + var constructorOnly = new List<(Type MemberType, int Slot)>(); var seenRoots = new HashSet<(Type, string)>(); var depth = 0; for (var current = type; current != null && current != typeof(object); current = current.BaseType, depth++) @@ -855,8 +859,14 @@ namespace GameCult.Caching continue; var chain = OverrideChain(property).ToArray(); var setter = chain.Select(declaration => declaration.SetMethod).FirstOrDefault(method => method != null); - if (chain.All(declaration => declaration.GetMethod == null) || setter == null) + if (chain.All(declaration => declaration.GetMethod == null)) continue; + if (setter == null) + { + if (!IsIgnored(property) && GetKeyValue(property) is int constructorSlot) + constructorOnly.Add((property.PropertyType, constructorSlot)); + continue; + } // MessagePack's resolver reads the base declaration's [Key]/[IgnoreMember]; an override that changes them is refused. var root = chain[chain.Length - 1]; var divergentRoot = chain.Any(declaration => @@ -865,30 +875,33 @@ namespace GameCult.Caching : null; if (divergentRoot == null && IsIgnored(property)) continue; - candidates.Add((property, property.PropertyType, depth, IsWritable(setter), divergentRoot)); + // MessagePack's own rule: it calls a setter that is public, or any setter (init-only included) under AllowPrivate. + candidates.Add((property, property.PropertyType, depth, setter.IsPublic || allowPrivate, divergentRoot)); } } var rejections = new List(); - var messagePackObject = type.GetCustomAttribute(true); if (messagePackObject == null) rejections.Add(NotMessagePackObjectMessage(type.Name)); - else if (!type.IsVisible && !messagePackObject.AllowPrivate) + else if (!type.IsVisible && !allowPrivate) rejections.Add(NotVisibleMessage(type.Name)); - else if (messagePackObject.AllowPrivate) + else { - // AllowPrivate makes MessagePack read non-public members; the registry persists public members only. + // AllowPrivate makes MessagePack read non-public members, which the registry does not persist; without it MessagePack + // silently skips a keyed non-public member. const BindingFlags nonPublic = BindingFlags.DeclaredOnly | BindingFlags.Instance | BindingFlags.NonPublic; for (var current = type; current != null && current != typeof(object); current = current.BaseType) { foreach (var member in current.GetFields(nonPublic).Cast().Concat(current.GetProperties(nonPublic))) { - if (!member.IsDefined(typeof(CompilerGeneratedAttribute)) && !IsIgnored(member)) + if (allowPrivate && !member.IsDefined(typeof(CompilerGeneratedAttribute)) && !IsIgnored(member)) rejections.Add(UnmarkedNonPublicMemberMessage(type.Name, Qualified(member))); + else if (!allowPrivate && GetKeyValue(member) != null && !IsIgnored(member)) + rejections.Add(KeyedNonPublicMemberMessage(type.Name, Qualified(member))); } } } - var keyed =new List<(MemberInfo Member, Type MemberType, int Slot)>(); + var keyed = new List<(MemberInfo Member, Type MemberType, int Slot)>(); foreach (var candidate in candidates.OrderBy(candidate => candidate.Depth).ThenBy(candidate => candidate.Member.Name, StringComparer.Ordinal)) { var key = GetKeyValue(candidate.Member); @@ -901,11 +914,16 @@ namespace GameCult.Caching else { if (!candidate.Writable) - rejections.Add(NotWritableMessage(type.Name, candidate.Member.Name)); + rejections.Add(candidate.Member is FieldInfo + ? NotWritableMessage(type.Name, candidate.Member.Name) + : NonPublicSetterMessage(type.Name, candidate.Member.Name)); keyed.Add((candidate.Member, candidate.MemberType, slot)); } } + if (messagePackObject != null && !type.IsValueType && !HasMessagePackConstructor(type, allowPrivate, keyed.Select(entry => (entry.MemberType, entry.Slot)).Concat(constructorOnly))) + rejections.Add(NoConstructorMessage(type.Name)); + foreach (var group in keyed.GroupBy(entry => entry.Slot).Where(group => group.Count() > 1).OrderBy(group => group.Key)) { var pair = group.Take(2).ToArray(); @@ -947,9 +965,19 @@ namespace GameCult.Caching } } - private static bool IsWritable(MethodInfo setter) => - (setter.IsPublic || setter.IsAssembly || setter.IsFamilyOrAssembly) && - !setter.ReturnParameter.GetRequiredCustomModifiers().Any(modifier => modifier.FullName == "System.Runtime.CompilerServices.IsExternalInit"); + // MessagePack's DynamicObjectResolver: the [SerializationConstructor] if one is marked, otherwise any public constructor (any + // constructor under AllowPrivate) whose every parameter takes the keyed member at its position (slot == parameter index). + private static bool HasMessagePackConstructor(Type type, bool allowPrivate, IEnumerable<(Type MemberType, int Slot)> members) + { + var slots = new Dictionary(); + foreach (var member in members) + slots.TryAdd(member.Slot, member.MemberType); + var constructors = type.GetConstructors(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic); + var marked = constructors.Where(constructor => constructor.IsDefined(typeof(SerializationConstructorAttribute))).ToArray(); + return (marked.Length > 0 ? marked : constructors.Where(constructor => allowPrivate || constructor.IsPublic)).Any(constructor => + constructor.GetParameters().All(parameter => + slots.TryGetValue(parameter.Position, out var memberType) && parameter.ParameterType.IsAssignableFrom(memberType))); + } private static string Qualified(MemberInfo member) => member.DeclaringType!.Name.Split('`')[0] + "." + member.Name; @@ -969,7 +997,16 @@ namespace GameCult.Caching $"Cult document {documentTypeName} member {memberName} has a string [Key]; string keys are not supported; use integer [Key(n)]."; private static string NotWritableMessage(string documentTypeName, string memberName) => - $"Cult document {documentTypeName} member {memberName} is not writable; a persisted member needs a non-readonly field or a public or internal set accessor."; + $"Cult document {documentTypeName} member {memberName} is a readonly field; a persisted field must not be readonly."; + + private static string NonPublicSetterMessage(string documentTypeName, string memberName) => + $"Cult document {documentTypeName} member {memberName} has a non-public set accessor; MessagePack writes it but never reads it back without AllowPrivate, so make the setter public or add [MessagePackObject(AllowPrivate = true)]."; + + private static string KeyedNonPublicMemberMessage(string documentTypeName, string memberName) => + $"Cult document {documentTypeName} member {memberName} is non-public; MessagePack skips it silently, so [Key] on a non-public member requires [MessagePackObject(AllowPrivate = true)]."; + + private static string NoConstructorMessage(string documentTypeName) => + $"Cult document {documentTypeName} has no constructor MessagePack can call; add a parameterless constructor (public unless AllowPrivate), or one whose parameters take the [Key(0)], [Key(1)], ... members in order."; private static string DuplicateSlotMessage(string documentTypeName, string first, string second, int slot) => $"Cult document {documentTypeName} members {first} and {second} share [Key({slot})]; every persisted member needs a distinct [Key(n)]."; diff --git a/tests/GameCult.Caching.Tests/DocumentShapeTests.cs b/tests/GameCult.Caching.Tests/DocumentShapeTests.cs index dd12b640..8b8be54a 100644 --- a/tests/GameCult.Caching.Tests/DocumentShapeTests.cs +++ b/tests/GameCult.Caching.Tests/DocumentShapeTests.cs @@ -15,7 +15,8 @@ namespace GameCult.Caching.Tests // so a refused shape cannot break a registry built from the loaded assemblies. public class DocumentShapeTests { - private const string DivergentOverride = "overrides {0} with a different [Key] or [IgnoreMember]; MessagePack reads the base declaration's, so an override must repeat or omit them."; + private const string NonPublicSetter = "has a non-public set accessor; MessagePack writes it but never reads it back without AllowPrivate, so make the setter public or add [MessagePackObject(AllowPrivate = true)]."; + private const string DivergentOverride ="overrides {0} with a different [Key] or [IgnoreMember]; MessagePack reads the base declaration's, so an override must repeat or omit them."; private static readonly ModuleBuilder Rejected = AssemblyBuilder .DefineDynamicAssembly(new AssemblyName("RejectedShapes"), AssemblyBuilderAccess.Run) @@ -169,7 +170,52 @@ namespace GameCult.Caching.Tests Property(baseType, "Name", 0, MethodAttributes.Private, Key(0)); var type = Document("PrivSetDerived", baseType.CreateType()); Property(type, "X", 0, MethodAttributes.Public, Key(1)); - Reject(type.CreateType()!, "Cult document PrivSetDerived member Name is not writable; a persisted member needs a non-readonly field or a public or internal set accessor."); + Reject(type.CreateType()!, "Cult document PrivSetDerived member Name " + NonPublicSetter); + } + + // Without AllowPrivate MessagePack writes Name through the public getter and loads it back as null. + [Test] + public void InternalSetterWithoutAllowPrivate() + { + var type = Document("InternalSetter"); + Property(type, "Name", 0, MethodAttributes.Assembly, Key(0)); + Reject(type.CreateType()!, "Cult document InternalSetter member Name " + NonPublicSetter); + } + + // Without AllowPrivate MessagePack neither writes nor reads a non-public member, [Key] or not. + [Test] + public void KeyedNonPublicMemberWithoutAllowPrivate() + { + var type = Document("KeyedPrivate"); + Field(type, "Name", typeof(string), Key(0)); + type.DefineField("_secret", typeof(int), FieldAttributes.Private).SetCustomAttribute(Key(1)); + Reject(type.CreateType()!, "Cult document KeyedPrivate member KeyedPrivate._secret is non-public; MessagePack skips it silently, so [Key] on a non-public member requires [MessagePackObject(AllowPrivate = true)]."); + } + + // The shape of `class PrimaryCtor(int p)`: MessagePack throws "can't find matched constructor" on first serialize. + [Test] + public void AllowPrivatePrimaryConstructor() + { + var type = Rejected.DefineType("PrimaryCtor", TypeAttributes.NotPublic); + type.SetCustomAttribute(new CustomAttributeBuilder( + typeof(CultDocumentAttribute).GetConstructor(new[] { typeof(string), typeof(string) })!, new object[] { "shape.primary_ctor", "shape.primary_ctor.v1" })); + type.SetCustomAttribute(new CustomAttributeBuilder( + typeof(MessagePackObjectAttribute).GetConstructor(new[] { typeof(bool) })!, new object[] { false }, + new[] { typeof(MessagePackObjectAttribute).GetProperty(nameof(MessagePackObjectAttribute.AllowPrivate))! }, new object[] { true })); + Field(type, "Name", typeof(string), Key(0)); + var il = type.DefineConstructor(MethodAttributes.Public, CallingConventions.HasThis, new[] { typeof(int) }).GetILGenerator(); + il.Emit(OpCodes.Ldarg_0); + il.Emit(OpCodes.Call, typeof(object).GetConstructor(Type.EmptyTypes)!); + il.Emit(OpCodes.Ret); + Reject(type.CreateType()!, "Cult document PrimaryCtor has no constructor MessagePack can call; add a parameterless constructor (public unless AllowPrivate), or one whose parameters take the [Key(0)], [Key(1)], ... members in order."); + } + + [Test] + public void AllowPrivateSettersRoundTrip() + { + Assert.That(Convert.ToHexString(Accept(PrivateSetterNote.Make("n"))), Is.EqualTo("91A16E")); + Assert.That(Convert.ToHexString(Accept(new InitOnlyNote { Name = "n" })), Is.EqualTo("91A16E")); + Assert.That(Convert.ToHexString(Accept(new InternalSetterNote { Name = "n" })), Is.EqualTo("91A16E")); } [Test] @@ -272,6 +318,29 @@ namespace GameCult.Caching.Tests [Key(1)] public int Count; } + [CultDocument("shape.private_setter", "shape.private_setter.v1")] + [MessagePackObject(AllowPrivate = true)] + internal sealed class PrivateSetterNote + { + [Key(0)] public string Name { get; private set; } = ""; + + public static PrivateSetterNote Make(string name) => new() { Name = name }; + } + + [CultDocument("shape.init_only", "shape.init_only.v1")] + [MessagePackObject(AllowPrivate = true)] + internal sealed class InitOnlyNote + { + [Key(0)] public string Name { get; init; } = ""; + } + + [CultDocument("shape.internal_setter", "shape.internal_setter.v1")] + [MessagePackObject(AllowPrivate = true)] + internal sealed class InternalSetterNote + { + [Key(0)] public string Name { get; internal set; } = ""; + } + [MessagePackObject] public class OvrSameBase {