fix: Cypher -> SET n:Label on same node across row fanout must be idempotent - #4025

Up to standards ✅

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

Code Review - PR #4025: Cypher SET n:Label idempotency on row fanout

Overview

This PR fixes a real crash bug (issue #4017) where SET n:Label on nodes that appear multiple times in a cartesian row fanout would fail because the first row deleted and recreated the vertex with a new RID, leaving subsequent rows holding a stale (deleted) reference. The fix adds a labelReplacements: Map<RID, Vertex> map alongside the existing writtenDocs map to redirect stale references to the already-replaced vertex.

The approach is sound and consistent with the existing writtenDocs pattern. The fix is minimal and targeted.

Correctness

Looks good:

• originalRid is captured before the redirect, so labelReplacements.put(originalRid, newVertex) always maps from the original RID to the latest replacement, even if the vertex was already replaced once (chained replacement via row 3+).
• propagateUpdateToSameNodeAliases is correctly called with the (now-replaced) vertex so all aliases in the current result row are updated.
• The early-return idempotency check uses the redirected vertex (the new one), so its getTypeName() is current.

Potential issue - interaction between writtenDocs and labelReplacements:

If a SET clause combines a property assignment and a label assignment on the same variable in that order - e.g. SET n.role = 'emp', n:Employee - and the same node appears on multiple rows:

• Row 1: applyPropertySet saves RID1 into writtenDocs[RID1]. Then applyLabels deletes RID1, creates RID2, records labelReplacements[RID1] = RID2.
• Row 2: applyPropertySet calls resolveLatestDoc, finds RID1 in writtenDocs, updates the result to point to the deleted mutableRID1, then attempts mutableDoc.save() on it - which would likely fail.

The PR description only covers pure label-fanout. If the combined case is also broken before this PR, that should be noted; if it was working before, it now regresses. A test or explicit exclusion comment would help clarify this. The safest fix would be to remove writtenDocs[RID1] (or insert writtenDocs[RID2]) after a label replacement, but this may be out of scope for this PR.

Test Coverage

Good:

• setLabelIdempotentOnSameNodeAcrossRowFanout - exact reproducer.
• setMultipleLabelsIdempotentOnSameNodeAcrossRowFanout - multi-label variant.
• Both tests assert on the full ordered result, not just absence of exception.

Minor - inaccurate javadoc comment in test:

// The cartesian MATCH produces Alice twice and Charlie twice (age > peer)

Alice only qualifies once (age 30 > Bob's 25). The cartesian product yields the pairs (Alice, Bob), (Charlie, Bob), (Charlie, Alice) - so Alice appears once and Charlie twice. The assertion containsExactly("Alice", "Charlie", "Charlie") is correct; only the comment is wrong.

Missing test - edge preservation under fanout:
The existing tests check that the replaced vertex has the right name but do not verify that edges are preserved correctly after the label replacement. A test with connected vertices (e.g., a Person with a WORKS_AT edge) would give stronger confidence that the delete+copy-edges+create flow is correct for the fanout case specifically.

Code Quality

Comment cleanup is good: The old comment explaining why label SET was not covered by writtenDocs has been removed cleanly, and the new field comment explains the new mechanism concisely.

Consistent with existing style: The new map follows the same anonymous-ResultSet-field pattern as writtenDocs. The final keyword is used correctly. No unnecessary abstraction added.

Minor - applyLabels signature is now long:

private void applyLabels(final SetClause.SetItem item, final Result result,
    final Map<RID, Vertex> labelReplacements) {

The two-line signature is fine given the project's printWidth: 160 style. No action needed.

Performance

The HashMap lookup/put is O(1) and only executed on the LABELS branch. No regression for the common single-row case. For very large fanouts with many distinct label-changed nodes the map grows, but this mirrors the writtenDocs pattern already accepted in the codebase.

Summary

• Bug fix is correct for the stated scenario (pure label SET + row fanout).
• One potential concern around the combined property+label SET order interaction; worth a comment or test to clarify whether it is in scope.
• One inaccurate javadoc comment in the first test (Alice appears once, not twice).
• Edge-preservation test under the fanout case would strengthen confidence.

Overall this is a solid, targeted fix. Recommend addressing the test comment and considering a note on the writtenDocs/labelReplacements interaction before merging.

🤖 Generated with Claude Code

Code Review - fix: Cypher SET n:Label idempotency across row fanout

Overview

This PR fixes a real crash: SET n:Label on a cartesian-MATCH result set deleted a vertex on row 1, then row 2 tried to operate on the same (now-deleted) vertex and threw Record not found or Duplicated key. The fix introduces a labelReplacements: Map<RID, Vertex> in the ResultSet anonymous class (mirroring the existing writtenDocs pattern) to redirect later rows to the already-replaced vertex.

The approach is sound and follows existing conventions well.

Correctness

The core fix is correct. When prior != null:

1. propagateUpdateToSameNodeAliases updates the current result row's aliases to point to the replacement.
2. vertex = prior redirects the local reference.
3. The type-equality check vertex.getTypeName().equals(newTypeName) short-circuits because prior already carries the new type - clean idempotency.

One subtle concern - writtenDocs not seeded with the new vertex RID: After newVertex.save(), the new vertex is not added to writtenDocs under its new RID. If a subsequent row in the same fanout does a property SET on the (now-replaced) vertex, resolveLatestDoc won't find it in writtenDocs via the new RID. The result alias was updated by propagateUpdateToSameNodeAliases, so the property SET would still operate on newVertex - but the cross-row accumulation for the new RID is untracked. Low risk in practice but worth considering:

// After newVertex.save() and before vertex.delete():
writtenDocs.put(newVertex.getIdentity(), newVertex.modify()); // optional, for combined SET n.prop+n:Label safety

Pre-existing Issue (not introduced here, but surfaced)

Edge properties are silently dropped during label replacement. Lines 354-357 copy edge topology but not edge properties:

for (final Edge edge : vertex.getEdges(Vertex.DIRECTION.OUT))
    newVertex.newEdge(edge.getTypeName(), edge.getVertex(Vertex.DIRECTION.IN));

Any properties stored on the original edges are lost. This existed before this PR, but the new fanout test (setLabelPreservesEdgesAcrossRowFanout) only verifies that the edge exists, not that its properties survive. A follow-up issue/test would be useful.

Minor Observations

allLabels.contains(label) is O(n) on a List (line 336-337). Label lists are tiny in practice so this is harmless, but a LinkedHashSet would be idiomatic for deduplication while preserving insertion order:

final Set<String> allLabelsSet = new LinkedHashSet<>(existingLabels);
allLabelsSet.addAll(item.getLabels());
final List<String> allLabels = new ArrayList<>(allLabelsSet);

vertex = prior reassignment after pattern-match variable (line 329). Reassigning a variable introduced by instanceof pattern matching is valid Java 16+ but slightly surprising to read. Consider a shadowing rename for clarity - though this is purely stylistic and doesn't affect correctness.

Old comment removed appropriately. The previous comment saying "Label SET is intentionally not covered by the writtenDocs pattern" is gone - good, since the fix now does interact with writtenDocs (via writtenDocs.remove(originalRid)).

Test Coverage

Three new tests - good coverage:

• setLabelIdempotentOnSameNodeAcrossRowFanout - single label, cartesian MATCH (direct reproducer)
• setMultipleLabelsIdempotentOnSameNodeAcrossRowFanout - multiple labels in one SET
• setLabelPreservesEdgesAcrossRowFanout - verifies edges survive the fanout path

The acknowledged gap (combined SET n.prop = v, n:Label across fanout) doesn't need to be fixed now, but tracking it in a follow-up issue would be helpful so it isn't forgotten.

Summary

The fix is well-targeted, follows existing patterns, is well-tested, and resolves the crash. The edge-property loss is pre-existing and out of scope. The writtenDocs seeding note above is the only functional improvement worth considering before merge; everything else is minor polish.

Verdict: Approve with optional minor suggestions.

Code Review - PR #4025: fix: Cypher SET n:Label idempotency on row fanout

Overview

This PR correctly fixes a crash bug (#4017): during a cartesian MATCH, SET n:Label deleted+recreated the vertex on row 1 with a new RID; row 2 still held the old (deleted) reference and threw Record not found or Duplicated key. The fix adds a labelReplacements: Map<RID, Vertex> alongside the existing writtenDocs map to redirect later rows to the already-replaced vertex.

The approach mirrors the existing writtenDocs pattern well.

Issues

1. Duplicated chain-traversal logic (minor refactor suggestion)

The same "walk the replacement chain to its head" loop appears verbatim in two places:

• applySetOperations (pre-resolution pass)
• applyLabels (local resolution)

Extracting it avoids future drift:

private Vertex resolveLatestVertex(final Vertex vertex, final Map<RID, Vertex> replacements) {
  Vertex current = vertex;
  Vertex next;
  while ((next = replacements.get(current.getIdentity())) != null)
    current = next;
  return current;
}

2. New vertex RID not seeded into writtenDocs (low-risk gap)

After newVertex.save(), writtenDocs only has the old RID removed (writtenDocs.remove(originalRid)). If a combined SET n.prop = v, n:Label query hits the same fanout, the new vertex RID isn't in writtenDocs, so the cross-row accumulation path won't find it. The alias update via propagateUpdateToSameNodeAliases makes property SET still land on the right object, but it is untracked. The existing comment acknowledges ordering-dependent behaviour here - consider a follow-up issue to track it.

3. Test javadoc comment is inaccurate

In setLabelIdempotentOnSameNodeAcrossRowFanout:

// Alice qualifies for one pair (n=Alice > m=Bob), Charlie for two pairs ...
// Expected rows: Alice, Charlie, Charlie.

Alice's cartesian result is one row, Charlie's is two (Charlie > Alice, Charlie > Bob). The comment says "Alice appears once" which is correct, but the assertThat(names).containsExactly("Alice", "Charlie", "Charlie") assertion has Alice first - the comment should clarify the ORDER BY guarantees that ordering (Alice < Charlie alphabetically), not that Alice appears fewer times. Minor wording fix.

4. Pre-existing issue surfaced by new test: edge properties are silently dropped

setLabelPreservesEdgesAcrossRowFanout verifies the edge exists but not that edge properties survive. The edge copy loop in applyLabels:

for (final Edge edge : vertex.getEdges(Vertex.DIRECTION.OUT))
    newVertex.newEdge(edge.getTypeName(), edge.getVertex(Vertex.DIRECTION.IN));

...copies topology only - any properties on edges are lost. This is pre-existing (out of scope here), but worth a follow-up issue.

Positives

• Three well-targeted tests covering the exact reproducer, the multi-label variant, and edge preservation.
• The writtenDocs.remove(originalRid) call is the right defensive move - prevents stale MutableDocument reads.
• The idempotency short-circuit via vertex.getTypeName().equals(newTypeName) is clean and correct.
• Old misleading comment ("Label SET is intentionally not covered by the writtenDocs pattern") is properly removed.

Summary

The fix is correct, well-tested, and follows existing patterns. The main actionable items before merge:

1. Must fix: Correct the inaccurate javadoc comment in the first test.
2. Nice to have: Extract the chain-traversal into a helper to avoid duplication.
3. Optional: Open follow-up issues for (a) combined SET prop+label fanout ordering and (b) edge property loss on label replacement.

Verdict: Approve with minor suggestions.

Reviewed with Claude Code

Codecov Report

❌ Patch coverage is 63.63636% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.24%. Comparing base (9a5b0e1) to head (48d049a).
⚠️ Report is 4 commits behind head on main.

@@            Coverage Diff             @@
##             main    #4025      +/-   ##
==========================================
- Coverage   64.91%   64.24%   -0.67%     
==========================================
  Files        1597     1597              
  Lines      120132   120168      +36     
  Branches    25580    25587       +7     
==========================================
- Hits        77981    77204     -777     
- Misses      31377    32296     +919     
+ Partials    10774    10668     -106     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.