Handle concurrent Tree.RemoveStyle (#883)

This commit addresses an issue where if a node has an attribute with the same key as the RemoveStyle is simultaneously inserted by Tree.Edit, the attribute of the concurrently inserted Node with the same key gets deleted. To resolve this issue, this commit adds filtering logic in RemoveStyle to prevent attribute deletion from concurrently inserted nodes.
yorkie-team · Jun 3, 2024 · f3dbee7 · f3dbee7
1 parent 3647f35
commit f3dbee7
Show file tree

Hide file tree

Showing 5 changed files with 62 additions and 32 deletions.
diff --git a/api/converter/from_pb.go b/api/converter/from_pb.go
@@ -511,23 +511,24 @@ func fromTreeStyle(pbTreeStyle *api.Operation_TreeStyle) (*operations.TreeStyle,
 		return nil, err
 	}
 
+	createdAtMapByActor, err := fromCreatedAtMapByActor(
+		pbTreeStyle.CreatedAtMapByActor,
+	)
+	if err != nil {
+		return nil, err
+	}
+
 	if len(pbTreeStyle.AttributesToRemove) > 0 {
 		return operations.NewTreeStyleRemove(
 			parentCreatedAt,
 			from,
 			to,
+			createdAtMapByActor,
 			pbTreeStyle.AttributesToRemove,
 			executedAt,
 		), nil
 	}
 
-	createdAtMapByActor, err := fromCreatedAtMapByActor(
-		pbTreeStyle.CreatedAtMapByActor,
-	)
-	if err != nil {
-		return nil, err
-	}
-
 	return operations.NewTreeStyle(
 		parentCreatedAt,
 		from,

diff --git a/pkg/document/crdt/tree.go b/pkg/document/crdt/tree.go
@@ -394,6 +394,10 @@ func (n *TreeNode) canDelete(removedAt *time.Ticket, maxCreatedAt *time.Ticket)
 }
 
 func (n *TreeNode) canStyle(editedAt *time.Ticket, maxCreatedAt *time.Ticket) bool {
+	if n.IsText() {
+		return false
+	}
+
 	return !n.id.CreatedAt.After(maxCreatedAt) &&
 		(n.removedAt == nil || editedAt.After(n.removedAt))
 }
@@ -982,7 +986,7 @@ func (t *Tree) Style(
 			}
 		}
 
-		if node.canStyle(editedAt, maxCreatedAt) && !node.IsText() && len(attrs) > 0 {
+		if node.canStyle(editedAt, maxCreatedAt) && len(attrs) > 0 {
 			maxCreatedAt = createdAtMapByActor[actorIDHex]
 			createdAt := node.id.CreatedAt
 			if maxCreatedAt == nil || createdAt.After(maxCreatedAt) {
@@ -1011,37 +1015,56 @@ func (t *Tree) RemoveStyle(
 	to *TreePos,
 	attrs []string,
 	editedAt *time.Ticket,
-) ([]GCPair, error) {
+	maxCreatedAtMapByActor map[string]*time.Ticket,
+) (map[string]*time.Ticket, []GCPair, error) {
 	fromParent, fromLeft, err := t.FindTreeNodesWithSplitText(from, editedAt)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 	toParent, toLeft, err := t.FindTreeNodesWithSplitText(to, editedAt)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
 	var pairs []GCPair
+	createdAtMapByActor := make(map[string]*time.Ticket)
 	if err = t.traverseInPosRange(fromParent, fromLeft, toParent, toLeft, func(token index.TreeToken[*TreeNode], _ bool) {
 		node := token.Node
-		if node.IsRemoved() || node.IsText() {
-			return
+		actorIDHex := node.id.CreatedAt.ActorIDHex()
+
+		var maxCreatedAt *time.Ticket
+		if maxCreatedAtMapByActor == nil {
+			maxCreatedAt = time.MaxTicket
+		} else {
+			if createdAt, ok := maxCreatedAtMapByActor[actorIDHex]; ok {
+				maxCreatedAt = createdAt
+			} else {
+				maxCreatedAt = time.InitialTicket
+			}
 		}
 
-		for _, attr := range attrs {
-			rhtNodes := node.RemoveAttr(attr, editedAt)
-			for _, rhtNode := range rhtNodes {
-				pairs = append(pairs, GCPair{
-					Parent: node,
-					Child:  rhtNode,
-				})
+		if node.canStyle(editedAt, maxCreatedAt) && len(attrs) > 0 {
+			maxCreatedAt = createdAtMapByActor[actorIDHex]
+			createdAt := node.id.CreatedAt
+			if maxCreatedAt == nil || createdAt.After(maxCreatedAt) {
+				createdAtMapByActor[actorIDHex] = createdAt
+			}
+
+			for _, attr := range attrs {
+				rhtNodes := node.RemoveAttr(attr, editedAt)
+				for _, rhtNode := range rhtNodes {
+					pairs = append(pairs, GCPair{
+						Parent: node,
+						Child:  rhtNode,
+					})
+				}
 			}
 		}
 	}); err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
-	return pairs, nil
+	return createdAtMapByActor, pairs, nil
 }
 
 // FindTreeNodesWithSplitText finds TreeNode of the given crdt.TreePos and

diff --git a/pkg/document/json/tree.go b/pkg/document/json/tree.go
@@ -262,7 +262,7 @@ func (t *Tree) RemoveStyle(fromIdx, toIdx int, attributesToRemove []string) bool
 	}
 
 	ticket := t.context.IssueTimeTicket()
-	pairs, err := t.Tree.RemoveStyle(fromPos, toPos, attributesToRemove, ticket)
+	maxCreationMapByActor, pairs, err := t.Tree.RemoveStyle(fromPos, toPos, attributesToRemove, ticket, nil)
 	if err != nil {
 		panic(err)
 	}
@@ -275,6 +275,7 @@ func (t *Tree) RemoveStyle(fromIdx, toIdx int, attributesToRemove []string) bool
 		t.CreatedAt(),
 		fromPos,
 		toPos,
+		maxCreationMapByActor,
 		attributesToRemove,
 		ticket,
 	))

diff --git a/pkg/document/operations/tree_style.go b/pkg/document/operations/tree_style.go
@@ -71,16 +71,18 @@ func NewTreeStyleRemove(
 	parentCreatedAt *time.Ticket,
 	from *crdt.TreePos,
 	to *crdt.TreePos,
+	maxCreatedAtMapByActor map[string]*time.Ticket,
 	attributesToRemove []string,
 	executedAt *time.Ticket,
 ) *TreeStyle {
 	return &TreeStyle{
-		parentCreatedAt:    parentCreatedAt,
-		from:               from,
-		to:                 to,
-		attributes:         map[string]string{},
-		attributesToRemove: attributesToRemove,
-		executedAt:         executedAt,
+		parentCreatedAt:        parentCreatedAt,
+		from:                   from,
+		to:                     to,
+		maxCreatedAtMapByActor: maxCreatedAtMapByActor,
+		attributes:             map[string]string{},
+		attributesToRemove:     attributesToRemove,
+		executedAt:             executedAt,
 	}
 }
 
@@ -100,7 +102,7 @@ func (e *TreeStyle) Execute(root *crdt.Root) error {
 			return err
 		}
 	} else {
-		pairs, err = obj.RemoveStyle(e.from, e.to, e.attributesToRemove, e.executedAt)
+		_, pairs, err = obj.RemoveStyle(e.from, e.to, e.attributesToRemove, e.executedAt, e.maxCreatedAtMapByActor)
 		if err != nil {
 			return err
 		}

diff --git a/test/integration/tree_concurrency_test.go b/test/integration/tree_concurrency_test.go
@@ -490,7 +490,10 @@ func TestTreeConcurrencyEditStyle(t *testing.T) {
 	}
 	initialXML := `<root><p color="red">a</p><p color="red">b</p><p color="red">c</p></root>`
 
-	content := &json.TreeNode{Type: "p", Attributes: map[string]string{"italic": "true"}, Children: []json.TreeNode{{Type: "text", Value: `d`}}}
+	content := &json.TreeNode{Type: "p", Attributes: map[string]string{
+		"italic": "true",
+		"color":  "blue",
+	}, Children: []json.TreeNode{{Type: "text", Value: `d`}}}
 
 	ranges := []twoRangesType{
 		// equal: <p>b</p> - <p>b</p>
@@ -519,7 +522,7 @@ func TestTreeConcurrencyEditStyle(t *testing.T) {
 	}
 
 	styleOperations := []operationInterface{
-		styleOperationType{RangeAll, StyleRemove, "color", "", `remove-bold`},
+		styleOperationType{RangeAll, StyleRemove, "color", "", `remove-color`},
 		styleOperationType{RangeAll, StyleSet, "bold", "aa", `set-bold-aa`},
 	}