Skip to content

Commit

Permalink
Remove panic from crdt.RGATreeList
Browse files Browse the repository at this point in the history
  • Loading branch information
sejongk committed Aug 2, 2023
1 parent 1b557be commit 78825d3
Show file tree
Hide file tree
Showing 12 changed files with 94 additions and 58 deletions.
4 changes: 3 additions & 1 deletion api/converter/from_bytes.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
27 changes: 16 additions & 11 deletions pkg/document/crdt/array.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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)
}

Expand All @@ -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.
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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.
Expand Down
9 changes: 6 additions & 3 deletions pkg/document/crdt/array_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
})
}
2 changes: 1 addition & 1 deletion pkg/document/crdt/element.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
8 changes: 4 additions & 4 deletions pkg/document/crdt/element_rht.go
Original file line number Diff line number Diff line change
Expand Up @@ -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("fail to find the given createdAt: %s", createdAt.Key())
}

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.
Expand Down
2 changes: 1 addition & 1 deletion pkg/document/crdt/object.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
48 changes: 28 additions & 20 deletions pkg/document/crdt/rga_tree_list.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
Expand Down Expand Up @@ -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("fail to find the given createdAt: %s", createdAt.Key())
}

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.
Expand All @@ -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("fail to find the given prevCreatedAt: %s", prevCreatedAt.Key())
}

node, ok := a.nodeMapByCreatedAt[createdAt.Key()]
if !ok {
panic("fail to find the given createdAt: " + createdAt.Key())
return fmt.Errorf("fail to find the given createdAt: %s", createdAt.Key())
}

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("fail to find the given prevCreatedAt: %s", createdAt.Key())
}

for {
Expand All @@ -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.
Expand All @@ -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("fail to find the given createdAt: %s", createdAt.Key())
}

for node.next != nil && node.next.PositionedAt().After(executedAt) {
node = node.next
}

return node
return node, nil
}

func (a *RGATreeList) release(node *RGATreeListNode) {
Expand All @@ -325,13 +328,18 @@ 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
}

a.nodeMapByIndex.InsertAfter(prevNode.indexNode, newNode.indexNode)
a.nodeMapByCreatedAt[value.CreatedAt().Key()] = newNode
return nil
}
25 changes: 16 additions & 9 deletions pkg/document/crdt/root_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down Expand Up @@ -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())
Expand Down
13 changes: 10 additions & 3 deletions pkg/document/json/array.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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(),
Expand All @@ -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)
}
}
5 changes: 4 additions & 1 deletion pkg/document/operations/add.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 1 addition & 3 deletions pkg/document/operations/move.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
5 changes: 4 additions & 1 deletion pkg/document/operations/remove.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down

0 comments on commit 78825d3

Please sign in to comment.