diff --git a/src/GameCult.Caching/Contracts/cultcache-schema-compatibility.md b/src/GameCult.Caching/Contracts/cultcache-schema-compatibility.md index 9669044c..297c3184 100644 --- a/src/GameCult.Caching/Contracts/cultcache-schema-compatibility.md +++ b/src/GameCult.Caching/Contracts/cultcache-schema-compatibility.md @@ -34,23 +34,32 @@ A member's slot is its MessagePack `[Key(n)]` integer. MessagePack's `[Key]` and the registry refuses a document type without `[MessagePackObject]` and a non-public document type without `[MessagePackObject(AllowPrivate = true)]`. 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 readonly -persisted field. +The registry also refuses a public member without `[Key]` or `[IgnoreMember]` +(get-only properties included; MessagePack throws on them), a string `[Key]`, +two members sharing a slot (including `new`-hidden members), a hidden persisted +member, and an override whose `[Key]` or `[IgnoreMember]` differs from its base +declaration's (MessagePack reads the base declaration). 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. +every keyed public field and property, get-only ones included, because +MessagePack writes them all. A `[Key]` on a non-public member without +`AllowPrivate` is refused because MessagePack skips that member. With +`AllowPrivate`, MessagePack also reads non-public members, so every non-public +instance field or property must be `[IgnoreMember]`. + +A class needs a constructor MessagePack can call: the `[SerializationConstructor]` +if one is marked, otherwise the longest constructor (public unless +`AllowPrivate`) whose every parameter takes the keyed member at its position +(`[Key(0)]`, `[Key(1)]`, ...). A parameterless constructor always qualifies; a +primary constructor usually does not. MessagePack reads a member back through +its setter, or through that constructor when the member's slot is below the +constructor's parameter count. A setter counts when it is public, or any setter +(private, internal, init-only) under `AllowPrivate`; a readonly field counts as +writable under `AllowPrivate` only. A keyed member MessagePack cannot read back +either way is refused: a get-only property, a readonly field without +`AllowPrivate`, or a property whose setter is non-public without +`AllowPrivate`, when the constructor does not fill it. ## Which schema a record carries diff --git a/src/GameCult.Caching/CultCache.cs b/src/GameCult.Caching/CultCache.cs index 47440394..940bfc85 100644 --- a/src/GameCult.Caching/CultCache.cs +++ b/src/GameCult.Caching/CultCache.cs @@ -833,24 +833,24 @@ namespace GameCult.Caching public int[] DefaultedMissingSlots { get; } } - // Public instance fields and get/set properties, most-derived declaration first; GetCustomAttributes inherits a property's - // attributes up its override chain, as MessagePack's own resolver reads them. + // Public instance fields and properties (get-only included: MessagePack writes every keyed public member), most-derived + // declaration first; GetCustomAttributes inherits a property's attributes up its override chain, as MessagePack's resolver reads them. 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; + // Writable: MessagePack can assign the member after construction. A member that is not must be filled by the constructor. 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++) { foreach (var field in current.GetFields(declared)) { + // MessagePack's rule: a readonly field is writable under AllowPrivate only. if (!IsIgnored(field)) - candidates.Add((field, field.FieldType, depth, !field.IsInitOnly, null)); + candidates.Add((field, field.FieldType, depth, !field.IsInitOnly || allowPrivate, null)); } foreach (var property in current.GetProperties(declared)) @@ -861,12 +861,6 @@ namespace GameCult.Caching var setter = chain.Select(declaration => declaration.SetMethod).FirstOrDefault(method => method != 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 => @@ -876,7 +870,7 @@ namespace GameCult.Caching if (divergentRoot == null && IsIgnored(property)) continue; // 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)); + candidates.Add((property, property.PropertyType, depth, setter != null && (setter.IsPublic || allowPrivate), divergentRoot)); } } @@ -901,7 +895,7 @@ namespace GameCult.Caching } } } - var keyed = new List<(MemberInfo Member, Type MemberType, int Slot)>(); + var keyed = new List<(MemberInfo Member, Type MemberType, int Slot, bool Writable)>(); foreach (var candidate in candidates.OrderBy(candidate => candidate.Depth).ThenBy(candidate => candidate.Member.Name, StringComparer.Ordinal)) { var key = GetKeyValue(candidate.Member); @@ -912,17 +906,29 @@ namespace GameCult.Caching else if (key is not int slot) rejections.Add(UnkeyedMemberMessage(type.Name, candidate.Member.Name)); else - { - if (!candidate.Writable) - 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)); - } + keyed.Add((candidate.Member, candidate.MemberType, slot, candidate.Writable)); } - if (messagePackObject != null && !type.IsValueType && !HasMessagePackConstructor(type, allowPrivate, keyed.Select(entry => (entry.MemberType, entry.Slot)).Concat(constructorOnly))) - rejections.Add(NoConstructorMessage(type.Name)); + if (messagePackObject != null) + { + // A member MessagePack cannot assign is read back only through the picked constructor's parameter at its slot. + var arity = ConstructorArity(type, allowPrivate, keyed.Select(entry => (entry.MemberType, entry.Slot))); + if (arity == null) + rejections.Add(NoConstructorMessage(type.Name)); + else + { + foreach (var entry in keyed.Where(entry => !entry.Writable && entry.Slot >= arity)) + { + rejections.Add(entry.Member switch + { + FieldInfo => ReadonlyUnfilledMessage(type.Name, entry.Member.Name, entry.Slot), + PropertyInfo property when property.SetMethod == null && OverrideChain(property).All(declaration => declaration.SetMethod == null) => + GetOnlyUnfilledMessage(type.Name, entry.Member.Name, entry.Slot), + _ => NonPublicSetterMessage(type.Name, entry.Member.Name) + }); + } + } + } foreach (var group in keyed.GroupBy(entry => entry.Slot).Where(group => group.Count() > 1).OrderBy(group => group.Key)) { @@ -965,18 +971,23 @@ namespace GameCult.Caching } } - // 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) + // MessagePack's DynamicObjectResolver picks the [SerializationConstructor] if one is marked, otherwise the longest public constructor + // (any constructor under AllowPrivate) whose every parameter takes the keyed member at its position (slot == parameter index). + // Returns that constructor's parameter count, 0 for a struct with none, or null when MessagePack finds no constructor. + private static int? ConstructorArity(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))); + var arities = (marked.Length > 0 ? marked : constructors.Where(constructor => allowPrivate || constructor.IsPublic)) + .Select(constructor => constructor.GetParameters()) + .Where(parameters => parameters.All(parameter => + slots.TryGetValue(parameter.Position, out var memberType) && parameter.ParameterType.IsAssignableFrom(memberType))) + .Select(parameters => (int?)parameters.Length) + .ToArray(); + return arities.Length > 0 ? arities.Max() : type.IsValueType && marked.Length == 0 ? 0 : null; } private static string Qualified(MemberInfo member) => member.DeclaringType!.Name.Split('`')[0] + "." + member.Name; @@ -991,13 +1002,16 @@ namespace GameCult.Caching $"Cult document {documentTypeName} member {memberName} is non-public; [MessagePackObject(AllowPrivate = true)] makes MessagePack read it, so mark it [IgnoreMember]."; internal static string UnkeyedMemberMessage(string documentTypeName, string memberName) => - $"Cult document {documentTypeName} member {memberName} has no [Key]; every persisted member of a [CultDocument] type needs an explicit [Key(n)]."; + $"Cult document {documentTypeName} member {memberName} has no [Key]; MessagePack requires [Key(n)] or [IgnoreMember] on every public member, so mark it one or the other."; private static string StringKeyMessage(string documentTypeName, string memberName) => $"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 a readonly field; a persisted field must not be readonly."; + private static string ReadonlyUnfilledMessage(string documentTypeName, string memberName, int slot) => + $"Cult document {documentTypeName} member {memberName} is a readonly field that no constructor fills; MessagePack writes it but never reads it back, so give the constructor MessagePack calls a parameter at position {slot}, drop readonly, or add [MessagePackObject(AllowPrivate = true)]."; + + private static string GetOnlyUnfilledMessage(string documentTypeName, string memberName, int slot) => + $"Cult document {documentTypeName} member {memberName} is a get-only property that no constructor fills; MessagePack writes it but never reads it back, so give the constructor MessagePack calls a parameter at position {slot}, add a setter, or mark it [IgnoreMember]."; 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)]."; diff --git a/tests/GameCult.Caching.Tests/DocumentShapeTests.cs b/tests/GameCult.Caching.Tests/DocumentShapeTests.cs index 8b8be54a..c34834ca 100644 --- a/tests/GameCult.Caching.Tests/DocumentShapeTests.cs +++ b/tests/GameCult.Caching.Tests/DocumentShapeTests.cs @@ -16,6 +16,8 @@ namespace GameCult.Caching.Tests public class DocumentShapeTests { 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 Unkeyed = "has no [Key]; MessagePack requires [Key(n)] or [IgnoreMember] on every public member, so mark it one or the other."; + private const string ReadonlyUnfilled = "is a readonly field that no constructor fills; MessagePack writes it but never reads it back, so give the constructor MessagePack calls a parameter at position {0}, drop readonly, 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 @@ -104,7 +106,7 @@ namespace GameCult.Caching.Tests var type = Document("Unkeyed"); Field(type, "Name", typeof(string), Key(0)); Field(type, "Count", typeof(int)); - Reject(type.CreateType()!, "Cult document Unkeyed member Count has no [Key]; every persisted member of a [CultDocument] type needs an explicit [Key(n)]."); + Reject(type.CreateType()!, "Cult document Unkeyed member Count " + Unkeyed); } // MessagePack's DynamicObjectResolver reads the base declaration: it throws "key is duplicated" for this shape. @@ -218,6 +220,81 @@ namespace GameCult.Caching.Tests Assert.That(Convert.ToHexString(Accept(new InternalSetterNote { Name = "n" })), Is.EqualTo("91A16E")); } + // MessagePack fills a get-only property through the longest constructor whose parameters take the keyed members by position. + [Test] + public void GetOnlyPropertyFilledByConstructor() + { + Assert.That(Convert.ToHexString(Accept(new GetOnlyNote("n", 2))), Is.EqualTo("92A16E02")); + AssertSlots("0:Name", "1:Count"); + } + + // The three-parameter constructor has no member at position 2, so MessagePack falls back to the one-parameter constructor. + [Test] + public void ReadonlyFieldFilledByConstructor() + { + Assert.That(Convert.ToHexString(Accept(new ReadonlyNote("n") { Count = 3 })), Is.EqualTo("92A16E03")); + AssertSlots("0:Name", "1:Count"); + } + + // Under AllowPrivate MessagePack writes a readonly field directly; no constructor has to fill it. + [Test] + public void ReadonlyFieldUnderAllowPrivate() + { + var sample = new ReadonlyPrivateNote(); + typeof(ReadonlyPrivateNote).GetField(nameof(ReadonlyPrivateNote.Name))!.SetValue(sample, "n"); + Assert.That(Convert.ToHexString(Accept(sample)), Is.EqualTo("91A16E")); + Assert.That(((ReadonlyPrivateNote)CultDocumentMessagePackSerialization.DeserializeUntyped( + typeof(ReadonlyPrivateNote), Accept(sample), CultDocumentRegistry.ForTypes(new[] { typeof(ReadonlyPrivateNote) }))).Name, Is.EqualTo("n")); + AssertSlots("0:Name"); + } + + // MessagePack throws "all public members must mark KeyAttribute or IgnoreMemberAttribute" for this shape. + [Test] + public void UnkeyedGetOnlyProperty() + { + var type = Document("UnkeyedGetter"); + Field(type, "Name", typeof(string), Key(0)); + GetOnlyProperty(type, "Upper"); + Reject(type.CreateType()!, "Cult document UnkeyedGetter member Upper " + Unkeyed); + } + + // MessagePack writes Label and loads it back as the default: only the parameterless constructor exists. + [Test] + public void GetOnlyPropertyNoConstructorFills() + { + var type = Document("GetterUnfilled"); + Field(type, "Name", typeof(string), Key(0)); + GetOnlyProperty(type, "Label", Key(1)); + Reject(type.CreateType()!, "Cult document GetterUnfilled member Label is a get-only property that no constructor fills; MessagePack writes it but never reads it back, so give the constructor MessagePack calls a parameter at position 1, add a setter, or mark it [IgnoreMember]."); + } + + // Without AllowPrivate MessagePack writes Name and loads it back as the field initializer's value. + [Test] + public void ReadonlyFieldNoConstructorFills() + { + var type = Document("ReadonlyUnfilled"); + type.DefineField("Name", typeof(string), FieldAttributes.Public | FieldAttributes.InitOnly).SetCustomAttribute(Key(0)); + Reject(type.CreateType()!, "Cult document ReadonlyUnfilled member Name " + string.Format(ReadonlyUnfilled, 0)); + } + + // MessagePack picks the one-parameter constructor, which fills Name but not Other; Other loads back as its default. + [Test] + public void ReadonlyFieldBeyondPickedConstructor() + { + var type = Document("ReadonlyBeyond"); + var name = type.DefineField("Name", typeof(string), FieldAttributes.Public | FieldAttributes.InitOnly); + name.SetCustomAttribute(Key(0)); + type.DefineField("Other", typeof(string), FieldAttributes.Public | FieldAttributes.InitOnly).SetCustomAttribute(Key(1)); + var il = type.DefineConstructor(MethodAttributes.Public, CallingConventions.HasThis, new[] { typeof(string) }).GetILGenerator(); + il.Emit(OpCodes.Ldarg_0); + il.Emit(OpCodes.Call, typeof(object).GetConstructor(Type.EmptyTypes)!); + il.Emit(OpCodes.Ldarg_0); + il.Emit(OpCodes.Ldarg_1); + il.Emit(OpCodes.Stfld, name); + il.Emit(OpCodes.Ret); + Reject(type.CreateType()!, "Cult document ReadonlyBeyond member Other " + string.Format(ReadonlyUnfilled, 1)); + } + [Test] public void StringKey() { @@ -238,6 +315,23 @@ namespace GameCult.Caching.Tests return payload; } + private static void AssertSlots(params string[] expected) where T : class => + Assert.That(CultDocumentRegistry.ForTypes(new[] { typeof(T) }).GetRequired().ToCatalogEntry().Members.Select(member => $"{member.Slot}:{member.MemberName}"), Is.EqualTo(expected)); + + private static void GetOnlyProperty(TypeBuilder type, string name, params CustomAttributeBuilder[] attributes) + { + var backing = type.DefineField("_" + name, typeof(string), FieldAttributes.Private); + backing.SetCustomAttribute(new CustomAttributeBuilder(typeof(IgnoreMemberAttribute).GetConstructor(Type.EmptyTypes)!, Array.Empty())); + var getter = type.DefineMethod("get_" + name, MethodAttributes.Public | MethodAttributes.SpecialName | MethodAttributes.HideBySig, typeof(string), Type.EmptyTypes); + var il = getter.GetILGenerator(); + il.Emit(OpCodes.Ldarg_0); + il.Emit(OpCodes.Ldfld, backing); + il.Emit(OpCodes.Ret); + var property = type.DefineProperty(name, PropertyAttributes.None, typeof(string), null); + property.SetGetMethod(getter); + foreach (var attribute in attributes) property.SetCustomAttribute(attribute); + } + private static void Reject(Type type, string expected) => Assert.That(Assert.Throws(() => CultDocumentRegistry.ForTypes(new[] { type }))!.Message, Is.EqualTo(expected)); @@ -341,6 +435,39 @@ namespace GameCult.Caching.Tests [Key(0)] public string Name { get; internal set; } = ""; } + [CultDocument("shape.get_only", "shape.get_only.v1")] + [MessagePackObject] + public sealed class GetOnlyNote + { + public GetOnlyNote(string name, int count) + { + Name = name; + Count = count; + } + + [Key(0)] public string Name { get; } + [Key(1)] public int Count { get; set; } + } + + [CultDocument("shape.readonly", "shape.readonly.v1")] + [MessagePackObject] + public sealed class ReadonlyNote + { + public ReadonlyNote() => Name = ""; + public ReadonlyNote(string name) => Name = name; + public ReadonlyNote(string name, int count, string extra) => Name = name; + + [Key(0)] public readonly string Name; + [Key(1)] public int Count { get; set; } + } + + [CultDocument("shape.readonly_private", "shape.readonly_private.v1")] + [MessagePackObject(AllowPrivate = true)] + internal sealed class ReadonlyPrivateNote + { + [Key(0)] public readonly string Name = "init"; + } + [MessagePackObject] public class OvrSameBase {