fix: Cypher -> WITH-carried node variable becomes null after anonymous CREATE or MERGE

[robfrank]

$(cat <<'EOF'

Summary

Fixes #4019.

Anonymous node patterns (no variable) in CREATE and MERGE used "n" as a hardcoded fallback variable name and stored the newly created vertex at that result-row key, silently overwriting any n already bound by an upstream MATCH/WITH.

• CreateStep.java (createPath, single-node branch): removed the "n" default; the created vertex is only bound to the row when the pattern has an explicit variable.
• MergeStep.java (mergeSingleNodeAll): same fix for both the match path and the create path.

Test plan

• Issue4019WithNodeVarNullAfterCreateMergeTest - 5 regression tests covering all cases from the issue report:
  • MATCH (n) WITH n CREATE (:Temp) - node var survives anonymous CREATE
  • MATCH (n) WITH n MERGE (:Temp) - node var survives anonymous MERGE
  • MATCH (n) WITH n, 1 AS x CREATE (:Temp) - both node var and scalar survive
  • scalar alias + CREATE (was already correct - control)
  • WITH n without any write (was already correct - control)
• OpenCypherCreateTest, OpenCypherMergeTest, OpenCypherMergeActionsTest, OpenCypherSetTest - no regressions

🤖 Generated with Claude Code
EOF
)

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric	Results
Complexity	0

View in Codacy

🟢 Coverage 100.00% diff coverage · -8.56% coverage variation

Metric	Results
Coverage variation	✅ -8.56% coverage variation
Diff coverage	✅ 100.00% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (bdbf985)	120168	88733	73.84%
Head commit (371efb9)	151425 (+31257)	98857 (+10124)	65.28% (-8.56%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#4029)	8	8	100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

_{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.}

[gemini-code-assist]

Code Review

This pull request addresses an issue where anonymous CREATE and MERGE operations in OpenCypher would incorrectly overwrite existing variables in the result set. The fix ensures that properties are only set on the result if a variable is explicitly defined in the node pattern. A new regression test suite has been added to verify the fix. Reviewer feedback suggests refactoring the code to use a local variable for the node pattern's variable name to improve readability and performance by avoiding redundant method calls.

[gemini-code-assist]

Capturing the variable name in a local variable improves readability and avoids redundant calls to nodePattern.getVariable(), especially since it is used multiple times throughout this method.

    final String variable = nodePattern.getVariable();
    // Only check for already-bound variable when explicitly named by the user.
    // Anonymous MERGE nodes (no variable) should always search for matches.
    if (variable != null) {
      final Object existing = baseResult.getProperty(variable);

[gemini-code-assist]

Use the local variable introduced earlier to avoid redundant getter calls inside the loop.

Suggested change

	if (nodePattern.getVariable() != null)
	r.setProperty(nodePattern.getVariable(), v);
	if (variable != null)
	r.setProperty(variable, v);

[gemini-code-assist]

Use the local variable introduced earlier.

Suggested change

	if (nodePattern.getVariable() != null)
	baseResult.setProperty(nodePattern.getVariable(), vertex);
	if (variable != null)
	baseResult.setProperty(variable, vertex);

[claude]

Code Review - PR #4029: Fix Cypher WITH-carried node variable becoming null after anonymous CREATE/MERGE

Overview

This PR fixes a real and well-understood bug: anonymous node patterns (no variable) in CREATE and MERGE were using "n" as a hardcoded fallback key and silently overwriting any pre-existing n binding carried from an upstream MATCH/WITH. The fix is correctly targeted and minimal.

---

Bug Fix Correctness - Looks Good

The core changes in both CreateStep.java and MergeStep.java are correct:

Before:

final String variable = nodePattern.getVariable() != null ? nodePattern.getVariable() : "n";
result.setProperty(variable, vertex);

After:

if (nodePattern.getVariable() != null)
  result.setProperty(nodePattern.getVariable(), vertex);

The hardcoded "n" fallback was always wrong - in Cypher, anonymous patterns have no variable binding by design, so nothing should be stored in the result row for them.

I also verified that:

• mergePathAll and its traversal helpers (traverseFromNode, findAllMatchingPaths) already had the correct guard (if (nodePattern.getVariable() != null) before every setProperty), so those were not affected.
• The path-based CREATE branch (the else in createPath) was already correct for the same reason.
• Only the two single-node branches (createPath simple case and mergeSingleNodeAll) had this bug - and both are fixed here.

---

Test Coverage

Five regression tests are a solid set and cover the meaningful variants. A few observations:

Missing schema pre-creation for Temp4019:
All existing OpenCypher tests in this suite (e.g., OpenCypherCreateTest, GAVEligibilityTest, CypherRelationDirectionTest) explicitly call database.getSchema().createVertexType(...) in @BeforeEach for every label used in queries. The new test relies on ArcadeDB auto-creating the Temp4019 type when Cypher executes CREATE (:Temp4019 ...) or MERGE (:Temp4019 ...).

This may work in practice, but it is inconsistent with the established test pattern and could be fragile if that auto-creation behavior ever changes. Consider adding:

database.getSchema().createVertexType("Temp4019");

to @BeforeEach.

Lambda workaround:
The ResultSet[] ref = new ResultSet[1] trick is used in four of the five tests to capture a value inside a database.transaction() lambda. This is a common Java workaround for the effectively-final constraint, but withoutCreateNodeVarIsCorrect avoids it naturally by using database.query() directly (no write, no transaction needed). This inconsistency is minor but worth noting - the workaround could alternatively be avoided by returning the result directly from the transaction lambda if the API supports it, or extracting the rows collection inside the transaction block.

Control tests are valuable:
The inclusion of scalarAliasAloneStillWorksWithCreate and withoutCreateNodeVarIsCorrect as control/baseline tests is good practice to confirm the fix doesn't regress already-working cases.

---

Code Style

The fix follows the project's conventions correctly:

• Single-statement if without curly braces (per CLAUDE.md: "if statements with only one child sub-statement don't require a curly brace open/close")
• No unnecessary comments added
• AssertJ assertions used in tests (assertThat(...).isEqualTo(...))

---

Summary

Area	Status
Bug fix correctness	Correct and well-targeted
Scope of fix	Appropriate - only the two affected branches
Test coverage	Good - 5 focused regression tests
Schema pre-creation in tests	Minor gap - Temp4019 not explicitly created in @BeforeEach
Code style	Follows project conventions
Performance	No concern - removes work, adds nothing
Security	No concern

The fix itself is solid. The only actionable suggestion is to add database.getSchema().createVertexType("Temp4019") to @BeforeEach for consistency with the rest of the test suite.

🤖 Generated with Claude Code

[claude]

Code Review

Overview

This PR fixes a correctness bug where anonymous node patterns (no variable) in CREATE and MERGE used "n" as a hardcoded fallback variable name, silently overwriting any n already bound by an upstream MATCH/WITH. The fix is minimal and targeted: remove the fallback and only bind the vertex to the result row when an explicit variable is present.

---

Correctness - the fix is right

CreateStep.java: The single-node branch now correctly skips result.setProperty(...) when the pattern is anonymous. The path-with-relationships branch (else) already had if (nodePattern.getVariable() != null) guards (lines 303-315), so that code was not affected.

MergeStep.java: All three binding points are fixed:

• Early-exit for pre-bound variables (unchanged - still correctly gated on variable != null)
• Match loop: r.setProperty(variable, v) now guarded
• Create path: baseResult.setProperty(variable, vertex) now guarded

The internal " wasCreated" flag and the ON CREATE SET/ON MATCH SET dispatch are not affected by the variable being null - those reference variables from the SET clause items, not from the MERGE node pattern.

One readability note: In mergeSingleNodeAll, variable is extracted once at the top, but then checked with if (variable != null) in two separate places further down. A future reader has to keep in mind it cannot change between those checks. This is fine, but it might be cleaner to inline nodePattern.getVariable() at the guard sites (matching the style of the early-exit block that was already present). Very minor point.

---

Test Coverage

The 5 regression tests cover the core cases well. A couple of gaps worth considering:

1. Missing: scalar alias + anonymous MERGE - There is scalarAliasAloneStillWorksWithCreate but no equivalent for MERGE. Given the fix touches both steps, a mirror test would complete the matrix.

2. Missing: ON CREATE SET on anonymous MERGE - A query like MATCH (n:Person4019) WITH n MERGE (:Temp4019 {x:10}) ON CREATE SET n.flag = true RETURN n.name, n.flag would verify that ON CREATE SET (referencing a different variable from the anonymous MERGE node) still works after the fix.

3. No assertion on side-effect count - nodeVarCarriedThroughWithMustSurviveAnonymousMerge verifies n is preserved but does not assert how many Temp4019 nodes were created. That's acceptable for a regression test focused on variable binding, but documenting the expected count in a comment would help future readers understand intent.

---

Style

The test class has a multi-paragraph Javadoc block comment (lines 38-44). Per project guidelines, comment blocks should be one short line at most. A single-sentence class-level comment (or none at all, since the class name is descriptive) would be preferred.

---

Summary

The fix is correct, clean, and well-targeted. The two missing test cases (scalar alias + MERGE, and ON CREATE SET with anonymous node) are worth adding for completeness, but they do not affect the correctness of the fix itself. The existing tests cover the reported issue thoroughly.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.25%. Comparing base (bdbf985) to head (371efb9).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4029      +/-   ##
==========================================
- Coverage   64.83%   64.25%   -0.59%     
==========================================
  Files        1597     1597              
  Lines      120168   120170       +2     
  Branches    25587    25589       +2     
==========================================
- Hits        77916    77219     -697     
- Misses      31466    32280     +814     
+ Partials    10786    10671     -115     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.