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 c661ea65226e9c57abbe5177e7deb5c55727cef9 Author: Yang Xia <[email protected]> AuthorDate: Fri Jun 12 00:16:08 2026 -0700 Add traversal-accepting has/hasLabel/is/V/E/property/where steps and mutation guards Wire the runtime-resolved predicates into the filter, lookup, and mutation steps and their DSL entry points: - has(key|T|label, traversal), hasLabel(traversal): normalized to a HasContainer holding P.eq(traversal) so there is a single resolution path; the direct traversal form on HasContainer is removed. - is(traversal), where(P) with a traversal-bearing predicate. - V(traversal)/E(traversal) start steps seed a synthetic traverser (consistent with mergeV/mergeE) so the id traversal can run without an incoming traverser. - property(traversal) producing a Map of key/value pairs, dispatched by an internal mapForm flag rather than a sentinel key. - All/Any/None step support for traversal-bearing predicates. Mutation safety is enforced in two layers: ChildTraversalValidator at DSL construction time and ChildTraversalVerificationStrategy at strategy time, the latter validating any step marked with the new AcceptsChildPredicateTraversal interface. GraphStep/TinkerGraphStepStrategy skip folding traversal-bearing HasContainers into index lookups since their value is dynamic. --- .../process/traversal/TraversalStrategies.java | 2 + .../traversal/dsl/graph/GraphTraversal.java | 150 +++++++++++++- .../traversal/dsl/graph/GraphTraversalSource.java | 37 ++++ .../gremlin/process/traversal/dsl/graph/__.java | 49 +++++ .../step/AcceptsChildPredicateTraversal.java | 34 +++ .../process/traversal/step/HasContainerHolder.java | 2 +- .../process/traversal/step/filter/AllStep.java | 30 ++- .../process/traversal/step/filter/AnyStep.java | 30 ++- .../process/traversal/step/filter/HasStep.java | 109 +++++++++- .../process/traversal/step/filter/IsStep.java | 28 ++- .../process/traversal/step/filter/NoneStep.java | 32 ++- .../traversal/step/filter/WherePredicateStep.java | 40 +++- .../process/traversal/step/map/GraphStep.java | 124 ++++++++++- .../traversal/step/sideEffect/AddPropertyStep.java | 129 +++++++++++- .../step/sideEffect/AddPropertyStepContract.java | 4 +- .../sideEffect/AddPropertyStepPlaceholder.java | 16 +- .../process/traversal/step/util/HasContainer.java | 10 +- .../optimization/InlineFilterStrategy.java | 9 + .../ChildTraversalVerificationStrategy.java | 61 ++++++ .../traversal/util/ChildTraversalValidator.java | 62 ++++++ .../step/CloneIndependenceTraversalTest.java | 120 +++++++++++ .../traversal/step/util/HasContainerTest.java | 90 ++++++++ .../ChildTraversalVerificationStrategyTest.java | 69 +++++++ .../util/ChildTraversalValidatorTest.java | 229 +++++++++++++++++++++ .../traversal/step/sideEffect/TinkerGraphStep.java | 8 + .../optimization/TinkerGraphStepStrategy.java | 14 +- .../TinkerGraphStepStrategyTraversalTest.java | 205 ++++++++++++++++++ 27 files changed, 1649 insertions(+), 44 deletions(-) diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/TraversalStrategies.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/TraversalStrategies.java index bccb983665..e3f0d90ff6 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/TraversalStrategies.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/TraversalStrategies.java @@ -52,6 +52,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.strategy.optimization.Path import org.apache.tinkerpop.gremlin.process.traversal.strategy.optimization.PathRetractionStrategy; import org.apache.tinkerpop.gremlin.process.traversal.strategy.optimization.ProductiveByStrategy; import org.apache.tinkerpop.gremlin.process.traversal.strategy.optimization.RepeatUnrollStrategy; +import org.apache.tinkerpop.gremlin.process.traversal.strategy.verification.ChildTraversalVerificationStrategy; import org.apache.tinkerpop.gremlin.process.traversal.strategy.verification.ComputerVerificationStrategy; import org.apache.tinkerpop.gremlin.process.traversal.strategy.verification.EdgeLabelVerificationStrategy; import org.apache.tinkerpop.gremlin.process.traversal.strategy.verification.LambdaRestrictionStrategy; @@ -287,6 +288,7 @@ public interface TraversalStrategies extends Serializable, Cloneable, Iterable<T LazyBarrierStrategy.instance(), ProfileStrategy.instance(), StandardVerificationStrategy.instance(), + ChildTraversalVerificationStrategy.instance(), GValueReductionStrategy.instance()); registerStrategies(Graph.class, graphStrategies); registerStrategies(EmptyGraph.class, new DefaultTraversalStrategies()); 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 c42859f7a5..2b8ec3eca7 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 @@ -209,6 +209,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.step.util.WithOptions; import org.apache.tinkerpop.gremlin.process.traversal.traverser.util.TraverserSet; import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper; import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalMetrics; +import org.apache.tinkerpop.gremlin.process.traversal.util.ChildTraversalValidator; import org.apache.tinkerpop.gremlin.structure.Column; import org.apache.tinkerpop.gremlin.structure.Direction; import org.apache.tinkerpop.gremlin.structure.Edge; @@ -414,6 +415,23 @@ public interface GraphTraversal<S, E> extends Traversal<S, E> { return this.asAdmin().addStep(step); } + /** + * A {@code V} step that accepts a child traversal whose results are used as vertex IDs for lookup. + * This form is only valid as a mid-traversal step; using it as a start step will throw an + * {@link IllegalStateException} at runtime because there is no traverser context to evaluate the child traversal. + * + * @param traversal the child traversal that produces vertex IDs + * @return the traversal with an appended {@link GraphStep} + * @see <a href="http://tinkerpop.apache.org/docs/${project.version}/reference/#graph-step"; target="_blank">Reference Documentation - Graph Step</a> + * @since 4.0.0 + */ + public default GraphTraversal<S, Vertex> V(final Traversal<?, ?> traversal) { + if (null == traversal) return V(new Object[]{ null }); + ChildTraversalValidator.validate(traversal.asAdmin()); + this.asAdmin().getGremlinLang().addStep(Symbols.V, traversal); + return this.asAdmin().addStep(new GraphStep<>(this.asAdmin(), Vertex.class, false, traversal.asAdmin())); + } + /** * A {@code E} step is usually used to start a traversal but it may also be used mid-traversal. * @@ -437,6 +455,23 @@ public interface GraphTraversal<S, E> extends Traversal<S, E> { return this.asAdmin().addStep(step); } + /** + * A {@code E} step that accepts a child traversal whose results are used as edge IDs for lookup. + * This form is only valid as a mid-traversal step; using it as a start step will throw an + * {@link IllegalStateException} at runtime because there is no traverser context to evaluate the child traversal. + * + * @param traversal the child traversal that produces edge IDs + * @return the traversal with an appended {@link GraphStep} + * @see <a href="http://tinkerpop.apache.org/docs/${project.version}/reference/#e-step"; target="_blank">Reference Documentation - E Step</a> + * @since 4.0.0 + */ + public default GraphTraversal<S, Edge> E(final Traversal<?, ?> traversal) { + if (null == traversal) return E(new Object[]{ null }); + ChildTraversalValidator.validate(traversal.asAdmin()); + this.asAdmin().getGremlinLang().addStep(Symbols.E, traversal); + return this.asAdmin().addStep(new GraphStep<>(this.asAdmin(), Edge.class, false, traversal.asAdmin())); + } + /** * Map the {@link Vertex} to its adjacent vertices given a direction. * @@ -2670,6 +2705,47 @@ public interface GraphTraversal<S, E> extends Traversal<S, E> { } } + /** + * Filters vertices, edges and vertex properties based on their properties using a child traversal + * whose results are resolved at runtime against the current traverser. The resolved results are used + * with {@code P.within()} semantics for comparison against the named property. + * + * @param propertyKey the key of the property to filter on + * @param traversal the child traversal whose results are used as the filter value + * @return the traversal with an appended {@link HasStep} + * @see <a href="http://tinkerpop.apache.org/docs/${project.version}/reference/#has-step"; target="_blank">Reference Documentation - Has Step</a> + * @since 4.0.0 + */ + public default GraphTraversal<S, E> has(final String propertyKey, final Traversal<?, ?> traversal) { + if (null == traversal) return has(propertyKey, (Object) null); + ChildTraversalValidator.validate(traversal.asAdmin()); + this.asAdmin().getGremlinLang().addStep(Symbols.has, propertyKey, traversal); + final HasContainer hasContainer = new HasContainer(propertyKey, P.eq(traversal.asAdmin())); + return this.asAdmin().addStep(new HasStep(this.asAdmin(), hasContainer)); + } + + /** + * Filters vertices, edges and vertex properties based on their properties using a child traversal + * whose results are resolved at runtime against the current traverser. The resolved results are used + * with {@code P.within()} semantics for comparison against the T-accessor value (id or label). + * + * @param accessor the {@link T} accessor of the property to filter on + * @param traversal the child traversal whose results are used as the filter value + * @return the traversal with an appended {@link HasStep} + * @see <a href="http://tinkerpop.apache.org/docs/${project.version}/reference/#has-step"; target="_blank">Reference Documentation - Has Step</a> + * @since 4.0.0 + */ + public default GraphTraversal<S, E> has(final T accessor, final Traversal<?, ?> traversal) { + if (null == accessor) + throw new IllegalArgumentException("The T accessor value of has(T,Traversal) cannot be null"); + if (null == traversal) return has(accessor, (Object) null); + ChildTraversalValidator.validate(traversal.asAdmin()); + + this.asAdmin().getGremlinLang().addStep(Symbols.has, accessor, traversal); + final HasContainer hasContainer = new HasContainer(accessor.getAccessor(), P.eq(traversal.asAdmin())); + return this.asAdmin().addStep(new HasStep(this.asAdmin(), hasContainer)); + } + /** * Filters vertices, edges and vertex properties based on their properties. * @@ -2702,6 +2778,27 @@ public interface GraphTraversal<S, E> extends Traversal<S, E> { return TraversalHelper.addHasContainer(this.asAdmin(), new HasContainer(propertyKey, value instanceof P ? (P) value : P.eq(value))); } + /** + * Filters vertices, edges and vertex properties based on their label and a property value resolved + * from a child traversal at runtime. The label is matched first, then the child traversal results + * are used with {@code P.within()} semantics for comparison against the named property. + * + * @param label the label of the {@link Element} + * @param propertyKey the key of the property to filter on + * @param traversal the child traversal whose results are used as the filter value + * @return the traversal with an appended {@link HasStep} + * @see <a href="http://tinkerpop.apache.org/docs/${project.version}/reference/#has-step"; target="_blank">Reference Documentation - Has Step</a> + * @since 4.0.0 + */ + public default GraphTraversal<S, E> has(final String label, final String propertyKey, final Traversal<?, ?> traversal) { + if (null == traversal) return has(label, propertyKey, (Object) null); + ChildTraversalValidator.validate(traversal.asAdmin()); + this.asAdmin().getGremlinLang().addStep(Symbols.has, label, propertyKey, traversal); + TraversalHelper.addHasContainer(this.asAdmin(), new HasContainer(T.label.getAccessor(), P.eq(label))); + final HasContainer hasContainer = new HasContainer(propertyKey, P.eq(traversal.asAdmin())); + return this.asAdmin().addStep(new HasStep(this.asAdmin(), hasContainer)); + } + /** * Filters vertices, edges and vertex properties based on the existence of properties. * @@ -2797,6 +2894,23 @@ public interface GraphTraversal<S, E> extends Traversal<S, E> { } } + /** + * Filters vertices, edges and vertex properties based on their label using a child traversal + * whose results are resolved at runtime against the current traverser. + * + * @param traversal the child traversal whose results are used as the label filter value + * @return the traversal with an appended {@link HasStep} + * @see <a href="http://tinkerpop.apache.org/docs/${project.version}/reference/#has-step"; target="_blank">Reference Documentation - Has Step</a> + * @since 4.0.0 + */ + public default GraphTraversal<S, E> hasLabel(final Traversal<?, ?> traversal) { + if (null == traversal) return hasLabel((String) null); + ChildTraversalValidator.validate(traversal.asAdmin()); + this.asAdmin().getGremlinLang().addStep(Symbols.hasLabel, traversal); + final HasContainer hasContainer = new HasContainer(T.label.getAccessor(), P.eq(traversal.asAdmin())); + return this.asAdmin().addStep(new HasStep(this.asAdmin(), hasContainer)); + } + /** * Filters vertices, edges and vertex properties based on their label. * @@ -3022,7 +3136,14 @@ public interface GraphTraversal<S, E> extends Traversal<S, E> { */ public default GraphTraversal<S, E> is(final Object value) { this.asAdmin().getGremlinLang().addStep(Symbols.is, value); - P<E> predicate = value instanceof P ? (P<E>) value : P.eq((E) value); + P<E> predicate; + if (value instanceof P) { + predicate = (P<E>) value; + } else if (value instanceof Traversal) { + predicate = (P<E>) P.eq(((Traversal<?, ?>) value).asAdmin()); + } else { + predicate = P.eq((E) value); + } return this.asAdmin().addStep(predicate.isParameterized() ? new IsStepPlaceholder<>(this.asAdmin(), predicate) : new IsStep<>(this.asAdmin(), predicate)); } @@ -3825,6 +3946,25 @@ public interface GraphTraversal<S, E> extends Traversal<S, E> { } return this; } + + /** + * Sets properties on the current element using a child traversal that produces a {@code Map}. + * Each entry in the resulting Map becomes a property (key → value) on the element. The traversal + * is evaluated at execution time against the current traverser. + * + * @param mapTraversal a traversal that produces a {@code Map<String, Object>} of property key/value pairs + * @return the traversal with the {@link AddPropertyStepPlaceholder} added + * @see <a href="http://tinkerpop.apache.org/docs/${project.version}/reference/#addproperty-step"; target="_blank">AddProperty Step</a> + * @since 4.0.0 + */ + public default GraphTraversal<S, E> property(final Traversal<?, ?> mapTraversal) { + if (null == mapTraversal) return this; + ChildTraversalValidator.validate(mapTraversal.asAdmin()); + this.asAdmin().getGremlinLang().addStep(Symbols.property, mapTraversal); + this.asAdmin().addStep(new AddPropertyStepPlaceholder(this.asAdmin(), null, null, mapTraversal.asAdmin(), true)); + return this; + } + ///////////////////// BRANCH STEPS ///////////////////// /** @@ -3972,6 +4112,10 @@ public interface GraphTraversal<S, E> extends Traversal<S, E> { if (choosePredicate.isParameterized()) { throw new IllegalArgumentException("Parameterized predicates are not supported by choose()"); } + if (choosePredicate.hasTraversal()) { + throw new IllegalArgumentException("Traversal-bearing predicates are not supported by choose(). " + + "Use choose(__.is(P.op(traversal)), trueChoice, falseChoice) instead."); + } 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)); } @@ -4009,6 +4153,10 @@ public interface GraphTraversal<S, E> extends Traversal<S, E> { if (choosePredicate.isParameterized()) { throw new IllegalArgumentException("Parameterized predicates are not supported by choose()"); } + if (choosePredicate.hasTraversal()) { + throw new IllegalArgumentException("Traversal-bearing predicates are not supported by choose(). " + + "Use choose(__.is(P.op(traversal)), trueChoice) instead."); + } 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/dsl/graph/GraphTraversalSource.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversalSource.java index aec1d6fadd..3bce63c401 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversalSource.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversalSource.java @@ -42,6 +42,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect.InjectStep import org.apache.tinkerpop.gremlin.process.traversal.step.map.GraphStepContract; import org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration.RequirementsStrategy; import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement; +import org.apache.tinkerpop.gremlin.process.traversal.util.ChildTraversalValidator; import org.apache.tinkerpop.gremlin.structure.Direction; import org.apache.tinkerpop.gremlin.structure.Edge; import org.apache.tinkerpop.gremlin.structure.Graph; @@ -540,6 +541,24 @@ public class GraphTraversalSource implements TraversalSource { return traversal.addStep(step); } + /** + * Spawns a {@link GraphTraversal} starting with vertices whose IDs are resolved from a child traversal. + * As a start step, a synthetic traverser is generated to seed the child traversal evaluation, + * consistent with how {@code mergeV(traversal)} handles start steps. The child traversal should + * be self-contained (e.g., {@code __.V(1).id()}) rather than dependent on an incoming traverser. + * + * @param traversal the child traversal that produces vertex IDs + * @since 4.0.0 + */ + public GraphTraversal<Vertex, Vertex> V(final Traversal<?, ?> traversal) { + if (null == traversal) return V(new Object[]{ null }); + ChildTraversalValidator.validate(traversal.asAdmin()); + final GraphTraversalSource clone = this.clone(); + clone.gremlinLang.addStep(GraphTraversal.Symbols.V, traversal); + final GraphTraversal.Admin<Vertex, Vertex> traversalAdmin = new DefaultGraphTraversal<>(clone); + return traversalAdmin.addStep(new GraphStep<>(traversalAdmin, Vertex.class, true, traversal.asAdmin())); + } + /** * Spawns a {@link GraphTraversal} starting with all edges or some subset of edges as specified by their unique * identifier. @@ -561,6 +580,24 @@ public class GraphTraversalSource implements TraversalSource { return traversal.addStep(step); } + /** + * Spawns a {@link GraphTraversal} starting with edges whose IDs are resolved from a child traversal. + * As a start step, a synthetic traverser is generated to seed the child traversal evaluation, + * consistent with how {@code mergeE(traversal)} handles start steps. The child traversal should + * be self-contained (e.g., {@code __.V(1).outE().id()}) rather than dependent on an incoming traverser. + * + * @param traversal the child traversal that produces edge IDs + * @since 4.0.0 + */ + public GraphTraversal<Edge, Edge> E(final Traversal<?, ?> traversal) { + if (null == traversal) return E(new Object[]{ null }); + ChildTraversalValidator.validate(traversal.asAdmin()); + final GraphTraversalSource clone = this.clone(); + clone.gremlinLang.addStep(GraphTraversal.Symbols.E, traversal); + final GraphTraversal.Admin<Edge, Edge> traversalAdmin = new DefaultGraphTraversal<>(clone); + return traversalAdmin.addStep(new GraphStep<>(traversalAdmin, Edge.class, true, traversal.asAdmin())); + } + /** * Spawns a {@link GraphTraversal} starting with a list of available services. * diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/__.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/__.java index 01986b4534..46eddc4e2f 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/__.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/__.java @@ -140,6 +140,13 @@ public class __ { return __.<A>start().V(vertexIdsOrElements); } + /** + * @see GraphTraversal#V(Traversal) + */ + public static <A> GraphTraversal<A, Vertex> V(final Traversal<?, ?> traversal) { + return __.<A>start().V(traversal); + } + /** * @see GraphTraversal#E(Object...) */ @@ -147,6 +154,13 @@ public class __ { return __.<A>start().E(edgeIdsOrElements); } + /** + * @see GraphTraversal#E(Traversal) + */ + public static <A> GraphTraversal<A, Edge> E(final Traversal<?, ?> traversal) { + return __.<A>start().E(traversal); + } + /** * @see GraphTraversal#to(Direction) */ @@ -1092,6 +1106,13 @@ public class __ { return __.<A>start().has(propertyKey, predicate); } + /** + * @see GraphTraversal#has(String, Traversal) + */ + public static <A> GraphTraversal<A, A> has(final String propertyKey, final Traversal<?, ?> traversal) { + return __.<A>start().has(propertyKey, traversal); + } + /** * @see GraphTraversal#has(T, P) */ @@ -1113,6 +1134,13 @@ public class __ { return __.<A>start().has(accessor, value); } + /** + * @see GraphTraversal#has(T, Traversal) + */ + public static <A> GraphTraversal<A, A> has(final T accessor, final Traversal<?, ?> traversal) { + return __.<A>start().has(accessor, traversal); + } + /** * @see GraphTraversal#has(String, String, Object) */ @@ -1142,6 +1170,13 @@ public class __ { return __.<A>start().has(label, propertyKey, predicate); } + /** + * @see GraphTraversal#has(String, String, Traversal) + */ + public static <A> GraphTraversal<A, A> has(final String label, final String propertyKey, final Traversal<?, ?> traversal) { + return __.<A>start().has(label, propertyKey, traversal); + } + /** * @see GraphTraversal#has(String) */ @@ -1177,6 +1212,13 @@ public class __ { return __.<A>start().hasLabel(predicate); } + /** + * @see GraphTraversal#hasLabel(Traversal) + */ + public static <A> GraphTraversal<A, A> hasLabel(final Traversal<?, ?> traversal) { + return __.<A>start().hasLabel(traversal); + } + /** * @see GraphTraversal#hasId(Object, Object...) */ @@ -1558,6 +1600,13 @@ public class __ { return __.<A>start().property(value); } + /** + * @see GraphTraversal#property(Traversal) + */ + public static <A> GraphTraversal<A, A> property(final Traversal<?, ?> mapTraversal) { + return __.<A>start().property(mapTraversal); + } + ///////////////////// BRANCH STEPS ///////////////////// /** diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/AcceptsChildPredicateTraversal.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/AcceptsChildPredicateTraversal.java new file mode 100644 index 0000000000..fef3841441 --- /dev/null +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/AcceptsChildPredicateTraversal.java @@ -0,0 +1,34 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.tinkerpop.gremlin.process.traversal.step; + +/** + * Marker interface for steps that accept user-supplied child traversals embedded as predicate or lookup + * arguments (for example {@code has(key, traversal)}, {@code is(P.gt(traversal))}, + * {@code V(traversal)}, or {@code property(traversal)}). + * <p> + * The {@link org.apache.tinkerpop.gremlin.process.traversal.strategy.verification.ChildTraversalVerificationStrategy} + * validates the local children of every step implementing this interface, rejecting child traversals that + * contain mutating steps. New steps that accept such traversals should implement this interface so they are + * validated automatically rather than relying on a hardcoded step list. + * + * @since 4.0.0 + */ +public interface AcceptsChildPredicateTraversal { +} 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 f016aa03f4..47343d3ea6 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()).collect(Collectors.toList()); + return getHasContainers().stream().map(p -> p.getPredicate()).filter(p -> p != null).collect(Collectors.toList()); } public default HasContainerHolder<S, E> asConcreteStep() { diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/AllStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/AllStep.java index 64853f6614..f17795dc0b 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/AllStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/AllStep.java @@ -21,15 +21,19 @@ package org.apache.tinkerpop.gremlin.process.traversal.step.filter; import org.apache.tinkerpop.gremlin.process.traversal.P; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.Traverser; +import org.apache.tinkerpop.gremlin.process.traversal.step.AcceptsChildPredicateTraversal; +import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement; import org.apache.tinkerpop.gremlin.structure.util.StringFactory; import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils; -import java.util.EnumSet; +import java.util.ArrayList; +import java.util.Collections; import java.util.Iterator; +import java.util.List; import java.util.Set; -public final class AllStep<S, S2> extends FilterStep<S> { +public final class AllStep<S, S2> extends FilterStep<S> implements TraversalParent, AcceptsChildPredicateTraversal { private P<S2> predicate; @@ -41,10 +45,16 @@ public final class AllStep<S, S2> extends FilterStep<S> { } this.predicate = predicate; + P.integrateTraversals(this.predicate, this); } @Override protected boolean filter(final Traverser.Admin<S> traverser) { + if (this.predicate.hasTraversal()) { + this.predicate.resolve(traverser); + if (this.predicate.isResolvedEmpty()) return false; + } + final S item = traverser.get(); if (item instanceof Iterable || item instanceof Iterator || ((item != null) && item.getClass().isArray())) { @@ -65,6 +75,14 @@ public final class AllStep<S, S2> extends FilterStep<S> { return StringFactory.stepString(this, this.predicate); } + @SuppressWarnings("unchecked") + @Override + public <S, E> List<Traversal.Admin<S, E>> getLocalChildren() { + final List<Traversal.Admin<?, ?>> traversals = new ArrayList<>(); + P.collectTraversals(this.predicate, traversals); + return (List) Collections.unmodifiableList(traversals); + } + @Override public AllStep<S, S2> clone() { final AllStep<S, S2> clone = (AllStep<S, S2>) super.clone(); @@ -74,6 +92,12 @@ public final class AllStep<S, S2> extends FilterStep<S> { @Override public Set<TraverserRequirement> getRequirements() { - return EnumSet.of(TraverserRequirement.OBJECT); + return this.getSelfAndChildRequirements(TraverserRequirement.OBJECT); + } + + @Override + public void setTraversal(final Traversal.Admin<?, ?> parentTraversal) { + super.setTraversal(parentTraversal); + P.integrateTraversals(this.predicate, this); } } diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/AnyStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/AnyStep.java index 9881186d7c..ed0216934c 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/AnyStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/AnyStep.java @@ -21,15 +21,19 @@ package org.apache.tinkerpop.gremlin.process.traversal.step.filter; import org.apache.tinkerpop.gremlin.process.traversal.P; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.Traverser; +import org.apache.tinkerpop.gremlin.process.traversal.step.AcceptsChildPredicateTraversal; +import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement; import org.apache.tinkerpop.gremlin.structure.util.StringFactory; import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils; -import java.util.EnumSet; +import java.util.ArrayList; +import java.util.Collections; import java.util.Iterator; +import java.util.List; import java.util.Set; -public final class AnyStep<S, S2> extends FilterStep<S> { +public final class AnyStep<S, S2> extends FilterStep<S> implements TraversalParent, AcceptsChildPredicateTraversal { private P<S2> predicate; @@ -41,10 +45,16 @@ public final class AnyStep<S, S2> extends FilterStep<S> { } this.predicate = predicate; + P.integrateTraversals(this.predicate, this); } @Override protected boolean filter(final Traverser.Admin<S> traverser) { + if (this.predicate.hasTraversal()) { + this.predicate.resolve(traverser); + if (this.predicate.isResolvedEmpty()) return false; + } + final S item = traverser.get(); if (item instanceof Iterable || item instanceof Iterator || ((item != null) && item.getClass().isArray())) { @@ -65,6 +75,14 @@ public final class AnyStep<S, S2> extends FilterStep<S> { return StringFactory.stepString(this, this.predicate); } + @SuppressWarnings("unchecked") + @Override + public <S, E> List<Traversal.Admin<S, E>> getLocalChildren() { + final List<Traversal.Admin<?, ?>> traversals = new ArrayList<>(); + P.collectTraversals(this.predicate, traversals); + return (List) Collections.unmodifiableList(traversals); + } + @Override public AnyStep<S, S2> clone() { final AnyStep<S, S2> clone = (AnyStep<S, S2>) super.clone(); @@ -74,6 +92,12 @@ public final class AnyStep<S, S2> extends FilterStep<S> { @Override public Set<TraverserRequirement> getRequirements() { - return EnumSet.of(TraverserRequirement.OBJECT); + return this.getSelfAndChildRequirements(TraverserRequirement.OBJECT); + } + + @Override + public void setTraversal(final Traversal.Admin<?, ?> parentTraversal) { + super.setTraversal(parentTraversal); + P.integrateTraversals(this.predicate, this); } } 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 e7ef009565..1b56dbdfeb 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 @@ -18,10 +18,13 @@ */ package org.apache.tinkerpop.gremlin.process.traversal.step.filter; +import org.apache.tinkerpop.gremlin.process.traversal.P; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.Traverser; +import org.apache.tinkerpop.gremlin.process.traversal.step.AcceptsChildPredicateTraversal; import org.apache.tinkerpop.gremlin.process.traversal.step.Configuring; import org.apache.tinkerpop.gremlin.process.traversal.step.HasContainerHolder; +import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; import org.apache.tinkerpop.gremlin.process.traversal.step.util.HasContainer; import org.apache.tinkerpop.gremlin.process.traversal.step.util.Parameters; import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement; @@ -31,22 +34,25 @@ import org.apache.tinkerpop.gremlin.structure.util.StringFactory; import java.util.ArrayList; import java.util.Collections; -import java.util.EnumSet; import java.util.List; import java.util.Set; /** * @author Marko A. Rodriguez (http://markorodriguez.com) */ -public class HasStep<S extends Element> extends FilterStep<S> implements HasContainerHolder<S, S>, Configuring { +public class HasStep<S extends Element> extends FilterStep<S> implements HasContainerHolder<S, S>, Configuring, TraversalParent, AcceptsChildPredicateTraversal { private final Parameters parameters = new Parameters(); private List<HasContainer> hasContainers; + private List<Traversal.Admin<?, ?>> childTraversals = new ArrayList<>(); public HasStep(final Traversal.Admin traversal, final HasContainer... hasContainers) { super(traversal); this.hasContainers = new ArrayList<>(); - Collections.addAll(this.hasContainers, hasContainers); + for (final HasContainer hc : hasContainers) { + this.hasContainers.add(hc); + collectChildTraversals(hc); + } } @Override @@ -63,14 +69,71 @@ public class HasStep<S extends Element> extends FilterStep<S> implements HasCont protected boolean filter(final Traverser.Admin<S> traverser) { // the generic S is defined as Element but Property can also be used with HasStep so this seems to cause // problems with some jdk versions. - if (traverser.get() instanceof Element) - return HasContainer.testAll(traverser.get(), this.hasContainers); - else if (traverser.get() instanceof Property) - return HasContainer.testAll((Property) traverser.get(), this.hasContainers); - else + // Check Property BEFORE Element because VertexProperty implements both interfaces. + // HasContainer.test(Property) correctly handles T.key and T.value accessors, whereas + // HasContainer.test(Element) would fall through to element.properties("~key") which fails. + if (traverser.get() instanceof Property) { + return testAllWithTraversals(traverser, (Property) traverser.get()); + } else if (traverser.get() instanceof Element) { + return testAllWithTraversals(traverser, (Element) traverser.get()); + } else { throw new IllegalStateException(String.format( "Traverser to has() must be of type Property or Element, not %s", traverser.get().getClass().getName())); + } + } + + /** + * Tests all HasContainers against an Element. Traversal-bearing containers carry a {@code P} predicate + * (e.g. {@code P.eq(traversal)}) whose child traversal is resolved against the current traverser before + * the standard {@link HasContainer#test(Element)} is applied. + */ + private boolean testAllWithTraversals(final Traverser.Admin<S> traverser, final Element element) { + for (final HasContainer hc : this.hasContainers) { + if (!resolvePredicate(hc, traverser) || !hc.test(element)) { + return false; + } + } + return true; + } + + /** + * Tests all HasContainers against a Property. See {@link #testAllWithTraversals(Traverser.Admin, Element)}. + */ + private boolean testAllWithTraversals(final Traverser.Admin<S> traverser, final Property property) { + for (final HasContainer hc : this.hasContainers) { + if (!resolvePredicate(hc, traverser) || !hc.test(property)) { + return false; + } + } + return true; + } + + /** + * Resolves a container's predicate traversal (if any) against the current traverser, mutating the + * predicate with the resolved value for this test cycle. + * + * @return {@code false} if the predicate resolved empty (no comparison value), meaning the element + * cannot match and the caller should short-circuit; {@code true} otherwise. + */ + private boolean resolvePredicate(final HasContainer hc, final Traverser.Admin<S> traverser) { + if (hc.hasTraversal()) { + hc.getPredicate().resolve(traverser); + return !hc.getPredicate().isResolvedEmpty(); + } + return true; + } + + /** + * Collects the child traversals embedded within a HasContainer's predicate (including + * {@link org.apache.tinkerpop.gremlin.process.traversal.util.ConnectiveP}, which may carry traversals on + * child predicates) into {@link #childTraversals}. Parenting is deferred to + * {@link #setTraversal(Traversal.Admin)} which is the single integration point. + */ + private void collectChildTraversals(final HasContainer hc) { + if (hc.getPredicate() != null && hc.getPredicate().hasTraversal()) { + P.collectTraversals(hc.getPredicate(), this.childTraversals); + } } @Override @@ -91,23 +154,49 @@ public class HasStep<S extends Element> extends FilterStep<S> implements HasCont @Override public void addHasContainer(final HasContainer hasContainer) { this.hasContainers.add(hasContainer); + // A container may be added after this step has already been parented (e.g. by a strategy), so collect + // its child traversals and integrate just those new ones immediately. + final int before = this.childTraversals.size(); + collectChildTraversals(hasContainer); + for (int i = before; i < this.childTraversals.size(); i++) { + this.integrateChild(this.childTraversals.get(i)); + } + } + + @SuppressWarnings("unchecked") + @Override + public <S, E> List<Traversal.Admin<S, E>> getLocalChildren() { + return (List) Collections.unmodifiableList(this.childTraversals); } @Override public Set<TraverserRequirement> getRequirements() { - return EnumSet.of(TraverserRequirement.OBJECT); + return this.getSelfAndChildRequirements(TraverserRequirement.OBJECT); } @Override public HasStep<S> clone() { final HasStep<S> clone = (HasStep<S>) super.clone(); clone.hasContainers = new ArrayList<>(); + clone.childTraversals = new ArrayList<>(); for (final HasContainer hasContainer : this.hasContainers) { - clone.addHasContainer(hasContainer.clone()); + final HasContainer clonedHc = hasContainer.clone(); + clone.hasContainers.add(clonedHc); + if (clonedHc.getPredicate() != null && clonedHc.getPredicate().hasTraversal()) { + P.collectTraversals(clonedHc.getPredicate(), clone.childTraversals); + } } return clone; } + @Override + public void setTraversal(final Traversal.Admin<?, ?> parentTraversal) { + super.setTraversal(parentTraversal); + for (final Traversal.Admin<?, ?> childTraversal : this.childTraversals) { + this.integrateChild(childTraversal); + } + } + @Override public int hashCode() { int result = super.hashCode(); diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/IsStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/IsStep.java index 70ad55ff31..e49c7e3537 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/IsStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/IsStep.java @@ -21,27 +21,36 @@ package org.apache.tinkerpop.gremlin.process.traversal.step.filter; import org.apache.tinkerpop.gremlin.process.traversal.P; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.Traverser; +import org.apache.tinkerpop.gremlin.process.traversal.step.AcceptsChildPredicateTraversal; +import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement; import org.apache.tinkerpop.gremlin.structure.util.StringFactory; -import java.util.EnumSet; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; import java.util.Set; /** * @author Daniel Kuppitz (http://gremlin.guru) * @author Marko A. Rodriguez (http://markorodriguez.com) */ -public final class IsStep<S> extends FilterStep<S> implements IsStepContract<S> { +public final class IsStep<S> extends FilterStep<S> implements IsStepContract<S>, TraversalParent, AcceptsChildPredicateTraversal { private P<S> predicate; public IsStep(final Traversal.Admin traversal, final P<S> predicate) { super(traversal); this.predicate = predicate; + P.integrateTraversals(this.predicate, this); } @Override protected boolean filter(final Traverser.Admin<S> traverser) { + if (this.predicate.hasTraversal()) { + this.predicate.resolve(traverser); + if (this.predicate.isResolvedEmpty()) return false; + } return this.predicate.test(traverser.get()); } @@ -55,6 +64,14 @@ public final class IsStep<S> extends FilterStep<S> implements IsStepContract<S> return this.predicate; } + @SuppressWarnings("unchecked") + @Override + public <S, E> List<Traversal.Admin<S, E>> getLocalChildren() { + final List<Traversal.Admin<?, ?>> traversals = new ArrayList<>(); + P.collectTraversals(this.predicate, traversals); + return (List) Collections.unmodifiableList(traversals); + } + @Override public IsStep<S> clone() { final IsStep<S> clone = (IsStep<S>) super.clone(); @@ -69,7 +86,12 @@ public final class IsStep<S> extends FilterStep<S> implements IsStepContract<S> @Override public Set<TraverserRequirement> getRequirements() { - return EnumSet.of(TraverserRequirement.OBJECT); + return this.getSelfAndChildRequirements(TraverserRequirement.OBJECT); } + @Override + public void setTraversal(final Traversal.Admin<?, ?> parentTraversal) { + super.setTraversal(parentTraversal); + P.integrateTraversals(this.predicate, this); + } } diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/NoneStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/NoneStep.java index 542d16ca5c..6344e3a2b8 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/NoneStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/NoneStep.java @@ -21,15 +21,19 @@ package org.apache.tinkerpop.gremlin.process.traversal.step.filter; import org.apache.tinkerpop.gremlin.process.traversal.P; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.Traverser; +import org.apache.tinkerpop.gremlin.process.traversal.step.AcceptsChildPredicateTraversal; +import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement; import org.apache.tinkerpop.gremlin.structure.util.StringFactory; import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils; -import java.util.EnumSet; +import java.util.ArrayList; +import java.util.Collections; import java.util.Iterator; +import java.util.List; import java.util.Set; -public final class NoneStep<S, S2> extends FilterStep<S> { +public final class NoneStep<S, S2> extends FilterStep<S> implements TraversalParent, AcceptsChildPredicateTraversal { private P<S2> predicate; @@ -41,10 +45,18 @@ public final class NoneStep<S, S2> extends FilterStep<S> { } this.predicate = predicate; + P.integrateTraversals(this.predicate, this); } @Override protected boolean filter(final Traverser.Admin<S> traverser) { + if (this.predicate.hasTraversal()) { + this.predicate.resolve(traverser); + // Vacuously true: if the predicate can't be evaluated (no comparison value), + // then no item can match, so none() is satisfied. + if (this.predicate.isResolvedEmpty()) return true; + } + final S item = traverser.get(); if (item instanceof Iterable || item instanceof Iterator || ((item != null) && item.getClass().isArray())) { @@ -65,6 +77,14 @@ public final class NoneStep<S, S2> extends FilterStep<S> { return StringFactory.stepString(this, this.predicate); } + @SuppressWarnings("unchecked") + @Override + public <S, E> List<Traversal.Admin<S, E>> getLocalChildren() { + final List<Traversal.Admin<?, ?>> traversals = new ArrayList<>(); + P.collectTraversals(this.predicate, traversals); + return (List) Collections.unmodifiableList(traversals); + } + @Override public NoneStep<S, S2> clone() { final NoneStep<S, S2> clone = (NoneStep<S, S2>) super.clone(); @@ -74,6 +94,12 @@ public final class NoneStep<S, S2> extends FilterStep<S> { @Override public Set<TraverserRequirement> getRequirements() { - return EnumSet.of(TraverserRequirement.OBJECT); + return this.getSelfAndChildRequirements(TraverserRequirement.OBJECT); + } + + @Override + public void setTraversal(final Traversal.Admin<?, ?> parentTraversal) { + super.setTraversal(parentTraversal); + P.integrateTraversals(this.predicate, this); } } diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WherePredicateStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WherePredicateStep.java index 77c3f4abc9..6a2a3a5770 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WherePredicateStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WherePredicateStep.java @@ -22,6 +22,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.P; import org.apache.tinkerpop.gremlin.process.traversal.Pop; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.Traverser; +import org.apache.tinkerpop.gremlin.process.traversal.step.AcceptsChildPredicateTraversal; import org.apache.tinkerpop.gremlin.process.traversal.step.ByModulating; import org.apache.tinkerpop.gremlin.process.traversal.step.PathProcessor; import org.apache.tinkerpop.gremlin.process.traversal.step.Scoping; @@ -45,7 +46,7 @@ import java.util.Set; /** * @author Marko A. Rodriguez (http://markorodriguez.com) */ -public final class WherePredicateStep<S> extends FilterStep<S> implements Scoping, PathProcessor, ByModulating, TraversalParent { +public final class WherePredicateStep<S> extends FilterStep<S> implements Scoping, PathProcessor, ByModulating, TraversalParent, AcceptsChildPredicateTraversal { protected String startKey; protected List<String> selectKeys; @@ -54,6 +55,7 @@ public final class WherePredicateStep<S> extends FilterStep<S> implements Scopin protected Set<String> keepLabels; protected TraversalRing<S, ?> traversalRing = new TraversalRing<>(); + private List<Traversal.Admin<?, ?>> predicateTraversals; public WherePredicateStep(final Traversal.Admin traversal, final Optional<String> startKey, final P<String> predicate) { super(traversal); @@ -63,12 +65,19 @@ public final class WherePredicateStep<S> extends FilterStep<S> implements Scopin this.predicate = (P) predicate; this.selectKeys = new ArrayList<>(); this.configurePredicates(this.predicate); + // Cache and integrate child traversals from the predicate + this.predicateTraversals = new ArrayList<>(); + P.collectTraversals(this.predicate, this.predicateTraversals); + for (final Traversal.Admin<?, ?> t : this.predicateTraversals) { + this.integrateChild(t); + } } private void configurePredicates(final P<Object> predicate) { if (predicate instanceof ConnectiveP) ((ConnectiveP<Object>) predicate).getPredicates().forEach(this::configurePredicates); - else { + else if (!predicate.hasTraversal()) { + // Only configure scope keys for non-traversal predicates (string label references) final String selectKey = getSelectKey(predicate); this.selectKeys.add(selectKey); this.scopeKeys.add(selectKey); @@ -112,14 +121,28 @@ public final class WherePredicateStep<S> extends FilterStep<S> implements Scopin @Override protected boolean filter(final Traverser.Admin<S> traverser) { + // Resolve the left-hand value (current traverser or startKey scope value) final TraversalProduct product = null == this.startKey ? TraversalUtil.produce(traverser, this.traversalRing.next()) : TraversalUtil.produce((S) this.getSafeScopeValue(Pop.last, this.startKey, traverser), this.traversalRing.next()); - final boolean predicateValuesProductive = this.setPredicateValues(this.predicate, traverser, this.selectKeys.iterator()); - this.traversalRing.reset(); + if (!product.isProductive()) { + this.traversalRing.reset(); + return false; + } - return product.isProductive() && predicateValuesProductive && this.predicate.test(product.get()); + if (this.predicate.hasTraversal()) { + // Traversal-bearing predicate: resolve the child traversal, then test + this.traversalRing.reset(); + this.predicate.resolve(traverser); + if (this.predicate.isResolvedEmpty()) return false; + return this.predicate.test(product.get()); + } else { + // Standard scope-label path: resolve predicate values from path labels + final boolean predicateValuesProductive = this.setPredicateValues(this.predicate, traverser, this.selectKeys.iterator()); + this.traversalRing.reset(); + return predicateValuesProductive && this.predicate.test(product.get()); + } } @Override @@ -166,7 +189,12 @@ public final class WherePredicateStep<S> extends FilterStep<S> implements Scopin @Override public List<Traversal.Admin<S, ?>> getLocalChildren() { - return (List) this.traversalRing.getTraversals(); + if (this.predicateTraversals.isEmpty()) { + return (List) this.traversalRing.getTraversals(); + } + final List<Traversal.Admin<S, ?>> children = new ArrayList<>((List) this.traversalRing.getTraversals()); + children.addAll((List) this.predicateTraversals); + return children; } @Override diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GraphStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GraphStep.java index 7e64cd391a..c000c7c164 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GraphStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GraphStep.java @@ -24,11 +24,14 @@ import org.apache.tinkerpop.gremlin.process.traversal.Contains; import org.apache.tinkerpop.gremlin.process.traversal.Step; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.Traverser; +import org.apache.tinkerpop.gremlin.process.traversal.step.AcceptsChildPredicateTraversal; import org.apache.tinkerpop.gremlin.process.traversal.step.GValue; +import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; import org.apache.tinkerpop.gremlin.process.traversal.step.util.AbstractStep; import org.apache.tinkerpop.gremlin.process.traversal.step.util.HasContainer; import org.apache.tinkerpop.gremlin.process.traversal.step.util.Parameters; import org.apache.tinkerpop.gremlin.process.traversal.util.FastNoSuchElementException; +import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalUtil; import org.apache.tinkerpop.gremlin.structure.Edge; import org.apache.tinkerpop.gremlin.structure.Element; import org.apache.tinkerpop.gremlin.structure.T; @@ -37,10 +40,12 @@ import org.apache.tinkerpop.gremlin.structure.util.CloseableIterator; import org.apache.tinkerpop.gremlin.structure.util.StringFactory; import org.apache.tinkerpop.gremlin.util.iterator.EmptyIterator; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.Iterator; +import java.util.List; import java.util.Objects; import java.util.function.Supplier; @@ -48,7 +53,7 @@ import java.util.function.Supplier; * @author Marko A. Rodriguez (http://markorodriguez.com) * @author Pieter Martin */ -public class GraphStep<S, E extends Element> extends AbstractStep<S, E> implements GraphStepContract<S, E> { +public class GraphStep<S, E extends Element> extends AbstractStep<S, E> implements GraphStepContract<S, E>, TraversalParent, AcceptsChildPredicateTraversal { protected Parameters parameters = new Parameters(); protected final Class<E> returnClass; @@ -58,6 +63,7 @@ public class GraphStep<S, E extends Element> extends AbstractStep<S, E> implemen protected boolean done = false; private Traverser.Admin<S> head = null; private Iterator<E> iterator = EmptyIterator.instance(); + private Traversal.Admin<?, ?> idTraversal; public GraphStep(final Traversal.Admin traversal, final Class<E> returnClass, final boolean isStart, final Object... ids) { @@ -76,7 +82,36 @@ public class GraphStep<S, E extends Element> extends AbstractStep<S, E> implemen this.getTraversal().getGraph().get().edges(this.ids)); } + public GraphStep(final Traversal.Admin traversal, final Class<E> returnClass, final boolean isStart, final Traversal.Admin<?, ?> idTraversal) { + this(traversal, returnClass, isStart); + this.idTraversal = idTraversal; + if (this.idTraversal != null) { + this.integrateChild(this.idTraversal); + } + } + + /** + * Returns the child traversal used to resolve element IDs, or {@code null} if literal IDs are used. + */ + public Traversal.Admin<?, ?> getIdTraversal() { + return this.idTraversal; + } + + /** + * Sets the child traversal used to resolve element IDs. Calls {@link #integrateChild(Traversal.Admin)} + * on the provided traversal. + */ + public void setIdTraversal(final Traversal.Admin<?, ?> idTraversal) { + this.idTraversal = idTraversal; + if (this.idTraversal != null) { + this.integrateChild(this.idTraversal); + } + } + public String toString() { + if (this.idTraversal != null) { + return StringFactory.stepString(this, this.returnClass.getSimpleName().toLowerCase(), this.idTraversal); + } return StringFactory.stepString(this, this.returnClass.getSimpleName().toLowerCase(), Arrays.toString(this.ids)); } @@ -164,16 +199,65 @@ public class GraphStep<S, E extends Element> extends AbstractStep<S, E> implemen if (this.isStart) { if (this.done) throw FastNoSuchElementException.instance(); - else { + if (this.idTraversal != null) { + // Start step with idTraversal: generate a synthetic traverser to seed + // the child traversal, consistent with how mergeV/addV handle start steps. + this.done = true; + final Traverser.Admin<S> syntheticTraverser = (Traverser.Admin<S>) + this.getTraversal().getTraverserGenerator().generate(false, (Step) this, 1L); + final Object[] resolvedIds = resolveTraversalIds(syntheticTraverser); + this.iterator = lookupElements(resolvedIds); + } else { this.done = true; this.iterator = null == this.iteratorSupplier ? EmptyIterator.instance() : this.iteratorSupplier.get(); } } else { this.head = this.starts.next(); - this.iterator = null == this.iteratorSupplier ? EmptyIterator.instance() : this.iteratorSupplier.get(); + if (this.idTraversal != null) { + final Object[] resolvedIds = resolveTraversalIds(this.head); + this.iterator = lookupElements(resolvedIds); + } else { + this.iterator = null == this.iteratorSupplier ? EmptyIterator.instance() : this.iteratorSupplier.get(); + } + } + } + } + } + + /** + * Resolves element IDs by evaluating the child traversal against the current traverser. + * Handles Element (extract ID), Collection (unpack), and raw values. + */ + @SuppressWarnings("unchecked") + private Object[] resolveTraversalIds(final Traverser.Admin<S> traverser) { + final List<Object> ids = new ArrayList<>(); + TraversalUtil.prepare(traverser, (Traversal.Admin) this.idTraversal); + while (this.idTraversal.hasNext()) { + final Object result = this.idTraversal.next(); + if (result instanceof Element) { + ids.add(((Element) result).id()); + } else if (result instanceof Collection) { + for (final Object item : (Collection<?>) result) { + ids.add(item instanceof Element ? ((Element) item).id() : item); } + } else { + ids.add(result); } } + return ids.toArray(); + } + + /** + * Looks up elements (vertices or edges) by the given IDs using the graph. + */ + @SuppressWarnings("unchecked") + private Iterator<E> lookupElements(final Object[] elementIds) { + if (elementIds.length == 0) { + return EmptyIterator.instance(); + } + return (Iterator<E>) (Vertex.class.isAssignableFrom(this.returnClass) ? + this.getTraversal().getGraph().get().vertices(elementIds) : + this.getTraversal().getGraph().get().edges(elementIds)); } @Override @@ -182,6 +266,9 @@ public class GraphStep<S, E extends Element> extends AbstractStep<S, E> implemen this.head = null; this.done = false; this.iterator = EmptyIterator.instance(); + if (this.idTraversal != null) { + this.idTraversal.reset(); + } } @Override @@ -195,6 +282,31 @@ public class GraphStep<S, E extends Element> extends AbstractStep<S, E> implemen return result; } + @SuppressWarnings("unchecked") + @Override + public <S, E> List<Traversal.Admin<S, E>> getLocalChildren() { + return this.idTraversal != null + ? Collections.singletonList((Traversal.Admin<S, E>) this.idTraversal) + : Collections.emptyList(); + } + + @Override + public GraphStep<S, E> clone() { + final GraphStep<S, E> clone = (GraphStep<S, E>) super.clone(); + if (this.idTraversal != null) { + clone.idTraversal = this.idTraversal.clone(); + } + return clone; + } + + @Override + public void setTraversal(final Traversal.Admin<?, ?> parentTraversal) { + super.setTraversal(parentTraversal); + if (this.idTraversal != null) { + this.integrateChild(this.idTraversal); + } + } + /** * Attempts to close an underlying iterator if it is of type {@link CloseableIterator}. Graph providers may choose * to return this interface containing their vertices and edges if there are expensive resources that might need to @@ -203,6 +315,11 @@ public class GraphStep<S, E extends Element> extends AbstractStep<S, E> implemen @Override public void close() { CloseableIterator.closeIterator(iterator); + try { + TraversalParent.super.close(); + } catch (final Exception e) { + throw new RuntimeException(e); + } } /** @@ -213,6 +330,7 @@ public class GraphStep<S, E extends Element> extends AbstractStep<S, E> implemen * @return true if the {@link HasContainer} updated ids and thus, was processed. */ public static boolean processHasContainerIds(final GraphStep<?, ?> graphStep, final HasContainer hasContainer) { + if (hasContainer.hasTraversal()) return false; final String key = hasContainer.getKey(); if (key != null && key.equals(T.id.getAccessor()) && graphStep.ids.length == 0 && (hasContainer.getBiPredicate() == Compare.eq || hasContainer.getBiPredicate() == Contains.within)) { 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 4223f98bd5..7b5acbf44a 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 @@ -25,6 +25,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.step.util.Parameters; import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.*; import org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration.EventStrategy; import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement; +import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalUtil; import org.apache.tinkerpop.gremlin.structure.Edge; import org.apache.tinkerpop.gremlin.structure.Element; import org.apache.tinkerpop.gremlin.structure.Property; @@ -35,6 +36,7 @@ import org.apache.tinkerpop.gremlin.structure.util.StringFactory; import org.apache.tinkerpop.gremlin.structure.util.keyed.KeyedProperty; import org.apache.tinkerpop.gremlin.structure.util.keyed.KeyedVertexProperty; +import java.util.ArrayList; import java.util.Iterator; import java.util.List; import java.util.Map; @@ -47,15 +49,34 @@ 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. + */ + private final boolean mapForm; private CallbackRegistry<Event.ElementPropertyChangedEvent> callbackRegistry; public AddPropertyStep(final Traversal.Admin traversal, final VertexProperty.Cardinality cardinality, final Object keyObject, final Object valueObject) { + this(traversal, cardinality, keyObject, valueObject, false); + } + + public AddPropertyStep(final Traversal.Admin traversal, final VertexProperty.Cardinality cardinality, final Object keyObject, final Object valueObject, final boolean mapForm) { super(traversal); this.internalParameters.set(this, T.key, keyObject, T.value, valueObject); this.cardinality = cardinality; + this.mapForm = mapForm; } @Override @@ -89,11 +110,109 @@ public class AddPropertyStep<S extends Element> extends SideEffectStep<S> implem throw new IllegalStateException(String.format("T.%s is immutable on existing elements", ((T) k).name())); final String key = (String) k; - final Object value = this.internalParameters.get(traverser, T.value, () -> { - throw new IllegalStateException("The AddPropertyStep does not have a provided value: " + this); - }).get(0); + + // Check if the raw value is a traversal to enable multi-result handling. + // Exclude ConstantTraversal which is used internally by TinkerPop to wrap literal values. + final List<Object> rawValues = this.internalParameters.get(T.value, null); + final boolean valueIsTraversal = !rawValues.isEmpty() + && rawValues.get(0) instanceof Traversal.Admin + && !(rawValues.get(0) instanceof ConstantTraversal); + + if (valueIsTraversal) { + handleTraversalValue(traverser, key); + } else { + final Object value = this.internalParameters.get(traverser, T.value, () -> { + throw new IllegalStateException("The AddPropertyStep does not have a provided value: " + this); + }).get(0); + final Object[] vertexPropertyKeyValues = this.internalParameters.getKeyValues(traverser, T.key, T.value); + applyPropertyMutation(traverser, key, value, vertexPropertyKeyValues); + } + } + + /** + * Handles the case where the value argument is a child traversal, supporting multi-result + * resolution, Map-producing traversals, and cardinality-aware property setting. + */ + private void handleTraversalValue(final Traverser.Admin<S> traverser, final String key) { + final List<Object> rawValues = this.internalParameters.get(T.value, null); + final Traversal.Admin<?, ?> valueTraversal = (Traversal.Admin<?, ?>) rawValues.get(0); + + // Collect all results from the child traversal + final List<Object> results = new ArrayList<>(); + final Traverser.Admin<S> trav = traverser; + final Iterator<?> itty = TraversalUtil.<S, Object>applyAll(trav, (Traversal.Admin<S, Object>) (Traversal.Admin) valueTraversal); + while (itty.hasNext()) { + results.add(itty.next()); + } + + // No-result case: skip mutation, pass element through unchanged + if (results.isEmpty()) { + return; + } + + final Element element = traverser.get(); final Object[] vertexPropertyKeyValues = this.internalParameters.getKeyValues(traverser, T.key, T.value); + // Map-producing form: single-argument property(traversal) where the traversal produces a Map. + // Only the dedicated map form expands a Map into per-entry properties; for the key+traversal form a + // Map result is treated as an ordinary property value. + if (this.mapForm) { + if (results.size() == 1 && results.get(0) instanceof Map) { + final Map<?, ?> map = (Map<?, ?>) results.get(0); + for (final Map.Entry<?, ?> entry : map.entrySet()) { + if (!(entry.getKey() instanceof String)) { + throw new IllegalArgumentException( + "Property key must be a String but found " + entry.getKey().getClass().getSimpleName()); + } + final String mapKey = (String) entry.getKey(); + applyPropertyMutation(traverser, mapKey, entry.getValue(), vertexPropertyKeyValues); + } + return; + } + + // The map form requires a Map result. Reject anything else rather than guessing. + final Object result = results.get(0); + throw new IllegalArgumentException( + "property(traversal) requires the traversal to produce a Map, but got: " + + (result == null ? "null" : result.getClass().getSimpleName())); + } + + // Multi-result handling with cardinality awareness + if (results.size() > 1) { + // Determine effective cardinality + final VertexProperty.Cardinality effectiveCard = this.cardinality != null + ? this.cardinality + : (element instanceof Vertex ? element.graph().features().vertex().getCardinality(key) : null); + + if (effectiveCard == VertexProperty.Cardinality.single) { + throw new IllegalArgumentException( + "Single-cardinality property requires exactly one value, but traversal produced " + + results.size() + " results"); + } + + // For list/set cardinality with multiple results, add each as a separate property value + if (effectiveCard == VertexProperty.Cardinality.list || effectiveCard == VertexProperty.Cardinality.set) { + if (!(element instanceof Vertex)) { + throw new IllegalStateException(String.format( + "Property cardinality can only be set for a Vertex but the traversal encountered %s for key: %s", + element.getClass().getSimpleName(), key)); + } + for (final Object v : results) { + applyPropertyMutation(traverser, key, v, vertexPropertyKeyValues); + } + return; + } + } + + // Single result or default cardinality: use the first result + applyPropertyMutation(traverser, key, results.get(0), vertexPropertyKeyValues); + } + + /** + * Applies a single property mutation to the element, handling eventing and cardinality. + */ + private void applyPropertyMutation(final Traverser.Admin<S> traverser, final String key, + final Object value, final Object[] vertexPropertyKeyValues) { final Element element = traverser.get(); // can't set cardinality if the element is something other than a vertex as only vertices can have @@ -116,7 +235,7 @@ public class AddPropertyStep<S extends Element> extends SideEffectStep<S> implem final VertexProperty.Cardinality card = this.cardinality != null ? this.cardinality - : element.graph().features().vertex().getCardinality(key); + : (element instanceof Vertex ? element.graph().features().vertex().getCardinality(key) : null); // update property if (element instanceof Vertex) { @@ -183,7 +302,7 @@ public class AddPropertyStep<S extends Element> extends SideEffectStep<S> implem @Override public int hashCode() { - final int hash = super.hashCode() ^ this.internalParameters.hashCode() ^ this.withConfiguration.hashCode(); + int hash = super.hashCode() ^ this.internalParameters.hashCode() ^ this.withConfiguration.hashCode() ^ Boolean.hashCode(this.mapForm); return (null != this.cardinality) ? (hash ^ cardinality.hashCode()) : hash; } diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStepContract.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStepContract.java index c6547ad425..70f430db4a 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStepContract.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStepContract.java @@ -19,6 +19,7 @@ package org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect; import org.apache.tinkerpop.gremlin.process.traversal.Step; +import org.apache.tinkerpop.gremlin.process.traversal.step.AcceptsChildPredicateTraversal; import org.apache.tinkerpop.gremlin.process.traversal.step.Configuring; import org.apache.tinkerpop.gremlin.process.traversal.step.Deleting; import org.apache.tinkerpop.gremlin.process.traversal.step.GValue; @@ -33,7 +34,8 @@ import java.util.HashSet; import java.util.List; public interface AddPropertyStepContract<S> extends Step<S, S>, TraversalParent, Scoping, PropertiesHolder, - Writing<Event.ElementPropertyChangedEvent>, Deleting<Event.ElementPropertyChangedEvent>, Configuring { + Writing<Event.ElementPropertyChangedEvent>, Deleting<Event.ElementPropertyChangedEvent>, Configuring, + AcceptsChildPredicateTraversal { /** * Concrete implementations of this contract that can be referenced as TinkerPop implementations. diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStepPlaceholder.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStepPlaceholder.java index 3bb1c087cf..3dd2734cf3 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStepPlaceholder.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStepPlaceholder.java @@ -62,8 +62,18 @@ public class AddPropertyStepPlaceholder<S extends Element> extends SideEffectSte private Parameters withConfiguration = new Parameters(); + /** + * Marks the single-argument {@code property(traversal)} (Map-producing) form. + */ + private boolean mapForm; + public AddPropertyStepPlaceholder(final Traversal.Admin traversal, final VertexProperty.Cardinality cardinality, final Object keyObject, final Object valueObject) { + this(traversal, cardinality, keyObject, valueObject, false); + } + + public AddPropertyStepPlaceholder(final Traversal.Admin traversal, final VertexProperty.Cardinality cardinality, final Object keyObject, final Object valueObject, final boolean mapForm) { super(traversal); + this.mapForm = mapForm; if (keyObject instanceof GValue) { throw new IllegalArgumentException("GValue is not allowed for property keys"); } @@ -128,13 +138,14 @@ public class AddPropertyStepPlaceholder<S extends Element> extends SideEffectSte return Objects.equals(key, that.key) && Objects.equals(value, that.value) && cardinality == that.cardinality && + mapForm == that.mapForm && Objects.equals(properties, that.properties) && Objects.equals(withConfiguration, that.withConfiguration); } @Override public int hashCode() { - return Objects.hash(super.hashCode(), key, value, cardinality, properties, withConfiguration); + return Objects.hash(super.hashCode(), key, value, cardinality, mapForm, properties, withConfiguration); } @Override @@ -176,6 +187,7 @@ public class AddPropertyStepPlaceholder<S extends Element> extends SideEffectSte public AddPropertyStepPlaceholder<S> clone() { final AddPropertyStepPlaceholder<S> clone = (AddPropertyStepPlaceholder<S>) super.clone(); clone.cardinality = cardinality; + clone.mapForm = mapForm; // Attempt to deep clone key for Traversal and GValue. Shallow copy is fine if key is a String or enum if (this.key instanceof Traversal) { @@ -218,7 +230,7 @@ public class AddPropertyStepPlaceholder<S extends Element> extends SideEffectSte @Override public AddPropertyStep<S> asConcreteStep() { - AddPropertyStep<S> step = new AddPropertyStep<>(traversal, cardinality, key, value instanceof GValue ? ((GValue<?>) value).get() : value); + AddPropertyStep<S> step = new AddPropertyStep<>(traversal, cardinality, key, value instanceof GValue ? ((GValue<?>) value).get() : value, mapForm); for (final Map.Entry<Object, List<Object>> entry : properties.entrySet()) { for (Object value : entry.getValue()) { 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 99a9c4e959..c6d0d1fe29 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.clone(); + clone.predicate = this.predicate != null ? this.predicate.clone() : null; return clone; } catch (final CloneNotSupportedException e) { throw new IllegalStateException(e.getMessage(), e); @@ -142,6 +142,14 @@ public class HasContainer implements Serializable, Cloneable, Predicate<Element> return this.predicate.getValue(); } + /** + * Determines if this {@code HasContainer}'s predicate holds a child traversal whose result is resolved + * at runtime (e.g. {@code P.eq(traversal)} or {@code P.within(traversal)}). + */ + public boolean hasTraversal() { + return this.predicate != null && this.predicate.hasTraversal(); + } + //////////// /** diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategy.java index 8a6edcde28..d8c260d87f 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategy.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategy.java @@ -114,6 +114,10 @@ public final class InlineFilterStrategy extends AbstractTraversalStrategy<Traver /////////////////////////// private static boolean processHasStep(final HasStep<?> step, final Traversal.Admin<?, ?> traversal) { + if (step.getHasContainers().stream().anyMatch(HasContainer::hasTraversal)) return false; + if (step.getPreviousStep() instanceof HasStep) { + if (((HasStep<?>) step.getPreviousStep()).getHasContainers().stream().anyMatch(HasContainer::hasTraversal)) return false; + } if (step.getPreviousStep() instanceof HasStep) { final HasStep<?> previousStep = (HasStep<?>) step.getPreviousStep(); final List<HasContainer> hasContainers = new ArrayList<>(step.getHasContainers()); @@ -221,6 +225,11 @@ public final class InlineFilterStrategy extends AbstractTraversalStrategy<Traver InlineFilterStrategy.instance().apply(childTraversal); for (final Step<?, ?> childStep : childTraversal.getSteps()) { if (childStep instanceof HasStep) { + // Skip optimization if any HasContainer has a traversal-bearing predicate + if (((HasStep<?>) childStep).getHasContainers().stream().anyMatch(HasContainer::hasTraversal)) { + process = false; + break; + } P p = null; for (final HasContainer hasContainer : ((HasStep<?>) childStep).getHasContainers()) { if (null == key) diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/verification/ChildTraversalVerificationStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/verification/ChildTraversalVerificationStrategy.java new file mode 100644 index 0000000000..240cc08a85 --- /dev/null +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/verification/ChildTraversalVerificationStrategy.java @@ -0,0 +1,61 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.tinkerpop.gremlin.process.traversal.strategy.verification; + +import org.apache.tinkerpop.gremlin.process.traversal.Step; +import org.apache.tinkerpop.gremlin.process.traversal.Traversal; +import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; +import org.apache.tinkerpop.gremlin.process.traversal.step.AcceptsChildPredicateTraversal; +import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; +import org.apache.tinkerpop.gremlin.process.traversal.strategy.AbstractTraversalStrategy; +import org.apache.tinkerpop.gremlin.process.traversal.util.ChildTraversalValidator; + +/** + * Validates that child traversals in filter, lookup, and mutation steps do not contain mutating steps. + * Serves as a safety net for programmatic traversal construction that bypasses the DSL's + * construction-time validation. + */ +public final class ChildTraversalVerificationStrategy + extends AbstractTraversalStrategy<TraversalStrategy.VerificationStrategy> + implements TraversalStrategy.VerificationStrategy { + + private static final ChildTraversalVerificationStrategy INSTANCE = new ChildTraversalVerificationStrategy(); + + private ChildTraversalVerificationStrategy() { + } + + @Override + public void apply(final Traversal.Admin<?, ?> traversal) { + for (final Step<?, ?> step : traversal.getSteps()) { + if (step instanceof AcceptsChildPredicateTraversal && step instanceof TraversalParent) { + for (final Traversal.Admin<?, ?> child : ((TraversalParent) step).getLocalChildren()) { + try { + ChildTraversalValidator.validate(child); + } catch (final IllegalArgumentException e) { + throw new VerificationException(e.getMessage(), traversal); + } + } + } + } + } + + public static ChildTraversalVerificationStrategy instance() { + return INSTANCE; + } +} diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/ChildTraversalValidator.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/ChildTraversalValidator.java new file mode 100644 index 0000000000..5e54d6d599 --- /dev/null +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/ChildTraversalValidator.java @@ -0,0 +1,62 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.tinkerpop.gremlin.process.traversal.util; + +import org.apache.tinkerpop.gremlin.process.traversal.Step; +import org.apache.tinkerpop.gremlin.process.traversal.Traversal; +import org.apache.tinkerpop.gremlin.process.traversal.step.Mutating; +import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; + +/** + * Validates that child traversals do not contain mutating steps. Child traversals used as + * arguments to filter predicates ({@code has()}, {@code is()}, etc.), lookup steps + * ({@code V(traversal)}, {@code E(traversal)}), and mutation steps ({@code property(traversal)}) + * must be read-only - their purpose is to compute values, not produce side effects. + * <p> + * Recursion walks both {@link TraversalParent#getLocalChildren()} and + * {@link TraversalParent#getGlobalChildren()} to detect mutations nested inside + * {@code map()}, {@code union()}, {@code choose()}, {@code coalesce()}, or any other parent step. + */ +public final class ChildTraversalValidator { + + private ChildTraversalValidator() { + } + + /** + * Validates that a child traversal contains no {@link Mutating} steps at any nesting depth. + * Throws {@link IllegalArgumentException} if one is found. + */ + public static void validate(final Traversal.Admin<?, ?> child) { + for (final Step<?, ?> step : child.getSteps()) { + if (step instanceof Mutating) { + throw new IllegalArgumentException( + "Child traversal contains mutating step " + + step.getClass().getSimpleName() + ". Mutating steps are not allowed in child traversals."); + } + if (step instanceof TraversalParent) { + for (final Traversal.Admin<?, ?> nested : ((TraversalParent) step).getLocalChildren()) { + validate(nested); + } + for (final Traversal.Admin<?, ?> nested : ((TraversalParent) step).getGlobalChildren()) { + validate(nested); + } + } + } + } +} 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 new file mode 100644 index 0000000000..09af6da179 --- /dev/null +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/CloneIndependenceTraversalTest.java @@ -0,0 +1,120 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.tinkerpop.gremlin.process.traversal.step; + +import org.apache.tinkerpop.gremlin.process.traversal.P; +import org.apache.tinkerpop.gremlin.process.traversal.Traversal; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; +import org.apache.tinkerpop.gremlin.process.traversal.step.filter.HasStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.map.GraphStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.util.HasContainer; +import org.apache.tinkerpop.gremlin.process.traversal.util.EmptyTraversal; +import org.apache.tinkerpop.gremlin.structure.Vertex; +import org.junit.Test; + +import java.util.List; + +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.not; +import static org.hamcrest.CoreMatchers.notNullValue; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.core.IsSame.sameInstance; + +/** + * Property 8: Clone independence for traversal-bearing steps. + * <p> + * 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 { + + @Test + public void shouldProduceIndependentCloneForHasStepWithTraversalValue() { + // Create a HasStep with a traversal-bearing HasContainer (P.eq(traversal)) + final Traversal.Admin<?, ?> childTraversal = __.constant("marko").asAdmin(); + final HasContainer hc = new HasContainer("name", P.eq(childTraversal)); + final HasStep<Vertex> original = new HasStep<>(EmptyTraversal.instance(), hc); + + // Clone the step + final HasStep<Vertex> clone = original.clone(); + + // Verify the clone has child traversals + final List<Traversal.Admin<?, ?>> originalChildren = (List) original.getLocalChildren(); + final List<Traversal.Admin<?, ?>> cloneChildren = (List) clone.getLocalChildren(); + + assertThat(originalChildren.size(), is(1)); + assertThat(cloneChildren.size(), is(1)); + + // The child traversals should be different instances (deep copy) + assertThat(cloneChildren.get(0), is(not(sameInstance(originalChildren.get(0))))); + + // Verify HasContainers are also independent + final List<HasContainer> originalContainers = original.getHasContainers(); + final List<HasContainer> cloneContainers = clone.getHasContainers(); + assertThat(cloneContainers.get(0), is(not(sameInstance(originalContainers.get(0))))); + assertThat(cloneContainers.get(0).getPredicate().getTraversalValue(), + is(not(sameInstance(originalContainers.get(0).getPredicate().getTraversalValue())))); + } + + @Test + public void shouldProduceIndependentCloneForGraphStepWithIdTraversal() { + // Create a GraphStep with an idTraversal + final Traversal.Admin<?, ?> idTraversal = __.select("ids").asAdmin(); + final GraphStep<Vertex, Vertex> original = + new GraphStep<>(EmptyTraversal.instance(), Vertex.class, false, idTraversal); + + // Clone the step + final GraphStep<Vertex, Vertex> clone = original.clone(); + + // Verify the clone has a child traversal + assertThat(original.getIdTraversal(), is(notNullValue())); + assertThat(clone.getIdTraversal(), is(notNullValue())); + + // The idTraversal should be a different instance (deep copy) + assertThat(clone.getIdTraversal(), is(not(sameInstance(original.getIdTraversal())))); + } + + @Test + public void shouldProduceIndependentCloneForHasStepWithMultipleContainers() { + // Create a HasStep with multiple HasContainers, some with traversals + final HasContainer hc1 = new HasContainer("name", P.eq(__.constant("marko").asAdmin())); + final HasContainer hc2 = new HasContainer("age", P.eq(__.constant(29).asAdmin())); + final HasStep<Vertex> original = new HasStep<>(EmptyTraversal.instance(), hc1, hc2); + + // Clone the step + final HasStep<Vertex> clone = original.clone(); + + // Verify all containers are independent + final List<HasContainer> originalContainers = original.getHasContainers(); + final List<HasContainer> cloneContainers = clone.getHasContainers(); + + assertThat(originalContainers.size(), is(2)); + assertThat(cloneContainers.size(), is(2)); + + for (int i = 0; i < originalContainers.size(); i++) { + assertThat(cloneContainers.get(i), is(not(sameInstance(originalContainers.get(i))))); + assertThat(cloneContainers.get(i).getPredicate().getTraversalValue(), + is(not(sameInstance(originalContainers.get(i).getPredicate().getTraversalValue())))); + } + } + +} 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 new file mode 100644 index 0000000000..4049b20eb6 --- /dev/null +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/HasContainerTest.java @@ -0,0 +1,90 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.tinkerpop.gremlin.process.traversal.step.util; + +import org.apache.tinkerpop.gremlin.process.traversal.P; +import org.apache.tinkerpop.gremlin.process.traversal.Traversal; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; +import org.junit.Test; + +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.notNullValue; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.core.IsNot.not; +import static org.hamcrest.core.IsSame.sameInstance; +import static org.junit.Assert.assertEquals; + +/** + * Tests for {@link HasContainer}, including traversal-bearing predicates and lifecycle methods. + * A traversal-bearing container holds a {@link P} whose value is resolved at runtime from a child traversal + * (e.g. {@code P.eq(traversal)}); the container exposes this via {@link HasContainer#hasTraversal()}. + */ +public class HasContainerTest { + + @Test + public void shouldReturnFalseForHasTraversalWithLiteralPredicate() { + final HasContainer hc = new HasContainer("name", P.eq("marko")); + assertThat(hc.hasTraversal(), is(false)); + } + + @Test + public void shouldReturnTrueForHasTraversalWithTraversalPredicate() { + final Traversal.Admin<?, ?> traversal = __.identity().asAdmin(); + final HasContainer hc = new HasContainer("name", P.eq(traversal)); + assertThat(hc.hasTraversal(), is(true)); + } + + @Test + public void shouldSetFieldsCorrectlyWithTraversalPredicate() { + final Traversal.Admin<?, ?> traversal = __.identity().asAdmin(); + 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))); + } + + @Test + public void shouldCloneProduceIndependentDeepCopyOfTraversalPredicate() { + final Traversal.Admin<?, ?> traversal = __.identity().asAdmin(); + final HasContainer original = new HasContainer("name", P.eq(traversal)); + final HasContainer clone = original.clone(); + + // clone's predicate should carry a traversal that is not the same instance + assertThat(clone.getPredicate().getTraversalValue(), is(notNullValue())); + assertThat(clone.getPredicate().getTraversalValue(), + is(not(sameInstance(original.getPredicate().getTraversalValue())))); + + // key should be equal + assertEquals(original.getKey(), clone.getKey()); + + // both should still report hasTraversal + assertThat(clone.hasTraversal(), is(true)); + } + + @Test + public void shouldCloneLiteralHasContainerWithoutTraversal() { + final HasContainer original = new HasContainer("name", P.eq("marko")); + final HasContainer clone = original.clone(); + + assertThat(clone.hasTraversal(), is(false)); + assertEquals("name", clone.getKey()); + assertEquals(P.eq("marko"), clone.getPredicate()); + } +} diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/verification/ChildTraversalVerificationStrategyTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/verification/ChildTraversalVerificationStrategyTest.java new file mode 100644 index 0000000000..feb0688283 --- /dev/null +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/verification/ChildTraversalVerificationStrategyTest.java @@ -0,0 +1,69 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.tinkerpop.gremlin.process.traversal.strategy.verification; + +import org.apache.tinkerpop.gremlin.process.traversal.Traversal; +import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategies; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; +import org.apache.tinkerpop.gremlin.structure.Graph; +import org.apache.tinkerpop.gremlin.structure.util.empty.EmptyGraph; +import org.junit.Test; + +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.assertFalse; + +/** + * Tests for the ChildTraversalVerificationStrategy (strategy-time layer). + */ +public class ChildTraversalVerificationStrategyTest { + + private final GraphTraversalSource g = EmptyGraph.instance().traversal(); + + @Test + public void shouldBeRegisteredByDefault() { + // The strategy should be present in the default Graph strategy set (not EmptyGraph which has none) + assertTrue(TraversalStrategies.GlobalCache + .getStrategies(Graph.class) + .getStrategy(ChildTraversalVerificationStrategy.class).isPresent()); + } + + @Test + public void shouldBeRemovableViaWithoutStrategies() { + // Users should be able to remove the strategy if needed + final GraphTraversalSource gNoVerify = g.withoutStrategies(ChildTraversalVerificationStrategy.class); + assertFalse(gNoVerify.getStrategies().getStrategy(ChildTraversalVerificationStrategy.class).isPresent()); + } + + @Test + public void shouldNotRejectValidReadOnlyTraversal() { + // Valid traversal should pass strategy application without error + final Traversal.Admin<?, ?> traversal = g.V().has("name", __.V().values("name")).asAdmin(); + traversal.applyStrategies(); + // If we get here without exception, the test passes + } + + @Test + public void shouldNotRejectValidLookupTraversal() { + // Valid V(traversal) should pass + final Traversal.Admin<?, ?> traversal = g.V().V(__.out("knows").id()).asAdmin(); + traversal.applyStrategies(); + } +} 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 new file mode 100644 index 0000000000..68b99296ec --- /dev/null +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/util/ChildTraversalValidatorTest.java @@ -0,0 +1,229 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.tinkerpop.gremlin.process.traversal.util; + +import org.apache.tinkerpop.gremlin.process.traversal.P; +import org.apache.tinkerpop.gremlin.process.traversal.TextP; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; +import org.apache.tinkerpop.gremlin.structure.util.empty.EmptyGraph; +import org.junit.Test; + +import static org.junit.Assert.assertThrows; + +/** + * Tests for construction-time child traversal validation. + */ +public class ChildTraversalValidatorTest { + + private final GraphTraversalSource g = EmptyGraph.instance().traversal(); + + // ===== FILTER CONTEXT: should reject mutating steps ===== + + @Test + public void shouldRejectAddVInHasTraversal() { + assertThrows(IllegalArgumentException.class, () -> + g.V().has("name", __.addV("x").values("name"))); + } + + @Test + public void shouldRejectDropInHasTraversal() { + assertThrows(IllegalArgumentException.class, () -> + g.V().has("name", __.V().drop().constant("x"))); + } + + @Test + public void shouldRejectNestedMutatingInHasTraversal() { + assertThrows(IllegalArgumentException.class, () -> + g.V().has("name", __.V().map(__.addV("x")).values("name"))); + } + + @Test + public void shouldRejectMutatingInHasWithTAccessor() { + assertThrows(IllegalArgumentException.class, () -> + g.V().has(org.apache.tinkerpop.gremlin.structure.T.label, __.addV("x").label())); + } + + @Test + public void shouldRejectMutatingInHasWithLabel() { + assertThrows(IllegalArgumentException.class, () -> + g.V().has("person", "name", __.addV("x").values("name"))); + } + + // ===== LOOKUP CONTEXT: should reject mutating steps ===== + + @Test + public void shouldRejectAddVInMidTraversalV() { + assertThrows(IllegalArgumentException.class, () -> + g.V().V(__.addV("x").id())); + } + + @Test + public void shouldRejectAddVInMidTraversalE() { + assertThrows(IllegalArgumentException.class, () -> + g.V().E(__.addV("x").id())); + } + + @Test + public void shouldRejectAddVInStartStepV() { + assertThrows(IllegalArgumentException.class, () -> + g.V(__.addV("x").id())); + } + + @Test + public void shouldRejectAddVInStartStepE() { + assertThrows(IllegalArgumentException.class, () -> + g.E(__.addV("x").id())); + } + + // ===== P FACTORY METHODS: should reject mutating steps ===== + + @Test + public void shouldRejectMutatingInPEq() { + assertThrows(IllegalArgumentException.class, () -> + P.eq(__.addV("x").values("name"))); + } + + @Test + public void shouldRejectMutatingInPNeq() { + assertThrows(IllegalArgumentException.class, () -> + P.neq(__.addV("x").values("name"))); + } + + @Test + public void shouldRejectMutatingInPGt() { + assertThrows(IllegalArgumentException.class, () -> + P.gt(__.addV("x").values("age"))); + } + + @Test + public void shouldRejectMutatingInPLt() { + assertThrows(IllegalArgumentException.class, () -> + P.lt(__.addV("x").values("age"))); + } + + @Test + public void shouldRejectMutatingInPGte() { + assertThrows(IllegalArgumentException.class, () -> + P.gte(__.addV("x").values("age"))); + } + + @Test + public void shouldRejectMutatingInPLte() { + assertThrows(IllegalArgumentException.class, () -> + P.lte(__.addV("x").values("age"))); + } + + @Test + public void shouldRejectMutatingInPWithin() { + assertThrows(IllegalArgumentException.class, () -> + P.within(__.addV("x").values("name"))); + } + + @Test + public void shouldRejectMutatingInPWithout() { + assertThrows(IllegalArgumentException.class, () -> + P.without(__.addV("x").values("name"))); + } + + @Test + public void shouldRejectMutatingInMultiTraversalWithin() { + assertThrows(IllegalArgumentException.class, () -> + P.within(__.V().values("name"), __.addV("x").values("name"))); + } + + @Test + public void shouldRejectMutatingInMultiTraversalWithout() { + assertThrows(IllegalArgumentException.class, () -> + P.without(__.V().values("name"), __.addV("x").values("name"))); + } + + // ===== TextP FACTORY METHODS: should reject mutating steps ===== + + @Test + public void shouldRejectMutatingInTextPStartingWith() { + assertThrows(IllegalArgumentException.class, () -> + TextP.startingWith(__.addV("x").values("name"))); + } + + @Test + public void shouldRejectMutatingInTextPContaining() { + assertThrows(IllegalArgumentException.class, () -> + TextP.containing(__.addV("x").values("name"))); + } + + // ===== MUTATION CONTEXT: should reject DropStep but allow other mutations ===== + + @Test + public void shouldRejectDropInPropertyTraversal() { + assertThrows(IllegalArgumentException.class, () -> + g.V().property(__.V().map(__.drop()).project("x").by("name"))); + } + + @Test + public void shouldRejectNestedDropInPropertyTraversal() { + assertThrows(IllegalArgumentException.class, () -> + g.V().property(__.V().union(__.drop(), __.constant("x")).project("k").by())); + } + + // ===== VALID TRAVERSALS: should pass without error ===== + + @Test + public void shouldAllowReadOnlyHasTraversal() { + // Should not throw + g.V().has("name", __.V().values("name")); + } + + @Test + public void shouldAllowNavigationInHasTraversal() { + // Should not throw + g.V().has("name", __.V().out("knows").values("name")); + } + + @Test + public void shouldAllowReadOnlyLookupTraversal() { + // Should not throw + g.V().V(__.out("knows").id()); + } + + @Test + public void shouldAllowReadOnlyStartStepV() { + // Should not throw + g.V(__.V().id()); + } + + @Test + public void shouldAllowReadOnlyPredicate() { + // Should not throw + P.eq(__.V().values("age")); + } + + @Test + public void shouldRejectAddVInMutationContext() { + // addV is now blocked in property(traversal) - all mutating steps are blocked in all contexts + assertThrows(IllegalArgumentException.class, () -> + g.V().property(__.V().addV("temp").project("k").by("name"))); + } + + @Test + public void shouldAllowReadOnlyPropertyTraversal() { + // Should not throw + g.V().property(__.V().project("name").by("name")); + } +} diff --git a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/step/sideEffect/TinkerGraphStep.java b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/step/sideEffect/TinkerGraphStep.java index a950daeefe..fc7c64b060 100644 --- a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/step/sideEffect/TinkerGraphStep.java +++ b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/step/sideEffect/TinkerGraphStep.java @@ -59,6 +59,14 @@ public final class TinkerGraphStep<S, E extends Element> extends GraphStep<S, E> super(originalGraphStep.getTraversal(), originalGraphStep.getReturnClass(), originalGraphStep.isStartStep(), originalGraphStep.getIds()); originalGraphStep.getLabels().forEach(this::addLabel); + // preserve the idTraversal if the original GraphStep had one + if (originalGraphStep instanceof GraphStep) { + final GraphStep<S, E> gs = (GraphStep<S, E>) originalGraphStep; + if (gs.getIdTraversal() != null) { + this.setIdTraversal(gs.getIdTraversal()); + } + } + // we used to only setIteratorSupplier() if there were no ids OR the first id was instanceof Element, // but that allowed the filter in g.V(v).has('k','v') to be ignored. this created problems for // PartitionStrategy which wants to prevent someone from passing "v" from one TraversalSource to diff --git a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/strategy/optimization/TinkerGraphStepStrategy.java b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/strategy/optimization/TinkerGraphStepStrategy.java index c9e19593a7..947cdcefa3 100644 --- a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/strategy/optimization/TinkerGraphStepStrategy.java +++ b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/strategy/optimization/TinkerGraphStepStrategy.java @@ -54,8 +54,18 @@ public final class TinkerGraphStepStrategy extends AbstractTraversalStrategy<Tra Step<?, ?> currentStep = tinkerGraphStep.getNextStep(); while (currentStep instanceof HasStep || currentStep instanceof NoOpBarrierStep) { if (currentStep instanceof HasStep) { - List<HasContainer> hasContainers = ((HasContainerHolder) currentStep).getHasContainers(); - for (HasContainer hasContainer : hasContainers) { + final List<HasContainer> hasContainers = ((HasContainerHolder) currentStep).getHasContainers(); + + // skip folding this HasStep if any HasContainer holds a child traversal - + // its value is dynamic (resolved per-traverser) and cannot be pushed into + // TinkerGraphStep. Continue to the next step so that subsequent literal + // HasSteps can still be folded. + if (hasContainers.stream().anyMatch(HasContainer::hasTraversal)) { + currentStep = currentStep.getNextStep(); + continue; + } + + for (final HasContainer hasContainer : hasContainers) { if (!GraphStep.processHasContainerIds(tinkerGraphStep, hasContainer)) tinkerGraphStep.addHasContainer(hasContainer); } diff --git a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/strategy/optimization/TinkerGraphStepStrategyTraversalTest.java b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/strategy/optimization/TinkerGraphStepStrategyTraversalTest.java new file mode 100644 index 0000000000..a91e89ea52 --- /dev/null +++ b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/strategy/optimization/TinkerGraphStepStrategyTraversalTest.java @@ -0,0 +1,205 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.tinkerpop.gremlin.tinkergraph.process.traversal.strategy.optimization; + +import org.apache.tinkerpop.gremlin.process.traversal.Step; +import org.apache.tinkerpop.gremlin.process.traversal.Traversal; +import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategies; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversal; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; +import org.apache.tinkerpop.gremlin.process.traversal.step.filter.HasStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.util.HasContainer; +import org.apache.tinkerpop.gremlin.process.traversal.util.DefaultTraversalStrategies; +import org.apache.tinkerpop.gremlin.structure.Graph; +import org.apache.tinkerpop.gremlin.structure.Vertex; +import org.apache.tinkerpop.gremlin.tinkergraph.process.traversal.step.sideEffect.TinkerGraphStep; +import org.apache.tinkerpop.gremlin.tinkergraph.structure.TinkerGraph; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import java.util.List; + +import static org.hamcrest.CoreMatchers.instanceOf; +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.greaterThanOrEqualTo; +import static org.hamcrest.Matchers.hasSize; + +/** + * Property 9: Traversal-bearing HasContainers are not folded into GraphStep. + * <p> + * For any traversal of the form {@code g.V().has(key, traversal)} or {@code g.V().has(key, P.eq(traversal))}, + * after applying {@link TinkerGraphStepStrategy}, the {@link HasStep} SHALL remain as a separate step in the + * traversal plan and SHALL NOT be folded into the {@link TinkerGraphStep}. + * <p> + * <b>Validates: Requirements 8.1, 8.2, 8.3</b> + */ +public class TinkerGraphStepStrategyTraversalTest { + + private Graph graph; + private GraphTraversalSource g; + + @Before + public void setup() { + graph = TinkerGraph.open(); + g = graph.traversal(); + } + + @After + public void teardown() throws Exception { + graph.close(); + } + + /** + * Applies TinkerGraphStepStrategy to the given traversal and returns the step list. + */ + private List<Step> applyStrategy(final Traversal.Admin<?, ?> traversal) { + final TraversalStrategies strategies = new DefaultTraversalStrategies(); + strategies.addStrategies(TinkerGraphStepStrategy.instance()); + traversal.setStrategies(strategies); + traversal.applyStrategies(); + return traversal.getSteps(); + } + + @Test + public void shouldNotFoldTraversalBearingHasContainerIntoGraphStep() { + // g.V().has("name", __.constant("marko")) - traversal-bearing HasContainer + final GraphTraversal<Vertex, Vertex> traversal = g.V().has("name", __.constant("marko")); + final List<Step> steps = applyStrategy(traversal.asAdmin()); + + // The traversal plan should contain at least 2 steps: + // TinkerGraphStep (replacing GraphStep) and HasStep (not folded) + assertThat(steps, hasSize(greaterThanOrEqualTo(2))); + assertThat(steps.get(0), instanceOf(TinkerGraphStep.class)); + + // The HasStep should remain as a separate step + final boolean hasStepPresent = steps.stream().anyMatch(s -> s instanceof HasStep); + assertThat("HasStep with traversal-bearing HasContainer should NOT be folded into TinkerGraphStep", + hasStepPresent, is(true)); + + // The TinkerGraphStep should have no folded HasContainers + final TinkerGraphStep<?, ?> tinkerGraphStep = (TinkerGraphStep<?, ?>) steps.get(0); + assertThat("TinkerGraphStep should have no folded HasContainers for traversal-bearing has()", + tinkerGraphStep.getHasContainers(), hasSize(0)); + } + + @Test + public void shouldStillFoldLiteralHasContainerIntoGraphStep() { + // g.V().has("name", "marko") - literal HasContainer should still be folded + final GraphTraversal<Vertex, Vertex> traversal = g.V().has("name", "marko"); + final List<Step> steps = applyStrategy(traversal.asAdmin()); + + // After folding, the HasStep should be absorbed into TinkerGraphStep + assertThat(steps, hasSize(1)); + assertThat(steps.get(0), instanceOf(TinkerGraphStep.class)); + + // The TinkerGraphStep should have the folded HasContainer + final TinkerGraphStep<?, ?> tinkerGraphStep = (TinkerGraphStep<?, ?>) steps.get(0); + assertThat(tinkerGraphStep.getHasContainers(), hasSize(1)); + } + + @Test + public void shouldNotFoldPredicateWithTraversalIntoGraphStep() { + // g.V().has("name", P.eq(__.constant("marko"))) - predicate with traversal + final GraphTraversal<Vertex, Vertex> traversal = + g.V().has("name", org.apache.tinkerpop.gremlin.process.traversal.P.eq(__.constant("marko").asAdmin())); + final List<Step> steps = applyStrategy(traversal.asAdmin()); + + // The HasStep should remain as a separate step + assertThat(steps, hasSize(greaterThanOrEqualTo(2))); + assertThat(steps.get(0), instanceOf(TinkerGraphStep.class)); + + final boolean hasStepPresent = steps.stream().anyMatch(s -> s instanceof HasStep); + assertThat("HasStep with P.eq(traversal) should NOT be folded into TinkerGraphStep", + hasStepPresent, is(true)); + } + + @Test + public void shouldFoldLiteralHasButNotTraversalHasInMixedTraversal() { + // g.V().has("name", "marko").has("age", __.constant(29)) + // Both containers end up in the same HasStep (TraversalHelper.addHasContainer merges). + // The strategy skips the entire HasStep because it contains a traversal-bearing container. + final GraphTraversal<Vertex, Vertex> traversal = + g.V().has("name", "marko").has("age", __.constant(29)); + final List<Step> steps = applyStrategy(traversal.asAdmin()); + + // TinkerGraphStep should have NO folded HasContainers (the merged HasStep is skipped) + assertThat(steps.get(0), instanceOf(TinkerGraphStep.class)); + final TinkerGraphStep<?, ?> tinkerGraphStep = (TinkerGraphStep<?, ?>) steps.get(0); + + // The HasStep with both containers should remain as a separate step + final boolean hasStepPresent = steps.stream().anyMatch(s -> s instanceof HasStep); + assertThat("HasStep with mixed containers should remain as separate step", + hasStepPresent, is(true)); + } + + @Test + public void shouldFoldLiteralHasAfterBarrierAndTraversalHas() { + // g.V().has("age", __.constant(29)).barrier().has("name", "marko") + // The barrier separates the two HasSteps. The strategy should: + // - Skip the traversal-bearing HasStep (has("age", traversal)) + // - Stop at the barrier (not a HasStep or NoOpBarrierStep... actually NoOpBarrierStep IS handled) + // Let's use a different separator. Actually NoOpBarrierStep is handled by the while loop. + // The key test is: after skipping a traversal-bearing HasStep, the strategy continues + // and can fold subsequent literal HasSteps. + // + // With separate HasSteps (not merged), the strategy should fold the literal one. + // We can force separate HasSteps by inserting a NoOpBarrierStep between them. + final GraphTraversal<Vertex, Vertex> traversal = + g.V().has("age", __.constant(29)).barrier().has("name", "marko"); + final List<Step> steps = applyStrategy(traversal.asAdmin()); + + // TinkerGraphStep should have the literal "name" HasContainer folded + assertThat(steps.get(0), instanceOf(TinkerGraphStep.class)); + final TinkerGraphStep<?, ?> tinkerGraphStep = (TinkerGraphStep<?, ?>) steps.get(0); + assertThat("Literal HasContainer after barrier should be folded even when preceded by traversal-bearing HasStep", + tinkerGraphStep.getHasContainers(), hasSize(1)); + assertThat(tinkerGraphStep.getHasContainers().get(0).getKey(), is("name")); + + // The traversal-bearing HasStep for "age" should remain + final boolean hasStepPresent = steps.stream().anyMatch(s -> s instanceof HasStep); + assertThat("Traversal-bearing HasStep should remain as separate step", + hasStepPresent, is(true)); + } + + @Test + public void shouldFoldMultipleLiteralHasStepsSeparatedByTraversalHas() { + // g.V().has("name", "marko").has("age", __.constant(29)).barrier().has("lang", "java") + // has("name") is a separate literal HasStep, has("age") is a separate traversal HasStep, + // has("lang") is a separate literal HasStep after the barrier. + // Strategy should fold both literal HasSteps and skip the traversal one. + final GraphTraversal<Vertex, Vertex> traversal = + g.V().has("name", "marko").has("age", __.constant(29)).barrier().has("lang", "java"); + final List<Step> steps = applyStrategy(traversal.asAdmin()); + + // TinkerGraphStep should have both literal HasContainers folded ("name" and "lang") + assertThat(steps.get(0), instanceOf(TinkerGraphStep.class)); + final TinkerGraphStep<?, ?> tinkerGraphStep = (TinkerGraphStep<?, ?>) steps.get(0); + assertThat("Both literal HasContainers should be folded", + tinkerGraphStep.getHasContainers(), hasSize(2)); + + // The traversal-bearing HasStep (age) should remain + final boolean hasStepPresent = steps.stream().anyMatch(s -> s instanceof HasStep); + assertThat("Traversal-bearing HasStep should remain as separate step", + hasStepPresent, is(true)); + } + +}
