Uploaded image for project: 'Jackrabbit Oak'
  1. Jackrabbit Oak
  2. OAK-7388

MergingNodeStateDiff may recreate nodes that were previously removed to resolve conflicts

    XMLWordPrintableJSON

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.9.0, 1.10.0
    • Component/s: core
    • Labels:
      None

      Description

      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.

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

      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.

      1. MergingNodeStateDiff#childNodeAdded is invoked because of :conflicts. This eventually results in the deletion of the conflicting child node.
      2. MergingNodeStateDiff#childNodeChanged is called because in ModifiedNodeState#compareAgainstBaseState the children are compared with the != operator instead of using Object#equals.
      3. 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.
      4. MemoryNodeBuilder#hasChildNode correctly returns false, because the child was removed in step 1. The return value of false triggers the next step.
      5. 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.

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

      The end result is that MergingNodeStateDiff inadvertently adds the node that was removed in order to resolve a conflict.

        Attachments

          Activity

            People

            • Assignee:
              frm Francesco Mari
              Reporter:
              frm Francesco Mari
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: