diff --git a/Cat.Network.Generator.Test/CatNetworkGeneratorTests.cs b/Cat.Network.Generator.Test/CatNetworkGeneratorTests.cs index e25be79..e46987f 100644 --- a/Cat.Network.Generator.Test/CatNetworkGeneratorTests.cs +++ b/Cat.Network.Generator.Test/CatNetworkGeneratorTests.cs @@ -579,14 +579,7 @@ public partial class Parent : NetworkObject { { return; } - if (value is not null) - { - global::Cat.Network.INetworkObject networkValue = value; - if (networkValue.Parent is not null && (!global::System.Object.ReferenceEquals(networkValue.Parent, this) || networkValue.PropertyIndex != propertyIndex)) - { - throw new global::System.InvalidOperationException("NetworkObjects may only occupy one networked property at a time."); - } - } + global::Cat.Network.NetworkObject.ValidateAttachment(value, this, propertyIndex, isCollectionItem: false); field = value; if (oldValue is not null) { diff --git a/Cat.Network.Generator/NetworkObjectPropertiesGenerator.cs b/Cat.Network.Generator/NetworkObjectPropertiesGenerator.cs index fe1b0e0..337caf5 100644 --- a/Cat.Network.Generator/NetworkObjectPropertiesGenerator.cs +++ b/Cat.Network.Generator/NetworkObjectPropertiesGenerator.cs @@ -457,14 +457,7 @@ namespace {0}; {{ return; }} - if (value is not null) - {{ - global::Cat.Network.INetworkObject networkValue = value; - if (networkValue.Parent is not null && (!global::System.Object.ReferenceEquals(networkValue.Parent, this) || networkValue.PropertyIndex != propertyIndex)) - {{ - throw new global::System.InvalidOperationException("NetworkObjects may only occupy one networked property at a time."); - }} - }} + global::Cat.Network.NetworkObject.ValidateAttachment(value, this, propertyIndex, isCollectionItem: false); field = value; if (oldValue is not null) {{ diff --git a/Cat.Network.Test/Entities/AttachmentTestTypes.cs b/Cat.Network.Test/Entities/AttachmentTestTypes.cs new file mode 100644 index 0000000..93300a4 --- /dev/null +++ b/Cat.Network.Test/Entities/AttachmentTestTypes.cs @@ -0,0 +1,19 @@ +namespace Cat.Network.Test.Entities; + +[NetworkObject] +public sealed partial class AttachmentNode : NetworkObject { + [NetworkProperty] + public partial int Value { get; set; } + + [NetworkProperty] + public partial AttachmentNode? Child { get; set; } + + [NetworkProperty] + public partial AttachmentNode? OtherChild { get; set; } + + [NetworkCollection] + public partial NetworkList Children { get; } + + [NetworkCollection] + public partial NetworkDictionary ChildrenByKey { get; } +} diff --git a/Cat.Network.Test/NetworkObjectAttachmentTests.cs b/Cat.Network.Test/NetworkObjectAttachmentTests.cs new file mode 100644 index 0000000..60d3614 --- /dev/null +++ b/Cat.Network.Test/NetworkObjectAttachmentTests.cs @@ -0,0 +1,299 @@ +using System.Reflection; +using Cat.Network.Test.Entities; + +namespace Cat.Network.Test; + +public sealed class NetworkObjectAttachmentTests { + [TestCase("Property", TestName = "SelfPropertyAttachmentRejectsWithoutMutation")] + [TestCase("ListAdd", TestName = "SelfListAttachmentRejectsWithoutMutation")] + [TestCase("ListInsert")] + [TestCase("ListSet")] + [TestCase("DictionaryAdd", TestName = "SelfDictionaryAttachmentRejectsWithoutMutation")] + [TestCase("DictionarySet")] + [TestCase("DictionaryNewKey")] + public void SelfAttachmentRejectsWithoutMutation(string operation) { + AttachmentNode owner = CreateDestination(); + AssertRejectedWithoutMutation(owner, owner, owner, operation); + } + + [TestCase("Property")] + [TestCase("ListSet")] + [TestCase("DictionarySet")] + public void RejectedReplacementLeavesCleanDirtyStateUnchanged(string operation) { + AttachmentNode owner = CreateDestination(); + ClearDirtyState(owner); + AssertRejectedWithoutMutation(owner, owner, owner, operation); + } + + [TestCase("Property")] + [TestCase("ListAdd")] + [TestCase("ListInsert")] + [TestCase("ListSet")] + [TestCase("DictionaryAdd")] + [TestCase("DictionarySet")] + [TestCase("DictionaryNewKey")] + public void MultiLevelAncestorAttachmentRejectsWithoutMutation(string operation) { + AttachmentNode owner = CreateDestination(); + AttachmentNode middle = new() { Value = 20 }; + AttachmentNode root = new() { Value = 30, Child = middle }; + middle.Children.Add(owner); + AssertRejectedWithoutMutation(root, owner, root, operation); + } + + [TestCase("Property")] + [TestCase("ListAdd")] + [TestCase("ListInsert")] + [TestCase("ListSet")] + [TestCase("DictionaryAdd")] + [TestCase("DictionarySet")] + [TestCase("DictionaryNewKey")] + public void ConflictingParentAttachmentRejectsWithoutMutation(string operation) { + AttachmentNode owner = CreateDestination(); + AttachmentNode candidate = new() { Value = 40 }; + AttachmentNode otherParent = new() { Value = 50 }; + otherParent.ChildrenByKey.Add(5, candidate); + AssertRejectedWithoutMutation(owner, owner, candidate, operation, otherParent); + } + + [TestCase("Property")] + [TestCase("ListAdd")] + [TestCase("ListInsert")] + [TestCase("ListSet")] + [TestCase("DictionaryAdd")] + [TestCase("DictionarySet")] + [TestCase("DictionaryNewKey")] + public void DifferentSlotOnSameOwnerRejectsWithoutMutation(string operation) { + AttachmentNode owner = CreateDestination(); + AttachmentNode candidate = new() { Value = 40 }; + owner.OtherChild = candidate; + AssertRejectedWithoutMutation(owner, owner, candidate, operation); + } + + [TestCase("ListAdd")] + [TestCase("ListInsert")] + [TestCase("DictionaryAdd")] + [TestCase("DictionaryNewKey")] + public void SameChildCannotOccupyTwoEntriesInOneCollection(string operation) { + AttachmentNode owner = CreateDestination(); + AttachmentNode candidate = operation.StartsWith("List", StringComparison.Ordinal) + ? owner.Children[0]! + : owner.ChildrenByKey[1]!; + AssertRejectedWithoutMutation(owner, owner, candidate, operation); + } + + [TestCase("Property")] + [TestCase("ListSet")] + [TestCase("DictionarySet")] + public void ReassigningSameInstanceIsANoOp(string operation) { + AttachmentNode owner = CreateDestination(); + AttachmentNode candidate = operation switch { + "Property" => owner.Child!, + "ListSet" => owner.Children[0]!, + _ => owner.ChildrenByKey[1]! + }; + TreeSnapshot before = new(owner); + Attach(owner, candidate, operation); + before.AssertUnchanged(); + } + + [TestCase("Property", "ListAdd")] + [TestCase("ListSet", "DictionaryAdd")] + [TestCase("DictionarySet", "Property")] + public void DetachedChildCanMoveAndStillPropagatesDirtyState(string oldOperation, string newOperation) { + AttachmentNode firstOwner = CreateDestination(); + AttachmentNode child = new() { Value = 40 }; + Attach(firstOwner, child, oldOperation); + switch (oldOperation) { + case "Property": firstOwner.Child = null; break; + case "ListSet": firstOwner.Children.RemoveAt(0); break; + case "DictionarySet": firstOwner.ChildrenByKey.Remove(1); break; + } + + Assert.Multiple(() => { + Assert.That(((INetworkObject)child).Parent, Is.Null); + Assert.That(((INetworkObject)child).PropertyIndex, Is.EqualTo(-1)); + Assert.That(((INetworkObject)child).IsCollectionItem, Is.False); + }); + + AttachmentNode secondOwner = CreateDestination(); + AttachmentNode root = new() { Value = 50 }; + root.ChildrenByKey.Add(10, secondOwner); + Attach(secondOwner, child, newOperation); + ClearDirtyState(firstOwner); + ClearDirtyState(root); + int changed = 0; + child.ValueChanged += (_, _) => changed++; + child.Value++; + + Assert.Multiple(() => { + Assert.That(((INetworkObject)child).Parent, Is.SameAs(secondOwner)); + Assert.That(((INetworkObject)child).IsCollectionItem, Is.EqualTo(newOperation != "Property")); + Assert.That(((INetworkObject)secondOwner).PropertyStates[((INetworkObject)child).PropertyIndex], Is.EqualTo(NetworkPropertyState.Modified)); + Assert.That(((INetworkObject)root).PropertyStates[((INetworkObject)secondOwner).PropertyIndex], Is.EqualTo(NetworkPropertyState.Modified)); + Assert.That(((INetworkObject)firstOwner).PropertyStates, Is.All.EqualTo(NetworkPropertyState.Unchanged)); + Assert.That(changed, Is.EqualTo(1)); + Assert.That(child.Anchor, Is.Null); + }); + } + + [TestCase("Property", false)] + [TestCase("ListAdd", false)] + [TestCase("DictionaryAdd", false)] + [TestCase("Property", true)] + [TestCase("ListAdd", true)] + [TestCase("DictionaryAdd", true)] + public void AlreadyCyclicOwnerChainRejectsWithoutMutation(string operation, bool nullValue) { + AttachmentNode owner = CreateDestination(); + AttachmentNode firstAncestor = new() { Value = 20 }; + AttachmentNode secondAncestor = new() { Value = 30 }; + AttachmentNode? candidate = nullValue ? null : new() { Value = 40 }; + ((INetworkObject)owner).Parent = firstAncestor; + ((INetworkObject)owner).PropertyIndex = 0; + ((INetworkObject)firstAncestor).Parent = secondAncestor; + ((INetworkObject)firstAncestor).PropertyIndex = 0; + ((INetworkObject)secondAncestor).Parent = firstAncestor; + ((INetworkObject)secondAncestor).PropertyIndex = 0; + + try { + // Snapshot follows member values, not the deliberately corrupted parent links. + AssertRejectedWithoutMutation(owner, owner, candidate, operation, firstAncestor, secondAncestor); + } finally { + ((INetworkObject)owner).Parent = null; + ((INetworkObject)firstAncestor).Parent = null; + ((INetworkObject)secondAncestor).Parent = null; + } + } + + [TestCase(false)] + [TestCase(true)] + public void DeserializedReplacementValidatesBeforeDetachingPreviousValue(bool dictionary) { + AttachmentNode owner = CreateDestination(); + TreeSnapshot before = new(owner); + object collection = dictionary ? owner.ChildrenByKey : owner.Children; + Type collectionType = dictionary ? typeof(NetworkDictionary) : typeof(NetworkList); + MethodInfo set = collectionType.GetMethod("SetDeserialized", BindingFlags.Instance | BindingFlags.NonPublic)!; + + // Exercise the shared replacement hook directly with an invalid decoded object. + TargetInvocationException? error = Assert.Throws(() => set.Invoke(collection, [dictionary ? 1 : 0, owner])); + Assert.That(error!.InnerException, Is.TypeOf()); + before.AssertUnchanged(); + } + + private static AttachmentNode CreateDestination() { + AttachmentNode owner = new() { Value = 10, Child = new AttachmentNode { Value = 11 } }; + owner.Children.Add(new AttachmentNode { Value = 12 }); + owner.ChildrenByKey.Add(1, new AttachmentNode { Value = 13 }); + return owner; + } + + private static void Attach(AttachmentNode owner, AttachmentNode? candidate, string operation) { + switch (operation) { + case "Property": owner.Child = candidate; break; + case "ListAdd": owner.Children.Add(candidate); break; + case "ListInsert": owner.Children.Insert(0, candidate); break; + case "ListSet": owner.Children[0] = candidate; break; + case "DictionaryAdd": owner.ChildrenByKey.Add(2, candidate); break; + case "DictionarySet": owner.ChildrenByKey[1] = candidate; break; + case "DictionaryNewKey": owner.ChildrenByKey[2] = candidate; break; + default: throw new ArgumentException("Unknown attachment operation.", nameof(operation)); + } + } + + private static void AssertRejectedWithoutMutation(AttachmentNode root, AttachmentNode owner, AttachmentNode? candidate, string operation, params AttachmentNode[] otherRoots) { + TreeSnapshot before = new([root, .. otherRoots, candidate ?? root]); + Assert.That(() => Attach(owner, candidate, operation), Throws.TypeOf()); + before.AssertUnchanged(); + } + + private static (INetworkObjectSerializer Serializer, SerializationContext Context) GetSerializer() { + TypeCatalogue catalogue = new(); + catalogue.Register(typeof(AttachmentNode)); + catalogue.TryFindSerializer(typeof(AttachmentNode), out INetworkObjectSerializer? serializer); + return (serializer!, new SerializationContext(catalogue)); + } + + private static byte[] Serialize(AttachmentNode node, MemberSelectionMode mode) { + (INetworkObjectSerializer serializer, SerializationContext context) = GetSerializer(); + BufferWriter writer = new(); + serializer.Serialize(writer, node, context, new SerializationOptions(mode, MemberIdentificationMode.Index)); + return writer.GetWrittenSpan().ToArray(); + } + + private static void ClearDirtyState(AttachmentNode root) { + (INetworkObjectSerializer serializer, SerializationContext context) = GetSerializer(); + serializer.ClearDirtyState(root, context); + } + + private sealed class TreeSnapshot { + private readonly List checks = []; + private int events; + + public TreeSnapshot(params AttachmentNode[] roots) { + HashSet seen = new(ReferenceEqualityComparer.Instance); + foreach (AttachmentNode root in roots) { + Capture(root, seen); + } + } + + public void AssertUnchanged() { + Assert.Multiple(() => { + foreach (Action check in checks) { + check(); + } + Assert.That(events, Is.Zero, "Rejected or identical assignments must not raise events."); + }); + } + + private void Capture(AttachmentNode node, HashSet seen) { + if (!seen.Add(node)) { + return; + } + + INetworkObject state = node; + NetworkObject? parent = state.Parent; + int propertyIndex = state.PropertyIndex; + bool collectionItem = state.IsCollectionItem; + NetworkPropertyState[] propertyStates = state.PropertyStates.ToArray(); + AttachmentNode? child = node.Child; + AttachmentNode? otherChild = node.OtherChild; + AttachmentNode?[] list = node.Children.ToArray(); + KeyValuePair[] dictionary = node.ChildrenByKey.ToArray(); + byte[] fullPayload = Serialize(node, MemberSelectionMode.All); + byte[] dirtyPayload = Serialize(node, MemberSelectionMode.Dirty); + node.PropertyChanged += (_, _) => events++; + node.ChildChanged += (_, _) => events++; + node.OtherChildChanged += (_, _) => events++; + node.Children.ItemAdded += (_, _) => events++; + node.Children.ItemRemoved += (_, _) => events++; + node.Children.IndexChanged += (_, _) => events++; + node.ChildrenByKey.ItemAdded += (_, _) => events++; + node.ChildrenByKey.ItemRemoved += (_, _) => events++; + node.ChildrenByKey.ValueChanged += (_, _) => events++; + + checks.Add(() => { + Assert.That(state.Parent, Is.SameAs(parent)); + Assert.That(state.PropertyIndex, Is.EqualTo(propertyIndex)); + Assert.That(state.IsCollectionItem, Is.EqualTo(collectionItem)); + Assert.That(state.PropertyStates, Is.EqualTo(propertyStates)); + Assert.That(node.Child, Is.SameAs(child)); + Assert.That(node.OtherChild, Is.SameAs(otherChild)); + Assert.That(node.Children.Count, Is.EqualTo(list.Length)); + for (int index = 0; index < Math.Min(node.Children.Count, list.Length); index++) { + Assert.That(node.Children[index], Is.SameAs(list[index])); + } + Assert.That(node.ChildrenByKey.Keys, Is.EquivalentTo(dictionary.Select(entry => entry.Key))); + foreach ((int key, AttachmentNode? value) in dictionary) { + Assert.That(node.ChildrenByKey[key], Is.SameAs(value)); + } + Assert.That(Serialize(node, MemberSelectionMode.All), Is.EqualTo(fullPayload)); + Assert.That(Serialize(node, MemberSelectionMode.Dirty), Is.EqualTo(dirtyPayload)); + }); + + foreach (AttachmentNode? descendant in new[] { child, otherChild }.Concat(list).Concat(dictionary.Select(entry => entry.Value))) { + if (descendant is not null) { + Capture(descendant, seen); + } + } + } + } +} diff --git a/Cat.Network/NetworkDictionary.cs b/Cat.Network/NetworkDictionary.cs index ef0d9c4..7a5d658 100644 --- a/Cat.Network/NetworkDictionary.cs +++ b/Cat.Network/NetworkDictionary.cs @@ -297,11 +297,11 @@ protected void AddDeserialized(TKey key, TValue value) { protected void SetDeserialized(TKey key, TValue value) { bool replacedValue = Items.TryGetValue(key, out TValue? previous); + ValidateValueForAssignment(value); if (replacedValue) { OnValueRemoving(previous!); } - ValidateValueForAssignment(value); Items[key] = value; OnValueAdded(value); if (replacedValue) { diff --git a/Cat.Network/NetworkList.cs b/Cat.Network/NetworkList.cs index 665207d..bcd1c24 100644 --- a/Cat.Network/NetworkList.cs +++ b/Cat.Network/NetworkList.cs @@ -298,8 +298,8 @@ protected void InsertDeserialized(int index, T item) { protected void SetDeserialized(int index, T item) { T previous = Items[index]; - OnItemRemoving(previous); ValidateItemForAssignment(item); + OnItemRemoving(previous); Items[index] = item; OnItemAdded(item); IndexChanged?.Invoke(this, index); diff --git a/Cat.Network/NetworkObject.cs b/Cat.Network/NetworkObject.cs index 7f4b61b..fa4b88b 100644 --- a/Cat.Network/NetworkObject.cs +++ b/Cat.Network/NetworkObject.cs @@ -27,6 +27,34 @@ protected NetworkObject() { public abstract NetworkObject Clone(); + /// Checks ownership and parent cycles before changing an object attachment. + /// + /// Used by generated setters in consumer assemblies and by object collections. + /// Properties may retain the same owner and property index; collection items must be unattached. + /// + protected internal static void ValidateAttachment(NetworkObject? value, NetworkObject? owner, int propertyIndex, bool isCollectionItem) { + if (value is INetworkObject networkValue && networkValue.Parent is not null && + (isCollectionItem || !ReferenceEquals(networkValue.Parent, owner) || networkValue.PropertyIndex != propertyIndex)) { + throw new InvalidOperationException("NetworkObjects may only occupy one networked property or collection item at a time."); + } + + NetworkObject? ancestor = owner; + NetworkObject? fast = owner; + while (ancestor is not null) { + if (ReferenceEquals(ancestor, value)) { + throw new InvalidOperationException("A NetworkObject cannot be attached to itself or one of its descendants."); + } + + ancestor = ((INetworkObject)ancestor).Parent; + // Detect an already corrupted parent chain without allocating for each assignment. + fast = fast is null ? null : ((INetworkObject)fast).Parent; + fast = fast is null ? null : ((INetworkObject)fast).Parent; + if (ancestor is not null && ReferenceEquals(ancestor, fast)) { + throw new InvalidOperationException("Cannot attach a NetworkObject to an owner with a cyclic parent chain."); + } + } + } + private protected virtual NetworkObject? GetAnchor() { return ((INetworkObject?)((INetworkObject)this).Parent)?.Anchor; } diff --git a/Cat.Network/NetworkObjectDictionary.cs b/Cat.Network/NetworkObjectDictionary.cs index 66f8808..d95cce4 100644 --- a/Cat.Network/NetworkObjectDictionary.cs +++ b/Cat.Network/NetworkObjectDictionary.cs @@ -2,14 +2,7 @@ namespace Cat.Network; public sealed class NetworkObjectDictionary : NetworkDictionary where TKey : notnull where TValue : NetworkObject? { protected override void ValidateValueForAssignment(TValue value) { - if (value is null) { - return; - } - - INetworkObject networkObject = value; - if (networkObject.Parent is not null) { - throw new InvalidOperationException("NetworkObjects may only occupy one networked property or list at a time."); - } + NetworkObject.ValidateAttachment(value, Owner, PropertyIndex, isCollectionItem: true); } protected override void OnValueAdded(TValue value) { diff --git a/Cat.Network/NetworkObjectList.cs b/Cat.Network/NetworkObjectList.cs index 16c10fe..e0fc6ed 100644 --- a/Cat.Network/NetworkObjectList.cs +++ b/Cat.Network/NetworkObjectList.cs @@ -2,14 +2,7 @@ namespace Cat.Network; public sealed class NetworkObjectList : NetworkList where T : NetworkObject? { protected override void ValidateItemForAssignment(T item) { - if (item is null) { - return; - } - - INetworkObject networkObject = item; - if (networkObject.Parent is not null) { - throw new InvalidOperationException("NetworkObjects may only occupy one networked property or list at a time."); - } + NetworkObject.ValidateAttachment(item, Owner, PropertyIndex, isCollectionItem: true); } protected override void OnItemAdded(T item) { diff --git a/README.md b/README.md index ca06dc1..9458b04 100644 --- a/README.md +++ b/README.md @@ -63,6 +63,10 @@ public partial class InventoryState : NetworkObject { Collection properties are initialized by generated code. Do not assign them yourself. +Nested objects form ownership trees: an object can occupy one networked property or collection item at a time. Detach a child before moving it elsewhere. Assigning an object to itself or beneath one of its descendants throws `InvalidOperationException` before changing the previous attachment, collection contents, dirty state, or events. + +Keep the runtime and source generator from the same package release or source revision, and rebuild projects that declare network types when updating them. Generated property setters call the runtime's attachment validator; previously compiled setters must be regenerated to gain the cycle checks. This does not change serialized schemas or wire data. + ## Register Types Runtime serialization uses a `TypeCatalogue`. Register every concrete network object type that may be serialized or deserialized.