Repository navigation
Match Couchbase Server for tombstones, expiry and nil writes - #101
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Three unresolved findings remain, including one critical error-handling issue.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Aligns rosmar with Couchbase Server semantics for tombstones, expiry, CAS, xattrs, subdocuments, and nil writes.
Changes:
- Updates deletion, resurrection, CAS, expiry, and nil-write behavior.
- Adjusts xattr and subdocument handling.
- Expands regression and compatibility test coverage.
| File | Reviewed changes | Findings |
|---|---|---|
xattrs_test.go |
Adds xattr, tombstone, and update coverage. | None |
collection+xattrs.go |
Updates xattr operations, expiry, tombstone handling, and virtual metadata. | Critical error propagation issue in DeleteWithXattrs; moderate missing $document.deleted subpath support. |
collection+subdoc.go |
Handles subdocument writes over tombstones. | None |
collection.go |
Adjusts CAS, deletion, resurrection, nil writes, and update behavior. | Moderate MissingError retry issue in Update. |
collection_test.go |
Adds broad behavior and expiry coverage. | None |
bucket_test.go |
Extends expiration tests. | None |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
bbrks
left a comment
There was a problem hiding this comment.
repro test covering nil/empty byte slices (run same test cases against cb server to compare behaviour - test as-suggested should match cb server/gocb)
one edge case around xattr-only write
| }) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
| // TestNilWritesStoreBody checks that Set, SetRaw, Add, AddRaw and WriteWithXattrs with a nil value write | |
| // a live document, as Couchbase Server does: an untyped nil writes a null JSON body and a nil []byte writes an empty body. | |
| func TestNilWritesStoreBody(t *testing.T) { | |
| ctx := t.Context() | |
| dataStore := makeTestBucket(t).DefaultDataStore(ctx).(*Collection) | |
| testCases := []struct { | |
| name string | |
| live bool // write over a live doc with a system and a user xattr | |
| write func(t *testing.T, key string) | |
| expectedBody string | |
| }{ | |
| { | |
| name: "Set", | |
| write: func(t *testing.T, key string) { require.NoError(t, dataStore.Set(ctx, key, 0, nil, nil)) }, | |
| expectedBody: "null", | |
| }, | |
| { | |
| name: "SetOverLive", | |
| live: true, | |
| write: func(t *testing.T, key string) { require.NoError(t, dataStore.Set(ctx, key, 0, nil, nil)) }, | |
| expectedBody: "null", | |
| }, | |
| { | |
| name: "SetRaw", | |
| write: func(t *testing.T, key string) { require.NoError(t, dataStore.SetRaw(ctx, key, 0, nil, nil)) }, | |
| }, | |
| { | |
| name: "Add", | |
| write: func(t *testing.T, key string) { | |
| added, err := dataStore.Add(ctx, key, 0, nil) | |
| require.NoError(t, err) | |
| require.True(t, added) | |
| }, | |
| expectedBody: "null", | |
| }, | |
| { | |
| name: "AddRaw", | |
| write: func(t *testing.T, key string) { | |
| added, err := dataStore.AddRaw(ctx, key, 0, nil) | |
| require.NoError(t, err) | |
| require.True(t, added) | |
| }, | |
| }, | |
| { | |
| name: "WriteWithXattrsNilBody", | |
| write: func(t *testing.T, key string) { | |
| _, err := dataStore.WriteWithXattrs(ctx, key, 0, 0, nil, map[string][]byte{"_sync": []byte(`{"rev":"1-a"}`)}, nil, nil) | |
| require.NoError(t, err) | |
| }, | |
| expectedBody: "null", | |
| }, | |
| } | |
| for _, tc := range testCases { | |
| t.Run(tc.name, func(t *testing.T) { | |
| key := t.Name() | |
| if tc.live { | |
| _, err := dataStore.WriteWithXattrs(ctx, key, 0, 0, []byte(`{"foo":"bar"}`), | |
| map[string][]byte{"_sync": []byte(`{"rev":"1-a"}`), "user": []byte(`{"u":1}`)}, nil, nil) | |
| require.NoError(t, err) | |
| } | |
| tc.write(t, key) | |
| assert.False(t, getDocRow(t, dataStore, key).tombstone, "nil write created a tombstone") | |
| body, _, err := dataStore.GetRaw(ctx, key) | |
| require.NoError(t, err) | |
| require.Equal(t, tc.expectedBody, string(body)) | |
| if tc.live { | |
| xattrs, _, err := dataStore.GetXattrs(ctx, key, []string{"_sync", "user"}) | |
| require.NoError(t, err) | |
| require.JSONEq(t, `{"rev":"1-a"}`, string(xattrs["_sync"])) | |
| require.JSONEq(t, `{"u":1}`, string(xattrs["user"])) | |
| } | |
| }) | |
| } | |
| } | |
There was a problem hiding this comment.
Added unchanged in f8e7228. Set/Add with nil now store null, and SetRaw/AddRaw store an empty body. Before, rosmar stored a NULL value, which Get read as missing.
Each change below was checked against Couchbase Server 8.0.1 with a matching test in sync_gateway's base package. Deletes and tombstone writes: - Treat a tombstone as missing in Delete, Remove and DeleteSubDocPaths, and a CAS of 0 as no CAS check in Remove, RemoveXattrs and WriteTombstoneWithXattrs with deleteBody. - Return not found from RemoveXattrs on a missing key. - Remove user xattrs in DeleteWithXattrs. When an xattr is missing, delete only the body of a live document and leave a tombstone unchanged. - Keep the body in WriteTombstoneWithXattrs without deleteBody. CAS writes and xattr updates: - Return not found from WriteCas and WriteWithXattrs for a non-zero CAS on a tombstone, and retry that in WriteUpdateWithXattrs. - Resurrect a tombstone with a CAS of 0 in WriteWithXattrs. - Ignore the tombstone CAS in WriteSubDoc, as an upsert does. - Return a CAS mismatch from WriteResurrectionWithXattrs on a live document. - Create a missing document with an empty body in SetXattrs. - Require an existing document in UpdateXattrs, and treat a CAS of 0 as no CAS check. - Keep the tombstone flag in step with the body after every write. Expiry: - Delete, Remove, DeleteWithXattrs and an Update delete leave a tombstone with no expiry. - A write over a tombstone with PreserveExpiry (Set, WriteWithXattrs, WriteResurrectionWithXattrs) has no expiry. - WriteUpdateWithXattrs uses its exp argument. A local variable hid it, so only UpdatedDoc.Expiry was applied. - $document has exptime and deleted fields. Nil values: - WriteCas never creates a tombstone. An untyped nil writes a null body, and a nil []byte writes an empty body, with or without Raw. - Update reads a tombstone with a CAS of 0 and inserts over it or over a missing key, even for a delete. A nil callback value writes an empty body. Tests: - The tests that match sync_gateway have the same names and cases in both repos: TestDataStoreOperationErrors, TestDeleteAfterTombstoneResurrection, TestDeleteWithXattrs, TestDeleteWithXattrsMissingXattr, TestTombstoneExpiry, TestTombstoneFlagMatchesBody, TestTombstoneRemovesUserXattrs, TestUpdateByDocState, TestWriteCasNil, TestWriteTombstoneWithXattrsKeepBody, TestWriteUpdateWithXattrsDocDeletedDuringUpdate, TestWriteWithXattrsResurrectTombstoneZeroCas and TestXattrOnlyWriteKeepsTombstone. - TestExpirationSkipsTombstones expects a tombstone expiry of 0 from the delete paths. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Like Couchbase Server, Set and Add with a nil value write a null body, SetRaw and AddRaw write an empty body, and an xattr-only insert writes a null body. None of them stores a NULL value that reads as missing. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Stacked on #99.
Makes rosmar match Couchbase Server for tombstones, expiry and nil writes. Each behavior has a sync_gateway test with the same name and cases. That test passes on Couchbase Server 8.0.1 and on this branch.
What changes
expargument, which a local variable hid.null, and a nil[]bytewrites an empty body. Update inserts over a missing key or a tombstone, even for a delete, as the gocb version in sync_gateway does.Testing
go test -race ./...andgolangci-lintpass.basepasses against this branch on rosmar.auth,dbandrestpass against the same changes with Serialize OpenBucket and stop expiration before closing the bucket #100 applied.🤖 Generated with Claude Code