You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by xi...@apache.org on 2024/09/05 17:13:49 UTC
(tinkerpop) branch 3.7-dev updated: [TINKERPOP-3110] Fix Bytecode build logic for strategies (#2757)
This is an automated email from the ASF dual-hosted git repository.
xiazcy pushed a commit to branch 3.7-dev
in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
The following commit(s) were added to refs/heads/3.7-dev by this push:
new 69c440fccf [TINKERPOP-3110] Fix Bytecode build logic for strategies (#2757)
69c440fccf is described below
commit 69c440fccf909779cf2a4cac4b5d647f46ad21a2
Author: saikiranboga <sk...@gmail.com>
AuthorDate: Thu Sep 5 22:43:44 2024 +0530
[TINKERPOP-3110] Fix Bytecode build logic for strategies (#2757)
Fixed bug in Bytecode build logic where duplicate strategies were added instead of replacing the existing ones
---
CHANGELOG.asciidoc | 1 +
.../gremlin/process/traversal/Bytecode.java | 19 ++++++-
.../process/traversal/util/BytecodeHelper.java | 21 ++++++++
.../gremlin/process/traversal/BytecodeTest.java | 63 ++++++++++++++++++++++
.../process/traversal/util/BytecodeHelperTest.java | 20 +++----
5 files changed, 114 insertions(+), 10 deletions(-)
diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc
index f00204b921..822a3f442c 100644
--- a/CHANGELOG.asciidoc
+++ b/CHANGELOG.asciidoc
@@ -39,6 +39,7 @@ This release also includes changes from <<release-3-6-8, 3.6.8>>.
* Gremlin Server docker containers shutdown gracefully when receiving a SIGTERM.
* Added DefaultIdManager.STRING for proper string id creation/handling.
* Allowed specification of an `Operator` as a reducer in `withSideEffect` when parsing with the grammar.
+* Fixed bug in Bytecode build logic where duplicate strategies were added instead of replacing the existing ones.
[[release-3-7-2]]
=== TinkerPop 3.7.2 (April 8, 2024)
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Bytecode.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Bytecode.java
index 5b88cea5fc..f9c627290a 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Bytecode.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Bytecode.java
@@ -20,6 +20,7 @@
package org.apache.tinkerpop.gremlin.process.traversal;
import org.apache.tinkerpop.gremlin.process.traversal.strategy.TraversalStrategyProxy;
+import org.apache.tinkerpop.gremlin.process.traversal.util.BytecodeHelper;
import org.apache.tinkerpop.gremlin.structure.util.StringFactory;
import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
@@ -77,10 +78,26 @@ public class Bytecode implements Cloneable, Serializable {
((TraversalStrategyProxy) arguments[i]).getStrategyClass() :
(Class) arguments[i];
}
+ // Remove them first to avoid duplicating information, we will add them below
+ BytecodeHelper.removeStrategies(this, TraversalSource.Symbols.withoutStrategies, classes);
this.sourceInstructions.add(new Instruction(sourceName, classes));
}
- } else
+ } else if (TraversalSource.Symbols.withStrategies.equals(sourceName)) {
+ final Class<TraversalStrategy>[] classes = new Class[arguments.length];
+ for (int i = 0; i < arguments.length; i++) {
+ if (!(arguments[i] instanceof TraversalStrategy)) {
+ throw new IllegalArgumentException("The argument " + arguments[i] + " is not a TraversalStrategy");
+ }
+
+ classes[i] = arguments[i] instanceof TraversalStrategyProxy ?
+ ((TraversalStrategyProxy) arguments[i]).getStrategyClass() :
+ (Class) arguments[i].getClass();
+ }
+ BytecodeHelper.removeStrategies(this, TraversalSource.Symbols.withStrategies, classes);
+ this.sourceInstructions.add(new Instruction(sourceName, flattenArguments(arguments)));
+ } else {
this.sourceInstructions.add(new Instruction(sourceName, flattenArguments(arguments)));
+ }
Bindings.clear();
}
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/BytecodeHelper.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/BytecodeHelper.java
index 7d93c7fbbc..9a508be6d5 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/BytecodeHelper.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/BytecodeHelper.java
@@ -164,6 +164,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect.SubgraphSt
import org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect.TraversalSideEffectStep;
import org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect.TreeSideEffectStep;
import org.apache.tinkerpop.gremlin.process.traversal.step.util.ProfileStep;
+import org.apache.tinkerpop.gremlin.process.traversal.strategy.TraversalStrategyProxy;
import org.apache.tinkerpop.gremlin.structure.util.detached.DetachedFactory;
import org.apache.tinkerpop.gremlin.util.function.Lambda;
import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
@@ -350,6 +351,26 @@ public final class BytecodeHelper {
os -> (A) os.getArguments()[0]);
}
+ public static boolean removeStrategies(final Bytecode bytecode, final String operator, final Class<TraversalStrategy>[] clazzes) {
+ return bytecode.getSourceInstructions().removeIf(
+ s -> {
+ if (!operator.equals(s.getOperator()) || clazzes.length != s.getArguments().length) {
+ return false;
+ }
+ for (int i = 0; i < clazzes.length; i++) {
+ final Class<?> sClass =
+ s.getArguments()[i] instanceof TraversalStrategyProxy ?
+ ((TraversalStrategyProxy) s.getArguments()[i]).getStrategyClass() :
+ ((s.getArguments()[i] instanceof Class) ? (Class<?>) s.getArguments()[i] : s.getArguments()[i].getClass());
+ if (!(clazzes[i].isAssignableFrom(sClass))) {
+ return false;
+ }
+ }
+ return true;
+ }
+ );
+ }
+
public static Bytecode filterInstructions(final Bytecode bytecode, final Predicate<Bytecode.Instruction> predicate) {
final Bytecode clone = new Bytecode();
for (final Bytecode.Instruction instruction : bytecode.getSourceInstructions()) {
diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/BytecodeTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/BytecodeTest.java
index b653a450e3..bc00a238c5 100644
--- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/BytecodeTest.java
+++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/BytecodeTest.java
@@ -19,6 +19,7 @@
package org.apache.tinkerpop.gremlin.process.traversal;
+import org.apache.commons.configuration2.MapConfiguration;
import org.apache.tinkerpop.gremlin.process.computer.Computer;
import org.apache.tinkerpop.gremlin.process.computer.traversal.strategy.decoration.VertexProgramStrategy;
import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversal;
@@ -31,10 +32,13 @@ import org.apache.tinkerpop.gremlin.structure.util.empty.EmptyGraph;
import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
import org.junit.Test;
+import java.util.HashMap;
import java.util.List;
import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertTrue;
/**
* @author Marko A. Rodriguez (http://markorodriguez.com)
@@ -145,6 +149,65 @@ public class BytecodeTest {
assertEquals("created", binding.value());
}
+
+ @Test
+ public void testOnlySingleInstructionPerStrategyOnBytecode() throws Exception {
+ GraphTraversalSource g = EmptyGraph.instance().traversal();
+ assertFalse(g.getStrategies().getStrategy(SubgraphStrategy.class).isPresent());
+ g = g.withStrategies(SubgraphStrategy.create(new MapConfiguration(new HashMap<String, Object>() {{
+ put("vertices", __.hasLabel("person"));
+ put("vertexProperties", __.limit(0));
+ put("edges", __.hasLabel("knows"));
+ }})));
+ assertTrue(g.getStrategies().getStrategy(SubgraphStrategy.class).isPresent());
+ Bytecode bytecode = g.getBytecode();
+ assertEquals(1, bytecode.getSourceInstructions().get(0).getArguments().length);
+ assertEquals(SubgraphStrategy.class, bytecode.getSourceInstructions().get(0).getArguments()[0].getClass());
+
+ // Test applying another withStrategies with same strategy does not add a new entry.
+ g = g.withStrategies(SubgraphStrategy.create(new MapConfiguration(new HashMap<String, Object>() {{
+ put("vertices", __.hasLabel("person"));
+ put("vertexProperties", __.limit(0));
+ put("edges", __.hasLabel("knows"));
+ }})));
+ assertEquals(1, bytecode.getSourceInstructions().get(0).getArguments().length);
+ assertEquals(SubgraphStrategy.class, bytecode.getSourceInstructions().get(0).getArguments()[0].getClass());
+
+ g = g.withoutStrategies(SubgraphStrategy.class);
+ assertFalse(g.getStrategies().getStrategy(SubgraphStrategy.class).isPresent());
+ bytecode = g.getBytecode();
+ assertEquals(2, bytecode.getSourceInstructions().size());
+ // 1 withStrategies instruction
+ assertEquals(1, bytecode.getSourceInstructions().get(0).getArguments().length);
+ // 1 withoutStrategies instruction
+ assertEquals(1, bytecode.getSourceInstructions().get(1).getArguments().length);
+
+ // Reset everything
+ g = EmptyGraph.instance().traversal();
+ assertFalse(g.getStrategies().getStrategy(ReadOnlyStrategy.class).isPresent());
+ g = g.withStrategies(ReadOnlyStrategy.instance())
+ .withStrategies(ReadOnlyStrategy.instance())
+ .withStrategies(SubgraphStrategy.build().edges(__.hasLabel("knows")).create())
+ .withStrategies(SubgraphStrategy.build().edges(__.hasLabel("knows")).create());
+ assertTrue(g.getStrategies().getStrategy(ReadOnlyStrategy.class).isPresent());
+ assertTrue(g.getStrategies().getStrategy(SubgraphStrategy.class).isPresent());
+ bytecode = g.getBytecode();
+ // Two instructions for two unique withStrategies() steps
+ assertEquals(2, bytecode.getSourceInstructions().size());
+ assertEquals(1, bytecode.getSourceInstructions().get(0).getArguments().length);
+ assertEquals(1, bytecode.getSourceInstructions().get(1).getArguments().length);
+ assertEquals(2, g.getStrategies().toList().size());
+
+ g = g.withoutStrategies(ReadOnlyStrategy.class).withoutStrategies(ReadOnlyStrategy.class);
+ assertFalse(g.getStrategies().getStrategy(ReadOnlyStrategy.class).isPresent());
+ assertEquals(1, g.getStrategies().toList().size());
+ bytecode = g.getBytecode();
+ // 2 withStrategies instructions above, 1 withoutStrategies instruction
+ assertEquals(3, bytecode.getSourceInstructions().size());
+ // 1 withoutStrategies instruction, no duplicates
+ assertEquals(1, bytecode.getSourceInstructions().get(2).getArguments().length);
+ }
+
@Test
public void shouldConvertStrategies() {
final GraphTraversalSource g = EmptyGraph.instance().traversal();
diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/util/BytecodeHelperTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/util/BytecodeHelperTest.java
index d0ca789be8..75aa1fb3da 100644
--- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/util/BytecodeHelperTest.java
+++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/util/BytecodeHelperTest.java
@@ -45,6 +45,7 @@ import static org.apache.tinkerpop.gremlin.process.traversal.GraphOp.TX_ROLLBACK
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.core.Is.is;
import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
/**
@@ -71,18 +72,19 @@ public class BytecodeHelperTest {
public void shouldFindStrategy() {
final Iterator<OptionsStrategy> itty = BytecodeHelper.findStrategies(g.with("x").with("y", 100).V().asAdmin().getBytecode(), OptionsStrategy.class);
int counter = 0;
- while(itty.hasNext()) {
- final OptionsStrategy strategy = itty.next();
- if (strategy.getOptions().keySet().contains("x")) {
- assertThat(strategy.getOptions().get("x"), is(true));
- counter++;
- } else if (strategy.getOptions().keySet().contains("y")) {
- assertEquals(100, strategy.getOptions().get("y"));
- counter++;
- }
+
+ final OptionsStrategy strategy = itty.next();
+ if (strategy.getOptions().keySet().contains("x")) {
+ assertThat(strategy.getOptions().get("x"), is(true));
+ counter++;
}
+ if (strategy.getOptions().keySet().contains("y")) {
+ assertEquals(100, strategy.getOptions().get("y"));
+ counter++;
+ }
assertEquals(2, counter);
+ assertFalse(itty.hasNext()); // There should only be a single source instruction per strategy.
}
@Test