[jira] [Commented] (OAK-7388) MergingNodeStateDiff may recreate nodes that were previously removed to resolve conflicts
[
https://issues.apache.org/jira/browse/OAK-7388?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16427015#comment-16427015
]
Alex Deparvu commented on OAK-7388:
---
+1 good stuff!
> 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)
[jira] [Commented] (OAK-7388) MergingNodeStateDiff may recreate nodes that were previously removed to resolve conflicts
[
https://issues.apache.org/jira/browse/OAK-7388?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=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)
[jira] [Commented] (OAK-7388) MergingNodeStateDiff may recreate nodes that were previously removed to resolve conflicts
[
https://issues.apache.org/jira/browse/OAK-7388?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=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)
