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.

Sign up for GitHub

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

Handle concurrent Tree.RemoveStyle #883

Merged
merged 1 commit into from
Jun 3, 2024
Merged

Handle concurrent Tree.RemoveStyle #883

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions api/converter/from_pb.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
53 changes: 38 additions & 15 deletions pkg/document/crdt/tree.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
Expand Down Expand Up @@ -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 {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

Codebase Verification

The canStyle method in tree.go explicitly prevents text nodes from being styled by returning false if n.IsText() is true. This contradicts the change in the Style method, which now allows text nodes to be styled.

  • pkg/document/crdt/tree.go:397-399: The canStyle method returns false for text nodes.
  • pkg/document/crdt/tree.go:989: The Style method now allows text nodes to be styled if canStyle returns true.

This inconsistency needs to be addressed to ensure the logic is coherent.

Analysis chain

Verify the consistency of allowing text nodes to be styled here, given the update in canStyle method.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify if there are any other methods that contradict the logic of not styling text nodes.

# Test: Search for methods related to styling text nodes. Expect: No other methods should allow styling text nodes.
rg --type go $'IsText() && .*Style'

Length of output: 36

Script:

#!/bin/bash
# Search for the implementation of the `canStyle` method in the codebase.
ast-grep --lang go --pattern 'func ($_ $_) canStyle($_, $_) bool { $$$ }'

# Search for all usages of the `canStyle` method in the codebase.
rg --type go 'canStyle'

Length of output: 1524

maxCreatedAt = createdAtMapByActor[actorIDHex]
createdAt := node.id.CreatedAt
if maxCreatedAt == nil || createdAt.After(maxCreatedAt) {
Expand Down Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion pkg/document/json/tree.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand All @@ -275,6 +275,7 @@ func (t *Tree) RemoveStyle(fromIdx, toIdx int, attributesToRemove []string) bool
t.CreatedAt(),
fromPos,
toPos,
maxCreationMapByActor,
attributesToRemove,
ticket,
))
Expand Down
16 changes: 9 additions & 7 deletions pkg/document/operations/tree_style.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
}

Expand All @@ -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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

Codebase Verification

The search results indicate that there are existing tests for the RemoveStyle method in test/integration/tree_concurrency_test.go and test/integration/tree_test.go. However, there are no tests specifically covering the maxCreatedAtMapByActor parameter.

  • test/integration/tree_concurrency_test.go
  • test/integration/tree_test.go

Please add unit tests that specifically target the new maxCreatedAtMapByActor parameter to ensure its correctness under various concurrent conditions.

Analysis chain

Ensure that the RemoveStyle method correctly handles the new maxCreatedAtMapByActor parameter. This change is crucial for maintaining the integrity of node attributes in concurrent editing scenarios. It would be beneficial to add more detailed unit tests specifically targeting this new logic to ensure its correctness under various concurrent conditions.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that the new logic in `RemoveStyle` is covered by unit tests.

# Test: Search for unit tests covering the new logic in `RemoveStyle`.
rg --type go $'func Test.*RemoveStyle' test/

Length of output: 45

Script:

#!/bin/bash
# Locate the definition of the `RemoveStyle` method to understand its context.
ast-grep --lang go --pattern 'func ($_).RemoveStyle($_, $_, $_, $_, $_) { $$$ }'

# Search for any tests related to `RemoveStyle`.
rg --type go 'RemoveStyle' test/

# Specifically look for tests involving the `maxCreatedAtMapByActor` parameter.
rg --type go 'maxCreatedAtMapByActor' test/

Length of output: 608

if err != nil {
return err
}
Expand Down
7 changes: 5 additions & 2 deletions test/integration/tree_concurrency_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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`},
}

Expand Down
Loading