[
https://issues.apache.org/jira/browse/TINKERPOP-3294?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18126981#comment-18126981
]
ASF GitHub Bot commented on TINKERPOP-3294:
-------------------------------------------
paul-lartigau commented on code in PR #3715:
URL: https://github.com/apache/tinkerpop/pull/3715#discussion_r4268438281
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStep.java:
##########
@@ -126,7 +126,7 @@ protected void sideEffect(final Traverser.Admin<S>
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) :
Review Comment:
You're right, I missed that. I've patched it on my side in a fork for now,
so applying it to 3.8.x only is fine, and I'll pick it up there later. I'll
retarget this PR to 3.8-dev.
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStep.java:
##########
@@ -126,7 +126,7 @@ protected void sideEffect(final Traverser.Admin<S>
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) :
Review Comment:
You're right, I missed that. I've patched it on my side in a fork for now,
so applying it to 3.8.x only is fine, and I'll pick it up there later. I'll
retarget this PR to 3.8-dev.
> AddPropertyStep does not capture the old property for EventStrategy when
> cardinality is not specified
> -----------------------------------------------------------------------------------------------------
>
> Key: TINKERPOP-3294
> URL: https://issues.apache.org/jira/browse/TINKERPOP-3294
> Project: TinkerPop
> Issue Type: Bug
> Components: process
> Affects Versions: 3.7.7, 3.8.2
> Reporter: Paul
> Priority: Minor
> Fix For: 3.7.8, 3.8.3
>
>
> When EventStrategy is configured, AddPropertyStep calls
> captureRemovedProperty() to find the property being replaced, so it can be
> passed to vertexPropertyChanged() as the old value.
> captureRemovedProperty() checks the {{cardinality}} field of the step. That
> field is only set when the cardinality is given explicitly in the traversal,
> e.g. {{{}property(single, "name", "x"){}}}. With {{property("name", "x")}} it
> is null, so neither the {{single}} nor the {{set}} branch is taken and the
> old value is always reported as empty.
> The step already falls back to
> {{graph.features().vertex().getCardinality(key)}} when it writes the property
> (see TINKERPOP-2159), but that fallback is not used by
> captureRemovedProperty().
> To reproduce, with TinkerGraph (default cardinality {{{}single{}}}):
> {code:java}
> MutationListener listener = ...; // records oldValue in vertexPropertyChanged
> GraphTraversalSource g = traversal().withEmbedded(graph)
> .withStrategies(EventStrategy.build().addListener(listener).create());
> g.addV().property("name", "a").iterate();
> g.V().property("name", "b").iterate();
> // oldValue received by the listener is vp[empty], expected vp[name->a]
> {code}
> With {{property(single, "name", "b")}} the old value is reported correctly.
> The fix is to compute the effective cardinality once and pass it to
> captureRemovedProperty(). On 3.8-dev the fallback is computed after the call
> to captureRemovedProperty(), so it also needs to be moved up. The code
> differs between the two branches, so I plan to open one PR for 3.7-dev and
> one for 3.8-dev.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
