From 763df5ab4037df020b18bad504c2ae92fcceed7f Mon Sep 17 00:00:00 2001 From: Paul Lartigau Date: Tue, 13 Oct 2026 11:12:03 +0200 Subject: [PATCH] TINKERPOP-3294 Fixed AddPropertyStep old property capture with default cardinality Assisted-by: Claude Code:claude-opus-5-5 --- CHANGELOG.asciidoc | 2 + .../step/sideEffect/AddPropertyStep.java | 19 +-- .../decoration/EventStrategyProcessTest.java | 131 ++++++++++++++++++ .../tinkergraph/TinkerGraphProvider.java | 15 +- .../TinkerTransactionGraphProvider.java | 15 +- 5 files changed, 171 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index f3bbafa6463..b0f02d733ab 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 4223f98bd5c..691c11ad9f1 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 @@ protected void sideEffect(final Traverser.Admin traverser) { "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 optEventStrategy = getTraversal().getStrategies().getStrategy(EventStrategy.class); final boolean eventingIsConfigured = EventUtil.hasAnyCallbacks(callbackRegistry) && optEventStrategy.isPresent(); @@ -111,17 +115,13 @@ protected void sideEffect(final Traverser.Admin traverser) { // 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 @@ protected void sideEffect(final Traverser.Admin traverser) { } 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 properties = element.properties(key); while (properties.hasNext()) { final Property property = properties.next(); @@ -153,7 +154,7 @@ private Property captureRemovedProperty(final Element element, final String key, break; } } - } else if (cardinality == VertexProperty.Cardinality.single) { + } else if (effectiveCardinality == VertexProperty.Cardinality.single) { removedProperty = element.property(key); } } else { 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 0bad3387320..4c4c8d434eb 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.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 void vertexPropertyChanged(final Vertex element, final VertexProperty old 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 6d9dce7c01b..ed595ed3dce 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.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 Map getBaseConfiguration(final String graphName, final Cl 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 @@ protected static boolean requiresListCardinalityAsDefault(final LoadGraphWith.Gr 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 8248e296c2d..e82847148e4 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.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 Map getBaseConfiguration(final String graphName, final Cl 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 @@ protected static boolean requiresListCardinalityAsDefault(final LoadGraphWith.Gr 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")); } /**