From cab16554db0baaf72e441eea7f5d833d41576a8e Mon Sep 17 00:00:00 2001 From: Andrea Child Date: Wed, 2 Apr 2025 09:09:10 -0700 Subject: [PATCH] TINKERPOP-3121 Throw IllegalArgumentException if multiple by modulators are used by group count step. --- CHANGELOG.asciidoc | 1 + docs/src/upgrade/release-3.8.x.asciidoc | 7 +++++++ .../traversal/step/map/GroupCountStep.java | 3 +++ .../sideEffect/GroupCountSideEffectStep.java | 3 +++ .../step/map/GroupCountStepTest.java | 6 ++++++ .../GroupCountSideEffectStepTest.java | 6 ++++++ .../Gherkin/Gremlin.cs | 2 ++ gremlin-go/driver/cucumber/gremlin.go | 2 ++ .../test/cucumber/gremlin.js | 2 ++ .../src/main/python/radish/gremlin.py | 2 ++ .../features/sideEffect/GroupCount.feature | 20 ++++++++++++++++++- 11 files changed, 53 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 749af123e9a..d1299c2112f 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -49,6 +49,7 @@ This release also includes changes from <>. * 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) diff --git a/docs/src/upgrade/release-3.8.x.asciidoc b/docs/src/upgrade/release-3.8.x.asciidoc index 1ceadf3b054..163799fa61c 100644 --- a/docs/src/upgrade/release-3.8.x.asciidoc +++ b/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 diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GroupCountStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GroupCountStep.java index 77fd379edd0..69bbb22be82 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GroupCountStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GroupCountStep.java @@ -75,6 +75,9 @@ public Set 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); } diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/GroupCountSideEffectStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/GroupCountSideEffectStep.java index 820ba398fab..186153647d1 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/GroupCountSideEffectStep.java +++ b/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); } diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GroupCountStepTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GroupCountStepTest.java index f4eb7acd960..e53cef85d9b 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GroupCountStepTest.java +++ b/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 getTraversals() { __.groupCount().by("age") ); } + + @Test(expected = IllegalStateException.class) + public void shouldThrowForMultipleByModulators() { + __.groupCount().by("name").by("age"); + } } diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/GroupCountSideEffectStepTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/GroupCountSideEffectStepTest.java index a43e9b8f975..c0426de4266 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/GroupCountSideEffectStepTest.java +++ b/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 getTraversals() { __.groupCount("y").by("age") ); } + + @Test(expected = IllegalStateException.class) + public void shouldThrowForMultipleByModulators() { + __.groupCount("x").by("name").by("age"); + } } diff --git a/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/Gremlin.cs b/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/Gremlin.cs index 3657c8ef40a..5fe2306b9af 100644 --- a/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/Gremlin.cs +++ b/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/Gremlin.cs @@ -1612,6 +1612,8 @@ private static IDictionary, ITraversal>> {(g,p) =>g.V().GroupCount().By(__.BothE().Count())}}, {"g_V_both_groupCountXaX_out_capXaX_selectXkeysX_unfold_both_groupCountXaX_capXaX", new List, ITraversal>> {(g,p) =>g.V().Both().GroupCount("a").Out().Cap("a").Select(Column.Keys).Unfold().Both().GroupCount("a").Cap("a")}}, {"g_V_hasXperson_name_markoX_bothXknowsX_groupCount_byXvaluesXnameX_foldX", new List, ITraversal>> {(g,p) =>g.V().Has("person", "name", "marko").Both("knows").GroupCount().By(__.Values("name").Fold())}}, + {"g_V_outXcreatedX_groupCount_byXnameX_byXageX", new List, ITraversal>> {(g,p) =>g.V().Out("created").GroupCount().By("name").By("age")}}, + {"g_V_outXcreatedX_groupCountXxX_byXnameX_byXageX", new List, ITraversal>> {(g,p) =>g.V().Out("created").GroupCount("x").By("name").By("age")}}, {"g_VX1X_out_injectXv2X_name", new List, ITraversal>> {(g,p) =>g.V(p["vid1"]).Out().Inject((Vertex) p["v2"]).Values("name")}}, {"g_VX1X_out_name_injectXdanielX_asXaX_mapXlengthX_path", new List, ITraversal>>()}, // skipping as it contains a lambda {"g_VX1X_injectXg_VX4XX_out_name", new List, ITraversal>> {(g,p) =>g.V(p["vid1"]).Inject((Vertex) p["v2"]).Out().Values("name")}}, diff --git a/gremlin-go/driver/cucumber/gremlin.go b/gremlin-go/driver/cucumber/gremlin.go index 94d8bc44a86..c6589e423f1 100644 --- a/gremlin-go/driver/cucumber/gremlin.go +++ b/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")}}, diff --git a/gremlin-javascript/src/main/javascript/gremlin-javascript/test/cucumber/gremlin.js b/gremlin-javascript/src/main/javascript/gremlin-javascript/test/cucumber/gremlin.js index 7f012d60fee..f6f6a2cc71a 100644 --- a/gremlin-javascript/src/main/javascript/gremlin-javascript/test/cucumber/gremlin.js +++ b/gremlin-javascript/src/main/javascript/gremlin-javascript/test/cucumber/gremlin.js @@ -1602,6 +1602,8 @@ const gremlins = { g_V_groupCount_byXbothE_countX: [function({g}) { return g.V().groupCount().by(__.bothE().count()) }], g_V_both_groupCountXaX_out_capXaX_selectXkeysX_unfold_both_groupCountXaX_capXaX: [function({g}) { return g.V().both().groupCount("a").out().cap("a").select(Column.keys).unfold().both().groupCount("a").cap("a") }], g_V_hasXperson_name_markoX_bothXknowsX_groupCount_byXvaluesXnameX_foldX: [function({g}) { return g.V().has("person", "name", "marko").both("knows").groupCount().by(__.values("name").fold()) }], + g_V_outXcreatedX_groupCount_byXnameX_byXageX: [function({g}) { return g.V().out("created").groupCount().by("name").by("age") }], + g_V_outXcreatedX_groupCountXxX_byXnameX_byXageX: [function({g}) { return g.V().out("created").groupCount("x").by("name").by("age") }], g_VX1X_out_injectXv2X_name: [function({g, vid1, v2}) { return 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: [function({g, vid1, v2}) { return g.V(vid1).inject(v2).out().values("name") }], diff --git a/gremlin-python/src/main/python/radish/gremlin.py b/gremlin-python/src/main/python/radish/gremlin.py index 53623e3a3cb..c18a546ef2e 100644 --- a/gremlin-python/src/main/python/radish/gremlin.py +++ b/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'))], diff --git a/gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/sideEffect/GroupCount.feature b/gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/sideEffect/GroupCount.feature index 8298d219463..b8533e4037f 100644 --- a/gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/sideEffect/GroupCount.feature +++ b/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"}] | \ No newline at end of file + | 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 \ No newline at end of file