jackrabbit-oak-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michael Dürig (JIRA) <j...@apache.org>
Subject [jira] [Commented] (OAK-7388) MergingNodeStateDiff may recreate nodes that were previously removed to resolve conflicts
Date Wed, 04 Apr 2018 13:16:00 GMT

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

Michael Dürig commented on OAK-7388:
------------------------------------

Excellent analysis! The first snipped of code should be made into a unit test. And of course
we should try to fix this!

> 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)

Mime
View raw message