Remove GenericArgument from from() and to() (#3252)

from() and to() will now only accept Strings (alias for __.select()) and Traversals. If users wish to pass a vertex id, they may embed it in __.V() or __.constant().

The primary motivation here is to ensure consistent behaviour accross providers regardless of if their id types are numerics, strings, or other.

This change is a followup to an unintended consequence stemming from #3133. The issue with directly allowing ID's to be passed into from/to, it creates a weird asymmetry for graphs which use Strings as their ids. For example g.addE().from(1).to(2) would work fine and create an edge from V[1] to V[2], however if the graph used String ids, the equivalent traversal (g.addE().from("a").to("b")) does not work, as the Strings are interpreted as path labels (converted to __.select("a")) instead of vertex ids. There are some ugly workarounds which can partially mitigate this issue, but overall there is no generalized fix which allows all traversals to behave the same regardless of the vertex id type.

---------

Co-authored-by: andreachild <andrea.child@improving.com>

Author: Cole-Greer

docs/src/dev/provider/gremlin-semantics.asciidoc

@@ -535,6 +535,12 @@ The `addE()` step can be used as both a start step and a mid-traversal step. Whe
 and `to()` must be specified. When used as a mid-traversal step, the current traverser becomes the source vertex and
 only `to()` needs to be specified.
 
+The gremlin-lang grammar only permits `Traversal` and `String` (alias for `__.select(String)`) arguments in `from()` and
+`to()`. The `Traversal` must either produce a `Vertex` which is attachable to the graph, or it must produce the id of an
+existing `Vertex` in the graph. `GraphTraversal` implementations in GLVs may optionally support `from(Vertex)` and
+`to(Vertex)` as syntactic sugar. If translating to gremlin-lang scripts, these sugared modulators must be converted to
+`from(__.V(vertex.id()))` or `from(__.constant(vertex.id()))` (and equivalents for `to()`).
+
 *Exceptions*
 
 * If the edge label is null, an `IllegalArgumentException` will be thrown.

docs/src/reference/the-traversal.asciidoc

@@ -553,6 +553,7 @@ vMarko = g.V().has('name','marko').next()
 vPeter = g.V().has('name','peter').next()
 g.V(vMarko).addE('knows').to(vPeter) <6>
 g.addE('knows').from(vMarko).to(vPeter) <7>
+g.addE('knows').from(__.V(1)).to(__.constant(6)) <8>
 ----
 
 <1> Add a co-developer edge with a year-property between marko and his collaborators.
@@ -563,6 +564,7 @@ g.addE('knows').from(vMarko).to(vPeter) <7>
 supports user provided ids.
 <6> Add an edge between marko and peter given the directed (detached) vertex references.
 <7> Add an edge between marko and peter given the directed (detached) vertex references.
+<8> Use child traversals producing either a vertex, or vertex id to add an edge between marko and peter.
 
 *Additional References*
 

docs/src/upgrade/release-3.8.x.asciidoc

@@ -737,41 +737,44 @@ after construction. All usages of `P.getOriginalValue()` should be replaced with
 
 A number of changes have been introduced to the Gremlin grammar to help make it be more consistent and easier to use.
 
-*Removed Vertex References for Grammar*
+*Removed StructureVertex from Grammar*
 
 The grammar allowed the construction of a `Vertex` by way of syntax like `new Vertex(1,'person')` (or with similar
 arguments to `ReferenceVertex`). This syntax has been removed as it served little purpose within the grammar as it
 merely adds more characters to wrap around the identifier, which could simply be used by itself.
 
 The `V()` step, as well as the `from()` and `to()` modulators used with `addE()`, previously accepted `Vertex` as
-arguments in the grammar. In its place, the `from()` and `to()` modulators can now directly accept a vertex id in place
-of a `Vertex` when used with `addE()` (`V()` has always accepted ids in addition to vertices). When using these steps in
-`gremlin-lang` scripts, the vertex id must be used directly.
+arguments in the grammar. The `V()` step has always accepted vertex ids as arguments, and continues to do so. The
+`from()` and `to()` modulators for `addE()` continue to accept `String` arguments (which are a shorthand for
+`__.select(String)`), as well as `Traversal` arguments. As always, these `Traversal` arguments may produce `Vertex`
+objects (such as `__.V(1)`) to directly bind to from/to. Newly added in 3.8.0, the `Traversal` may also produce the id
+of a vertex present in the graph (such as `__.constant(1)`), which will then bind to from/to.
 
-This change has no effect on the `GraphTraversal` API, nor on `gremlin-groovy` scripts. Vertices can continue to be used
-directly in those contexts.
+When using these steps in `gremlin-lang` scripts, a `Traversal` or `String` argument must be used directly. This change
+has no effect on the `GraphTraversal` API, nor on `gremlin-groovy` scripts. Vertices can continue to be used directly in
+those contexts.
 
 [source,text]
 ----
-// 3.7.3
+// 3.7.4
 gremlin> v1 = g.V(1).next()
 ==>v[1]
-gremlin> v2 = g.V(2).next()
-==>v[2]
-gremlin> script = String.format("g.V(new Vertex(%s)).outE().where(inV().is(new Vertex(%s)))", v1.id(), v2.id())
-==>g.V(new Vertex(1)).outE().where(inV().is(new Vertex(2)))
+gremlin> v6 = g.V(6).next()
+==>v[6]
+gremlin> script = String.format("g.addE('knows').from(new Vertex(%s)).to(new ReferenceVertex(%s))", v1.id(), v6.id())
+==>g.addE('knows').from(new Vertex(1)).to(new ReferenceVertex(6))
 gremlin> client.submit(script).all().get().get(0).getEdge()
-==>e[7][1-knows->2]
+==>e[0][1-knows->6]
 
 // 3.8.0
 gremlin> v1 = g.V(1).next()
 ==>v[1]
-gremlin> v2 = g.V(2).next()
-==>v[2]
-gremlin> script = String.format("g.V(%s).outE().where(inV().id().is(%s))", v1.id(), v2.id())
-==>g.V(1).outE().where(inV().id().is(2))
+gremlin> v6 = g.V(6).next()
+==>v[6]
+gremlin> script = String.format("g.addE('knows').from(__.V(%s)).to(__.constant(%s))", v1.id(), v6.id())
+==>g.addE('knows').from(__.V(1)).to(__.constant(6))
 gremlin> client.submit(script).all().get().get(0).getEdge()
-==>e[7][1-knows->2]
+==>e[0][1-knows->6]
 ----
 
 *`new` keyword is now optional*

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/language/grammar/DefaultGremlinBaseVisitor.java

@@ -403,10 +403,6 @@ protected void notImplemented(final ParseTree ctx) {
 	 * {@inheritDoc}
 	 */
 	@Override public T visitTraversalMethod_from_String(final GremlinParser.TraversalMethod_from_StringContext ctx) { notImplemented(ctx); return null; }
-	/**
-	 * {@inheritDoc}
-	 */
-	@Override public T visitTraversalMethod_from_GenricArgument(final GremlinParser.TraversalMethod_from_GenricArgumentContext ctx) { return null; }
 	/**
 	 * {@inheritDoc}
 	 */
@@ -839,10 +835,6 @@ protected void notImplemented(final ParseTree ctx) {
 	 * {@inheritDoc}
 	 */
 	@Override public T visitTraversalMethod_to_String(final GremlinParser.TraversalMethod_to_StringContext ctx) { notImplemented(ctx); return null; }
-	/**
-	 * {@inheritDoc}
-	 */
-	@Override public T visitTraversalMethod_to_GenricArgument(final GremlinParser.TraversalMethod_to_GenricArgumentContext ctx) { notImplemented(ctx); return null; }
 	/**
 	 * {@inheritDoc}
 	 */

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/language/grammar/TraversalMethodVisitor.java

@@ -1865,22 +1865,6 @@ public Traversal visitTraversalMethod_option_Predicate_Traversal(final GremlinPa
                 antlr.tvisitor.visitNestedTraversal(ctx.nestedTraversal()));
     }
 
-    /**
-     * {@inheritDoc}
-     */
-    @Override
-    public GraphTraversal visitTraversalMethod_from_GenricArgument(final GremlinParser.TraversalMethod_from_GenricArgumentContext ctx) {
-        return graphTraversal.from(antlr.argumentVisitor.visitGenericArgument(ctx.genericArgument()));
-    }
-
-    /**
-     * {@inheritDoc}
-     */
-    @Override
-    public Traversal visitTraversalMethod_to_GenricArgument(final GremlinParser.TraversalMethod_to_GenricArgumentContext ctx) {
-        return graphTraversal.to(antlr.argumentVisitor.visitGenericArgument(ctx.genericArgument()));
-    }
-
     /**
      * {@inheritDoc}
      */

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

@@ -1603,14 +1603,14 @@ public default GraphTraversal<S, E> from(final String fromStepLabel) {
      * @see <a href="http://tinkerpop.apache.org/docs/${project.version}/reference/#addedge-step" target="_blank">Reference Documentation - From Step</a>
      * @since 3.8.0
      */
-    public default GraphTraversal<S, E> from(final GValue<Vertex> fromVertex) {
+    public default GraphTraversal<S, E> from(final GValue<?> fromVertex) {
         final Step<?,?> prev = this.asAdmin().getEndStep();
         if (!(prev instanceof FromToModulating))
             throw new IllegalArgumentException(String.format(
                     "The from() step cannot follow %s", prev.getClass().getSimpleName()));
 
         this.asAdmin().getBytecode().addStep(Symbols.from, fromVertex);
-        ((FromToModulating) prev).addFrom(new GValueConstantTraversal<S, Vertex>(fromVertex));
+        ((FromToModulating) prev).addFrom(new GValueConstantTraversal<>(fromVertex));
         return this;
     }
 
@@ -1662,14 +1662,14 @@ public default GraphTraversal<S, E> to(final String toStepLabel) {
      * @see <a href="http://tinkerpop.apache.org/docs/${project.version}/reference/#addedge-step" target="_blank">Reference Documentation - From Step</a>
      * @since 3.8.0
      */
-    public default GraphTraversal<S, E> to(final GValue<Vertex> toVertex) {
+    public default GraphTraversal<S, E> to(final GValue<?> toVertex) {
         final Step<?,?> prev = this.asAdmin().getEndStep();
         if (!(prev instanceof FromToModulating))
             throw new IllegalArgumentException(String.format(
                     "The to() step cannot follow %s", prev.getClass().getSimpleName()));
 
         this.asAdmin().getBytecode().addStep(Symbols.to, toVertex);
-        ((FromToModulating) prev).addTo(new GValueConstantTraversal<S, Vertex>(toVertex));
+        ((FromToModulating) prev).addTo(new GValueConstantTraversal<>(toVertex));
         return this;
     }
 
@@ -1682,7 +1682,7 @@ public default GraphTraversal<S, E> to(final GValue<Vertex> toVertex) {
      * @see <a href="http://tinkerpop.apache.org/docs/${project.version}/reference/#addedge-step" target="_blank">Reference Documentation - From Step</a>
      * @since 3.1.0-incubating
      */
-    public default GraphTraversal<S, E> to(final Traversal<?, Object> toVertex) {
+    public default GraphTraversal<S, E> to(final Traversal<?, ?> toVertex) {
         final Step<?,?> prev = this.asAdmin().getEndStep();
         if (!(prev instanceof FromToModulating))
             throw new IllegalArgumentException(String.format(
@@ -1702,7 +1702,7 @@ public default GraphTraversal<S, E> to(final Traversal<?, Object> toVertex) {
      * @see <a href="http://tinkerpop.apache.org/docs/${project.version}/reference/#addedge-step" target="_blank">Reference Documentation - From Step</a>
      * @since 3.1.0-incubating
      */
-    public default GraphTraversal<S, E> from(final Traversal<?, Object> fromVertex) {
+    public default GraphTraversal<S, E> from(final Traversal<?, ?> fromVertex) {
         final Step<?,?> prev = this.asAdmin().getEndStep();
         if (!(prev instanceof FromToModulating))
             throw new IllegalArgumentException(String.format(
@@ -1722,13 +1722,7 @@ public default GraphTraversal<S, E> from(final Traversal<?, Object> fromVertex)
      * @see <a href="http://tinkerpop.apache.org/docs/${project.version}/reference/#addedge-step" target="_blank">Reference Documentation - From Step</a>
      * @since 3.3.0
      */
-    public default GraphTraversal<S, E> to(final Object toVertex) {
-        if (toVertex instanceof String) {
-            return this.to((String) toVertex);
-        } else if (toVertex instanceof Traversal) {
-            this.to((Traversal<?, Object>)toVertex);
-            return this;
-        }
+     public default GraphTraversal<S, E> to(final Vertex toVertex) {
         final Step<?,?> prev = this.asAdmin().getEndStep();
         if (!(prev instanceof FromToModulating))
             throw new IllegalArgumentException(String.format(
@@ -1741,34 +1735,6 @@ public default GraphTraversal<S, E> to(final Object toVertex) {
         return this;
     }
 
-    /**
-     * When used as a modifier to {@link #addE(String)} this method specifies the traversal to use for selecting the
-     * outgoing vertex of the newly added {@link Edge}.
-     *
-     * @param fromVertex the vertex for selecting the outgoing vertex
-     * @return the traversal with the modified {@link AddEdgeStepContract}
-     * @see <a href="http://tinkerpop.apache.org/docs/${project.version}/reference/#addedge-step" target="_blank">Reference Documentation - From Step</a>
-     * @since 3.3.0
-     */
-    public default GraphTraversal<S, E> from(final Object fromVertex) {
-        if (fromVertex instanceof String) {
-            return this.from((String) fromVertex);
-        } else if (fromVertex instanceof Traversal) {
-            this.from((Traversal<?, Object>)fromVertex);
-            return this;
-        }
-        final Step<?,?> prev = this.asAdmin().getEndStep();
-        if (!(prev instanceof FromToModulating))
-            throw new IllegalArgumentException(String.format(
-                    "The from() step cannot follow %s", prev.getClass().getSimpleName()));
-
-        this.asAdmin().getBytecode().addStep(Symbols.from, fromVertex);
-        ((FromToModulating) prev).addFrom(fromVertex instanceof GValue ?
-                new GValueConstantTraversal<>((GValue<Object>) fromVertex) :
-                __.constant(fromVertex).asAdmin());
-        return this;
-    }
-
     /**
      * Map the {@link Traverser} to a {@link Double} according to the mathematical expression provided in the argument.
      *

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

@@ -20,7 +20,6 @@
 
 import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
 import org.apache.tinkerpop.gremlin.process.traversal.Traverser;
-import org.apache.tinkerpop.gremlin.process.traversal.lambda.ConstantTraversal;
 import org.apache.tinkerpop.gremlin.process.traversal.lambda.GValueConstantTraversal;
 import org.apache.tinkerpop.gremlin.process.traversal.step.GValue;
 import org.apache.tinkerpop.gremlin.process.traversal.step.GValueHolder;

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

@@ -207,12 +207,6 @@ public static Iterable<Object[]> data() {
                         List.of(__.constant("name"), __.constant("cole")),
                         null, null
                 },
-                {AddEdgeStepContract.class,
-                        g.addE("label").from(1).to(2).property("name", __.constant("cole")),
-                        List.of(),
-                        List.of(__.constant("cole"), __.constant(1), __.constant(2)),
-                        null, null
-                },
                 {AddEdgeStepContract.class,
                         g.addE("label").from(__.V(1)).to(__.V(2))
                                 .property("name", __.constant("cole"))
@@ -234,15 +228,9 @@ public static Iterable<Object[]> data() {
                         null, null
                 },
                 {AddEdgeStepContract.class,
-                        g.addE("label").from(1).to(2).property(__.constant("name"), __.constant("cole")),
-                        List.of(),
-                        List.of(__.constant("name"), __.constant("cole"), __.constant(1), __.constant(2)),
-                        null, null
-                },
-                {AddEdgeStepContract.class,
-                        g.inject(1).addE("label").from(1).to(2).property("name", __.constant("cole")),
+                        g.addE("label").from(__.V(1)).to(__.V(2)).property(__.constant("name"), __.constant("cole")),
                         List.of(),
-                        List.of(__.constant("cole"), __.constant(1), __.constant(2)),
+                        List.of(__.constant("name"), __.constant("cole"), __.V(1), __.V(2)),
                         null, null
                 },
                 {AddEdgeStepContract.class,
@@ -258,9 +246,9 @@ public static Iterable<Object[]> data() {
                         null, null
                 },
                 {AddEdgeStepContract.class,
-                        g.inject(1).addE("label").from(1).to(2).property(__.constant("name"), __.constant("cole")),
+                        g.inject(1).addE("label").from(__.V(1)).to(__.V(2)).property(__.constant("name"), __.constant("cole")),
                         List.of(),
-                        List.of(__.constant("name"), __.constant("cole"), __.constant(1), __.constant(2)),
+                        List.of(__.constant("name"), __.constant("cole"), __.V(1), __.V(2)),
                         null, null
                 },
                 {CallStepContract.class,

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

@@ -29,6 +29,7 @@
 import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.DefaultGraphTraversal;
 import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversal;
 import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource;
+import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__;
 import org.apache.tinkerpop.gremlin.process.traversal.step.GValue;
 import org.apache.tinkerpop.gremlin.process.traversal.step.GValueStepTest;
 import org.apache.tinkerpop.gremlin.structure.Edge;
@@ -55,7 +56,7 @@ protected List<Traversal> getTraversals() {
                 g.addE("created").property("a", "b"),
                 g.addE("knows").property("a", "b").property("c", "e"),
                 g.addE("knows").property("c", "e"),
-                g.addE("knows").from(1).to(2).property("a", "b"),
+                g.addE("knows").from(__.V(1)).to(__.V(2)).property("a", "b"),
                 g.addE(GValue.of("label", "knows")).property("a", "b"),
                 g.addE(GValue.of("label", "created")).property("a", GValue.of("prop", "b")),
                 g.addE(GValue.of("label", "knows")).property("a", GValue.of("prop1", "b")).property("c", GValue.of("prop2", "e")),
@@ -211,10 +212,10 @@ public void getGValuesShouldReturnAllGValues() {
     }
 
     @Test
-    public void getGValuesNonShouldReturnEmptyCollection() {
+    public void getGValuesNoneShouldReturnEmptyCollection() {
         GraphTraversal.Admin<Edge, Edge> traversal = g.addE("likes")
-                .from(1)
-                .to(2)
+                .from(__.V(1))
+                .to(__.V(2))
                 .property(T.id, "1234")
                 .property("rating", "great")
                 .asAdmin();

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

@@ -60,7 +60,7 @@ protected List<Traversal> getTraversals() {
                 __.addE("created").property("a", "b"),
                 __.addE("knows").property("a", "b").property("c", "e"),
                 __.addE("knows").property("c", "e"),
-                __.addE("knows").from(1).to(2).property("a", "b"),
+                __.addE("knows").from(__.V(1)).to(__.V(2)).property("a", "b"),
                 __.addE(GValue.of("label", "knows")).property("a", "b"),
                 __.addE(GValue.of("label", "created")).property("a", GValue.of("prop", "b")),
                 __.addE(GValue.of("label", "knows")).property("a", GValue.of("prop1", "b")).property("c", GValue.of("prop2", "e")),
@@ -304,10 +304,10 @@ public void getGValuesShouldReturnAllGValues() {
     }
 
     @Test
-    public void getGValuesNonShouldReturnEmptyCollection() {
+    public void getGValuesNoneShouldReturnEmptyCollection() {
         GraphTraversal.Admin<Object, Edge> traversal = __.addE("likes")
-                .from(1)
-                .to(2)
+                .from(__.V(1))
+                .to(__.V(2))
                 .property(T.id, "1234")
                 .property("rating", "great")
                 .asAdmin();