From 459a0872782f0a119a121459472ec55ceeb11100 Mon Sep 17 00:00:00 2001 From: emplam27 Date: Tue, 25 Apr 2023 11:52:33 +0900 Subject: [PATCH 1/6] Remove panic method in crdt text - go sdk crdt text has the condition that cause panic - remove panic prevent shutdown server when crdt text error occurred --- pkg/document/crdt/array.go | 12 +++++++++--- pkg/document/crdt/counter.go | 4 ++-- pkg/document/crdt/element.go | 2 +- pkg/document/crdt/object.go | 12 +++++++++--- pkg/document/crdt/primitive.go | 4 ++-- pkg/document/crdt/primitive_test.go | 2 +- pkg/document/crdt/root.go | 10 ++++++++-- pkg/document/crdt/text.go | 6 +++--- pkg/document/document.go | 6 +++++- pkg/document/json/array.go | 7 ++++++- pkg/document/json/object.go | 7 ++++++- pkg/document/operations/add.go | 7 ++++++- pkg/document/operations/set.go | 7 ++++++- server/rpc/auth/webhook.go | 2 +- 14 files changed, 65 insertions(+), 23 deletions(-) diff --git a/pkg/document/crdt/array.go b/pkg/document/crdt/array.go index df5be3e4e..aa7d293f6 100644 --- a/pkg/document/crdt/array.go +++ b/pkg/document/crdt/array.go @@ -17,6 +17,8 @@ package crdt import ( + "fmt" + "github.com/yorkie-team/yorkie/pkg/document/time" ) @@ -95,16 +97,20 @@ func (a *Array) StructureAsString() string { } // DeepCopy copies itself deeply. -func (a *Array) DeepCopy() Element { +func (a *Array) DeepCopy() (Element, error) { elements := NewRGATreeList() for _, node := range a.elements.Nodes() { - elements.Add(node.elem.DeepCopy()) + copiedNode, err := node.elem.DeepCopy() + if err != nil { + return nil, fmt.Errorf("crdt array DeepCopy: %w", err) + } + elements.Add(copiedNode) } array := NewArray(elements, a.createdAt) array.removedAt = a.removedAt - return array + return array, nil } // CreatedAt returns the creation time of this array. diff --git a/pkg/document/crdt/counter.go b/pkg/document/crdt/counter.go index b31d57df6..0bf7c5159 100644 --- a/pkg/document/crdt/counter.go +++ b/pkg/document/crdt/counter.go @@ -96,9 +96,9 @@ func (p *Counter) Marshal() string { } // DeepCopy copies itself deeply. -func (p *Counter) DeepCopy() Element { +func (p *Counter) DeepCopy() (Element, error) { counter := *p - return &counter + return &counter, nil } // CreatedAt returns the creation time. diff --git a/pkg/document/crdt/element.go b/pkg/document/crdt/element.go index c915b8c8a..c2f155b1f 100644 --- a/pkg/document/crdt/element.go +++ b/pkg/document/crdt/element.go @@ -47,7 +47,7 @@ type Element interface { Marshal() string // DeepCopy copies itself deeply. - DeepCopy() Element + DeepCopy() (Element, error) // CreatedAt returns the creation time of this element. CreatedAt() *time.Ticket diff --git a/pkg/document/crdt/object.go b/pkg/document/crdt/object.go index f93fc84b3..4522cc7f4 100644 --- a/pkg/document/crdt/object.go +++ b/pkg/document/crdt/object.go @@ -17,6 +17,8 @@ package crdt import ( + "fmt" + "github.com/yorkie-team/yorkie/pkg/document/time" ) @@ -94,16 +96,20 @@ func (o *Object) Marshal() string { } // DeepCopy copies itself deeply. -func (o *Object) DeepCopy() Element { +func (o *Object) DeepCopy() (Element, error) { members := NewElementRHT() for _, node := range o.memberNodes.Nodes() { - members.Set(node.key, node.elem.DeepCopy()) + copiedNode, err := node.elem.DeepCopy() + if err != nil { + return nil, fmt.Errorf("crdt object DeepCopy: %w", err) + } + members.Set(node.key, copiedNode) } obj := NewObject(members, o.createdAt) obj.removedAt = o.removedAt - return obj + return obj, nil } // CreatedAt returns the creation time of this object. diff --git a/pkg/document/crdt/primitive.go b/pkg/document/crdt/primitive.go index 2bdc94b3f..0308db5c9 100644 --- a/pkg/document/crdt/primitive.go +++ b/pkg/document/crdt/primitive.go @@ -219,9 +219,9 @@ func (p *Primitive) Marshal() string { } // DeepCopy copies itself deeply. -func (p *Primitive) DeepCopy() Element { +func (p *Primitive) DeepCopy() (Element, error) { primitive := *p - return &primitive + return &primitive, nil } // CreatedAt returns the creation time. diff --git a/pkg/document/crdt/primitive_test.go b/pkg/document/crdt/primitive_test.go index ce5110f4a..8d4d7d2d5 100644 --- a/pkg/document/crdt/primitive_test.go +++ b/pkg/document/crdt/primitive_test.go @@ -53,7 +53,7 @@ func TestPrimitive(t *testing.T) { assert.Equal(t, prim.Value(), crdt.ValueFromBytes(prim.ValueType(), prim.Bytes())) assert.Equal(t, prim.Marshal(), test.marshal) - copied := prim.DeepCopy() + copied, _ := prim.DeepCopy() assert.Equal(t, prim.CreatedAt(), copied.CreatedAt()) assert.Equal(t, prim.MovedAt(), copied.MovedAt()) assert.Equal(t, prim.Marshal(), copied.Marshal()) diff --git a/pkg/document/crdt/root.go b/pkg/document/crdt/root.go index 9d4454341..530c58c62 100644 --- a/pkg/document/crdt/root.go +++ b/pkg/document/crdt/root.go @@ -20,6 +20,8 @@ package crdt import ( + "fmt" + "github.com/yorkie-team/yorkie/pkg/document/time" ) @@ -101,8 +103,12 @@ func (r *Root) RegisterTextElementWithGarbage(textType TextElement) { } // DeepCopy copies itself deeply. -func (r *Root) DeepCopy() *Root { - return NewRoot(r.object.DeepCopy().(*Object)) +func (r *Root) DeepCopy() (*Root, error) { + copiedObject, err := r.object.DeepCopy() + if err != nil { + return nil, fmt.Errorf("crdt root DeepCopy: %w", err) + } + return NewRoot(copiedObject.(*Object)), nil } // GarbageCollect purge elements that were removed before the given time. diff --git a/pkg/document/crdt/text.go b/pkg/document/crdt/text.go index dac3f6e3d..657255d4a 100644 --- a/pkg/document/crdt/text.go +++ b/pkg/document/crdt/text.go @@ -166,7 +166,7 @@ func (t *Text) Marshal() string { } // DeepCopy copies itself deeply. -func (t *Text) DeepCopy() Element { +func (t *Text) DeepCopy() (Element, error) { rgaTreeSplit := NewRGATreeSplit(InitialTextNode()) current := rgaTreeSplit.InitialHead() @@ -176,13 +176,13 @@ func (t *Text) DeepCopy() Element { if insPrevID != nil { insPrevNode := rgaTreeSplit.FindNode(insPrevID) if insPrevNode == nil { - panic("insPrevNode should be presence") + return nil, fmt.Errorf("crdt text DeepCopy: insPrevNode should be presence") } current.SetInsPrev(insPrevNode) } } - return NewText(rgaTreeSplit, t.createdAt) + return NewText(rgaTreeSplit, t.createdAt), nil } // CreatedAt returns the creation time of this Text. diff --git a/pkg/document/document.go b/pkg/document/document.go index cc1bcbaf6..5b16c102d 100644 --- a/pkg/document/document.go +++ b/pkg/document/document.go @@ -215,7 +215,11 @@ func (d *Document) GarbageLen() int { func (d *Document) ensureClone() { if d.clone == nil { - d.clone = d.doc.root.DeepCopy() + copiedDoc, err := d.doc.root.DeepCopy() + if err != nil { + panic("document ensureClone: " + err.Error()) + } + d.clone = copiedDoc } } diff --git a/pkg/document/json/array.go b/pkg/document/json/array.go index 1c0ba8e03..bf9088bbc 100644 --- a/pkg/document/json/array.go +++ b/pkg/document/json/array.go @@ -17,6 +17,7 @@ package json import ( + "fmt" gotime "time" "github.com/yorkie-team/yorkie/pkg/document/change" @@ -186,10 +187,14 @@ func (p *Array) insertAfterInternal( elem := creator(ticket) value := toOriginal(elem) + copiedValue, err := value.DeepCopy() + if err != nil { + panic(fmt.Sprintf("json array insertAfterInternal: %s", err.Error())) + } p.context.Push(operations.NewAdd( p.Array.CreatedAt(), prevCreatedAt, - value.DeepCopy(), + copiedValue, ticket, )) diff --git a/pkg/document/json/object.go b/pkg/document/json/object.go index c951d7fd1..55186227f 100644 --- a/pkg/document/json/object.go +++ b/pkg/document/json/object.go @@ -17,6 +17,7 @@ package json import ( + "fmt" gotime "time" "github.com/yorkie-team/yorkie/pkg/document/change" @@ -257,10 +258,14 @@ func (p *Object) setInternal( elem := creator(ticket) value := toOriginal(elem) + copiedValue, err := value.DeepCopy() + if err != nil { + panic(fmt.Sprintf("json object setInternal: %s", err.Error())) + } p.context.Push(operations.NewSet( p.CreatedAt(), k, - value.DeepCopy(), + copiedValue, ticket, )) diff --git a/pkg/document/operations/add.go b/pkg/document/operations/add.go index eb4be7aa9..5b84d7f9b 100644 --- a/pkg/document/operations/add.go +++ b/pkg/document/operations/add.go @@ -17,6 +17,8 @@ package operations import ( + "fmt" + "github.com/yorkie-team/yorkie/pkg/document/crdt" "github.com/yorkie-team/yorkie/pkg/document/time" ) @@ -60,7 +62,10 @@ func (o *Add) Execute(root *crdt.Root) error { return ErrNotApplicableDataType } - value := o.value.DeepCopy() + value, err := o.value.DeepCopy() + if err != nil { + return fmt.Errorf("add operations: %w", err) + } obj.InsertAfter(o.prevCreatedAt, value) root.RegisterElement(value) diff --git a/pkg/document/operations/set.go b/pkg/document/operations/set.go index 398bdd16e..0a5b7a730 100644 --- a/pkg/document/operations/set.go +++ b/pkg/document/operations/set.go @@ -17,6 +17,8 @@ package operations import ( + "fmt" + "github.com/yorkie-team/yorkie/pkg/document/crdt" "github.com/yorkie-team/yorkie/pkg/document/time" ) @@ -61,7 +63,10 @@ func (o *Set) Execute(root *crdt.Root) error { return ErrNotApplicableDataType } - value := o.value.DeepCopy() + value, err := o.value.DeepCopy() + if err != nil { + return fmt.Errorf("set operations: %w", err) + } removed := obj.Set(o.key, value) root.RegisterElement(value) if removed != nil { diff --git a/server/rpc/auth/webhook.go b/server/rpc/auth/webhook.go index cb15a53c8..457116e4d 100644 --- a/server/rpc/auth/webhook.go +++ b/server/rpc/auth/webhook.go @@ -130,7 +130,7 @@ func withExponentialBackoff(ctx context.Context, cfg *backend.Config, webhookFn select { case <-ctx.Done(): - return ctx.Err() + return fmt.Errorf("%w", ctx.Err()) case <-time.After(waitBeforeRetry): } From e2a373b5fcb1c72f68fd3b3c2ccabc114737f3ed Mon Sep 17 00:00:00 2001 From: emplam27 Date: Tue, 25 Apr 2023 19:07:19 +0900 Subject: [PATCH 2/6] Remove panic method in crdt rga_tree_split --- api/converter/from_bytes.go | 27 ++-- api/converter/from_pb.go | 9 +- pkg/document/crdt/element.go | 2 +- pkg/document/crdt/rga_tree_split.go | 153 +++++++++++++++-------- pkg/document/crdt/rga_tree_split_test.go | 3 +- pkg/document/crdt/root.go | 9 +- pkg/document/crdt/root_test.go | 46 ++++--- pkg/document/crdt/text.go | 39 ++++-- pkg/document/crdt/text_test.go | 17 +-- pkg/document/document.go | 16 ++- pkg/document/internal_document.go | 7 +- pkg/document/json/object.go | 6 +- pkg/document/json/text.go | 21 +++- pkg/document/operations/edit.go | 12 +- pkg/llrb/llrb.go | 106 +++++++++++----- pkg/llrb/llrb_test.go | 16 +-- 16 files changed, 327 insertions(+), 162 deletions(-) diff --git a/api/converter/from_bytes.go b/api/converter/from_bytes.go index 3b60be6ab..8e71bf90b 100644 --- a/api/converter/from_bytes.go +++ b/api/converter/from_bytes.go @@ -163,34 +163,43 @@ func fromJSONText( ) (*crdt.Text, error) { createdAt, err := fromTimeTicket(pbText.CreatedAt) if err != nil { - return nil, err + return nil, fmt.Errorf("from bytes fromJsonText: %w", err) } movedAt, err := fromTimeTicket(pbText.MovedAt) if err != nil { - return nil, err + return nil, fmt.Errorf("from bytes fromJsonText: %w", err) } removedAt, err := fromTimeTicket(pbText.RemovedAt) if err != nil { - return nil, err + return nil, fmt.Errorf("from bytes fromJsonText: %w", err) } - rgaTreeSplit := crdt.NewRGATreeSplit(crdt.InitialTextNode()) + rgaTreeSplit, err := crdt.NewRGATreeSplit(crdt.InitialTextNode()) + if err != nil { + return nil, fmt.Errorf("from bytes fromJsonText: %w", err) + } current := rgaTreeSplit.InitialHead() for _, pbNode := range pbText.Nodes { textNode, err := fromTextNode(pbNode) if err != nil { - return nil, err + return nil, fmt.Errorf("from bytes fromJsonText: %w", err) + } + current, err = rgaTreeSplit.InsertAfter(current, textNode) + if err != nil { + return nil, fmt.Errorf("from bytes fromJsonText: %w", err) } - current = rgaTreeSplit.InsertAfter(current, textNode) insPrevID, err := fromTextNodeID(pbNode.InsPrevId) if err != nil { - return nil, err + return nil, fmt.Errorf("from bytes fromJsonText: %w", err) } if insPrevID != nil { - insPrevNode := rgaTreeSplit.FindNode(insPrevID) + insPrevNode, err := rgaTreeSplit.FindNode(insPrevID) + if err != nil { + return nil, fmt.Errorf("from bytes fromJsonText: %w", err) + } if insPrevNode == nil { - panic("insPrevNode should be presence") + return nil, fmt.Errorf("from bytes fromJsonText: insPrevNode should be presence") } current.SetInsPrev(insPrevNode) } diff --git a/api/converter/from_pb.go b/api/converter/from_pb.go index b39ecb56c..5be276b29 100644 --- a/api/converter/from_pb.go +++ b/api/converter/from_pb.go @@ -611,10 +611,11 @@ func fromElement(pbElement *api.JSONElementSimple) (crdt.Element, error) { if err != nil { return nil, err } - return crdt.NewText( - crdt.NewRGATreeSplit(crdt.InitialTextNode()), - createdAt, - ), nil + rgaTreeSplit, err := crdt.NewRGATreeSplit(crdt.InitialTextNode()) + if err != nil { + return nil, err + } + return crdt.NewText(rgaTreeSplit, createdAt), nil case api.ValueType_VALUE_TYPE_INTEGER_CNT: fallthrough case api.ValueType_VALUE_TYPE_LONG_CNT: diff --git a/pkg/document/crdt/element.go b/pkg/document/crdt/element.go index c2f155b1f..88aded4f1 100644 --- a/pkg/document/crdt/element.go +++ b/pkg/document/crdt/element.go @@ -38,7 +38,7 @@ type Container interface { type TextElement interface { Element removedNodesLen() int - purgeTextNodesWithGarbage(ticket *time.Ticket) int + purgeTextNodesWithGarbage(ticket *time.Ticket) (int, error) } // Element represents JSON element. diff --git a/pkg/document/crdt/rga_tree_split.go b/pkg/document/crdt/rga_tree_split.go index 12a9aca32..d10f4ae57 100644 --- a/pkg/document/crdt/rga_tree_split.go +++ b/pkg/document/crdt/rga_tree_split.go @@ -42,31 +42,34 @@ func NewRGATreeSplitNodeID(createdAt *time.Ticket, offset int) *RGATreeSplitNode } // Compare returns an integer comparing two ID. The result will be 0 if -// id==other, -1 if id < other, and +1 if id > other. If the receiver or -// argument is nil, it would panic at runtime. -func (id *RGATreeSplitNodeID) Compare(other llrb.Key) int { +// id==other, -1 if id < other, and +1 if id > other. +func (id *RGATreeSplitNodeID) Compare(other llrb.Key) (int, error) { if id == nil || other == nil { - panic("RGATreeSplitNodeID cannot be null") + return 0, fmt.Errorf("rga tree split Compare: RGATreeSplitNodeID cannot be null") } o := other.(*RGATreeSplitNodeID) compare := id.createdAt.Compare(o.createdAt) if compare != 0 { - return compare + return compare, nil } if id.offset > o.offset { - return 1 + return 1, nil } else if id.offset < o.offset { - return -1 + return -1, nil } - return 0 + return 0, nil } // Equal returns whether given ID equals to this ID or not. -func (id *RGATreeSplitNodeID) Equal(other llrb.Key) bool { - return id.Compare(other) == 0 +func (id *RGATreeSplitNodeID) Equal(other llrb.Key) (bool, error) { + keyCompare, err := id.Compare(other) + if err != nil { + return false, fmt.Errorf("rga tree split Equal: %w", err) + } + return keyCompare == 0, nil } // CreatedAt returns the creation time of this ID. @@ -136,11 +139,15 @@ func (pos *RGATreeSplitNodePos) RelativeOffset() int { } // Equal returns the whether the given pos equals or not. -func (pos *RGATreeSplitNodePos) Equal(other *RGATreeSplitNodePos) bool { - if !pos.id.Equal(other.id) { - return false +func (pos *RGATreeSplitNodePos) Equal(other *RGATreeSplitNodePos) (bool, error) { + result, err := pos.id.Equal(other.id) + if err != nil { + return false, fmt.Errorf("rga tree split Equal: %w", err) + } + if !result { + return false, nil } - return pos.relativeOffset == other.relativeOffset + return pos.relativeOffset == other.relativeOffset, nil } // Selection represents the selection of text range in the editor. @@ -296,17 +303,20 @@ type RGATreeSplit[V RGATreeSplitValue] struct { } // NewRGATreeSplit creates a new instance of RGATreeSplit. -func NewRGATreeSplit[V RGATreeSplitValue](initialHead *RGATreeSplitNode[V]) *RGATreeSplit[V] { +func NewRGATreeSplit[V RGATreeSplitValue](initialHead *RGATreeSplitNode[V]) (*RGATreeSplit[V], error) { treeByIndex := splay.NewTree(initialHead.indexNode) treeByID := llrb.NewTree[*RGATreeSplitNodeID, *RGATreeSplitNode[V]]() - treeByID.Put(initialHead.ID(), initialHead) + _, err := treeByID.Put(initialHead.ID(), initialHead) + if err != nil { + return nil, fmt.Errorf("rga tree split NewRGATreeSplit: %w", err) + } return &RGATreeSplit[V]{ initialHead: initialHead, treeByIndex: treeByIndex, treeByID: treeByID, removedNodeMap: make(map[string]*RGATreeSplitNode[V]), - } + }, nil } func (s *RGATreeSplit[V]) createRange(from, to int) (*RGATreeSplitNodePos, *RGATreeSplitNodePos) { @@ -330,52 +340,65 @@ func (s *RGATreeSplit[V]) findNodePos(index int) *RGATreeSplitNodePos { func (s *RGATreeSplit[V]) findNodeWithSplit( pos *RGATreeSplitNodePos, updatedAt *time.Ticket, -) (*RGATreeSplitNode[V], *RGATreeSplitNode[V]) { +) (*RGATreeSplitNode[V], *RGATreeSplitNode[V], error) { absoluteID := pos.getAbsoluteID() - node := s.findFloorNodePreferToLeft(absoluteID) + node, err := s.findFloorNodePreferToLeft(absoluteID) + if err != nil { + return nil, nil, fmt.Errorf("findNodeWithSplit: %w", err) + } relativeOffset := absoluteID.offset - node.id.offset - s.splitNode(node, relativeOffset) + _, err = s.splitNode(node, relativeOffset) + if err != nil { + return nil, nil, fmt.Errorf("findNodeWithSplit: %w", err) + } for node.next != nil && node.next.createdAt().After(updatedAt) { node = node.next } - return node, node.next + return node, node.next, nil } -func (s *RGATreeSplit[V]) findFloorNodePreferToLeft(id *RGATreeSplitNodeID) *RGATreeSplitNode[V] { - node := s.findFloorNode(id) +func (s *RGATreeSplit[V]) findFloorNodePreferToLeft(id *RGATreeSplitNodeID) (*RGATreeSplitNode[V], error) { + node, err := s.findFloorNode(id) + if err != nil { + return nil, fmt.Errorf("findFloorNodePreferToLeft: %w", err) + } if node == nil { - panic("the node of the given id should be found: " + s.StructureAsString()) + return nil, + fmt.Errorf("findFloorNodePreferToLeft: the node of the given id should be found: " + s.StructureAsString()) } if id.offset > 0 && node.id.offset == id.offset { // NOTE: InsPrev may not be present due to GC. if node.insPrev == nil { - return node + return node, nil } node = node.insPrev } - return node + return node, nil } -func (s *RGATreeSplit[V]) splitNode(node *RGATreeSplitNode[V], offset int) *RGATreeSplitNode[V] { +func (s *RGATreeSplit[V]) splitNode(node *RGATreeSplitNode[V], offset int) (*RGATreeSplitNode[V], error) { if offset > node.contentLen() { - panic("offset should be less than or equal to length: " + s.StructureAsString()) + return nil, fmt.Errorf("splitNode: offset should be less than or equal to length: " + s.StructureAsString()) } if offset == 0 { - return node + return node, nil } else if offset == node.contentLen() { - return node.next + return node.next, nil } splitNode := node.split(offset) s.treeByIndex.UpdateWeight(splitNode.indexNode) - s.InsertAfter(node, splitNode) + _, err := s.InsertAfter(node, splitNode) + if err != nil { + return nil, fmt.Errorf("splitNode: %w", err) + } insNext := node.insNext if insNext != nil { @@ -383,21 +406,24 @@ func (s *RGATreeSplit[V]) splitNode(node *RGATreeSplitNode[V], offset int) *RGAT } splitNode.SetInsPrev(node) - return splitNode + return splitNode, nil } // InsertAfter inserts the given node after the given previous node. -func (s *RGATreeSplit[V]) InsertAfter(prev, node *RGATreeSplitNode[V]) *RGATreeSplitNode[V] { +func (s *RGATreeSplit[V]) InsertAfter(prev, node *RGATreeSplitNode[V]) (*RGATreeSplitNode[V], error) { next := prev.next node.setPrev(prev) if next != nil { next.setPrev(node) } - s.treeByID.Put(node.id, node) + _, err := s.treeByID.Put(node.id, node) + if err != nil { + return nil, fmt.Errorf("rga tree split InsertAfter: %w", err) + } s.treeByIndex.InsertAfter(prev.indexNode, node.indexNode) - return node + return node, nil } // InitialHead returns the head node of this RGATreeSplit. @@ -406,12 +432,17 @@ func (s *RGATreeSplit[V]) InitialHead() *RGATreeSplitNode[V] { } // FindNode returns the node of the given ID. -func (s *RGATreeSplit[V]) FindNode(id *RGATreeSplitNodeID) *RGATreeSplitNode[V] { +func (s *RGATreeSplit[V]) FindNode(id *RGATreeSplitNodeID) (*RGATreeSplitNode[V], error) { if id == nil { - return nil + return nil, nil + } + + node, err := s.findFloorNode(id) + if err != nil { + return nil, fmt.Errorf("rga tree split FindNode: %w", err) } - return s.findFloorNode(id) + return node, nil } // CheckWeight returns false when there is an incorrect weight node. @@ -420,17 +451,24 @@ func (s *RGATreeSplit[V]) CheckWeight() bool { return s.treeByIndex.CheckWeight() } -func (s *RGATreeSplit[V]) findFloorNode(id *RGATreeSplitNodeID) *RGATreeSplitNode[V] { - key, value := s.treeByID.Floor(id) +func (s *RGATreeSplit[V]) findFloorNode(id *RGATreeSplitNodeID) (*RGATreeSplitNode[V], error) { + key, value, err := s.treeByID.Floor(id) + if err != nil { + return nil, fmt.Errorf("rga tree split findFloorNode: %w", err) + } if key == nil { - return nil + return nil, nil } - if !key.Equal(id) && !key.hasSameCreatedAt(id) { - return nil + result, err := key.Equal(id) + if err != nil { + return nil, fmt.Errorf("rga tree split findFloorNode: %w", err) + } + if !result && !key.hasSameCreatedAt(id) { + return nil, nil } - return value + return value, nil } func (s *RGATreeSplit[V]) edit( @@ -439,10 +477,16 @@ func (s *RGATreeSplit[V]) edit( latestCreatedAtMapByActor map[string]*time.Ticket, content V, editedAt *time.Ticket, -) (*RGATreeSplitNodePos, map[string]*time.Ticket) { +) (*RGATreeSplitNodePos, map[string]*time.Ticket, error) { // 01. Split nodes with from and to - toLeft, toRight := s.findNodeWithSplit(to, editedAt) - fromLeft, fromRight := s.findNodeWithSplit(from, editedAt) + toLeft, toRight, err := s.findNodeWithSplit(to, editedAt) + if err != nil { + return nil, nil, err + } + fromLeft, fromRight, err := s.findNodeWithSplit(from, editedAt) + if err != nil { + return nil, nil, err + } // 02. delete between from and to nodesToDelete := s.findBetween(fromRight, toRight) @@ -458,7 +502,10 @@ func (s *RGATreeSplit[V]) edit( // 03. insert a new node if content.Len() > 0 { - inserted := s.InsertAfter(fromLeft, NewRGATreeSplitNode(NewRGATreeSplitNodeID(editedAt, 0), content)) + inserted, err := s.InsertAfter(fromLeft, NewRGATreeSplitNode(NewRGATreeSplitNodeID(editedAt, 0), content)) + if err != nil { + return nil, nil, err + } caretPos = NewRGATreeSplitNodePos(inserted.id, inserted.contentLen()) } @@ -467,7 +514,7 @@ func (s *RGATreeSplit[V]) edit( s.removedNodeMap[key] = removedNode } - return caretPos, latestCreatedAtMap + return caretPos, latestCreatedAtMap, nil } func (s *RGATreeSplit[V]) findBetween(from, to *RGATreeSplitNode[V]) []*RGATreeSplitNode[V] { @@ -608,19 +655,21 @@ func (s *RGATreeSplit[V]) removedNodesLen() int { } // purgeTextNodesWithGarbage physically purges nodes that have been removed. -func (s *RGATreeSplit[V]) purgeTextNodesWithGarbage(ticket *time.Ticket) int { +func (s *RGATreeSplit[V]) purgeTextNodesWithGarbage(ticket *time.Ticket) (int, error) { count := 0 for _, node := range s.removedNodeMap { if node.removedAt != nil && ticket.Compare(node.removedAt) >= 0 { s.treeByIndex.Delete(node.indexNode) s.purge(node) - s.treeByID.Remove(node.id) + if err := s.treeByID.Remove(node.id); err != nil { + return 0, fmt.Errorf("rga tree split purgeTextNodesWithGarbage: %w", err) + } delete(s.removedNodeMap, node.id.key()) count++ } } - return count + return count, nil } // purge physically purge the given node from RGATreeSplit. diff --git a/pkg/document/crdt/rga_tree_split_test.go b/pkg/document/crdt/rga_tree_split_test.go index a4832f5bd..54a565080 100644 --- a/pkg/document/crdt/rga_tree_split_test.go +++ b/pkg/document/crdt/rga_tree_split_test.go @@ -12,6 +12,7 @@ import ( func TestRGATreeSplit(t *testing.T) { t.Run("compare nil id panic test", func(t *testing.T) { id := crdt.NewRGATreeSplitNodeID(time.InitialTicket, 0) - assert.Panics(t, func() { id.Compare(nil) }, "ID cannot be null") + _, err := id.Compare(nil) + assert.Errorf(t, err, "ID cannot be null") }) } diff --git a/pkg/document/crdt/root.go b/pkg/document/crdt/root.go index 530c58c62..e2fea02ae 100644 --- a/pkg/document/crdt/root.go +++ b/pkg/document/crdt/root.go @@ -112,7 +112,7 @@ func (r *Root) DeepCopy() (*Root, error) { } // GarbageCollect purge elements that were removed before the given time. -func (r *Root) GarbageCollect(ticket *time.Ticket) int { +func (r *Root) GarbageCollect(ticket *time.Ticket) (int, error) { count := 0 for _, pair := range r.removedElementPairMapByCreatedAt { @@ -123,14 +123,17 @@ func (r *Root) GarbageCollect(ticket *time.Ticket) int { } for _, text := range r.textElementWithGarbageMapByCreatedAt { - purgedTextNodes := text.purgeTextNodesWithGarbage(ticket) + purgedTextNodes, err := text.purgeTextNodesWithGarbage(ticket) + if err != nil { + return 0, fmt.Errorf("crdt root GarbageCollect: %w", err) + } if purgedTextNodes > 0 { delete(r.textElementWithGarbageMapByCreatedAt, text.CreatedAt().Key()) } count += purgedTextNodes } - return count + return count, nil } // ElementMapLen returns the size of element map. diff --git a/pkg/document/crdt/root_test.go b/pkg/document/crdt/root_test.go index f4d72f056..9fa2af0a8 100644 --- a/pkg/document/crdt/root_test.go +++ b/pkg/document/crdt/root_test.go @@ -27,7 +27,8 @@ import ( ) func registerTextElementWithGarbage(fromPos, toPos *crdt.RGATreeSplitNodePos, root *crdt.Root, text crdt.TextElement) { - if !fromPos.Equal(toPos) { + result, _ := fromPos.Equal(toPos) + if !result { root.RegisterTextElementWithGarbage(text) } } @@ -49,35 +50,37 @@ func TestRoot(t *testing.T) { assert.Equal(t, "[0,2]", array.Marshal()) assert.Equal(t, 1, root.GarbageLen()) - assert.Equal(t, 1, root.GarbageCollect(time.MaxTicket)) + gc, _ := root.GarbageCollect(time.MaxTicket) + assert.Equal(t, 1, gc) assert.Equal(t, 0, root.GarbageLen()) }) t.Run("garbage collection for text test", func(t *testing.T) { root := helper.TestRoot() ctx := helper.TextChangeContext(root) - text := crdt.NewText(crdt.NewRGATreeSplit(crdt.InitialTextNode()), ctx.IssueTimeTicket()) + rgaTreeSplit, _ := crdt.NewRGATreeSplit(crdt.InitialTextNode()) + text := crdt.NewText(rgaTreeSplit, ctx.IssueTimeTicket()) fromPos, toPos := text.CreateRange(0, 0) - text.Edit(fromPos, toPos, nil, "Hello World", nil, ctx.IssueTimeTicket()) + _, _, _ = text.Edit(fromPos, toPos, nil, "Hello World", nil, ctx.IssueTimeTicket()) registerTextElementWithGarbage(fromPos, toPos, root, text) assert.Equal(t, "Hello World", text.String()) assert.Equal(t, 0, root.GarbageLen()) fromPos, toPos = text.CreateRange(5, 10) - text.Edit(fromPos, toPos, nil, "Yorkie", nil, ctx.IssueTimeTicket()) + _, _, _ = text.Edit(fromPos, toPos, nil, "Yorkie", nil, ctx.IssueTimeTicket()) registerTextElementWithGarbage(fromPos, toPos, root, text) assert.Equal(t, "HelloYorkied", text.String()) assert.Equal(t, 1, root.GarbageLen()) fromPos, toPos = text.CreateRange(0, 5) - text.Edit(fromPos, toPos, nil, "", nil, ctx.IssueTimeTicket()) + _, _, _ = text.Edit(fromPos, toPos, nil, "", nil, ctx.IssueTimeTicket()) registerTextElementWithGarbage(fromPos, toPos, root, text) assert.Equal(t, "Yorkied", text.String()) assert.Equal(t, 2, root.GarbageLen()) fromPos, toPos = text.CreateRange(6, 7) - text.Edit(fromPos, toPos, nil, "", nil, ctx.IssueTimeTicket()) + _, _, _ = text.Edit(fromPos, toPos, nil, "", nil, ctx.IssueTimeTicket()) registerTextElementWithGarbage(fromPos, toPos, root, text) assert.Equal(t, "Yorkie", text.String()) assert.Equal(t, 3, root.GarbageLen()) @@ -87,7 +90,8 @@ func TestRoot(t *testing.T) { nodeLen := len(text.Nodes()) assert.Equal(t, 4, nodeLen) - assert.Equal(t, 3, root.GarbageCollect(time.MaxTicket)) + gc, _ := root.GarbageCollect(time.MaxTicket) + assert.Equal(t, 3, gc) assert.Equal(t, 0, root.GarbageLen()) nodeLen = len(text.Nodes()) assert.Equal(t, 1, nodeLen) @@ -104,7 +108,8 @@ func TestRoot(t *testing.T) { root := helper.TestRoot() ctx := helper.TextChangeContext(root) - text := crdt.NewText(crdt.NewRGATreeSplit(crdt.InitialTextNode()), ctx.IssueTimeTicket()) + rgaTreeSplit, _ := crdt.NewRGATreeSplit(crdt.InitialTextNode()) + text := crdt.NewText(rgaTreeSplit, ctx.IssueTimeTicket()) tests := []test{ {from: 0, to: 0, content: "Yorkie", want: "Yorkie", garbage: 0}, @@ -115,35 +120,37 @@ func TestRoot(t *testing.T) { for _, tc := range tests { fromPos, toPos := text.CreateRange(tc.from, tc.to) - text.Edit(fromPos, toPos, nil, tc.content, nil, ctx.IssueTimeTicket()) + _, _, _ = text.Edit(fromPos, toPos, nil, tc.content, nil, ctx.IssueTimeTicket()) registerTextElementWithGarbage(fromPos, toPos, root, text) assert.Equal(t, tc.want, text.String()) assert.Equal(t, tc.garbage, root.GarbageLen()) } - assert.Equal(t, 3, root.GarbageCollect(time.MaxTicket)) + gc, _ := root.GarbageCollect(time.MaxTicket) + assert.Equal(t, 3, gc) assert.Equal(t, 0, root.GarbageLen()) }) t.Run("garbage collection for rich text test", func(t *testing.T) { root := helper.TestRoot() ctx := helper.TextChangeContext(root) - text := crdt.NewText(crdt.NewRGATreeSplit(crdt.InitialTextNode()), ctx.IssueTimeTicket()) + rgaTreeSplit, _ := crdt.NewRGATreeSplit(crdt.InitialTextNode()) + text := crdt.NewText(rgaTreeSplit, ctx.IssueTimeTicket()) fromPos, toPos := text.CreateRange(0, 0) - text.Edit(fromPos, toPos, nil, "Hello World", nil, ctx.IssueTimeTicket()) + _, _, _ = text.Edit(fromPos, toPos, nil, "Hello World", nil, ctx.IssueTimeTicket()) registerTextElementWithGarbage(fromPos, toPos, root, text) assert.Equal(t, `[{"val":"Hello World"}]`, text.Marshal()) assert.Equal(t, 0, root.GarbageLen()) fromPos, toPos = text.CreateRange(6, 11) - text.Edit(fromPos, toPos, nil, "Yorkie", nil, ctx.IssueTimeTicket()) + _, _, _ = text.Edit(fromPos, toPos, nil, "Yorkie", nil, ctx.IssueTimeTicket()) registerTextElementWithGarbage(fromPos, toPos, root, text) assert.Equal(t, `[{"val":"Hello "},{"val":"Yorkie"}]`, text.Marshal()) assert.Equal(t, 1, root.GarbageLen()) fromPos, toPos = text.CreateRange(0, 6) - text.Edit(fromPos, toPos, nil, "", nil, ctx.IssueTimeTicket()) + _, _, _ = text.Edit(fromPos, toPos, nil, "", nil, ctx.IssueTimeTicket()) registerTextElementWithGarbage(fromPos, toPos, root, text) assert.Equal(t, `[{"val":"Yorkie"}]`, text.Marshal()) assert.Equal(t, 2, root.GarbageLen()) @@ -153,7 +160,8 @@ func TestRoot(t *testing.T) { nodeLen := len(text.Nodes()) assert.Equal(t, 3, nodeLen) - assert.Equal(t, 2, root.GarbageCollect(time.MaxTicket)) + gc, _ := root.GarbageCollect(time.MaxTicket) + assert.Equal(t, 2, gc) assert.Equal(t, 0, root.GarbageLen()) nodeLen = len(text.Nodes()) @@ -179,7 +187,8 @@ func TestRoot(t *testing.T) { assert.Equal(t, `{"1":1,"3":3}`, obj.Marshal()) assert.Equal(t, 4, root.GarbageLen()) - assert.Equal(t, 4, root.GarbageCollect(time.MaxTicket)) + gc, _ := root.GarbageCollect(time.MaxTicket) + assert.Equal(t, 4, gc) assert.Equal(t, 0, root.GarbageLen()) deleted = obj.Delete("3", ctx.IssueTimeTicket()) @@ -187,7 +196,8 @@ func TestRoot(t *testing.T) { assert.Equal(t, `{"1":1}`, obj.Marshal()) assert.Equal(t, 1, root.GarbageLen()) - assert.Equal(t, 1, root.GarbageCollect(time.MaxTicket)) + gc, _ = root.GarbageCollect(time.MaxTicket) + assert.Equal(t, 1, gc) assert.Equal(t, 0, root.GarbageLen()) }) } diff --git a/pkg/document/crdt/text.go b/pkg/document/crdt/text.go index 657255d4a..402c678c8 100644 --- a/pkg/document/crdt/text.go +++ b/pkg/document/crdt/text.go @@ -167,14 +167,23 @@ func (t *Text) Marshal() string { // DeepCopy copies itself deeply. func (t *Text) DeepCopy() (Element, error) { - rgaTreeSplit := NewRGATreeSplit(InitialTextNode()) + rgaTreeSplit, err := NewRGATreeSplit(InitialTextNode()) + if err != nil { + return nil, fmt.Errorf("crdt text DeepCopy: %s", err) + } current := rgaTreeSplit.InitialHead() for _, node := range t.Nodes() { - current = rgaTreeSplit.InsertAfter(current, node.DeepCopy()) + current, err := rgaTreeSplit.InsertAfter(current, node.DeepCopy()) + if err != nil { + return nil, fmt.Errorf("crdt text DeepCopy: %s", err) + } insPrevID := node.InsPrevID() if insPrevID != nil { - insPrevNode := rgaTreeSplit.FindNode(insPrevID) + insPrevNode, err := rgaTreeSplit.FindNode(insPrevID) + if err != nil { + return nil, fmt.Errorf("crdt text DeepCopy: %s", err) + } if insPrevNode == nil { return nil, fmt.Errorf("crdt text DeepCopy: insPrevNode should be presence") } @@ -233,21 +242,24 @@ func (t *Text) Edit( content string, attributes map[string]string, executedAt *time.Ticket, -) (*RGATreeSplitNodePos, map[string]*time.Ticket) { +) (*RGATreeSplitNodePos, map[string]*time.Ticket, error) { val := NewTextValue(content, NewRHT()) for key, value := range attributes { val.attrs.Set(key, value, executedAt) } - cursorPos, latestCreatedAtMapByActor := t.rgaTreeSplit.edit( + cursorPos, latestCreatedAtMapByActor, err := t.rgaTreeSplit.edit( from, to, latestCreatedAtMapByActor, val, executedAt, ) + if err != nil { + return nil, nil, fmt.Errorf("crdt text Edit: %s", err) + } - return cursorPos, latestCreatedAtMapByActor + return cursorPos, latestCreatedAtMapByActor, nil } // Style applies the given attributes of the given range. @@ -256,10 +268,16 @@ func (t *Text) Style( to *RGATreeSplitNodePos, attributes map[string]string, executedAt *time.Ticket, -) { +) error { // 01. Split nodes with from and to - _, toRight := t.rgaTreeSplit.findNodeWithSplit(to, executedAt) - _, fromRight := t.rgaTreeSplit.findNodeWithSplit(from, executedAt) + _, toRight, err := t.rgaTreeSplit.findNodeWithSplit(to, executedAt) + if err != nil { + return fmt.Errorf("crdt text Style: %s", err) + } + _, fromRight, err := t.rgaTreeSplit.findNodeWithSplit(from, executedAt) + if err != nil { + return fmt.Errorf("crdt text Style: %s", err) + } // 02. style nodes between from and to nodes := t.rgaTreeSplit.findBetween(fromRight, toRight) @@ -269,6 +287,7 @@ func (t *Text) Style( val.attrs.Set(key, value, executedAt) } } + return nil } // Select stores that the given range has been selected. @@ -305,6 +324,6 @@ func (t *Text) removedNodesLen() int { } // purgeTextNodesWithGarbage physically purges nodes that have been removed. -func (t *Text) purgeTextNodesWithGarbage(ticket *time.Ticket) int { +func (t *Text) purgeTextNodesWithGarbage(ticket *time.Ticket) (int, error) { return t.rgaTreeSplit.purgeTextNodesWithGarbage(ticket) } diff --git a/pkg/document/crdt/text_test.go b/pkg/document/crdt/text_test.go index 04fa873c9..1023041f9 100644 --- a/pkg/document/crdt/text_test.go +++ b/pkg/document/crdt/text_test.go @@ -29,14 +29,15 @@ func TestText(t *testing.T) { t.Run("marshal test", func(t *testing.T) { root := helper.TestRoot() ctx := helper.TextChangeContext(root) - text := crdt.NewText(crdt.NewRGATreeSplit(crdt.InitialTextNode()), ctx.IssueTimeTicket()) + rgaTreeSplit, _ := crdt.NewRGATreeSplit(crdt.InitialTextNode()) + text := crdt.NewText(rgaTreeSplit, ctx.IssueTimeTicket()) fromPos, toPos := text.CreateRange(0, 0) - text.Edit(fromPos, toPos, nil, "Hello World", nil, ctx.IssueTimeTicket()) + _, _, _ = text.Edit(fromPos, toPos, nil, "Hello World", nil, ctx.IssueTimeTicket()) assert.Equal(t, `[{"val":"Hello World"}]`, text.Marshal()) fromPos, toPos = text.CreateRange(6, 11) - text.Edit(fromPos, toPos, nil, "Yorkie", nil, ctx.IssueTimeTicket()) + _, _, _ = text.Edit(fromPos, toPos, nil, "Yorkie", nil, ctx.IssueTimeTicket()) assert.Equal(t, `[{"val":"Hello "},{"val":"Yorkie"}]`, text.Marshal()) }) @@ -65,19 +66,19 @@ func TestText(t *testing.T) { t.Run("marshal test", func(t *testing.T) { root := helper.TestRoot() ctx := helper.TextChangeContext(root) - - text := crdt.NewText(crdt.NewRGATreeSplit(crdt.InitialTextNode()), ctx.IssueTimeTicket()) + rgaTreeSplit, _ := crdt.NewRGATreeSplit(crdt.InitialTextNode()) + text := crdt.NewText(rgaTreeSplit, ctx.IssueTimeTicket()) fromPos, toPos := text.CreateRange(0, 0) - text.Edit(fromPos, toPos, nil, "Hello World", nil, ctx.IssueTimeTicket()) + _, _, _ = text.Edit(fromPos, toPos, nil, "Hello World", nil, ctx.IssueTimeTicket()) assert.Equal(t, `[{"val":"Hello World"}]`, text.Marshal()) fromPos, toPos = text.CreateRange(6, 11) - text.Edit(fromPos, toPos, nil, "Yorkie", nil, ctx.IssueTimeTicket()) + _, _, _ = text.Edit(fromPos, toPos, nil, "Yorkie", nil, ctx.IssueTimeTicket()) assert.Equal(t, `[{"val":"Hello "},{"val":"Yorkie"}]`, text.Marshal()) fromPos, toPos = text.CreateRange(0, 1) - text.Style(fromPos, toPos, map[string]string{"b": "1"}, ctx.IssueTimeTicket()) + _ = text.Style(fromPos, toPos, map[string]string{"b": "1"}, ctx.IssueTimeTicket()) assert.Equal( t, `[{"attrs":{"b":"1"},"val":"H"},{"val":"ello "},{"val":"Yorkie"}]`, diff --git a/pkg/document/document.go b/pkg/document/document.go index 5b16c102d..997a064af 100644 --- a/pkg/document/document.go +++ b/pkg/document/document.go @@ -121,7 +121,7 @@ func (d *Document) ApplyChangePack(pack *change.Pack) error { d.doc.checkpoint = d.doc.checkpoint.Forward(pack.Checkpoint) // 04. Do Garbage collection. - d.GarbageCollect(pack.MinSyncedTicket) + _, _ = d.GarbageCollect(pack.MinSyncedTicket) // 05. Update the status. if pack.IsRemoved { @@ -201,11 +201,19 @@ func (d *Document) Root() *json.Object { } // GarbageCollect purge elements that were removed before the given time. -func (d *Document) GarbageCollect(ticket *time.Ticket) int { +func (d *Document) GarbageCollect(ticket *time.Ticket) (int, error) { if d.clone != nil { - d.clone.GarbageCollect(ticket) + _, err := d.clone.GarbageCollect(ticket) + if err != nil { + return 0, fmt.Errorf("document GarbageCollect: %w", err) + } + } + + gc, err := d.doc.GarbageCollect(ticket) + if err != nil { + return 0, fmt.Errorf("document GarbageCollect: %w", err) } - return d.doc.GarbageCollect(ticket) + return gc, nil } // GarbageLen returns the count of removed elements. diff --git a/pkg/document/internal_document.go b/pkg/document/internal_document.go index 1a0b7ca83..77b1133ae 100644 --- a/pkg/document/internal_document.go +++ b/pkg/document/internal_document.go @@ -133,14 +133,17 @@ func (d *InternalDocument) ApplyChangePack(pack *change.Pack) error { d.checkpoint = d.checkpoint.Forward(pack.Checkpoint) if pack.MinSyncedTicket != nil { - d.GarbageCollect(pack.MinSyncedTicket) + _, err := d.GarbageCollect(pack.MinSyncedTicket) + if err != nil { + return err + } } return nil } // GarbageCollect purge elements that were removed before the given time. -func (d *InternalDocument) GarbageCollect(ticket *time.Ticket) int { +func (d *InternalDocument) GarbageCollect(ticket *time.Ticket) (int, error) { return d.root.GarbageCollect(ticket) } diff --git a/pkg/document/json/object.go b/pkg/document/json/object.go index 55186227f..f9e3b283f 100644 --- a/pkg/document/json/object.go +++ b/pkg/document/json/object.go @@ -62,9 +62,13 @@ func (p *Object) SetNewArray(k string) *Array { // SetNewText sets a new Text for the given key. func (p *Object) SetNewText(k string) *Text { v := p.setInternal(k, func(ticket *time.Ticket) crdt.Element { + rgaTreeSplit, err := crdt.NewRGATreeSplit(crdt.InitialTextNode()) + if err != nil { + panic(fmt.Sprintf("json object SetNewText err: %s", err)) + } return NewText( p.context, - crdt.NewText(crdt.NewRGATreeSplit(crdt.InitialTextNode()), ticket), + crdt.NewText(rgaTreeSplit, ticket), ) }) diff --git a/pkg/document/json/text.go b/pkg/document/json/text.go index 4b4f656e0..b420af166 100644 --- a/pkg/document/json/text.go +++ b/pkg/document/json/text.go @@ -17,6 +17,8 @@ package json import ( + "fmt" + "github.com/yorkie-team/yorkie/pkg/document/change" "github.com/yorkie-team/yorkie/pkg/document/crdt" "github.com/yorkie-team/yorkie/pkg/document/operations" @@ -40,7 +42,7 @@ func NewText(ctx *change.Context, text *crdt.Text) *Text { // Edit edits the given range with the given content and attributes. func (p *Text) Edit(from, to int, content string, attributes ...map[string]string) *Text { if from > to { - panic("from should be less than or equal to to") + panic("json test Edit: from should be less than or equal to to") } fromPos, toPos := p.Text.CreateRange(from, to) @@ -52,7 +54,7 @@ func (p *Text) Edit(from, to int, content string, attributes ...map[string]strin } ticket := p.context.IssueTimeTicket() - _, maxCreationMapByActor := p.Text.Edit( + _, maxCreationMapByActor, err := p.Text.Edit( fromPos, toPos, nil, @@ -60,6 +62,9 @@ func (p *Text) Edit(from, to int, content string, attributes ...map[string]strin attrs, ticket, ) + if err != nil { + panic(fmt.Sprintf("json text Edit: %s", err.Error())) + } p.context.Push(operations.NewEdit( p.CreatedAt(), @@ -70,7 +75,11 @@ func (p *Text) Edit(from, to int, content string, attributes ...map[string]strin attrs, ticket, )) - if !fromPos.Equal(toPos) { + result, err := fromPos.Equal(toPos) + if err != nil { + panic(fmt.Sprintf("json text Edit: %s", err.Error())) + } + if !result { p.context.RegisterTextElementWithGarbage(p) } @@ -80,12 +89,12 @@ func (p *Text) Edit(from, to int, content string, attributes ...map[string]strin // Style applies the style of the given range. func (p *Text) Style(from, to int, attributes map[string]string) *Text { if from > to { - panic("from should be less than or equal to to") + panic("json test Style: from should be less than or equal to to") } fromPos, toPos := p.Text.CreateRange(from, to) ticket := p.context.IssueTimeTicket() - p.Text.Style( + _ = p.Text.Style( fromPos, toPos, attributes, @@ -106,7 +115,7 @@ func (p *Text) Style(from, to int, attributes map[string]string) *Text { // Select stores that the given range has been selected. func (p *Text) Select(from, to int) *Text { if from > to { - panic("from should be less than or equal to to") + panic("json text Select: from should be less than or equal to to") } fromPos, toPos := p.Text.CreateRange(from, to) diff --git a/pkg/document/operations/edit.go b/pkg/document/operations/edit.go index 98cab3ed5..f58e6d348 100644 --- a/pkg/document/operations/edit.go +++ b/pkg/document/operations/edit.go @@ -17,6 +17,8 @@ package operations import ( + "fmt" + "github.com/yorkie-team/yorkie/pkg/document/crdt" "github.com/yorkie-team/yorkie/pkg/document/time" ) @@ -75,12 +77,16 @@ func (e *Edit) Execute(root *crdt.Root) error { switch obj := parent.(type) { case *crdt.Text: - obj.Edit(e.from, e.to, e.latestCreatedAtMapByActor, e.content, e.attributes, e.executedAt) - if !e.from.Equal(e.to) { + _, _, _ = obj.Edit(e.from, e.to, e.latestCreatedAtMapByActor, e.content, e.attributes, e.executedAt) + result, err := e.from.Equal(e.to) + if err != nil { + return fmt.Errorf("edit operations %w", err) + } + if !result { root.RegisterTextElementWithGarbage(obj) } default: - return ErrNotApplicableDataType + return fmt.Errorf("edit operations %w", ErrNotApplicableDataType) } return nil diff --git a/pkg/llrb/llrb.go b/pkg/llrb/llrb.go index 316effac0..4c29492cb 100644 --- a/pkg/llrb/llrb.go +++ b/pkg/llrb/llrb.go @@ -18,6 +18,7 @@ package llrb import ( + "fmt" "strings" ) @@ -27,7 +28,7 @@ type Key interface { // a.Compare(b) = 1 => a > b // a.Compare(b) = 0 => a == b // a.Compare(b) = -1 => a < b - Compare(k Key) int + Compare(k Key) (int, error) } // Value represents the data stored in the nodes of Tree. @@ -72,10 +73,15 @@ func NewTree[K Key, V Value]() *Tree[K, V] { } // Put puts the value of the given key. -func (t *Tree[K, V]) Put(k K, v V) V { - t.root = t.put(t.root, k, v) +func (t *Tree[K, V]) Put(k K, v V) (V, error) { + var zeroV V + var err error + t.root, err = t.put(t.root, k, v) + if err != nil { + return zeroV, fmt.Errorf("llrb Put: %w", err) + } t.root.isRed = false - return v + return v, nil } func (t *Tree[K, V]) String() string { @@ -87,31 +93,42 @@ func (t *Tree[K, V]) String() string { } // Remove removes the value of the given key. -func (t *Tree[K, V]) Remove(key K) { +func (t *Tree[K, V]) Remove(key K) error { if !isRed(t.root.left) && !isRed(t.root.right) { t.root.isRed = true } - t.root = t.remove(t.root, key) + var err error + t.root, err = t.remove(t.root, key) + if err != nil { + return fmt.Errorf("llrb Remove: %w", err) + } if t.root != nil { t.root.isRed = false } + + return nil } // Floor returns the greatest key less than or equal to the given key. -func (t *Tree[K, V]) Floor(key K) (K, V) { +func (t *Tree[K, V]) Floor(key K) (K, V, error) { + var zeroK K + var zeroV V node := t.root for node != nil { - compare := key.Compare(node.key) - if compare > 0 { + keyCompare, err := key.Compare(node.key) + if err != nil { + return zeroK, zeroV, fmt.Errorf("llrb Floor: %w", err) + } + if keyCompare > 0 { if node.right != nil { node.right.parent = node node = node.right } else { - return node.key, node.value + return node.key, node.value, nil } - } else if compare < 0 { + } else if keyCompare < 0 { if node.left != nil { node.left.parent = node node = node.left @@ -124,29 +141,36 @@ func (t *Tree[K, V]) Floor(key K) (K, V) { } // TODO(hackerwins): check below warning - return parent.key, parent.value + return parent.key, parent.value, nil } } else { - return node.key, node.value + return node.key, node.value, nil } } - var zeroK K - var zeroV V - return zeroK, zeroV + return zeroK, zeroV, nil } -func (t *Tree[K, V]) put(node *Node[K, V], key K, value V) *Node[K, V] { +func (t *Tree[K, V]) put(node *Node[K, V], key K, value V) (*Node[K, V], error) { if node == nil { t.size++ - return NewNode(key, value, true) + return NewNode(key, value, true), nil } - compare := key.Compare(node.key) - if compare < 0 { - node.left = t.put(node.left, key, value) - } else if compare > 0 { - node.right = t.put(node.right, key, value) + keyCompare, err := key.Compare(node.key) + if err != nil { + return nil, fmt.Errorf("llrb put: %w", err) + } + if keyCompare < 0 { + node.left, err = t.put(node.left, key, value) + if err != nil { + return nil, fmt.Errorf("llrb put: %w", err) + } + } else if keyCompare > 0 { + node.right, err = t.put(node.right, key, value) + if err != nil { + return nil, fmt.Errorf("llrb put: %w", err) + } } else { node.value = value } @@ -163,41 +187,59 @@ func (t *Tree[K, V]) put(node *Node[K, V], key K, value V) *Node[K, V] { flipColors(node) } - return node + return node, nil } -func (t *Tree[K, V]) remove(node *Node[K, V], key K) *Node[K, V] { - if key.Compare(node.key) < 0 { +func (t *Tree[K, V]) remove(node *Node[K, V], key K) (*Node[K, V], error) { + keyCompare, err := key.Compare(node.key) + if err != nil { + return nil, fmt.Errorf("llrb remove: %w", err) + } + if keyCompare < 0 { if !isRed(node.left) && !isRed(node.left.left) { node = moveRedLeft(node) } - node.left = t.remove(node.left, key) + node.left, err = t.remove(node.left, key) + if err != nil { + return nil, fmt.Errorf("llrb remove: %w", err) + } } else { if isRed(node.left) { node = rotateRight(node) } - if key.Compare(node.key) == 0 && node.right == nil { + keyCompare, err := key.Compare(node.key) + if err != nil { + return nil, fmt.Errorf("llrb remove: %w", err) + } + if keyCompare == 0 && node.right == nil { t.size-- - return nil + return nil, nil } if !isRed(node.right) && !isRed(node.right.left) { node = moveRedRight(node) } - if key.Compare(node.key) == 0 { + keyCompare, err = key.Compare(node.key) + if err != nil { + return nil, fmt.Errorf("llrb remove: %w", err) + } + if keyCompare == 0 { t.size-- smallest := min(node.right) node.value = smallest.value node.key = smallest.key node.right = removeMin(node.right) } else { - node.right = t.remove(node.right, key) + node.right, err = t.remove(node.right, key) + if err != nil { + return nil, fmt.Errorf("llrb remove: %w", err) + } } } - return fixUp(node) + return fixUp(node), nil } func rotateLeft[K Key, V Value](node *Node[K, V]) *Node[K, V] { diff --git a/pkg/llrb/llrb_test.go b/pkg/llrb/llrb_test.go index 9003ac1e6..8cda45748 100644 --- a/pkg/llrb/llrb_test.go +++ b/pkg/llrb/llrb_test.go @@ -35,14 +35,14 @@ func newIntKey(key int) *intKey { } } -func (k *intKey) Compare(other llrb.Key) int { +func (k *intKey) Compare(other llrb.Key) (int, error) { o := other.(*intKey) if k.key > o.key { - return 1 + return 1, nil } else if k.key < o.key { - return -1 + return -1, nil } else { - return 0 + return 0, nil } } @@ -73,17 +73,17 @@ func TestTree(t *testing.T) { for _, array := range arrays { tree := llrb.NewTree[*intKey, *intValue]() for _, value := range array { - tree.Put(newIntKey(value), newIntValue(value)) + _, _ = tree.Put(newIntKey(value), newIntValue(value)) } assert.Equal(t, "0,1,2,3,4,5,6,7,8,9", tree.String()) - tree.Remove(newIntKey(8)) + _ = tree.Remove(newIntKey(8)) assert.Equal(t, "0,1,2,3,4,5,6,7,9", tree.String()) - tree.Remove(newIntKey(2)) + _ = tree.Remove(newIntKey(2)) assert.Equal(t, "0,1,3,4,5,6,7,9", tree.String()) - tree.Remove(newIntKey(5)) + _ = tree.Remove(newIntKey(5)) assert.Equal(t, "0,1,3,4,6,7,9", tree.String()) } }) From 5b1460e5ba88efc0d18efd362ec90f314ae38707 Mon Sep 17 00:00:00 2001 From: emplam27 Date: Tue, 25 Apr 2023 19:07:55 +0900 Subject: [PATCH 3/6] Remove panic method in crdt rga_tree_split --- pkg/document/document_test.go | 4 ++-- pkg/document/operations/style.go | 2 +- test/bench/document_bench_test.go | 9 ++++++--- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/pkg/document/document_test.go b/pkg/document/document_test.go index b4a221a31..2a54dc444 100644 --- a/pkg/document/document_test.go +++ b/pkg/document/document_test.go @@ -486,7 +486,7 @@ func TestDocument(t *testing.T) { ) assert.Equal(t, 1, doc.GarbageLen()) - doc.GarbageCollect(time.MaxTicket) + _, _ = doc.GarbageCollect(time.MaxTicket) assert.Equal(t, 0, doc.GarbageLen()) assert.Equal( t, @@ -519,7 +519,7 @@ func TestDocument(t *testing.T) { assert.Equal(t, "{}", doc.Marshal()) assert.Equal(t, 2, doc.GarbageLen()) - doc.GarbageCollect(time.MaxTicket) + _, _ = doc.GarbageCollect(time.MaxTicket) assert.Equal(t, "{}", doc.Marshal()) assert.Equal(t, 0, doc.GarbageLen()) }) diff --git a/pkg/document/operations/style.go b/pkg/document/operations/style.go index 46484c0f4..bc5350810 100644 --- a/pkg/document/operations/style.go +++ b/pkg/document/operations/style.go @@ -64,7 +64,7 @@ func (e *Style) Execute(root *crdt.Root) error { return ErrNotApplicableDataType } - obj.Style(e.from, e.to, e.attributes, e.executedAt) + _ = obj.Style(e.from, e.to, e.attributes, e.executedAt) return nil } diff --git a/test/bench/document_bench_test.go b/test/bench/document_bench_test.go index d71f28895..377f7aaaf 100644 --- a/test/bench/document_bench_test.go +++ b/test/bench/document_bench_test.go @@ -536,7 +536,8 @@ func benchmarkTextEditGC(cnt int, b *testing.B) { }, "replace contents with b") assert.NoError(b, err) assert.Equal(b, cnt, doc.GarbageLen()) - assert.Equal(b, cnt, doc.GarbageCollect(time.MaxTicket)) + gc, _ := doc.GarbageCollect(time.MaxTicket) + assert.Equal(b, cnt, gc) } } @@ -568,7 +569,8 @@ func benchmarkTextSplitGC(cnt int, b *testing.B) { assert.NoError(b, err) assert.Equal(b, cnt, doc.GarbageLen()) - assert.Equal(b, cnt, doc.GarbageCollect(time.MaxTicket)) + gc, _ := doc.GarbageCollect(time.MaxTicket) + assert.Equal(b, cnt, gc) } } @@ -606,7 +608,8 @@ func benchmarkArrayGC(cnt int, b *testing.B) { }, "deletes the array") assert.NoError(b, err) - assert.Equal(b, cnt+1, doc.GarbageCollect(time.MaxTicket)) + gc, _ := doc.GarbageCollect(time.MaxTicket) + assert.Equal(b, cnt+1, gc) assert.NoError(b, err) } } From adb262c5c9c206e9c4e9a0088eb9d517c4096c77 Mon Sep 17 00:00:00 2001 From: emplam27 Date: Wed, 26 Apr 2023 17:37:14 +0900 Subject: [PATCH 4/6] test error assertion & rollback formatting errors --- api/converter/from_bytes.go | 18 ++++----- pkg/document/crdt/array.go | 4 +- pkg/document/crdt/object.go | 4 +- pkg/document/crdt/primitive_test.go | 3 +- pkg/document/crdt/rga_tree_split.go | 28 +++++++------- pkg/document/crdt/rga_tree_split_test.go | 2 +- pkg/document/crdt/root.go | 6 +-- pkg/document/crdt/root_test.go | 48 ++++++++++++++++-------- pkg/document/crdt/text.go | 14 +++---- pkg/document/crdt/text_test.go | 21 +++++++---- pkg/document/document.go | 25 ++++++++---- pkg/document/document_test.go | 6 ++- pkg/document/json/array.go | 3 +- pkg/document/json/object.go | 5 +-- pkg/document/json/text.go | 12 +++--- pkg/document/operations/add.go | 4 +- pkg/document/operations/edit.go | 6 +-- pkg/document/operations/set.go | 4 +- pkg/document/operations/style.go | 4 +- pkg/llrb/llrb.go | 23 ++++++------ pkg/llrb/llrb_test.go | 12 ++++-- server/rpc/auth/webhook.go | 2 +- test/bench/document_bench_test.go | 9 +++-- 23 files changed, 145 insertions(+), 118 deletions(-) diff --git a/api/converter/from_bytes.go b/api/converter/from_bytes.go index 8e71bf90b..12ab26a71 100644 --- a/api/converter/from_bytes.go +++ b/api/converter/from_bytes.go @@ -163,43 +163,43 @@ func fromJSONText( ) (*crdt.Text, error) { createdAt, err := fromTimeTicket(pbText.CreatedAt) if err != nil { - return nil, fmt.Errorf("from bytes fromJsonText: %w", err) + return nil, err } movedAt, err := fromTimeTicket(pbText.MovedAt) if err != nil { - return nil, fmt.Errorf("from bytes fromJsonText: %w", err) + return nil, err } removedAt, err := fromTimeTicket(pbText.RemovedAt) if err != nil { - return nil, fmt.Errorf("from bytes fromJsonText: %w", err) + return nil, err } rgaTreeSplit, err := crdt.NewRGATreeSplit(crdt.InitialTextNode()) if err != nil { - return nil, fmt.Errorf("from bytes fromJsonText: %w", err) + return nil, err } current := rgaTreeSplit.InitialHead() for _, pbNode := range pbText.Nodes { textNode, err := fromTextNode(pbNode) if err != nil { - return nil, fmt.Errorf("from bytes fromJsonText: %w", err) + return nil, err } current, err = rgaTreeSplit.InsertAfter(current, textNode) if err != nil { - return nil, fmt.Errorf("from bytes fromJsonText: %w", err) + return nil, err } insPrevID, err := fromTextNodeID(pbNode.InsPrevId) if err != nil { - return nil, fmt.Errorf("from bytes fromJsonText: %w", err) + return nil, err } if insPrevID != nil { insPrevNode, err := rgaTreeSplit.FindNode(insPrevID) if err != nil { - return nil, fmt.Errorf("from bytes fromJsonText: %w", err) + return nil, err } if insPrevNode == nil { - return nil, fmt.Errorf("from bytes fromJsonText: insPrevNode should be presence") + return nil, fmt.Errorf("insPrevNode should be presence") } current.SetInsPrev(insPrevNode) } diff --git a/pkg/document/crdt/array.go b/pkg/document/crdt/array.go index aa7d293f6..e6f5cb918 100644 --- a/pkg/document/crdt/array.go +++ b/pkg/document/crdt/array.go @@ -17,8 +17,6 @@ package crdt import ( - "fmt" - "github.com/yorkie-team/yorkie/pkg/document/time" ) @@ -103,7 +101,7 @@ func (a *Array) DeepCopy() (Element, error) { for _, node := range a.elements.Nodes() { copiedNode, err := node.elem.DeepCopy() if err != nil { - return nil, fmt.Errorf("crdt array DeepCopy: %w", err) + return nil, err } elements.Add(copiedNode) } diff --git a/pkg/document/crdt/object.go b/pkg/document/crdt/object.go index 4522cc7f4..2793261da 100644 --- a/pkg/document/crdt/object.go +++ b/pkg/document/crdt/object.go @@ -17,8 +17,6 @@ package crdt import ( - "fmt" - "github.com/yorkie-team/yorkie/pkg/document/time" ) @@ -102,7 +100,7 @@ func (o *Object) DeepCopy() (Element, error) { for _, node := range o.memberNodes.Nodes() { copiedNode, err := node.elem.DeepCopy() if err != nil { - return nil, fmt.Errorf("crdt object DeepCopy: %w", err) + return nil, err } members.Set(node.key, copiedNode) } diff --git a/pkg/document/crdt/primitive_test.go b/pkg/document/crdt/primitive_test.go index 8d4d7d2d5..b209875a2 100644 --- a/pkg/document/crdt/primitive_test.go +++ b/pkg/document/crdt/primitive_test.go @@ -53,7 +53,8 @@ func TestPrimitive(t *testing.T) { assert.Equal(t, prim.Value(), crdt.ValueFromBytes(prim.ValueType(), prim.Bytes())) assert.Equal(t, prim.Marshal(), test.marshal) - copied, _ := prim.DeepCopy() + copied, err := prim.DeepCopy() + assert.NoError(t, err) assert.Equal(t, prim.CreatedAt(), copied.CreatedAt()) assert.Equal(t, prim.MovedAt(), copied.MovedAt()) assert.Equal(t, prim.Marshal(), copied.Marshal()) diff --git a/pkg/document/crdt/rga_tree_split.go b/pkg/document/crdt/rga_tree_split.go index d10f4ae57..a1bc38b0a 100644 --- a/pkg/document/crdt/rga_tree_split.go +++ b/pkg/document/crdt/rga_tree_split.go @@ -45,7 +45,7 @@ func NewRGATreeSplitNodeID(createdAt *time.Ticket, offset int) *RGATreeSplitNode // id==other, -1 if id < other, and +1 if id > other. func (id *RGATreeSplitNodeID) Compare(other llrb.Key) (int, error) { if id == nil || other == nil { - return 0, fmt.Errorf("rga tree split Compare: RGATreeSplitNodeID cannot be null") + return 0, fmt.Errorf("RGATreeSplitNodeID cannot be null") } o := other.(*RGATreeSplitNodeID) @@ -67,7 +67,7 @@ func (id *RGATreeSplitNodeID) Compare(other llrb.Key) (int, error) { func (id *RGATreeSplitNodeID) Equal(other llrb.Key) (bool, error) { keyCompare, err := id.Compare(other) if err != nil { - return false, fmt.Errorf("rga tree split Equal: %w", err) + return false, err } return keyCompare == 0, nil } @@ -142,7 +142,7 @@ func (pos *RGATreeSplitNodePos) RelativeOffset() int { func (pos *RGATreeSplitNodePos) Equal(other *RGATreeSplitNodePos) (bool, error) { result, err := pos.id.Equal(other.id) if err != nil { - return false, fmt.Errorf("rga tree split Equal: %w", err) + return false, err } if !result { return false, nil @@ -308,7 +308,7 @@ func NewRGATreeSplit[V RGATreeSplitValue](initialHead *RGATreeSplitNode[V]) (*RG treeByID := llrb.NewTree[*RGATreeSplitNodeID, *RGATreeSplitNode[V]]() _, err := treeByID.Put(initialHead.ID(), initialHead) if err != nil { - return nil, fmt.Errorf("rga tree split NewRGATreeSplit: %w", err) + return nil, err } return &RGATreeSplit[V]{ @@ -344,14 +344,14 @@ func (s *RGATreeSplit[V]) findNodeWithSplit( absoluteID := pos.getAbsoluteID() node, err := s.findFloorNodePreferToLeft(absoluteID) if err != nil { - return nil, nil, fmt.Errorf("findNodeWithSplit: %w", err) + return nil, nil, err } relativeOffset := absoluteID.offset - node.id.offset _, err = s.splitNode(node, relativeOffset) if err != nil { - return nil, nil, fmt.Errorf("findNodeWithSplit: %w", err) + return nil, nil, err } for node.next != nil && node.next.createdAt().After(updatedAt) { @@ -364,11 +364,11 @@ func (s *RGATreeSplit[V]) findNodeWithSplit( func (s *RGATreeSplit[V]) findFloorNodePreferToLeft(id *RGATreeSplitNodeID) (*RGATreeSplitNode[V], error) { node, err := s.findFloorNode(id) if err != nil { - return nil, fmt.Errorf("findFloorNodePreferToLeft: %w", err) + return nil, err } if node == nil { return nil, - fmt.Errorf("findFloorNodePreferToLeft: the node of the given id should be found: " + s.StructureAsString()) + fmt.Errorf("the node of the given id should be found: " + s.StructureAsString()) } if id.offset > 0 && node.id.offset == id.offset { @@ -384,7 +384,7 @@ func (s *RGATreeSplit[V]) findFloorNodePreferToLeft(id *RGATreeSplitNodeID) (*RG func (s *RGATreeSplit[V]) splitNode(node *RGATreeSplitNode[V], offset int) (*RGATreeSplitNode[V], error) { if offset > node.contentLen() { - return nil, fmt.Errorf("splitNode: offset should be less than or equal to length: " + s.StructureAsString()) + return nil, fmt.Errorf("offset should be less than or equal to length: " + s.StructureAsString()) } if offset == 0 { @@ -397,7 +397,7 @@ func (s *RGATreeSplit[V]) splitNode(node *RGATreeSplitNode[V], offset int) (*RGA s.treeByIndex.UpdateWeight(splitNode.indexNode) _, err := s.InsertAfter(node, splitNode) if err != nil { - return nil, fmt.Errorf("splitNode: %w", err) + return nil, err } insNext := node.insNext @@ -419,7 +419,7 @@ func (s *RGATreeSplit[V]) InsertAfter(prev, node *RGATreeSplitNode[V]) (*RGATree _, err := s.treeByID.Put(node.id, node) if err != nil { - return nil, fmt.Errorf("rga tree split InsertAfter: %w", err) + return nil, err } s.treeByIndex.InsertAfter(prev.indexNode, node.indexNode) @@ -439,7 +439,7 @@ func (s *RGATreeSplit[V]) FindNode(id *RGATreeSplitNodeID) (*RGATreeSplitNode[V] node, err := s.findFloorNode(id) if err != nil { - return nil, fmt.Errorf("rga tree split FindNode: %w", err) + return nil, err } return node, nil @@ -454,7 +454,7 @@ func (s *RGATreeSplit[V]) CheckWeight() bool { func (s *RGATreeSplit[V]) findFloorNode(id *RGATreeSplitNodeID) (*RGATreeSplitNode[V], error) { key, value, err := s.treeByID.Floor(id) if err != nil { - return nil, fmt.Errorf("rga tree split findFloorNode: %w", err) + return nil, err } if key == nil { return nil, nil @@ -462,7 +462,7 @@ func (s *RGATreeSplit[V]) findFloorNode(id *RGATreeSplitNodeID) (*RGATreeSplitNo result, err := key.Equal(id) if err != nil { - return nil, fmt.Errorf("rga tree split findFloorNode: %w", err) + return nil, err } if !result && !key.hasSameCreatedAt(id) { return nil, nil diff --git a/pkg/document/crdt/rga_tree_split_test.go b/pkg/document/crdt/rga_tree_split_test.go index 54a565080..32d82ae61 100644 --- a/pkg/document/crdt/rga_tree_split_test.go +++ b/pkg/document/crdt/rga_tree_split_test.go @@ -10,7 +10,7 @@ import ( ) func TestRGATreeSplit(t *testing.T) { - t.Run("compare nil id panic test", func(t *testing.T) { + t.Run("compare nil id error test", func(t *testing.T) { id := crdt.NewRGATreeSplitNodeID(time.InitialTicket, 0) _, err := id.Compare(nil) assert.Errorf(t, err, "ID cannot be null") diff --git a/pkg/document/crdt/root.go b/pkg/document/crdt/root.go index e2fea02ae..75363e59d 100644 --- a/pkg/document/crdt/root.go +++ b/pkg/document/crdt/root.go @@ -20,8 +20,6 @@ package crdt import ( - "fmt" - "github.com/yorkie-team/yorkie/pkg/document/time" ) @@ -106,7 +104,7 @@ func (r *Root) RegisterTextElementWithGarbage(textType TextElement) { func (r *Root) DeepCopy() (*Root, error) { copiedObject, err := r.object.DeepCopy() if err != nil { - return nil, fmt.Errorf("crdt root DeepCopy: %w", err) + return nil, err } return NewRoot(copiedObject.(*Object)), nil } @@ -125,7 +123,7 @@ func (r *Root) GarbageCollect(ticket *time.Ticket) (int, error) { for _, text := range r.textElementWithGarbageMapByCreatedAt { purgedTextNodes, err := text.purgeTextNodesWithGarbage(ticket) if err != nil { - return 0, fmt.Errorf("crdt root GarbageCollect: %w", err) + return 0, err } if purgedTextNodes > 0 { delete(r.textElementWithGarbageMapByCreatedAt, text.CreatedAt().Key()) diff --git a/pkg/document/crdt/root_test.go b/pkg/document/crdt/root_test.go index 9fa2af0a8..e24a8d114 100644 --- a/pkg/document/crdt/root_test.go +++ b/pkg/document/crdt/root_test.go @@ -50,7 +50,8 @@ func TestRoot(t *testing.T) { assert.Equal(t, "[0,2]", array.Marshal()) assert.Equal(t, 1, root.GarbageLen()) - gc, _ := root.GarbageCollect(time.MaxTicket) + gc, err := root.GarbageCollect(time.MaxTicket) + assert.NoError(t, err) assert.Equal(t, 1, gc) assert.Equal(t, 0, root.GarbageLen()) }) @@ -62,25 +63,29 @@ func TestRoot(t *testing.T) { text := crdt.NewText(rgaTreeSplit, ctx.IssueTimeTicket()) fromPos, toPos := text.CreateRange(0, 0) - _, _, _ = text.Edit(fromPos, toPos, nil, "Hello World", nil, ctx.IssueTimeTicket()) + _, _, err := text.Edit(fromPos, toPos, nil, "Hello World", nil, ctx.IssueTimeTicket()) + assert.NoError(t, err) registerTextElementWithGarbage(fromPos, toPos, root, text) assert.Equal(t, "Hello World", text.String()) assert.Equal(t, 0, root.GarbageLen()) fromPos, toPos = text.CreateRange(5, 10) - _, _, _ = text.Edit(fromPos, toPos, nil, "Yorkie", nil, ctx.IssueTimeTicket()) + _, _, err = text.Edit(fromPos, toPos, nil, "Yorkie", nil, ctx.IssueTimeTicket()) + assert.NoError(t, err) registerTextElementWithGarbage(fromPos, toPos, root, text) assert.Equal(t, "HelloYorkied", text.String()) assert.Equal(t, 1, root.GarbageLen()) fromPos, toPos = text.CreateRange(0, 5) - _, _, _ = text.Edit(fromPos, toPos, nil, "", nil, ctx.IssueTimeTicket()) + _, _, err = text.Edit(fromPos, toPos, nil, "", nil, ctx.IssueTimeTicket()) + assert.NoError(t, err) registerTextElementWithGarbage(fromPos, toPos, root, text) assert.Equal(t, "Yorkied", text.String()) assert.Equal(t, 2, root.GarbageLen()) fromPos, toPos = text.CreateRange(6, 7) - _, _, _ = text.Edit(fromPos, toPos, nil, "", nil, ctx.IssueTimeTicket()) + _, _, err = text.Edit(fromPos, toPos, nil, "", nil, ctx.IssueTimeTicket()) + assert.NoError(t, err) registerTextElementWithGarbage(fromPos, toPos, root, text) assert.Equal(t, "Yorkie", text.String()) assert.Equal(t, 3, root.GarbageLen()) @@ -90,7 +95,8 @@ func TestRoot(t *testing.T) { nodeLen := len(text.Nodes()) assert.Equal(t, 4, nodeLen) - gc, _ := root.GarbageCollect(time.MaxTicket) + gc, err := root.GarbageCollect(time.MaxTicket) + assert.NoError(t, err) assert.Equal(t, 3, gc) assert.Equal(t, 0, root.GarbageLen()) nodeLen = len(text.Nodes()) @@ -108,7 +114,8 @@ func TestRoot(t *testing.T) { root := helper.TestRoot() ctx := helper.TextChangeContext(root) - rgaTreeSplit, _ := crdt.NewRGATreeSplit(crdt.InitialTextNode()) + rgaTreeSplit, err := crdt.NewRGATreeSplit(crdt.InitialTextNode()) + assert.NoError(t, err) text := crdt.NewText(rgaTreeSplit, ctx.IssueTimeTicket()) tests := []test{ @@ -120,13 +127,15 @@ func TestRoot(t *testing.T) { for _, tc := range tests { fromPos, toPos := text.CreateRange(tc.from, tc.to) - _, _, _ = text.Edit(fromPos, toPos, nil, tc.content, nil, ctx.IssueTimeTicket()) + _, _, err = text.Edit(fromPos, toPos, nil, tc.content, nil, ctx.IssueTimeTicket()) + assert.NoError(t, err) registerTextElementWithGarbage(fromPos, toPos, root, text) assert.Equal(t, tc.want, text.String()) assert.Equal(t, tc.garbage, root.GarbageLen()) } - gc, _ := root.GarbageCollect(time.MaxTicket) + gc, err := root.GarbageCollect(time.MaxTicket) + assert.NoError(t, err) assert.Equal(t, 3, gc) assert.Equal(t, 0, root.GarbageLen()) }) @@ -134,23 +143,27 @@ func TestRoot(t *testing.T) { t.Run("garbage collection for rich text test", func(t *testing.T) { root := helper.TestRoot() ctx := helper.TextChangeContext(root) - rgaTreeSplit, _ := crdt.NewRGATreeSplit(crdt.InitialTextNode()) + rgaTreeSplit, err := crdt.NewRGATreeSplit(crdt.InitialTextNode()) + assert.NoError(t, err) text := crdt.NewText(rgaTreeSplit, ctx.IssueTimeTicket()) fromPos, toPos := text.CreateRange(0, 0) - _, _, _ = text.Edit(fromPos, toPos, nil, "Hello World", nil, ctx.IssueTimeTicket()) + _, _, err = text.Edit(fromPos, toPos, nil, "Hello World", nil, ctx.IssueTimeTicket()) + assert.NoError(t, err) registerTextElementWithGarbage(fromPos, toPos, root, text) assert.Equal(t, `[{"val":"Hello World"}]`, text.Marshal()) assert.Equal(t, 0, root.GarbageLen()) fromPos, toPos = text.CreateRange(6, 11) - _, _, _ = text.Edit(fromPos, toPos, nil, "Yorkie", nil, ctx.IssueTimeTicket()) + _, _, err = text.Edit(fromPos, toPos, nil, "Yorkie", nil, ctx.IssueTimeTicket()) + assert.NoError(t, err) registerTextElementWithGarbage(fromPos, toPos, root, text) assert.Equal(t, `[{"val":"Hello "},{"val":"Yorkie"}]`, text.Marshal()) assert.Equal(t, 1, root.GarbageLen()) fromPos, toPos = text.CreateRange(0, 6) - _, _, _ = text.Edit(fromPos, toPos, nil, "", nil, ctx.IssueTimeTicket()) + _, _, err = text.Edit(fromPos, toPos, nil, "", nil, ctx.IssueTimeTicket()) + assert.NoError(t, err) registerTextElementWithGarbage(fromPos, toPos, root, text) assert.Equal(t, `[{"val":"Yorkie"}]`, text.Marshal()) assert.Equal(t, 2, root.GarbageLen()) @@ -160,7 +173,8 @@ func TestRoot(t *testing.T) { nodeLen := len(text.Nodes()) assert.Equal(t, 3, nodeLen) - gc, _ := root.GarbageCollect(time.MaxTicket) + gc, err := root.GarbageCollect(time.MaxTicket) + assert.NoError(t, err) assert.Equal(t, 2, gc) assert.Equal(t, 0, root.GarbageLen()) @@ -187,7 +201,8 @@ func TestRoot(t *testing.T) { assert.Equal(t, `{"1":1,"3":3}`, obj.Marshal()) assert.Equal(t, 4, root.GarbageLen()) - gc, _ := root.GarbageCollect(time.MaxTicket) + gc, err := root.GarbageCollect(time.MaxTicket) + assert.NoError(t, err) assert.Equal(t, 4, gc) assert.Equal(t, 0, root.GarbageLen()) @@ -196,7 +211,8 @@ func TestRoot(t *testing.T) { assert.Equal(t, `{"1":1}`, obj.Marshal()) assert.Equal(t, 1, root.GarbageLen()) - gc, _ = root.GarbageCollect(time.MaxTicket) + gc, err = root.GarbageCollect(time.MaxTicket) + assert.NoError(t, err) assert.Equal(t, 1, gc) assert.Equal(t, 0, root.GarbageLen()) }) diff --git a/pkg/document/crdt/text.go b/pkg/document/crdt/text.go index 402c678c8..a54946951 100644 --- a/pkg/document/crdt/text.go +++ b/pkg/document/crdt/text.go @@ -169,23 +169,23 @@ func (t *Text) Marshal() string { func (t *Text) DeepCopy() (Element, error) { rgaTreeSplit, err := NewRGATreeSplit(InitialTextNode()) if err != nil { - return nil, fmt.Errorf("crdt text DeepCopy: %s", err) + return nil, err } current := rgaTreeSplit.InitialHead() for _, node := range t.Nodes() { current, err := rgaTreeSplit.InsertAfter(current, node.DeepCopy()) if err != nil { - return nil, fmt.Errorf("crdt text DeepCopy: %s", err) + return nil, err } insPrevID := node.InsPrevID() if insPrevID != nil { insPrevNode, err := rgaTreeSplit.FindNode(insPrevID) if err != nil { - return nil, fmt.Errorf("crdt text DeepCopy: %s", err) + return nil, err } if insPrevNode == nil { - return nil, fmt.Errorf("crdt text DeepCopy: insPrevNode should be presence") + return nil, fmt.Errorf("insPrevNode should be presence") } current.SetInsPrev(insPrevNode) } @@ -256,7 +256,7 @@ func (t *Text) Edit( executedAt, ) if err != nil { - return nil, nil, fmt.Errorf("crdt text Edit: %s", err) + return nil, nil, err } return cursorPos, latestCreatedAtMapByActor, nil @@ -272,11 +272,11 @@ func (t *Text) Style( // 01. Split nodes with from and to _, toRight, err := t.rgaTreeSplit.findNodeWithSplit(to, executedAt) if err != nil { - return fmt.Errorf("crdt text Style: %s", err) + return err } _, fromRight, err := t.rgaTreeSplit.findNodeWithSplit(from, executedAt) if err != nil { - return fmt.Errorf("crdt text Style: %s", err) + return err } // 02. style nodes between from and to diff --git a/pkg/document/crdt/text_test.go b/pkg/document/crdt/text_test.go index 1023041f9..48e0526ef 100644 --- a/pkg/document/crdt/text_test.go +++ b/pkg/document/crdt/text_test.go @@ -29,15 +29,18 @@ func TestText(t *testing.T) { t.Run("marshal test", func(t *testing.T) { root := helper.TestRoot() ctx := helper.TextChangeContext(root) - rgaTreeSplit, _ := crdt.NewRGATreeSplit(crdt.InitialTextNode()) + rgaTreeSplit, err := crdt.NewRGATreeSplit(crdt.InitialTextNode()) + assert.NoError(t, err) text := crdt.NewText(rgaTreeSplit, ctx.IssueTimeTicket()) fromPos, toPos := text.CreateRange(0, 0) - _, _, _ = text.Edit(fromPos, toPos, nil, "Hello World", nil, ctx.IssueTimeTicket()) + _, _, err = text.Edit(fromPos, toPos, nil, "Hello World", nil, ctx.IssueTimeTicket()) + assert.NoError(t, err) assert.Equal(t, `[{"val":"Hello World"}]`, text.Marshal()) fromPos, toPos = text.CreateRange(6, 11) - _, _, _ = text.Edit(fromPos, toPos, nil, "Yorkie", nil, ctx.IssueTimeTicket()) + _, _, err = text.Edit(fromPos, toPos, nil, "Yorkie", nil, ctx.IssueTimeTicket()) + assert.NoError(t, err) assert.Equal(t, `[{"val":"Hello "},{"val":"Yorkie"}]`, text.Marshal()) }) @@ -66,19 +69,23 @@ func TestText(t *testing.T) { t.Run("marshal test", func(t *testing.T) { root := helper.TestRoot() ctx := helper.TextChangeContext(root) - rgaTreeSplit, _ := crdt.NewRGATreeSplit(crdt.InitialTextNode()) + rgaTreeSplit, err := crdt.NewRGATreeSplit(crdt.InitialTextNode()) + assert.NoError(t, err) text := crdt.NewText(rgaTreeSplit, ctx.IssueTimeTicket()) fromPos, toPos := text.CreateRange(0, 0) - _, _, _ = text.Edit(fromPos, toPos, nil, "Hello World", nil, ctx.IssueTimeTicket()) + _, _, err = text.Edit(fromPos, toPos, nil, "Hello World", nil, ctx.IssueTimeTicket()) + assert.NoError(t, err) assert.Equal(t, `[{"val":"Hello World"}]`, text.Marshal()) fromPos, toPos = text.CreateRange(6, 11) - _, _, _ = text.Edit(fromPos, toPos, nil, "Yorkie", nil, ctx.IssueTimeTicket()) + _, _, err = text.Edit(fromPos, toPos, nil, "Yorkie", nil, ctx.IssueTimeTicket()) + assert.NoError(t, err) assert.Equal(t, `[{"val":"Hello "},{"val":"Yorkie"}]`, text.Marshal()) fromPos, toPos = text.CreateRange(0, 1) - _ = text.Style(fromPos, toPos, map[string]string{"b": "1"}, ctx.IssueTimeTicket()) + err = text.Style(fromPos, toPos, map[string]string{"b": "1"}, ctx.IssueTimeTicket()) + assert.NoError(t, err) assert.Equal( t, `[{"attrs":{"b":"1"},"val":"H"},{"val":"ello "},{"val":"Yorkie"}]`, diff --git a/pkg/document/document.go b/pkg/document/document.go index 997a064af..e2306f9b9 100644 --- a/pkg/document/document.go +++ b/pkg/document/document.go @@ -59,7 +59,9 @@ func (d *Document) Update( return ErrDocumentRemoved } - d.ensureClone() + if err := d.ensureClone(); err != nil { + return err + } ctx := change.NewContext( d.doc.changeID.Next(), @@ -95,7 +97,9 @@ func (d *Document) ApplyChangePack(pack *change.Pack) error { return err } } else { - d.ensureClone() + if err := d.ensureClone(); err != nil { + return err + } for _, c := range pack.Changes { if err := c.Execute(d.clone); err != nil { @@ -121,7 +125,9 @@ func (d *Document) ApplyChangePack(pack *change.Pack) error { d.doc.checkpoint = d.doc.checkpoint.Forward(pack.Checkpoint) // 04. Do Garbage collection. - _, _ = d.GarbageCollect(pack.MinSyncedTicket) + if _, err := d.GarbageCollect(pack.MinSyncedTicket); err != nil { + return err + } // 05. Update the status. if pack.IsRemoved { @@ -194,7 +200,9 @@ func (d *Document) RootObject() *crdt.Object { // Root returns the root object of this document. func (d *Document) Root() *json.Object { - d.ensureClone() + if err := d.ensureClone(); err != nil { + panic(err) + } ctx := change.NewContext(d.doc.changeID.Next(), "", d.clone) return json.NewObject(ctx, d.clone.Object()) @@ -205,13 +213,13 @@ func (d *Document) GarbageCollect(ticket *time.Ticket) (int, error) { if d.clone != nil { _, err := d.clone.GarbageCollect(ticket) if err != nil { - return 0, fmt.Errorf("document GarbageCollect: %w", err) + return 0, err } } gc, err := d.doc.GarbageCollect(ticket) if err != nil { - return 0, fmt.Errorf("document GarbageCollect: %w", err) + return 0, err } return gc, nil } @@ -221,14 +229,15 @@ func (d *Document) GarbageLen() int { return d.doc.GarbageLen() } -func (d *Document) ensureClone() { +func (d *Document) ensureClone() error { if d.clone == nil { copiedDoc, err := d.doc.root.DeepCopy() if err != nil { - panic("document ensureClone: " + err.Error()) + return err } d.clone = copiedDoc } + return nil } func messageFromMsgAndArgs(msgAndArgs ...interface{}) string { diff --git a/pkg/document/document_test.go b/pkg/document/document_test.go index 2a54dc444..98e248a4c 100644 --- a/pkg/document/document_test.go +++ b/pkg/document/document_test.go @@ -486,7 +486,8 @@ func TestDocument(t *testing.T) { ) assert.Equal(t, 1, doc.GarbageLen()) - _, _ = doc.GarbageCollect(time.MaxTicket) + _, err = doc.GarbageCollect(time.MaxTicket) + assert.NoError(t, err) assert.Equal(t, 0, doc.GarbageLen()) assert.Equal( t, @@ -519,7 +520,8 @@ func TestDocument(t *testing.T) { assert.Equal(t, "{}", doc.Marshal()) assert.Equal(t, 2, doc.GarbageLen()) - _, _ = doc.GarbageCollect(time.MaxTicket) + _, err = doc.GarbageCollect(time.MaxTicket) + assert.NoError(t, err) assert.Equal(t, "{}", doc.Marshal()) assert.Equal(t, 0, doc.GarbageLen()) }) diff --git a/pkg/document/json/array.go b/pkg/document/json/array.go index bf9088bbc..ab6f164fd 100644 --- a/pkg/document/json/array.go +++ b/pkg/document/json/array.go @@ -17,7 +17,6 @@ package json import ( - "fmt" gotime "time" "github.com/yorkie-team/yorkie/pkg/document/change" @@ -189,7 +188,7 @@ func (p *Array) insertAfterInternal( copiedValue, err := value.DeepCopy() if err != nil { - panic(fmt.Sprintf("json array insertAfterInternal: %s", err.Error())) + panic(err) } p.context.Push(operations.NewAdd( p.Array.CreatedAt(), diff --git a/pkg/document/json/object.go b/pkg/document/json/object.go index f9e3b283f..fcf47f5d5 100644 --- a/pkg/document/json/object.go +++ b/pkg/document/json/object.go @@ -17,7 +17,6 @@ package json import ( - "fmt" gotime "time" "github.com/yorkie-team/yorkie/pkg/document/change" @@ -64,7 +63,7 @@ func (p *Object) SetNewText(k string) *Text { v := p.setInternal(k, func(ticket *time.Ticket) crdt.Element { rgaTreeSplit, err := crdt.NewRGATreeSplit(crdt.InitialTextNode()) if err != nil { - panic(fmt.Sprintf("json object SetNewText err: %s", err)) + panic(err) } return NewText( p.context, @@ -264,7 +263,7 @@ func (p *Object) setInternal( copiedValue, err := value.DeepCopy() if err != nil { - panic(fmt.Sprintf("json object setInternal: %s", err.Error())) + panic(err) } p.context.Push(operations.NewSet( p.CreatedAt(), diff --git a/pkg/document/json/text.go b/pkg/document/json/text.go index b420af166..290eb7b12 100644 --- a/pkg/document/json/text.go +++ b/pkg/document/json/text.go @@ -17,8 +17,6 @@ package json import ( - "fmt" - "github.com/yorkie-team/yorkie/pkg/document/change" "github.com/yorkie-team/yorkie/pkg/document/crdt" "github.com/yorkie-team/yorkie/pkg/document/operations" @@ -42,7 +40,7 @@ func NewText(ctx *change.Context, text *crdt.Text) *Text { // Edit edits the given range with the given content and attributes. func (p *Text) Edit(from, to int, content string, attributes ...map[string]string) *Text { if from > to { - panic("json test Edit: from should be less than or equal to to") + panic("from should be less than or equal to to") } fromPos, toPos := p.Text.CreateRange(from, to) @@ -63,7 +61,7 @@ func (p *Text) Edit(from, to int, content string, attributes ...map[string]strin ticket, ) if err != nil { - panic(fmt.Sprintf("json text Edit: %s", err.Error())) + panic(err) } p.context.Push(operations.NewEdit( @@ -77,7 +75,7 @@ func (p *Text) Edit(from, to int, content string, attributes ...map[string]strin )) result, err := fromPos.Equal(toPos) if err != nil { - panic(fmt.Sprintf("json text Edit: %s", err.Error())) + panic(err) } if !result { p.context.RegisterTextElementWithGarbage(p) @@ -89,7 +87,7 @@ func (p *Text) Edit(from, to int, content string, attributes ...map[string]strin // Style applies the style of the given range. func (p *Text) Style(from, to int, attributes map[string]string) *Text { if from > to { - panic("json test Style: from should be less than or equal to to") + panic("from should be less than or equal to to") } fromPos, toPos := p.Text.CreateRange(from, to) @@ -115,7 +113,7 @@ func (p *Text) Style(from, to int, attributes map[string]string) *Text { // Select stores that the given range has been selected. func (p *Text) Select(from, to int) *Text { if from > to { - panic("json text Select: from should be less than or equal to to") + panic("from should be less than or equal to to") } fromPos, toPos := p.Text.CreateRange(from, to) diff --git a/pkg/document/operations/add.go b/pkg/document/operations/add.go index 5b84d7f9b..d2e1ec58a 100644 --- a/pkg/document/operations/add.go +++ b/pkg/document/operations/add.go @@ -17,8 +17,6 @@ package operations import ( - "fmt" - "github.com/yorkie-team/yorkie/pkg/document/crdt" "github.com/yorkie-team/yorkie/pkg/document/time" ) @@ -64,7 +62,7 @@ func (o *Add) Execute(root *crdt.Root) error { value, err := o.value.DeepCopy() if err != nil { - return fmt.Errorf("add operations: %w", err) + return err } obj.InsertAfter(o.prevCreatedAt, value) diff --git a/pkg/document/operations/edit.go b/pkg/document/operations/edit.go index f58e6d348..448e39506 100644 --- a/pkg/document/operations/edit.go +++ b/pkg/document/operations/edit.go @@ -17,8 +17,6 @@ package operations import ( - "fmt" - "github.com/yorkie-team/yorkie/pkg/document/crdt" "github.com/yorkie-team/yorkie/pkg/document/time" ) @@ -80,13 +78,13 @@ func (e *Edit) Execute(root *crdt.Root) error { _, _, _ = obj.Edit(e.from, e.to, e.latestCreatedAtMapByActor, e.content, e.attributes, e.executedAt) result, err := e.from.Equal(e.to) if err != nil { - return fmt.Errorf("edit operations %w", err) + return err } if !result { root.RegisterTextElementWithGarbage(obj) } default: - return fmt.Errorf("edit operations %w", ErrNotApplicableDataType) + return ErrNotApplicableDataType } return nil diff --git a/pkg/document/operations/set.go b/pkg/document/operations/set.go index 0a5b7a730..2e2a66ce2 100644 --- a/pkg/document/operations/set.go +++ b/pkg/document/operations/set.go @@ -17,8 +17,6 @@ package operations import ( - "fmt" - "github.com/yorkie-team/yorkie/pkg/document/crdt" "github.com/yorkie-team/yorkie/pkg/document/time" ) @@ -65,7 +63,7 @@ func (o *Set) Execute(root *crdt.Root) error { value, err := o.value.DeepCopy() if err != nil { - return fmt.Errorf("set operations: %w", err) + return err } removed := obj.Set(o.key, value) root.RegisterElement(value) diff --git a/pkg/document/operations/style.go b/pkg/document/operations/style.go index bc5350810..c1d18e239 100644 --- a/pkg/document/operations/style.go +++ b/pkg/document/operations/style.go @@ -64,7 +64,9 @@ func (e *Style) Execute(root *crdt.Root) error { return ErrNotApplicableDataType } - _ = obj.Style(e.from, e.to, e.attributes, e.executedAt) + if err := obj.Style(e.from, e.to, e.attributes, e.executedAt); err != nil { + return err + } return nil } diff --git a/pkg/llrb/llrb.go b/pkg/llrb/llrb.go index 4c29492cb..937cf6d55 100644 --- a/pkg/llrb/llrb.go +++ b/pkg/llrb/llrb.go @@ -18,7 +18,6 @@ package llrb import ( - "fmt" "strings" ) @@ -78,7 +77,7 @@ func (t *Tree[K, V]) Put(k K, v V) (V, error) { var err error t.root, err = t.put(t.root, k, v) if err != nil { - return zeroV, fmt.Errorf("llrb Put: %w", err) + return zeroV, err } t.root.isRed = false return v, nil @@ -101,7 +100,7 @@ func (t *Tree[K, V]) Remove(key K) error { var err error t.root, err = t.remove(t.root, key) if err != nil { - return fmt.Errorf("llrb Remove: %w", err) + return err } if t.root != nil { t.root.isRed = false @@ -119,7 +118,7 @@ func (t *Tree[K, V]) Floor(key K) (K, V, error) { for node != nil { keyCompare, err := key.Compare(node.key) if err != nil { - return zeroK, zeroV, fmt.Errorf("llrb Floor: %w", err) + return zeroK, zeroV, err } if keyCompare > 0 { if node.right != nil { @@ -159,17 +158,17 @@ func (t *Tree[K, V]) put(node *Node[K, V], key K, value V) (*Node[K, V], error) keyCompare, err := key.Compare(node.key) if err != nil { - return nil, fmt.Errorf("llrb put: %w", err) + return nil, err } if keyCompare < 0 { node.left, err = t.put(node.left, key, value) if err != nil { - return nil, fmt.Errorf("llrb put: %w", err) + return nil, err } } else if keyCompare > 0 { node.right, err = t.put(node.right, key, value) if err != nil { - return nil, fmt.Errorf("llrb put: %w", err) + return nil, err } } else { node.value = value @@ -193,7 +192,7 @@ func (t *Tree[K, V]) put(node *Node[K, V], key K, value V) (*Node[K, V], error) func (t *Tree[K, V]) remove(node *Node[K, V], key K) (*Node[K, V], error) { keyCompare, err := key.Compare(node.key) if err != nil { - return nil, fmt.Errorf("llrb remove: %w", err) + return nil, err } if keyCompare < 0 { if !isRed(node.left) && !isRed(node.left.left) { @@ -201,7 +200,7 @@ func (t *Tree[K, V]) remove(node *Node[K, V], key K) (*Node[K, V], error) { } node.left, err = t.remove(node.left, key) if err != nil { - return nil, fmt.Errorf("llrb remove: %w", err) + return nil, err } } else { if isRed(node.left) { @@ -210,7 +209,7 @@ func (t *Tree[K, V]) remove(node *Node[K, V], key K) (*Node[K, V], error) { keyCompare, err := key.Compare(node.key) if err != nil { - return nil, fmt.Errorf("llrb remove: %w", err) + return nil, err } if keyCompare == 0 && node.right == nil { t.size-- @@ -223,7 +222,7 @@ func (t *Tree[K, V]) remove(node *Node[K, V], key K) (*Node[K, V], error) { keyCompare, err = key.Compare(node.key) if err != nil { - return nil, fmt.Errorf("llrb remove: %w", err) + return nil, err } if keyCompare == 0 { t.size-- @@ -234,7 +233,7 @@ func (t *Tree[K, V]) remove(node *Node[K, V], key K) (*Node[K, V], error) { } else { node.right, err = t.remove(node.right, key) if err != nil { - return nil, fmt.Errorf("llrb remove: %w", err) + return nil, err } } } diff --git a/pkg/llrb/llrb_test.go b/pkg/llrb/llrb_test.go index 8cda45748..092303599 100644 --- a/pkg/llrb/llrb_test.go +++ b/pkg/llrb/llrb_test.go @@ -73,17 +73,21 @@ func TestTree(t *testing.T) { for _, array := range arrays { tree := llrb.NewTree[*intKey, *intValue]() for _, value := range array { - _, _ = tree.Put(newIntKey(value), newIntValue(value)) + _, err := tree.Put(newIntKey(value), newIntValue(value)) + assert.NoError(t, err) } assert.Equal(t, "0,1,2,3,4,5,6,7,8,9", tree.String()) - _ = tree.Remove(newIntKey(8)) + err := tree.Remove(newIntKey(8)) + assert.NoError(t, err) assert.Equal(t, "0,1,2,3,4,5,6,7,9", tree.String()) - _ = tree.Remove(newIntKey(2)) + err = tree.Remove(newIntKey(2)) + assert.NoError(t, err) assert.Equal(t, "0,1,3,4,5,6,7,9", tree.String()) - _ = tree.Remove(newIntKey(5)) + err = tree.Remove(newIntKey(5)) + assert.NoError(t, err) assert.Equal(t, "0,1,3,4,6,7,9", tree.String()) } }) diff --git a/server/rpc/auth/webhook.go b/server/rpc/auth/webhook.go index 457116e4d..cb15a53c8 100644 --- a/server/rpc/auth/webhook.go +++ b/server/rpc/auth/webhook.go @@ -130,7 +130,7 @@ func withExponentialBackoff(ctx context.Context, cfg *backend.Config, webhookFn select { case <-ctx.Done(): - return fmt.Errorf("%w", ctx.Err()) + return ctx.Err() case <-time.After(waitBeforeRetry): } diff --git a/test/bench/document_bench_test.go b/test/bench/document_bench_test.go index 377f7aaaf..f0e0dd3ff 100644 --- a/test/bench/document_bench_test.go +++ b/test/bench/document_bench_test.go @@ -536,7 +536,8 @@ func benchmarkTextEditGC(cnt int, b *testing.B) { }, "replace contents with b") assert.NoError(b, err) assert.Equal(b, cnt, doc.GarbageLen()) - gc, _ := doc.GarbageCollect(time.MaxTicket) + gc, err := doc.GarbageCollect(time.MaxTicket) + assert.NoError(b, err) assert.Equal(b, cnt, gc) } } @@ -569,7 +570,8 @@ func benchmarkTextSplitGC(cnt int, b *testing.B) { assert.NoError(b, err) assert.Equal(b, cnt, doc.GarbageLen()) - gc, _ := doc.GarbageCollect(time.MaxTicket) + gc, err := doc.GarbageCollect(time.MaxTicket) + assert.NoError(b, err) assert.Equal(b, cnt, gc) } } @@ -608,7 +610,8 @@ func benchmarkArrayGC(cnt int, b *testing.B) { }, "deletes the array") assert.NoError(b, err) - gc, _ := doc.GarbageCollect(time.MaxTicket) + gc, err := doc.GarbageCollect(time.MaxTicket) + assert.NoError(b, err) assert.Equal(b, cnt+1, gc) assert.NoError(b, err) } From 72f99c9fc9ea43a129a011a03021a9b06aea4da1 Mon Sep 17 00:00:00 2001 From: emplam27 Date: Wed, 26 Apr 2023 19:27:54 +0900 Subject: [PATCH 5/6] rollback Compare method occurred panic --- api/converter/from_bytes.go | 16 +--- api/converter/from_pb.go | 9 +-- pkg/document/crdt/element.go | 2 +- pkg/document/crdt/rga_tree_split.go | 99 +++++++++--------------- pkg/document/crdt/rga_tree_split_test.go | 5 +- pkg/document/crdt/root.go | 9 +-- pkg/document/crdt/root_test.go | 42 +++------- pkg/document/crdt/text.go | 26 ++----- pkg/document/crdt/text_test.go | 12 +-- pkg/document/document.go | 18 +---- pkg/document/document_test.go | 6 +- pkg/document/internal_document.go | 7 +- pkg/document/json/object.go | 6 +- pkg/document/json/text.go | 12 ++- pkg/document/operations/edit.go | 5 +- pkg/llrb/llrb.go | 96 +++++++---------------- pkg/llrb/llrb_test.go | 20 ++--- test/bench/document_bench_test.go | 12 +-- 18 files changed, 127 insertions(+), 275 deletions(-) diff --git a/api/converter/from_bytes.go b/api/converter/from_bytes.go index 12ab26a71..872825caa 100644 --- a/api/converter/from_bytes.go +++ b/api/converter/from_bytes.go @@ -174,30 +174,20 @@ func fromJSONText( return nil, err } - rgaTreeSplit, err := crdt.NewRGATreeSplit(crdt.InitialTextNode()) - if err != nil { - return nil, err - } - + rgaTreeSplit := crdt.NewRGATreeSplit(crdt.InitialTextNode()) current := rgaTreeSplit.InitialHead() for _, pbNode := range pbText.Nodes { textNode, err := fromTextNode(pbNode) if err != nil { return nil, err } - current, err = rgaTreeSplit.InsertAfter(current, textNode) - if err != nil { - return nil, err - } + current = rgaTreeSplit.InsertAfter(current, textNode) insPrevID, err := fromTextNodeID(pbNode.InsPrevId) if err != nil { return nil, err } if insPrevID != nil { - insPrevNode, err := rgaTreeSplit.FindNode(insPrevID) - if err != nil { - return nil, err - } + insPrevNode := rgaTreeSplit.FindNode(insPrevID) if insPrevNode == nil { return nil, fmt.Errorf("insPrevNode should be presence") } diff --git a/api/converter/from_pb.go b/api/converter/from_pb.go index 5be276b29..b39ecb56c 100644 --- a/api/converter/from_pb.go +++ b/api/converter/from_pb.go @@ -611,11 +611,10 @@ func fromElement(pbElement *api.JSONElementSimple) (crdt.Element, error) { if err != nil { return nil, err } - rgaTreeSplit, err := crdt.NewRGATreeSplit(crdt.InitialTextNode()) - if err != nil { - return nil, err - } - return crdt.NewText(rgaTreeSplit, createdAt), nil + return crdt.NewText( + crdt.NewRGATreeSplit(crdt.InitialTextNode()), + createdAt, + ), nil case api.ValueType_VALUE_TYPE_INTEGER_CNT: fallthrough case api.ValueType_VALUE_TYPE_LONG_CNT: diff --git a/pkg/document/crdt/element.go b/pkg/document/crdt/element.go index 88aded4f1..c2f155b1f 100644 --- a/pkg/document/crdt/element.go +++ b/pkg/document/crdt/element.go @@ -38,7 +38,7 @@ type Container interface { type TextElement interface { Element removedNodesLen() int - purgeTextNodesWithGarbage(ticket *time.Ticket) (int, error) + purgeTextNodesWithGarbage(ticket *time.Ticket) int } // Element represents JSON element. diff --git a/pkg/document/crdt/rga_tree_split.go b/pkg/document/crdt/rga_tree_split.go index a1bc38b0a..17f7d036b 100644 --- a/pkg/document/crdt/rga_tree_split.go +++ b/pkg/document/crdt/rga_tree_split.go @@ -42,34 +42,32 @@ func NewRGATreeSplitNodeID(createdAt *time.Ticket, offset int) *RGATreeSplitNode } // Compare returns an integer comparing two ID. The result will be 0 if -// id==other, -1 if id < other, and +1 if id > other. -func (id *RGATreeSplitNodeID) Compare(other llrb.Key) (int, error) { +// id==other, -1 if id < other, and +1 if id > other. If the receiver or +// argument is nil, it would panic at runtime. This method is following +// golang standard interface. +func (id *RGATreeSplitNodeID) Compare(other llrb.Key) int { if id == nil || other == nil { - return 0, fmt.Errorf("RGATreeSplitNodeID cannot be null") + panic("RGATreeSplitNodeID cannot be null") } o := other.(*RGATreeSplitNodeID) compare := id.createdAt.Compare(o.createdAt) if compare != 0 { - return compare, nil + return compare } if id.offset > o.offset { - return 1, nil + return 1 } else if id.offset < o.offset { - return -1, nil + return -1 } - return 0, nil + return 0 } // Equal returns whether given ID equals to this ID or not. -func (id *RGATreeSplitNodeID) Equal(other llrb.Key) (bool, error) { - keyCompare, err := id.Compare(other) - if err != nil { - return false, err - } - return keyCompare == 0, nil +func (id *RGATreeSplitNodeID) Equal(other llrb.Key) bool { + return id.Compare(other) == 0 } // CreatedAt returns the creation time of this ID. @@ -139,15 +137,11 @@ func (pos *RGATreeSplitNodePos) RelativeOffset() int { } // Equal returns the whether the given pos equals or not. -func (pos *RGATreeSplitNodePos) Equal(other *RGATreeSplitNodePos) (bool, error) { - result, err := pos.id.Equal(other.id) - if err != nil { - return false, err - } - if !result { - return false, nil +func (pos *RGATreeSplitNodePos) Equal(other *RGATreeSplitNodePos) bool { + if !pos.id.Equal(other.id) { + return false } - return pos.relativeOffset == other.relativeOffset, nil + return pos.relativeOffset == other.relativeOffset } // Selection represents the selection of text range in the editor. @@ -303,20 +297,17 @@ type RGATreeSplit[V RGATreeSplitValue] struct { } // NewRGATreeSplit creates a new instance of RGATreeSplit. -func NewRGATreeSplit[V RGATreeSplitValue](initialHead *RGATreeSplitNode[V]) (*RGATreeSplit[V], error) { +func NewRGATreeSplit[V RGATreeSplitValue](initialHead *RGATreeSplitNode[V]) *RGATreeSplit[V] { treeByIndex := splay.NewTree(initialHead.indexNode) treeByID := llrb.NewTree[*RGATreeSplitNodeID, *RGATreeSplitNode[V]]() - _, err := treeByID.Put(initialHead.ID(), initialHead) - if err != nil { - return nil, err - } + treeByID.Put(initialHead.ID(), initialHead) return &RGATreeSplit[V]{ initialHead: initialHead, treeByIndex: treeByIndex, treeByID: treeByID, removedNodeMap: make(map[string]*RGATreeSplitNode[V]), - }, nil + } } func (s *RGATreeSplit[V]) createRange(from, to int) (*RGATreeSplitNodePos, *RGATreeSplitNodePos) { @@ -362,13 +353,9 @@ func (s *RGATreeSplit[V]) findNodeWithSplit( } func (s *RGATreeSplit[V]) findFloorNodePreferToLeft(id *RGATreeSplitNodeID) (*RGATreeSplitNode[V], error) { - node, err := s.findFloorNode(id) - if err != nil { - return nil, err - } + node := s.findFloorNode(id) if node == nil { - return nil, - fmt.Errorf("the node of the given id should be found: " + s.StructureAsString()) + return nil, fmt.Errorf("the node of the given id should be found: " + s.StructureAsString()) } if id.offset > 0 && node.id.offset == id.offset { @@ -395,10 +382,7 @@ func (s *RGATreeSplit[V]) splitNode(node *RGATreeSplitNode[V], offset int) (*RGA splitNode := node.split(offset) s.treeByIndex.UpdateWeight(splitNode.indexNode) - _, err := s.InsertAfter(node, splitNode) - if err != nil { - return nil, err - } + s.InsertAfter(node, splitNode) insNext := node.insNext if insNext != nil { @@ -410,20 +394,17 @@ func (s *RGATreeSplit[V]) splitNode(node *RGATreeSplitNode[V], offset int) (*RGA } // InsertAfter inserts the given node after the given previous node. -func (s *RGATreeSplit[V]) InsertAfter(prev, node *RGATreeSplitNode[V]) (*RGATreeSplitNode[V], error) { +func (s *RGATreeSplit[V]) InsertAfter(prev, node *RGATreeSplitNode[V]) *RGATreeSplitNode[V] { next := prev.next node.setPrev(prev) if next != nil { next.setPrev(node) } - _, err := s.treeByID.Put(node.id, node) - if err != nil { - return nil, err - } + s.treeByID.Put(node.id, node) s.treeByIndex.InsertAfter(prev.indexNode, node.indexNode) - return node, nil + return node } // InitialHead returns the head node of this RGATreeSplit. @@ -432,9 +413,9 @@ func (s *RGATreeSplit[V]) InitialHead() *RGATreeSplitNode[V] { } // FindNode returns the node of the given ID. -func (s *RGATreeSplit[V]) FindNode(id *RGATreeSplitNodeID) (*RGATreeSplitNode[V], error) { +func (s *RGATreeSplit[V]) FindNode(id *RGATreeSplitNodeID) *RGATreeSplitNode[V] { if id == nil { - return nil, nil + return nil } node, err := s.findFloorNode(id) @@ -451,24 +432,17 @@ func (s *RGATreeSplit[V]) CheckWeight() bool { return s.treeByIndex.CheckWeight() } -func (s *RGATreeSplit[V]) findFloorNode(id *RGATreeSplitNodeID) (*RGATreeSplitNode[V], error) { - key, value, err := s.treeByID.Floor(id) - if err != nil { - return nil, err - } +func (s *RGATreeSplit[V]) findFloorNode(id *RGATreeSplitNodeID) *RGATreeSplitNode[V] { + key, value := s.treeByID.Floor(id) if key == nil { - return nil, nil + return nil } - result, err := key.Equal(id) - if err != nil { - return nil, err - } - if !result && !key.hasSameCreatedAt(id) { - return nil, nil + if !key.Equal(id) && !key.hasSameCreatedAt(id) { + return nil } - return value, nil + return value } func (s *RGATreeSplit[V]) edit( @@ -502,10 +476,7 @@ func (s *RGATreeSplit[V]) edit( // 03. insert a new node if content.Len() > 0 { - inserted, err := s.InsertAfter(fromLeft, NewRGATreeSplitNode(NewRGATreeSplitNodeID(editedAt, 0), content)) - if err != nil { - return nil, nil, err - } + inserted := s.InsertAfter(fromLeft, NewRGATreeSplitNode(NewRGATreeSplitNodeID(editedAt, 0), content)) caretPos = NewRGATreeSplitNodePos(inserted.id, inserted.contentLen()) } @@ -655,7 +626,7 @@ func (s *RGATreeSplit[V]) removedNodesLen() int { } // purgeTextNodesWithGarbage physically purges nodes that have been removed. -func (s *RGATreeSplit[V]) purgeTextNodesWithGarbage(ticket *time.Ticket) (int, error) { +func (s *RGATreeSplit[V]) purgeTextNodesWithGarbage(ticket *time.Ticket) int { count := 0 for _, node := range s.removedNodeMap { if node.removedAt != nil && ticket.Compare(node.removedAt) >= 0 { @@ -669,7 +640,7 @@ func (s *RGATreeSplit[V]) purgeTextNodesWithGarbage(ticket *time.Ticket) (int, e } } - return count, nil + return count } // purge physically purge the given node from RGATreeSplit. diff --git a/pkg/document/crdt/rga_tree_split_test.go b/pkg/document/crdt/rga_tree_split_test.go index 32d82ae61..a4832f5bd 100644 --- a/pkg/document/crdt/rga_tree_split_test.go +++ b/pkg/document/crdt/rga_tree_split_test.go @@ -10,9 +10,8 @@ import ( ) func TestRGATreeSplit(t *testing.T) { - t.Run("compare nil id error test", func(t *testing.T) { + t.Run("compare nil id panic test", func(t *testing.T) { id := crdt.NewRGATreeSplitNodeID(time.InitialTicket, 0) - _, err := id.Compare(nil) - assert.Errorf(t, err, "ID cannot be null") + assert.Panics(t, func() { id.Compare(nil) }, "ID cannot be null") }) } diff --git a/pkg/document/crdt/root.go b/pkg/document/crdt/root.go index 75363e59d..7aa7a2dd4 100644 --- a/pkg/document/crdt/root.go +++ b/pkg/document/crdt/root.go @@ -110,7 +110,7 @@ func (r *Root) DeepCopy() (*Root, error) { } // GarbageCollect purge elements that were removed before the given time. -func (r *Root) GarbageCollect(ticket *time.Ticket) (int, error) { +func (r *Root) GarbageCollect(ticket *time.Ticket) int { count := 0 for _, pair := range r.removedElementPairMapByCreatedAt { @@ -121,17 +121,14 @@ func (r *Root) GarbageCollect(ticket *time.Ticket) (int, error) { } for _, text := range r.textElementWithGarbageMapByCreatedAt { - purgedTextNodes, err := text.purgeTextNodesWithGarbage(ticket) - if err != nil { - return 0, err - } + purgedTextNodes := text.purgeTextNodesWithGarbage(ticket) if purgedTextNodes > 0 { delete(r.textElementWithGarbageMapByCreatedAt, text.CreatedAt().Key()) } count += purgedTextNodes } - return count, nil + return count } // ElementMapLen returns the size of element map. diff --git a/pkg/document/crdt/root_test.go b/pkg/document/crdt/root_test.go index e24a8d114..a78bca532 100644 --- a/pkg/document/crdt/root_test.go +++ b/pkg/document/crdt/root_test.go @@ -27,8 +27,7 @@ import ( ) func registerTextElementWithGarbage(fromPos, toPos *crdt.RGATreeSplitNodePos, root *crdt.Root, text crdt.TextElement) { - result, _ := fromPos.Equal(toPos) - if !result { + if !fromPos.Equal(toPos) { root.RegisterTextElementWithGarbage(text) } } @@ -50,17 +49,14 @@ func TestRoot(t *testing.T) { assert.Equal(t, "[0,2]", array.Marshal()) assert.Equal(t, 1, root.GarbageLen()) - gc, err := root.GarbageCollect(time.MaxTicket) - assert.NoError(t, err) - assert.Equal(t, 1, gc) + assert.Equal(t, 1, root.GarbageCollect(time.MaxTicket)) assert.Equal(t, 0, root.GarbageLen()) }) t.Run("garbage collection for text test", func(t *testing.T) { root := helper.TestRoot() ctx := helper.TextChangeContext(root) - rgaTreeSplit, _ := crdt.NewRGATreeSplit(crdt.InitialTextNode()) - text := crdt.NewText(rgaTreeSplit, ctx.IssueTimeTicket()) + text := crdt.NewText(crdt.NewRGATreeSplit(crdt.InitialTextNode()), ctx.IssueTimeTicket()) fromPos, toPos := text.CreateRange(0, 0) _, _, err := text.Edit(fromPos, toPos, nil, "Hello World", nil, ctx.IssueTimeTicket()) @@ -95,9 +91,7 @@ func TestRoot(t *testing.T) { nodeLen := len(text.Nodes()) assert.Equal(t, 4, nodeLen) - gc, err := root.GarbageCollect(time.MaxTicket) - assert.NoError(t, err) - assert.Equal(t, 3, gc) + assert.Equal(t, 3, root.GarbageCollect(time.MaxTicket)) assert.Equal(t, 0, root.GarbageLen()) nodeLen = len(text.Nodes()) assert.Equal(t, 1, nodeLen) @@ -114,9 +108,7 @@ func TestRoot(t *testing.T) { root := helper.TestRoot() ctx := helper.TextChangeContext(root) - rgaTreeSplit, err := crdt.NewRGATreeSplit(crdt.InitialTextNode()) - assert.NoError(t, err) - text := crdt.NewText(rgaTreeSplit, ctx.IssueTimeTicket()) + text := crdt.NewText(crdt.NewRGATreeSplit(crdt.InitialTextNode()), ctx.IssueTimeTicket()) tests := []test{ {from: 0, to: 0, content: "Yorkie", want: "Yorkie", garbage: 0}, @@ -127,28 +119,24 @@ func TestRoot(t *testing.T) { for _, tc := range tests { fromPos, toPos := text.CreateRange(tc.from, tc.to) - _, _, err = text.Edit(fromPos, toPos, nil, tc.content, nil, ctx.IssueTimeTicket()) + _, _, err := text.Edit(fromPos, toPos, nil, tc.content, nil, ctx.IssueTimeTicket()) assert.NoError(t, err) registerTextElementWithGarbage(fromPos, toPos, root, text) assert.Equal(t, tc.want, text.String()) assert.Equal(t, tc.garbage, root.GarbageLen()) } - gc, err := root.GarbageCollect(time.MaxTicket) - assert.NoError(t, err) - assert.Equal(t, 3, gc) + assert.Equal(t, 3, root.GarbageCollect(time.MaxTicket)) assert.Equal(t, 0, root.GarbageLen()) }) t.Run("garbage collection for rich text test", func(t *testing.T) { root := helper.TestRoot() ctx := helper.TextChangeContext(root) - rgaTreeSplit, err := crdt.NewRGATreeSplit(crdt.InitialTextNode()) - assert.NoError(t, err) - text := crdt.NewText(rgaTreeSplit, ctx.IssueTimeTicket()) + text := crdt.NewText(crdt.NewRGATreeSplit(crdt.InitialTextNode()), ctx.IssueTimeTicket()) fromPos, toPos := text.CreateRange(0, 0) - _, _, err = text.Edit(fromPos, toPos, nil, "Hello World", nil, ctx.IssueTimeTicket()) + _, _, err := text.Edit(fromPos, toPos, nil, "Hello World", nil, ctx.IssueTimeTicket()) assert.NoError(t, err) registerTextElementWithGarbage(fromPos, toPos, root, text) assert.Equal(t, `[{"val":"Hello World"}]`, text.Marshal()) @@ -173,9 +161,7 @@ func TestRoot(t *testing.T) { nodeLen := len(text.Nodes()) assert.Equal(t, 3, nodeLen) - gc, err := root.GarbageCollect(time.MaxTicket) - assert.NoError(t, err) - assert.Equal(t, 2, gc) + assert.Equal(t, 2, root.GarbageCollect(time.MaxTicket)) assert.Equal(t, 0, root.GarbageLen()) nodeLen = len(text.Nodes()) @@ -201,9 +187,7 @@ func TestRoot(t *testing.T) { assert.Equal(t, `{"1":1,"3":3}`, obj.Marshal()) assert.Equal(t, 4, root.GarbageLen()) - gc, err := root.GarbageCollect(time.MaxTicket) - assert.NoError(t, err) - assert.Equal(t, 4, gc) + assert.Equal(t, 4, root.GarbageCollect(time.MaxTicket)) assert.Equal(t, 0, root.GarbageLen()) deleted = obj.Delete("3", ctx.IssueTimeTicket()) @@ -211,9 +195,7 @@ func TestRoot(t *testing.T) { assert.Equal(t, `{"1":1}`, obj.Marshal()) assert.Equal(t, 1, root.GarbageLen()) - gc, err = root.GarbageCollect(time.MaxTicket) - assert.NoError(t, err) - assert.Equal(t, 1, gc) + assert.Equal(t, 1, root.GarbageCollect(time.MaxTicket)) assert.Equal(t, 0, root.GarbageLen()) }) } diff --git a/pkg/document/crdt/text.go b/pkg/document/crdt/text.go index a54946951..4f235db56 100644 --- a/pkg/document/crdt/text.go +++ b/pkg/document/crdt/text.go @@ -167,23 +167,14 @@ func (t *Text) Marshal() string { // DeepCopy copies itself deeply. func (t *Text) DeepCopy() (Element, error) { - rgaTreeSplit, err := NewRGATreeSplit(InitialTextNode()) - if err != nil { - return nil, err - } - + rgaTreeSplit := NewRGATreeSplit(InitialTextNode()) current := rgaTreeSplit.InitialHead() + for _, node := range t.Nodes() { - current, err := rgaTreeSplit.InsertAfter(current, node.DeepCopy()) - if err != nil { - return nil, err - } + current = rgaTreeSplit.InsertAfter(current, node.DeepCopy()) insPrevID := node.InsPrevID() if insPrevID != nil { - insPrevNode, err := rgaTreeSplit.FindNode(insPrevID) - if err != nil { - return nil, err - } + insPrevNode := rgaTreeSplit.FindNode(insPrevID) if insPrevNode == nil { return nil, fmt.Errorf("insPrevNode should be presence") } @@ -248,18 +239,13 @@ func (t *Text) Edit( val.attrs.Set(key, value, executedAt) } - cursorPos, latestCreatedAtMapByActor, err := t.rgaTreeSplit.edit( + return t.rgaTreeSplit.edit( from, to, latestCreatedAtMapByActor, val, executedAt, ) - if err != nil { - return nil, nil, err - } - - return cursorPos, latestCreatedAtMapByActor, nil } // Style applies the given attributes of the given range. @@ -324,6 +310,6 @@ func (t *Text) removedNodesLen() int { } // purgeTextNodesWithGarbage physically purges nodes that have been removed. -func (t *Text) purgeTextNodesWithGarbage(ticket *time.Ticket) (int, error) { +func (t *Text) purgeTextNodesWithGarbage(ticket *time.Ticket) int { return t.rgaTreeSplit.purgeTextNodesWithGarbage(ticket) } diff --git a/pkg/document/crdt/text_test.go b/pkg/document/crdt/text_test.go index 48e0526ef..92b4083ff 100644 --- a/pkg/document/crdt/text_test.go +++ b/pkg/document/crdt/text_test.go @@ -29,12 +29,10 @@ func TestText(t *testing.T) { t.Run("marshal test", func(t *testing.T) { root := helper.TestRoot() ctx := helper.TextChangeContext(root) - rgaTreeSplit, err := crdt.NewRGATreeSplit(crdt.InitialTextNode()) - assert.NoError(t, err) - text := crdt.NewText(rgaTreeSplit, ctx.IssueTimeTicket()) + text := crdt.NewText(crdt.NewRGATreeSplit(crdt.InitialTextNode()), ctx.IssueTimeTicket()) fromPos, toPos := text.CreateRange(0, 0) - _, _, err = text.Edit(fromPos, toPos, nil, "Hello World", nil, ctx.IssueTimeTicket()) + _, _, err := text.Edit(fromPos, toPos, nil, "Hello World", nil, ctx.IssueTimeTicket()) assert.NoError(t, err) assert.Equal(t, `[{"val":"Hello World"}]`, text.Marshal()) @@ -69,12 +67,10 @@ func TestText(t *testing.T) { t.Run("marshal test", func(t *testing.T) { root := helper.TestRoot() ctx := helper.TextChangeContext(root) - rgaTreeSplit, err := crdt.NewRGATreeSplit(crdt.InitialTextNode()) - assert.NoError(t, err) - text := crdt.NewText(rgaTreeSplit, ctx.IssueTimeTicket()) + text := crdt.NewText(crdt.NewRGATreeSplit(crdt.InitialTextNode()), ctx.IssueTimeTicket()) fromPos, toPos := text.CreateRange(0, 0) - _, _, err = text.Edit(fromPos, toPos, nil, "Hello World", nil, ctx.IssueTimeTicket()) + _, _, err := text.Edit(fromPos, toPos, nil, "Hello World", nil, ctx.IssueTimeTicket()) assert.NoError(t, err) assert.Equal(t, `[{"val":"Hello World"}]`, text.Marshal()) diff --git a/pkg/document/document.go b/pkg/document/document.go index e2306f9b9..821a91269 100644 --- a/pkg/document/document.go +++ b/pkg/document/document.go @@ -125,9 +125,7 @@ func (d *Document) ApplyChangePack(pack *change.Pack) error { d.doc.checkpoint = d.doc.checkpoint.Forward(pack.Checkpoint) // 04. Do Garbage collection. - if _, err := d.GarbageCollect(pack.MinSyncedTicket); err != nil { - return err - } + d.GarbageCollect(pack.MinSyncedTicket) // 05. Update the status. if pack.IsRemoved { @@ -209,19 +207,11 @@ func (d *Document) Root() *json.Object { } // GarbageCollect purge elements that were removed before the given time. -func (d *Document) GarbageCollect(ticket *time.Ticket) (int, error) { +func (d *Document) GarbageCollect(ticket *time.Ticket) int { if d.clone != nil { - _, err := d.clone.GarbageCollect(ticket) - if err != nil { - return 0, err - } - } - - gc, err := d.doc.GarbageCollect(ticket) - if err != nil { - return 0, err + d.clone.GarbageCollect(ticket) } - return gc, nil + return d.doc.GarbageCollect(ticket) } // GarbageLen returns the count of removed elements. diff --git a/pkg/document/document_test.go b/pkg/document/document_test.go index 98e248a4c..b4a221a31 100644 --- a/pkg/document/document_test.go +++ b/pkg/document/document_test.go @@ -486,8 +486,7 @@ func TestDocument(t *testing.T) { ) assert.Equal(t, 1, doc.GarbageLen()) - _, err = doc.GarbageCollect(time.MaxTicket) - assert.NoError(t, err) + doc.GarbageCollect(time.MaxTicket) assert.Equal(t, 0, doc.GarbageLen()) assert.Equal( t, @@ -520,8 +519,7 @@ func TestDocument(t *testing.T) { assert.Equal(t, "{}", doc.Marshal()) assert.Equal(t, 2, doc.GarbageLen()) - _, err = doc.GarbageCollect(time.MaxTicket) - assert.NoError(t, err) + doc.GarbageCollect(time.MaxTicket) assert.Equal(t, "{}", doc.Marshal()) assert.Equal(t, 0, doc.GarbageLen()) }) diff --git a/pkg/document/internal_document.go b/pkg/document/internal_document.go index 77b1133ae..1a0b7ca83 100644 --- a/pkg/document/internal_document.go +++ b/pkg/document/internal_document.go @@ -133,17 +133,14 @@ func (d *InternalDocument) ApplyChangePack(pack *change.Pack) error { d.checkpoint = d.checkpoint.Forward(pack.Checkpoint) if pack.MinSyncedTicket != nil { - _, err := d.GarbageCollect(pack.MinSyncedTicket) - if err != nil { - return err - } + d.GarbageCollect(pack.MinSyncedTicket) } return nil } // GarbageCollect purge elements that were removed before the given time. -func (d *InternalDocument) GarbageCollect(ticket *time.Ticket) (int, error) { +func (d *InternalDocument) GarbageCollect(ticket *time.Ticket) int { return d.root.GarbageCollect(ticket) } diff --git a/pkg/document/json/object.go b/pkg/document/json/object.go index fcf47f5d5..8255fedbf 100644 --- a/pkg/document/json/object.go +++ b/pkg/document/json/object.go @@ -61,13 +61,9 @@ func (p *Object) SetNewArray(k string) *Array { // SetNewText sets a new Text for the given key. func (p *Object) SetNewText(k string) *Text { v := p.setInternal(k, func(ticket *time.Ticket) crdt.Element { - rgaTreeSplit, err := crdt.NewRGATreeSplit(crdt.InitialTextNode()) - if err != nil { - panic(err) - } return NewText( p.context, - crdt.NewText(rgaTreeSplit, ticket), + crdt.NewText(crdt.NewRGATreeSplit(crdt.InitialTextNode()), ticket), ) }) diff --git a/pkg/document/json/text.go b/pkg/document/json/text.go index 290eb7b12..f667a7e26 100644 --- a/pkg/document/json/text.go +++ b/pkg/document/json/text.go @@ -73,11 +73,7 @@ func (p *Text) Edit(from, to int, content string, attributes ...map[string]strin attrs, ticket, )) - result, err := fromPos.Equal(toPos) - if err != nil { - panic(err) - } - if !result { + if !fromPos.Equal(toPos) { p.context.RegisterTextElementWithGarbage(p) } @@ -92,12 +88,14 @@ func (p *Text) Style(from, to int, attributes map[string]string) *Text { fromPos, toPos := p.Text.CreateRange(from, to) ticket := p.context.IssueTimeTicket() - _ = p.Text.Style( + if err := p.Text.Style( fromPos, toPos, attributes, ticket, - ) + ); err != nil { + panic(err) + } p.context.Push(operations.NewStyle( p.CreatedAt(), diff --git a/pkg/document/operations/edit.go b/pkg/document/operations/edit.go index 448e39506..b1fd50baa 100644 --- a/pkg/document/operations/edit.go +++ b/pkg/document/operations/edit.go @@ -75,12 +75,11 @@ func (e *Edit) Execute(root *crdt.Root) error { switch obj := parent.(type) { case *crdt.Text: - _, _, _ = obj.Edit(e.from, e.to, e.latestCreatedAtMapByActor, e.content, e.attributes, e.executedAt) - result, err := e.from.Equal(e.to) + _, _, err := obj.Edit(e.from, e.to, e.latestCreatedAtMapByActor, e.content, e.attributes, e.executedAt) if err != nil { return err } - if !result { + if !e.from.Equal(e.to) { root.RegisterTextElementWithGarbage(obj) } default: diff --git a/pkg/llrb/llrb.go b/pkg/llrb/llrb.go index 937cf6d55..2847adb45 100644 --- a/pkg/llrb/llrb.go +++ b/pkg/llrb/llrb.go @@ -27,7 +27,7 @@ type Key interface { // a.Compare(b) = 1 => a > b // a.Compare(b) = 0 => a == b // a.Compare(b) = -1 => a < b - Compare(k Key) (int, error) + Compare(k Key) int } // Value represents the data stored in the nodes of Tree. @@ -80,7 +80,7 @@ func (t *Tree[K, V]) Put(k K, v V) (V, error) { return zeroV, err } t.root.isRed = false - return v, nil + return v } func (t *Tree[K, V]) String() string { @@ -92,42 +92,31 @@ func (t *Tree[K, V]) String() string { } // Remove removes the value of the given key. -func (t *Tree[K, V]) Remove(key K) error { +func (t *Tree[K, V]) Remove(key K) { if !isRed(t.root.left) && !isRed(t.root.right) { t.root.isRed = true } - var err error - t.root, err = t.remove(t.root, key) - if err != nil { - return err - } + t.root = t.remove(t.root, key) if t.root != nil { t.root.isRed = false } - - return nil } // Floor returns the greatest key less than or equal to the given key. -func (t *Tree[K, V]) Floor(key K) (K, V, error) { - var zeroK K - var zeroV V +func (t *Tree[K, V]) Floor(key K) (K, V) { node := t.root for node != nil { - keyCompare, err := key.Compare(node.key) - if err != nil { - return zeroK, zeroV, err - } - if keyCompare > 0 { + compare := key.Compare(node.key) + if compare > 0 { if node.right != nil { node.right.parent = node node = node.right } else { - return node.key, node.value, nil + return node.key, node.value } - } else if keyCompare < 0 { + } else if compare < 0 { if node.left != nil { node.left.parent = node node = node.left @@ -140,36 +129,29 @@ func (t *Tree[K, V]) Floor(key K) (K, V, error) { } // TODO(hackerwins): check below warning - return parent.key, parent.value, nil + return parent.key, parent.value } } else { - return node.key, node.value, nil + return node.key, node.value } } - return zeroK, zeroV, nil + var zeroK K + var zeroV V + return zeroK, zeroV } -func (t *Tree[K, V]) put(node *Node[K, V], key K, value V) (*Node[K, V], error) { +func (t *Tree[K, V]) put(node *Node[K, V], key K, value V) *Node[K, V] { if node == nil { t.size++ - return NewNode(key, value, true), nil + return NewNode(key, value, true) } - keyCompare, err := key.Compare(node.key) - if err != nil { - return nil, err - } - if keyCompare < 0 { - node.left, err = t.put(node.left, key, value) - if err != nil { - return nil, err - } - } else if keyCompare > 0 { - node.right, err = t.put(node.right, key, value) - if err != nil { - return nil, err - } + compare := key.Compare(node.key) + if compare < 0 { + node.left = t.put(node.left, key, value) + } else if compare > 0 { + node.right = t.put(node.right, key, value) } else { node.value = value } @@ -186,59 +168,41 @@ func (t *Tree[K, V]) put(node *Node[K, V], key K, value V) (*Node[K, V], error) flipColors(node) } - return node, nil + return node } -func (t *Tree[K, V]) remove(node *Node[K, V], key K) (*Node[K, V], error) { - keyCompare, err := key.Compare(node.key) - if err != nil { - return nil, err - } - if keyCompare < 0 { +func (t *Tree[K, V]) remove(node *Node[K, V], key K) *Node[K, V] { + if key.Compare(node.key) < 0 { if !isRed(node.left) && !isRed(node.left.left) { node = moveRedLeft(node) } - node.left, err = t.remove(node.left, key) - if err != nil { - return nil, err - } + node.left = t.remove(node.left, key) } else { if isRed(node.left) { node = rotateRight(node) } - keyCompare, err := key.Compare(node.key) - if err != nil { - return nil, err - } - if keyCompare == 0 && node.right == nil { + if key.Compare(node.key) == 0 && node.right == nil { t.size-- - return nil, nil + return nil } if !isRed(node.right) && !isRed(node.right.left) { node = moveRedRight(node) } - keyCompare, err = key.Compare(node.key) - if err != nil { - return nil, err - } - if keyCompare == 0 { + if key.Compare(node.key) == 0 { t.size-- smallest := min(node.right) node.value = smallest.value node.key = smallest.key node.right = removeMin(node.right) } else { - node.right, err = t.remove(node.right, key) - if err != nil { - return nil, err - } + node.right = t.remove(node.right, key) } } - return fixUp(node), nil + return fixUp(node) } func rotateLeft[K Key, V Value](node *Node[K, V]) *Node[K, V] { diff --git a/pkg/llrb/llrb_test.go b/pkg/llrb/llrb_test.go index 092303599..9003ac1e6 100644 --- a/pkg/llrb/llrb_test.go +++ b/pkg/llrb/llrb_test.go @@ -35,14 +35,14 @@ func newIntKey(key int) *intKey { } } -func (k *intKey) Compare(other llrb.Key) (int, error) { +func (k *intKey) Compare(other llrb.Key) int { o := other.(*intKey) if k.key > o.key { - return 1, nil + return 1 } else if k.key < o.key { - return -1, nil + return -1 } else { - return 0, nil + return 0 } } @@ -73,21 +73,17 @@ func TestTree(t *testing.T) { for _, array := range arrays { tree := llrb.NewTree[*intKey, *intValue]() for _, value := range array { - _, err := tree.Put(newIntKey(value), newIntValue(value)) - assert.NoError(t, err) + tree.Put(newIntKey(value), newIntValue(value)) } assert.Equal(t, "0,1,2,3,4,5,6,7,8,9", tree.String()) - err := tree.Remove(newIntKey(8)) - assert.NoError(t, err) + tree.Remove(newIntKey(8)) assert.Equal(t, "0,1,2,3,4,5,6,7,9", tree.String()) - err = tree.Remove(newIntKey(2)) - assert.NoError(t, err) + tree.Remove(newIntKey(2)) assert.Equal(t, "0,1,3,4,5,6,7,9", tree.String()) - err = tree.Remove(newIntKey(5)) - assert.NoError(t, err) + tree.Remove(newIntKey(5)) assert.Equal(t, "0,1,3,4,6,7,9", tree.String()) } }) diff --git a/test/bench/document_bench_test.go b/test/bench/document_bench_test.go index f0e0dd3ff..d71f28895 100644 --- a/test/bench/document_bench_test.go +++ b/test/bench/document_bench_test.go @@ -536,9 +536,7 @@ func benchmarkTextEditGC(cnt int, b *testing.B) { }, "replace contents with b") assert.NoError(b, err) assert.Equal(b, cnt, doc.GarbageLen()) - gc, err := doc.GarbageCollect(time.MaxTicket) - assert.NoError(b, err) - assert.Equal(b, cnt, gc) + assert.Equal(b, cnt, doc.GarbageCollect(time.MaxTicket)) } } @@ -570,9 +568,7 @@ func benchmarkTextSplitGC(cnt int, b *testing.B) { assert.NoError(b, err) assert.Equal(b, cnt, doc.GarbageLen()) - gc, err := doc.GarbageCollect(time.MaxTicket) - assert.NoError(b, err) - assert.Equal(b, cnt, gc) + assert.Equal(b, cnt, doc.GarbageCollect(time.MaxTicket)) } } @@ -610,9 +606,7 @@ func benchmarkArrayGC(cnt int, b *testing.B) { }, "deletes the array") assert.NoError(b, err) - gc, err := doc.GarbageCollect(time.MaxTicket) - assert.NoError(b, err) - assert.Equal(b, cnt+1, gc) + assert.Equal(b, cnt+1, doc.GarbageCollect(time.MaxTicket)) assert.NoError(b, err) } } From 350a5cd83ff50288615b5c2f83236865e6af53e1 Mon Sep 17 00:00:00 2001 From: emplam27 Date: Wed, 26 Apr 2023 19:29:47 +0900 Subject: [PATCH 6/6] Changes by hackerwins --- pkg/document/crdt/rga_tree_split.go | 11 ++--------- pkg/document/operations/style.go | 5 +---- pkg/llrb/llrb.go | 9 ++------- 3 files changed, 5 insertions(+), 20 deletions(-) diff --git a/pkg/document/crdt/rga_tree_split.go b/pkg/document/crdt/rga_tree_split.go index 17f7d036b..582142635 100644 --- a/pkg/document/crdt/rga_tree_split.go +++ b/pkg/document/crdt/rga_tree_split.go @@ -418,12 +418,7 @@ func (s *RGATreeSplit[V]) FindNode(id *RGATreeSplitNodeID) *RGATreeSplitNode[V] return nil } - node, err := s.findFloorNode(id) - if err != nil { - return nil, err - } - - return node, nil + return s.findFloorNode(id) } // CheckWeight returns false when there is an incorrect weight node. @@ -632,9 +627,7 @@ func (s *RGATreeSplit[V]) purgeTextNodesWithGarbage(ticket *time.Ticket) int { if node.removedAt != nil && ticket.Compare(node.removedAt) >= 0 { s.treeByIndex.Delete(node.indexNode) s.purge(node) - if err := s.treeByID.Remove(node.id); err != nil { - return 0, fmt.Errorf("rga tree split purgeTextNodesWithGarbage: %w", err) - } + s.treeByID.Remove(node.id) delete(s.removedNodeMap, node.id.key()) count++ } diff --git a/pkg/document/operations/style.go b/pkg/document/operations/style.go index c1d18e239..069f8a4d1 100644 --- a/pkg/document/operations/style.go +++ b/pkg/document/operations/style.go @@ -64,10 +64,7 @@ func (e *Style) Execute(root *crdt.Root) error { return ErrNotApplicableDataType } - if err := obj.Style(e.from, e.to, e.attributes, e.executedAt); err != nil { - return err - } - return nil + return obj.Style(e.from, e.to, e.attributes, e.executedAt) } // From returns the start point of the editing range. diff --git a/pkg/llrb/llrb.go b/pkg/llrb/llrb.go index 2847adb45..316effac0 100644 --- a/pkg/llrb/llrb.go +++ b/pkg/llrb/llrb.go @@ -72,13 +72,8 @@ func NewTree[K Key, V Value]() *Tree[K, V] { } // Put puts the value of the given key. -func (t *Tree[K, V]) Put(k K, v V) (V, error) { - var zeroV V - var err error - t.root, err = t.put(t.root, k, v) - if err != nil { - return zeroV, err - } +func (t *Tree[K, V]) Put(k K, v V) V { + t.root = t.put(t.root, k, v) t.root.isRed = false return v }