[jira] [Closed] (OAK-7388) MergingNodeStateDiff may recreate nodes that were previously removed to resolve conflicts

["Davide Giannella (JIRA)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/OAK-7388?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Davide Giannella closed OAK-7388.
---------------------------------

Bulk close 1.9.0

> 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.9.0, 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)