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

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.

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

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.

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

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

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