This is an automated email from the ASF dual-hosted git repository.
Cole-Greer pushed a commit to branch 3.8-dev
in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
The following commit(s) were added to refs/heads/3.8-dev by this push:
new 5f1c592a59 TINKERPOP-3294: AddPropertyStep does not capture the old
property for EventStrategy when cardinality is not specified (#3715)
5f1c592a59 is described below
commit 5f1c592a599bb22a6f1a9da9ff3e9e2233c00103
Author: Paul Lartigau <[email protected]>
AuthorDate: Tue Oct 27 17:59:55 2026 +0100
TINKERPOP-3294: AddPropertyStep does not capture the old property for
EventStrategy when cardinality is not specified (#3715)
captureRemovedProperty() was checking the step's cardinality field, which
is null when property() is called without an explicit cardinality. The old
value passed to vertexPropertyChanged() was then always empty, even though the
property itself was written with the graph's default cardinality.
The effective cardinality (explicit one, or the graph default for the key)
was already computed in sideEffect() but not used. It is now passed to
captureRemovedProperty().
Assisted-by: Claude Code:claude-opus-5-5
---------
Co-authored-by: Paul Lartigau <[email protected]>
---
CHANGELOG.asciidoc | 2 +
.../traversal/step/sideEffect/AddPropertyStep.java | 25 ++--
.../decoration/EventStrategyProcessTest.java | 131 +++++++++++++++++++++
.../gremlin/tinkergraph/TinkerGraphProvider.java | 15 ++-
.../TinkerTransactionGraphProvider.java | 15 ++-
5 files changed, 176 insertions(+), 12 deletions(-)
diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc
index 913b838581..f568cbec14 100644
--- a/CHANGELOG.asciidoc
+++ b/CHANGELOG.asciidoc
@@ -27,6 +27,8 @@
image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima
This release also includes changes from prior 3.7.x releases.
+* Fixed `AddPropertyStep` not passing the old property to `EventStrategy`
listeners when the cardinality was not specified in `property()`.
+
[[release-3-8-2]]
=== TinkerPop 3.8.2 (Release Date: September 1, 2026)
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..e2ee1ddfaf 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
@@ -104,6 +104,10 @@ public class AddPropertyStep<S extends Element> extends
SideEffectStep<S> implem
"Property cardinality can only be set for a Vertex but the
traversal encountered %s for key: %s",
element.getClass().getSimpleName(), key));
+ final VertexProperty.Cardinality effectiveCardinality =
this.cardinality != null
+ ? this.cardinality
+ : element.graph().features().vertex().getCardinality(key);
+
final Optional<EventStrategy> optEventStrategy =
getTraversal().getStrategies().getStrategy(EventStrategy.class);
final boolean eventingIsConfigured =
EventUtil.hasAnyCallbacks(callbackRegistry)
&& optEventStrategy.isPresent();
@@ -111,17 +115,13 @@ public class AddPropertyStep<S extends Element> extends
SideEffectStep<S> implem
// find property to remove. only need to capture the removedProperty
if eventing is configured
final Property removedProperty = eventingIsConfigured ?
- captureRemovedProperty(element, key, value, es) :
+ captureRemovedProperty(element, key, value,
effectiveCardinality, es) :
VertexProperty.empty();
- final VertexProperty.Cardinality card = this.cardinality != null
- ? this.cardinality
- : element.graph().features().vertex().getCardinality(key);
-
// update property
if (element instanceof Vertex) {
- if (null != card) {
- ((Vertex) element).property(card, key, value,
vertexPropertyKeyValues);
+ if (null != effectiveCardinality) {
+ ((Vertex) element).property(effectiveCardinality, key, value,
vertexPropertyKeyValues);
} else if (vertexPropertyKeyValues.length > 0) {
((Vertex) element).property(key, value,
vertexPropertyKeyValues);
} else {
@@ -140,11 +140,12 @@ public class AddPropertyStep<S extends Element> extends
SideEffectStep<S> implem
}
private Property captureRemovedProperty(final Element element, final
String key, final Object value,
+ final VertexProperty.Cardinality
effectiveCardinality,
final EventStrategy es){
Property removedProperty = VertexProperty.empty();
if (element instanceof Vertex) {
- if (cardinality == VertexProperty.Cardinality.set) {
+ if (effectiveCardinality == VertexProperty.Cardinality.set) {
final Iterator<? extends Property> properties =
element.properties(key);
while (properties.hasNext()) {
final Property property = properties.next();
@@ -153,8 +154,12 @@ public class AddPropertyStep<S extends Element> extends
SideEffectStep<S> implem
break;
}
}
- } else if (cardinality == VertexProperty.Cardinality.single) {
- removedProperty = element.property(key);
+ } else if (effectiveCardinality ==
VertexProperty.Cardinality.single) {
+ // Only tracks the first removed property, potentially missing
events if existing cardinality was Set or List. TINKERPOP-3300
+ final Iterator<? extends Property> existing =
element.properties(key);
+ if (existing.hasNext()) {
+ removedProperty = existing.next();
+ }
}
} else {
removedProperty = element.property(key);
diff --git
a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/EventStrategyProcessTest.java
b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/EventStrategyProcessTest.java
index 0bad338732..4c4c8d434e 100644
---
a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/EventStrategyProcessTest.java
+++
b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/EventStrategyProcessTest.java
@@ -57,6 +57,7 @@ import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.core.IsInstanceOf.instanceOf;
import static org.junit.Assert.assertEquals;
+import static org.junit.Assume.assumeTrue;
/**
* @author Stephen Mallette (http://stephen.genoprime.com)
@@ -1051,6 +1052,136 @@ public class EventStrategyProcessTest extends
AbstractGremlinProcessTest {
assertThat(triggered.get(), is(true));
}
+ @Test
+ @FeatureRequirementSet(FeatureRequirementSet.Package.VERTICES_ONLY)
+ public void
shouldCaptureOldVertexPropertyWhenChangedWithDefaultCardinality() {
+ assumeTrue(graph.features().vertex().getCardinality("to-change") ==
VertexProperty.Cardinality.single);
+
+ final AtomicBoolean triggered = new AtomicBoolean(false);
+ final Vertex v = graph.addVertex();
+ v.property("to-change", "blah");
+
+ final MutationListener listener = new AbstractMutationListener() {
+ @Override
+ public void vertexPropertyChanged(final Vertex element, final
VertexProperty oldValue, final Object setValue, final Object...
vertexPropertyKeyValues) {
+ assertEquals("to-change", oldValue.key());
+ assertEquals("blah", oldValue.value());
+ assertEquals("dah", setValue);
+ triggered.set(true);
+ }
+ };
+ final EventStrategy.Builder builder =
EventStrategy.build().addListener(listener);
+
+ if (graph.features().graph().supportsTransactions())
+ builder.eventQueue(new
EventStrategy.TransactionalEventQueue(graph));
+
+ final EventStrategy eventStrategy = builder.create();
+ final GraphTraversalSource gts = create(eventStrategy);
+
+ gts.V(v).property("to-change", "dah").iterate();
+ tryCommit(graph);
+
+ assertEquals(1, IteratorUtils.count(g.V(v).properties()));
+ assertThat(triggered.get(), is(true));
+ }
+
+ @Test
+ @FeatureRequirementSet(FeatureRequirementSet.Package.VERTICES_ONLY)
+ public void
shouldCaptureOldVertexPropertyWhenChangedWithDefaultSetCardinality() {
+ assumeTrue(graph.features().vertex().getCardinality("to-change") ==
VertexProperty.Cardinality.set);
+
+ final AtomicBoolean triggered = new AtomicBoolean(false);
+ final Vertex v = graph.addVertex();
+ v.property("to-change", "blah");
+
+ final MutationListener listener = new AbstractMutationListener() {
+ @Override
+ public void vertexPropertyChanged(final Vertex element, final
VertexProperty oldValue, final Object setValue, final Object...
vertexPropertyKeyValues) {
+ assertEquals("to-change", oldValue.key());
+ assertEquals("blah", oldValue.value());
+ assertEquals("blah", setValue);
+ triggered.set(true);
+ }
+ };
+ final EventStrategy.Builder builder =
EventStrategy.build().addListener(listener);
+
+ if (graph.features().graph().supportsTransactions())
+ builder.eventQueue(new
EventStrategy.TransactionalEventQueue(graph));
+
+ final EventStrategy eventStrategy = builder.create();
+ final GraphTraversalSource gts = create(eventStrategy);
+
+ gts.V(v).property("to-change", "blah").iterate();
+ tryCommit(graph);
+
+ assertEquals(1, IteratorUtils.count(g.V(v).properties()));
+ assertThat(triggered.get(), is(true));
+ }
+
+ @Test
+ @FeatureRequirementSet(FeatureRequirementSet.Package.VERTICES_ONLY)
+ public void
shouldNotCaptureOldVertexPropertyWhenValueIsNewWithDefaultSetCardinality() {
+ assumeTrue(graph.features().vertex().getCardinality("to-change") ==
VertexProperty.Cardinality.set);
+
+ final AtomicBoolean triggered = new AtomicBoolean(false);
+ final Vertex v = graph.addVertex();
+ v.property("to-change", "blah");
+
+ final MutationListener listener = new AbstractMutationListener() {
+ @Override
+ public void vertexPropertyChanged(final Vertex element, final
VertexProperty oldValue, final Object setValue, final Object...
vertexPropertyKeyValues) {
+ assertThat(oldValue.isPresent(), is(false));
+ assertEquals("dah", setValue);
+ triggered.set(true);
+ }
+ };
+ final EventStrategy.Builder builder =
EventStrategy.build().addListener(listener);
+
+ if (graph.features().graph().supportsTransactions())
+ builder.eventQueue(new
EventStrategy.TransactionalEventQueue(graph));
+
+ final EventStrategy eventStrategy = builder.create();
+ final GraphTraversalSource gts = create(eventStrategy);
+
+ gts.V(v).property("to-change", "dah").iterate();
+ tryCommit(graph);
+
+ assertEquals(2, IteratorUtils.count(g.V(v).properties()));
+ assertThat(triggered.get(), is(true));
+ }
+
+ @Test
+ @FeatureRequirementSet(FeatureRequirementSet.Package.VERTICES_ONLY)
+ public void
shouldNotCaptureOldVertexPropertyWhenAddedWithDefaultListCardinality() {
+ assumeTrue(graph.features().vertex().getCardinality("to-change") ==
VertexProperty.Cardinality.list);
+
+ final AtomicBoolean triggered = new AtomicBoolean(false);
+ final Vertex v = graph.addVertex();
+ v.property("to-change", "blah");
+
+ final MutationListener listener = new AbstractMutationListener() {
+ @Override
+ public void vertexPropertyChanged(final Vertex element, final
VertexProperty oldValue, final Object setValue, final Object...
vertexPropertyKeyValues) {
+ assertThat(oldValue.isPresent(), is(false));
+ assertEquals("blah", setValue);
+ triggered.set(true);
+ }
+ };
+ final EventStrategy.Builder builder =
EventStrategy.build().addListener(listener);
+
+ if (graph.features().graph().supportsTransactions())
+ builder.eventQueue(new
EventStrategy.TransactionalEventQueue(graph));
+
+ final EventStrategy eventStrategy = builder.create();
+ final GraphTraversalSource gts = create(eventStrategy);
+
+ gts.V(v).property("to-change", "blah").iterate();
+ tryCommit(graph);
+
+ assertEquals(2, IteratorUtils.count(g.V(v).properties()));
+ assertThat(triggered.get(), is(true));
+ }
+
@Test
@FeatureRequirementSet(FeatureRequirementSet.Package.VERTICES_ONLY)
public void shouldDetachVertexPropertyWhenNew() {
diff --git
a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/TinkerGraphProvider.java
b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/TinkerGraphProvider.java
index 6d9dce7c01..ed595ed3dc 100644
---
a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/TinkerGraphProvider.java
+++
b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/TinkerGraphProvider.java
@@ -22,6 +22,7 @@ import org.apache.commons.configuration2.Configuration;
import org.apache.tinkerpop.gremlin.AbstractGraphProvider;
import org.apache.tinkerpop.gremlin.LoadGraphWith;
import org.apache.tinkerpop.gremlin.TestHelper;
+import
org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration.EventStrategyProcessTest;
import org.apache.tinkerpop.gremlin.structure.Graph;
import org.apache.tinkerpop.gremlin.structure.GraphTest;
import org.apache.tinkerpop.gremlin.structure.VertexProperty;
@@ -70,6 +71,8 @@ public class TinkerGraphProvider extends
AbstractGraphProvider {
put(TinkerGraph.GREMLIN_TINKERGRAPH_VERTEX_PROPERTY_ID_MANAGER,
idMaker);
if (requiresListCardinalityAsDefault(loadGraphWith, test,
testMethodName))
put(TinkerGraph.GREMLIN_TINKERGRAPH_DEFAULT_VERTEX_PROPERTY_CARDINALITY,
VertexProperty.Cardinality.list.name());
+ if (requiresSetCardinalityAsDefault(test, testMethodName))
+
put(TinkerGraph.GREMLIN_TINKERGRAPH_DEFAULT_VERTEX_PROPERTY_CARDINALITY,
VertexProperty.Cardinality.set.name());
if (requiresPersistence(test, testMethodName)) {
put(TinkerGraph.GREMLIN_TINKERGRAPH_GRAPH_FORMAT, "gryo");
put(TinkerGraph.GREMLIN_TINKERGRAPH_GRAPH_LOCATION,TestHelper.makeTestDataFile(test,
"temp", testMethodName + ".kryo"));
@@ -109,7 +112,17 @@ public class TinkerGraphProvider extends
AbstractGraphProvider {
final Class<?>
test, final String testMethodName) {
return loadGraphWith == LoadGraphWith.GraphData.CREW
|| (test == StarGraphTest.class &&
testMethodName.equals("shouldAttachWithCreateMethod"))
- || (test == DetachedGraphTest.class &&
testMethodName.equals("testAttachableCreateMethod"));
+ || (test == DetachedGraphTest.class &&
testMethodName.equals("testAttachableCreateMethod"))
+ || (test == EventStrategyProcessTest.class &&
testMethodName.equals("shouldNotCaptureOldVertexPropertyWhenAddedWithDefaultListCardinality"));
+ }
+
+ /**
+ * Determines if a test requires set cardinality as the default.
+ */
+ protected static boolean requiresSetCardinalityAsDefault(final Class<?>
test, final String testMethodName) {
+ return test == EventStrategyProcessTest.class
+ &&
(testMethodName.equals("shouldCaptureOldVertexPropertyWhenChangedWithDefaultSetCardinality")
+ ||
testMethodName.equals("shouldNotCaptureOldVertexPropertyWhenValueIsNewWithDefaultSetCardinality"));
}
/**
diff --git
a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/TinkerTransactionGraphProvider.java
b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/TinkerTransactionGraphProvider.java
index 8248e296c2..e82847148e 100644
---
a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/TinkerTransactionGraphProvider.java
+++
b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/TinkerTransactionGraphProvider.java
@@ -22,6 +22,7 @@ import org.apache.commons.configuration2.Configuration;
import org.apache.tinkerpop.gremlin.AbstractGraphProvider;
import org.apache.tinkerpop.gremlin.LoadGraphWith;
import org.apache.tinkerpop.gremlin.TestHelper;
+import
org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration.EventStrategyProcessTest;
import org.apache.tinkerpop.gremlin.structure.Graph;
import org.apache.tinkerpop.gremlin.structure.GraphTest;
import org.apache.tinkerpop.gremlin.structure.VertexProperty;
@@ -69,6 +70,8 @@ public class TinkerTransactionGraphProvider extends
AbstractGraphProvider {
put(TinkerTransactionGraph.GREMLIN_TINKERGRAPH_VERTEX_PROPERTY_ID_MANAGER,
idMaker);
if (requiresListCardinalityAsDefault(loadGraphWith, test,
testMethodName))
put(TinkerTransactionGraph.GREMLIN_TINKERGRAPH_DEFAULT_VERTEX_PROPERTY_CARDINALITY,
VertexProperty.Cardinality.list.name());
+ if (requiresSetCardinalityAsDefault(test, testMethodName))
+
put(TinkerTransactionGraph.GREMLIN_TINKERGRAPH_DEFAULT_VERTEX_PROPERTY_CARDINALITY,
VertexProperty.Cardinality.set.name());
if (requiresPersistence(test, testMethodName)) {
put(TinkerTransactionGraph.GREMLIN_TINKERGRAPH_GRAPH_FORMAT,
"gryo");
put(TinkerTransactionGraph.GREMLIN_TINKERGRAPH_GRAPH_LOCATION,TestHelper.makeTestDataFile(test,
"temp", testMethodName + ".kryo"));
@@ -108,7 +111,17 @@ public class TinkerTransactionGraphProvider extends
AbstractGraphProvider {
final Class<?>
test, final String testMethodName) {
return loadGraphWith == LoadGraphWith.GraphData.CREW
|| (test == StarGraphTest.class &&
testMethodName.equals("shouldAttachWithCreateMethod"))
- || (test == DetachedGraphTest.class &&
testMethodName.equals("testAttachableCreateMethod"));
+ || (test == DetachedGraphTest.class &&
testMethodName.equals("testAttachableCreateMethod"))
+ || (test == EventStrategyProcessTest.class &&
testMethodName.equals("shouldNotCaptureOldVertexPropertyWhenAddedWithDefaultListCardinality"));
+ }
+
+ /**
+ * Determines if a test requires set cardinality as the default.
+ */
+ protected static boolean requiresSetCardinalityAsDefault(final Class<?>
test, final String testMethodName) {
+ return test == EventStrategyProcessTest.class
+ &&
(testMethodName.equals("shouldCaptureOldVertexPropertyWhenChangedWithDefaultSetCardinality")
+ ||
testMethodName.equals("shouldNotCaptureOldVertexPropertyWhenValueIsNewWithDefaultSetCardinality"));
}
/**
