test(#4689): regression tests for MATCH (u:User) RETURN u NoSuchElementException

[robfrank]

Summary

• Adds regression test Issue4689MatchReturnVertexIT for issue java.util.NoSuchElementException #4689, which reports MATCH (u:User) RETURN u throwing java.util.NoSuchElementException via HTTP, while MATCH (u:User) RETURN u{.*} as user works
• Covers 7 scenarios: whole-vertex return, projection workaround, SQL SELECT FROM, SQL field selection, >100 vertices (pagination boundary), vertices with edges, and the default (non-studio) HTTP serializer path
• Extensive investigation traced the error path through FinalProjectionStep, NodeByLabelScan, JsonSerializer, and the HTTP serialization layer; all code paths appear correct and all 7 tests pass on the current codebase

Closes #4689

The investigation found a structural inconsistency in FinalProjectionStep.filterResult(): when a single Document property is returned, the result has both element=vertex AND content={"u":vertex} set simultaneously. This creates getPropertyNames() vs getProperty() inconsistency but does not cause NSE in the current serialization path (which uses the _projectionName early-return that calls document.toMap() directly). The regression tests provide coverage so any future regression is caught immediately.

Test plan

• Run mvn test -pl server -Dtest=Issue4689MatchReturnVertexIT - should show 7 tests passing
• Verify MATCH (u:User) RETURN u returns correct vertex data via HTTP
• Verify SELECT FROM <type> returns correct records via HTTP
• Verify the default (non-studio) serializer path returns HTTP 200 for vertex queries

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Coverage 96.67% diff coverage · -8.13% coverage variation

Metric	Results
Coverage variation	✅ -8.13% coverage variation
Diff coverage	✅ 96.67% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (2ba493d)	131866	99084	75.14%
Head commit (881ba78)	163732 (+31866)	109710 (+10626)	67.01% (-8.13%)

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 (#4690)	30	29	96.67%

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 adds documentation analyzing a reported NoSuchElementException when returning whole vertices in Cypher and SQL, along with a new regression test suite (Issue4689MatchReturnVertexIT) to verify these query paths. The review feedback highlights critical improvements for the tests: explicitly creating and populating vertex types in the SQL tests to ensure they actually exercise the serialization paths, and dynamically resolving the HTTP server URL and database name using base class helper methods to prevent test flakiness.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

The test queries V1, but V1 is not created or populated within this test. If V1 is empty or does not exist, the query will either fail with a schema error or return an empty result set. If it returns an empty result set, the vertex serialization code path is never executed, meaning this regression test would pass vacuously without actually verifying that the NoSuchElementException is resolved.

To ensure the test is robust, self-contained, and actually exercises the serialization path, we should explicitly create a vertex type, insert a record, and query that type.

  void sqlSelectFromTypeShouldNotThrow() throws Exception {
    executeCommand(0, "sql", "CREATE VERTEX TYPE SqlUser");
    executeCommand(0, "sql", "INSERT INTO SqlUser SET name = 'Alice', age = 30");

    // SQL: returning whole record should work (same issue with SQL as with Cypher)
    final JSONObject response = executeCommand(0, "sql", "SELECT FROM SqlUser");

    assertThat(response).isNotNull();
    assertThat(response.has("result")).as("SELECT FROM should not throw NoSuchElementException").isTrue();
  }

[robfrank]

Fixed in the current commit. sqlSelectFromTypeShouldNotThrow now creates its own SqlSelectAll vertex type and populates it with 2 records before querying - no dependency on V1.

[gemini-code-assist]

Similar to the previous test, querying V1 without creating it or inserting records makes the test dependent on external state or run vacuously on an empty result set.

To ensure test isolation and correctness, explicitly create a vertex type, insert a record, and query it.

  void sqlSelectFieldsShouldWork() throws Exception {
    executeCommand(0, "sql", "CREATE VERTEX TYPE SqlUser2");
    executeCommand(0, "sql", "INSERT INTO SqlUser2 SET name = 'Bob', age = 25");

    // Selecting individual fields should work (workaround for SQL)
    final JSONObject response = executeCommand(0, "sql", "SELECT name FROM SqlUser2");

    assertThat(response).isNotNull();
    assertThat(response.has("result")).isTrue();
  }

[robfrank]

Fixed in the current commit. sqlSelectFieldsShouldWork now creates its own SqlSelectFields vertex type with 1 record - no dependency on V1.

[gemini-code-assist]

Hardcoding the host, port (127.0.0.1:2480), and database name (graph) can lead to test flakiness or failures if the test server is configured to run on a dynamic port (e.g., in parallel execution environments or CI pipelines) or if the database name is different.

We should use the helper methods getHttpServerUrl(0) and getDatabase(0).getName() provided by BaseGraphServerTest to dynamically resolve the correct URL.

Suggested change

	final java.net.HttpURLConnection connection = (java.net.HttpURLConnection) new java.net.URI(
	"http://127.0.0.1:2480/api/v1/command/graph").toURL().openConnection();
	final java.net.HttpURLConnection connection = (java.net.HttpURLConnection) new java.net.URI(
	getHttpServerUrl(0) + "/api/v1/command/" + getDatabase(0).getName()).toURL().openConnection();

[robfrank]

Updated to use getDatabaseName() for the database name portion. The port pattern "http://127.0.0.1:2480" matches the existing convention in BaseGraphServerTest.executeCommand() (which uses "http://127.0.0.1:248" + serverIndex). No getHttpServerUrl() helper exists in the base class.

[claude]

Code Review

Overview

This PR adds regression tests for issue #4689 (MATCH (u:User) RETURN u throwing NoSuchElementException via HTTP). The investigation found the bug cannot be reproduced on the current codebase, so only regression coverage is added. It also commits a markdown investigation document.

---

Issues

Critical

Misleading PR title and branch name
The branch is fix/4689-... and the title says fix(#4689), but nothing is actually fixed - the bug couldn't be reproduced. This should be titled something like test(#4689): add regression tests for MATCH RETURN vertex via HTTP. Merging this as a "fix" will mislead future readers of the git history.

Unaddressed structural inconsistency
The PR itself documents a real latent bug in FinalProjectionStep.filterResult(): when RETURN u returns a single Document, the result has both element=vertex AND content={"u":vertex} set, making getPropertyNames() and getProperty() inconsistent. The current serialization path happens to avoid it via the _projectionName early-return, but this is fragile. A follow-up issue/TODO should be filed so this isn't lost.

---

Correctness

Test isolation: cross-test state dependency
cypherMatchReturnVertexProjectionWorkaround() queries IssueUser vertices but creates none itself - it relies on state left by cypherMatchReturnWholeVertexShouldNotThrow(). JUnit 5 does not guarantee test method ordering by default. If the projection test runs first, it returns an empty result set and still passes (because assertThat(response.has("result")) is true even with zero results). Each test should set up its own data, or the class should share setup via @BeforeEach/populateDatabase() override.

Weak assertions - empty results pass the tests
All 7 tests only check response.has("result"), not what is inside it. The original failure was HTTP 500 (no result key), so the null-check on response would catch a regression - but these tests would also pass if the queries silently returned 0 rows due to a data-setup bug. At minimum, assert on result count:

// After executing the MATCH query that should return 2 vertices:
assertThat(response.getJSONArray("result").length()).isEqualTo(2);

---

Style (per CLAUDE.md: "don't use fully qualified names if possible, always import the class")

cypherMatchReturnVertexWithDefaultSerializer() uses raw fully-qualified names throughout:

// Current - violates project conventions
final java.net.HttpURLConnection connection = (java.net.HttpURLConnection) new java.net.URI(...)
    .toURL().openConnection();
connection.setRequestProperty("Authorization",
    "Basic " + java.util.Base64.getEncoder().encodeToString(...));
formatPayload(connection, "opencypher", ..., null, java.util.Collections.emptyMap());

Should import HttpURLConnection, URI, Base64, Collections (same as BaseGraphServerTest already does).

Also note: this test duplicates the pattern already handled by executeCommand(). Consider using the inherited method and passing null for the serializer argument (if supported) rather than duplicating the raw HTTP connection logic.

---

Performance tagging (per CLAUDE.md)

cypherMatchReturnManyVerticesShouldWork() creates 110 vertices via 110 separate HTTP round-trips in a loop. This is noticeably slow and should be annotated:

@Test
@Tag("slow")
void cypherMatchReturnManyVerticesShouldWork() throws Exception {

Required import: import org.junit.jupiter.api.Tag;

---

Committed documentation file

docs/4689-cypher-match-return-vertex.md is an investigation log, not project documentation. Investigation notes belong in the PR description (where they already appear), not in the committed codebase. Committing per-issue investigation files into docs/ creates permanent noise in the tree. I'd suggest dropping this file from the PR.

---

Summary

Area	Finding
PR framing	Should be test(#4689), not fix(#4689)
Latent bug	FinalProjectionStep dual element+content state is undocumented and unfiled
Test isolation	Cross-test state dependency via shared IssueUser type
Assertions	Checking has("result") is too weak; assert actual result counts
Style	Fully qualified names in DefaultSerializer test
Tagging	110-vertex loop test needs @Tag("slow")
Docs file	Investigation note should not be committed to repo

The test idea is sound and the coverage is valuable. With the assertion strength improved, isolation fixed, style corrected, and the doc file removed, this would be a clean regression safety net.

[claude]

Code Review: PR #4689 - Regression tests for MATCH (u:User) RETURN u

Overview

This PR adds 7 regression tests for issue #4689 (Cypher MATCH ... RETURN u throwing NoSuchElementException via HTTP) plus an investigation document in docs/. Since the bug could not be reproduced on the current codebase, the contribution is purely test coverage to catch a future regression.

The tests are generally well-structured and follow project conventions. A few issues worth addressing before merge:

---

Issues

Critical - Incomplete assertion in the pagination boundary test

cypherMatchReturnManyVerticesShouldWork is the only test exercising the >100-vertex path (the stated reason for using 110 vertices), yet it only asserts:

assertThat(response.has("result")).isTrue();

This assertion would pass even if only 1 vertex were returned, making it no test at all for pagination. It should assert the full count:

final JSONObject result = response.getJSONObject("result");
assertThat(result.getJSONArray("records").length())
    .as("Should return all 110 vertices across pagination batches")
    .isEqualTo(110);

Similarly, cypherMatchReturnVertexWithEdgesShouldWork asserts the vertex count but the test comment says "edge counting runs in setMetadata()" - it would be stronger to also validate that at least one vertex's @in or @out edge count is non-zero.

---

Minor - Multi-line Javadoc violates project conventions

Per CLAUDE.md: "Never write multi-paragraph docstrings or multi-line comment blocks - one short line max."

The class-level Javadoc is three lines. Trim to a single line, e.g.:

/** Regression test for issue #4689: MATCH (u:IssueUser) RETURN u must not throw NSE via HTTP. */
class Issue4689MatchReturnVertexIT extends BaseGraphServerTest {

---

Minor - Documentation file in docs/

CLAUDE.md: "NEVER create documentation files (.md) or README files unless explicitly requested by the User."*

The investigation memo docs/4689-cypher-match-return-vertex.md is detailed and useful for context, but it belongs in the PR description or a GitHub comment rather than the repo tree. The PR description already captures the key conclusions. Suggest removing it from the committed diff.

---

Nit - 110 individual HTTP calls for bulk insert

cypherMatchReturnManyVerticesShouldWork issues 110 separate HTTP round-trips to create vertices. Using UNWIND would be much faster and wouldn't need the @Tag("slow"):

UNWIND range(0, 109) AS i CREATE (:ManyUsers {idx: i, name: 'User' + toString(i)})

The slow tag is a valid workaround, but this is worth noting.

---

Nit - Descriptive comments add noise

Several inline comments describe what the code does rather than why (e.g., // Each test creates its own isolated type to avoid ordering dependencies). CLAUDE.md reserves comments for non-obvious WHY. The type-naming strategy is self-evident from the test structure.

---

What is done well

• Each test uses its own uniquely named type, preventing cross-test pollution without needing @AfterEach cleanup (the base class recreates the DB from scratch before each test via deleteDatabaseFolders()).
• Response format is parsed correctly: studio-serializer tests use response.getJSONObject("result").getJSONArray("records") while the direct-HTTP default-serializer test uses response.getJSONArray("result") - these match what AbstractQueryHandler.serializeResultSet() actually produces.
• @Tag("slow") is applied at method level only on the slow test, not the whole class - correct per CLAUDE.md.
• The IT suffix and package location follow existing conventions.
• AssertJ assertions with descriptive messages throughout.

---

Summary

The primary ask before merge is to fix the incomplete assertion in cypherMatchReturnManyVerticesShouldWork - it tests nothing meaningful about pagination as written. The doc file and Javadoc verbosity are lower-priority but worth a quick cleanup.

[claude]

Code Review: PR #4689 Regression Tests

Overview

This PR adds 7 regression tests for issue #4689 (MATCH (u:IssueUser) RETURN u throwing NoSuchElementException via HTTP) along with a detailed investigation document in docs/. The root cause was not reproduced, but the tests serve as a coverage baseline that will catch any future regression.

Issues / Suggestions

1. Missing @Tag("slow") on the 110-vertex test

Per project conventions, functional tests with large batches must be annotated. cypherMatchReturnManyVerticesShouldWork creates 110 vertices and is explicitly testing a pagination boundary - that qualifies. Apply at the method level since only this one method is slow:

@Tag("slow")
@Test
void cypherMatchReturnManyVerticesShouldWork() throws Exception { ... }

Required import: import org.junit.jupiter.api.Tag;

2. Deprecated new URL(String) constructor (Java 21 target)

In cypherMatchReturnVertexWithDefaultSerializer, the test uses the deprecated constructor. Since the project targets Java 21+, this should match the modern pattern already used in Issue4148ArrayOfLongsHttpIT:

// Instead of: new URL("http://127.0.0.1:2480/api/v1/command/" + getDatabaseName())
new URI("http://127.0.0.1:2480/api/v1/command/" + getDatabaseName()).toURL()

Add import: import java.net.URI;

3. Tests only verify record count, not content

The tests confirm the right number of records is returned but do not verify the actual field data. Since the bug was an NSE during serialization, asserting that returned records contain expected fields would strengthen these tests considerably:

final JSONArray records = result.getJSONArray("records");
assertThat(records.length()).isEqualTo(2);
// Confirm vertex properties are actually serialized, not just empty objects
final JSONObject first = records.getJSONObject(0);
assertThat(first.has("name")).as("Vertex properties must be serialized").isTrue();

Without this, a response of [{}, {}] would still pass all tests.

4. Test isolation may be fragile on retries

Each test creates vertices with unique type names (IssueUser, ManyUsers, etc.), which avoids inter-test conflicts in a single run. However, repeated test runs or CI retries leave data behind since there is no cleanup. The count assertion in cypherMatchReturnWholeVertexShouldNotThrow would fail with count > 2 on a retry. Consider a @BeforeEach that drops/recreates the types, or use isGreaterThanOrEqualTo for count assertions.

5. The docs/ Markdown file may go stale quickly

docs/4689-cypher-match-return-vertex.md references specific code paths and line numbers that will drift as the codebase evolves. The existing docs/ pattern seems to be for per-issue analysis, so this is reasonable - just be aware that future readers may find the references misleading as code changes.

Positives

• Good coverage of both the failing (RETURN u) and working workaround (RETURN u{.*}) paths
• The last test correctly exercises the non-studio default serializer, which is a meaningfully different code path
• The edge-count assertion in cypherMatchReturnVertexWithEdgesShouldWork is clever and validates the setMetadata path
• Correct use of assertThat(...).as("message") throughout for descriptive failure output
• Clean use of unique type names per test method to avoid most cross-test pollution

Summary

Top three actionable items before merge:

1. Add @Tag("slow") to cypherMatchReturnManyVerticesShouldWork
2. Replace deprecated new URL(String) with new URI(...).toURL()
3. Add at least one property-level assertion per test to confirm data is actually serialized, not just counted

[claude]

Code Review - Issue #4689 Regression Tests

Overview

This PR adds 7 regression tests for issue #4689 (MATCH (u:User) RETURN u throwing NoSuchElementException via HTTP) plus an investigation tracking doc. The issue was not reproducible on the current codebase, so this is a pure safety net rather than a fix. PR title accurately reflects this with the test() prefix.

---

Test File - Issue4689MatchReturnVertexIT.java

Positives

• Clean use of unique vertex/edge type names per test (IssueUser, IssueUserProj, SqlSelectAll, etc.) avoids cross-test type collisions.
• AssertJ is used throughout and assertion messages are descriptive.
• Test 7 (cypherMatchReturnVertexWithDefaultSerializer) correctly exercises the non-studio serializer path in isolation using a raw HttpURLConnection, which is exactly what is needed to replicate the original bug report.
• Using UNWIND range(0, 109) for bulk vertex creation in the pagination-boundary test is the right call.

Issues

Medium - Fragile dependency on base-class schema in cypherMatchReturnVertexWithEdgesShouldWork

The edge creation command relies on E1 already existing because BaseGraphServerTest.populateDatabase() creates it. If populateDatabase() ever stops creating E1, or if this test is extracted into a standalone class, this silently breaks. Prefer making the test self-contained:

executeCommand(0, "sql", "CREATE EDGE TYPE IF NOT EXISTS TestEdge");
executeCommand(0, "sql",
    "CREATE EDGE TestEdge FROM (SELECT FROM UserEdgeTest WHERE name = 'Src') TO (SELECT FROM UserEdgeTest WHERE name = 'Dst')");

Medium - Unexplained response-structure divergence in test 7

Tests 1-6 (studio serializer) parse as response.getJSONObject("result").getJSONArray("records"), while test 7 (default serializer) parses as response.getJSONArray("result"). This is intentional but a one-line comment would save the next reader from having to rediscover it:

// Default serializer returns {"result": [...]} directly; studio wraps as {"result": {"records": [...]}}

Low - Null-response failure message is opaque

When executeCommand catches an HTTP 500 internally it returns null. The current pattern:

assertThat(response).isNotNull();
assertThat(response.has("result")).as("Response should have 'result' key ...").isTrue();

means a 500 surfaces as "expected not null" rather than the helpful message on line 2. Move the description to the null check:

assertThat(response).as("MATCH RETURN u must not throw - server returned null (likely HTTP 500)").isNotNull();

Low - @Tag("slow") consideration on pagination test

cypherMatchReturnManyVerticesShouldWork creates 110 vertices and does a full scan via HTTP. The PR removed @Tag("slow") after replacing the loop with UNWIND - fair, but 110-record full-scan HTTP queries are heavier than the other tests. CLAUDE.md says to tag @Tag("slow") for "large batches". Whether 110 records crosses that bar is debatable, but worth a deliberate call.

---

Docs File - docs/4689-cypher-match-return-vertex.md

This file contains AI review-cycle tracking notes referencing specific reviewer names ("Claude (CORRECTNESS): ...", "Gemini (HIGH x2): ...") and internal process artifacts (commit SHAs, "clean-approval after 3 cycles"). Removal was flagged in review cycle 1 and deferred citing precedent from docs/4274-* files.

That precedent is valid, but those files document technical investigations (WAL bugs, compression details). This one documents the review process itself, which will be stale within weeks. Recommend either removing it entirely or trimming to just the technical analysis ("Summary" through "Conclusion") and dropping the "Review Cycles" section. Minor point - the tests are what matter.

---

Latent Issue (Informational)

The PR description correctly identifies the FinalProjectionStep.filterResult() dual element+content state as a latent fragility. Recommend opening a follow-up GitHub issue to track it before this PR merges.

---

Summary

Area	Verdict
Test correctness	Pass - all 7 scenarios cover the stated issue
Test isolation	Pass - fresh DB per test via @BeforeEach in base class
Assertions	Good - descriptive messages, AssertJ throughout
Schema dependency	E1 coupling in edge test is fragile - easy to fix
Response format divergence	Needs a comment in test 7
Docs file	Consider trimming or removing review-cycle section

The tests themselves are solid and coverage is appropriate for the issue. The E1 schema coupling is the only change worth fixing before merge.

[codecov]

Codecov Report

❌ Patch coverage is 93.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.32%. Comparing base (2ba493d) to head (881ba78).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...src/main/java/com/arcadedb/graph/EdgeIterator.java	93.33%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #4690      +/-   ##
============================================
- Coverage     66.02%   65.32%   -0.70%     
- Complexity        0      573     +573     
============================================
  Files          1673     1673              
  Lines        131866   131929      +63     
  Branches      28238    28244       +6     
============================================
- Hits          87058    86179     -879     
- Misses        32815    33849    +1034     
+ Partials      11993    11901      -92     

☔ View full report in Codecov by Harness.
📢 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.

[claude]

Code Review

Overview

This PR adds regression tests for issue #4689 where MATCH (u:User) RETURN u threw NoSuchElementException via HTTP. The investigation couldn't reproduce the bug on the current codebase, so the PR's value is as a safety net. Two test classes and one tracking document are added.

---

Issues

MODERATE - Tracking doc belongs in the issue tracker, not the repo

docs/4689-cypher-match-return-vertex.md contains PR review cycle notes, a list of linked PRs, and investigation findings that are ephemeral (they reference specific commit SHAs and a PR URL). These will rot immediately. The root-cause analysis section is valuable, but it already lives in the PR description. Committing review-cycle metadata to the source tree is unusual even if prior docs like docs/4274-* exist as precedents - those precedents may simply be a pattern worth stopping. At minimum, trim this file to just the root-cause analysis; the "Review Cycles" and "Final State" sections have no future value in the codebase.

MINOR - RemoteDatabase instances are never closed in Issue4689MatchReturnVertexRemoteIT

Each of the 5 test methods constructs a RemoteDatabase but never calls close() or wraps it in try-with-resources. The established pattern in RemoteDatabaseIT and RemoteGraphBatchIT is to call database.close() explicitly or use try (RemoteDatabase db = ...). Example from RemoteDatabaseIT:

try (RemoteDatabase tx = new RemoteDatabase("127.0.0.1", 2480 + serverIndex, DATABASE_NAME, "root", ...)) {
    ...
}

Issue4267CountTypeIT also omits close(), but that's the exception; the bulk of the remote tests do close. Unclosed connections can cause flakiness at teardown when the server checks active handles.

MINOR - Issue4689MatchReturnVertexIT does not override getDatabaseName()

The sibling class Issue4689MatchReturnVertexRemoteIT correctly overrides getDatabaseName() to return "issue4689", isolating its data. The HTTP IT class falls back to the default "graph" database that BaseGraphServerTest pre-populates with V1/V2 types and vertices. While the unique type names (IssueUser, SqlSelectAll, etc.) avoid count collisions today, the asymmetry is a maintenance hazard - a future refactor to BaseGraphServerTest.populateDatabase() could silently break the count assertions here. Note that executeCommand() in BaseGraphServerTest hardcodes "/api/v1/command/graph" in its URL, so adding an override alone is not sufficient - you'd also need a local override of executeCommand (or the base method updated to use getDatabaseName()).

---

Positive Observations

• Good scenario coverage: whole-vertex return, projection workaround, SQL SELECT FROM, SQL field selection, >100-vertex UNWIND pagination boundary, vertices with edges, and the non-Studio HTTP serializer path.
• Using UNWIND for the 110-vertex bulk insert is the right call - avoids the N-round-trip loop and removes the need for @Tag("slow").
• The @out/@in edge-count assertion in cypherMatchReturnVertexWithEdgesShouldWork is a good sanity check that the serialization layer is correctly calling setMetadata().
• assertThat(...).as(...) pattern is used consistently, following project conventions.
• AssertJ assertions used throughout, as required by CLAUDE.md.
• License headers and SPDX identifiers are present and correct.
• The documented latent fragility in FinalProjectionStep.filterResult() (dual element+content state) is a useful finding that should be tracked as a follow-up issue if it hasn't been filed yet.

---

Summary

The test logic itself is solid. The two actionable items before merge are: (1) add try-with-resources (or explicit close()) on RemoteDatabase in Issue4689MatchReturnVertexRemoteIT, and (2) trim or remove docs/4689-cypher-match-return-vertex.md - keep at most the root-cause analysis, and drop the review-cycle history.

[claude]

Code Review - PR #4690: Regression tests for MATCH (u:User) RETURN u NSE

Overview

This PR adds regression test coverage for issue #4689 across three test classes and a documentation file. The investigation work is thorough and well-described. Below are specific issues found in the test code.

---

CRITICAL - Resource Leak

File: server/src/test/java/com/arcadedb/remote/Issue4689MatchReturnVertexRemoteIT.java

Every test method creates a RemoteDatabase instance but never closes it:

final RemoteDatabase database = new RemoteDatabase("127.0.0.1", 2480, getDatabaseName(), "root", ...);

If RemoteDatabase holds HTTP connections, sockets, or thread resources, these are leaked on each test run. All five methods have this problem. Fix with try-with-resources:

try (final RemoteDatabase database = new RemoteDatabase(...)) {
    // test body
}

---

Important Issues

Hardcoded port 2480 in two places

• Issue4689MatchReturnVertexIT.java line 437: new URL("http://127.0.0.1:2480/api/v1/command/" + getDatabaseName())
• Issue4689MatchReturnVertexRemoteIT.java lines 203, 225, 241, 261, 278: new RemoteDatabase("127.0.0.1", 2480, ...)

The server port should be obtained dynamically (e.g. getServer(0).getHttpServer().getPort() or similar pattern used in existing tests). A hardcoded 2480 will silently pass on the default but break in any environment where the port is configured differently.

matchReturnHubVertexWithManyEdges still uses a loop - should be tagged @Tag("slow")

Issue4689StudioSerializerIT.java lines 564-565:

for (int i = 0; i < 25; i++)
    executeCommand(0, "opencypher", "CREATE (u:StudioSpoke {idx: " + i + "})");

The PR description says @Tag("slow") was removed after the main IT switched to UNWIND, but the studio IT still fires 25 individual HTTP round trips in a loop. Either replace with UNWIND range(0,24) AS i CREATE (:StudioSpoke {idx: i}) (single command), or annotate the method with @Tag("slow") per CLAUDE.md guidelines.

---

Correctness / Test Quality

cypherMatchReturnVertexWithEdgesOverRemote does not validate edge metadata

Issue4689MatchReturnVertexRemoteIT.java lines 276-295: The test comment says it "exercises the vertex serialization with edge metadata over the wire," but the assertions only check isVertex() and a count of 2. The analogous HTTP test (cypherMatchReturnVertexWithEdgesShouldWork) verifies @out/@in counts; the remote test should do the same on the returned Vertex.

sqlSelectWholeRecordOverRemote asserts isVertex() on a non-graph type

Issue4689MatchReturnVertexRemoteIT.java line 254:

assertThat(row.isVertex()).as("SELECT FROM should produce a vertex element over RemoteDatabase").isTrue();

SqlRemoteAll is created with CREATE VERTEX TYPE, so it is a vertex - but the assertion message says "a vertex element" as if this is expected for any record. A Document record (CREATE DOCUMENT TYPE ...) would fail this assertion. The assertion is correct as written; the comment just misleads. Consider renaming the type SqlRemoteVertex or adjusting the assertion message.

---

Minor / Style

Documentation file includes review-process metadata

docs/4689-cypher-match-return-vertex.md sections "Review Cycles" and "Final State" (lines 118-155) record commit SHAs, reviewer names, and iteration summaries. This is development diary content. The bug investigation and structural inconsistency analysis is genuinely useful to preserve; the review-cycle bookkeeping belongs in the PR description or GitHub comments, not committed to the repo. Consider trimming to just the investigation analysis and "Changes Made" / "Test Results" sections.

cypherMatchReturnVertexProjectionWorkaround count assertion

Issue4689MatchReturnVertexIT.java line 360:

assertThat(result.getJSONArray("records").length()).as("Projection workaround should return 1 vertex").isEqualTo(1);

One vertex (IssueUserProj) was created, and this asserts exactly 1. The assertion is correct but the .as(...) message says "1 vertex" - fine, but if this type was already populated by a prior test run in a shared database the count could differ. Make sure BaseGraphServerTest guarantees a clean database for each test class - if it does not, explicit type-scoped cleanup in @BeforeEach/@AfterEach would make these count assertions reliable.

---

Summary

Severity	Count	Notes
Critical	1	RemoteDatabase never closed (resource leak in all 5 remote tests)
Important	2	Hardcoded port 2480; hub-vertex loop should use UNWIND or @Tag("slow")
Correctness	2	Missing edge-metadata assertion in remote test; misleading assertion message
Minor	2	Review-cycle metadata in docs file; count assertions relying on clean DB

The investigation and coverage strategy are solid. The resource leak and hardcoded port are the items that need fixing before merge.

[claude]

Review: PR #4690 - EdgeIterator NSE on dangling edge

Thorough read of the core fix (EdgeIterator.java) plus the four test files. Overall this is a solid, well-investigated fix that lands on the right approach. The rewrite of hasNext()/next() into a peek/pending pattern is correct and mirrors the already-robust IteratorFilterBase.hasNext(true) idiom, so it is consistent with the established codebase convention. Reproduction tests are excellent. A few points below worth addressing before merge.

Correctness - looks good

• The root-cause analysis is right: the old next() lazily caught RecordNotFoundException and continued, leaving hasNext() (which only checked the segment position) inconsistent, so a for-each over a dangling pointer threw NoSuchElementException. Moving validation into hasNext() with a pending flag fixes the contract.
• reset() correctly clears pending/nextEdgeRID/nextVertexRID - important, since without it a stale pending=true would survive a reset.
• Lightweight edges (position < 0) correctly skip validation - they have no record to dangle. Good.
• remove() semantics preserved: lastElementPosition is still the entry start and currentPosition the entry end at remove time.

1. Performance: doubled lookupByRID per edge (CLAUDE.md "performance mantra")

hasNext() now calls lookupByRID(nextEdgeRID, false) to validate, discards the result, then next() calls lookupByRID(nextEdgeRID, false) again to return the edge. That is two lookups per non-lightweight edge on the hot traversal path. Within a transaction the second hit is cached, but it is still redundant work on every edge of every getEdges(DIRECTION) iteration.

Suggestion: store the record loaded in hasNext() in a field (e.g. private Edge nextEdge;) and return it directly from next(), clearing it alongside pending. (For the record, the edge-count path is unaffected - EdgeLinkedList.count() uses EdgeSegment.count(), not iteration - so the regression is scoped to actual edge iteration.)

2. Robustness gap: only RecordNotFoundException is caught

The commit message states the record is force-loaded "when the transaction isolation is REPEATABLE_READ or the edge's bucket/type no longer resolves," but hasNext() only catches RecordNotFoundException. If an edge's type/bucket was dropped, lookupByRID (or later getTypeByBucketId in next()) can throw a different exception (e.g. SchemaException/IllegalArgumentException), which would propagate and re-create the same HTTP 500 the fix targets. The sibling EdgeIteratorFilter/IteratorFilterBase catch the broader Exception for exactly this reason. Consider widening the catch to match.

3. Divergence from EdgeIteratorFilter: no self-heal, no log

EdgeIteratorFilter.handleCorruption() removes the dangling RID from the segment (self-heals in READ_WRITE mode) and logs a throttled WARNING. The new EdgeIterator silently skips the dangling pointer with no log and no repair, so the corruption persists and is re-validated (load + catch) on every future iteration of that vertex (a perpetual cost on corrupted data), and operators get no signal it exists. Not a blocker, but aligning behavior (at least a throttled WARNING, ideally the same repair) would be more consistent and would stop the repeated re-validation.

4. Tests - strong coverage

DanglingEdgeIteratorTest reliably reproduces the NSE at the engine level (fails before / passes after), and Issue4689StudioSerializerIT.matchReturnVertexWithDanglingEdgeDoesNotReturn500 covers the 500->200 path end-to-end. The studio-serializer scenarios (edges between results, edges outside the result, self-loop, 25-edge hub, bidirectional) are a thoughtful exercise of the "filter out not connected edges" loop. @Tag usage and self-contained types per CLAUDE.md are respected. Nice work.

5. Minor / housekeeping

• The PR title and description are now stale: they describe a tests-only change and state "all code paths appear correct ... bug still not reproducible," but the final commit is the actual EdgeIterator fix. Please update the description so it reflects that this PR fixes the bug, not just adds tests.
• docs/4689-cypher-match-return-vertex.md is an investigation/tracking doc committed to the repo - fine if intentional, but confirm you want it in the tree long-term.

Verification

I could not run mvn in this sandbox to independently confirm the tests, so I am relying on the PR's stated results and CI. Recommend confirming mvn -pl engine test -Dtest=DanglingEdgeIteratorTest and the server ITs are green in CI.

Net: approach is correct and consistent with the codebase. I'd like to see #1 (avoid the double lookup) and #2 (widen the catch to match the filter) addressed; #3 is a recommended follow-up.

Reviewed by Claude Opus 4.8