Repository: yorkie-team/yorkie
Pull request #164 (2021-03-24 02:12:07)

Remove errors that occur when insPrev does not exist
<!--  Thanks for sending a pull request! -->

**What this PR does / why we need it**:

Remove errors that occur when `insPrev` does not exist.

InsPrev can be removed due to GC, so remove the panic code added as an assertion.

**Which issue(s) this PR fixes**:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
-->
Fixes yorkie-team/yorkie-codepair#71

**Special notes for your reviewer**:

Text implementation in Agent is used by Agent, and unlike JS SDK, it does not expose `Change` events. Therefore, the error related to `insPrev` could not be reproduced.

**Does this PR introduce a user-facing change?**:
<!--
If no, just write "NONE" in the release-note block below.
If yes, a release note is required:
Enter your extended release note in the block below. If the PR requires additional action from users switching to the new release, include the string "action required".
-->
```release-note

```

**Additional documentation**:

<!--
This section can be blank if this pull request does not require a release note.

Please use the following format for linking documentation:
- [Usage]: <link>
- [Other doc]: <link>
-->
```docs

```

**Checklist**:
- [x] Added relevant tests or not required
- [x] Didn't break anything


codecov[bot]: # [Codecov](https://codecov.io/gh/yorkie-team/yorkie/pull/164?src=pr&el=h1) Report
> Merging [#164](https://codecov.io/gh/yorkie-team/yorkie/pull/164?src=pr&el=desc) (4815239) into [main](https://codecov.io/gh/yorkie-team/yorkie/commit/1c7ba7616c3f5127a201e8a8f01a74c1fbcb41e0?el=desc) (1c7ba76) will **increase** coverage by `0.28%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/yorkie-team/yorkie/pull/164/graphs/tree.svg?width=650&height=150&src=pr&token=P6COB1ZNIC)](https://codecov.io/gh/yorkie-team/yorkie/pull/164?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##             main     #164      +/-   ##
==========================================
+ Coverage   59.34%   59.62%   +0.28%     
==========================================
  Files          37       37              
  Lines        3289     3292       +3     
==========================================
+ Hits         1952     1963      +11     
+ Misses       1151     1144       -7     
+ Partials      186      185       -1     
```


| [Impacted Files](https://codecov.io/gh/yorkie-team/yorkie/pull/164?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/document/json/rga\_tree\_split.go](https://codecov.io/gh/yorkie-team/yorkie/pull/164/diff?src=pr&el=tree#diff-cGtnL2RvY3VtZW50L2pzb24vcmdhX3RyZWVfc3BsaXQuZ28=) | `80.26% <ø> (+0.71%)` | :arrow_up: |
| [pkg/document/document.go](https://codecov.io/gh/yorkie-team/yorkie/pull/164/diff?src=pr&el=tree#diff-cGtnL2RvY3VtZW50L2RvY3VtZW50Lmdv) | `52.23% <100.00%> (+8.48%)` | :arrow_up: |
| [pkg/splay/splay.go](https://codecov.io/gh/yorkie-team/yorkie/pull/164/diff?src=pr&el=tree#diff-cGtnL3NwbGF5L3NwbGF5Lmdv) | `63.80% <100.00%> (+0.44%)` | :arrow_up: |
| [pkg/document/internal\_document.go](https://codecov.io/gh/yorkie-team/yorkie/pull/164/diff?src=pr&el=tree#diff-cGtnL2RvY3VtZW50L2ludGVybmFsX2RvY3VtZW50Lmdv) | `22.03% <0.00%> (+3.38%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/yorkie-team/yorkie/pull/164?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/yorkie-team/yorkie/pull/164?src=pr&el=footer). Last update [1c7ba76...4815239](https://codecov.io/gh/yorkie-team/yorkie/pull/164?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/pkg/document/document.go b/pkg/document/document.go
index 1336118e..b273d75c 100644
--- a/pkg/document/document.go
+++ b/pkg/document/document.go
@@ -176,6 +176,14 @@ func (d *Document) RootObject() *json.Object {
 	return d.doc.RootObject()
 }
 
+// Root returns the proxy of the root object.
+func (d *Document) Root() *proxy.ObjectProxy {
+	d.ensureClone()
+
+	ctx := change.NewContext(d.doc.changeID.Next(), "", d.clone)
+	return proxy.NewObjectProxy(ctx, d.clone.Object())
+}
+
 // GarbageCollect purge elements that were removed before the given time.
 func (d *Document) GarbageCollect(ticket *time.Ticket) int {
 	if d.clone != nil {
diff --git a/pkg/document/json/rga_tree_split.go b/pkg/document/json/rga_tree_split.go
index fe31515f..94b43178 100644
--- a/pkg/document/json/rga_tree_split.go
+++ b/pkg/document/json/rga_tree_split.go
@@ -339,9 +339,9 @@ func (s *RGATreeSplit) findFloorNodePreferToLeft(id *RGATreeSplitNodeID) *RGATre
 	}
 
 	if id.offset > 0 && node.id.offset == id.offset {
+		// NOTE: InsPrev may not be present due to GC.
 		if node.insPrev == nil {
-			log.Logger.Error(s.AnnotatedString())
-			panic("insPrev should be presence")
+			return node
 		}
 		node = node.insPrev
 	}
@@ -536,10 +536,6 @@ func (s *RGATreeSplit) AnnotatedString() string {
 
 	node := s.initialHead
 	for node != nil {
-		if node.id.offset > 0 && node.insPrev == nil {
-			log.Logger.Warn("insPrev should be presence")
-		}
-
 		if node.removedAt != nil {
 			result = append(result, fmt.Sprintf(
 				"{%s}",
diff --git a/pkg/splay/splay.go b/pkg/splay/splay.go
index 116d5f8b..e47c70ee 100644
--- a/pkg/splay/splay.go
+++ b/pkg/splay/splay.go
@@ -183,6 +183,10 @@ func (t *Tree) IndexOf(node *Node) int {
 
 // Find returns the Node and offset of the given index.
 func (t *Tree) Find(index int) (*Node, int) {
+	if t.root == nil {
+		return nil, 0
+	}
+
 	node := t.root
 	offset := index
 	for {


Test patch:
diff --git a/pkg/document/document_test.go b/pkg/document/document_test.go
index 103a8d8b..a6d7706e 100644
--- a/pkg/document/document_test.go
+++ b/pkg/document/document_test.go
@@ -26,6 +26,7 @@ import (
 	"github.com/yorkie-team/yorkie/pkg/document"
 	"github.com/yorkie-team/yorkie/pkg/document/checkpoint"
 	"github.com/yorkie-team/yorkie/pkg/document/proxy"
+	"github.com/yorkie-team/yorkie/pkg/document/time"
 )
 
 var (
@@ -455,4 +456,41 @@ func TestDocument(t *testing.T) {
 		assert.Equal(t, err, errDummy, "should returns the dummy error")
 		assert.Equal(t, `{"k1":{"k1.1":1,"k1.2":2}}`, doc.Marshal())
 	})
+
+	t.Run("text garbage collection test", func(t *testing.T) {
+		doc := document.New("c1", "d1")
+
+		err := doc.Update(func(root *proxy.ObjectProxy) error {
+			root.SetNewText("text")
+			root.GetText("text").Edit(0, 0, "ABCD")
+			root.GetText("text").Edit(0, 2, "12")
+			return nil
+		})
+		assert.NoError(t, err)
+		assert.Equal(
+			t,
+			`[0:0:00:0 ][1:3:00:0 12]{1:2:00:0 AB}[1:2:00:2 CD]`,
+			doc.Root().GetText("text").AnnotatedString(),
+		)
+
+		assert.Equal(t, 1, doc.GarbageLen())
+		doc.GarbageCollect(time.MaxTicket)
+		assert.Equal(t, 0, doc.GarbageLen())
+		assert.Equal(
+			t,
+			`[0:0:00:0 ][1:3:00:0 12][1:2:00:2 CD]`,
+			doc.Root().GetText("text").AnnotatedString(),
+		)
+
+		err = doc.Update(func(root *proxy.ObjectProxy) error {
+			root.GetText("text").Edit(2, 4, "")
+			return nil
+		})
+		assert.NoError(t, err)
+		assert.Equal(
+			t,
+			`[0:0:00:0 ][1:3:00:0 12]{1:2:00:2 CD}`,
+			doc.Root().GetText("text").AnnotatedString(),
+		)
+	})
 }
diff --git a/pkg/splay/splay_test.go b/pkg/splay/splay_test.go
index 1c03f4a9..1d86865d 100644
--- a/pkg/splay/splay_test.go
+++ b/pkg/splay/splay_test.go
@@ -46,6 +46,10 @@ func TestSplayTree(t *testing.T) {
 	t.Run("insert and splay test", func(t *testing.T) {
 		tree := splay.NewTree(nil)
 
+		node, idx := tree.Find(0)
+		assert.Nil(t, node)
+		assert.Equal(t, 0, idx)
+
 		nodeA := tree.Insert(newSplayNode("A2"))
 		assert.Equal(t, "[2,2]A2", tree.AnnotatedString())
 		nodeB := tree.Insert(newSplayNode("B23"))