fix(long-term): stop silent data loss in the entity write path - #256
Open
frigidom1024 wants to merge 1 commit into
Open
fix(long-term): stop silent data loss in the entity write path#256frigidom1024 wants to merge 1 commit into
frigidom1024 wants to merge 1 commit into
Conversation
Four defects where a call reported success while the graph did not hold what the caller was told it held. They compound: add_entity hands out an id that addresses no node, and add_relationship then acknowledges the write that id was used for, so the failure only surfaces as missing data much later. Fixes neo4j-labs#79. * aliases are written where the alias lookup does not read. add_entity stored them inside the JSON metadata blob while GET_ENTITY_BY_NAME reads a top-level `aliases` property — which is what MERGE_ENTITIES already wrote to target.aliases — so an entity was never findable by an alias passed to add_entity. `aliases` is now a top-level list property everywhere; ON MATCH appends only aliases not already present, in one statement, and _add_alias_to_entity drops its read-modify-write of the whole metadata blob. Rows written earlier still read back through a metadata fallback in _parse_entity. build_create_entity_query is public and callers do drive it with their own parameter dicts, so the new $aliases reference is opt-in (include_aliases=False, following the include_location precedent) — an unconditional one broke six existing tests with ParameterMissing. * add_entity returned an id that addressed no node. The query MERGEs on (name, type), so a repeat add hits ON MATCH, keeps the pre-existing node's id and discards the freshly minted one — which was returned anyway. The returned entity now adopts what the query stored. This is issue neo4j-labs#79's root cause. The report describes the returned id as a "ghost entity"; no second node is created, which is why the fix belongs in the return value and not in a caller-side name lookup. With it, the reporting bridge's existing add_entity/add_relationship sequence works unmodified. * add_relationship acknowledged a write that matched nothing. CREATE_ENTITY_RELATIONSHIP MATCHes both endpoints before MERGEing the edge, so ids addressing no node write zero rows and the method still returned a Relationship. It now raises NotFoundError naming both ids. Re-adding an existing edge also returned a freshly minted uuid and the arguments rather than the stored values; the query now projects r.id/r.description/r.confidence, because Result.data() flattens a bare RETURN r to a (start_props, type, end_props) tuple and drops the relationship's own properties. * merge_duplicate_entities orphaned most of the entity's edges. The docstring said relationships were transferred; the query carried subqueries for MENTIONS and SAME_AS only, so a merge silently dropped RELATED_TO (both directions), both provenance edges (EXTRACTED_FROM, EXTRACTED_BY) and the v0.2 APPLIES_TO / TOUCHED audit edges. All are now copied onto the surviving entity, each tagged migrated_from. Edges are copied rather than moved so the merge stays reversible. Bolt backend only. NAMS is unaffected: its add_relationship raises NotSupportedError, and its add_entity already follows `merged_into` and asserts the canonical id, so neo4j-labs#79's defect does not exist there. Tests: tests/unit/test_long_term_write_integrity.py (34) and tests/integration/test_long_term_write_integrity.py (22, including TestIssue79 reproducing the report's own sequence). Every defect here is invisible to a mocked client — the mechanism is a real MERGE/ON MATCH branch — and the integration suite caught two bugs in this patch, including the Result.data() flattening above. 12 of the 22 fail against the pre-fix code (8c29880) and all 22 pass against this commit; the other 10 assert behaviour that did not change. Co-Authored-By: Claude Code <noreply@anthropic.com>
frigidom1024
force-pushed
the
fix/long-term-silent-data-loss
branch
from
September 12, 2026 13:09
d808c23 to
dbf5e21
Compare
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #256 +/- ##
=======================================
Coverage ? 68.22%
=======================================
Files ? 140
Lines ? 13745
Branches ? 2024
=======================================
Hits ? 9377
Misses ? 3796
Partials ? 572
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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
2 participants
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.
Four defects where a call reported success while the graph did not hold what the caller was told it held. They compound: add_entity hands out an id that addresses no node, and add_relationship then acknowledges the write that id was used for, so the failure only surfaces as missing data much later.
Fixes #79.
aliases are written where the alias lookup does not read. add_entity stored them inside the JSON metadata blob while GET_ENTITY_BY_NAME reads a top-level
aliasesproperty — which is what MERGE_ENTITIES already wrote to target.aliases — so an entity was never findable by an alias passed to add_entity.aliasesis now a top-level list property everywhere; ON MATCH appends only aliases not already present, in one statement, and _add_alias_to_entity drops its read-modify-write of the whole metadata blob. Rows written earlier still read back through a metadata fallback in _parse_entity.build_create_entity_query is public and callers do drive it with their own parameter dicts, so the new $aliases reference is opt-in (include_aliases=False, following the include_location precedent) — an unconditional one broke six existing tests with ParameterMissing.
add_entity returned an id that addressed no node. The query MERGEs on (name, type), so a repeat add hits ON MATCH, keeps the pre-existing node's id and discards the freshly minted one — which was returned anyway. The returned entity now adopts what the query stored.
This is issue # Bug:
add_relationshipsilently creates no edges when target entity already exists #79's root cause. The report describes the returned id as a "ghost entity"; no second node is created, which is why the fix belongs in the return value and not in a caller-side name lookup. With it, the reporting bridge's existing add_entity/add_relationship sequence works unmodified.add_relationship acknowledged a write that matched nothing. CREATE_ENTITY_RELATIONSHIP MATCHes both endpoints before MERGEing the edge, so ids addressing no node write zero rows and the method still returned a Relationship. It now raises NotFoundError naming both ids. Re-adding an existing edge also returned a freshly minted uuid and the arguments rather than the stored values; the query now projects r.id/r.description/r.confidence, because Result.data() flattens a bare RETURN r to a (start_props, type, end_props) tuple and drops the relationship's own properties.
merge_duplicate_entities orphaned most of the entity's edges. The docstring said relationships were transferred; the query carried subqueries for MENTIONS and SAME_AS only, so a merge silently dropped RELATED_TO (both directions), both provenance edges (EXTRACTED_FROM, EXTRACTED_BY) and the v0.2 APPLIES_TO / TOUCHED audit edges. All are now copied onto the surviving entity, each tagged migrated_from. Edges are copied rather than moved so the merge stays reversible.
Bolt backend only. NAMS is unaffected: its add_relationship raises NotSupportedError, and its add_entity already follows
merged_intoand asserts the canonical id, so #79's defect does not exist there.Tests: tests/unit/test_long_term_write_integrity.py (34) and tests/integration/test_long_term_write_integrity.py (22, including TestIssue79 reproducing the report's own sequence). Every defect here is invisible to a mocked client — the mechanism is a real MERGE/ON MATCH branch — and the integration suite caught two bugs in this patch, including the Result.data() flattening above. 12 of the 22 fail against the pre-fix code (8c29880) and all 22 pass against this commit; the other 10 assert behaviour that did not change.