From 017f158d87013a1ea6941f0c3bdaddc8edf05155 Mon Sep 17 00:00:00 2001 From: Tor Colvin Date: Thu, 8 Oct 2026 11:47:33 -0400 Subject: [PATCH] Treat a path mismatch in DeleteWithXattrs like a missing xattr Like Couchbase Server, a path through a scalar xattr value fails the combined delete in the same way as a missing xattr. On a live document, only the body is deleted. On a tombstone, the error is ErrPathMismatch, or not found when every path fails. Co-Authored-By: Claude Opus 5.5 (1M context) --- collection+xattrs.go | 37 +++++++++-------- collection_test.go | 95 ++++++++++++++++++++++++++++++++++---------- 2 files changed, 95 insertions(+), 37 deletions(-) diff --git a/collection+xattrs.go b/collection+xattrs.go index 2955feb..e86f3cb 100644 --- a/collection+xattrs.go +++ b/collection+xattrs.go @@ -595,17 +595,17 @@ func (c *Collection) DeleteWithXattrs(ctx context.Context, key string, xattrKeys if err != nil { return nil, remapKeyError(err, key) } - // Like Couchbase Server, a missing xattr fails the combined delete, leaving only the body delete on a live doc. - missing, err := missingSubDocPaths(e.xattrs, xattrKeys) + // Like Couchbase Server, a failed xattr path fails the combined delete, leaving only the body delete on a live doc. + failed, failedErr, err := failedSubDocPaths(e.xattrs, xattrKeys) if err != nil { return nil, err } - if len(missing) > 0 { + if len(failed) > 0 { if !bodyExists { - if len(missing) == len(xattrKeys) { + if len(failed) == len(xattrKeys) { return nil, sgbucket.MissingError{Key: key} } - return nil, fmt.Errorf("%v: %w", missing, sgbucket.ErrPathNotFound) + return nil, fmt.Errorf("%v: %w", failed, failedErr) } } else if e.xattrs, err = deleteSubDocPaths(e.xattrs, xattrKeys...); err != nil { return nil, err @@ -909,21 +909,24 @@ func deleteSubDocPaths(rawXattrs []byte, paths ...string) ([]byte, error) { return rawXattrs, nil } -// missingSubDocPaths returns the paths that are not present in the raw xattrs JSON. -func missingSubDocPaths(rawXattrs []byte, paths []string) (missing []string, err error) { - for _, path := range paths { - found := false - processXattrs(rawXattrs, func(xattrs semiParsedXattrs) { - found, err = deleteNestedKey(xattrs, strings.Split(path, ".")) - }) - if err != nil { - return nil, err +// failedSubDocPaths returns the paths that a Couchbase Server subdoc remove fails on, and the error for the first of +// them: ErrPathNotFound for a missing path, or ErrPathMismatch for a path through a value that is not a JSON object. +func failedSubDocPaths(rawXattrs []byte, paths []string) (failed []string, firstErr error, err error) { + var xattrs map[string]any + if len(rawXattrs) > 0 { + if err := json.Unmarshal(rawXattrs, &xattrs); err != nil { + return nil, nil, err } - if !found { - missing = append(missing, path) + } + for _, path := range paths { + if _, pathErr := evalSubdocPath(xattrs, strings.Split(path, ".")); pathErr != nil { + failed = append(failed, path) + if firstErr == nil { + firstErr = pathErr + } } } - return missing, nil + return failed, firstErr, nil } // deleteSubDocPath removes a single subdoc path from the raw xattrs JSON. diff --git a/collection_test.go b/collection_test.go index a904a0b..9e77b1f 100644 --- a/collection_test.go +++ b/collection_test.go @@ -1517,10 +1517,10 @@ func TestDataStoreOperationErrors(t *testing.T) { // remaining document, when it is asked to delete an xattr that the document does not have. func TestDeleteWithXattrsMissingXattr(t *testing.T) { const ( - success = "success" - notFound = "not found" - otherError = "other error" - missingName = "_missingxattr" + success = "success" + notFound = "not found" + pathNotFound = "path not found" + missingName = "_missingxattr" ) ctx := t.Context() ensureNoLeaks(t) @@ -1565,7 +1565,7 @@ func TestDeleteWithXattrsMissingXattr(t *testing.T) { tombstone: true, xattrs: []string{"_xattr1"}, deleteXattrs: []string{"_xattr1", missingName}, - expected: otherError, + expected: pathNotFound, expectedRemaining: []string{"_xattr1"}, }, { @@ -1603,9 +1603,8 @@ func TestDeleteWithXattrsMissingXattr(t *testing.T) { require.NoError(t, err) case notFound: require.ErrorAs(t, err, &sgbucket.MissingError{}) - case otherError: - require.Error(t, err) - require.NotErrorAs(t, err, &sgbucket.MissingError{}) + case pathNotFound: + require.ErrorIs(t, err, sgbucket.ErrPathNotFound) } _, _, err = coll.GetRaw(ctx, key) @@ -1622,25 +1621,81 @@ func TestDeleteWithXattrsMissingXattr(t *testing.T) { } } -// TestDeleteWithXattrsPathMismatch checks that a path through a scalar xattr value fails without deleting the body. +// TestDeleteWithXattrsPathMismatch checks that DeleteWithXattrs treats a path through a scalar xattr like a missing +// xattr, as Couchbase Server does. func TestDeleteWithXattrsPathMismatch(t *testing.T) { + const ( + success = "success" + notFound = "not found" + mismatch = "mismatch" + ) ctx := t.Context() ensureNoLeaks(t) coll := makeTestBucket(t).DefaultDataStore(ctx) - key := t.Name() - body := []byte(`{"foo":"bar"}`) - cas, err := coll.WriteWithXattrs(ctx, key, 0, 0, body, map[string][]byte{"_sync": []byte(`"scalar"`)}, nil, nil) - require.NoError(t, err) + testCases := []struct { + name string + tombstone bool + xattrs map[string][]byte + deleteXattrs []string + expected string + }{ + { + name: "live doc, scalar xattr", + xattrs: map[string][]byte{"_sync": []byte(`"scalar"`)}, + deleteXattrs: []string{"_sync.rev"}, + expected: success, + }, + { + name: "live doc, scalar xattr and present xattr", + xattrs: map[string][]byte{"_sync": []byte(`"scalar"`), "_xattr1": []byte(`{"seq":1}`)}, + deleteXattrs: []string{"_sync.rev", "_xattr1"}, + expected: success, + }, + { + name: "tombstone, scalar xattr", + tombstone: true, + xattrs: map[string][]byte{"_sync": []byte(`"scalar"`)}, + deleteXattrs: []string{"_sync.rev"}, + expected: notFound, + }, + { + name: "tombstone, scalar xattr and present xattr", + tombstone: true, + xattrs: map[string][]byte{"_sync": []byte(`"scalar"`), "_xattr1": []byte(`{"seq":1}`)}, + deleteXattrs: []string{"_sync.rev", "_xattr1"}, + expected: mismatch, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + key := t.Name() + cas, err := coll.WriteWithXattrs(ctx, key, 0, 0, []byte(`{"foo":"bar"}`), tc.xattrs, nil, nil) + require.NoError(t, err) + if tc.tombstone { + _, err = coll.Remove(ctx, key, cas) + require.NoError(t, err) + } - err = coll.DeleteWithXattrs(ctx, key, []string{"_sync.rev"}) - require.ErrorIs(t, err, sgbucket.ErrPathMismatch) + err = coll.DeleteWithXattrs(ctx, key, tc.deleteXattrs) + switch tc.expected { + case success: + require.NoError(t, err) + case notFound: + require.ErrorAs(t, err, &sgbucket.MissingError{}) + case mismatch: + require.ErrorIs(t, err, sgbucket.ErrPathMismatch) + } - rawBody, xattrs, getCas, err := coll.GetWithXattrs(ctx, key, []string{"_sync"}) - require.NoError(t, err) - require.Equal(t, cas, getCas) - require.JSONEq(t, string(body), string(rawBody)) - require.JSONEq(t, `"scalar"`, string(xattrs["_sync"])) + _, _, err = coll.GetRaw(ctx, key) + require.ErrorAs(t, err, &sgbucket.MissingError{}) + for xattrName, value := range tc.xattrs { + xattrs, _, err := coll.GetXattrs(ctx, key, []string{xattrName}) + require.NoError(t, err) + require.JSONEq(t, string(value), string(xattrs[xattrName])) + } + }) + } } // TestWriteCasNil checks that a nil WriteCas value never creates a tombstone: an untyped nil writes a null JSON body