[ 
https://issues.apache.org/jira/browse/OAK-7388?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16425526#comment-16425526
 ] 

Francesco Mari commented on OAK-7388:
-------------------------------------

I added a failing unit test at r1828343.

> MergingNodeStateDiff may recreate nodes that were previously removed to 
> resolve conflicts
> -----------------------------------------------------------------------------------------
>
>                 Key: OAK-7388
>                 URL: https://issues.apache.org/jira/browse/OAK-7388
>             Project: Jackrabbit Oak
>          Issue Type: Improvement
>          Components: core
>            Reporter: Francesco Mari
>            Assignee: Francesco Mari
>            Priority: Major
>             Fix For: 1.10
>
>
> {{MergingNodeStateDiff}} might behave incorrectly when the resolution of a 
> conflict involves the deletion of the conflicting node. I spotted this issue 
> in a use case that can be expressed by the following code.
> {noformat}
> NodeState root = EmptyNodeState.EMPTY_NODE;
> NodeState withProperty;
> {
>     NodeBuilder builder = root.builder();
>     builder.child("c").setProperty("foo", "bar");
>     withProperty = builder.getNodeState();
> }
> NodeState withUpdatedProperty;
> {
>     NodeBuilder builder = withProperty.builder();
>     builder.child("c").setProperty("foo", "baz");
>     withUpdatedProperty = builder.getNodeState();
> }
> NodeState withRemovedChild;
> {
>     NodeBuilder builder = withProperty.builder();
>     builder.child("c").remove();
>     withRemovedChild = builder.getNodeState();
> }
> NodeBuilder mergedBuilder = withUpdatedProperty.builder();
> withRemovedChild.compareAgainstBaseState(withProperty, new 
> ConflictAnnotatingRebaseDiff(mergedBuilder));
> NodeState merged = 
> ConflictHook.of(DefaultThreeWayConflictHandler.OURS).processCommit(
>       mergedBuilder.getBaseState(), 
>       mergedBuilder.getNodeState(), 
>       CommitInfo.EMPTY
> );
> assertFalse(merged.hasChildNode("c"));
> {noformat}
> The assertion at the end of the code fails becauuse `merged` actually has a 
> child node named `c`, and `c` is an empty node. After digging into the issue, 
> I figured out that the problem is caused by the following steps.
> # {{MergingNodeStateDiff#childNodeAdded}} is invoked because of 
> {{:conflicts}}. This eventually results in the deletion of the conflicting 
> child node.
> # {{MergingNodeStateDiff#childNodeChanged}} is called because in 
> {{ModifiedNodeState#compareAgainstBaseState}} the children are compared with 
> the {{!=}} operator instead of using {{Object#equals}}.
> # {{org.apache.jackrabbit.oak.spi.state.NodeBuilder#child}} is called in 
> order to setup a new {{MergingNodeStateDiff}} to descend into the subtree 
> that was detected as modified.
> # {{MemoryNodeBuilder#hasChildNode}} correctly returns {{false}}, because the 
> child was removed in step 1. The return value of {{false}} triggers the next 
> step.
> # {{MemoryNodeBuilder#setChildNode(java.lang.String)}} is invoked in order to 
> setup a new, empty child node.
> In other words, the snippet above can be rewritten like the following.
> {noformat}
> NodeState root = EmptyNodeState.EMPTY_NODE;
> NodeState withProperty;
> {
>     NodeBuilder builder = root.builder();
>     builder.child("c").setProperty("foo", "bar");
>     withProperty = builder.getNodeState();
> }
> NodeState withUpdatedProperty;
> {
>     NodeBuilder builder = withProperty.builder();
>     builder.child("c").setProperty("foo", "baz");
>     withUpdatedProperty = builder.getNodeState();
> }
> NodeState withRemovedChild;
> {
>     NodeBuilder builder = withProperty.builder();
>     builder.child("c").remove();
>     withRemovedChild = builder.getNodeState();
> }
> NodeBuilder mergedBuilder = withUpdatedProperty.builder();
> // As per MergingNodeStateDiff.childNodeAdded()
> mergedBuilder.child("c").remove();
> // As per ModifiedNodeState#compareAgainstBaseState()
> if (withUpdatedProperty.getChildNode("c") != 
> withRemovedChild.getChildNode("c")) {
>     // As per MergingNodeStateDiff.childNodeChanged()
>     mergedBuilder.child("c");
> }
> NodeState merged = mergedBuilder.getNodeState();
> assertFalse(merged.hasChildNode("c"));
> {noformat}
> The end result is that {{MergingNodeStateDiff}} inadvertently adds the node 
> that was removed in order to resolve a conflict.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Reply via email to