Major 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.
- 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.
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.