Skip to content

Commit

Permalink
Return ErrAlreadyDetached when reattaching (#908)
Browse files Browse the repository at this point in the history 
This commit addresses the need for handling reattaching detached
documents. When reattaching a document that was previously detached,
special processing is required to prevent attachment in cases where
tombstone nodes may have been garbage collected by another client during
editing.

If attaching a new document with the same key, it should attach normally.
Therefore, there needs to be a distinction between attaching a new
document and reattaching.

While there is no difference in DB.ClientInfo, the Request ChangePack's
Checkpoint.ServerSeq value can be used to distinguish between attaching
a new document and reattaching a detached document. In cases where
Checkpoint.ServerSeq is greater than 0, the ErrAlreadyDetached error
should be returned.
  • Loading branch information
@hackerwins
hackerwins authored Jun 27, 2024
1 parent f86a4f3 commit 4226417
Show file tree
Hide file tree
Showing 8 changed files with 66 additions and 34 deletions.
5 changes: 5 additions & 0 deletions pkg/document/change/pack.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,3 +76,8 @@ func (p *Pack) OperationsLen() int {
}
return operations
}

// IsAttached returns whether the document is attached or not.
func (p *Pack) IsAttached() bool {
return p.Checkpoint.ServerSeq != 0
}
8 changes: 7 additions & 1 deletion server/backend/database/client_info.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ var (
ErrDocumentNotAttached = errors.New("document not attached")
ErrDocumentNeverAttached = errors.New("client has never attached the document")
ErrDocumentAlreadyAttached = errors.New("document already attached")
ErrDocumentAlreadyDetached = errors.New("document already detached")
)

// Below are statuses of the client.
Expand Down Expand Up @@ -104,7 +105,7 @@ func (i *ClientInfo) Deactivate() {
}

// AttachDocument attaches the given document to this client.
func (i *ClientInfo) AttachDocument(docID types.ID) error {
func (i *ClientInfo) AttachDocument(docID types.ID, alreadyAttached bool) error {
if i.Status != ClientActivated {
return fmt.Errorf("client(%s) attaches %s: %w",
i.ID, docID, ErrClientNotActivated)
Expand All @@ -114,6 +115,11 @@ func (i *ClientInfo) AttachDocument(docID types.ID) error {
i.Documents = make(map[types.ID]*ClientDocInfo)
}

if alreadyAttached && i.hasDocument(docID) && i.Documents[docID].Status == DocumentDetached {
return fmt.Errorf("client(%s) attaches %s: %w",
i.ID, docID, ErrDocumentAlreadyDetached)
}

if i.hasDocument(docID) && i.Documents[docID].Status == DocumentAttached {
return fmt.Errorf("client(%s) attaches %s: %w",
i.ID, docID, ErrDocumentAlreadyAttached)
Expand Down
12 changes: 6 additions & 6 deletions server/backend/database/client_info_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ func TestClientInfo(t *testing.T) {
Status: database.ClientActivated,
}

err := clientInfo.AttachDocument(dummyDocID)
err := clientInfo.AttachDocument(dummyDocID, false)
assert.NoError(t, err)
isAttached, err := clientInfo.IsAttached(dummyDocID)
assert.NoError(t, err)
Expand All @@ -54,7 +54,7 @@ func TestClientInfo(t *testing.T) {
assert.NoError(t, err)
assert.False(t, isAttached)

err = clientInfo.AttachDocument(dummyDocID)
err = clientInfo.AttachDocument(dummyDocID, false)
assert.NoError(t, err)
isAttached, err = clientInfo.IsAttached(dummyDocID)
assert.NoError(t, err)
Expand Down Expand Up @@ -85,7 +85,7 @@ func TestClientInfo(t *testing.T) {
Status: database.ClientActivated,
}

err := clientInfo.AttachDocument(dummyDocID)
err := clientInfo.AttachDocument(dummyDocID, false)
assert.NoError(t, err)
isAttached, err := clientInfo.IsAttached(dummyDocID)
assert.NoError(t, err)
Expand All @@ -102,7 +102,7 @@ func TestClientInfo(t *testing.T) {
Status: database.ClientDeactivated,
}

err := clientInfo.AttachDocument(dummyDocID)
err := clientInfo.AttachDocument(dummyDocID, false)
assert.ErrorIs(t, err, database.ErrClientNotActivated)

err = clientInfo.EnsureDocumentAttached(dummyDocID)
Expand Down Expand Up @@ -136,13 +136,13 @@ func TestClientInfo(t *testing.T) {
Status: database.ClientActivated,
}

err := clientInfo.AttachDocument(dummyDocID)
err := clientInfo.AttachDocument(dummyDocID, false)
assert.NoError(t, err)
isAttached, err := clientInfo.IsAttached(dummyDocID)
assert.NoError(t, err)
assert.True(t, isAttached)

err = clientInfo.AttachDocument(dummyDocID)
err = clientInfo.AttachDocument(dummyDocID, false)
assert.ErrorIs(t, err, database.ErrDocumentAlreadyAttached)
})
}
38 changes: 19 additions & 19 deletions server/backend/database/testcases/testcases.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ func RunFindChangesBetweenServerSeqsTest(
clientInfo, _ := db.ActivateClient(ctx, projectID, t.Name())
docInfo, _ := db.FindDocInfoByKeyAndOwner(ctx, clientInfo.RefKey(), docKey, true)
docRefKey := docInfo.RefKey()
assert.NoError(t, clientInfo.AttachDocument(docInfo.ID))
assert.NoError(t, clientInfo.AttachDocument(docInfo.ID, false))
assert.NoError(t, db.UpdateClientInfoAfterPushPull(ctx, clientInfo, docInfo))

bytesID, _ := clientInfo.ID.Bytes()
Expand Down Expand Up @@ -720,7 +720,7 @@ func RunCreateChangeInfosTest(t *testing.T, db database.Database, projectID type
clientInfo, _ := db.ActivateClient(ctx, projectID, t.Name())
docInfo, _ := db.FindDocInfoByKeyAndOwner(ctx, clientInfo.RefKey(), docKey, true)
docRefKey := docInfo.RefKey()
assert.NoError(t, clientInfo.AttachDocument(docInfo.ID))
assert.NoError(t, clientInfo.AttachDocument(docInfo.ID, false))
assert.NoError(t, db.UpdateClientInfoAfterPushPull(ctx, clientInfo, docInfo))

// 02. Remove the document and check the document is removed.
Expand All @@ -739,7 +739,7 @@ func RunCreateChangeInfosTest(t *testing.T, db database.Database, projectID type
clientInfo1, _ := db.ActivateClient(ctx, projectID, t.Name())
docInfo1, _ := db.FindDocInfoByKeyAndOwner(ctx, clientInfo1.RefKey(), docKey, true)
docRefKey1 := docInfo1.RefKey()
assert.NoError(t, clientInfo1.AttachDocument(docRefKey1.DocID))
assert.NoError(t, clientInfo1.AttachDocument(docRefKey1.DocID, false))
assert.NoError(t, db.UpdateClientInfoAfterPushPull(ctx, clientInfo1, docInfo1))

// 02. Remove the document.
Expand All @@ -750,7 +750,7 @@ func RunCreateChangeInfosTest(t *testing.T, db database.Database, projectID type
// 03. Create a document with same key and check they have same key but different id.
docInfo2, _ := db.FindDocInfoByKeyAndOwner(ctx, clientInfo1.RefKey(), docKey, true)
docRefKey2 := docInfo2.RefKey()
assert.NoError(t, clientInfo1.AttachDocument(docRefKey2.DocID))
assert.NoError(t, clientInfo1.AttachDocument(docRefKey2.DocID, false))
assert.NoError(t, db.UpdateClientInfoAfterPushPull(ctx, clientInfo1, docInfo2))
assert.Equal(t, docInfo1.Key, docInfo2.Key)
assert.NotEqual(t, docInfo1.ID, docInfo2.ID)
Expand All @@ -763,7 +763,7 @@ func RunCreateChangeInfosTest(t *testing.T, db database.Database, projectID type
clientInfo, _ := db.ActivateClient(ctx, projectID, t.Name())
docInfo, _ := db.FindDocInfoByKeyAndOwner(ctx, clientInfo.RefKey(), docKey, true)
docRefKey := docInfo.RefKey()
assert.NoError(t, clientInfo.AttachDocument(docInfo.ID))
assert.NoError(t, clientInfo.AttachDocument(docInfo.ID, false))
assert.NoError(t, db.UpdateClientInfoAfterPushPull(ctx, clientInfo, docInfo))

doc := document.New(key.Key(t.Name()))
Expand Down Expand Up @@ -801,7 +801,7 @@ func RunUpdateClientInfoAfterPushPullTest(t *testing.T, db database.Database, pr

err = db.UpdateClientInfoAfterPushPull(ctx, clientInfo, docInfo)
assert.ErrorIs(t, err, database.ErrDocumentNeverAttached)
assert.NoError(t, clientInfo.AttachDocument(docInfo.ID))
assert.NoError(t, clientInfo.AttachDocument(docInfo.ID, false))
assert.NoError(t, db.UpdateClientInfoAfterPushPull(ctx, clientInfo, docInfo))
})

Expand All @@ -813,7 +813,7 @@ func RunUpdateClientInfoAfterPushPullTest(t *testing.T, db database.Database, pr
docInfo, err := db.FindDocInfoByKeyAndOwner(ctx, clientInfo.RefKey(), docKey, true)
assert.NoError(t, err)

assert.NoError(t, clientInfo.AttachDocument(docInfo.ID))
assert.NoError(t, clientInfo.AttachDocument(docInfo.ID, false))
assert.NoError(t, db.UpdateClientInfoAfterPushPull(ctx, clientInfo, docInfo))

result, err := db.FindClientInfoByRefKey(ctx, clientInfo.RefKey())
Expand All @@ -831,7 +831,7 @@ func RunUpdateClientInfoAfterPushPullTest(t *testing.T, db database.Database, pr
docInfo, err := db.FindDocInfoByKeyAndOwner(ctx, clientInfo.RefKey(), docKey, true)
assert.NoError(t, err)

assert.NoError(t, clientInfo.AttachDocument(docInfo.ID))
assert.NoError(t, clientInfo.AttachDocument(docInfo.ID, false))
clientInfo.Documents[docInfo.ID].ServerSeq = 1
clientInfo.Documents[docInfo.ID].ClientSeq = 1
assert.NoError(t, db.UpdateClientInfoAfterPushPull(ctx, clientInfo, docInfo))
Expand Down Expand Up @@ -873,7 +873,7 @@ func RunUpdateClientInfoAfterPushPullTest(t *testing.T, db database.Database, pr
docInfo, err := db.FindDocInfoByKeyAndOwner(ctx, clientInfo.RefKey(), docKey, true)
assert.NoError(t, err)

assert.NoError(t, clientInfo.AttachDocument(docInfo.ID))
assert.NoError(t, clientInfo.AttachDocument(docInfo.ID, false))
clientInfo.Documents[docInfo.ID].ServerSeq = 1
clientInfo.Documents[docInfo.ID].ClientSeq = 1
assert.NoError(t, db.UpdateClientInfoAfterPushPull(ctx, clientInfo, docInfo))
Expand Down Expand Up @@ -902,7 +902,7 @@ func RunUpdateClientInfoAfterPushPullTest(t *testing.T, db database.Database, pr
docInfo, err := db.FindDocInfoByKeyAndOwner(ctx, clientInfo.RefKey(), docKey, true)
assert.NoError(t, err)

assert.NoError(t, clientInfo.AttachDocument(docInfo.ID))
assert.NoError(t, clientInfo.AttachDocument(docInfo.ID, false))
clientInfo.Documents[docInfo.ID].ServerSeq = 1
clientInfo.Documents[docInfo.ID].ClientSeq = 1
assert.NoError(t, db.UpdateClientInfoAfterPushPull(ctx, clientInfo, docInfo))
Expand Down Expand Up @@ -931,7 +931,7 @@ func RunUpdateClientInfoAfterPushPullTest(t *testing.T, db database.Database, pr
docInfo, err := db.FindDocInfoByKeyAndOwner(ctx, clientInfo.RefKey(), docKey, true)
assert.NoError(t, err)

assert.NoError(t, clientInfo.AttachDocument(docInfo.ID))
assert.NoError(t, clientInfo.AttachDocument(docInfo.ID, false))
assert.NoError(t, db.UpdateClientInfoAfterPushPull(ctx, clientInfo, docInfo))

clientInfo.ID = "invalid clientInfo id"
Expand Down Expand Up @@ -962,7 +962,7 @@ func RunIsDocumentAttachedTest(t *testing.T, db database.Database, projectID typ
assert.False(t, attached)

// 02. Check if document is attached after attaching
assert.NoError(t, c1.AttachDocument(docRefKey1.DocID))
assert.NoError(t, c1.AttachDocument(docRefKey1.DocID, false))
assert.NoError(t, db.UpdateClientInfoAfterPushPull(ctx, c1, d1))
attached, err = db.IsDocumentAttached(ctx, docRefKey1, "")
assert.NoError(t, err)
Expand All @@ -976,9 +976,9 @@ func RunIsDocumentAttachedTest(t *testing.T, db database.Database, projectID typ
assert.False(t, attached)

// 04. Check if document is attached after two clients attaching
assert.NoError(t, c1.AttachDocument(docRefKey1.DocID))
assert.NoError(t, c1.AttachDocument(docRefKey1.DocID, false))
assert.NoError(t, db.UpdateClientInfoAfterPushPull(ctx, c1, d1))
assert.NoError(t, c2.AttachDocument(docRefKey1.DocID))
assert.NoError(t, c2.AttachDocument(docRefKey1.DocID, false))
assert.NoError(t, db.UpdateClientInfoAfterPushPull(ctx, c2, d1))
attached, err = db.IsDocumentAttached(ctx, docRefKey1, "")
assert.NoError(t, err)
Expand Down Expand Up @@ -1012,14 +1012,14 @@ func RunIsDocumentAttachedTest(t *testing.T, db database.Database, projectID typ

// 01. Check if documents are attached after attaching
docRefKey1 := d1.RefKey()
assert.NoError(t, c1.AttachDocument(docRefKey1.DocID))
assert.NoError(t, c1.AttachDocument(docRefKey1.DocID, false))
assert.NoError(t, db.UpdateClientInfoAfterPushPull(ctx, c1, d1))
attached, err := db.IsDocumentAttached(ctx, docRefKey1, "")
assert.NoError(t, err)
assert.True(t, attached)

docRefKey2 := d2.RefKey()
assert.NoError(t, c1.AttachDocument(docRefKey2.DocID))
assert.NoError(t, c1.AttachDocument(docRefKey2.DocID, false))
assert.NoError(t, db.UpdateClientInfoAfterPushPull(ctx, c1, d2))
attached, err = db.IsDocumentAttached(ctx, docRefKey2, "")
assert.NoError(t, err)
Expand Down Expand Up @@ -1061,7 +1061,7 @@ func RunIsDocumentAttachedTest(t *testing.T, db database.Database, projectID typ
assert.False(t, attached)

// 02. Check if document is attached after attaching
assert.NoError(t, c1.AttachDocument(docRefKey1.DocID))
assert.NoError(t, c1.AttachDocument(docRefKey1.DocID, false))
assert.NoError(t, db.UpdateClientInfoAfterPushPull(ctx, c1, d1))
attached, err = db.IsDocumentAttached(ctx, docRefKey1, "")
assert.NoError(t, err)
Expand All @@ -1081,9 +1081,9 @@ func RunIsDocumentAttachedTest(t *testing.T, db database.Database, projectID typ
assert.False(t, attached)

// 04. Check if document is attached after two clients attaching
assert.NoError(t, c1.AttachDocument(docRefKey1.DocID))
assert.NoError(t, c1.AttachDocument(docRefKey1.DocID, false))
assert.NoError(t, db.UpdateClientInfoAfterPushPull(ctx, c1, d1))
assert.NoError(t, c2.AttachDocument(docRefKey1.DocID))
assert.NoError(t, c2.AttachDocument(docRefKey1.DocID, false))
assert.NoError(t, db.UpdateClientInfoAfterPushPull(ctx, c2, d1))
attached, err = db.IsDocumentAttached(ctx, docRefKey1, "")
assert.NoError(t, err)
Expand Down
1 change: 1 addition & 0 deletions server/rpc/connecthelper/status.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ var errorToCode = map[error]connect.Code{
database.ErrClientNotActivated: connect.CodeFailedPrecondition,
database.ErrDocumentNotAttached: connect.CodeFailedPrecondition,
database.ErrDocumentAlreadyAttached: connect.CodeFailedPrecondition,
database.ErrDocumentAlreadyDetached: connect.CodeFailedPrecondition,
documents.ErrDocumentAttached: connect.CodeFailedPrecondition,
packs.ErrInvalidServerSeq: connect.CodeFailedPrecondition,
database.ErrConflictOnUpdate: connect.CodeFailedPrecondition,
Expand Down
2 changes: 1 addition & 1 deletion server/rpc/yorkie_server.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ func (s *yorkieServer) AttachDocument(
return nil, err
}

if err := clientInfo.AttachDocument(docInfo.ID); err != nil {
if err := clientInfo.AttachDocument(docInfo.ID, pack.IsAttached()); err != nil {
return nil, err
}

Expand Down
2 changes: 1 addition & 1 deletion test/bench/push_pull_bench_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ func setUpClientsAndDocs(
assert.NoError(b, err)
docInfo, err := be.DB.FindDocInfoByKeyAndOwner(ctx, clientInfo.RefKey(), docKey, true)
assert.NoError(b, err)
assert.NoError(b, clientInfo.AttachDocument(docInfo.ID))
assert.NoError(b, clientInfo.AttachDocument(docInfo.ID, false))
assert.NoError(b, be.DB.UpdateClientInfoAfterPushPull(ctx, clientInfo, docInfo))

bytesID, _ := clientInfo.ID.Bytes()
Expand Down
32 changes: 26 additions & 6 deletions test/integration/document_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"testing"
"time"

"connectrpc.com/connect"
"github.com/stretchr/testify/assert"

"github.com/yorkie-team/yorkie/client"
Expand Down Expand Up @@ -75,17 +76,35 @@ func TestDocument(t *testing.T) {
assert.Error(t, err)
})

t.Run("detach removeIfNotAttached flag test", func(t *testing.T) {
// 01. create a document and attach it to c1
t.Run("reattach test", func(t *testing.T) {
ctx := context.Background()

// 01. reattach a detached document
doc := document.New(helper.TestDocKey(t))
err := doc.Update(func(root *json.Object, p *presence.Presence) error {
assert.NoError(t, c1.Attach(ctx, doc))
assert.NoError(t, doc.Update(func(root *json.Object, p *presence.Presence) error {
root.SetString("k1", "v1")
return nil
}, "update k1 with v1")
assert.NoError(t, err)
}))
assert.NoError(t, c1.Detach(ctx, doc))
assert.Equal(t, connect.CodeFailedPrecondition, connect.CodeOf(c1.Attach(ctx, doc)))

err = c1.Attach(ctx, doc)
// 02. attach a new document with the same key
doc2 := document.New(helper.TestDocKey(t))
assert.NoError(t, c1.Attach(ctx, doc2))
assert.NoError(t, c1.Detach(ctx, doc2))

// 03. reattach but without updating the document
doc3 := document.New(helper.TestDocKey(t))
assert.NoError(t, c1.Attach(ctx, doc3))
assert.NoError(t, c1.Detach(ctx, doc3))
})

t.Run("detach removeIfNotAttached flag test", func(t *testing.T) {
// 01. create a document and attach it to c1
ctx := context.Background()
doc := document.New(helper.TestDocKey(t))
err := c1.Attach(ctx, doc)
assert.NoError(t, err)
assert.True(t, doc.IsAttached())

Expand All @@ -96,6 +115,7 @@ func TestDocument(t *testing.T) {
assert.Equal(t, doc.Status(), document.StatusDetached)

// 03. attach again to c1 and check if it is attached normally
doc = document.New(helper.TestDocKey(t))
err = c1.Attach(ctx, doc)
assert.NoError(t, err)
assert.True(t, doc.IsAttached())
Expand Down

0 comments on commit 4226417

Please sign in to comment.