Closed hackerwins closed 2 weeks ago
[!WARNING]
Review failed
The pull request is closed.
The changes primarily add an IsAttached()
method to the Pack
struct, update the AttachDocument
method in ClientInfo
to accept an alreadyAttached
parameter, and handle new error mappings related to document attachment states. These modifications enhance the backend's capability to manage document attachment status by refining method signatures, adjusting error handling, and updating test cases accordingly.
Files/Groups | Change Summary |
---|---|
pkg/document/change/pack.go |
Added IsAttached() method to Pack struct. |
server/backend/database/client_info.go |
Added ErrDocumentAlreadyDetached and updated AttachDocument method to include alreadyAttached parameter. |
server/backend/database/client_info_test.go |
Updated AttachDocument method signature in various test cases to include alreadyAttached parameter. |
server/backend/database/testcases/testcases.go |
Added alreadyAttached parameter to AttachDocument calls within multiple test functions. |
server/rpc/connecthelper/status.go |
Added error mapping for database.ErrDocumentAlreadyDetached to connect.CodeFailedPrecondition . |
server/rpc/yorkie_server.go |
Updated AttachDocument method to use pack.IsAttached() when calling clientInfo.AttachDocument . |
test/bench/push_pull_bench_test.go |
Updated AttachDocument call to include alreadyAttached parameter in the setUpClientsAndDocs function. |
test/integration/document_test.go |
Added new imports and refactored test cases involving document attachment, detachment, and reattachment logic. |
A document's dance in data's embrace,
By Pack's new steps and Client's retrace,
Attached or not, a query so wise,
With bugs now fleeing, the tests comply.
In backend's realm, where code does bloom,
Improvements shine, dispelling gloom. ✨
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
What this PR does / why we need it:
Return ErrAlreadyDetached when reattaching
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.
Which issue(s) this PR fixes:
Addresses #904
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation:
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests