fix: detect circular references by reference, not by Equals - #28
Open
endel wants to merge 2 commits into
Open
Conversation
The Compile glob still referenced Assets/Plugins/GameDevWare.Serialization, which moved to Packages/com.gamedevware.serialization/Runtime in 3.0.0, so the library compiled no sources and the test project failed to build. Assisted-by: Claude Opus 5
Stack<object>.Contains() compares with EqualityComparer<object>.Default, so a type overriding Equals over a subset of its state was reported as a circular reference whenever an ancestor and a descendant agreed on that subset — equality by id, by kind, by owner. Two distinct objects, no cycle, but both Json.Serialize and MsgPack.Serialize threw. Compare by reference instead: only the same instance nested inside itself can recurse forever. Genuine cycles, including indirect ones, are still detected. Assisted-by: Claude Opus 5
Owner
|
I already did almost the same thing using |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The circular-reference guard added in 3.0.0 fires on graphs that aren't circular.
Stack<object>.Contains()compares withEqualityComparer<object>.Default—Equals(), not reference identity. So any type that overridesEqualsto compare a subset of its serialized state trips the guard as soon as an ancestor and a descendant agree on that subset. Types with full structural equality (a plain positionalrecord, or no override at all) are immune, since a finite tree can never equal its own subtree — but partial equality is a common pattern:{"SenderId":"u1","Text":"b","Quoted":{"SenderId":"u1","Text":"a","Quoted":null}}SerializationException: Circular reference detected for type 'ChatMsg'. Path: 'Quoted'.Two distinct objects, no cycle.
MsgPack.Serializebehaves the same. An expression tree with a repeated operator and equality byOp, or entity types with a duplicated id in a nesting relationship, fail the same way.The fix
SerializationContext.IsInHierarchy(object)walks the hierarchy comparing withReferenceEquals, and the three call sites (ArraySerializer,ObjectSerializer,DictionarySerializer) use it instead ofHierarchy.Contains(value). Only the same instance nested inside itself can recurse forever. It also behaves correctly for boxed value types, where each boxing produces a distinct object.Tests
Three tests added to
ResilienceTests: equal-but-distinct nested objects over MsgPack and JSON, plus a shared-instance (DAG) control. The first two fail onmasterwith the exception above and pass with the fix.I also checked the fix doesn't weaken real detection — self-referencing objects, indirect two-object cycles (
a.Child = b; b.Child = a), and a list containing itself all still throw, over both JSON and MsgPack. The existingCircularReference*andDeepNesting*tests are untouched and still pass.Full suite: 98 passed, 0 failed (95 before, plus the 3 new).
Note on the first commit
The .NET test suite doesn't build on
master—GameDevWare.Serialization.csprojstill globsAssets/Plugins/GameDevWare.Serialization/**/*.cs, which moved toPackages/com.gamedevware.serialization/Runtimein 3.0.0, so the library project compiles zero sources. The first commit repoints it, which is what makes the regression test runnable. It's separated out so you can drop or replace it if you handle that differently — the fix itself is entirely in the second commit.Found while validating the 3.0.0 update for the Colyseus Unity SDK (colyseus/colyseus-unity-sdk#265). Thanks for the release — the rest of it checked out cleanly on our side: MessagePack output is byte-identical to 2.4.x for all three
DateTimeKinds, and date parsing is unchanged across every format we tested.