TINKERPOP-3121 Prevent multiple by modulators or group count step

CHANGELOG.asciidoc

@@ -49,6 +49,7 @@ This release also includes changes from <<release-3-7-XXX, 3.7.XXX>>.
 * Deprecated `ProfilingAware.prepareForProfiling` method preferring to simply `resetBarrierFromValueTraversal` from the `Grouping` interface after strategy application.
 * 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 `groupCount()` to prevent an invalid usage of multiple `by()` modulators.
 
 == TinkerPop 3.7.0 (Gremfir Master of the Pan Flute)
 

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

@@ -210,6 +210,13 @@ gremlin> g.V().has("person","name",P.within("vadas","peter")).group().by().by(__
 
 See: link:https://issues.apache.org/jira/browse/TINKERPOP-2971[TINKERPOP-2971]
 
+==== groupCount() By Modulation Semantics
+
+The `groupCount()` step has been changed to throw an error if multiple `by()` modulators are applied. The previous 
+behavior would ignore previous `by()` modulators and apply the last one, which was not intuitive.
+
+See: link:https://issues.apache.org/jira/browse/TINKERPOP-3121[TINKERPOP-3121]
+
 === Upgrading for Providers
 
 ==== Graph System Providers

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

@@ -75,6 +75,9 @@ public Set<TraverserRequirement> getRequirements() {
 
     @Override
     public void modulateBy(final Traversal.Admin<?, ?> keyTraversal) throws UnsupportedOperationException {
+        if (this.keyTraversal != null) {
+            throw new IllegalStateException("GroupCount step can only have one by modulator");
+        }
         this.keyTraversal = this.integrateChild(keyTraversal);
     }
 

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

@@ -104,6 +104,9 @@ public int hashCode() {
 
     @Override
     public void modulateBy(final Traversal.Admin<?, ?> keyTraversal) throws UnsupportedOperationException {
+        if (this.keyTraversal != null) {
+            throw new IllegalStateException("GroupCount step can only have one by modulator");
+        }
         this.keyTraversal = this.integrateChild(keyTraversal);
     }
 

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

@@ -25,6 +25,7 @@
 
 import java.util.Arrays;
 import java.util.List;
+import org.junit.Test;
 
 /**
  * @author Marko A. Rodriguez (http://markorodriguez.com)
@@ -39,4 +40,9 @@ protected List<Traversal> getTraversals() {
                 __.groupCount().by("age")
         );
     }
+
+    @Test(expected = IllegalStateException.class)
+    public void shouldThrowForMultipleByModulators() {
+        __.groupCount().by("name").by("age");
+    }
 }

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

@@ -24,6 +24,7 @@
 
 import java.util.Arrays;
 import java.util.List;
+import org.junit.Test;
 
 /**
  * @author Daniel Kuppitz (http://gremlin.guru)
@@ -41,4 +42,9 @@ protected List<Traversal> getTraversals() {
                 __.groupCount("y").by("age")
         );
     }
+
+    @Test(expected = IllegalStateException.class)
+    public void shouldThrowForMultipleByModulators() {
+        __.groupCount("x").by("name").by("age");
+    }
 }

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

@@ -1612,6 +1612,8 @@ private static IDictionary<string, List<Func<GraphTraversalSource, IDictionary<s
                {"g_V_groupCount_byXbothE_countX", new List<Func<GraphTraversalSource, IDictionary<string, object>, ITraversal>> {(g,p) =>g.V().GroupCount<object>().By(__.BothE().Count())}}, 
                {"g_V_both_groupCountXaX_out_capXaX_selectXkeysX_unfold_both_groupCountXaX_capXaX", new List<Func<GraphTraversalSource, IDictionary<string, object>, ITraversal>> {(g,p) =>g.V().Both().GroupCount("a").Out().Cap<object>("a").Select<object>(Column.Keys).Unfold<object>().Both().GroupCount("a").Cap<object>("a")}}, 
                {"g_V_hasXperson_name_markoX_bothXknowsX_groupCount_byXvaluesXnameX_foldX", new List<Func<GraphTraversalSource, IDictionary<string, object>, ITraversal>> {(g,p) =>g.V().Has("person", "name", "marko").Both("knows").GroupCount<object>().By(__.Values<object>("name").Fold())}}, 
+               {"g_V_outXcreatedX_groupCount_byXnameX_byXageX", new List<Func<GraphTraversalSource, IDictionary<string, object>, ITraversal>> {(g,p) =>g.V().Out("created").GroupCount<object>().By("name").By("age")}}, 
+               {"g_V_outXcreatedX_groupCountXxX_byXnameX_byXageX", new List<Func<GraphTraversalSource, IDictionary<string, object>, ITraversal>> {(g,p) =>g.V().Out("created").GroupCount("x").By("name").By("age")}}, 
                {"g_VX1X_out_injectXv2X_name", new List<Func<GraphTraversalSource, IDictionary<string, object>, ITraversal>> {(g,p) =>g.V(p["vid1"]).Out().Inject((Vertex) p["v2"]).Values<object>("name")}}, 
                {"g_VX1X_out_name_injectXdanielX_asXaX_mapXlengthX_path", new List<Func<GraphTraversalSource, IDictionary<string, object>, ITraversal>>()},  // skipping as it contains a lambda
                {"g_VX1X_injectXg_VX4XX_out_name", new List<Func<GraphTraversalSource, IDictionary<string, object>, ITraversal>> {(g,p) =>g.V(p["vid1"]).Inject((Vertex) p["v2"]).Out().Values<object>("name")}}, 

gremlin-go/driver/cucumber/gremlin.go

@@ -1581,6 +1581,8 @@ var translationMap = map[string][]func(g *gremlingo.GraphTraversalSource, p map[
     "g_V_groupCount_byXbothE_countX": {func(g *gremlingo.GraphTraversalSource, p map[string]interface{}) *gremlingo.GraphTraversal {return g.V().GroupCount().By(gremlingo.T__.BothE().Count())}}, 
     "g_V_both_groupCountXaX_out_capXaX_selectXkeysX_unfold_both_groupCountXaX_capXaX": {func(g *gremlingo.GraphTraversalSource, p map[string]interface{}) *gremlingo.GraphTraversal {return g.V().Both().GroupCount("a").Out().Cap("a").Select(gremlingo.Column.Keys).Unfold().Both().GroupCount("a").Cap("a")}}, 
     "g_V_hasXperson_name_markoX_bothXknowsX_groupCount_byXvaluesXnameX_foldX": {func(g *gremlingo.GraphTraversalSource, p map[string]interface{}) *gremlingo.GraphTraversal {return g.V().Has("person", "name", "marko").Both("knows").GroupCount().By(gremlingo.T__.Values("name").Fold())}}, 
+    "g_V_outXcreatedX_groupCount_byXnameX_byXageX": {func(g *gremlingo.GraphTraversalSource, p map[string]interface{}) *gremlingo.GraphTraversal {return g.V().Out("created").GroupCount().By("name").By("age")}}, 
+    "g_V_outXcreatedX_groupCountXxX_byXnameX_byXageX": {func(g *gremlingo.GraphTraversalSource, p map[string]interface{}) *gremlingo.GraphTraversal {return g.V().Out("created").GroupCount("x").By("name").By("age")}}, 
     "g_VX1X_out_injectXv2X_name": {func(g *gremlingo.GraphTraversalSource, p map[string]interface{}) *gremlingo.GraphTraversal {return g.V(p["vid1"]).Out().Inject(p["v2"]).Values("name")}}, 
     "g_VX1X_out_name_injectXdanielX_asXaX_mapXlengthX_path": {func(g *gremlingo.GraphTraversalSource, p map[string]interface{}) *gremlingo.GraphTraversal {return nil}},  // skipping as it contains a lambda
     "g_VX1X_injectXg_VX4XX_out_name": {func(g *gremlingo.GraphTraversalSource, p map[string]interface{}) *gremlingo.GraphTraversal {return g.V(p["vid1"]).Inject(p["v2"]).Out().Values("name")}}, 

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

@@ -1584,6 +1584,8 @@
     'g_V_groupCount_byXbothE_countX': [(lambda g:g.V().group_count().by(__.both_e().count()))], 
     'g_V_both_groupCountXaX_out_capXaX_selectXkeysX_unfold_both_groupCountXaX_capXaX': [(lambda g:g.V().both().group_count('a').out().cap('a').select(Column.keys).unfold().both().group_count('a').cap('a'))], 
     'g_V_hasXperson_name_markoX_bothXknowsX_groupCount_byXvaluesXnameX_foldX': [(lambda g:g.V().has('person', 'name', 'marko').both('knows').group_count().by(__.values('name').fold()))], 
+    'g_V_outXcreatedX_groupCount_byXnameX_byXageX': [(lambda g:g.V().out('created').group_count().by('name').by('age'))], 
+    'g_V_outXcreatedX_groupCountXxX_byXnameX_byXageX': [(lambda g:g.V().out('created').group_count('x').by('name').by('age'))], 
     'g_VX1X_out_injectXv2X_name': [(lambda g, vid1=None,v2=None:g.V(vid1).out().inject(v2).values('name'))], 
     'g_VX1X_out_name_injectXdanielX_asXaX_mapXlengthX_path': [],  # skipping as it contains a lambda
     'g_VX1X_injectXg_VX4XX_out_name': [(lambda g, vid1=None,v2=None:g.V(vid1).inject(v2).out().values('name'))], 

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

@@ -225,4 +225,22 @@ Feature: Step - groupCount()
     When iterated to list
     Then the result should be unordered
       | result |
-      | m[{"l[josh]":"d[1].l","l[vadas]":"d[1].l"}] |
+      | m[{"l[josh]":"d[1].l","l[vadas]":"d[1].l"}] |
+
+  Scenario: g_V_outXcreatedX_groupCount_byXnameX_byXageX
+    Given the modern graph
+    And the traversal of
+      """
+      g.V().out("created").groupCount().by("name").by("age")
+      """
+    When iterated to list
+    Then the traversal will raise an error
+
+  Scenario: g_V_outXcreatedX_groupCountXxX_byXnameX_byXageX
+    Given the modern graph
+    And the traversal of
+      """
+      g.V().out("created").groupCount("x").by("name").by("age")
+      """
+    When iterated to list
+    Then the traversal will raise an error