Repository: yorkie-team/yorkie
Pull request #173 (2021-04-07 09:06:25)

Fix incorrect sequences when detaching documents
<!--  Thanks for sending a pull request! -->

**What this PR does / why we need it**:

When the client attaches to a new document with the same key after detaching the document, the previous sequence remains in DB. Therefore, the changes in the new document were not reflected.

https://github.com/yorkie-team/yorkie/blob/8015fefa13d35ab835486ba92e0da762026ddbd3/yorkie/packs/pack_service.go#L169

So when detaching the document, we initialize the checkpoint.

AS-IS

```
ClientInfo: C1
  ㄴ ClientDocInfo: D1, (2, 1), Detached
```

TO-BE

```
ClientInfo: C1
  ㄴ ClientDocInfo: D1, (0, 0), Detached
```

**Which issue(s) this PR fixes**:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
-->
Address https://github.com/yorkie-team/yorkie-codepair/issues/94

**Special notes for your reviewer**:

**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/173?src=pr&el=h1) Report
> Merging [#173](https://codecov.io/gh/yorkie-team/yorkie/pull/173?src=pr&el=desc) (e19f24c) into [main](https://codecov.io/gh/yorkie-team/yorkie/commit/8015fefa13d35ab835486ba92e0da762026ddbd3?el=desc) (8015fef) will **decrease** coverage by `0.03%`.
> The diff coverage is `0.00%`.

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

```diff
@@            Coverage Diff             @@
##             main     #173      +/-   ##
==========================================
- Coverage   59.85%   59.82%   -0.04%     
==========================================
  Files          38       38              
  Lines        3343     3345       +2     
==========================================
  Hits         2001     2001              
- Misses       1156     1158       +2     
  Partials      186      186              
```


| [Impacted Files](https://codecov.io/gh/yorkie-team/yorkie/pull/173?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [yorkie/backend/db/client\_info.go](https://codecov.io/gh/yorkie-team/yorkie/pull/173/diff?src=pr&el=tree#diff-eW9ya2llL2JhY2tlbmQvZGIvY2xpZW50X2luZm8uZ28=) | `0.00% <0.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/yorkie-team/yorkie/pull/173?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/173?src=pr&el=footer). Last update [8015fef...e19f24c](https://codecov.io/gh/yorkie-team/yorkie/pull/173?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/pkg/document/checkpoint/checkpoint.go b/pkg/document/checkpoint/checkpoint.go
index 7dad73b1..e8e9099e 100644
--- a/pkg/document/checkpoint/checkpoint.go
+++ b/pkg/document/checkpoint/checkpoint.go
@@ -18,11 +18,15 @@ package checkpoint
 
 import (
 	"fmt"
+	"math"
 )
 
 // Initial is the initial value of the checkpoint.
 var Initial = New(0, 0)
 
+// Max is the max value of the checkpoint.
+var Max = New(math.MaxUint64, math.MaxUint32)
+
 // Checkpoint is used to determine the client received changes.
 type Checkpoint struct {
 	ServerSeq uint64
diff --git a/yorkie/backend/db/client_info.go b/yorkie/backend/db/client_info.go
index d85116f9..5dbdd459 100644
--- a/yorkie/backend/db/client_info.go
+++ b/yorkie/backend/db/client_info.go
@@ -91,6 +91,8 @@ func (i *ClientInfo) DetachDocument(docID ID) error {
 	}
 
 	i.Documents[docID].Status = documentDetached
+	i.Documents[docID].ClientSeq = 0
+	i.Documents[docID].ServerSeq = 0
 	i.UpdatedAt = time.Now()
 
 	return nil
diff --git a/yorkie/backend/db/mongo/client.go b/yorkie/backend/db/mongo/client.go
index ea121f87..1ba931f1 100644
--- a/yorkie/backend/db/mongo/client.go
+++ b/yorkie/backend/db/mongo/client.go
@@ -193,9 +193,8 @@ func (c *Client) UpdateClientInfoAfterPushPull(
 ) error {
 	clientDocInfoKey := "documents." + docInfo.ID.String() + "."
 	clientDocInfo := clientInfo.Documents[docInfo.ID]
-	result := c.collection(ColClients).FindOneAndUpdate(ctx, bson.M{
-		"key": clientInfo.Key,
-	}, bson.M{
+
+	updater := bson.M{
 		"$max": bson.M{
 			clientDocInfoKey + "server_seq": clientDocInfo.ServerSeq,
 			clientDocInfoKey + "client_seq": clientDocInfo.ClientSeq,
@@ -204,7 +203,27 @@ func (c *Client) UpdateClientInfoAfterPushPull(
 			clientDocInfoKey + "status": clientDocInfo.Status,
 			"updated_at":                clientInfo.UpdatedAt,
 		},
-	})
+	}
+
+	attached, err := clientInfo.IsAttached(docInfo.ID)
+	if err != nil {
+		return err
+	}
+
+	if !attached {
+		updater = bson.M{
+			"$set": bson.M{
+				clientDocInfoKey + "server_seq": 0,
+				clientDocInfoKey + "client_seq": 0,
+				clientDocInfoKey + "status":     clientDocInfo.Status,
+				"updated_at":                    clientInfo.UpdatedAt,
+			},
+		}
+	}
+
+	result := c.collection(ColClients).FindOneAndUpdate(ctx, bson.M{
+		"key": clientInfo.Key,
+	}, updater)
 
 	if result.Err() != nil {
 		if result.Err() == mongo.ErrNoDocuments {
@@ -314,6 +333,7 @@ func (c *Client) StoreChangeInfos(
 		models,
 		options.BulkWrite().SetOrdered(true),
 	); err != nil {
+		log.Logger.Error(err)
 		return err
 	}
 
@@ -327,6 +347,7 @@ func (c *Client) StoreChangeInfos(
 		},
 	})
 	if err != nil {
+		log.Logger.Error(err)
 		return err
 	}
 	if res.MatchedCount == 0 {
diff --git a/yorkie/packs/pack_service.go b/yorkie/packs/pack_service.go
index 1aa73a12..5f6213a7 100644
--- a/yorkie/packs/pack_service.go
+++ b/yorkie/packs/pack_service.go
@@ -70,7 +70,7 @@ func PushPull(
 	}
 
 	// 03. store pushed changes, document info and checkpoint of the client to DB.
-	if reqPack.HasChanges() {
+	if len(pushedChanges) > 0 {
 		if err := be.DB.StoreChangeInfos(ctx, docInfo, initialServerSeq, pushedChanges); err != nil {
 			return nil, err
 		}
@@ -234,29 +234,20 @@ func pullChanges(
 	be *backend.Backend,
 	clientInfo *db.ClientInfo,
 	docInfo *db.DocInfo,
-	pack *change.Pack,
+	requestPack *change.Pack,
 	pushedCP *checkpoint.Checkpoint,
 	initialServerSeq uint64,
 ) (*checkpoint.Checkpoint, []*change.Change, error) {
-	fetchedChanges, err := be.DB.FindChangeInfosBetweenServerSeqs(
+	pulledChanges, err := be.DB.FindChangeInfosBetweenServerSeqs(
 		ctx,
 		docInfo.ID,
-		pack.Checkpoint.ServerSeq+1,
+		requestPack.Checkpoint.ServerSeq+1,
 		initialServerSeq,
 	)
 	if err != nil {
 		return nil, nil, err
 	}
 
-	var pulledChanges []*change.Change
-	for _, fetchedChange := range fetchedChanges {
-		if clientInfo.ID.String() == fetchedChange.ID().Actor().String() {
-			continue
-		}
-
-		pulledChanges = append(pulledChanges, fetchedChange)
-	}
-
 	pulledCP := pushedCP.NextServerSeq(docInfo.ServerSeq)
 
 	if len(pulledChanges) > 0 {


Test patch:
diff --git a/test/integration/document_test.go b/test/integration/document_test.go
index c3fc1234..cc579375 100644
--- a/test/integration/document_test.go
+++ b/test/integration/document_test.go
@@ -46,9 +46,9 @@ func TestDocument(t *testing.T) {
 		ctx := context.Background()
 		doc := document.New(helper.Collection, t.Name())
 		err := doc.Update(func(root *proxy.ObjectProxy) error {
-			root.SetString("k1", "k1")
+			root.SetString("k1", "v1")
 			return nil
-		}, "update k1 with k1")
+		}, "update k1 with v1")
 		assert.NoError(t, err)
 
 		err = c1.Attach(ctx, doc)
@@ -59,13 +59,16 @@ func TestDocument(t *testing.T) {
 		assert.NoError(t, err)
 		assert.False(t, doc.IsAttached())
 
-		err = c1.Attach(ctx, doc)
-		assert.NoError(t, err)
-		assert.True(t, doc.IsAttached())
+		doc2 := document.New(helper.Collection, t.Name())
+		err = doc2.Update(func(root *proxy.ObjectProxy) error {
+			root.SetString("k1", "v2")
+			return nil
+		}, "update k1 with v2")
 
-		err = c1.Detach(ctx, doc)
+		err = c1.Attach(ctx, doc2)
 		assert.NoError(t, err)
-		assert.False(t, doc.IsAttached())
+		assert.True(t, doc2.IsAttached())
+		assert.Equal(t, `{"k1":"v2"}`, doc2.Marshal())
 	})
 
 	t.Run("concurrent complex test", func(t *testing.T) {
diff --git a/yorkie/backend/db/client_info_test.go b/yorkie/backend/db/client_info_test.go
new file mode 100644
index 00000000..b914be70
--- /dev/null
+++ b/yorkie/backend/db/client_info_test.go
@@ -0,0 +1,34 @@
+package db_test
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/yorkie-team/yorkie/pkg/document/checkpoint"
+	"github.com/yorkie-team/yorkie/yorkie/backend/db"
+)
+
+func TestClientInfo(t *testing.T) {
+	t.Run("attach/detach document test", func(t *testing.T) {
+		docID := db.ID("000000000000000000000000")
+		clientInfo := db.ClientInfo{
+			Status: db.ClientActivated,
+		}
+
+		err := clientInfo.AttachDocument(docID)
+		assert.NoError(t, err)
+		isAttached, err := clientInfo.IsAttached(docID)
+		assert.NoError(t, err)
+		assert.True(t, isAttached)
+
+		err = clientInfo.UpdateCheckpoint(docID, checkpoint.Max)
+		assert.NoError(t, err)
+
+		err = clientInfo.DetachDocument(docID)
+		assert.NoError(t, err)
+		isAttached, err = clientInfo.IsAttached(docID)
+		assert.NoError(t, err)
+		assert.False(t, isAttached)
+	})
+}