tinkerpop-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ok...@apache.org
Subject [1/3] tinkerpop git commit: 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.
Date Tue, 07 Nov 2017 19:17:54 GMT
Repository: tinkerpop
Updated Branches:
  refs/heads/master 7057d5ffb -> 500c2435e


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/master
Commit: 74ca03dea1a7db7b2af39f46020cf8a75a2ea5c4
Parents: e20b8ae
Author: Marko A. Rodriguez <okrammarko@gmail.com>
Authored: Mon Nov 6 14:36:22 2017 -0700
Committer: Marko A. Rodriguez <okrammarko@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 ?


Mime
View raw message