update concat() to accept traversal varargs and not treat inject as s…

…pecial case in argument (#2306)

CHANGELOG.asciidoc

@@ -29,6 +29,9 @@ This release also includes changes from <<release-3-6-5, 3.6.6>> and <<release-3
 * Added Gherkin parsing support for specific string results using `str[]`.
 * Added the `trim()`, `lTrim()`, `rTrim()`, and `reverse()` steps to perform `String` manipulations.
 * Added `replace()`, `split()` and `substring()` steps to perform `String` manipulations.
+* Update `concat()` to accept `Traversal` varargs.
+* Corrected `concat()` signatures in `gremlin-dotnet`, `Concat()` is now used instead of `Concat<object>()`. *(breaking)*
+* Update `concat()` to not special treat `inject` in arguments and use `TraversalUtil.apply` on it as with any other child traversals. *(breaking)*
 * Checked graph features for meta-property support before trying to serialize them in `VertexPropertySerializer` for GraphBinary.
 * Added date manipulation steps `asDate`, `dateAdd` and `dateDiff`.
 * Added new data type `DT` to represent periods of time.

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

@@ -730,7 +730,7 @@ link:https://tinkerpop.apache.org/docs/x.y.z/reference/#call-step[reference]
 
 *Description:* Concatenates the incoming String traverser with the input String arguments, and return the joined String.
 
-*Syntax:* `concat()` | `concat(String...)` | `concat(Traversal)`
+*Syntax:* `concat()` | `concat(String...)` | `concat(Traversal, Traversal...)`
 
 [width="100%",options="header"]
 |=========================================================
@@ -740,10 +740,12 @@ link:https://tinkerpop.apache.org/docs/x.y.z/reference/#call-step[reference]
 
 *Arguments:*
 
-* `String...` - If one or more String values are provided, they will be concatenated together with the
-incoming traverser. If no argument is provided, the String value from the incoming traverser is returned.
-* `Traversal` - The `Traversal` value must resolve to a `String`. The first result returned from the traversal will be
-concatenated with the incoming traverser.
+* `concatStrings` - Varargs of `String`. If one or more String values are provided, they will be concatenated together
+with the incoming traverser. If no argument is provided, the String value from the incoming traverser is returned.
+* `concatTraveral` - A `Traversal` whose must value resolve to a `String`. The first result returned from the traversal will
+be concatenated with the incoming traverser.
+* `otherConcatTraverals` - Varargs of `Traversal`. Each `Traversal` value must resolve to a `String`. The first result
+returned from each traversal will be concatenated with the incoming traverser and the previous traversal arguments.
 
 Any `null` String values will be skipped when concatenated with non-`null` String values. If two `null` value are
 concatenated, the `null` value will be propagated and returned.

docs/src/reference/the-traversal.asciidoc

@@ -1143,39 +1143,48 @@ link:++https://tinkerpop.apache.org/javadocs/x.y.z/core/org/apache/tinkerpop/gre
 [[concat-step]]
 === Concat Step
 
-The `concat()`-step (*map*) concatenates one or more String values together to the incoming String traverser.
+The `concat()`-step (*map*) concatenates one or more String values together to the incoming String traverser. This step
+can take either String varargs or Traversal varargs.
+Any `null` String values will be skipped when concatenated with non-`null` String values. If two `null` value are
+concatenated, the `null` value will be propagated and returned.
 If the incoming traverser is a non-String value then an `IllegalArgumentException` will be thrown.
 
 [gremlin-groovy,modern]
 ----
-g.addV(constant("prefix_").concat(__.V(1).label())).property(id, 10) <1>
+g.addV(constant('prefix_').concat(__.V(1).label())).property(id, 10) <1>
 g.V(10).label()
 g.V().hasLabel('person').values('name').as('a').
     constant('Mr.').concat(__.select('a')) <2>
 g.V().hasLabel('software').as('a').values('name').
     concat(' uses ').
     concat(select('a').values('lang')) <3>
-g.V(1).outE().as("a").constant("").
-    concat(V(1).values("name")).
-    concat(" ").
-    concat(select("a").label()).
-    concat(" ").
-    concat(select("a").inV().values("name")) <4>
-g.inject('hello', 'hi').concat(__.V().values('name')) <5>
-g.inject('This').concat(' ').concat('is a ', 'gremlin.') <6>
+g.V(1).outE().as('a').V(1).values('name').
+    concat(' ').
+    concat(select('a').label()).
+    concat(' ').
+    concat(select("a").inV().values('name')) <4>
+g.V(1).outE().as('a').V(1).values('name').
+    concat(constant(' '),
+        select("a").label(),
+        constant(' '),
+        select('a').inV().values('name')) <5>
+g.inject('hello', 'hi').concat(__.V().values('name')) <6>
+g.inject('This').concat(' ').concat('is a ', 'gremlin.') <7>
 ----
 
 <1> Add a new vertex with id 10 which should be labeled like an existing vertex but with some prefix attached
 <2> Attach the prefix "Mr." to all the names using the `constant()`-step
 <3> Generate a string of software names and the language they use
 <4> Generate a string description for each of marko's outgoing edges
-<5> The `concat()` step will append the first result from the child traversal to the incoming traverser
-<6> A generic use of `concat()` to join strings together
+<5> Alternative way to generate the string description by using traversal varargs. Use the `constant()` step to add
+desired strings between arguments.
+<6> The `concat()` step will append the first result from the child traversal to the incoming traverser
+<7> A generic use of `concat()` to join strings together
 
 *Additional References*
 
-link:++https://tinkerpop.apache.org/javadocs/x.y.z/core/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.html#concat-String++[`concat(String...)`]
-link:++https://tinkerpop.apache.org/javadocs/x.y.z/core/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.html#concat-Traversal++[`concat(Traversal)`]
+link:++https://tinkerpop.apache.org/javadocs/x.y.z/core/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.html#concat-java.lang.String-++[`concat(String...)`]
+link:++https://tinkerpop.apache.org/javadocs/x.y.z/core/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.html#concat-org.apache.tinkerpop.gremlin.process.traversal.Traversal-org.apache.tinkerpop.gremlin.process.traversal.Traversal...-++[`concat(Taversal, Traversal...)`]
 link:++https://tinkerpop.apache.org/docs/x.y.z/dev/provider/#concat-step++[`Semantics`]
 
 [[connectedcomponent-step]]

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

@@ -30,7 +30,36 @@ complete list of all the modifications that are part of this release.
 === Upgrading for Users
 
 ==== String Manipulation Steps
-Additional String manipulation steps have been added to this version.
+Modifications to the concat() step as well as additional String manipulation steps have been added to this version.
+
+===== Updates to concat():
+Concat has been modified to take traversal varargs instead of a single traversal. Users no longer have to chain
+concat() steps together to concatenate multiple traversals:
+[source,text]
+----
+gremlin> g.V(1).outE().as("a").V(1).values("name").concat(select("a").label(), select("a").inV().values("name"))
+==>markocreatedlop
+==>markoknowsvadas
+==>markoknowsjosh
+----
+
+A notable breaking change from 3.7.0 is that we have output order of `inject()` as a child of `concat()` to be consistent with other parent steps. Any 3.7.0 uses of `concat(inject(X))` should change to `concat(constant(X))` to retain the old semantics.
+[source,text]
+----
+// 3.7.0
+gremlin> g.inject("a").concat(inject("b"))
+==>ab
+
+// 3.7.1
+gremlin> g.inject("a").concat(inject())
+==>aa
+gremlin> g.inject("a").concat(inject("b"))
+==>aa
+gremlin> g.inject("a").concat(constant("b"))
+==>ab
+----
+
+link:https://tinkerpop.apache.org/docs/x.y.z/reference/#concat-step[concat()-step],
 
 ===== String Steps asString(), length(), toLower(), toUpper():
 

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

@@ -923,7 +923,7 @@ protected void notImplemented(final ParseTree ctx) {
 	 * {@inheritDoc}
 	 */
 	@Override
-	public T visitTraversalMethod_concat_Traversal(final GremlinParser.TraversalMethod_concat_TraversalContext ctx) { notImplemented(ctx); return null; }
+	public T visitTraversalMethod_concat_Traversal_Traversal(final GremlinParser.TraversalMethod_concat_Traversal_TraversalContext ctx) { notImplemented(ctx); return null; }
 	/**
 	 * {@inheritDoc}
 	 */

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

@@ -1745,8 +1745,12 @@ public Traversal visitTraversalMethod_call_string_map_traversal(final GremlinPar
      * {@inheritDoc}
      */
     @Override
-    public GraphTraversal visitTraversalMethod_concat_Traversal(final GremlinParser.TraversalMethod_concat_TraversalContext ctx) {
-        return graphTraversal.concat(antlr.tvisitor.visitNestedTraversal(ctx.nestedTraversal()));
+    public GraphTraversal visitTraversalMethod_concat_Traversal_Traversal(final GremlinParser.TraversalMethod_concat_Traversal_TraversalContext ctx) {
+        if (ctx.getChildCount() == 4) {
+            return this.graphTraversal.concat(antlr.tvisitor.visitNestedTraversal(ctx.nestedTraversal()));
+        }
+        return this.graphTraversal.concat(antlr.tvisitor.visitNestedTraversal(ctx.nestedTraversal()),
+                antlr.tListVisitor.visitNestedTraversalList(ctx.nestedTraversalList()));
     }
 
     /**

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

@@ -1432,21 +1432,24 @@ default <E> GraphTraversal<S, E> call(final String service, final Map params, fi
     }
 
     /**
-     * Concatenate strings.
+     * Concatenate values of an arbitrary number of string traversals to the incoming traverser.
      *
      * @return the traversal with an appended {@link ConcatStep}.
+     * @param concatTraversal the traversal to concatenate.
+     * @param otherConcatTraversals additional traversals to concatenate.
      * @see <a href="http://tinkerpop.apache.org/docs/${project.version}/reference/#concat-step" target="_blank">Reference Documentation - Concat Step</a>
-     * @since 3.7.0
+     * @since 3.7.1
      */
-    public default GraphTraversal<S, String> concat(final Traversal<?, String> concatTraversal) {
-        this.asAdmin().getBytecode().addStep(Symbols.concat, concatTraversal);
-        return this.asAdmin().addStep(new ConcatStep<>(this.asAdmin(), concatTraversal));
+    public default GraphTraversal<S, String> concat(final Traversal<?, String> concatTraversal, final Traversal<?, String>... otherConcatTraversals) {
+        this.asAdmin().getBytecode().addStep(Symbols.concat, concatTraversal, otherConcatTraversals);
+        return this.asAdmin().addStep(new ConcatStep<>(this.asAdmin(), concatTraversal, otherConcatTraversals));
     }
 
     /**
-     * Concatenate strings.
+     * Concatenate an arbitrary number of strings to the incoming traverser.
      *
      * @return the traversal with an appended {@link ConcatStep}.
+     * @param concatStrings the String values to concatenate.
      * @see <a href="http://tinkerpop.apache.org/docs/${project.version}/reference/#concat-step" target="_blank">Reference Documentation - Concat Step</a>
      * @since 3.7.0
      */

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

@@ -588,10 +588,10 @@ public static <A> GraphTraversal<A, Double> math(final String expression) {
     }
 
     /**
-     * @see GraphTraversal#concat(Traversal)
+     * @see GraphTraversal#concat(Traversal, Traversal...)
      */
-    public static <A> GraphTraversal<A, String> concat(final Traversal<?, String> concatTraversal) {
-        return __.<A>start().concat(concatTraversal);
+    public static <A> GraphTraversal<A, String> concat(final Traversal<A, String> concatTraversal, final Traversal<A, String>... otherConcatTraversals) {
+        return __.<A>start().concat(concatTraversal, otherConcatTraversals);
     }
 
     /**

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

@@ -21,14 +21,17 @@
 import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
 import org.apache.tinkerpop.gremlin.process.traversal.Traverser;
 import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent;
-import org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect.InjectStep;
 import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement;
 import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalUtil;
 import org.apache.tinkerpop.gremlin.structure.util.StringFactory;
 
+import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 import java.util.Set;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 /**
  * Reference implementation for String concatenation step, a mid-traversal step which concatenates one or more
@@ -39,23 +42,26 @@
  */
 public final class ConcatStep<S> extends ScalarMapStep<S, String> implements TraversalParent {
 
+    private List<Traversal.Admin<S, String>> concatTraversals;
+    private List<String> concatStrings;
     private String stringArgsResult;
 
-    private Traversal.Admin<S, String> concatTraversal;
-
     // flag used to propagate the null value through if all strings to be concatenated are null
     private boolean isNullTraverser = true;
     private boolean isNullTraversal = true;
     private boolean isNullString = true;
 
     public ConcatStep(final Traversal.Admin traversal, final String... concatStrings) {
         super(traversal);
+        this.concatStrings = Collections.unmodifiableList(Arrays.asList(concatStrings));
         this.stringArgsResult = processStrings(concatStrings);
     }
 
-    public ConcatStep(final Traversal.Admin traversal, final Traversal<S, String> concatTraversal) {
+    public ConcatStep(final Traversal.Admin traversal, final Traversal<?, String> concatTraversal, final Traversal<?, String>... otherConcatTraversals) {
         super(traversal);
-        this.concatTraversal = this.integrateChild(concatTraversal.asAdmin());
+        this.concatTraversals = new ArrayList<>(Collections.singletonList((Traversal.Admin<S, String>) concatTraversal.asAdmin()));
+        this.concatTraversals.addAll(Stream.of(otherConcatTraversals).map(ct -> (Traversal.Admin<S, String>) ct.asAdmin()).collect(Collectors.toList()));
+        this.concatTraversals.forEach(this::integrateChild);
     }
 
     @Override
@@ -74,18 +80,14 @@ protected String map(final Traverser.Admin<S> traverser) {
             sb.append(traverser.get());
         }
 
-        if (null != this.concatTraversal) {
-            if (this.concatTraversal.getStartStep() instanceof InjectStep) { // inject as start step is processed separately
-                if (this.concatTraversal.hasNext()) {
-                    final String result = this.concatTraversal.next();
-                    if (null != result) {
-                        this.isNullTraversal = false;
-                        sb.append(result);
-                    }
+        if (null != this.concatTraversals) {
+            this.concatTraversals.forEach(ct -> {
+                final String result = TraversalUtil.apply(traverser, ct);
+                if (null != result) {
+                    this.isNullTraversal = false;
+                    sb.append(result);
                 }
-            } else {
-                sb.append(TraversalUtil.apply(traverser, this.concatTraversal));
-            }
+            });
         }
 
         if (!this.isNullString) {
@@ -117,40 +119,59 @@ private String processStrings(final String[] concatStrings) {
 
     @Override
     public Set<TraverserRequirement> getRequirements() {
-        return Collections.singleton(TraverserRequirement.OBJECT);
+        return this.getSelfAndChildRequirements(TraverserRequirement.OBJECT);
     }
 
     @Override
     public void setTraversal(final Traversal.Admin<?, ?> parentTraversal) {
         super.setTraversal(parentTraversal);
-        this.integrateChild(this.concatTraversal);
+        if (null != this.concatTraversals)
+            for (final Traversal.Admin<S, String> traversal : this.concatTraversals) {
+                this.integrateChild(traversal);
+            }
     }
 
     @Override
     public ConcatStep<S> clone() {
         final ConcatStep<S> clone = (ConcatStep<S>) super.clone();
-        if (null != this.concatTraversal)
-            clone.concatTraversal = this.concatTraversal.clone();
+        if (null != this.concatTraversals) {
+            clone.concatTraversals = new ArrayList<>();
+            for (final Traversal.Admin<S, String> concatTraversals : this.concatTraversals) {
+                clone.concatTraversals.add(concatTraversals.clone());
+            }
+        }
+        if (null != this.concatStrings) {
+            clone.concatStrings = new ArrayList<>();
+            clone.concatStrings.addAll(this.concatStrings);
+        }
         return clone;
     }
 
     @Override
     public List<Traversal.Admin<S, String>> getLocalChildren() {
-        return null == this.concatTraversal ? Collections.emptyList() : Collections.singletonList(this.concatTraversal);
+        return null == this.concatTraversals ? Collections.emptyList() : this.concatTraversals;
     }
 
     @Override
     public String toString() {
-        return StringFactory.stepString(this, this.concatTraversal);
+        if (null != this.concatTraversals)
+            return StringFactory.stepString(this, this.concatTraversals);
+        return StringFactory.stepString(this, this.concatStrings);
     }
 
     @Override
     public int hashCode() {
         int result = super.hashCode();
-        if (null != this.concatTraversal)
-            result = super.hashCode() ^ this.concatTraversal.hashCode();
-        if (null != this.stringArgsResult)
-            result = super.hashCode() ^ this.stringArgsResult.hashCode();
+        if (null != this.concatTraversals) {
+            for (final Traversal t : this.concatTraversals) {
+                result = 31 * result + (null != t ? t.hashCode() : 0);
+            }
+        }
+        if (null != this.concatStrings) {
+            for (final String s : this.concatStrings) {
+                result = 31 * result + (null != s ? s.hashCode() : 0);
+            }
+        }
         return result;
     }
 }

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

@@ -528,7 +528,6 @@ static final class SymbolHelper {
             TO_CS_MAP.put(GraphTraversal.Symbols.cap, "Cap<object>");
             TO_CS_MAP.put(GraphTraversal.Symbols.choose, "Choose<object>");
             TO_CS_MAP.put(GraphTraversal.Symbols.coalesce, "Coalesce<object>");
-            TO_CS_MAP.put(GraphTraversal.Symbols.concat, "Concat<object>");
             TO_CS_MAP.put(GraphTraversal.Symbols.constant, "Constant<object>");
             TO_CS_MAP.put(GraphTraversal.Symbols.elementMap, "ElementMap<object>");
             TO_CS_MAP.put(GraphTraversal.Symbols.flatMap, "FlatMap<object>");

gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/language/grammar/TraversalMethodVisitorTest.java

@@ -1153,6 +1153,34 @@ public void shouldParseTraversalMethod_midTraversal_E_multipleArgs_spawning() th
                 eval("g.V().coalesce(__.E(1,2),__.addE('person'))"));
     }
 
+    @Test
+    public void shouldParseTraversalMethod_concat_Empty() throws Exception {
+        compare(g.V().concat(), eval("g.V().concat()"));
+    }
+
+    @Test
+    public void shouldParseTraversalMethod_concat_multipleStringArgs() throws Exception {
+        compare(g.V().concat("hello", "world"), eval("g.V().concat('hello', 'world')"));
+    }
+
+    @Test
+    public void shouldParseTraversalMethod_concat_traversal() throws Exception {
+        compare(g.V().concat(constant("hello")),
+                eval("g.V().concat(__.constant('hello'))"));
+    }
+
+    @Test
+    public void shouldParseTraversalMethod_concat_multipleTraversalArgs() throws Exception {
+        compare(g.V().concat(constant("hello"), constant("world")),
+                eval("g.V().concat(__.constant('hello'), __.constant('world'))"));
+    }
+
+    @Test
+    public void shouldParseTraversalMethod_concat_ArgsWithNulls() throws Exception {
+        compare(g.V().concat(null, "hello"),
+                eval("g.V().concat(null, 'hello')"));
+    }
+
     @Test
     public void shouldParseTraversalMethod_asString_Empty() throws Exception {
         compare(g.V().asString(), eval("g.V().asString()"));