Make graph attributes iteration order canonical - #286
Merged
Conversation
GraphAttributesImpl backed its map with a java.util.HashMap, and Serialization.serializeGraphAttributes iterates that map's entrySet() straight into the byte stream. HashMap iteration order is an implementation detail, so the serialized bytes were a function of insertion history rather than of content alone. Switch the field to a TreeMap so iteration is sorted by key and the output bytes become a pure function of the content. This is a prerequisite for byte-pinned serialization fixtures. TreeMap rejects null keys where HashMap accepted them, and throws NPE from deep inside the map on get(null)/containsKey(null) where HashMap returned null/false. Add explicit Objects.requireNonNull(key, "key") guards to every public method taking a key so the failure is intentional and well-messaged. deepHashCode and deepEquals are unchanged and remain correct: Map.hashCode() is specified as the order-independent sum of entry hash codes, and deepEquals goes through MapDeepEquals which compares by key lookup. The read path is unaffected -- deserializeGraphAttributes just puts entries into the map -- so previously written files still load identically. Only the order of newly written graph-attribute entries changes; the format itself is untouched and Serialization.VERSION is not bumped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
GraphAttributesImplbacked its map with aHashMap, andSerialization.serializeGraphAttributesiterates that map'sentrySet()straight into the byte stream. HashMap iteration order is an implementation detail, so the serialized bytes were a function of insertion history rather than of content alone.Switching to a
TreeMapmakes iteration sorted by key, so the same attributes always produce the same bytes regardless of the order they were set in.TreeMaprejects null keys whereHashMapaccepted them, and throws NPE from deep inside the map onget(null)whereHashMapreturned null. AddedObjects.requireNonNull(key, "key")to all nine public methods taking a key, so the failure is intentional and well-messaged rather than incidental.deepHashCodeanddeepEqualsare unchanged and remain correct —Map.hashCode()is specified as the order-independent sum of entry hash codes, anddeepEqualsgoes throughMapDeepEquals, which compares by key lookup.The read path is unaffected, so previously written files still load identically. Only the ordering of newly written attribute entries changes; the format itself is untouched and
Serialization.VERSIONis not bumped.1597 tests pass.
🤖 Generated with Claude Code