[TINKERPOP-3149] Prevent multiple by modulators for sack step and changed GroupCount.feature test to verify the error message received. (#3102)

Author: andreachild

CHANGELOG.asciidoc

@@ -66,7 +66,7 @@ This release also includes changes from <<release-3-7-XXX, 3.7.XXX>>.
 * Added missing strategies in `gremlin-go`, updated certain strategies to take varargs and updated `GoTranslatorVisitor` for corresponding translations.
 * Fixed `BigInt` and `BigDecimal` parsing in `gremlin-go` cucumber test suite, fixed `UnscaledValue` type in `BigDecimal` struct and added `ParseBigDecimal` method.
 * Added validation to `valueMap()`/`propertyMap()`/`groupCount()` to prevent an invalid usage of multiple `by()` modulators.
-* Added validation to `groupCount()` to prevent an invalid usage of multiple `by()` modulators.
+* Added validation to `groupCount()` and `sack()` to prevent an invalid usage of multiple `by()` modulators.
 * Deprecated `ProcessLimitedStandardSuite` and `ProcessLimitedComputerSuite` in favor of `ProcessEmbeddedStandardSuite` and `ProcessEmbeddedComputerSuite` respectively.
 * Deprecated `ProcessStandardSuite` and the `ProcessComputerSuite` in favor of Gherkin testing and the `ProcessEmbeddedStandardSuite` and `ProcessEmbeddedComputerSuite` for testing JVM-specific Gremlin behaviors.
 * Removed lambda oriented Gremlin testing from Gherkin test suite.

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

@@ -51,6 +51,9 @@ public SackValueStep(final Traversal.Admin traversal, final BiFunction<A, B, A>
 
     @Override
     public void modulateBy(final Traversal.Admin<?, ?> sackTraversal) {
+        if (this.sackTraversal != null) {
+            throw new IllegalStateException("Sack step can only have one by modulator");
+        }
         this.sackTraversal = this.integrateChild(sackTraversal);
     }
 

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

@@ -27,6 +27,7 @@
 import java.util.Arrays;
 import java.util.List;
 import java.util.Map;
+import org.junit.Test;
 
 /**
  * @author Daniel Kuppitz (http://gremlin.guru)
@@ -49,4 +50,9 @@ protected List<Traversal> getTraversals() {
                 })
         );
     }
+
+    @Test(expected = IllegalStateException.class)
+    public void shouldThrowForMultipleByModulators() {
+        __.sack(Operator.assign).by("age").by("name");
+    }
 }

gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/Gremlin.cs

@@ -1640,6 +1640,7 @@ private static IDictionary<string, List<Func<GraphTraversalSource, IDictionary<s
                {"g_V_sackXassignX_byXageX_sack", new List<Func<GraphTraversalSource, IDictionary<string, object>, ITraversal>> {(g,p) =>g.V().Sack(Operator.Assign).By("age").Sack<object>()}}, 
                {"g_withSackXBigInteger_TEN_powX1000X_assignX_V_localXoutXknowsX_barrierXnormSackXX_inXknowsX_barrier_sack", new List<Func<GraphTraversalSource, IDictionary<string, object>, ITraversal>> {(g,p) =>g.WithSack(p["xx1"], Operator.Assign).V().Local<object>(__.Out("knows").Barrier(Barrier.NormSack)).In("knows").Barrier().Sack<object>()}}, 
                {"g_withSackX2X_V_sackXdivX_byXconstantX4_0XX_sack", new List<Func<GraphTraversalSource, IDictionary<string, object>, ITraversal>> {(g,p) =>g.WithSack(2).V().Sack(Operator.Div).By(__.Constant<object>(p["xx1"])).Sack<object>()}}, 
+               {"g_V_sackXassignX_byXageX_byXnameX_sack", new List<Func<GraphTraversalSource, IDictionary<string, object>, ITraversal>> {(g,p) =>g.V().Sack(Operator.Assign).By("age").By("name").Sack<object>()}}, 
                {"g_V_sideEffectXidentityX", new List<Func<GraphTraversalSource, IDictionary<string, object>, ITraversal>> {(g,p) =>g.V().SideEffect(__.Identity())}}, 
                {"g_V_sideEffectXidentity_valuesXnameXX", new List<Func<GraphTraversalSource, IDictionary<string, object>, ITraversal>> {(g,p) =>g.V().SideEffect(__.Identity().Values<object>("name"))}}, 
                {"g_V_sideEffectXpropertyXage_22X", new List<Func<GraphTraversalSource, IDictionary<string, object>, ITraversal>> {(g,p) =>g.AddV("person").Property("age", 21), (g,p) =>g.V().SideEffect(__.Property("age", 22)), (g,p) =>g.V().Has("age", 21), (g,p) =>g.V().Has("age", 22)}}, 

gremlin-go/driver/cucumber/gremlin.go

@@ -1609,6 +1609,7 @@ var translationMap = map[string][]func(g *gremlingo.GraphTraversalSource, p map[
     "g_V_sackXassignX_byXageX_sack": {func(g *gremlingo.GraphTraversalSource, p map[string]interface{}) *gremlingo.GraphTraversal {return g.V().Sack(gremlingo.Operator.Assign).By("age").Sack()}}, 
     "g_withSackXBigInteger_TEN_powX1000X_assignX_V_localXoutXknowsX_barrierXnormSackXX_inXknowsX_barrier_sack": {func(g *gremlingo.GraphTraversalSource, p map[string]interface{}) *gremlingo.GraphTraversal {return g.WithSack(p["xx1"], gremlingo.Operator.Assign).V().Local(gremlingo.T__.Out("knows").Barrier(gremlingo.Barrier.NormSack)).In("knows").Barrier().Sack()}}, 
     "g_withSackX2X_V_sackXdivX_byXconstantX4_0XX_sack": {func(g *gremlingo.GraphTraversalSource, p map[string]interface{}) *gremlingo.GraphTraversal {return g.WithSack(2).V().Sack(gremlingo.Operator.Div).By(gremlingo.T__.Constant(p["xx1"])).Sack()}}, 
+    "g_V_sackXassignX_byXageX_byXnameX_sack": {func(g *gremlingo.GraphTraversalSource, p map[string]interface{}) *gremlingo.GraphTraversal {return g.V().Sack(gremlingo.Operator.Assign).By("age").By("name").Sack()}}, 
     "g_V_sideEffectXidentityX": {func(g *gremlingo.GraphTraversalSource, p map[string]interface{}) *gremlingo.GraphTraversal {return g.V().SideEffect(gremlingo.T__.Identity())}}, 
     "g_V_sideEffectXidentity_valuesXnameXX": {func(g *gremlingo.GraphTraversalSource, p map[string]interface{}) *gremlingo.GraphTraversal {return g.V().SideEffect(gremlingo.T__.Identity().Values("name"))}}, 
     "g_V_sideEffectXpropertyXage_22X": {func(g *gremlingo.GraphTraversalSource, p map[string]interface{}) *gremlingo.GraphTraversal {return g.AddV("person").Property("age", 21)}, func(g *gremlingo.GraphTraversalSource, p map[string]interface{}) *gremlingo.GraphTraversal {return g.V().SideEffect(gremlingo.T__.Property("age", 22))}, func(g *gremlingo.GraphTraversalSource, p map[string]interface{}) *gremlingo.GraphTraversal {return g.V().Has("age", 21)}, func(g *gremlingo.GraphTraversalSource, p map[string]interface{}) *gremlingo.GraphTraversal {return g.V().Has("age", 22)}}, 

gremlin-javascript/src/main/javascript/gremlin-javascript/test/cucumber/gremlin.js

@@ -1612,6 +1612,7 @@
     'g_V_sackXassignX_byXageX_sack': [(lambda g:g.V().sack(Operator.assign).by('age').sack())], 
     'g_withSackXBigInteger_TEN_powX1000X_assignX_V_localXoutXknowsX_barrierXnormSackXX_inXknowsX_barrier_sack': [(lambda g, xx1=None:g.with_sack(xx1, Operator.assign).V().local(__.out('knows').barrier(Barrier.norm_sack)).in_('knows').barrier().sack())], 
     'g_withSackX2X_V_sackXdivX_byXconstantX4_0XX_sack': [(lambda g, xx1=None:g.with_sack(2).V().sack(Operator.div).by(__.constant(xx1)).sack())], 
+    'g_V_sackXassignX_byXageX_byXnameX_sack': [(lambda g:g.V().sack(Operator.assign).by('age').by('name').sack())], 
     'g_V_sideEffectXidentityX': [(lambda g:g.V().side_effect(__.identity()))], 
     'g_V_sideEffectXidentity_valuesXnameXX': [(lambda g:g.V().side_effect(__.identity().values('name')))], 
     'g_V_sideEffectXpropertyXage_22X': [(lambda g:g.add_v('person').property('age', 21)), (lambda g:g.V().side_effect(__.property('age', 22))), (lambda g:g.V().has('age', 21)), (lambda g:g.V().has('age', 22))], 

gremlin-python/src/main/python/radish/gremlin.py

@@ -243,4 +243,4 @@ Feature: Step - groupCount()
       g.V().out("created").groupCount("x").by("name").by("age")
       """
     When iterated to list
-    Then the traversal will raise an error
+    Then the traversal will raise an error with message containing text of "GroupCount step can only have one by modulator"

gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/sideEffect/GroupCount.feature

@@ -140,4 +140,13 @@ Feature: Step - sack()
       | d[0.5].d |
       | d[0.5].d |
       | d[0.5].d |
-      | d[0.5].d |
+      | d[0.5].d |
+
+  Scenario: g_V_sackXassignX_byXageX_byXnameX_sack
+    Given the modern graph
+    And the traversal of
+      """
+      g.V().sack(assign).by("age").by("name").sack()
+      """
+    When iterated to list
+    Then the traversal will raise an error with message containing text of "Sack step can only have one by modulator"