CBG-4796 Match Rosmar xattr and tombstone behaviour to Couchbase Server

base/collection_xattr_test.go

@@ -12,6 +12,7 @@ import (
 	"errors"
 	"fmt"
 	"maps"
+	"slices"
 	"testing"
 
 	sgbucket "github.com/couchbase/sg-bucket"
@@ -903,7 +904,7 @@ func TestWriteTombstoneWithXattrs(t *testing.T) {
 
 			body, xattrs, _, err := col.GetWithXattrs(ctx, docID, xattrKeys)
 			require.NoError(t, err)
-			if test.finalBody == nil || UnitTestUrlIsWalrus() {
+			if test.finalBody == nil {
 				require.Equal(t, "", string(body))
 			} else {
 				require.JSONEq(t, string(test.finalBody), string(body))
@@ -1124,9 +1125,7 @@ func TestWriteUpdateWithXattrs(t *testing.T) {
 				return
 			}
 			require.NoError(t, err)
-			if UnitTestUrlIsWalrus() && test.updatedDoc.IsTombstone {
-				require.Equal(t, "", string(body))
-			} else if len(test.finalBody) > 0 {
+			if len(test.finalBody) > 0 {
 				require.JSONEq(t, string(test.finalBody), string(body))
 			} else {
 				require.Equal(t, string(test.finalBody), string(body))
@@ -1959,3 +1958,120 @@ func TestMetadataStoreSubdocStoreWriteOperations(t *testing.T) {
 	require.Error(t, err)
 	require.True(t, IsDocNotFoundError(err))
 }
+
+// TestDeleteWithXattrs tests various combinations of deleting documents (with zero to many xattrs, some of which may not exist) across bucket implementations to ensure consistency in behavior.
+func TestDeleteWithXattrs(t *testing.T) {
+	ctx := TestCtx(t)
+	bucket := GetTestBucket(t)
+	defer bucket.Close(ctx)
+
+	col := bucket.GetSingleDataStore()
+
+	tests := []struct {
+		name           string
+		xattrsValues   map[string][]byte
+		xattrsToDelete []string
+		expectedXattrs []string
+	}{
+		{
+			name:           "delete with no xattrs",
+			xattrsValues:   nil,
+			xattrsToDelete: nil,
+			expectedXattrs: nil,
+		},
+		{
+			name:           "delete one only existing xattr",
+			xattrsValues:   map[string][]byte{"xattr1": []byte(`{"a":"b"}`)},
+			xattrsToDelete: []string{"xattr1"},
+			expectedXattrs: nil,
+		},
+		{
+			name:           "delete two existing xattrs",
+			xattrsValues:   map[string][]byte{"xattr1": []byte(`{"a":"b"}`), "xattr2": []byte(`{"c":"d"}`)},
+			xattrsToDelete: []string{"xattr1", "xattr2"},
+			expectedXattrs: nil,
+		},
+		{
+			name:           "delete one xattr and one non-existing xattr",
+			xattrsValues:   map[string][]byte{"xattr1": []byte(`{"a":"b"}`)},
+			xattrsToDelete: []string{"xattr1", "notexists"},
+			expectedXattrs: nil,
+		},
+		{
+			name:           "delete one non-existing xattr",
+			xattrsValues:   map[string][]byte{"xattr1": []byte(`{"a":"b"}`)},
+			xattrsToDelete: []string{"notexists"},
+			expectedXattrs: nil, // user xattrs get removed along with regular delete...
+		},
+		{
+			name:           "create and delete system xattr",
+			xattrsValues:   map[string][]byte{"_sync": []byte(`{"a":"b"}`)},
+			xattrsToDelete: []string{"_sync"},
+			expectedXattrs: nil,
+		},
+		{
+			name:           "create and delete normal and system xattr",
+			xattrsValues:   map[string][]byte{"xattr1": []byte(`{"a":"b"}`), "_sync": []byte(`{"a":"b"}`)},
+			xattrsToDelete: []string{"_sync"},
+			expectedXattrs: nil, // user xattrs get removed along with regular delete...
+		},
+		{
+			name:           "create normal and system and do regular delete",
+			xattrsValues:   map[string][]byte{"xattr1": []byte(`{"a":"b"}`), "_sync": []byte(`{"a":"b"}`)},
+			xattrsToDelete: nil,
+			expectedXattrs: []string{"_sync"},
+		},
+		{
+			name:           "create normal and system and delete system",
+			xattrsValues:   map[string][]byte{"xattr1": []byte(`{"a":"b"}`), "_sync": []byte(`{"a":"b"}`)},
+			xattrsToDelete: []string{"_sync"},
+			expectedXattrs: nil, // user xattrs get removed along with regular delete...
+		},
+		{
+			name:           "create two system xattrs and delete one",
+			xattrsValues:   map[string][]byte{"_sync": []byte(`{"a":"b"}`), "_globalSync": []byte(`{"a":"b"}`)},
+			xattrsToDelete: []string{"_sync"},
+			expectedXattrs: []string{"_globalSync"},
+		},
+		{
+			name:           "create normal xattr and two system xattrs and delete one system xattr",
+			xattrsValues:   map[string][]byte{"xattr1": []byte(`{"a":"b"}`), "_sync": []byte(`{"a":"b"}`), "_globalSync": []byte(`{"a":"b"}`)},
+			xattrsToDelete: []string{"_sync"},
+			expectedXattrs: []string{"_globalSync"}, // user xattrs get removed along with regular delete...
+		},
+		{
+			name:           "delete non-existing system xattr on doc with no xattrs",
+			xattrsValues:   nil,
+			xattrsToDelete: []string{"_systemXattr"},
+			expectedXattrs: nil,
+		},
+		{
+			name:           "create xattr and delete non-existing system xattr",
+			xattrsValues:   map[string][]byte{"xattr1": []byte(`{"a":"b"}`)},
+			xattrsToDelete: []string{"_sync"},
+			expectedXattrs: nil, // user xattrs get removed along with regular delete...
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			docID := t.Name()
+
+			_, err := col.WriteWithXattrs(ctx, docID, 0, 0, []byte(`{"foo": "bar"}`), test.xattrsValues, nil, nil)
+			require.NoError(t, err)
+
+			err = col.DeleteWithXattrs(ctx, docID, test.xattrsToDelete)
+			require.NoError(t, err)
+
+			v, xv, _, err := col.GetWithXattrs(ctx, docID, slices.Collect(maps.Keys(test.xattrsValues)))
+			if len(test.expectedXattrs) == 0 {
+				assert.Errorf(t, err, "Expected document and all xattrs to be deleted, but it still exists (no error on get)")
+				assert.Truef(t, IsDocNotFoundError(err), "Expected document to be deleted, but got an error other than not found: %v", err)
+			} else {
+				require.NoErrorf(t, err, "Expected document or at least one xattr to still exist with xattrs after deletion")
+			}
+			assert.Nil(t, v, "Expected document to be deleted, but it still exists")
+			assert.Equal(t, test.expectedXattrs, slices.Collect(maps.Keys(xv)), "Expected xattrs to match expected values after deletion")
+		})
+	}
+}

db/attachment_test.go

@@ -28,6 +28,7 @@ import (
 	"github.com/couchbase/sync_gateway/channels"
 	"github.com/couchbase/sync_gateway/testing/assert"
 	"github.com/couchbase/sync_gateway/testing/require"
+	"github.com/couchbase/sync_gateway/testing/sgtest"
 )
 
 func unmarshalBody(t *testing.T, j string) Body {
@@ -2014,7 +2015,7 @@ func TestWritePathsPreserveUnmigratedAttachmentMetadata(t *testing.T) {
 			_, err := collection.UpdateSyncFun(ctx, `function(doc){channel("preResync");}`)
 			require.NoError(t, err)
 
-			docID := SafeDocumentName(t, t.Name())
+			docID := sgtest.SafeDocumentName(t, t.Name())
 			var rev1ID string
 			if tc.create != nil {
 				rev1ID = tc.create(t, ctx, collection, docID)

db/delta_cache_test.go

@@ -19,6 +19,7 @@ import (
 	"github.com/couchbase/sync_gateway/channels"
 	"github.com/couchbase/sync_gateway/testing/assert"
 	"github.com/couchbase/sync_gateway/testing/require"
+	"github.com/couchbase/sync_gateway/testing/sgtest"
 )
 
 // TestDeltaCacheStats verifies that DeltaCacheNumItems, DeltaCacheHit and DeltaCacheMiss
@@ -44,7 +45,7 @@ func TestDeltaCacheStats(t *testing.T) {
 
 	collection, ctx := GetSingleDatabaseCollectionWithUser(ctx, t, db)
 
-	docID := SafeDocumentName(t, t.Name())
+	docID := sgtest.SafeDocumentName(t, t.Name())
 	rev1ID, doc1, err := collection.Put(ctx, docID, Body{"foo": "bar"})
 	require.NoError(t, err)
 	rev1CV := doc1.HLV.GetCurrentVersionString()
@@ -97,7 +98,7 @@ func TestDeltaCacheStatsWithBypassRevisionCache(t *testing.T) {
 
 	collection, ctx := GetSingleDatabaseCollectionWithUser(ctx, t, db)
 
-	docID := SafeDocumentName(t, t.Name())
+	docID := sgtest.SafeDocumentName(t, t.Name())
 	rev1ID, doc1, err := collection.Put(ctx, docID, Body{"foo": "bar"})
 	require.NoError(t, err)
 	rev1CV := doc1.HLV.GetCurrentVersionString()
@@ -148,7 +149,7 @@ func TestNumberBasedEvictionForDeltaCache(t *testing.T) {
 
 	collection, ctx := GetSingleDatabaseCollectionWithUser(ctx, t, db)
 
-	docID := SafeDocumentName(t, t.Name())
+	docID := sgtest.SafeDocumentName(t, t.Name())
 	_, doc, err := collection.Put(ctx, docID, Body{"test": "data"})
 	require.NoError(t, err)
 
@@ -201,7 +202,7 @@ func TestUpdateDeltaWhenNoDeltaCacheInit(t *testing.T) {
 	defer db.Close(ctx)
 	collection, ctx := GetSingleDatabaseCollectionWithUser(ctx, t, db)
 
-	docID := SafeDocumentName(t, t.Name())
+	docID := sgtest.SafeDocumentName(t, t.Name())
 	rev1ID, doc1, err := collection.Put(ctx, docID, Body{"foo": "bar"})
 	require.NoError(t, err)
 	rev1CV := doc1.HLV.GetCurrentVersionString()
@@ -292,7 +293,7 @@ func TestGetDeltaStaleChannelAfterUserXattrUpdate(t *testing.T) {
 	}
 
 	t.Run("CV", func(t *testing.T) {
-		docID := SafeDocumentName(t, t.Name()+"CV")
+		docID := sgtest.SafeDocumentName(t, t.Name()+"CV")
 		rev1ID, doc1, err := collection.Put(ctx, docID, Body{"channels": []string{channelNew}, "foo": "bar"})
 		require.NoError(t, err)
 		rev1CV := doc1.HLV.GetCurrentVersionString()
@@ -305,7 +306,7 @@ func TestGetDeltaStaleChannelAfterUserXattrUpdate(t *testing.T) {
 	})
 
 	t.Run("RevID", func(t *testing.T) {
-		docID := SafeDocumentName(t, t.Name()+"revID")
+		docID := sgtest.SafeDocumentName(t, t.Name()+"revID")
 		rev1ID, _, err := collection.Put(ctx, docID, Body{"channels": []string{channelNew}, "foo": "bar"})
 		require.NoError(t, err)
 
@@ -341,7 +342,7 @@ func TestGetDeltaThreeRevisions(t *testing.T) {
 
 	collection, ctx := GetSingleDatabaseCollectionWithUser(ctx, t, db)
 
-	docID := SafeDocumentName(t, t.Name())
+	docID := sgtest.SafeDocumentName(t, t.Name())
 	rev1ID, doc1, err := collection.Put(ctx, docID, Body{"foo": "bar"})
 	require.NoError(t, err)
 	rev1CV := doc1.HLV.GetCurrentVersionString()
@@ -425,7 +426,7 @@ func TestAccessCheckOnNonCurrentRevision(t *testing.T) {
 	collection, ctx := GetSingleDatabaseCollectionWithUser(ctx, t, db)
 	collection.ChannelMapper = channels.NewChannelMapper(ctx, channels.DocChannelsSyncFunction, db.Options.JavascriptTimeout)
 
-	docID := SafeDocumentName(t, t.Name())
+	docID := sgtest.SafeDocumentName(t, t.Name())
 	rev1ID, doc1, err := collection.Put(ctx, docID, Body{"channels": "A"})
 	require.NoError(t, err)
 	rev1CV := doc1.HLV.GetCurrentVersionString()

db/import_test.go

@@ -23,6 +23,7 @@ import (
 	"time"
 
 	"github.com/couchbase/sync_gateway/testing/require"
+	"github.com/couchbase/sync_gateway/testing/sgtest"
 
 	sgbucket "github.com/couchbase/sg-bucket"
 	"github.com/couchbase/sync_gateway/base"
@@ -113,7 +114,7 @@ func TestFeedImport(t *testing.T) {
 	}
 	for _, testCase := range testCases {
 		t.Run(testCase.name, func(t *testing.T) {
-			docID := SafeDocumentName(t, t.Name())
+			docID := sgtest.SafeDocumentName(t, t.Name())
 			db.CachedCCVEnabled.Store(testCase.eccv)
 			for vBucket := range db.numVBuckets {
 				db.CachedCCVStartingCas.Store(base.VBNo(vBucket), testCase.startingCAS)
@@ -326,7 +327,7 @@ func TestOnDemandImport(t *testing.T) {
 	}
 	for _, testCase := range testCases {
 		t.Run(testCase.name, func(t *testing.T) {
-			docID := SafeDocumentName(t, t.Name())
+			docID := sgtest.SafeDocumentName(t, t.Name())
 			db.CachedCCVEnabled.Store(testCase.eccv)
 			for vBucket := range db.numVBuckets {
 				db.CachedCCVStartingCas.Store(base.VBNo(vBucket), testCase.startingCAS)
@@ -1458,7 +1459,7 @@ func TestImportWithSyncCVAndNoVV(t *testing.T) {
 	defer db.Close(ctx)
 
 	collection, ctx := GetSingleDatabaseCollectionWithUser(ctx, t, db)
-	docID := SafeDocumentName(t, t.Name())
+	docID := sgtest.SafeDocumentName(t, t.Name())
 
 	_, doc, err := collection.Put(ctx, docID, Body{"foo": "baz"})
 	require.NoError(t, err)
@@ -2034,7 +2035,7 @@ func TestOnDemandImportMouRetargetedByConcurrentWrite(t *testing.T) {
 	defer dbc.Close(ctx)
 	collection, ctx := GetSingleDatabaseCollectionWithUser(ctx, t, dbc)
 
-	docID := SafeDocumentName(t, t.Name())
+	docID := sgtest.SafeDocumentName(t, t.Name())
 	_, _, err := collection.Put(ctx, docID, Body{"value": 1234})
 	require.NoError(t, err)
 	_, _, cas, err := collection.dataStore.GetWithXattrs(ctx, docID, []string{base.SyncXattrName})
@@ -2122,7 +2123,7 @@ func TestMouChainEndedByBodyWrite(t *testing.T) {
 			defer dbc.Close(ctx)
 			collection, ctx := GetSingleDatabaseCollectionWithUser(ctx, t, dbc)
 
-			docID := SafeDocumentName(t, t.Name())
+			docID := sgtest.SafeDocumentName(t, t.Name())
 			_, _, err := collection.Put(ctx, docID, Body{"value": 1234})
 			require.NoError(t, err)
 
@@ -2160,7 +2161,7 @@ func TestUserXattrImportMou(t *testing.T) {
 	defer dbc.Close(ctx)
 	collection, ctx := GetSingleDatabaseCollectionWithUser(ctx, t, dbc)
 
-	docID := SafeDocumentName(t, t.Name())
+	docID := sgtest.SafeDocumentName(t, t.Name())
 	_, doc, err := collection.Put(ctx, docID, Body{"value": 1234})
 	require.NoError(t, err)
 	revBeforeImport := doc.GetRevTreeID()
@@ -2228,7 +2229,7 @@ func TestDuplicateImportLeavesMouAlone(t *testing.T) {
 			defer dbc.Close(ctx)
 			collection, ctx := GetSingleDatabaseCollectionWithUser(ctx, t, dbc)
 
-			docID := SafeDocumentName(t, t.Name())
+			docID := sgtest.SafeDocumentName(t, t.Name())
 			_, doc, err := collection.Put(ctx, docID, Body{"value": 1234})
 			require.NoError(t, err)
 
@@ -2404,7 +2405,7 @@ func TestWriteWithPreImportExistingDoc(t *testing.T) {
 				defer dbCtx.Close(ctx)
 				collection, ctx := GetSingleDatabaseCollectionWithUser(ctx, t, dbCtx)
 
-				docID := SafeDocumentName(t, t.Name())
+				docID := sgtest.SafeDocumentName(t, t.Name())
 				prior.setup(t, ctx, collection, docID)
 
 				// the read that imports, and hands back the snapshot it read before importing
@@ -2449,7 +2450,7 @@ func TestOnDemandImportOfSupersededUserXattrWrite(t *testing.T) {
 	defer dbCtx.Close(ctx)
 	collection, ctx := GetSingleDatabaseCollectionWithUser(ctx, t, dbCtx)
 
-	docID := SafeDocumentName(t, t.Name())
+	docID := sgtest.SafeDocumentName(t, t.Name())
 	rev, doc, err := collection.Put(ctx, docID, Body{"foo": "sync gateway"})
 	require.NoError(t, err)
 	_, err = collection.dataStore.UpdateXattrs(ctx, docID, 0, doc.Cas, map[string][]byte{userXattrKey: []byte(`{"channels": ["a"]}`)}, nil)

db/revision_cache_test.go

@@ -24,6 +24,7 @@ import (
 	"github.com/couchbase/sync_gateway/base"
 	"github.com/couchbase/sync_gateway/testing/assert"
 	"github.com/couchbase/sync_gateway/testing/require"
+	"github.com/couchbase/sync_gateway/testing/sgtest"
 )
 
 // testBackingStore always returns an empty doc at rev:"1-abc" in channel "*" except for docs not in 'notFoundDocIDs'
@@ -2566,7 +2567,7 @@ func TestItemResidentInCacheBackupRevLoaded(t *testing.T) {
 			defer db.Close(ctx)
 			collection, ctx := GetSingleDatabaseCollectionWithUser(ctx, t, db)
 
-			docID := SafeDocumentName(t, t.Name())
+			docID := sgtest.SafeDocumentName(t, t.Name())
 			var firstRevisionID, firstCV string
 			_, doc1, err := collection.Put(ctx, docID, Body{"foo": "bar"})
 			require.NoError(t, err)
@@ -2639,8 +2640,8 @@ func TestMultipleRevCacheItemsForSameDocs(t *testing.T) {
 
 	collection, ctx := GetSingleDatabaseCollectionWithUser(ctx, t, db)
 
-	docID := SafeDocumentName(t, t.Name())
-	docID2 := SafeDocumentName(t, t.Name()+"_2")
+	docID := sgtest.SafeDocumentName(t, t.Name())
+	docID2 := sgtest.SafeDocumentName(t, t.Name()+"_2")
 
 	_, doc1, err := collection.Put(ctx, docID, Body{"foo": "bar"})
 	require.NoError(t, err)

db/revtree_test.go

@@ -24,6 +24,7 @@ import (
 	"github.com/couchbase/sync_gateway/base"
 	"github.com/couchbase/sync_gateway/testing/assert"
 	"github.com/couchbase/sync_gateway/testing/require"
+	"github.com/couchbase/sync_gateway/testing/sgtest"
 )
 
 // testmap is a revtree with a single branch
@@ -2574,7 +2575,7 @@ func TestInvalidRevTreeRepairMou(t *testing.T) {
 			collection, ctx := GetSingleDatabaseCollectionWithUser(ctx, t, dbCtx)
 			invalidRevTreeCount := dbCtx.DbStats.Database().InvalidRevTreeCount
 
-			docID := SafeDocumentName(t, t.Name())
+			docID := sgtest.SafeDocumentName(t, t.Name())
 			bodyCas, bodyRevSeqNo := tc.setup(t, ctx, collection, docID)
 			require.NotZero(t, bodyRevSeqNo)
 			dbCtx.FlushRevisionCacheForTest()

db/util_testing.go

@@ -1163,17 +1163,6 @@ func GetChangeEntryCV(t *testing.T, entry *ChangeEntry) Version {
 	return Version{}
 }
 
-// SafeDocumentName returns a document name free of any special characters for use in tests.
-func SafeDocumentName(t *testing.T, name string) string {
-	t.Helper()
-	docName := strings.ToLower(name)
-	for _, c := range []string{" ", "<", ">", "/", "="} {
-		docName = strings.ReplaceAll(docName, c, "_")
-	}
-	require.Less(t, len(docName), 251, "Document name %s is too long, must be less than 251 characters", name)
-	return docName
-}
-
 // WaitForPendingChanges blocks until the change-cache has caught up with the latest writes to the database. Fails the
 // test harness if cache does not catch up.
 func (db *DatabaseContext) WaitForPendingChanges(t testing.TB) {