This is an automated email from the ASF dual-hosted git repository. xiazcy pushed a commit to branch steps-taking-traversal in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit 400f5aead4d13b42550dc0c9bb78d3373970ae90 Author: Yang Xia <[email protected]> AuthorDate: Fri Jun 12 10:41:32 2026 -0700 Add prediate traversal restriction inside choose().option(). Eliminate leftover null guards and dead code from HasContainer unification Assisted-by: Kiro:claude-opus-4-6 --- CHANGELOG.asciidoc | 2 +- docs/src/reference/the-traversal.asciidoc | 13 ++----- .../traversal/dsl/graph/GraphTraversal.java | 4 +- .../process/traversal/step/HasContainerHolder.java | 2 +- .../process/traversal/step/branch/ChooseStep.java | 5 +++ .../process/traversal/step/filter/HasStep.java | 4 +- .../traversal/step/sideEffect/AddPropertyStep.java | 11 +----- .../process/traversal/step/util/HasContainer.java | 6 +-- .../GremlinLangTraversalRoundTripTest.java | 7 +--- .../gremlin/process/traversal/PTraversalTest.java | 44 ++++++++++------------ .../step/CloneIndependenceTraversalTest.java | 2 - .../traversal/step/util/HasContainerTest.java | 1 - .../util/ChildTraversalValidatorTest.java | 36 +++++++++++++++++- 13 files changed, 73 insertions(+), 64 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index f035157d1d..159df10fd3 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -40,7 +40,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima * Added `ChildTraversalVerificationStrategy` that blocks mutating steps (`addV`, `addE`, `drop`, etc.) inside child traversals. All child traversals must be read-only. * Added `hasLabel(Traversal)` overload with grammar support for dynamic label filtering. * Added traversal-bearing predicate support to `where(P)` - resolves child traversal and tests against current value when `P.hasTraversal()` is true. -* Added rejection of traversal-bearing predicates in `choose(P)` - use `choose(__.is(P.op(traversal)), ...)` form instead. +* Added rejection of traversal-bearing predicates in `choose(P)` and `choose().option(P, ...)` - the predicate's child traversal cannot be resolved in the branch-selection context. * Added runtime Map validation for `property(traversal)` - rejects results that are not a `Map` of property key/value pairs. * Added mixed traversal/literal detection in `P.within()` and `P.without()` - throws `IllegalArgumentException` with guidance to wrap literals in `__.constant()`. * Removed `uuid` dependency from `gremlin-javascript` in favor of the built-in `globalThis.crypto.randomUUID()`. diff --git a/docs/src/reference/the-traversal.asciidoc b/docs/src/reference/the-traversal.asciidoc index 790873c581..92cdbc6799 100644 --- a/docs/src/reference/the-traversal.asciidoc +++ b/docs/src/reference/the-traversal.asciidoc @@ -1253,16 +1253,9 @@ IMPORTANT: It is important to think of `choose()` as a branching step and not a intuitively lead to thinking the latter, where no match would mean to remove the traverser from the stream. As shown in the examples, this is not what happens. -NOTE: The `choose(P)` form does not support traversal-bearing predicates directly. To use a dynamic comparison as the -branch condition, wrap it in an `is()` step: `choose(__.is(P.op(traversal)), trueChoice, falseChoice)`. - -[gremlin-groovy,modern] ----- -g.V().values('age').choose(__.is(P.gt(__.V(1).values('age'))), __.constant('older'), __.constant('not older')) <1> ----- - -<1> For each age value, compare dynamically against marko's age using a traversal inside the predicate. If greater, -return "older"; otherwise return "not older". +NOTE: The `choose(P)` form and `choose().option(P, ...)` do not support traversal-bearing predicates. The +predicate's child traversal cannot be resolved in the branch-selection context because `choose()` evaluates +predicates through `PredicateTraversal` which does not have access to the traverser needed for resolution. The `choose()`-step can be used within a `map()` step to apply the branching logic to each element in a collection. diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java index 2b8ec3eca7..81844bd35b 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java @@ -4114,7 +4114,7 @@ public interface GraphTraversal<S, E> extends Traversal<S, E> { } if (choosePredicate.hasTraversal()) { throw new IllegalArgumentException("Traversal-bearing predicates are not supported by choose(). " + - "Use choose(__.is(P.op(traversal)), trueChoice, falseChoice) instead."); + "The predicate's child traversal cannot be resolved in this context."); } this.asAdmin().getGremlinLang().addStep(Symbols.choose, choosePredicate, trueChoice, falseChoice); return this.asAdmin().addStep(new ChooseStep<E, E2, Boolean>(this.asAdmin(), (Traversal.Admin<E, ?>) __.is(choosePredicate), (Traversal.Admin<E, E2>) trueChoice, (Traversal.Admin<E, E2>) falseChoice)); @@ -4155,7 +4155,7 @@ public interface GraphTraversal<S, E> extends Traversal<S, E> { } if (choosePredicate.hasTraversal()) { throw new IllegalArgumentException("Traversal-bearing predicates are not supported by choose(). " + - "Use choose(__.is(P.op(traversal)), trueChoice) instead."); + "The predicate's child traversal cannot be resolved in this context."); } this.asAdmin().getGremlinLang().addStep(Symbols.choose, choosePredicate, trueChoice); return this.asAdmin().addStep(new ChooseStep<E, E2, Boolean>(this.asAdmin(), (Traversal.Admin<E, ?>) __.is(choosePredicate), (Traversal.Admin<E, E2>) trueChoice, (Traversal.Admin<E, E2>) __.identity())); diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/HasContainerHolder.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/HasContainerHolder.java index 47343d3ea6..f016aa03f4 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/HasContainerHolder.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/HasContainerHolder.java @@ -53,7 +53,7 @@ public interface HasContainerHolder<S, E> extends GValueHolder<S, E> { } public default Collection<P<?>> getPredicatesGValueSafe() { - return getHasContainers().stream().map(p -> p.getPredicate()).filter(p -> p != null).collect(Collectors.toList()); + return getHasContainers().stream().map(p -> p.getPredicate()).collect(Collectors.toList()); } public default HasContainerHolder<S, E> asConcreteStep() { diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/ChooseStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/ChooseStep.java index 73bec11d64..c84028f901 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/ChooseStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/ChooseStep.java @@ -18,6 +18,7 @@ */ package org.apache.tinkerpop.gremlin.process.traversal.step.branch; +import org.apache.tinkerpop.gremlin.process.traversal.P; import org.apache.tinkerpop.gremlin.process.traversal.Pick; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.DefaultGraphTraversal; @@ -122,6 +123,10 @@ public final class ChooseStep<S, E, M> extends BranchStep<S, E, M> { } } else if (pickToken instanceof Traversal) { throw new IllegalArgumentException("Traversal is not allowed as a Pick token for choose().option()"); + } else if (pickToken instanceof P && ((P<?>) pickToken).hasTraversal()) { + throw new IllegalArgumentException( + "Traversal-bearing predicates are not supported as option keys in choose(). " + + "The predicate's child traversal cannot be resolved in this context."); } else { super.addChildOption(pickToken, traversalOption); } diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/HasStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/HasStep.java index 1b56dbdfeb..a69e8f7871 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/HasStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/HasStep.java @@ -131,7 +131,7 @@ public class HasStep<S extends Element> extends FilterStep<S> implements HasCont * {@link #setTraversal(Traversal.Admin)} which is the single integration point. */ private void collectChildTraversals(final HasContainer hc) { - if (hc.getPredicate() != null && hc.getPredicate().hasTraversal()) { + if (hc.getPredicate().hasTraversal()) { P.collectTraversals(hc.getPredicate(), this.childTraversals); } } @@ -182,7 +182,7 @@ public class HasStep<S extends Element> extends FilterStep<S> implements HasCont for (final HasContainer hasContainer : this.hasContainers) { final HasContainer clonedHc = hasContainer.clone(); clone.hasContainers.add(clonedHc); - if (clonedHc.getPredicate() != null && clonedHc.getPredicate().hasTraversal()) { + if (clonedHc.getPredicate().hasTraversal()) { P.collectTraversals(clonedHc.getPredicate(), clone.childTraversals); } } diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStep.java index 7b5acbf44a..0901a67196 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStep.java @@ -49,21 +49,12 @@ import java.util.Set; */ public class AddPropertyStep<S extends Element> extends SideEffectStep<S> implements AddPropertyStepContract<S> { - /** - * Internal placeholder key used for the single-argument {@code property(traversal)} (Map-producing) form, - * where the property keys are supplied by the traversal's resulting Map rather than by a caller-provided key. - * This value is never used for dispatch (see {@link #mapForm}); it exists only to give the parameter a - * stable, non-null key within {@link org.apache.tinkerpop.gremlin.process.traversal.step.util.Parameters}. - */ - static final String MAP_VALUE_TRAVERSAL_KEY = "~traversalMap"; - private Parameters internalParameters = new Parameters(); private Parameters withConfiguration = new Parameters(); private final VertexProperty.Cardinality cardinality; /** * Marks the single-argument {@code property(traversal)} form whose child traversal must produce a Map of - * property key/value pairs. Dispatch is driven by this flag rather than by inspecting the property key, so a - * caller-supplied key that happens to collide with {@link #MAP_VALUE_TRAVERSAL_KEY} is handled normally. + * property key/value pairs. Dispatch is driven by this flag rather than by inspecting the property key. */ private final boolean mapForm; private CallbackRegistry<Event.ElementPropertyChangedEvent> callbackRegistry; diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/HasContainer.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/HasContainer.java index c6d0d1fe29..82204d49f2 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/HasContainer.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/HasContainer.java @@ -110,7 +110,7 @@ public class HasContainer implements Serializable, Cloneable, Predicate<Element> public HasContainer clone() { try { final HasContainer clone = (HasContainer) super.clone(); - clone.predicate = this.predicate != null ? this.predicate.clone() : null; + clone.predicate = this.predicate.clone(); return clone; } catch (final CloneNotSupportedException e) { throw new IllegalStateException(e.getMessage(), e); @@ -119,7 +119,7 @@ public class HasContainer implements Serializable, Cloneable, Predicate<Element> @Override public int hashCode() { - return (this.key != null ? this.key.hashCode() : 0) ^ (this.predicate != null ? this.predicate.hashCode() : 0); + return (this.key != null ? this.key.hashCode() : 0) ^ this.predicate.hashCode(); } public final String getKey() { @@ -147,7 +147,7 @@ public class HasContainer implements Serializable, Cloneable, Predicate<Element> * at runtime (e.g. {@code P.eq(traversal)} or {@code P.within(traversal)}). */ public boolean hasTraversal() { - return this.predicate != null && this.predicate.hasTraversal(); + return this.predicate.hasTraversal(); } //////////// diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/GremlinLangTraversalRoundTripTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/GremlinLangTraversalRoundTripTest.java index 391938b9ca..ff1cc9a6a7 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/GremlinLangTraversalRoundTripTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/GremlinLangTraversalRoundTripTest.java @@ -29,12 +29,9 @@ import static org.apache.tinkerpop.gremlin.process.traversal.AnonymousTraversalS import static org.junit.Assert.assertEquals; /** - * Property 7: GremlinLang serialization round-trip preserves traversal arguments. - * <p> + * Tests that GremlinLang serialization round-trips preserve traversal arguments. * For any step containing a child traversal argument, serializing to GremlinLang and parsing back - * SHALL produce a structurally equivalent traversal. - * <p> - * <b>Validates: Requirements 1.3, 3.6, 4.5, 6.5, 7.3</b> + * produces a structurally equivalent traversal. */ public class GremlinLangTraversalRoundTripTest { diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/PTraversalTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/PTraversalTest.java index d52b45ae49..86c82c2b22 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/PTraversalTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/PTraversalTest.java @@ -26,6 +26,9 @@ import org.junit.experimental.runners.Enclosed; import org.junit.runner.RunWith; import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.notNullValue; +import static org.hamcrest.CoreMatchers.nullValue; +import static org.hamcrest.CoreMatchers.sameInstance; import static org.hamcrest.MatcherAssert.assertThat; /** @@ -36,13 +39,8 @@ import static org.hamcrest.MatcherAssert.assertThat; public class PTraversalTest { /** - * Property 10: Traversal detection in predicates is accurate. - * <p> - * For any P instance containing a child traversal, {@code P.hasTraversal()} SHALL return true. - * For any P instance containing only literal values or GValue variables, - * {@code P.hasTraversal()} SHALL return false. - * <p> - * <b>Validates: Requirements 9.4</b> + * Tests that traversal detection in predicates is accurate: {@code P.hasTraversal()} returns + * true for traversal-bearing predicates and false for literal/GValue predicates. */ public static class TraversalDetectionTest { @@ -74,25 +72,20 @@ public class PTraversalTest { public void shouldReturnTraversalValueWhenPresent() { final Traversal.Admin<?, ?> traversal = __.inject(42).asAdmin(); final P<Object> p = P.eq(traversal); - assertThat(p.getTraversalValue() == traversal, is(true)); + assertThat(p.getTraversalValue(), is(sameInstance(traversal))); } @Test public void shouldReturnNullTraversalValueForLiteral() { final P<String> p = P.eq("value"); - assertThat(p.getTraversalValue() == null, is(true)); + assertThat(p.getTraversalValue(), is(nullValue())); } } /** - * Property 3: Single-value predicate rejects multiple traversal results. - * <p> - * For any single-value predicate (eq, neq, gt, lt, gte, lte) and any child traversal that produces - * more than one result, the predicate SHALL throw an IllegalArgumentException. - * For any collection predicate (within, without) and any child traversal producing multiple results, - * the predicate SHALL accept all results as the collection value. - * <p> - * <b>Validates: Requirements 2.5</b> + * Tests first-result semantics: single-value predicates (eq, neq, gt, lt, gte, lte) take the + * first result from a multi-result traversal. Collection predicates (within, without) accept + * all results as the collection value. */ public static class SingleValuePredicateRejectionTest { @@ -100,8 +93,6 @@ public class PTraversalTest { return new B_O_Traverser<>(value, 1L); } - // --- Single-value predicates should throw on multiple results --- - // --- Single-value predicates take first result, ignore extras (consistent with by()) --- @SuppressWarnings("unchecked") @@ -267,7 +258,7 @@ public class PTraversalTest { @Test public void shouldReturnTraversalValuesListForMultiTraversal() { final P<Object> p = P.within(__.constant(1).asAdmin(), __.constant(2).asAdmin()); - assertThat(p.getTraversalValues() != null, is(true)); + assertThat(p.getTraversalValues(), is(notNullValue())); assertThat(p.getTraversalValues().size(), is(2)); } @@ -275,7 +266,7 @@ public class PTraversalTest { public void shouldReturnNullTraversalValueForMultiTraversal() { // Single traversalValue should be null when using multi-traversal form final P<Object> p = P.within(__.constant(1).asAdmin(), __.constant(2).asAdmin()); - assertThat(p.getTraversalValue() == null, is(true)); + assertThat(p.getTraversalValue(), is(nullValue())); } // --- Resolution and testing --- @@ -361,14 +352,17 @@ public class PTraversalTest { // Clone should have independent traversal values assertThat(clone.hasTraversal(), is(true)); - assertThat(clone.getTraversalValues() != null, is(true)); + assertThat(clone.getTraversalValues(), is(notNullValue())); assertThat(clone.getTraversalValues().size(), is(2)); - // Modifying clone should not affect original + // Resolve the clone - this mutates the clone's internal state clone.resolve(createTraverser("start")); assertThat(clone.test(1), is(true)); - // Original should still be unresolved (literals empty) - assertThat(original.getTraversalValues().size(), is(2)); + + // Original should be unaffected: resolving it independently should still work correctly + original.resolve(createTraverser("other")); + assertThat(original.test(1), is(true)); + assertThat(original.test(2), is(true)); } // --- Varargs detection --- diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/CloneIndependenceTraversalTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/CloneIndependenceTraversalTest.java index 09af6da179..4c8d6ff850 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/CloneIndependenceTraversalTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/CloneIndependenceTraversalTest.java @@ -42,8 +42,6 @@ import static org.hamcrest.core.IsSame.sameInstance; * For any step containing child traversal arguments, cloning the step SHALL produce a deep copy * where modifying the clone's child traversal state does NOT affect the original step's child * traversal state. - * <p> - * <b>Validates: Requirements 6.2</b> */ public class CloneIndependenceTraversalTest { diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/HasContainerTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/HasContainerTest.java index 4049b20eb6..efe7072ad0 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/HasContainerTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/HasContainerTest.java @@ -56,7 +56,6 @@ public class HasContainerTest { final HasContainer hc = new HasContainer("age", P.eq(traversal)); assertEquals("age", hc.getKey()); - assertThat(hc.getPredicate(), is(notNullValue())); assertThat(hc.getPredicate().getTraversalValue(), is(sameInstance(traversal))); } diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/util/ChildTraversalValidatorTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/util/ChildTraversalValidatorTest.java index 68b99296ec..49561b4cd9 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/util/ChildTraversalValidatorTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/util/ChildTraversalValidatorTest.java @@ -216,7 +216,7 @@ public class ChildTraversalValidatorTest { @Test public void shouldRejectAddVInMutationContext() { - // addV is now blocked in property(traversal) - all mutating steps are blocked in all contexts + // addV is blocked in property(traversal) - mutating steps are blocked in child traversals assertThrows(IllegalArgumentException.class, () -> g.V().property(__.V().addV("temp").project("k").by("name"))); } @@ -226,4 +226,36 @@ public class ChildTraversalValidatorTest { // Should not throw g.V().property(__.V().project("name").by("name")); } -} + + // ===== CHOOSE CONTEXT: should reject traversal-bearing predicates ===== + + @Test + public void shouldRejectTraversalPredicateInChooseIfThen() { + assertThrows(IllegalArgumentException.class, () -> + g.V().values("age").choose(P.gt(__.V().values("age")), __.constant("older"), __.constant("younger"))); + } + + @Test + public void shouldRejectTraversalPredicateInChooseIfOnly() { + assertThrows(IllegalArgumentException.class, () -> + g.V().values("age").choose(P.gt(__.V().values("age")), __.constant("older"))); + } + + @Test + public void shouldRejectTraversalPredicateInChooseOption() { + assertThrows(IllegalArgumentException.class, () -> + g.V().values("age").choose(__.identity()).option(P.gt(__.V().values("age")), __.constant("older"))); + } + + @Test + public void shouldAllowLiteralPredicateInChoose() { + // Literal predicates should still work + g.V().values("age").choose(P.gt(30), __.constant("older"), __.constant("younger")); + } + + @Test + public void shouldAllowLiteralPredicateInChooseOption() { + // Literal predicates as option keys should still work + g.V().values("age").choose(__.identity()).option(P.gt(30), __.constant("older")); + } +} \ No newline at end of file
