From f9bf0444e4bfe09dcea9b8dc05870ddaec4b6180 Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Tue, 25 Oct 2016 13:02:29 -0600 Subject: [PATCH 1/2] TraversalStrategy.GlobalCache.getStrategies() is now the point where static{} code blocks are loaded for Graph and GraphComputer classes. Added a test case to TraversalStrategiesTset that demonstrates that the GlobalCache can be manipulated without fear of static{} re-registering strategies. This is a much much safer model and, moreover, proved correct via testing. --- CHANGELOG.asciidoc | 1 + .../decoration/VertexProgramStrategy.java | 5 - .../traversal/TraversalStrategies.java | 11 +- .../process/TraversalStrategiesTest.java | 194 +++++++++++++++++- 4 files changed, 200 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index a281c3813f9..1c7441b5060 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -26,6 +26,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima TinkerPop 3.2.4 (Release Date: NOT OFFICIALLY RELEASED YET) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +* Added a class loader to `TraversalStrategies.GlobalCache` which guarantees strategies are registered prior to `GlobalCache.getStrategies()`. * Fixed a severe bug where `GraphComputer` strategies are not being loaded until the second use of the traversal source. [[release-3-2-3]] diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/traversal/strategy/decoration/VertexProgramStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/traversal/strategy/decoration/VertexProgramStrategy.java index 0eeae3cf64a..89e40cb97c2 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/traversal/strategy/decoration/VertexProgramStrategy.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/traversal/strategy/decoration/VertexProgramStrategy.java @@ -171,11 +171,6 @@ public void addGraphComputerStrategies(final TraversalSource traversalSource) { } } else graphComputerClass = this.computer.getGraphComputerClass(); - try { - Class.forName(graphComputerClass.getCanonicalName()); - } catch (final ClassNotFoundException e) { - throw new IllegalStateException(e.getMessage(), e); - } final List> graphComputerStrategies = TraversalStrategies.GlobalCache.getStrategies(graphComputerClass).toList(); traversalSource.getStrategies().addStrategies(graphComputerStrategies.toArray(new TraversalStrategy[graphComputerStrategies.size()])); } 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 c271a379e7e..015df704d36 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 @@ -225,7 +225,6 @@ public static final class GlobalCache { OrderLimitStrategy.instance(), PathProcessorStrategy.instance(), ComputerVerificationStrategy.instance()); - GRAPH_COMPUTER_CACHE.put(GraphComputer.class, graphComputerStrategies); } @@ -239,6 +238,16 @@ else if (GraphComputer.class.isAssignableFrom(graphOrGraphComputerClass)) } public static TraversalStrategies getStrategies(final Class graphOrGraphComputerClass) { + try { + // be sure to load the class so that its static{} traversal strategy registration component is loaded. + // this is more important for GraphComputer classes as they are typically not instantiated prior to strategy usage like Graph classes. + final String graphComputerClassName = null != graphOrGraphComputerClass.getDeclaringClass() ? + graphOrGraphComputerClass.getCanonicalName().replace("." + graphOrGraphComputerClass.getSimpleName(), "$" + graphOrGraphComputerClass.getSimpleName()) : + graphOrGraphComputerClass.getCanonicalName(); + Class.forName(graphComputerClassName); + } catch (final ClassNotFoundException e) { + throw new IllegalStateException(e.getMessage(), e); + } if (Graph.class.isAssignableFrom(graphOrGraphComputerClass)) { final TraversalStrategies traversalStrategies = GRAPH_CACHE.get(graphOrGraphComputerClass); return null == traversalStrategies ? GRAPH_CACHE.get(Graph.class) : traversalStrategies; diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/TraversalStrategiesTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/TraversalStrategiesTest.java index f316836c74d..3d320db3354 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/TraversalStrategiesTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/TraversalStrategiesTest.java @@ -18,26 +18,211 @@ */ package org.apache.tinkerpop.gremlin.process; +import org.apache.commons.configuration.BaseConfiguration; +import org.apache.commons.configuration.Configuration; +import org.apache.tinkerpop.gremlin.process.computer.ComputerResult; +import org.apache.tinkerpop.gremlin.process.computer.GraphComputer; +import org.apache.tinkerpop.gremlin.process.computer.MapReduce; +import org.apache.tinkerpop.gremlin.process.computer.VertexProgram; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategies; import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.strategy.AbstractTraversalStrategy; +import org.apache.tinkerpop.gremlin.structure.Edge; +import org.apache.tinkerpop.gremlin.structure.Graph; +import org.apache.tinkerpop.gremlin.structure.Transaction; +import org.apache.tinkerpop.gremlin.structure.Vertex; import org.junit.Test; -import java.util.Arrays; import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.Iterator; import java.util.List; import java.util.Set; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.Future; import java.util.stream.Collectors; import java.util.stream.Stream; -import static org.junit.Assert.*; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; /** * @author Matthias Broecheler (me@matthiasb.com) */ public class TraversalStrategiesTest { + @Test + public void shouldAllowUserManipulationOfGlobalCache() throws Exception { + /////////// + // GRAPH // + /////////// + TestGraph graph = new TestGraph(); + TraversalStrategies strategies = graph.traversal().getStrategies(); + assertFalse(TraversalStrategies.GlobalCache.getStrategies(Graph.class).toList().isEmpty()); + for (final TraversalStrategy strategy : TraversalStrategies.GlobalCache.getStrategies(Graph.class).toList()) { + assertTrue(strategies.getStrategy(strategy.getClass()).isPresent()); + } + for (final TraversalStrategy strategy : TraversalStrategies.GlobalCache.getStrategies(TestGraphComputer.class).toList()) { + assertFalse(strategies.getStrategy(strategy.getClass()).isPresent()); + } + assertTrue(strategies.getStrategy(StrategyA.class).isPresent()); + assertTrue(strategies.getStrategy(StrategyB.class).isPresent()); + assertFalse(strategies.getStrategy(StrategyC.class).isPresent()); + assertFalse(strategies.getStrategy(StrategyD.class).isPresent()); + strategies.addStrategies(new StrategyD()); + strategies.removeStrategies(StrategyA.class); + assertFalse(strategies.getStrategy(StrategyA.class).isPresent()); + assertTrue(strategies.getStrategy(StrategyD.class).isPresent()); + /// + graph = new TestGraph(); + strategies = graph.traversal().getStrategies(); + for (final TraversalStrategy strategy : TraversalStrategies.GlobalCache.getStrategies(Graph.class).toList()) { + assertTrue(strategies.getStrategy(strategy.getClass()).isPresent()); + } + for (final TraversalStrategy strategy : TraversalStrategies.GlobalCache.getStrategies(TestGraphComputer.class).toList()) { + assertFalse(strategies.getStrategy(strategy.getClass()).isPresent()); + } + assertFalse(strategies.getStrategy(StrategyA.class).isPresent()); + assertTrue(strategies.getStrategy(StrategyB.class).isPresent()); + assertFalse(strategies.getStrategy(StrategyC.class).isPresent()); + assertTrue(strategies.getStrategy(StrategyD.class).isPresent()); + ////////////////////// + /// GRAPH COMPUTER /// + ////////////////////// + strategies = TraversalStrategies.GlobalCache.getStrategies(TestGraphComputer.class); + assertFalse(TraversalStrategies.GlobalCache.getStrategies(GraphComputer.class).toList().isEmpty()); + for (final TraversalStrategy strategy : TraversalStrategies.GlobalCache.getStrategies(GraphComputer.class).toList()) { + assertTrue(strategies.getStrategy(strategy.getClass()).isPresent()); + } + for (final TraversalStrategy strategy : TraversalStrategies.GlobalCache.getStrategies(TestGraph.class).toList()) { + assertFalse(strategies.getStrategy(strategy.getClass()).isPresent()); + } + assertFalse(strategies.getStrategy(StrategyA.class).isPresent()); + assertFalse(strategies.getStrategy(StrategyB.class).isPresent()); + assertTrue(strategies.getStrategy(StrategyC.class).isPresent()); + strategies.addStrategies(new StrategyE()); + strategies.removeStrategies(StrategyC.class); + // + strategies = TraversalStrategies.GlobalCache.getStrategies(TestGraphComputer.class); + assertFalse(TraversalStrategies.GlobalCache.getStrategies(GraphComputer.class).toList().isEmpty()); + for (final TraversalStrategy strategy : TraversalStrategies.GlobalCache.getStrategies(GraphComputer.class).toList()) { + assertTrue(strategies.getStrategy(strategy.getClass()).isPresent()); + } + for (final TraversalStrategy strategy : TraversalStrategies.GlobalCache.getStrategies(TestGraph.class).toList()) { + assertFalse(strategies.getStrategy(strategy.getClass()).isPresent()); + } + assertFalse(strategies.getStrategy(StrategyA.class).isPresent()); + assertFalse(strategies.getStrategy(StrategyB.class).isPresent()); + assertFalse(strategies.getStrategy(StrategyC.class).isPresent()); + assertFalse(strategies.getStrategy(StrategyD.class).isPresent()); + assertTrue(strategies.getStrategy(StrategyE.class).isPresent()); + } + + public static class TestGraphComputer implements GraphComputer { + + static { + TraversalStrategies.GlobalCache.registerStrategies(TestGraphComputer.class, + TraversalStrategies.GlobalCache.getStrategies(GraphComputer.class).clone().addStrategies(new StrategyC())); + } + + @Override + public GraphComputer result(ResultGraph resultGraph) { + return this; + } + + @Override + public GraphComputer persist(Persist persist) { + return this; + } + + @Override + public GraphComputer program(VertexProgram vertexProgram) { + return this; + } + + @Override + public GraphComputer mapReduce(MapReduce mapReduce) { + return this; + } + + @Override + public GraphComputer workers(int workers) { + return this; + } + + @Override + public GraphComputer vertices(Traversal vertexFilter) throws IllegalArgumentException { + return this; + } + + @Override + public GraphComputer edges(Traversal edgeFilter) throws IllegalArgumentException { + return this; + } + + @Override + public Future submit() { + return new CompletableFuture<>(); + } + } + + public static class TestGraph implements Graph { + + static { + TraversalStrategies.GlobalCache.registerStrategies(TestGraph.class, + TraversalStrategies.GlobalCache.getStrategies(Graph.class).clone().addStrategies(new StrategyA(), new StrategyB())); + } + + @Override + public Vertex addVertex(Object... keyValues) { + return null; + } + + @Override + public C compute(Class graphComputerClass) throws IllegalArgumentException { + return (C) new TestGraphComputer(); + } + + @Override + public GraphComputer compute() throws IllegalArgumentException { + return new TestGraphComputer(); + } + + @Override + public Iterator vertices(Object... vertexIds) { + return Collections.emptyIterator(); + } + + @Override + public Iterator edges(Object... edgeIds) { + return Collections.emptyIterator(); + } + + @Override + public Transaction tx() { + return null; + } + + @Override + public void close() throws Exception { + + } + + @Override + public Variables variables() { + return null; + } + + @Override + public Configuration configuration() { + return new BaseConfiguration(); + } + } + /** * Tests that {@link org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategies#sortStrategies(java.util.List)} * works as advertised. This class defines a bunch of dummy strategies which define an order. It is verified @@ -113,7 +298,7 @@ public void testTraversalStrategySorting() { assertTrue(s.indexOf(a) < s.indexOf(b)); // sort and then add more - s = new ArrayList<>((List)Arrays.asList(b,a,c)); + s = new ArrayList<>((List) Arrays.asList(b, a, c)); s = TraversalStrategies.sortStrategies(s); assertEquals(3, s.size()); assertEquals(a, s.get(0)); @@ -220,7 +405,6 @@ public static class StrategyO extends DummyStrategy { } - private static class DummyStrategy extends AbstractTraversalStrategy { @Override @@ -262,7 +446,7 @@ public void testTraversalStrategySortingWithCategories() { assertEquals(e, s.get(3)); //full reverse sorting - s = Arrays.asList(k,e,d,c,b,a); + s = Arrays.asList(k, e, d, c, b, a); s = TraversalStrategies.sortStrategies(s); assertEquals(6, s.size()); assertEquals(a, s.get(0)); From b0bedf6b441edddbfd2e005641fee0a044b3b552 Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Tue, 25 Oct 2016 13:07:01 -0600 Subject: [PATCH 2/2] added authorship to TraversalStrategies java file. --- .../tinkerpop/gremlin/process/TraversalStrategiesTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/TraversalStrategiesTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/TraversalStrategiesTest.java index 3d320db3354..bb58a069dce 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/TraversalStrategiesTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/TraversalStrategiesTest.java @@ -52,6 +52,7 @@ /** * @author Matthias Broecheler (me@matthiasb.com) + * @author Marko A. Rodriguez (marko@markorodriguez.com) */ public class TraversalStrategiesTest {