From c6ed71ba24040bac694028be9e18be022795a0db Mon Sep 17 00:00:00 2001 From: Sejong Kim <46182768+sejongk@users.noreply.github.com> Date: Thu, 3 Aug 2023 18:31:35 +0900 Subject: [PATCH] Remove panic from crdt.RGATreeList (#596) --- api/converter/from_bytes.go | 4 +- pkg/document/crdt/array.go | 27 ++++++---- pkg/document/crdt/array_test.go | 9 ++-- pkg/document/crdt/element.go | 2 +- pkg/document/crdt/element_rht.go | 8 +-- pkg/document/crdt/object.go | 2 +- pkg/document/crdt/rga_tree_list.go | 48 ++++++++++------- pkg/document/crdt/rga_tree_list_test.go | 72 +++++++++++++++++++++++++ pkg/document/crdt/root_test.go | 25 +++++---- pkg/document/json/array.go | 13 +++-- pkg/document/operations/add.go | 5 +- pkg/document/operations/move.go | 4 +- pkg/document/operations/remove.go | 5 +- 13 files changed, 166 insertions(+), 58 deletions(-) create mode 100644 pkg/document/crdt/rga_tree_list_test.go diff --git a/api/converter/from_bytes.go b/api/converter/from_bytes.go index 6f0b6e8fa..04662aded 100644 --- a/api/converter/from_bytes.go +++ b/api/converter/from_bytes.go @@ -147,7 +147,9 @@ func fromJSONArray(pbArr *api.JSONElement_JSONArray) (*crdt.Array, error) { if err != nil { return nil, err } - elements.Add(elem) + if err = elements.Add(elem); err != nil { + return nil, err + } } createdAt, err := fromTimeTicket(pbArr.CreatedAt) diff --git a/pkg/document/crdt/array.go b/pkg/document/crdt/array.go index 9faf44419..458c42027 100644 --- a/pkg/document/crdt/array.go +++ b/pkg/document/crdt/array.go @@ -43,9 +43,8 @@ func (a *Array) Purge(elem Element) error { } // Add adds the given element at the last. -func (a *Array) Add(elem Element) *Array { - a.elements.Add(elem) - return a +func (a *Array) Add(elem Element) error { + return a.elements.Add(elem) } // Get returns the element of the given index. @@ -59,7 +58,7 @@ func (a *Array) Get(idx int) (Element, error) { // FindPrevCreatedAt returns the creation time of the previous element of the // given element. -func (a *Array) FindPrevCreatedAt(createdAt *time.Ticket) *time.Ticket { +func (a *Array) FindPrevCreatedAt(createdAt *time.Ticket) (*time.Ticket, error) { return a.elements.FindPrevCreatedAt(createdAt) } @@ -74,8 +73,8 @@ func (a *Array) Delete(idx int, deletedAt *time.Ticket) (Element, error) { // MoveAfter moves the given `createdAt` element after the `prevCreatedAt` // element. -func (a *Array) MoveAfter(prevCreatedAt, createdAt, executedAt *time.Ticket) { - a.elements.MoveAfter(prevCreatedAt, createdAt, executedAt) +func (a *Array) MoveAfter(prevCreatedAt, createdAt, executedAt *time.Ticket) error { + return a.elements.MoveAfter(prevCreatedAt, createdAt, executedAt) } // Elements returns an array of elements contained in this RGATreeList. @@ -111,7 +110,9 @@ func (a *Array) DeepCopy() (Element, error) { if err != nil { return nil, err } - elements.Add(copiedNode) + if err = elements.Add(copiedNode); err != nil { + return nil, err + } } array := NewArray(elements, a.createdAt) @@ -160,13 +161,17 @@ func (a *Array) LastCreatedAt() *time.Ticket { } // InsertAfter inserts the given element after the given previous element. -func (a *Array) InsertAfter(prevCreatedAt *time.Ticket, element Element) { - a.elements.InsertAfter(prevCreatedAt, element) +func (a *Array) InsertAfter(prevCreatedAt *time.Ticket, element Element) error { + return a.elements.InsertAfter(prevCreatedAt, element) } // DeleteByCreatedAt deletes the given element. -func (a *Array) DeleteByCreatedAt(createdAt *time.Ticket, deletedAt *time.Ticket) Element { - return a.elements.DeleteByCreatedAt(createdAt, deletedAt).elem +func (a *Array) DeleteByCreatedAt(createdAt *time.Ticket, deletedAt *time.Ticket) (Element, error) { + node, err := a.elements.DeleteByCreatedAt(createdAt, deletedAt) + if err != nil { + return nil, err + } + return node.elem, nil } // Len returns length of this Array. diff --git a/pkg/document/crdt/array_test.go b/pkg/document/crdt/array_test.go index 805708e08..920422cda 100644 --- a/pkg/document/crdt/array_test.go +++ b/pkg/document/crdt/array_test.go @@ -32,11 +32,14 @@ func TestArray(t *testing.T) { a := crdt.NewArray(crdt.NewRGATreeList(), ctx.IssueTimeTicket()) - a.Add(crdt.NewPrimitive("1", ctx.IssueTimeTicket())) + err := a.Add(crdt.NewPrimitive("1", ctx.IssueTimeTicket())) + assert.NoError(t, err) assert.Equal(t, `["1"]`, a.Marshal()) - a.Add(crdt.NewPrimitive("2", ctx.IssueTimeTicket())) + err = a.Add(crdt.NewPrimitive("2", ctx.IssueTimeTicket())) + assert.NoError(t, err) assert.Equal(t, `["1","2"]`, a.Marshal()) - a.Add(crdt.NewPrimitive("3", ctx.IssueTimeTicket())) + err = a.Add(crdt.NewPrimitive("3", ctx.IssueTimeTicket())) + assert.NoError(t, err) assert.Equal(t, `["1","2","3"]`, a.Marshal()) }) } diff --git a/pkg/document/crdt/element.go b/pkg/document/crdt/element.go index 2702d2de2..5330ced41 100644 --- a/pkg/document/crdt/element.go +++ b/pkg/document/crdt/element.go @@ -36,7 +36,7 @@ type Container interface { Descendants(callback func(elem Element, parent Container) bool) // DeleteByCreatedAt removes the given element from this container. - DeleteByCreatedAt(createdAt *time.Ticket, deletedAt *time.Ticket) Element + DeleteByCreatedAt(createdAt *time.Ticket, deletedAt *time.Ticket) (Element, error) } // GCElement represents Element which has GC. diff --git a/pkg/document/crdt/element_rht.go b/pkg/document/crdt/element_rht.go index 5d90699e9..8186bc3f0 100644 --- a/pkg/document/crdt/element_rht.go +++ b/pkg/document/crdt/element_rht.go @@ -129,17 +129,17 @@ func (rht *ElementRHT) Delete(k string, deletedAt *time.Ticket) Element { } // DeleteByCreatedAt deletes the Element of the given creation time. -func (rht *ElementRHT) DeleteByCreatedAt(createdAt *time.Ticket, deletedAt *time.Ticket) Element { +func (rht *ElementRHT) DeleteByCreatedAt(createdAt *time.Ticket, deletedAt *time.Ticket) (Element, error) { node, ok := rht.nodeMapByCreatedAt[createdAt.Key()] if !ok { - return nil + return nil, fmt.Errorf("DeleteByCreatedAt %s: %w", createdAt.Key(), ErrChildNotFound) } if !node.Remove(deletedAt) { - return nil + return nil, nil } - return node.elem + return node.elem, nil } // Elements returns a map of elements because the map easy to use for loop. diff --git a/pkg/document/crdt/object.go b/pkg/document/crdt/object.go index c27d43d32..abcba45e4 100644 --- a/pkg/document/crdt/object.go +++ b/pkg/document/crdt/object.go @@ -63,7 +63,7 @@ func (o *Object) Has(k string) bool { } // DeleteByCreatedAt deletes the element of the given creation time. -func (o *Object) DeleteByCreatedAt(createdAt *time.Ticket, deletedAt *time.Ticket) Element { +func (o *Object) DeleteByCreatedAt(createdAt *time.Ticket, deletedAt *time.Ticket) (Element, error) { return o.memberNodes.DeleteByCreatedAt(createdAt, deletedAt) } diff --git a/pkg/document/crdt/rga_tree_list.go b/pkg/document/crdt/rga_tree_list.go index 0273b4c6f..26d132857 100644 --- a/pkg/document/crdt/rga_tree_list.go +++ b/pkg/document/crdt/rga_tree_list.go @@ -149,8 +149,8 @@ func (a *RGATreeList) Marshal() string { } // Add adds the given element at the last. -func (a *RGATreeList) Add(elem Element) { - a.insertAfter(a.last.CreatedAt(), elem, elem.CreatedAt()) +func (a *RGATreeList) Add(elem Element) error { + return a.insertAfter(a.last.CreatedAt(), elem, elem.CreatedAt()) } // Nodes returns an array of elements contained in this RGATreeList. @@ -175,8 +175,8 @@ func (a *RGATreeList) LastCreatedAt() *time.Ticket { } // InsertAfter inserts the given element after the given previous element. -func (a *RGATreeList) InsertAfter(prevCreatedAt *time.Ticket, elem Element) { - a.insertAfter(prevCreatedAt, elem, elem.CreatedAt()) +func (a *RGATreeList) InsertAfter(prevCreatedAt *time.Ticket, elem Element) error { + return a.insertAfter(prevCreatedAt, elem, elem.CreatedAt()) } // Get returns the element of the given index. @@ -207,17 +207,17 @@ func (a *RGATreeList) Get(idx int) (*RGATreeListNode, error) { } // DeleteByCreatedAt deletes the given element. -func (a *RGATreeList) DeleteByCreatedAt(createdAt *time.Ticket, deletedAt *time.Ticket) *RGATreeListNode { +func (a *RGATreeList) DeleteByCreatedAt(createdAt *time.Ticket, deletedAt *time.Ticket) (*RGATreeListNode, error) { node, ok := a.nodeMapByCreatedAt[createdAt.Key()] if !ok { - panic("fail to find the given createdAt: " + createdAt.Key()) + return nil, fmt.Errorf("DeleteByCreatedAt %s: %w", createdAt.Key(), ErrChildNotFound) } alreadyRemoved := node.isRemoved() if node.elem.Remove(deletedAt) && !alreadyRemoved { a.nodeMapByIndex.Splay(node.indexNode) } - return node + return node, nil } // Len returns length of this RGATreeList. @@ -237,35 +237,38 @@ func (a *RGATreeList) Delete(idx int, deletedAt *time.Ticket) (*RGATreeListNode, if err != nil { return nil, err } - return a.DeleteByCreatedAt(target.CreatedAt(), deletedAt), nil + return a.DeleteByCreatedAt(target.CreatedAt(), deletedAt) } // MoveAfter moves the given `createdAt` element after the `prevCreatedAt` // element. -func (a *RGATreeList) MoveAfter(prevCreatedAt, createdAt, executedAt *time.Ticket) { +func (a *RGATreeList) MoveAfter(prevCreatedAt, createdAt, executedAt *time.Ticket) error { prevNode, ok := a.nodeMapByCreatedAt[prevCreatedAt.Key()] if !ok { - panic("fail to find the given prevCreatedAt: " + prevCreatedAt.Key()) + return fmt.Errorf("MoveAfter %s: %w", prevCreatedAt.Key(), ErrChildNotFound) } node, ok := a.nodeMapByCreatedAt[createdAt.Key()] if !ok { - panic("fail to find the given createdAt: " + createdAt.Key()) + return fmt.Errorf("MoveAfter %s: %w", createdAt.Key(), ErrChildNotFound) } if node.elem.MovedAt() == nil || executedAt.After(node.elem.MovedAt()) { a.release(node) - a.insertAfter(prevNode.CreatedAt(), node.elem, executedAt) + if err := a.insertAfter(prevNode.CreatedAt(), node.elem, executedAt); err != nil { + return err + } node.elem.SetMovedAt(executedAt) } + return nil } // FindPrevCreatedAt returns the creation time of the previous element of the // given element. -func (a *RGATreeList) FindPrevCreatedAt(createdAt *time.Ticket) *time.Ticket { +func (a *RGATreeList) FindPrevCreatedAt(createdAt *time.Ticket) (*time.Ticket, error) { node, ok := a.nodeMapByCreatedAt[createdAt.Key()] if !ok { - panic("fail to find the given prevCreatedAt: " + createdAt.Key()) + return nil, fmt.Errorf("FindPrevCreatedAt %s: %w", createdAt.Key(), ErrChildNotFound) } for { @@ -275,7 +278,7 @@ func (a *RGATreeList) FindPrevCreatedAt(createdAt *time.Ticket) *time.Ticket { } } - return node.CreatedAt() + return node.CreatedAt(), nil } // purge physically purge child element. @@ -293,17 +296,17 @@ func (a *RGATreeList) purge(elem Element) error { func (a *RGATreeList) findNextBeforeExecutedAt( createdAt *time.Ticket, executedAt *time.Ticket, -) *RGATreeListNode { +) (*RGATreeListNode, error) { node, ok := a.nodeMapByCreatedAt[createdAt.Key()] if !ok { - panic("fail to find the given createdAt: " + createdAt.Key()) + return nil, fmt.Errorf("findNextBeforeExecutedAt %s: %w", createdAt.Key(), ErrChildNotFound) } for node.next != nil && node.next.PositionedAt().After(executedAt) { node = node.next } - return node + return node, nil } func (a *RGATreeList) release(node *RGATreeListNode) { @@ -325,8 +328,12 @@ func (a *RGATreeList) insertAfter( prevCreatedAt *time.Ticket, value Element, executedAt *time.Ticket, -) { - prevNode := a.findNextBeforeExecutedAt(prevCreatedAt, executedAt) +) error { + prevNode, err := a.findNextBeforeExecutedAt(prevCreatedAt, executedAt) + if err != nil { + return err + } + newNode := newRGATreeListNodeAfter(prevNode, value) if prevNode == a.last { a.last = newNode @@ -334,4 +341,5 @@ func (a *RGATreeList) insertAfter( a.nodeMapByIndex.InsertAfter(prevNode.indexNode, newNode.indexNode) a.nodeMapByCreatedAt[value.CreatedAt().Key()] = newNode + return nil } diff --git a/pkg/document/crdt/rga_tree_list_test.go b/pkg/document/crdt/rga_tree_list_test.go new file mode 100644 index 000000000..8f669f84a --- /dev/null +++ b/pkg/document/crdt/rga_tree_list_test.go @@ -0,0 +1,72 @@ +package crdt_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/yorkie-team/yorkie/pkg/document/crdt" + "github.com/yorkie-team/yorkie/test/helper" +) + +func TestRGATreeList(t *testing.T) { + t.Run("rga_tree_list operations test", func(t *testing.T) { + root := helper.TestRoot() + ctx := helper.TextChangeContext(root) + + elements := crdt.NewRGATreeList() + + var err error + for _, v := range []string{"1", "2", "3"} { + err = elements.Add(crdt.NewPrimitive(v, ctx.IssueTimeTicket())) + assert.NoError(t, err) + } + assert.Equal(t, `["1","2","3"]`, elements.Marshal()) + + nodes := elements.Nodes() + assert.Equal(t, len(nodes), 3) + + targetElement, err := elements.Get(1) + assert.NoError(t, err) + assert.Equal(t, `"2"`, targetElement.Element().Marshal()) + + prevCreatedAt, err := elements.FindPrevCreatedAt(targetElement.CreatedAt()) + assert.NoError(t, err) + assert.Equal(t, prevCreatedAt.Compare(targetElement.CreatedAt()), -1) + + err = elements.MoveAfter(targetElement.CreatedAt(), prevCreatedAt, ctx.IssueTimeTicket()) + assert.NoError(t, err) + assert.Equal(t, `["2","1","3"]`, elements.Marshal()) + + _, err = elements.DeleteByCreatedAt(targetElement.CreatedAt(), ctx.IssueTimeTicket()) + assert.NoError(t, err) + assert.Equal(t, `["1","3"]`, elements.Marshal()) + + _, err = elements.Delete(1, ctx.IssueTimeTicket()) + assert.NoError(t, err) + assert.Equal(t, `["1"]`, elements.Marshal()) + + }) + + t.Run("invalid createdAt test", func(t *testing.T) { + root := helper.TestRoot() + ctx := helper.TextChangeContext(root) + + validCreatedAt, invalidCreatedAt := ctx.IssueTimeTicket(), ctx.IssueTimeTicket() + elements := crdt.NewRGATreeList() + err := elements.Add(crdt.NewPrimitive("1", validCreatedAt)) + assert.NoError(t, err) + + _, err = elements.DeleteByCreatedAt(invalidCreatedAt, ctx.IssueTimeTicket()) + assert.ErrorIs(t, err, crdt.ErrChildNotFound) + + err = elements.MoveAfter(validCreatedAt, invalidCreatedAt, ctx.IssueTimeTicket()) + assert.ErrorIs(t, err, crdt.ErrChildNotFound) + + err = elements.MoveAfter(invalidCreatedAt, validCreatedAt, ctx.IssueTimeTicket()) + assert.ErrorIs(t, err, crdt.ErrChildNotFound) + + _, err = elements.FindPrevCreatedAt(invalidCreatedAt) + assert.ErrorIs(t, err, crdt.ErrChildNotFound) + }) +} diff --git a/pkg/document/crdt/root_test.go b/pkg/document/crdt/root_test.go index 64e5c77f4..df8f69a6e 100644 --- a/pkg/document/crdt/root_test.go +++ b/pkg/document/crdt/root_test.go @@ -36,15 +36,18 @@ func TestRoot(t *testing.T) { t.Run("garbage collection for array test", func(t *testing.T) { root := helper.TestRoot() ctx := helper.TextChangeContext(root) - array := crdt.NewArray(crdt.NewRGATreeList(), ctx.IssueTimeTicket()) - array.Add(crdt.NewPrimitive(0, ctx.IssueTimeTicket())) - array.Add(crdt.NewPrimitive(1, ctx.IssueTimeTicket())) - array.Add(crdt.NewPrimitive(2, ctx.IssueTimeTicket())) + array := crdt.NewArray(crdt.NewRGATreeList(), ctx.IssueTimeTicket()) + var err error + for _, v := range []int{0, 1, 2} { + err = array.Add(crdt.NewPrimitive(v, ctx.IssueTimeTicket())) + assert.NoError(t, err) + } assert.Equal(t, "[0,1,2]", array.Marshal()) targetElement, _ := array.Get(1) - array.DeleteByCreatedAt(targetElement.CreatedAt(), ctx.IssueTimeTicket()) + _, err = array.DeleteByCreatedAt(targetElement.CreatedAt(), ctx.IssueTimeTicket()) + assert.NoError(t, err) root.RegisterRemovedElementPair(array, targetElement) assert.Equal(t, "[0,2]", array.Marshal()) assert.Equal(t, 1, root.GarbageLen()) @@ -182,10 +185,14 @@ func TestRoot(t *testing.T) { obj := root.Object() obj.Set("1", crdt.NewPrimitive(1, ctx.IssueTimeTicket())) - arr := crdt.NewArray(crdt.NewRGATreeList(), ctx.IssueTimeTicket()). - Add(crdt.NewPrimitive(1, ctx.IssueTimeTicket())). - Add(crdt.NewPrimitive(2, ctx.IssueTimeTicket())). - Add(crdt.NewPrimitive(3, ctx.IssueTimeTicket())) + + arr := crdt.NewArray(crdt.NewRGATreeList(), ctx.IssueTimeTicket()) + var err error + for _, v := range []int{1, 2, 3} { + err = arr.Add(crdt.NewPrimitive(v, ctx.IssueTimeTicket())) + assert.NoError(t, err) + } + obj.Set("2", arr) obj.Set("3", crdt.NewPrimitive(3, ctx.IssueTimeTicket())) assert.Equal(t, `{"1":1,"2":[1,2,3],"3":3}`, root.Object().Marshal()) diff --git a/pkg/document/json/array.go b/pkg/document/json/array.go index 7f7c0bca9..5d13de46d 100644 --- a/pkg/document/json/array.go +++ b/pkg/document/json/array.go @@ -214,7 +214,9 @@ func (p *Array) insertAfterInternal( ticket, )) - p.InsertAfter(prevCreatedAt, value) + if err = p.InsertAfter(prevCreatedAt, value); err != nil { + panic(err) + } p.context.RegisterElement(value) return elem @@ -223,7 +225,10 @@ func (p *Array) insertAfterInternal( func (p *Array) moveBeforeInternal(nextCreatedAt, createdAt *time.Ticket) { ticket := p.context.IssueTimeTicket() - prevCreatedAt := p.FindPrevCreatedAt(nextCreatedAt) + prevCreatedAt, err := p.FindPrevCreatedAt(nextCreatedAt) + if err != nil { + panic(err) + } p.context.Push(operations.NewMove( p.Array.CreatedAt(), @@ -232,5 +237,7 @@ func (p *Array) moveBeforeInternal(nextCreatedAt, createdAt *time.Ticket) { ticket, )) - p.MoveAfter(prevCreatedAt, createdAt, ticket) + if err = p.MoveAfter(prevCreatedAt, createdAt, ticket); err != nil { + panic(err) + } } diff --git a/pkg/document/operations/add.go b/pkg/document/operations/add.go index d2e1ec58a..9ea18d5d8 100644 --- a/pkg/document/operations/add.go +++ b/pkg/document/operations/add.go @@ -64,7 +64,10 @@ func (o *Add) Execute(root *crdt.Root) error { if err != nil { return err } - obj.InsertAfter(o.prevCreatedAt, value) + + if err = obj.InsertAfter(o.prevCreatedAt, value); err != nil { + return err + } root.RegisterElement(value) return nil diff --git a/pkg/document/operations/move.go b/pkg/document/operations/move.go index 265006292..5207b8ecf 100644 --- a/pkg/document/operations/move.go +++ b/pkg/document/operations/move.go @@ -60,9 +60,7 @@ func (o *Move) Execute(root *crdt.Root) error { return ErrNotApplicableDataType } - obj.MoveAfter(o.prevCreatedAt, o.createdAt, o.executedAt) - - return nil + return obj.MoveAfter(o.prevCreatedAt, o.createdAt, o.executedAt) } // CreatedAt returns the creation time of the target element. diff --git a/pkg/document/operations/remove.go b/pkg/document/operations/remove.go index dce83903c..4abf3cdcc 100644 --- a/pkg/document/operations/remove.go +++ b/pkg/document/operations/remove.go @@ -53,7 +53,10 @@ func (o *Remove) Execute(root *crdt.Root) error { switch parent := parentElem.(type) { case crdt.Container: - elem := parent.DeleteByCreatedAt(o.createdAt, o.executedAt) + elem, err := parent.DeleteByCreatedAt(o.createdAt, o.executedAt) + if err != nil { + return err + } if elem != nil { root.RegisterRemovedElementPair(parent, elem) }