fix(#4645): shortestPath edge:true ClassCastException + wrong vertex in BOTH direction

[robfrank]

Closes #4645

Summary

This PR fixes two bugs in the shortestPath function's edge:true mode that together made shortestPath(..., {'direction':'BOTH','edge':true}) always fail:

• Bug 1 (ClassCastException): EdgeToVertexIterable.iterator() cast edges.iterator() unconditionally to EdgeIterator. When direction:BOTH, the recursive call for the side with no edges returns GraphEngine.EMPTY_EDGE_LIST whose iterator() is Collections.emptyIterator() - not an EdgeIterator. Fixed by guarding with instanceof ResettableIterator (the common base of both EdgeIterator and EdgeIteratorFilter).

• Bug 2 (wrong neighbor returned): EdgeToVertexIterator.next() called edge.getVertex(direction) which returns the current vertex (the OUT/source end for OUT direction), not the neighbor. The BFS made no progress because every discovered "neighbor" was the vertex itself. Fixed by using the opposite direction: direction == OUT ? IN : OUT.

Test plan

• SQLFunctionShortestPathTest.edgeTrueDirectionBothWithAsymmetricEdges - new regression test; verifies shortestPath(a, b, {'direction':'BOTH','edge':true}) returns [a-rid, edge-rid, b-rid] for a simple directed graph where 'a' has only an outgoing edge
• All 12 existing SQLFunctionShortestPathTest tests pass (no regression)
• CypherReduceAndShortestPathTest (26 tests) passes
• Run: mvn test -pl engine -Dtest=SQLFunctionShortestPathTest

[gemini-code-assist]

Code Review

This pull request addresses a ClassCastException and a traversal direction bug in the shortestPath function when using direction BOTH and edge true. It updates EdgeToVertexIterable to handle non-ResettableIterator instances and fixes EdgeToVertexIterator to correctly return the opposite-end vertex. A regression test is also added. The review feedback highlights a potential issue where silently returning an empty iterator for non-resettable iterators could lead to silent correctness bugs, and suggests throwing an exception if a non-empty, non-resettable iterator is encountered.

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]

Silently returning an empty iterator when iter is not an instance of ResettableIterator can lead to silent correctness bugs if a non-empty, non-resettable iterator is passed (for example, standard Java collections like ArrayList or HashSet).

If the iterator is not a ResettableIterator, we should check if it is empty. If it is empty, returning Collections.emptyIterator() is perfectly fine. If it is not empty, we should throw an exception (or wrap it) to prevent silent data loss or incorrect query results.

    final Iterator<Edge> iter = edges.iterator();
    if (iter instanceof ResettableIterator) {
      return new EdgeToVertexIterator((ResettableIterator<Edge>) iter, direction);
    }
    if (!iter.hasNext()) {
      return Collections.emptyIterator();
    }
    throw new IllegalArgumentException("The edges iterator must be an instance of ResettableIterator when not empty.");

[robfrank]

Resolved in 705af13. EdgeToVertexIterable.iterator() now returns Collections.emptyIterator() only when the non-resettable iterator is empty, and throws IllegalArgumentException otherwise so a non-empty, non-resettable iterator fails loudly instead of silently dropping edges. In practice the only non-resettable case is GraphEngine.EMPTY_EDGE_LIST (always empty); a comment documents that invariant.

[claude]

Code Review

Overview

This PR fixes two real bugs that caused shortestPath(..., {'direction':'BOTH','edge':true}) to always fail. The root-cause analysis is correct, the fixes are minimal and targeted, and a regression test is included. Good work overall - here are a few suggestions to make it stronger.

---

Bug 1 - ClassCastException fix (EdgeToVertexIterable.java)

Correct. The cast to EdgeIterator was unconditional; now it guards with instanceof ResettableIterator and returns an empty iterator for the EMPTY_EDGE_LIST case.

One design concern worth noting: returning Collections.emptyIterator() silently swallows any Iterable<Edge> whose iterator is not a ResettableIterator. Today that only happens with EMPTY_EDGE_LIST, but a future implementation could accidentally lose edges with no warning. Consider adding a comment explaining why this case is expected:

// iterator() returns Collections.emptyIterator() only for EMPTY_EDGE_LIST
if (!(iter instanceof ResettableIterator))
  return Collections.emptyIterator();

---

Bug 2 - Wrong neighbor vertex (EdgeToVertexIterator.java)

Correct. getVertex(direction) returned the source end; using the opposite direction returns the actual neighbor. The ternary is readable enough for a one-liner.

Minor style point: the line in next() is quite long - extracting the direction flip to a local variable would aid readability, though this is not a blocker:

final Vertex.DIRECTION neighborEnd = direction == Vertex.DIRECTION.OUT ? Vertex.DIRECTION.IN : Vertex.DIRECTION.OUT;
return edgeIterator.next().getVertex(neighborEnd);

---

Test coverage

The new test edgeTrueDirectionBothWithAsymmetricEdges covers the critical regression. A few gaps worth addressing:

1. Middle element not verified. The test checks hasSize(3), getFirst(), and getLast(), but never asserts that result.get(1) is the edge's RID. This leaves the "edge included" part of edge:true unverified:

// capture edge RID inside the transaction
final RID[] edgeRid = new RID[1];
graph.transaction(() -> {
  ...
  edgeRid[0] = verts[0].newEdge("BugSP_E", verts[1]).getIdentity();
});
...
assertThat(result.get(1)).isEqualTo(edgeRid[0]);

2. Reverse asymmetry not tested. The test only covers "a has an OUT edge, searching BOTH from a to b". The mirror case - searching from b to a (where b has no OUT edge) - would verify the IN direction half of the fix.

3. direction:IN with edge:true is also affected by Bug 2. A dedicated test for the pure IN case would add confidence.

---

Documentation file

docs/4645-shortestpath-edge-both-classcastexception.md duplicates the PR description. The project does not appear to follow a convention of adding per-bug markdown files to docs/. Unless this feeds a changelog generator, consider removing it to keep the repo tidy.

---

Summary

Area	Status
Bug 1 fix correctness	Correct
Bug 2 fix correctness	Correct
Regression test present	Yes
Edge RID assertion in test	Missing
Reverse / IN direction tests	Missing
Docs file	Likely not needed

The core fixes are sound. The main ask before merging is asserting the edge RID in the result and adding the reverse/IN direction test cases to make the regression suite complete.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Coverage 88.89% diff coverage · -7.32% coverage variation

Metric	Results
Coverage variation	✅ -7.32% coverage variation
Diff coverage	✅ 88.89% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (898aebe)	130249	96403	74.01%
Head commit (94a0e70)	162109 (+31860)	108115 (+11712)	66.69% (-7.32%)

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 (#4646)	9	8	88.89%

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

[codecov]

Codecov Report

❌ Patch coverage is 77.77778% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.02%. Comparing base (898aebe) to head (94a0e70).
⚠️ Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
.../java/com/arcadedb/graph/EdgeToVertexIterable.java	66.66%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #4646      +/-   ##
============================================
+ Coverage     64.95%   65.02%   +0.06%     
  Complexity      520      520              
============================================
  Files          1668     1668              
  Lines        130249   130306      +57     
  Branches      27931    27943      +12     
============================================
+ Hits          84600    84725     +125     
+ Misses        33880    33819      -61     
+ Partials      11769    11762       -7     

☔ 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 (Cycle 2 follow-up)

Reviewing HEAD 705af13ba - this is a follow-up pass after the cycle-1 feedback was applied.

What was addressed from the previous review

All four cycle-1 suggestions were applied:

• IllegalArgumentException for non-empty, non-ResettableIterator cases - done; the defensive guard now fails loudly instead of silently dropping edges.
• neighborEnd local variable extracted in EdgeToVertexIterator.next() - done; readability improved.
• Edge RID assertion (result.get(1)) added to the asymmetric test - done.
• Reverse / IN direction tests added (edgeTrueDirectionBothReverseAsymmetric, edgeTrueDirectionIn) - done; both exercise the previously untested halves.

Remaining observations

1. One uncovered branch (minor)

Codecov flags 1 missing + 1 partial line in EdgeToVertexIterable. The missing line is the throw new IllegalArgumentException path - the branch where a non-empty, non-ResettableIterator is passed. This is a defensive guard against misuse that cannot be triggered via the public shortestPath API today, so no test is strictly needed. Worth noting for future maintainers, but not a blocker.

2. neighborEnd recomputed on every next() call

// EdgeToVertexIterator.next()
final Vertex.DIRECTION neighborEnd = direction == Vertex.DIRECTION.OUT ? Vertex.DIRECTION.IN : Vertex.DIRECTION.OUT;

This ternary runs once per iterated edge. It is a constant derived from an immutable field, so it could be precomputed in the constructor and stored. The JIT will almost certainly hoist it anyway, so this is purely cosmetic - not worth a re-review cycle.

3. Docs file

The docs/4645-shortestpath-edge-both-classcastexception.md file is intentionally retained as part of the resolve-issue workflow convention. Noted and understood.

Summary

Area	Status
Bug 1 fix (ClassCastException)	Correct
Bug 2 fix (wrong neighbor vertex)	Correct
Defensive guard (non-empty non-resettable)	In place
Edge RID assertion in tests	Present
Reverse + IN direction tests	Present
Uncovered defensive branch	Minor / acceptable

The fixes are correct, the regression tests are thorough, and all cycle-1 feedback was addressed. The PR is ready to merge.