[
https://issues.apache.org/jira/browse/TINKERPOP-3294?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18126664#comment-18126664
]
ASF GitHub Bot commented on TINKERPOP-3294:
-------------------------------------------
paul-lartigau opened a new pull request, #3715:
URL: https://github.com/apache/tinkerpop/pull/3715
https://issues.apache.org/jira/browse/TINKERPOP-3294
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().
Testing:
* Added shouldCaptureOldVertexPropertyWhenChangedWithDefaultCardinality to
EventStrategyProcessTest. It is skipped for graphs whose default cardinality
for the key is not single.
* Ran EventStrategyProcessTest through TinkerGraphProcessStandardTest:
passes with the fix, fails without it.
Merge notes: the code differs on 3.8-dev, the fallback cardinality is
computed after the call to captureRemovedProperty(), so it needs to be moved
above it when merging forward.
> 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)
