TINKERPOP-2863 Update hasId step mid-traversal processing (#2019)

updated hasId() implementation to also unroll ids in lists in addition to arrays, so P.within is applied to Collection

CHANGELOG.asciidoc

@@ -40,6 +40,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima
 * Added `Traversal.lock()` to provide an explicit way to finalize a traversal object.
 * Changed `Traversal.getGraph()` to get its `Graph` object from itself or, if not available, its parent.
 * Added `AuthInfoProvider` interface and `NewDynamicAuth()` to gremlin-go for dynamic authentication support.
+* Fixed bug where `hasId()` unrolls ids in Java arrays to put into `P.within` but not ids in lists, this also aligned behavior of start-step and mid-traversal hasId().
 * Bumped to `snakeyaml` 2.0 to fix security vulnerability.
 * Bumped to Apache `commons-configuration` 2.9.0 to fix security vulnerability.
 * Improved `count` step optimization for negative values in input for 'eq' comparison.

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java

@@ -1608,35 +1608,31 @@ public default GraphTraversal<S, E> hasId(final Object id, final Object... other
             return this.hasId((P) id);
         }
         else {
-            Object[] ids;
+            this.asAdmin().getBytecode().addStep(Symbols.hasId, id, otherIds);
+
+            //using ArrayList instead of arrays given P.within() turns all arguments into lists
+            final List<Object> ids = new ArrayList<>();
             if (id instanceof Object[]) {
-                ids = (Object[]) id;
-            } else {
-                ids = new Object[] {id};
-            }
-            int size = ids.length;
-            int capacity = size;
+                Collections.addAll(ids, (Object[]) id);
+            } else if (id instanceof Collection) {
+                // as ids are unrolled when it's in array, they should also be unrolled when it's a list.
+                // this also aligns with behavior of hasId() when it's pushed down to g.V() (TINKERPOP-2863)
+                ids.addAll((Collection<?>) id);
+            } else
+                ids.add(id);
+
+            // unrolling ids from lists works cleaner with Collection too, as otherwise they will need to
+            // be turned into array first
             for (final Object i : otherIds) {
-                if (i.getClass().isArray()) {
-                    final Object[] tmp = (Object[]) i;
-                    int newLength = size + tmp.length;
-                    if (capacity < newLength) {
-                        ids = Arrays.copyOf(ids, capacity = size + tmp.length);
-                    }
-                    System.arraycopy(tmp, 0, ids, size, tmp.length);
-                    size = newLength;
-                } else {
-                    if (capacity == size) {
-                        ids = Arrays.copyOf(ids, capacity = size * 2);
-                    }
-                    ids[size++] = i;
-                }
-            }
-            if (capacity > size) {
-                ids = Arrays.copyOf(ids, size);
+                if (i instanceof Object[]) {
+                    Collections.addAll(ids, (Object[]) i);
+                } else if (i instanceof Collection) {
+                    ids.addAll((Collection<?>) i);
+                } else
+                    ids.add(i);
             }
-            this.asAdmin().getBytecode().addStep(Symbols.hasId, ids);
-            return TraversalHelper.addHasContainer(this.asAdmin(), new HasContainer(T.id.getAccessor(), ids.length == 1 ? P.eq(ids[0]) : P.within(ids)));
+
+            return TraversalHelper.addHasContainer(this.asAdmin(), new HasContainer(T.id.getAccessor(), ids.size() == 1 ? P.eq(ids.get(0)) : P.within(ids)));
         }
     }
 

gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/HasStepTest.java

@@ -25,6 +25,7 @@
 import org.junit.Test;
 
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.List;
 
 import static org.apache.tinkerpop.gremlin.process.traversal.P.eq;
@@ -66,11 +67,14 @@ public void testVarargsCompatibility() {
                 Arrays.asList(
                         __.hasId(1),
                         __.hasId(eq(1)),
-                        __.hasId(new Integer[]{1})),
+                        __.hasId(new Integer[]{1}),
+                        __.hasId(Collections.singletonList(1))),
                 Arrays.asList(
                         __.hasId(1, 2),
                         __.hasId(within(1, 2)),
-                        __.hasId(new Integer[]{1, 2})),
+                        __.hasId(new Integer[]{1, 2}),
+                        __.hasId(Arrays.asList(1, 2)),
+                        __.hasId(Collections.singletonList(1), Collections.singletonList(2))),
 
                 // hasLabel(Object label, Object... moreLabels) should be compatible with hasLabel(Object... labels)
                 Arrays.asList(

gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/Gremlin.cs

@@ -188,6 +188,9 @@ public static void InstantiateTranslationsForTestRun()
                {"g_VX1X_out_hasIdX2X", new List<Func<GraphTraversalSource, IDictionary<string, object>, ITraversal>> {(g,p) =>g.V(p["vid1"]).Out().HasId(p["vid2"])}}, 
                {"g_VX1X_out_hasXid_2_3X", new List<Func<GraphTraversalSource, IDictionary<string, object>, ITraversal>> {(g,p) =>g.V(p["vid1"]).Out().HasId(p["vid2"],p["vid3"])}}, 
                {"g_VX1X_out_hasXid_2AsString_3AsStringX", new List<Func<GraphTraversalSource, IDictionary<string, object>, ITraversal>> {(g,p) =>g.V(p["vid1"]).Out().HasId(p["vid2"],p["vid3"])}}, 
+               {"g_VX1X_out_hasXid_2_3X_inList", new List<Func<GraphTraversalSource, IDictionary<string, object>, ITraversal>> {(g,p) =>g.V(p["vid1"]).Out().HasId(p["xx1"])}}, 
+               {"g_V_hasXid_1_2X", new List<Func<GraphTraversalSource, IDictionary<string, object>, ITraversal>> {(g,p) =>g.V().HasId(p["vid1"],p["vid2"])}}, 
+               {"g_V_hasXid_1_2X_inList", new List<Func<GraphTraversalSource, IDictionary<string, object>, ITraversal>> {(g,p) =>g.V().HasId(p["xx1"])}}, 
                {"g_V_hasXblahX", new List<Func<GraphTraversalSource, IDictionary<string, object>, ITraversal>> {(g,p) =>g.V().Has("blah")}}, 
                {"g_EX7X_hasLabelXknowsX", new List<Func<GraphTraversalSource, IDictionary<string, object>, ITraversal>> {(g,p) =>g.E(p["eid7"]).HasLabel("knows")}}, 
                {"g_E_hasLabelXknowsX", new List<Func<GraphTraversalSource, IDictionary<string, object>, ITraversal>> {(g,p) =>g.E().HasLabel("knows")}}, 

gremlin-go/driver/cucumber/gremlin.go

@@ -159,6 +159,9 @@ var translationMap = map[string][]func(g *gremlingo.GraphTraversalSource, p map[
     "g_VX1X_out_hasIdX2X": {func(g *gremlingo.GraphTraversalSource, p map[string]interface{}) *gremlingo.GraphTraversal {return g.V(p["vid1"]).Out().HasId(p["vid2"])}}, 
     "g_VX1X_out_hasXid_2_3X": {func(g *gremlingo.GraphTraversalSource, p map[string]interface{}) *gremlingo.GraphTraversal {return g.V(p["vid1"]).Out().HasId(p["vid2"], p["vid3"])}}, 
     "g_VX1X_out_hasXid_2AsString_3AsStringX": {func(g *gremlingo.GraphTraversalSource, p map[string]interface{}) *gremlingo.GraphTraversal {return g.V(p["vid1"]).Out().HasId(p["vid2"], p["vid3"])}}, 
+    "g_VX1X_out_hasXid_2_3X_inList": {func(g *gremlingo.GraphTraversalSource, p map[string]interface{}) *gremlingo.GraphTraversal {return g.V(p["vid1"]).Out().HasId(p["xx1"])}}, 
+    "g_V_hasXid_1_2X": {func(g *gremlingo.GraphTraversalSource, p map[string]interface{}) *gremlingo.GraphTraversal {return g.V().HasId(p["vid1"], p["vid2"])}}, 
+    "g_V_hasXid_1_2X_inList": {func(g *gremlingo.GraphTraversalSource, p map[string]interface{}) *gremlingo.GraphTraversal {return g.V().HasId(p["xx1"])}}, 
     "g_V_hasXblahX": {func(g *gremlingo.GraphTraversalSource, p map[string]interface{}) *gremlingo.GraphTraversal {return g.V().Has("blah")}}, 
     "g_EX7X_hasLabelXknowsX": {func(g *gremlingo.GraphTraversalSource, p map[string]interface{}) *gremlingo.GraphTraversal {return g.E(p["eid7"]).HasLabel("knows")}}, 
     "g_E_hasLabelXknowsX": {func(g *gremlingo.GraphTraversalSource, p map[string]interface{}) *gremlingo.GraphTraversal {return g.E().HasLabel("knows")}}, 

gremlin-python/src/main/python/radish/gremlin.py

@@ -161,6 +161,9 @@
     'g_VX1X_out_hasIdX2X': [(lambda g, vid2=None,vid1=None:g.V(vid1).out().hasId(vid2))], 
     'g_VX1X_out_hasXid_2_3X': [(lambda g, vid3=None,vid2=None,vid1=None:g.V(vid1).out().hasId(vid2,vid3))], 
     'g_VX1X_out_hasXid_2AsString_3AsStringX': [(lambda g, vid3=None,vid2=None,vid1=None:g.V(vid1).out().hasId(vid2,vid3))], 
+    'g_VX1X_out_hasXid_2_3X_inList': [(lambda g, xx1=None,vid1=None:g.V(vid1).out().hasId(xx1))], 
+    'g_V_hasXid_1_2X': [(lambda g, vid2=None,vid1=None:g.V().hasId(vid1,vid2))], 
+    'g_V_hasXid_1_2X_inList': [(lambda g, xx1=None:g.V().hasId(xx1))], 
     'g_V_hasXblahX': [(lambda g:g.V().has('blah'))], 
     'g_EX7X_hasLabelXknowsX': [(lambda g, eid7=None:g.E(eid7).hasLabel('knows'))], 
     'g_E_hasLabelXknowsX': [(lambda g:g.E().hasLabel('knows'))], 

gremlin-test/features/filter/Has.feature

@@ -250,6 +250,47 @@ Feature: Step - has()
       | v[vadas] |
       | v[lop] |
 
+  Scenario: g_VX1X_out_hasXid_2_3X_inList
+    Given the modern graph
+    And using the parameter vid1 defined as "v[marko].id"
+    And using the parameter xx1 defined as "l[v[vadas].id,v[lop].id]"
+    And the traversal of
+      """
+      g.V(vid1).out().hasId(xx1)
+      """
+    When iterated to list
+    Then the result should be unordered
+      | result |
+      | v[vadas] |
+      | v[lop] |
+
+  Scenario: g_V_hasXid_1_2X
+    Given the modern graph
+    And using the parameter vid1 defined as "v[marko].id"
+    And using the parameter vid2 defined as "v[vadas].id"
+    And the traversal of
+      """
+      g.V().hasId(vid1, vid2)
+      """
+    When iterated to list
+    Then the result should be unordered
+      | result |
+      | v[marko] |
+      | v[vadas] |
+
+  Scenario: g_V_hasXid_1_2X_inList
+    Given the modern graph
+    And using the parameter xx1 defined as "l[v[marko].id,v[vadas].id]"
+    And the traversal of
+      """
+      g.V().hasId(xx1)
+      """
+    When iterated to list
+    Then the result should be unordered
+      | result |
+      | v[marko] |
+      | v[vadas] |
+
   Scenario: g_V_hasXblahX
     Given the modern graph
     And the traversal of

gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/HasTest.java

@@ -88,6 +88,12 @@ public abstract class HasTest extends AbstractGremlinProcessTest {
 
     public abstract Traversal<Vertex, Vertex> get_g_VX1X_out_hasIdX2_3X(final Object v1Id, final Object v2Id, final Object v3Id);
 
+    public abstract Traversal<Vertex, Vertex> get_g_VX1X_out_hasIdX2_3X_inList(final Object v1Id, final Object v2Id, final Object v3Id);
+
+    public abstract Traversal<Vertex, Vertex> get_g_V_hasIdX2_3X(final Object v1Id, final Object v2Id);
+
+    public abstract Traversal<Vertex, Vertex> get_g_V_hasIdX2_3X_inList(final Object v1Id, final Object v2Id);
+
     public abstract Traversal<Vertex, Vertex> get_g_V_hasXage_gt_30X();
 
     public abstract Traversal<Vertex, Vertex> get_g_V_hasXage_isXgt_30XX();
@@ -354,7 +360,7 @@ public void g_VX1X_out_hasXid_2_3X() {
         final Object id2 = convertToVertexId("vadas");
         final Object id3 = convertToVertexId("lop");
         final Traversal<Vertex, Vertex> traversal = get_g_VX1X_out_hasIdX2_3X(convertToVertexId("marko"), id2, id3);
-        assert_g_VX1X_out_hasXid_2_3X(id2, id3, traversal);
+        assert_g_has_2id(id2, id3, traversal);
     }
 
     @Test
@@ -363,14 +369,42 @@ public void g_VX1X_out_hasXid_2AsString_3AsStringX() {
         final Object id2 = convertToVertexId("vadas");
         final Object id3 = convertToVertexId("lop");
         final Traversal<Vertex, Vertex> traversal = get_g_VX1X_out_hasIdX2_3X(convertToVertexId("marko"), id2.toString(), id3.toString());
-        assert_g_VX1X_out_hasXid_2_3X(id2, id3, traversal);
+        assert_g_has_2id(id2, id3, traversal);
     }
 
-    protected void assert_g_VX1X_out_hasXid_2_3X(Object id2, Object id3, Traversal<Vertex, Vertex> traversal) {
+    @Test
+    @LoadGraphWith(MODERN)
+    public void g_VX1X_out_hasXid_2_3X_inList() {
+        final Object id2 = convertToVertexId("vadas");
+        final Object id3 = convertToVertexId("lop");
+        final Traversal<Vertex, Vertex> traversal = get_g_VX1X_out_hasIdX2_3X_inList(convertToVertexId("marko"), id2, id3);
+        assert_g_has_2id(id2, id3, traversal);
+    }
+
+    @Test
+    @LoadGraphWith(MODERN)
+    public void g_V_hasXid_1_2X() {
+        final Object id1 = convertToVertexId("marko");
+        final Object id2 = convertToVertexId("vadas");
+        final Traversal<Vertex, Vertex> traversal = get_g_V_hasIdX2_3X(id1, id2);
+        assert_g_has_2id(id1, id2, traversal);
+    }
+
+    @Test
+    @LoadGraphWith(MODERN)
+    public void g_V_hasXid_1_2X_inList() {
+        final Object id1 = convertToVertexId("marko");
+        final Object id2 = convertToVertexId("vadas");
+        final Traversal<Vertex, Vertex> traversal = get_g_V_hasIdX2_3X_inList(id1, id2);
+        assert_g_has_2id(id1, id2, traversal);
+    }
+
+    // asserts that the traversal returns two ids
+    protected void assert_g_has_2id(Object id1, Object id2, Traversal<Vertex, Vertex> traversal) {
         printTraversalForm(traversal);
         assertTrue(traversal.hasNext());
-        assertThat(traversal.next().id(), CoreMatchers.anyOf(is(id2), is(id3)));
-        assertThat(traversal.next().id(), CoreMatchers.anyOf(is(id2), is(id3)));
+        assertThat(traversal.next().id(), CoreMatchers.anyOf(is(id1), is(id2)));
+        assertThat(traversal.next().id(), CoreMatchers.anyOf(is(id1), is(id2)));
         assertThat(traversal.hasNext(), is(false));
     }
 
@@ -964,7 +998,6 @@ public Traversal<Vertex, Vertex> get_g_VXv1X_hasXage_gt_30X(final Object v1Id) {
         public Traversal<Vertex, Vertex> get_g_VX1X_out_hasXid_lt_3X(final Object v1Id, final Object v3Id) {
             return g.V(v1Id).out().has(T.id, P.lt(v3Id));
         }
-
         @Override
         public Traversal<Vertex, Vertex> get_g_VX1X_out_hasIdX2X(final Object v1Id, final Object v2Id) {
             return g.V(v1Id).out().hasId(v2Id);
@@ -975,6 +1008,21 @@ public Traversal<Vertex, Vertex> get_g_VX1X_out_hasIdX2_3X(final Object v1Id, fi
             return g.V(v1Id).out().hasId(v2Id, v3Id);
         }
 
+        @Override
+        public Traversal<Vertex, Vertex> get_g_VX1X_out_hasIdX2_3X_inList(final Object v1Id, final Object v2Id, final Object v3Id) {
+            return g.V(v1Id).out().hasId(Arrays.asList(v2Id, v3Id));
+        }
+
+        @Override
+        public Traversal<Vertex, Vertex> get_g_V_hasIdX2_3X(final Object v1Id, final Object v2Id) {
+            return g.V().hasId(v1Id, v2Id);
+        }
+
+        @Override
+        public Traversal<Vertex, Vertex> get_g_V_hasIdX2_3X_inList(final Object v1Id, final Object v2Id) {
+            return g.V().hasId(Arrays.asList(v1Id, v2Id));
+        }
+
         @Override
         public Traversal<Vertex, Vertex> get_g_V_hasXage_gt_30X() {
             return g.V().has("age", P.gt(30));

tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraphTest.java

@@ -687,6 +687,32 @@ public void shouldFoldPropertyStepForTokens() {
         assertThat(g.V("id").hasNext(), is(true));
     }
 
+    /**
+     * Validating that start-step hasId() unwraps ids in lists in addition to ids in arrays as per TINKERPOP-2863
+     */
+    @Test
+    public void shouldCheckWithinListsOfIdsForStartStepHasId() {
+        final GraphTraversalSource g = TinkerFactory.createModern().traversal();
+
+        final List<Vertex> expectedStartTraversal = g.V().hasId(1, 2).toList();
+
+        assertEquals(expectedStartTraversal, g.V().hasId(new Integer[]{1, 2}).toList());
+        assertEquals(expectedStartTraversal, g.V().hasId(Arrays.asList(1, 2)).toList());
+    }
+
+    /**
+     * Validating that mid-traversal hasId() also unwraps ids in lists in addition to ids in arrays as per TINKERPOP-2863
+     */
+    @Test
+    public void shouldCheckWithinListsOfIdsForMidTraversalHasId() {
+        final GraphTraversalSource g = TinkerFactory.createModern().traversal();
+
+        final List<Vertex> expectedMidTraversal = g.V().has("name", "marko").outE("knows").inV().hasId(2, 4).toList();
+
+        assertEquals(expectedMidTraversal, g.V().has("name", "marko").outE("knows").inV().hasId(new Integer[]{2, 4}).toList());
+        assertEquals(expectedMidTraversal, g.V().has("name", "marko").outE("knows").inV().hasId(Arrays.asList(2, 4)).toList());
+    }
+
     @Test
     public void shouldOptionalUsingWithComputer() {
         // not all systems will have 3+ available processors (e.g. travis)