Merge pull request #2036 from apache/TINKERPOP-2931

Added check for illegal hidden keys, & refactored searchVertices to allow subclasses to override search criteria.
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java
index 0bd44c9..b1e3875 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java
@@ -391,7 +391,7 @@
             return (Vertex) o;
         else if (o instanceof Map) {
             final Map search = (Map) o;
-            final Vertex v = IteratorUtils.findFirst(MergeVertexStep.searchVertices(getGraph(), search)).get();
+            final Vertex v = IteratorUtils.findFirst(searchVertices(search)).get();
             return tryAttachVertex(v);
         }
         throw new IllegalArgumentException(
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeStep.java
index b1ca557..cfa53ca 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeStep.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeStep.java
@@ -31,6 +31,7 @@
 import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
 import org.apache.tinkerpop.gremlin.process.traversal.Traverser;
 import org.apache.tinkerpop.gremlin.process.traversal.TraverserGenerator;
+import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversal;
 import org.apache.tinkerpop.gremlin.process.traversal.lambda.ConstantTraversal;
 import org.apache.tinkerpop.gremlin.process.traversal.lambda.IdentityTraversal;
 import org.apache.tinkerpop.gremlin.process.traversal.step.Deleting;
@@ -45,6 +46,9 @@
 import org.apache.tinkerpop.gremlin.structure.Direction;
 import org.apache.tinkerpop.gremlin.structure.Graph;
 import org.apache.tinkerpop.gremlin.structure.T;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.apache.tinkerpop.gremlin.structure.util.CloseableIterator;
+import org.apache.tinkerpop.gremlin.structure.util.ElementHelper;
 import org.apache.tinkerpop.gremlin.structure.util.StringFactory;
 
 /**
@@ -248,6 +252,8 @@
             if (ignoreTokens) {
                 if (!(k instanceof String)) {
                     throw new IllegalArgumentException(String.format("option(onMatch) expects keys in Map to be of String - check: %s", k));
+                } else {
+                    ElementHelper.validateProperty((String) k, v);
                 }
             } else {
                 if (!(k instanceof String) && !allowedTokens.contains(k)) {
@@ -255,10 +261,14 @@
                             "%s() and option(onCreate) args expect keys in Map to be either String or %s - check: %s",
                             op, allowedTokens, k));
                 }
-                if (k == T.label && !(v instanceof String)) {
-                    throw new IllegalArgumentException(String.format("%s() and option(onCreate) args expect T.label value to be of String - found: %s",
-                            op, v.getClass().getSimpleName()));
-
+                if (k == T.label) {
+                    if (!(v instanceof String)) {
+                        throw new IllegalArgumentException(String.format(
+                                "%s() and option(onCreate) args expect T.label value to be of String - found: %s", op,
+                                v.getClass().getSimpleName()));
+                    } else {
+                        ElementHelper.validateLabel((String) v);
+                    }
                 }
                 if (k == Direction.OUT && v instanceof Merge && v != Merge.outV) {
                     throw new IllegalArgumentException(String.format("Only Merge.outV token may be used for Direction.OUT, found: %s", v));
@@ -266,6 +276,9 @@
                 if (k == Direction.IN && v instanceof Merge && v != Merge.inV) {
                     throw new IllegalArgumentException(String.format("Only Merge.inV token may be used for Direction.IN, found: %s", v));
                 }
+                if (k instanceof String) {
+                    ElementHelper.validateProperty((String) k, v);
+                }
             }
         });
     }
@@ -292,6 +305,45 @@
         return map == null ? new LinkedHashMap() : map;
     }
 
+    /**
+     * Translate the Map into a g.V() traversal against the supplied graph. Graph providers will presumably optimize
+     * this traversal to use whatever indices are present and appropriate for efficiency.
+     *
+     * Callers are responsible for closing this iterator when finished.
+     */
+    protected CloseableIterator<Vertex> searchVertices(final Map search) {
+        if (search == null)
+            return CloseableIterator.empty();
+
+        final Graph graph = getGraph();
+        final Object id = search.get(T.id);
+        final String label = (String) search.get(T.label);
+
+        GraphTraversal t = searchVerticesTraversal(graph, id);
+        t = searchVerticesLabelConstraint(t, label);
+        t = searchVerticesPropertyConstraints(t, search);
+
+        // this should auto-close the underlying traversal
+        return CloseableIterator.of(t);
+    }
+
+    protected GraphTraversal searchVerticesTraversal(final Graph graph, final Object id) {
+        return id != null ? graph.traversal().V(id) : graph.traversal().V();
+    }
+
+    protected GraphTraversal searchVerticesLabelConstraint(GraphTraversal t, final String label) {
+        return label != null ? t.hasLabel(label) : t;
+    }
+
+    protected GraphTraversal searchVerticesPropertyConstraints(GraphTraversal t, final Map search) {
+        for (final Map.Entry e : ((Map<?,?>) search).entrySet()) {
+            final Object k = e.getKey();
+            if (!(k instanceof String)) continue;
+            t = t.has((String) k, e.getValue());
+        }
+        return t;
+    }
+
     @Override
     protected abstract Iterator<E> flatMap(final Traverser.Admin<S> traverser);
 
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java
index 70b45644..1408cfb 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java
@@ -74,46 +74,6 @@
         return allowedTokens;
     }
 
-    /**
-     * Translate the Map into a g.V() traversal against the supplied graph. Graph providers will presumably optimize
-     * this traversal to use whatever indices are present and appropriate for efficiency.
-     *
-     * Callers are responsible for closing this iterator when finished.
-     */
-    public static CloseableIterator<Vertex> searchVertices(final Graph graph, final Map search) {
-        if (search == null)
-            return CloseableIterator.empty();
-
-        final Object id = search.get(T.id);
-        final String label = (String) search.get(T.label);
-
-        GraphTraversal t = id != null ? graph.traversal().V(id) : graph.traversal().V();
-
-        if (label != null)
-            t = t.hasLabel(label);
-
-        // add property constraints
-        for (final Map.Entry e : ((Map<?,?>) search).entrySet()) {
-            final Object k = e.getKey();
-            if (!(k instanceof String)) continue;
-            t = t.has((String) k, e.getValue());
-        }
-
-        // this should auto-close the underlying traversal
-        return CloseableIterator.of(t);
-    }
-
-    /**
-     * Translate the Map into search criteria. Default implementation is to translate the Map into a g.V() traversal.
-     * Graph providers will presumably optimize this traversal to use whatever indices are present and appropriate for
-     * efficiency.
-     *
-     * Callers are responsible for closing this iterator when finished.
-     */
-    protected CloseableIterator<Vertex> searchVertices(final Map search) {
-        return searchVertices(getGraph(), search);
-    }
-
     @Override
     protected Iterator<Vertex> flatMap(final Traverser.Admin<S> traverser) {
         final Graph graph = getGraph();