Add golden-fixture regression suite for the serialization format - #285
Closed
mbastian wants to merge 2 commits into
Closed
Add golden-fixture regression suite for the serialization format#285mbastian wants to merge 2 commits into
mbastian wants to merge 2 commits into
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>
Pins the on-disk serialization format against golden files so the upcoming serializer dispatch refactor cannot silently change the bytes we write or break reading older files. Fixtures live in src/test/resources/serialization/, one directory per MINOR version. 0.4 through 0.7 are real files produced by those released versions, imported from the abandoned graphstore-compatibility-testing repo (0.6.13 and 0.6.14 were verified byte-identical, so only one copy is kept; 0.7 comes from a 0.7.0-SNAPSHOT build). Those are tiny and only cover the format skeleton. 0.8 is generated by the committed SerializationFixtureGenerator and carries the type-surface coverage: a column of every supported type including Character, char[] and the array types, both time representations, timestamp and interval maps of every value type, graph attributes, views, edge types, mixed directedness and dynamic edge weights. SerializationCompatibilityTest enforces three contracts: 1. Backward compatibility - every fixture, from 0.4 up, deserializes and holds the expected content. 2. Format drift - for the current minor only, serializing the model built by the generator must reproduce the committed bytes exactly. Older minors are exempt: we no longer write those formats. 3. Determinism - the same content always serializes to the same bytes, independently of the order it was built in, including graph attributes inserted in different orders. A round-trip-only test covers the surface that cannot be byte-pinned: generic Map and Set attribute values, which Serialization.serializeMap and serializeSet iterate in hash order. Byte mismatches report the first differing offset and the surrounding bytes rather than just "arrays differ". Contract 2 compares generator output to the committed file rather than re-serializing a model read back from it: deserialization is not byte-idempotent here (GraphVersion counters and TimeIndexStore.countMap are restored from the stream and then bumped again as elements are re-inserted), so no fixture could ever be a byte-level fixed point. The read path is covered by the content assertions of contract 1. No production code is touched and Serialization.VERSION is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
Author
|
Closing to split this up. The graph-attributes ordering change is going out as its own focused PR; the golden-fixture suite will follow separately once that lands. The fixture work is preserved on the |
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.
Pins the on-disk serialization format against golden files, so the upcoming serializer dispatch refactor cannot silently change the bytes we write or break reading older files.
Fixtures, one directory per minor version
src/test/resources/serialization/graphstore-compatibility-testingrepo. 0.6.13 and 0.6.14 were byte-identical so only one copy is kept; 0.7 comes from a0.7.0-SNAPSHOTbuild. They're tiny and only cover the format skeleton.SerializationFixtureGeneratorand carries the type-surface coverage: a column of every supported type includingCharacter,char[]and the array types, both time representations, timestamp and interval maps of every value type, graph attributes, views, edge types, mixed directedness, dynamic edge weights.This encodes the semver rule that a minor bump may change the byte format but older formats must stay readable.
Three contracts
Byte mismatches report the first differing offset and surrounding bytes, not just "arrays differ".
Graph attributes are now order-stable
GraphAttributesImplmoves fromHashMaptoTreeMapso serialized bytes are a function of content rather than insertion history — without this, byte-pinned fixtures would be fragile.TreeMaprejects null keys whereHashMapaccepted them, soObjects.requireNonNull(key, "key")is added to all nine public methods taking a key.Read path is unaffected and old files load identically; only the ordering of newly written attribute entries changes. No
VERSIONbump.Note on contract 2
It compares generator output to the committed file rather than re-serializing a model read back from one, because deserialization is not byte-idempotent in this codebase —
GraphVersioncounters andTimeIndexStore.countMapare restored from the stream and then incremented again as elements are re-inserted, so no fixture could ever be a fixed point. The read path is covered by contract 1's content assertions instead.Pre-existing issues surfaced, not fixed here
The fixtures pin current behaviour rather than papering over it. Found while building them:
TimeIndexStore.countMapdoubles on every load. A 4-node timestamped graph goes from[4, 2]to[8, 4]across a save/load cycle. Since it's a refcount released atif (--countMap[id] == 0), timestamps are never freed after a load.TextPropertieswidth/heightare written and read but then discarded —setTextPropertiescopies onlyrgba,size,text,visible.BigInteger[]/BigDecimal[]come back asObject[](values survive, component type doesn't).GraphViewStore.visibleViewisn't serialized at all.Testing
1627 tests pass, up from 1597. The byte-pin was verified to actually fail, by perturbing both a fixture model value and a serializer write order, then reverting.
🤖 Generated with Claude Code