Repository: tinkerpop
Updated Branches:
  refs/heads/tp31 e3c5d8ed1 -> 84f2d63d7


Re-assigned step labels as appropriate in SubgraphStrategy.

Step labels were not being re-written to replaced steps or to the subgraph 
filter steps and so traversals using .as('a') and the like were failing.


Project: http://git-wip-us.apache.org/repos/asf/tinkerpop/repo
Commit: http://git-wip-us.apache.org/repos/asf/tinkerpop/commit/84f2d63d
Tree: http://git-wip-us.apache.org/repos/asf/tinkerpop/tree/84f2d63d
Diff: http://git-wip-us.apache.org/repos/asf/tinkerpop/diff/84f2d63d

Branch: refs/heads/tp31
Commit: 84f2d63d7b758c21bb22a6d771d57479fce3d06b
Parents: e3c5d8e
Author: Stephen Mallette <sp...@genoprime.com>
Authored: Wed Jun 15 14:15:53 2016 -0400
Committer: Stephen Mallette <sp...@genoprime.com>
Committed: Wed Jun 15 14:29:43 2016 -0400

----------------------------------------------------------------------
 CHANGELOG.asciidoc                              |  1 +
 .../strategy/decoration/SubgraphStrategy.java   | 40 ++++++++++++++------
 .../decoration/SubgraphStrategyProcessTest.java | 22 +++++++++++
 3 files changed, 51 insertions(+), 12 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/84f2d63d/CHANGELOG.asciidoc
----------------------------------------------------------------------
diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc
index 7b5206b..a359eca 100644
--- a/CHANGELOG.asciidoc
+++ b/CHANGELOG.asciidoc
@@ -27,6 +27,7 @@ TinkerPop 3.1.3 (NOT OFFICIALLY RELEASED YET)
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
 * Avoid hamcrest conflict by using mockito-core instead of mockito-all 
dependency in `gremlin-test`.
+* Fixed bug in `SubgraphStrategy` where step labels were not being propogated 
properly to new steps injected by the strategy.
 * Defaulted to `Edge.DEFAULT` if no edge label was supplied in GraphML.
 * Fixed bug in `IoGraphTest` causing IllegalArgumentException: URI is not 
hierarchical error for external graph implementations.
 * Fixed a bug where timeout functions provided to the `GremlinExecutor` were 
not executing in the same thread as the script evaluation.

http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/84f2d63d/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategy.java
----------------------------------------------------------------------
diff --git 
a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategy.java
 
b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategy.java
index e2488fb..d328168 100644
--- 
a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategy.java
+++ 
b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategy.java
@@ -34,6 +34,7 @@ import 
org.apache.tinkerpop.gremlin.process.traversal.strategy.AbstractTraversal
 import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper;
 import org.apache.tinkerpop.gremlin.structure.Direction;
 import org.apache.tinkerpop.gremlin.structure.Edge;
+import org.apache.tinkerpop.gremlin.structure.Element;
 import org.apache.tinkerpop.gremlin.structure.Vertex;
 
 import java.util.ArrayList;
@@ -86,7 +87,7 @@ public final class SubgraphStrategy extends 
AbstractTraversalStrategy<TraversalS
             
vertexStepsToInsertFilterAfter.addAll(TraversalHelper.getStepsOfAssignableClass(AddVertexStartStep.class,
 traversal));
             
vertexStepsToInsertFilterAfter.addAll(graphSteps.stream().filter(GraphStep::returnsVertex).collect(Collectors.toList()));
 
-            vertexStepsToInsertFilterAfter.forEach(s -> 
TraversalHelper.insertAfterStep(new TraversalFilterStep<>(traversal, 
vertexCriterion.asAdmin().clone()), s, traversal));
+            applyCriterion(vertexStepsToInsertFilterAfter, traversal, 
vertexCriterion.asAdmin());
         }
 
         if (edgeCriterion != null) {
@@ -95,7 +96,7 @@ public final class SubgraphStrategy extends 
AbstractTraversalStrategy<TraversalS
             
edgeStepsToInsertFilterAfter.addAll(graphSteps.stream().filter(GraphStep::returnsEdge).collect(Collectors.toList()));
             
edgeStepsToInsertFilterAfter.addAll(vertexSteps.stream().filter(VertexStep::returnsEdge).collect(Collectors.toList()));
 
-            edgeStepsToInsertFilterAfter.forEach(s -> 
TraversalHelper.insertAfterStep(new TraversalFilterStep<>(traversal, 
edgeCriterion.asAdmin().clone()), s, traversal));
+            applyCriterion(edgeStepsToInsertFilterAfter, traversal, 
edgeCriterion.asAdmin());
         }
 
         // explode g.V().out() to g.V().outE().inV() only if there is an edge 
predicate otherwise
@@ -103,19 +104,19 @@ public final class SubgraphStrategy extends 
AbstractTraversalStrategy<TraversalS
             if (null == edgeCriterion)
                 TraversalHelper.insertAfterStep(new 
TraversalFilterStep<>(traversal, vertexCriterion.asAdmin().clone()), s, 
traversal);
             else {
-                final VertexStep replacementVertexStep = new 
VertexStep(traversal, Edge.class, s.getDirection(), s.getEdgeLabels());
-                Step intermediateFilterStep = null;
-                if (s.getDirection() == Direction.BOTH)
-                    intermediateFilterStep = new 
EdgeOtherVertexStep(traversal);
-                else
-                    intermediateFilterStep = new EdgeVertexStep(traversal, 
s.getDirection().opposite());
+                final VertexStep someEStep = new VertexStep(traversal, 
Edge.class, s.getDirection(), s.getEdgeLabels());
+                final Step someVStep = (s.getDirection() == Direction.BOTH) ?
+                        new EdgeOtherVertexStep(traversal) : new 
EdgeVertexStep(traversal, s.getDirection().opposite());
 
-                TraversalHelper.replaceStep(s, replacementVertexStep, 
traversal);
-                TraversalHelper.insertAfterStep(intermediateFilterStep, 
replacementVertexStep, traversal);
-                TraversalHelper.insertAfterStep(new 
TraversalFilterStep<>(traversal, edgeCriterion.asAdmin().clone()), 
replacementVertexStep, traversal);
+                // if s was labelled then propagate those labels to the new 
step that will return the vertex
+                transferLabels(s, someVStep);
+
+                TraversalHelper.replaceStep(s, someEStep, traversal);
+                TraversalHelper.insertAfterStep(someVStep, someEStep, 
traversal);
+                TraversalHelper.insertAfterStep(new 
TraversalFilterStep<>(traversal, edgeCriterion.asAdmin().clone()), someEStep, 
traversal);
 
                 if (vertexCriterion != null)
-                    TraversalHelper.insertAfterStep(new 
TraversalFilterStep<>(traversal, vertexCriterion.asAdmin().clone()), 
intermediateFilterStep, traversal);
+                    TraversalHelper.insertAfterStep(new 
TraversalFilterStep<>(traversal, vertexCriterion.asAdmin().clone()), someVStep, 
traversal);
             }
         });
     }
@@ -132,6 +133,21 @@ public final class SubgraphStrategy extends 
AbstractTraversalStrategy<TraversalS
         return new Builder();
     }
 
+    private void applyCriterion(final List<Step> stepsToApplyCriterionAfter, 
final Traversal.Admin traversal,
+                                final Traversal.Admin<? extends Element, ?> 
criterion) {
+        stepsToApplyCriterionAfter.forEach(s -> {
+            // re-assign the step label to the criterion because the label 
should apply seamlessly after the filter
+            final Step filter = new TraversalFilterStep<>(traversal, 
criterion.clone());
+            transferLabels(s, filter);
+            TraversalHelper.insertAfterStep(filter, s, traversal);
+        });
+    }
+
+    private static void transferLabels(final Step from, final Step to) {
+        from.getLabels().forEach(label -> to.addLabel((String) label));
+        to.getLabels().forEach(label -> from.removeLabel((String) label));
+    }
+
     public final static class Builder {
 
         private Traversal<Vertex, ?> vertexCriterion = null;

http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/84f2d63d/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategyProcessTest.java
----------------------------------------------------------------------
diff --git 
a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategyProcessTest.java
 
b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategyProcessTest.java
index 11f96d0..81ea296 100644
--- 
a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategyProcessTest.java
+++ 
b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategyProcessTest.java
@@ -32,11 +32,16 @@ import org.apache.tinkerpop.gremlin.structure.Vertex;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 
+import java.util.List;
 import java.util.NoSuchElementException;
 
 import static org.apache.tinkerpop.gremlin.LoadGraphWith.GraphData.MODERN;
 import static 
org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.bothE;
 import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.outE;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.core.IsCollectionContaining.hasItem;
+import static org.hamcrest.core.IsCollectionContaining.hasItems;
+import static org.hamcrest.core.IsNot.not;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
@@ -302,6 +307,23 @@ public class SubgraphStrategyProcessTest extends 
AbstractGremlinProcessTest {
         }
     }
 
+    @Test
+    @LoadGraphWith(MODERN)
+    @IgnoreEngine(TraversalEngine.Type.COMPUTER)
+    public void shouldFilterVertexCriterionAndKeepLabels() throws Exception {
+        // this will exclude "peter"
+        final Traversal<Vertex, ?> vertexCriterion = __.has("name", 
P.within("ripple", "josh", "marko"));
+
+        final SubgraphStrategy strategy = 
SubgraphStrategy.build().vertexCriterion(vertexCriterion).create();
+        final GraphTraversalSource sg = create(strategy);
+
+        assertEquals(9, g.V().as("a").out().in().as("b").dedup("a", 
"b").count().next().intValue());
+        assertEquals(2, sg.V().as("a").out().in().as("b").dedup("a", 
"b").count().next().intValue());
+
+        final List<Object> list = sg.V().as("a").out().in().as("b").dedup("a", 
"b").values("name").toList();
+        assertThat(list, hasItems("marko", "josh"));
+    }
+
     @Test(expected = NoSuchElementException.class)
     @LoadGraphWith(MODERN)
     public void shouldGetExcludedVertex() throws Exception {

Reply via email to