Cole-Greer commented on code in PR #3715:
URL: https://github.com/apache/tinkerpop/pull/3715#discussion_r4343344095
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStep.java:
##########
@@ -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);
Review Comment:
On further inspection, I think there's actually a deeper pre-existing bug
here in `captureRemovedProperty`, which is actually worsened by this fix.
The whole intent of this method is very clear under the assumption that
properties only ever have single cardinality. Writing a property on an existing
key results in an overwrite, which triggers events for one property removal,
and one addition. It still works ok for always `set` graphs (logs a remove and
add if a matching property is already present in the Set) as well as `list`
graphs (nothing ever gets removed).
Where this logic fails is if the new property is changing the existing
cardinality.
Consider the following example:
```
gremlin> g.addV().property(list, "email", "[email protected]").property(list,
"email", "[email protected]")
==>v[0]
gremlin> g.V().valueMap()
==>[email:[[email protected],[email protected]]]
gremlin> g.V().property(single, "email", "[email protected]")
==>v[0]
gremlin> g.V().valueMap()
==>[email:[[email protected]]]
```
During that second AddProperty, all the pre-existing list entries are
dropped. In this case we don't need to capture a single removed property, we
need to capture many. The same applies for replacing `Set` with `Single`
This is a pre-existing bug, which is actually worse than simply missing
removed property events, as this call to `element.property(key)` will actually
throw an exception if this already more than 1 property associated with the
key. The issue with respect to this PR, is it actually makes this code path
more likely to be hit, as it existing `cardinality = null` no-op case, with the
much more common `effectiveCardinality = default cardinality (often single)`
case.
Fixing this bug is a bigger change, as we need to rework the removed
property mechanism to track whole lists of removed properties. @paul-lartigau
I'll let you decide how you wish to proceed with this PR. We could include the
full fix to this other bug before proceeding, or if you want to get this PR in
now (as it is a self-contained fix it its own right), I would suggest we make
the temporary patch with a reference to a new JIRA:
```suggestion
// Only tracks the first removed property, potentially
missing events if existing cardinality was Set or List. TINKERPOP-XXXX
removedProperty = element.properties(key).next();
```
This would at least bypass the potential for an exception, which would
ensure no regressions from this change.
Let me know how you wish to proceed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
