updated hasId() implementation to also unroll ids in lists in additio…

…n to arrays, so P.within is applied to Collection

CHANGELOG.asciidoc

@@ -36,6 +36,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima
 * Fixed bug in parsing of `math()` expressions where variables were not being identified if they contained a text associated with a function.
 * Refactored `FilterRankingStrategy` to improve performance for deeply nested traversals.
 * 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.
 

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(

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

@@ -687,6 +687,23 @@ public void shouldFoldPropertyStepForTokens() {
         assertThat(g.V("id").hasNext(), is(true));
     }
 
+    /**
+     * Validating that hasId() also unwraps ids in lists in addition to ids in arrays as per TINKERPOP-2863
+     */
+    @Test
+    public void shouldCheckWithinListsOfIdsForHasId() {
+        final GraphTraversalSource g = TinkerFactory.createModern().traversal();
+
+        final List<Vertex> expectedStartTraversal = g.V().hasId(1, 2).toList();
+        final List<Vertex> expectedMidTraversal = g.V().has("name", "marko").outE("knows").inV().hasId(2, 4).toList();
+
+        assertEquals(expectedStartTraversal, g.V().hasId(new Integer[]{1, 2}).toList());
+        assertEquals(expectedStartTraversal,g.V().hasId(Arrays.asList(1, 2)).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)