Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix logic errors in TreeNode.DeepCopy #821

Merged
merged 14 commits into from
Mar 22, 2024
16 changes: 10 additions & 6 deletions pkg/document/crdt/tree.go
Original file line number Diff line number Diff line change
Expand Up @@ -298,10 +298,10 @@ func (n *TreeNode) SplitElement(offset int, issueTimeTicket func() *time.Ticket)

leftChildren := n.Index.Children(true)[0:offset]
rightChildren := n.Index.Children(true)[offset:]
if err := n.Index.SetChildrenInternal(leftChildren); err != nil {
if err := n.Index.SetChildren(leftChildren); err != nil {
return nil, err
}
if err := split.Index.SetChildrenInternal(rightChildren); err != nil {
if err := split.Index.SetChildren(rightChildren); err != nil {
return nil, err
}

Expand Down Expand Up @@ -353,13 +353,16 @@ func (n *TreeNode) InsertAt(newNode *TreeNode, offset int) error {

// DeepCopy copies itself deeply.
func (n *TreeNode) DeepCopy() (*TreeNode, error) {
var clone *TreeNode
var attrs *RHT
if n.Attrs != nil {
clone = NewTreeNode(n.ID, n.Type(), n.Attrs.DeepCopy(), n.Value)
} else {
clone = NewTreeNode(n.ID, n.Type(), nil, n.Value)
attrs = n.Attrs.DeepCopy()
}

clone := NewTreeNode(n.ID, n.Type(), attrs, n.Value)
clone.Index.Length = n.Index.Length
clone.RemovedAt = n.RemovedAt
clone.InsPrevID = n.InsPrevID
clone.InsNextID = n.InsNextID

if n.IsText() {
return clone, nil
Expand All @@ -374,6 +377,7 @@ func (n *TreeNode) DeepCopy() (*TreeNode, error) {

children = append(children, node.Index)
}

if err := clone.Index.SetChildren(children); err != nil {
return nil, err
}
Expand Down
49 changes: 49 additions & 0 deletions pkg/document/crdt/tree_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,25 @@ var (
}
)

func createHelloTree(t *testing.T, ctx *change.Context) *crdt.Tree {
// TODO(raararaara): This test should be generalized. e.g) createTree(ctx, "<r><p>hello</p></r>")
// https://pkg.go.dev/encoding/xml#Unmarshal
tree := crdt.NewTree(crdt.NewTreeNode(helper.PosT(ctx), "r", nil), helper.TimeT(ctx))
_, err := tree.EditT(0, 0, []*crdt.TreeNode{
crdt.NewTreeNode(helper.PosT(ctx), "p", nil),
}, 0, helper.TimeT(ctx), issueTimeTicket(ctx))
assert.NoError(t, err)

_, err = tree.EditT(1, 1, []*crdt.TreeNode{
crdt.NewTreeNode(helper.PosT(ctx), "text", nil, "hello"),
}, 0, helper.TimeT(ctx), issueTimeTicket(ctx))
assert.NoError(t, err)
assert.Equal(t, "<r><p>hello</p></r>", tree.ToXML())
assert.Equal(t, 7, tree.Root().Len())

return tree
}

func TestTreeNode(t *testing.T) {
t.Run("text node test", func(t *testing.T) {
node := crdt.NewTreeNode(dummyTreeNodeID, "text", nil, "hello")
Expand Down Expand Up @@ -107,6 +126,35 @@ func TestTreeNode(t *testing.T) {
assert.Equal(t, test.length-2, right.Len())
}
})

t.Run("deepcopy test with deletion", func(t *testing.T) {
ctx := helper.TextChangeContext(helper.TestRoot())
tree := createHelloTree(t, ctx)

// To make tree have a deletion to check length modification.
_, err := tree.EditT(4, 5, nil, 0, helper.TimeT(ctx), issueTimeTicket(ctx))
assert.NoError(t, err)
assert.Equal(t, "<r><p>helo</p></r>", tree.ToXML())
assert.Equal(t, 6, tree.Root().Len())

clone, err := tree.Root().DeepCopy()
assert.NoError(t, err)
helper.AssertEqualTreeNode(t, tree.Root(), clone)
})

t.Run("deepcopy test with split", func(t *testing.T) {
ctx := helper.TextChangeContext(helper.TestRoot())
tree := createHelloTree(t, ctx)

// To make tree have split text nodes.
_, err := tree.EditT(3, 3, nil, 0, helper.TimeT(ctx), issueTimeTicket(ctx))
assert.NoError(t, err)
assert.Equal(t, "<r><p>hello</p></r>", tree.ToXML())

clone, err := tree.Root().DeepCopy()
assert.NoError(t, err)
helper.AssertEqualTreeNode(t, tree.Root(), clone)
})
}

func TestTreeEdit(t *testing.T) {
Expand Down Expand Up @@ -416,6 +464,7 @@ func TestTreeEdit(t *testing.T) {
assert.NoError(t, err)
assert.Equal(t, 0, idx)
})

}

func TestTreeSplit(t *testing.T) {
Expand Down
17 changes: 1 addition & 16 deletions pkg/index/tree.go
Original file line number Diff line number Diff line change
Expand Up @@ -311,23 +311,8 @@ func (n *Node[V]) Children(includeRemovedNode ...bool) []*Node[V] {
}

// SetChildren sets the children of the given node.
func (n *Node[V]) SetChildren(children []*Node[V]) error {
if n.IsText() {
return ErrInvalidMethodCallForTextNode
}

n.children = children
for _, child := range children {
child.Parent = n
child.UpdateAncestorsSize()
}

return nil
}

// SetChildrenInternal sets the children of the given node.
// This method does not update the size of the ancestors.
func (n *Node[V]) SetChildrenInternal(children []*Node[V]) error {
func (n *Node[V]) SetChildren(children []*Node[V]) error {
if n.IsText() {
return ErrInvalidMethodCallForTextNode
}
Expand Down
22 changes: 22 additions & 0 deletions test/helper/helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,28 @@ func BuildTreeNode(node *json.TreeNode) *crdt.TreeNode {
return doc.Root().GetTree("test").Root()
}

type treeNodePair struct {
node *crdt.TreeNode
parentID *crdt.TreeNodeID
}

func createTreeNodePairs(node *crdt.TreeNode, parentID *crdt.TreeNodeID) []treeNodePair {
var pairs []treeNodePair

pairs = append(pairs, treeNodePair{node, parentID})
for _, child := range node.Index.Children(true) {
pairs = append(pairs, createTreeNodePairs(child.Value, node.ID)...)
}
return pairs
}

// AssertEqualTreeNode asserts that the given TreeNodes are equal.
func AssertEqualTreeNode(t *testing.T, nodeA, nodeB *crdt.TreeNode) {
pairsA := createTreeNodePairs(nodeA, nil)
pairsB := createTreeNodePairs(nodeB, nil)
assert.Equal(t, pairsA, pairsB)
}

var portOffset = 0

// TestConfig returns config for creating Yorkie instance.
Expand Down
Loading