Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions docs/4645-shortestpath-edge-both-classcastexception.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
# Fix #4645 - shortestPath edge:true ClassCastException on BOTH direction with empty side

## Issue

`shortestPath(..., {'direction':'BOTH','edge':true})` throws `ClassCastException` when one of the two directional sub-queries returns an empty edge list.

## Root Cause (two bugs)

**Bug 1 - ClassCastException:** `EdgeToVertexIterable.iterator()` unconditionally cast the result of `edges.iterator()` to `EdgeIterator`:
```java
return new EdgeToVertexIterator((EdgeIterator) edges.iterator(), direction);
```
When `direction:BOTH`, `SQLFunctionShortestPath.getVerticesAndEdges()` recurses into `OUT` and `IN`. For a vertex that has only an outgoing edge, the `IN` side returns `GraphEngine.EMPTY_EDGE_LIST`. That list's `iterator()` returns `Collections.emptyIterator()` which is NOT an `EdgeIterator`, causing `ClassCastException`.

**Bug 2 - Wrong vertex returned:** `EdgeToVertexIterator.next()` called `edge.getVertex(direction)` where `direction` is the traversal direction. For an OUT traversal from vertex 'a', this returns 'a' (the OUT/source end) rather than the neighbor 'b' (the IN/destination end). The BFS made no progress because every "neighbor" was the current vertex itself.

## Fix

**EdgeToVertexIterable:** check `instanceof ResettableIterator` (the common supertype of `EdgeIterator` and `EdgeIteratorFilter`) before casting. `EMPTY_EDGE_LIST.iterator()` returns `Collections.emptyIterator()` which is not a `ResettableIterator`, so it returns an empty iterator instead.

**EdgeToVertexIterator:** change field type to `ResettableIterator<Edge>` and fix `next()` to use the opposite direction (`direction == OUT ? IN : OUT`), which correctly returns the neighbor vertex.

## Files Changed

- `engine/src/main/java/com/arcadedb/graph/EdgeToVertexIterable.java` - guard against non-ResettableIterator (empty edge list)
- `engine/src/main/java/com/arcadedb/graph/EdgeToVertexIterator.java` - accept `ResettableIterator<Edge>`; fix `next()` to return the opposite-end vertex
- `engine/src/test/java/com/arcadedb/function/sql/graph/SQLFunctionShortestPathTest.java` - regression test for `edge:true` + `direction:BOTH` with asymmetric edges

## Verification

Run: `mvn test -pl engine -Dtest=SQLFunctionShortestPathTest`

Results: 12 tests run, 0 failures, 0 errors.
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@
*/
package com.arcadedb.graph;

import com.arcadedb.utility.ResettableIterator;

import java.util.Collections;
import java.util.Iterator;

/**
Expand All @@ -34,6 +37,9 @@ public EdgeToVertexIterable(final Iterable<Edge> edges, final Vertex.DIRECTION d

@Override
public Iterator<Vertex> iterator() {
return new EdgeToVertexIterator((EdgeIterator) edges.iterator(), direction);
final Iterator<Edge> iter = edges.iterator();
if (!(iter instanceof ResettableIterator))
return Collections.emptyIterator();
return new EdgeToVertexIterator((ResettableIterator<Edge>) iter, direction);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.");

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@
* Created by luigidellaquila on 02/07/16.
*/
public class EdgeToVertexIterator implements ResettableIterator<Vertex> {
private final EdgeIterator edgeIterator;
private final Vertex.DIRECTION direction;
private final ResettableIterator<Edge> edgeIterator;
private final Vertex.DIRECTION direction;

public EdgeToVertexIterator(final EdgeIterator iterator, final Vertex.DIRECTION direction) {
public EdgeToVertexIterator(final ResettableIterator<Edge> iterator, final Vertex.DIRECTION direction) {
if (direction == Vertex.DIRECTION.BOTH)
throw new IllegalArgumentException("edge to vertex iterator does not support BOTH as direction");

Expand All @@ -42,7 +42,7 @@ public boolean hasNext() {

@Override
public Vertex next() {
return edgeIterator.next().getVertex(direction);
return edgeIterator.next().getVertex(direction == Vertex.DIRECTION.OUT ? Vertex.DIRECTION.IN : Vertex.DIRECTION.OUT);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,36 @@ void consolidatedOptionsMap() throws Exception {
});
}

@Test
void edgeTrueDirectionBothWithAsymmetricEdges() throws Exception {
TestHelper.executeInNewDatabase("testEdgeBothAsymmetric", graph -> {
final MutableVertex[] verts = new MutableVertex[2];

graph.transaction(() -> {
graph.getSchema().createVertexType("BugSP_V");
graph.getSchema().createEdgeType("BugSP_E");

verts[0] = graph.newVertex("BugSP_V").set("name", "a").save();
verts[1] = graph.newVertex("BugSP_V").set("name", "b").save();
verts[0].newEdge("BugSP_E", verts[1]);
});

function = new SQLFunctionShortestPath();

final Map<String, Object> options = new HashMap<>();
options.put("direction", "BOTH");
options.put("edge", true);

final List<RID> result = function.execute(null, null, null, new Object[] { verts[0], verts[1], options },
new BasicCommandContext());

// expected: [a-rid, edge-rid, b-rid]
assertThat(result).hasSize(3);
assertThat(result.getFirst()).isEqualTo(verts[0].getIdentity());
assertThat(result.getLast()).isEqualTo(verts[1].getIdentity());
});
}

@Test
void rejectsUnknownOption() throws Exception {
TestHelper.executeInNewDatabase("testShortestPathUnknownOption", graph -> {
Expand Down
Loading