fixed a hasId([]) ArrayOutOfBoundsException bug that occurs in the rare situation where a user provides an empty collection of ids. Test cases developed by @dkuppitz.
Project: http://git-wip-us.apache.org/repos/asf/tinkerpop/repo Commit: http://git-wip-us.apache.org/repos/asf/tinkerpop/commit/74ca03de Tree: http://git-wip-us.apache.org/repos/asf/tinkerpop/tree/74ca03de Diff: http://git-wip-us.apache.org/repos/asf/tinkerpop/diff/74ca03de Branch: refs/heads/TINKERPOP-1489 Commit: 74ca03dea1a7db7b2af39f46020cf8a75a2ea5c4 Parents: e20b8ae Author: Marko A. Rodriguez <okramma...@gmail.com> Authored: Mon Nov 6 14:36:22 2017 -0700 Committer: Marko A. Rodriguez <okramma...@gmail.com> Committed: Mon Nov 6 14:36:22 2017 -0700 ---------------------------------------------------------------------- CHANGELOG.asciidoc | 1 + .../process/traversal/step/map/GraphStep.java | 15 +++-- .../traversal/step/util/HasContainer.java | 17 ++++-- .../traversal/step/filter/GroovyHasTest.groovy | 24 +++++++- .../process/traversal/step/filter/HasTest.java | 63 +++++++++++++++++++- .../step/sideEffect/Neo4jGraphStep.java | 4 ++ .../step/sideEffect/TinkerGraphStep.java | 8 ++- 7 files changed, 117 insertions(+), 15 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/74ca03de/CHANGELOG.asciidoc ---------------------------------------------------------------------- diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 52ba2a3..128d13d 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -23,6 +23,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima [[release-3-2-7]] === TinkerPop 3.2.7 (Release Date: NOT OFFICIALLY RELEASED YET) +* Fixed an `ArrayOutOfBoundsException` in `hasId()` for the rare situation when the provided collection is empty. * Bump to Netty 4.0.52 * `TraversalVertexProgram` ``profile()` now accounts for worker iteration in `GraphComputer` OLAP. * Added a test for self-edges and fixed `Neo4jVertex` to provided repeated self-edges on `BOTH`. http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/74ca03de/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GraphStep.java ---------------------------------------------------------------------- diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GraphStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GraphStep.java index 7ab7d13..e40271c 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GraphStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GraphStep.java @@ -36,8 +36,6 @@ import org.apache.tinkerpop.gremlin.structure.util.CloseableIterator; import org.apache.tinkerpop.gremlin.structure.util.StringFactory; import org.apache.tinkerpop.gremlin.util.iterator.EmptyIterator; -import java.io.Closeable; -import java.io.IOException; import java.util.Arrays; import java.util.Collection; import java.util.Collections; @@ -102,10 +100,15 @@ public class GraphStep<S, E extends Element> extends AbstractStep<S, E> implemen } public void addIds(final Object... newIds) { - this.ids = ArrayUtils.addAll(this.ids, - (newIds.length == 1 && newIds[0] instanceof Collection) ? - ((Collection) newIds[0]).toArray(new Object[((Collection) newIds[0]).size()]) : - newIds); + if (this.ids.length == 0 && + newIds.length == 1 && + newIds[0] instanceof Collection && ((Collection) newIds[0]).isEmpty()) + this.ids = null; + else + this.ids = ArrayUtils.addAll(this.ids, + (newIds.length == 1 && newIds[0] instanceof Collection) ? + ((Collection) newIds[0]).toArray(new Object[((Collection) newIds[0]).size()]) : + newIds); } public void clearIds() { http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/74ca03de/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/HasContainer.java ---------------------------------------------------------------------- diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/HasContainer.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/HasContainer.java index 3a3d9cc..08ab389 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/HasContainer.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/HasContainer.java @@ -59,14 +59,19 @@ public class HasContainer implements Serializable, Cloneable, Predicate<Element> // grab an instance of a value which is either the first item in a homogeneous collection or the value itself final Object valueInstance = this.predicate.getValue() instanceof Collection ? - ((Collection) this.predicate.getValue()).toArray()[0] : this.predicate.getValue(); + ((Collection) this.predicate.getValue()).isEmpty() ? + new Object() : + ((Collection) this.predicate.getValue()).toArray()[0] : + this.predicate.getValue(); // if the key being evaluated is id then the has() test can evaluate as a toString() representation of the // identifier. this could be done in the test() method but it seems cheaper to do the conversion once in // the constructor. the original value in P is maintained separately this.testingIdString = this.key.equals(T.id.getAccessor()) && valueInstance instanceof String; if (this.testingIdString) - this.predicate.setValue(this.predicate.getValue() instanceof Collection ? IteratorUtils.set(IteratorUtils.map(((Collection<Object>) this.predicate.getValue()).iterator(), Object::toString)) : this.predicate.getValue().toString()); + this.predicate.setValue(this.predicate.getValue() instanceof Collection ? + IteratorUtils.set(IteratorUtils.map(((Collection<Object>) this.predicate.getValue()).iterator(), Object::toString)) : + this.predicate.getValue().toString()); } } @@ -162,9 +167,11 @@ public class HasContainer implements Serializable, Cloneable, Predicate<Element> private void enforceHomogenousCollectionIfPresent(final Object predicateValue) { if (predicateValue instanceof Collection) { final Collection collection = (Collection) predicateValue; - Class<?> first = collection.toArray()[0].getClass(); - if (!((Collection) predicateValue).stream().map(Object::getClass).allMatch(c -> first.equals(c))) - throw new IllegalArgumentException("Has comparisons on a collection of ids require ids to all be of the same type"); + if (!collection.isEmpty()) { + Class<?> first = collection.toArray()[0].getClass(); + if (!((Collection) predicateValue).stream().map(Object::getClass).allMatch(c -> first.equals(c))) + throw new IllegalArgumentException("Has comparisons on a collection of ids require ids to all be of the same type"); + } } } http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/74ca03de/gremlin-groovy-test/src/main/groovy/org/apache/tinkerpop/gremlin/process/traversal/step/filter/GroovyHasTest.groovy ---------------------------------------------------------------------- diff --git a/gremlin-groovy-test/src/main/groovy/org/apache/tinkerpop/gremlin/process/traversal/step/filter/GroovyHasTest.groovy b/gremlin-groovy-test/src/main/groovy/org/apache/tinkerpop/gremlin/process/traversal/step/filter/GroovyHasTest.groovy index 76e79a4..c70a50d 100644 --- a/gremlin-groovy-test/src/main/groovy/org/apache/tinkerpop/gremlin/process/traversal/step/filter/GroovyHasTest.groovy +++ b/gremlin-groovy-test/src/main/groovy/org/apache/tinkerpop/gremlin/process/traversal/step/filter/GroovyHasTest.groovy @@ -18,7 +18,9 @@ */ package org.apache.tinkerpop.gremlin.process.traversal.step.filter +import org.apache.tinkerpop.gremlin.process.traversal.P import org.apache.tinkerpop.gremlin.process.traversal.Traversal +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__ import org.apache.tinkerpop.gremlin.process.traversal.util.ScriptTraversal import org.apache.tinkerpop.gremlin.structure.Edge import org.apache.tinkerpop.gremlin.structure.Vertex @@ -169,5 +171,25 @@ public abstract class GroovyHasTest { public Traversal<Vertex, Vertex> get_g_V_hasLabelXpersonX_hasLabelXsoftwareX() { new ScriptTraversal<>(g, "gremlin-groovy", "g.V.hasLabel('person').hasLabel('software')") } + + @Override + public Traversal<Vertex, Long> get_g_V_hasIdXemptyX_count() { + new ScriptTraversal<>(g, "gremlin-groovy", "g.V.hasId([]).count") + } + + @Override + public Traversal<Vertex, Long> get_g_V_hasIdXwithinXemptyXX_count() { + new ScriptTraversal<>(g, "gremlin-groovy", "g.V.hasId(within([])).count") + } + + @Override + public Traversal<Vertex, Long> get_g_V_hasIdXwithoutXemptyXX_count() { + new ScriptTraversal<>(g, "gremlin-groovy", "g.V.hasId(without([])).count") + } + + @Override + public Traversal<Vertex, Long> get_g_V_notXhasIdXwithinXemptyXXX_count() { + new ScriptTraversal<>(g, "gremlin-groovy", "g.V.not(hasId(within([]))).count") + } } -} +} \ No newline at end of file http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/74ca03de/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/HasTest.java ---------------------------------------------------------------------- diff --git a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/HasTest.java b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/HasTest.java index 9bd3e23..d8457f1 100644 --- a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/HasTest.java +++ b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/HasTest.java @@ -35,6 +35,7 @@ import org.junit.Test; import org.junit.runner.RunWith; import java.util.Arrays; +import java.util.Collections; import java.util.List; import static org.apache.tinkerpop.gremlin.LoadGraphWith.GraphData.CREW; @@ -108,6 +109,14 @@ public abstract class HasTest extends AbstractGremlinProcessTest { public abstract Traversal<Vertex, Vertex> get_g_V_hasLabelXpersonX_hasLabelXsoftwareX(); + public abstract Traversal<Vertex, Long> get_g_V_hasIdXemptyX_count(); + + public abstract Traversal<Vertex, Long> get_g_V_hasIdXwithinXemptyXX_count(); + + public abstract Traversal<Vertex, Long> get_g_V_hasIdXwithoutXemptyXX_count(); + + public abstract Traversal<Vertex, Long> get_g_V_notXhasIdXwithinXemptyXXX_count(); + @Test @LoadGraphWith(MODERN) public void g_V_outXcreatedX_hasXname__mapXlengthX_isXgtX3XXX_name() { @@ -449,6 +458,38 @@ public abstract class HasTest extends AbstractGremlinProcessTest { assertFalse(traversal.hasNext()); } + @Test + @LoadGraphWith(MODERN) + public void g_V_hasIdXemptyX_count() { + final Traversal<Vertex, Long> traversal = get_g_V_hasIdXemptyX_count(); + printTraversalForm(traversal); + assertEquals(0L, traversal.next().longValue()); + } + + @Test + @LoadGraphWith(MODERN) + public void g_V_hasIdXwithinXemptyXX_count() { + final Traversal<Vertex, Long> traversal = get_g_V_hasIdXwithinXemptyXX_count(); + printTraversalForm(traversal); + assertEquals(0L, traversal.next().longValue()); + } + + @Test + @LoadGraphWith(MODERN) + public void g_V_hasIdXwithoutXemptyXX_count() { + final Traversal<Vertex, Long> traversal = get_g_V_hasIdXwithoutXemptyXX_count(); + printTraversalForm(traversal); + assertEquals(6L, traversal.next().longValue()); + } + + @Test + @LoadGraphWith(MODERN) + public void g_V_notXhasIdXwithinXemptyXXX_count() { + final Traversal<Vertex, Long> traversal = get_g_V_notXhasIdXwithinXemptyXXX_count(); + printTraversalForm(traversal); + assertEquals(6L, traversal.next().longValue()); + } + public static class Traversals extends HasTest { @Override public Traversal<Edge, Edge> get_g_EX11X_outV_outE_hasXid_10X(final Object e11Id, final Object e8Id) { @@ -589,5 +630,25 @@ public abstract class HasTest extends AbstractGremlinProcessTest { public Traversal<Vertex, Vertex> get_g_V_hasLabelXpersonX_hasLabelXsoftwareX() { return g.V().hasLabel("person").hasLabel("software"); } + + @Override + public Traversal<Vertex, Long> get_g_V_hasIdXemptyX_count() { + return g.V().hasId(Collections.emptyList()).count(); + } + + @Override + public Traversal<Vertex, Long> get_g_V_hasIdXwithinXemptyXX_count() { + return g.V().hasId(P.within(Collections.emptyList())).count(); + } + + @Override + public Traversal<Vertex, Long> get_g_V_hasIdXwithoutXemptyXX_count() { + return g.V().hasId(P.without(Collections.emptyList())).count(); + } + + @Override + public Traversal<Vertex, Long> get_g_V_notXhasIdXwithinXemptyXXX_count() { + return g.V().not(__.hasId(P.within(Collections.emptyList()))).count(); + } } -} +} \ No newline at end of file http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/74ca03de/neo4j-gremlin/src/main/java/org/apache/tinkerpop/gremlin/neo4j/process/traversal/step/sideEffect/Neo4jGraphStep.java ---------------------------------------------------------------------- diff --git a/neo4j-gremlin/src/main/java/org/apache/tinkerpop/gremlin/neo4j/process/traversal/step/sideEffect/Neo4jGraphStep.java b/neo4j-gremlin/src/main/java/org/apache/tinkerpop/gremlin/neo4j/process/traversal/step/sideEffect/Neo4jGraphStep.java index 7df47d0..f196a8d 100644 --- a/neo4j-gremlin/src/main/java/org/apache/tinkerpop/gremlin/neo4j/process/traversal/step/sideEffect/Neo4jGraphStep.java +++ b/neo4j-gremlin/src/main/java/org/apache/tinkerpop/gremlin/neo4j/process/traversal/step/sideEffect/Neo4jGraphStep.java @@ -52,10 +52,14 @@ public final class Neo4jGraphStep<S, E extends Element> extends GraphStep<S, E> } private Iterator<? extends Edge> edges() { + if (null == this.ids) + return Collections.emptyIterator(); return IteratorUtils.filter(this.getTraversal().getGraph().get().edges(this.ids), edge -> HasContainer.testAll(edge, this.hasContainers)); } private Iterator<? extends Vertex> vertices() { + if (null == this.ids) + return Collections.emptyIterator(); final Neo4jGraph graph = (Neo4jGraph) this.getTraversal().getGraph().get(); return graph.getTrait().lookupVertices(graph, this.hasContainers, this.ids); } http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/74ca03de/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/step/sideEffect/TinkerGraphStep.java ---------------------------------------------------------------------- diff --git a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/step/sideEffect/TinkerGraphStep.java b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/step/sideEffect/TinkerGraphStep.java index 36c67ac..5933ebe 100644 --- a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/step/sideEffect/TinkerGraphStep.java +++ b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/step/sideEffect/TinkerGraphStep.java @@ -63,7 +63,9 @@ public final class TinkerGraphStep<S, E extends Element> extends GraphStep<S, E> final TinkerGraph graph = (TinkerGraph) this.getTraversal().getGraph().get(); final HasContainer indexedContainer = getIndexKey(Edge.class); // ids are present, filter on them first - if (this.ids != null && this.ids.length > 0) + if (null == this.ids) + return Collections.emptyIterator(); + else if (this.ids.length > 0) return this.iteratorList(graph.edges(this.ids)); else return null == indexedContainer ? @@ -77,7 +79,9 @@ public final class TinkerGraphStep<S, E extends Element> extends GraphStep<S, E> final TinkerGraph graph = (TinkerGraph) this.getTraversal().getGraph().get(); final HasContainer indexedContainer = getIndexKey(Vertex.class); // ids are present, filter on them first - if (this.ids != null && this.ids.length > 0) + if (null == this.ids) + return Collections.emptyIterator(); + else if (this.ids.length > 0) return this.iteratorList(graph.vertices(this.ids)); else return null == indexedContainer ?