wip

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/P.java

@@ -25,7 +25,9 @@
 import java.io.Serializable;
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.List;
 import java.util.function.Predicate;
+import java.util.stream.Collectors;
 
 /**
  * Predefined {@code Predicate} values that can be used to define filters to {@code has()} and {@code where()}.
@@ -75,10 +77,22 @@ public void setValue(final V value) {
 
     @Override
     public boolean test(final V testValue) {
-        if (this.value instanceof GValue)
+        if (this.value instanceof GValue) {
             return this.biPredicate.test(testValue, ((GValue<V>) this.value).get());
-        else
-            return this.biPredicate.test(testValue, this.value);
+        } else {
+            // this might be a bunch of GValue that need to be resolved. zomg
+            if (this.value instanceof List) {
+                return this.biPredicate.test(testValue, (V) ((List) this.value).stream().map(o -> {
+                    if (o instanceof GValue) {
+                        return ((GValue) o).get();
+                    } else {
+                        return o;
+                    }
+                }).collect(Collectors.toList()));
+            } else {
+                return this.biPredicate.test(testValue, this.value);
+            }
+        }
     }
 
     @Override

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

@@ -25,6 +25,7 @@
 import org.apache.tinkerpop.gremlin.process.computer.traversal.step.map.PeerPressureVertexProgramStep;
 import org.apache.tinkerpop.gremlin.process.computer.traversal.step.map.ProgramVertexProgramStep;
 import org.apache.tinkerpop.gremlin.process.computer.traversal.step.map.ShortestPathVertexProgramStep;
+import org.apache.tinkerpop.gremlin.process.traversal.Contains;
 import org.apache.tinkerpop.gremlin.process.traversal.DT;
 import org.apache.tinkerpop.gremlin.process.traversal.Failure;
 import org.apache.tinkerpop.gremlin.process.traversal.Merge;
@@ -2837,6 +2838,12 @@ public default GraphTraversal<S, E> hasId(final Object id, final Object... other
         } else {
             this.asAdmin().getBytecode().addStep(Symbols.hasId, id, otherIds);
 
+            // the logic for dealing with hasId([]) is sketchy historically, just trying to maintain what we were
+            // originally testing prior to GValue.
+            if (id instanceof GValue && ((GValue) id).getType().isCollection()) {
+                return TraversalHelper.addHasContainer(this.asAdmin(), new HasContainer(T.id.getAccessor(), new P(Contains.within, id)));
+            }
+
             //using ArrayList given P.within() turns all arguments into lists
             final List<Object> ids = new ArrayList<>();
             if (id instanceof Object[]) {

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/GValue.java

@@ -246,14 +246,14 @@ public static GValue<Map> ofMap(final String name, final Map value) {
     /**
      * Create a new {@code GValue} for a list value.
      */
-    public static GValue<List> ofList(final List value) {
+    public static <T> GValue<List<T>> ofList(final List<T> value) {
         return new GValue<>(GType.LIST, value);
     }
 
     /**
      * Create a new {@code GValue} for a list value with a specified name.
      */
-    public static GValue<List> ofList(final String name, final List value) {
+    public static <T> GValue<List<T>> ofList(final String name, final List<T> value) {
         return new GValue<>(name, GType.LIST, value);
     }
 

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GraphStep.java

@@ -43,6 +43,7 @@
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Iterator;
+import java.util.List;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.function.Supplier;
@@ -56,6 +57,7 @@ public class GraphStep<S, E extends Element> extends AbstractStep<S, E> implemen
     protected Parameters parameters = new Parameters();
     protected final Class<E> returnClass;
     protected GValue<?>[] ids;
+    protected boolean legacyLogicForPassingNoIds = false;
     protected transient Supplier<Iterator<E>> iteratorSupplier;
     protected boolean isStart;
     protected boolean done = false;
@@ -152,10 +154,21 @@ public GValue[] getIds() {
      * Gets the ids associated with this step as literal values rather than {@link GValue} objects.
      */
     public Object[] getResolvedIds() {
+        if (legacyLogicForPassingNoIds) return null;
         return resolveToValues(this.ids);
     }
 
     public void addIds(final Object... newIds) {
+        // there is some logic that has been around for a long time that used to set ids to null. it only happened here
+        // in this method and only occurred when the ids were already null or empty and the newIds were length 1 and
+        // an instance of List and that list was empty. so basically it would trigger for something like g.V().hasId([])
+        // which in turn would trigger an empty iterator in TinkerGraphStep and zero results. trying to maintain that
+        // logic now with GValue in the mix is tough because the context of what the meaning is gets lost by the time
+        // you get to calling getResolvedIds(). using a flag to try to maintain that legacy logic, but ultimately, all
+        // this needs to be rethought.
+        this.legacyLogicForPassingNoIds = newIds.length == 1 && ((newIds[0] instanceof List && ((List) newIds[0]).isEmpty()) ||
+                (newIds[0] instanceof GValue && ((GValue) newIds[0]).getType().isCollection() && ((List) ((GValue) newIds[0]).get()).isEmpty()));
+
         final GValue[] gvalues = convertToGValues(tryUnrollSingleCollectionArgument(newIds));
         this.ids = ArrayUtils.addAll(this.ids, gvalues);
     }

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/AbstractStep.java

@@ -28,9 +28,11 @@
 import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalInterruptedException;
 import org.apache.tinkerpop.gremlin.structure.util.StringFactory;
 
+import java.util.Collection;
 import java.util.Collections;
 import java.util.Iterator;
 import java.util.LinkedHashSet;
+import java.util.List;
 import java.util.NoSuchElementException;
 import java.util.Objects;
 import java.util.Set;
@@ -245,7 +247,22 @@ protected static <T> GValue<T>[] convertToGValues(final Object[] args) {
     }
 
     /**
-     * Converts {@link GValue} objects the argument array to their values to prevent them from leaking to the Graph API.
+     * Converts {@link GValue} objects arguments to their values to prevent them from leaking to the Graph API.
+     * Providers extending from this step should use this method to get actual values to prevent any {@link GValue}
+     * objects from leaking to the Graph API.
+     */
+    protected Object[] resolveToValues(final List<GValue<?>> gvalues) {
+        // convert gvalues to array
+        final Object[] newIds = new Object[gvalues.size()];
+        int i = 0;
+        for (final GValue<?> gvalue : gvalues) {
+            newIds[i++] = gvalue.get();
+        }
+        return newIds;
+    }
+
+    /**
+     * Converts {@link GValue} objects argument array to their values to prevent them from leaking to the Graph API.
      * Providers extending from this step should use this method to get actual values to prevent any {@link GValue}
      * objects from leaking to the Graph API.
      */

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/HasContainer.java

@@ -19,10 +19,12 @@
 package org.apache.tinkerpop.gremlin.process.traversal.step.util;
 
 import org.apache.tinkerpop.gremlin.process.traversal.P;
+import org.apache.tinkerpop.gremlin.process.traversal.step.GType;
 import org.apache.tinkerpop.gremlin.structure.Element;
 import org.apache.tinkerpop.gremlin.structure.Property;
 import org.apache.tinkerpop.gremlin.structure.T;
 import org.apache.tinkerpop.gremlin.structure.util.CloseableIterator;
+import org.apache.tinkerpop.gremlin.process.traversal.step.GValue;
 
 import java.io.Serializable;
 import java.util.Collection;
@@ -154,11 +156,11 @@ private boolean isStringTestable() {
             if (predicateValue instanceof Collection) {
                 final Collection collection = (Collection) predicateValue;
                 if (!collection.isEmpty()) {
-                    return ((Collection) predicateValue).stream().allMatch(c -> null == c || c instanceof String);
+                    return ((Collection) predicateValue).stream().allMatch(c -> null == c || c instanceof String || (c instanceof GValue && ((GValue) c).getType() == GType.STRING));
                 }
             }
 
-            return predicateValue instanceof String;
+            return predicateValue instanceof String || (predicateValue instanceof GValue && ((GValue) predicateValue).getType() == GType.STRING);
         }
 
         return false;

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

@@ -203,7 +203,7 @@ public void shouldCreateGValueFromMapWithName() {
     @Test
     public void shouldCreateGValueFromList() {
         final List<String> list = Arrays.asList("value1", "value2");
-        final GValue<List> gValue = GValue.ofList(list);
+        final GValue<List<String>> gValue = GValue.ofList(list);
         assertEquals(list, gValue.get());
         assertEquals(GType.LIST, gValue.getType());
         assertThat(gValue.isVariable(), is(false));
@@ -212,7 +212,7 @@ public void shouldCreateGValueFromList() {
     @Test
     public void shouldCreateGValueFromListWithName() {
         final List<String> list = Arrays.asList("value1", "value2");
-        final GValue<List> gValue = GValue.ofList("varName", list);
+        final GValue<List<String>> gValue = GValue.ofList("varName", list);
         assertEquals(list, gValue.get());
         assertEquals(GType.LIST, gValue.getType());
         assertEquals("varName", gValue.getName());

tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/step/sideEffect/TinkerGraphStep.java

@@ -71,11 +71,12 @@ private Iterator<? extends Edge> edges() {
         final AbstractTinkerGraph graph = (AbstractTinkerGraph) this.getTraversal().getGraph().get();
         final HasContainer indexedContainer = getIndexKey(Edge.class);
         Iterator<Edge> iterator;
+        final Object[] resolvedIds = this.getResolvedIds();
         // ids are present, filter on them first
-        if (null == this.ids)
+        if (null == resolvedIds)
             iterator = Collections.emptyIterator();
-        else if (this.ids.length > 0)
-            iterator = this.iteratorList(graph.edges(this.getResolvedIds()));
+        else if (resolvedIds.length > 0)
+            iterator = this.iteratorList(graph.edges(resolvedIds));
         else
             iterator = null == indexedContainer ?
                     this.iteratorList(graph.edges()) :
@@ -93,11 +94,12 @@ private Iterator<? extends Vertex> vertices() {
         final AbstractTinkerGraph graph = (AbstractTinkerGraph) this.getTraversal().getGraph().get();
         final HasContainer indexedContainer = getIndexKey(Vertex.class);
         Iterator<? extends Vertex> iterator;
+        final Object[] resolvedIds = this.getResolvedIds();
         // ids are present, filter on them first
-        if (null == this.ids)
+        if (null == resolvedIds)
             iterator = Collections.emptyIterator();
-        else if (this.ids.length > 0)
-            iterator = this.iteratorList(graph.vertices(this.getResolvedIds()));
+        else if (resolvedIds.length > 0)
+            iterator = this.iteratorList(graph.vertices(resolvedIds));
         else
             iterator = (null == indexedContainer ?
                     this.iteratorList(graph.vertices()) :