Ensure that Graph is set after strategy application

Discussed on #1699 CTR

CHANGELOG.asciidoc

@@ -40,6 +40,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima
 * Fixed bug in `PartitionStrategy` where the use of `AbstractLambdaTraversal` caused an unexpected exception.
 * Fixed bug where close requests for sessions were improperly validating the request in the `UnifiedChannelizer`.
 * Deprecated and removed functionality of the `connectOnStartup` option in `gremlin-javascript` to resolve potential `unhandledRejection` and race conditions.
+* Ensured `Graph` instance was set between `TraversalStrategy` executions.
 * Fixed potential `NullPointerException` in `gremlin-driver` where initialization of a `ConnectionPool` would fail but not throw an exception due to centralized error check being satisfied by a different process.
 * Fixed a bug where the JavaScript client would hang indefinitely on traversals if the connection to the server was terminated.
 

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/PartitionStrategy.java

@@ -126,7 +126,7 @@ public void apply(final Traversal.Admin<?, ?> traversal) {
         // writes.
         final Optional<Graph.Features.VertexFeatures> vertexFeatures;
         if (includeMetaProperties) {
-            final Graph graph = TraversalHelper.getRootTraversal(traversal).getGraph().orElseThrow(
+            final Graph graph = traversal.getGraph().orElseThrow(
                     () -> new IllegalStateException("PartitionStrategy does not work with anonymous Traversals when includeMetaProperties is enabled"));
             final Graph.Features.VertexFeatures vf = graph.features().vertex();
             final boolean supportsMetaProperties = vf.supportsMetaProperties();

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/DefaultTraversal.java

@@ -149,7 +149,16 @@ public void applyStrategies() throws IllegalStateException {
             final Iterator<TraversalStrategy<?>> strategyIterator = this.strategies.iterator();
             while (strategyIterator.hasNext()) {
                 final TraversalStrategy<?> strategy = strategyIterator.next();
-                TraversalHelper.applyTraversalRecursively(strategy::apply, this);
+                TraversalHelper.applyTraversalRecursively(t -> {
+                    strategy.apply(t);
+
+                    // after the strategy is applied, it may have modified the traversal where a new traversal object
+                    // was added. if the strategy didn't set the Graph object it could leave that new traversal in a
+                    // state where another strategy might fail if that dependency is not satisfied
+                    TraversalHelper.applyTraversalRecursively(i -> {
+                        if (hasGraph) i.setGraph(this.graph);
+                    }, this);
+                }, this);
             }
 
             // don't need to re-apply strategies to "this" - leads to endless recursion in GraphComputer.

tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraphTest.java

@@ -28,8 +28,13 @@
 import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy;
 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.lambda.AbstractLambdaTraversal;
+import org.apache.tinkerpop.gremlin.process.traversal.strategy.AbstractTraversalStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.strategy.optimization.CountStrategy;
 import org.apache.tinkerpop.gremlin.process.traversal.strategy.optimization.IdentityRemovalStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.strategy.verification.ComputerVerificationStrategy;
 import org.apache.tinkerpop.gremlin.process.traversal.strategy.verification.ReservedKeysVerificationStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.strategy.verification.VerificationException;
 import org.apache.tinkerpop.gremlin.process.traversal.util.Metrics;
 import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper;
 import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalMetrics;
@@ -51,6 +56,7 @@
 import org.apache.tinkerpop.gremlin.structure.io.gryo.GryoMapper;
 import org.apache.tinkerpop.gremlin.structure.io.gryo.GryoVersion;
 import org.apache.tinkerpop.gremlin.structure.io.gryo.GryoWriter;
+import org.apache.tinkerpop.gremlin.structure.util.empty.EmptyGraph;
 import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
 import org.apache.tinkerpop.shaded.jackson.databind.ObjectMapper;
 import org.apache.tinkerpop.shaded.kryo.ClassResolver;
@@ -70,6 +76,7 @@
 import java.io.InputStream;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
@@ -879,6 +886,28 @@ public void apply(final Traversal.Admin<?, ?> traversal) {
         assertEquals(3, traversal().withEmbedded(graph).V().hasLabel("person").count().next().intValue());
     }
 
+    /**
+     * Basically just trying to validate through {@link CountStrategy} that a child traversal constructed there gets
+     * its {@link Graph} instance set. By using {@link AssertGraphStrategy} an exception can get triggered in betweeen
+     * strategy applications to validate that the {@code Graph} is assigned.
+     */
+    @Test
+    public void shouldSetGraphBetweenStrategyApplicationsForNewChildTraversalsConstructedByStrategies() throws Exception {
+        final GraphTraversalSource g = TinkerGraph.open().traversal();
+        g.addV("node").property(T.id, 1).as("1")
+                .addV("node").property(T.id, 2).as("2")
+                .addV("node").property(T.id, 3).as("3")
+                .addV("node").property(T.id, 4).as("4")
+                .addE("child").from("1").to("2")
+                .addE("child").from("2").to("3")
+                .addE("child").from("4").to("3").iterate();
+
+        // this just needs to not throw an exception for it to pass
+        g.withStrategies(AssertGraphStrategy.instance()).V(3).repeat(__.inE("child").outV().simplePath())
+                .until(__.or(__.inE().count().is(0), __.loops().is(P.eq(2))))
+                .path().count().next();
+    }
+
     /**
      * Coerces a {@code Color} to a {@link TinkerGraph} during serialization.  Demonstrates how custom serializers
      * can be developed that can coerce one value to another during serialization.
@@ -990,4 +1019,31 @@ public boolean requiresVersion(final Object version) {
             return false;
         }
     }
+
+    /**
+     * Validates that a {@link Graph} is assigned to each {@link Traversal} if it is expected.
+     */
+    public static class AssertGraphStrategy extends AbstractTraversalStrategy<TraversalStrategy.VerificationStrategy> implements TraversalStrategy.VerificationStrategy {
+
+        public static final AssertGraphStrategy INSTANCE = new AssertGraphStrategy();
+
+        private AssertGraphStrategy() {}
+
+        @Override
+        public void apply(final Traversal.Admin<?, ?> traversal) {
+            if (!(traversal instanceof AbstractLambdaTraversal) && (!traversal.getGraph().isPresent()
+                    || traversal.getGraph().get().equals(EmptyGraph.instance())))
+                throw new VerificationException("Graph object not set on Traversal", traversal);
+        }
+
+        @Override
+        public Set<Class<? extends VerificationStrategy>> applyPost() {
+            return Collections.singleton(ComputerVerificationStrategy.class);
+        }
+
+        public static AssertGraphStrategy instance() {
+            return INSTANCE;
+        }
+
+    }
 }