[
https://issues.apache.org/jira/browse/TINKERPOP-3304?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18137946#comment-18137946
]
ASF GitHub Bot commented on TINKERPOP-3304:
-------------------------------------------
spmallette commented on PR #3759:
URL: https://github.com/apache/tinkerpop/pull/3759#issuecomment-6629405558
thanks for submitting this - looking at the tests holistically, i can't help
wondering if rather than adding new mergeV/E tests we'd do better to fix the
existing ones and perhaps add more in the established pattern. If
`StubMutationListener` would have helped to perform the right assertions from
the outset I think we would have caught this problem early on because we
already have `shouldTriggerAddVertexPropertyChangedViaMergeV` and
`shouldTriggerUpdateEdgePropertyAddedViaMergeE`. On top of that, nothing is
really asserting that `property()` is even returning `KeyedProperty` and
`KeyedVertexProperty` types (for that matter I'm not sure I even see tests
validating cases for when there was a key removed.
would you be up for hardening the test cases a bit more with respect to
these concerns? i think it would be nice to close these gaps.
> mergeV() and mergeE() pass a null old value to MutationListener when the
> property did not exist yet
> ---------------------------------------------------------------------------------------------------
>
> Key: TINKERPOP-3304
> URL: https://issues.apache.org/jira/browse/TINKERPOP-3304
> Project: TinkerPop
> Issue Type: Bug
> Components: process
> Affects Versions: 3.7.7, 3.8.2
> Reporter: Paul
> Priority: Minor
>
> When EventStrategy is configured, {{MergeVertexStep}} and {{MergeEdgeStep}}
> (onMatch) report the properties they set through
> {{EventUtil.registerVertexPropertyChange()}} and
> {{EventUtil.registerEdgePropertyChange()}}. When the property does not exist
> yet, both pass {{null}} as the old value:
> {code:java}
> final Property<Object> oldValue = p.isPresent() ?
> eventStrategy.detach(edge.property(key)) : null;
> {code}
> {{AddPropertyStep}} does not. Since 3.4.0 (TINKERPOP-1831, see the 3.4.x
> upgrade documentation: "the event now raises with a {{KeyedVertexProperty}}
> or {{KeyedProperty}} instance, which only contains a property key and no
> value in them"), events raised from new mutations carry a keyed property
> instead of {{null}}, so a listener can tell which property was added. This is
> asserted by {{EventStrategyProcessTest}}
> ({{shouldDetachPropertyOfEdgeWhenNew}},
> {{shouldDetachVertexPropertyWhenNew}}). {{mergeV()}}/{{mergeE()}}, added in
> 3.6.0, still raise {{null}}: the key is lost, and a listener written for the
> documented contract fails with a {{NullPointerException}} on
> {{oldValue.key()}} or {{oldValue.isPresent()}}.
> To reproduce, with TinkerGraph:
> {code:java}
> MutationListener listener = ...; // records the oldValue received by
> edgePropertyChanged()
> GraphTraversalSource g = traversal().withEmbedded(graph)
> .withStrategies(EventStrategy.build().addListener(listener).create());
> Vertex v = graph.addVertex();
> v.addEdge("self", v);
> g.E().property("a", 1).iterate();
> // oldValue is a KeyedProperty, key() == "a"
> g.V(v).mergeE(Map.of(T.label, "self")).option(Merge.onMatch, Map.of("b",
> 2)).iterate();
> // oldValue is null: nothing tells the listener that "b" is the property that
> was set
> {code}
> The same happens with {{mergeV(...).option(Merge.onMatch, ...)}} and
> {{vertexPropertyChanged()}}. The repro above gives the same result on the
> current 3.7-dev and 3.8-dev branches; the code is identical in 3.7.3, 3.8.2
> and master.
> We hit this with a listener that records the history of property changes: the
> first modification of an edge made through {{mergeE()}} is never recorded.
> Expected: the same as {{property()}}, a {{KeyedProperty}} /
> {{KeyedVertexProperty}} when the property is new.
> Pull request against 3.7-dev: https://github.com/apache/tinkerpop/pull/3759
> Notes: the Javadoc of {{Event.EdgePropertyChangedEvent}} and
> {{Event.VertexPropertyChangedEvent}} still says that {{oldValue}} is {{null}}
> for a new property; it predates the 3.4.0 change and is updated in the fix. A
> listener that tests {{oldValue == null}} on merge events would receive a
> non-null, non-present property after the fix.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
