From 21f3b3f8ac675e4e9eb641272b0aa88fb8c8ca13 Mon Sep 17 00:00:00 2001 From: Ben Brooks Date: Wed, 20 Aug 2025 14:28:12 +0100 Subject: [PATCH 01/10] Allow CV to be used in OCC writes (Document Update/Delete and the associated attachment endpoints) using existing `rev` parameter - which can automatically detect RevTreeID/CV. Switch test helpers to use CV in writes when available for coverage, and allow fallback to RevTreeID. Prevent CV OCC value in doc updates for docs in conflict or against non-current versions, since we don't maintain linear version history like we do for RevTrees and can't correlate an old CV with a branched revision ID. --- base/util.go | 15 ++ base/util_test.go | 26 +++ db/blip_handler.go | 2 + db/changes_test.go | 2 +- db/crud.go | 74 +++++-- db/crud_test.go | 2 +- db/database_test.go | 10 +- db/document.go | 58 +++++ db/utilities_hlv_testing.go | 54 ----- docs/api/components/parameters.yaml | 6 +- docs/api/components/schemas.yaml | 13 +- docs/api/paths/admin/keyspace-_bulk_docs.yaml | 4 +- docs/api/paths/admin/keyspace-_changes.yaml | 4 +- .../paths/admin/keyspace-_config-sync.yaml | 2 +- .../paths/admin/keyspace-_local-docid.yaml | 8 +- docs/api/paths/admin/keyspace-_raw-docid.yaml | 2 +- docs/api/paths/admin/keyspace-_revs_diff.yaml | 2 +- docs/api/paths/admin/keyspace-docid.yaml | 8 +- .../api/paths/public/keyspace-_bulk_docs.yaml | 4 +- docs/api/paths/public/keyspace-_changes.yaml | 2 +- .../paths/public/keyspace-_local-docid.yaml | 2 +- docs/api/paths/public/keyspace-docid.yaml | 6 +- rest/api_test.go | 42 ++++ rest/bootstrap_test.go | 3 +- rest/doc_api.go | 209 ++++++++++++++---- rest/utilities_testing.go | 29 ++- rest/utilities_testing_resttester.go | 40 +++- topologytest/sync_gateway_peer_test.go | 8 +- 28 files changed, 458 insertions(+), 179 deletions(-) diff --git a/base/util.go b/base/util.go index e3fbd889b5..47fa8502bc 100644 --- a/base/util.go +++ b/base/util.go @@ -1825,3 +1825,18 @@ func KeysPresent[K comparable, V any](m map[K]V, keys []K) []K { } return result } + +// IsRevTreeID checks if the string looks like a RevTree ID. +func IsRevTreeID(s string) bool { + // If we scan forwards past each digit until we hit `-`, we know this is a RevTree ID. + for i, r := range s { + if r >= '0' && r <= '9' { + continue + } + if r == '-' && i > 0 { + return true + } + break + } + return false +} diff --git a/base/util_test.go b/base/util_test.go index 4209c94dcf..925962a57c 100644 --- a/base/util_test.go +++ b/base/util_test.go @@ -1854,3 +1854,29 @@ func TestKeysPresent(t *testing.T) { }) } } + +func TestIsRevTreeID(t *testing.T) { + tests := []struct { + value string + expected bool + }{ + {"1-abc", true}, + {"1-abc123", true}, + {"1-abc123def456", true}, + {"1-abc123def456ghi789", true}, + {"1-abc123def456ghi789jkl012", true}, + {"123-abc123", true}, + {"1234567890-a", true}, + {"0-a", true}, + {"1", false}, + {"abc", false}, + {"a-abc", false}, + {"-abc", false}, + {"1a@bc", false}, + } + for _, tt := range tests { + t.Run(tt.value, func(t *testing.T) { + assert.Equalf(t, tt.expected, IsRevTreeID(tt.value), "IsRevTreeID(%v)", tt.value) + }) + } +} diff --git a/db/blip_handler.go b/db/blip_handler.go index 292f6b9d50..117b78fa14 100644 --- a/db/blip_handler.go +++ b/db/blip_handler.go @@ -852,6 +852,7 @@ func (bh *blipHandler) handleProposeChanges(rq *blip.Message) error { changeIsVector := false if versionVectorProtocol { + // TODO: CBG-4812 Use base.IsRevTreeID changeIsVector = strings.Contains(rev, "@") } if versionVectorProtocol && changeIsVector { @@ -1094,6 +1095,7 @@ func (bh *blipHandler) processRev(rq *blip.Message, stats *processRevStats) (err var incomingHLV *HybridLogicalVector // Build history/HLV var legacyRevList []string + // TODO: CBG-4812 Use base.IsRevTreeID changeIsVector := strings.Contains(rev, "@") if !bh.useHLV() || !changeIsVector { newDoc.RevID = rev diff --git a/db/changes_test.go b/db/changes_test.go index b2bc8d2d19..f294d3a7b7 100644 --- a/db/changes_test.go +++ b/db/changes_test.go @@ -418,7 +418,7 @@ func TestActiveOnlyCacheUpdate(t *testing.T) { // Tombstone 5 documents for i := 2; i <= 6; i++ { key := fmt.Sprintf("%s_%d", t.Name(), i) - _, _, err = collection.DeleteDoc(ctx, key, revId) + _, _, err = collection.DeleteDoc(ctx, key, DocVersion{RevTreeID: revId}) require.NoError(t, err, "Couldn't delete document") } diff --git a/db/crud.go b/db/crud.go index bc952f04ad..5fd73053c2 100644 --- a/db/crud.go +++ b/db/crud.go @@ -310,13 +310,13 @@ func (db *DatabaseCollectionWithUser) Get1xBody(ctx context.Context, docid strin } // Get Rev with all-or-none history based on specified 'history' flag -func (db *DatabaseCollectionWithUser) Get1xRevBody(ctx context.Context, docid, revid string, history bool, attachmentsSince []string) (Body, error) { +func (db *DatabaseCollectionWithUser) Get1xRevBody(ctx context.Context, docid, revOrCV string, history bool, attachmentsSince []string) (Body, error) { maxHistory := 0 if history { maxHistory = math.MaxInt32 } - return db.Get1xRevBodyWithHistory(ctx, docid, revid, Get1xRevBodyOptions{ + return db.Get1xRevBodyWithHistory(ctx, docid, revOrCV, Get1xRevBodyOptions{ MaxHistory: maxHistory, HistoryFrom: nil, AttachmentsSince: attachmentsSince, @@ -333,8 +333,8 @@ type Get1xRevBodyOptions struct { } // Retrieves rev with request history specified as collection of revids (historyFrom) -func (db *DatabaseCollectionWithUser) Get1xRevBodyWithHistory(ctx context.Context, docid, revtreeid string, opts Get1xRevBodyOptions) (Body, error) { - rev, err := db.getRev(ctx, docid, revtreeid, opts.MaxHistory, opts.HistoryFrom) +func (db *DatabaseCollectionWithUser) Get1xRevBodyWithHistory(ctx context.Context, docid, revOrCV string, opts Get1xRevBodyOptions) (Body, error) { + rev, err := db.getRev(ctx, docid, revOrCV, opts.MaxHistory, opts.HistoryFrom) if err != nil { return nil, err } @@ -1093,7 +1093,11 @@ func (db *DatabaseCollectionWithUser) Put(ctx context.Context, docid string, bod generation++ delete(body, BodyRev) - // Not extracting it yet because we need this property around to generate a rev ID + // remove CV before RevTreeID generation + matchCV, _ := body[BodyCV].(string) + delete(body, BodyCV) + + // Not extracting it yet because we need this property around to generate a RevTreeID deleted, _ := body[BodyDeleted].(bool) expiry, err := body.ExtractExpiry() @@ -1146,21 +1150,42 @@ func (db *DatabaseCollectionWithUser) Put(ctx context.Context, docid string, bod } var conflictErr error - // Make sure matchRev matches an existing leaf revision: - if matchRev == "" { - matchRev = doc.CurrentRev - if matchRev != "" { - // PUT with no parent rev given, but there is an existing current revision. - // This is OK as long as the current one is deleted. - if !doc.History[matchRev].Deleted { - conflictErr = base.HTTPErrorf(http.StatusConflict, "Document exists") - } else { - generation, _ = ParseRevID(ctx, matchRev) - generation++ + + // OCC check of matchCV against CV on the doc + if matchCV != "" { + if matchCV == doc.HLV.GetCurrentVersionString() { + // set matchRev to the current revision ID and allow existing codepaths to perform RevTree-based update. + matchRev = doc.CurrentRev + // bump generation based on retrieved RevTree ID + generation, _ = ParseRevID(ctx, matchRev) + generation++ + } else if doc.hasFlag(channels.Conflict | channels.Hidden) { + // Can't use CV as an OCC Value when a document is in conflict, or we're updating the non-winning leaf + // There's no way to get from a given old CV to a RevTreeID to perform the update correctly, since we don't maintain linear history for a given SourceID. + // Reject the request and force the user to resolve the conflict using RevTree IDs which does have linear history available. + conflictErr = base.HTTPErrorf(http.StatusBadRequest, "Cannot use CV to modify a document in conflict - resolve first with RevTree ID") + } else { + conflictErr = base.HTTPErrorf(http.StatusConflict, "Document revision conflict") + } + } + + if conflictErr == nil { + // Make sure matchRev matches an existing leaf revision: + if matchRev == "" { + matchRev = doc.CurrentRev + if matchRev != "" { + // PUT with no parent rev given, but there is an existing current revision. + // This is OK as long as the current one is deleted. + if !doc.History[matchRev].Deleted { + conflictErr = base.HTTPErrorf(http.StatusConflict, "Document exists") + } else { + generation, _ = ParseRevID(ctx, matchRev) + generation++ + } } + } else if !doc.History.isLeaf(matchRev) || db.IsIllegalConflict(ctx, doc, matchRev, deleted, false, nil) { + conflictErr = base.HTTPErrorf(http.StatusConflict, "Document revision conflict") } - } else if !doc.History.isLeaf(matchRev) || db.IsIllegalConflict(ctx, doc, matchRev, deleted, false, nil) { - conflictErr = base.HTTPErrorf(http.StatusConflict, "Document revision conflict") } // Make up a new _rev, and add it to the history: @@ -2893,7 +2918,8 @@ func (db *DatabaseCollectionWithUser) MarkPrincipalsChanged(ctx context.Context, // Creates a new document, assigning it a random doc ID. func (db *DatabaseCollectionWithUser) Post(ctx context.Context, body Body) (docid string, rev string, doc *Document, err error) { - if body[BodyRev] != nil { + // This error isn't very accurate, you just _cannot_ use POST to update an existing document - even if it does exist. We don't even bother checking for existence. + if body[BodyRev] != nil || body[BodyCV] != nil { return "", "", nil, base.HTTPErrorf(http.StatusNotFound, "No previous revision to replace") } @@ -2914,8 +2940,13 @@ func (db *DatabaseCollectionWithUser) Post(ctx context.Context, body Body) (doci } // Deletes a document, by adding a new revision whose _deleted property is true. -func (db *DatabaseCollectionWithUser) DeleteDoc(ctx context.Context, docid string, revid string) (string, *Document, error) { - body := Body{BodyDeleted: true, BodyRev: revid} +func (db *DatabaseCollectionWithUser) DeleteDoc(ctx context.Context, docid string, docVersion DocVersion) (string, *Document, error) { + body := Body{BodyDeleted: true} + if !docVersion.CV.IsEmpty() { + body[BodyCV] = docVersion.CV.String() + } else { + body[BodyRev] = docVersion.RevTreeID + } newRevID, doc, err := db.Put(ctx, docid, body) return newRevID, doc, err } @@ -3342,6 +3373,7 @@ func (db *DatabaseCollectionWithUser) CheckProposedVersion(ctx context.Context, // previousRev may be revTreeID or version var previousVersion Version previousRevFormat := "version" + // TODO: CBG-4812 Use base.IsRevTreeID if !strings.Contains(previousRev, "@") { previousRevFormat = "revTreeID" } diff --git a/db/crud_test.go b/db/crud_test.go index ea71228307..6d81191a8b 100644 --- a/db/crud_test.go +++ b/db/crud_test.go @@ -1158,7 +1158,7 @@ func TestGet1xRevAndChannels(t *testing.T) { assert.Equal(t, []interface{}{"a"}, revisions[RevisionsIds]) // Delete the document, creating tombstone revision rev3 - rev3, _, err := collection.DeleteDoc(ctx, docId, rev2) + rev3, _, err := collection.DeleteDoc(ctx, docId, DocVersion{RevTreeID: rev2}) require.NoError(t, err) bodyBytes, removed, err = collection.get1xRevFromDoc(ctx, doc2, rev3, true) assert.False(t, removed) diff --git a/db/database_test.go b/db/database_test.go index 59fdbf0473..615275af6f 100644 --- a/db/database_test.go +++ b/db/database_test.go @@ -731,7 +731,7 @@ func TestGetDeleted(t *testing.T) { rev1id, _, err := collection.Put(ctx, "doc1", body) assert.NoError(t, err, "Put") - rev2id, _, err := collection.DeleteDoc(ctx, "doc1", rev1id) + rev2id, _, err := collection.DeleteDoc(ctx, "doc1", DocVersion{RevTreeID: rev1id}) assert.NoError(t, err, "DeleteDoc") // Get the deleted doc with its history; equivalent to GET with ?revs=true @@ -1395,7 +1395,7 @@ func TestAllDocsOnly(t *testing.T) { } // Now delete one document and try again: - _, _, err = collection.DeleteDoc(ctx, ids[23].DocID, ids[23].RevID) + _, _, err = collection.DeleteDoc(ctx, ids[23].DocID, DocVersion{RevTreeID: ids[23].RevID}) assert.NoError(t, err, "Couldn't delete doc 23") alldocs, err = allDocIDs(ctx, collection.DatabaseCollection) @@ -1726,7 +1726,7 @@ func TestConflicts(t *testing.T) { ) // Delete 2-b; verify this makes 2-a current: - rev3, _, err := collection.DeleteDoc(ctx, "doc", "2-b") + rev3, _, err := collection.DeleteDoc(ctx, "doc", DocVersion{RevTreeID: "2-b"}) assert.NoError(t, err, "delete 2-b") rawBody, _, _ = collection.dataStore.GetRaw("doc") @@ -3369,7 +3369,7 @@ func TestTombstoneCompactionStopWithManager(t *testing.T) { docID := fmt.Sprintf("doc%d", i) rev, _, err := collection.Put(ctx, docID, Body{}) assert.NoError(t, err) - _, _, err = collection.DeleteDoc(ctx, docID, rev) + _, _, err = collection.DeleteDoc(ctx, docID, DocVersion{RevTreeID: rev}) assert.NoError(t, err) } @@ -3780,7 +3780,7 @@ func TestImportCompactPanic(t *testing.T) { // Create a document, then delete it, to create a tombstone rev, doc, err := collection.Put(ctx, "test", Body{}) require.NoError(t, err) - _, _, err = collection.DeleteDoc(ctx, doc.ID, rev) + _, _, err = collection.DeleteDoc(ctx, doc.ID, DocVersion{RevTreeID: rev}) require.NoError(t, err) require.NoError(t, collection.WaitForPendingChanges(ctx)) diff --git a/db/document.go b/db/document.go index f4e60f8ac2..694149e934 100644 --- a/db/document.go +++ b/db/document.go @@ -19,6 +19,7 @@ import ( "math" "net/http" "strconv" + "strings" "time" sgbucket "github.com/couchbase/sg-bucket" @@ -1484,3 +1485,60 @@ func (doc *Document) ExtractDocVersion() DocVersion { CV: *doc.HLV.ExtractCurrentVersionFromHLV(), } } + +// DocVersion represents a specific version of a document in an revID/HLV agnostic manner. +type DocVersion struct { + RevTreeID string + CV Version +} + +// String implements fmt.Stringer +func (v DocVersion) String() string { + return fmt.Sprintf("RevTreeID:%s,CV:%#v", v.RevTreeID, v.CV) +} + +// GoString implements fmt.GoStringer +func (v DocVersion) GoString() string { + return fmt.Sprintf("DocVersion{RevTreeID:%s,CV:%#v}", v.RevTreeID, v.CV) +} + +// DocVersionRevTreeEqual compares two DocVersions based on their RevTreeID. +func (v DocVersion) DocVersionRevTreeEqual(o DocVersion) bool { + if v.RevTreeID != o.RevTreeID { + return false + } + return true +} + +// GetRev returns the revision ID for the DocVersion. +func (v DocVersion) GetRev(useHLV bool) string { + if useHLV { + if v.CV.SourceID == "" { + return "" + } + return v.CV.String() + } else { + return v.RevTreeID + } +} + +// RevIDGeneration returns the Rev ID generation for the current version +func (v *DocVersion) RevIDGeneration() int { + if v == nil { + return 0 + } + gen, err := strconv.ParseInt(strings.Split(v.RevTreeID, "-")[0], 10, 64) + if err != nil { + base.AssertfCtx(context.TODO(), "Error parsing generation from rev ID %q: %v", v.RevTreeID, err) + return 0 + } + return int(gen) +} + +// RevIDDigest returns the Rev ID digest for the current version +func (v *DocVersion) RevIDDigest() string { + if v == nil { + return "" + } + return strings.Split(v.RevTreeID, "-")[1] +} diff --git a/db/utilities_hlv_testing.go b/db/utilities_hlv_testing.go index f90b82d87d..5397a4bb90 100644 --- a/db/utilities_hlv_testing.go +++ b/db/utilities_hlv_testing.go @@ -12,7 +12,6 @@ package db import ( "context" - "fmt" "strconv" "strings" "testing" @@ -22,59 +21,6 @@ import ( "github.com/stretchr/testify/require" ) -// DocVersion represents a specific version of a document in an revID/HLV agnostic manner. -type DocVersion struct { - RevTreeID string - CV Version -} - -func (v DocVersion) String() string { - return fmt.Sprintf("RevTreeID:%s,CV:%#v", v.RevTreeID, v.CV) -} - -func (v DocVersion) GoString() string { - return fmt.Sprintf("DocVersion{RevTreeID:%s,CV:%#v}", v.RevTreeID, v.CV) -} - -func (v DocVersion) DocVersionRevTreeEqual(o DocVersion) bool { - if v.RevTreeID != o.RevTreeID { - return false - } - return true -} - -func (v DocVersion) GetRev(useHLV bool) string { - if useHLV { - if v.CV.SourceID == "" { - return "" - } - return v.CV.String() - } else { - return v.RevTreeID - } -} - -// RevIDGeneration returns the Rev ID generation for the current version -func (v *DocVersion) RevIDGeneration() int { - if v == nil { - return 0 - } - gen, err := strconv.ParseInt(strings.Split(v.RevTreeID, "-")[0], 10, 64) - if err != nil { - base.AssertfCtx(context.TODO(), "Error parsing generation from rev ID %q: %v", v.RevTreeID, err) - return 0 - } - return int(gen) -} - -// RevIDDigest returns the Rev ID digest for the current version -func (v *DocVersion) RevIDDigest() string { - if v == nil { - return "" - } - return strings.Split(v.RevTreeID, "-")[1] -} - // HLVAgent performs HLV updates directly (not via SG) for simulating/testing interaction with non-SG HLV agents type HLVAgent struct { t *testing.T diff --git a/docs/api/components/parameters.yaml b/docs/api/components/parameters.yaml index bc1431cf8b..24ab42881c 100644 --- a/docs/api/components/parameters.yaml +++ b/docs/api/components/parameters.yaml @@ -13,13 +13,13 @@ DB-config-If-Match: schema: type: string description: 'If set to a configuration''s Etag value, enables optimistic concurrency control for the request. Returns HTTP 412 if another update happened underneath this one.' -If-Match: +Document-If-Match: name: If-Match in: header required: false schema: type: string - description: The revision ID to target. + description: An optimistic concurrency control (OCC) value used to prevent conflicts. Use the value returned in the ETag response header of the GET request for the resource being updated, or the latest known Revision Tree ID or Current Version of the document. Include-channels: name: channels in: query @@ -393,7 +393,7 @@ show_cv: required: false schema: type: boolean - description: Output the current version of the version vector in the response as property `_cv`. + description: Output the Current Version in the response as property `_cv`. startkey: name: startkey in: query diff --git a/docs/api/components/schemas.yaml b/docs/api/components/schemas.yaml index 194904f03d..39e968a9b6 100644 --- a/docs/api/components/schemas.yaml +++ b/docs/api/components/schemas.yaml @@ -494,7 +494,7 @@ Document: description: Prefix number for the latest revision. type: number ids: - description: 'Array of valid revision IDs, in reverse order (latest first).' + description: 'Array of valid Revision Tree IDs, in reverse order (latest first).' type: array items: type: string @@ -522,7 +522,10 @@ New-revision: description: Whether the request completed successfully. type: boolean rev: - description: The revision of the document. + description: The Revision Tree ID of the document. + type: string + cv: + description: The CV of the document. type: string required: - id @@ -558,9 +561,9 @@ Changes-feed: - type: object properties: cv: - description: The HLV Current Version associated with the change. This value requires the `version_type` preference set to `cv` and this document revision being written through SG 4.0+ + description: The Current Version associated with the change. This value requires the `version_type` preference set to `cv` and this document revision being written through SG 4.0+ type: string - title: HLV Current Version + title: Current Version uniqueItems: true uniqueItems: true last_seq: @@ -708,7 +711,7 @@ Replication: default: |- In priority order, this will cause - Deletes to always win (the delete with the longest revision history wins if both revisions are deletes) - - The revision with the longest revision history to win. This means the the revision with the most changes and therefore the highest revision ID will win. + - The revision with the longest revision history to win. This means the the revision with the most changes and therefore the highest Revision Tree ID will win. localWins: This will result in local revisions always being the winner in any conflict. remoteWins: This will result in remote revisions always being the winner in any conflict. custom: This will result in conflicts going through your own custom conflict resolver. You must provide this logic as a Javascript function in the `custom_conflict_resolver` parameter. diff --git a/docs/api/paths/admin/keyspace-_bulk_docs.yaml b/docs/api/paths/admin/keyspace-_bulk_docs.yaml index f209b6c039..dfa9834a3c 100644 --- a/docs/api/paths/admin/keyspace-_bulk_docs.yaml +++ b/docs/api/paths/admin/keyspace-_bulk_docs.yaml @@ -14,9 +14,9 @@ post: To create a new document, simply add the body in an object under `docs`. A doc ID will be generated by Sync Gateway unless `_id` is specified. - To update an existing document, provide the document ID (`_id`) and revision ID (`_rev`) as well as the new body values. + To update an existing document, provide the document ID (`_id`) and Revision Tree ID (`_rev`) as well as the new body values. - To delete an existing document, provide the document ID (`_id`), revision ID (`_rev`), and set the deletion flag (`_deleted`) to true. + To delete an existing document, provide the document ID (`_id`), Revision Tree ID (`_rev`), and set the deletion flag (`_deleted`) to true. Required Sync Gateway RBAC roles: diff --git a/docs/api/paths/admin/keyspace-_changes.yaml b/docs/api/paths/admin/keyspace-_changes.yaml index 9f328fb4ac..c19a89889f 100644 --- a/docs/api/paths/admin/keyspace-_changes.yaml +++ b/docs/api/paths/admin/keyspace-_changes.yaml @@ -113,7 +113,7 @@ get: - cv x-enumDescriptions: rev: 'Revision Tree IDs. For example: 1-293a80ce8f4874724732f27d35b3959a13cd96e0' - cv: 'HLV Current Version. For example: 1854e4e557cc0000@zTWkmBiYZgNQo7BHVZrB/Q' + cv: 'Current Version. For example: 1854e4e557cc0000@zTWkmBiYZgNQo7BHVZrB/Q' responses: '200': $ref: ../../components/responses.yaml#/changes-feed @@ -187,7 +187,7 @@ post: - cv x-enumDescriptions: rev: 'Revision Tree IDs. For example: 1-293a80ce8f4874724732f27d35b3959a13cd96e0' - cv: 'HLV Current Version. For example: 1854e4e557cc0000@zTWkmBiYZgNQo7BHVZrB/Q' + cv: 'Current Version. For example: 1854e4e557cc0000@zTWkmBiYZgNQo7BHVZrB/Q' responses: '200': $ref: ../../components/responses.yaml#/changes-feed diff --git a/docs/api/paths/admin/keyspace-_config-sync.yaml b/docs/api/paths/admin/keyspace-_config-sync.yaml index bb67438873..f3e8de8b79 100644 --- a/docs/api/paths/admin/keyspace-_config-sync.yaml +++ b/docs/api/paths/admin/keyspace-_config-sync.yaml @@ -87,7 +87,7 @@ delete: * Sync Gateway Architect parameters: - - $ref: ../../components/parameters.yaml#/If-Match + - $ref: ../../components/parameters.yaml#/Document-If-Match responses: '200': description: Successfully reset the sync function diff --git a/docs/api/paths/admin/keyspace-_local-docid.yaml b/docs/api/paths/admin/keyspace-_local-docid.yaml index 5866d75784..604e56eee8 100644 --- a/docs/api/paths/admin/keyspace-_local-docid.yaml +++ b/docs/api/paths/admin/keyspace-_local-docid.yaml @@ -37,7 +37,7 @@ get: put: summary: Upsert a local document description: |- - This request creates or updates a local document. Updating a local document requires that the revision ID be put in the body under `_rev`. + This request creates or updates a local document. Updating a local document requires that the Revision Tree ID be put in the body under `_rev`. Local document IDs are given a `_local/` prefix. Local documents are not replicated or indexed, don't support attachments, and don't save revision histories. In practice they are almost only used by the client's replicator, as a place to store replication checkpoint data. @@ -67,7 +67,7 @@ put: '404': $ref: ../../components/responses.yaml#/Not-found '409': - description: A revision ID conflict would result from updating this document revision. + description: A conflict would result from updating this document revision. tags: - Document operationId: put_keyspace-_local-docid @@ -84,7 +84,7 @@ delete: parameters: - name: rev in: query - description: The revision ID of the revision to delete. + description: The Revision Tree ID of the revision to delete. required: true schema: type: string @@ -96,7 +96,7 @@ delete: '404': $ref: ../../components/responses.yaml#/Not-found '409': - description: A revision ID conflict would result from deleting this document revision. + description: A conflict would result from deleting this document revision. tags: - Document operationId: delete_keyspace-_local-docid diff --git a/docs/api/paths/admin/keyspace-_raw-docid.yaml b/docs/api/paths/admin/keyspace-_raw-docid.yaml index 0e90774f31..e45d0ac2fb 100644 --- a/docs/api/paths/admin/keyspace-_raw-docid.yaml +++ b/docs/api/paths/admin/keyspace-_raw-docid.yaml @@ -62,7 +62,7 @@ get: nullable: true additionalProperties: true _vv: - description: Version vector for the document. + description: Version Vector for the document. type: object nullable: true additionalProperties: true diff --git a/docs/api/paths/admin/keyspace-_revs_diff.yaml b/docs/api/paths/admin/keyspace-_revs_diff.yaml index cd40e1c129..0b7418a4e7 100644 --- a/docs/api/paths/admin/keyspace-_revs_diff.yaml +++ b/docs/api/paths/admin/keyspace-_revs_diff.yaml @@ -10,7 +10,7 @@ parameters: post: summary: Compare revisions to what is in the database description: |- - Takes a set of document IDs, each with a set of revision IDs. For each document, an array of unknown revisions are returned with an array of known revisions that may be recent ancestors. + Takes a set of document IDs, each with a set of Revision Tree IDs. For each document, an array of unknown revisions are returned with an array of known revisions that may be recent ancestors. Required Sync Gateway RBAC roles: diff --git a/docs/api/paths/admin/keyspace-docid.yaml b/docs/api/paths/admin/keyspace-docid.yaml index 6c0743c461..90293933a7 100644 --- a/docs/api/paths/admin/keyspace-docid.yaml +++ b/docs/api/paths/admin/keyspace-docid.yaml @@ -43,7 +43,7 @@ get: description: The ID of the document. type: string _rev: - description: The revision ID of the document. + description: The Revision Tree ID of the document. type: string additionalProperties: true example: @@ -69,7 +69,7 @@ get: put: summary: Upsert a document description: |- - This will upsert a document meaning if it does not exist, then it will be created. Otherwise a new revision will be made for the existing document. A revision ID must be provided if targetting an existing document. + This will upsert a document, meaning if it does not exist then it will be created. Otherwise a new revision will be made for the existing document. A previous known version must be provided if targeting an existing document to prevent conflicts. A document ID must be specified for this endpoint. To let Sync Gateway generate the ID, use the `POST /{db}/` endpoint. @@ -85,7 +85,7 @@ put: - $ref: ../../components/parameters.yaml#/replicator2 - $ref: ../../components/parameters.yaml#/new_edits - $ref: ../../components/parameters.yaml#/rev - - $ref: ../../components/parameters.yaml#/If-Match + - $ref: ../../components/parameters.yaml#/Document-If-Match requestBody: content: application/json: @@ -126,7 +126,7 @@ delete: * Sync Gateway Application parameters: - $ref: ../../components/parameters.yaml#/rev - - $ref: ../../components/parameters.yaml#/If-Match + - $ref: ../../components/parameters.yaml#/Document-If-Match responses: '200': $ref: ../../components/responses.yaml#/New-revision diff --git a/docs/api/paths/public/keyspace-_bulk_docs.yaml b/docs/api/paths/public/keyspace-_bulk_docs.yaml index 32c836655d..c0f7d1df4f 100644 --- a/docs/api/paths/public/keyspace-_bulk_docs.yaml +++ b/docs/api/paths/public/keyspace-_bulk_docs.yaml @@ -14,9 +14,9 @@ post: To create a new document, simply add the body in an object under `docs`. A doc ID will be generated by Sync Gateway unless `_id` is specified. - To update an existing document, provide the document ID (`_id`) and revision ID (`_rev`) as well as the new body values. + To update an existing document, provide the document ID (`_id`) and Revision Tree ID (`_rev`) as well as the new body values. - To delete an existing document, provide the document ID (`_id`), revision ID (`_rev`), and set the deletion flag (`_deleted`) to true. + To delete an existing document, provide the document ID (`_id`), Revision Tree ID (`_rev`), and set the deletion flag (`_deleted`) to true. requestBody: content: application/json: diff --git a/docs/api/paths/public/keyspace-_changes.yaml b/docs/api/paths/public/keyspace-_changes.yaml index 019f52f7ba..6a1a348f9d 100644 --- a/docs/api/paths/public/keyspace-_changes.yaml +++ b/docs/api/paths/public/keyspace-_changes.yaml @@ -101,7 +101,7 @@ get: - cv x-enumDescriptions: rev: 'Revision Tree IDs. For example: 1-293a80ce8f4874724732f27d35b3959a13cd96e0' - cv: 'HLV Current Version. For example: 1854e4e557cc0000@zTWkmBiYZgNQo7BHVZrB/Q' + cv: 'Current Version. For example: 1854e4e557cc0000@zTWkmBiYZgNQo7BHVZrB/Q' responses: '200': $ref: ../../components/responses.yaml#/changes-feed diff --git a/docs/api/paths/public/keyspace-_local-docid.yaml b/docs/api/paths/public/keyspace-_local-docid.yaml index 5917b03ac6..7cb1431458 100644 --- a/docs/api/paths/public/keyspace-_local-docid.yaml +++ b/docs/api/paths/public/keyspace-_local-docid.yaml @@ -57,7 +57,7 @@ put: '404': $ref: ../../components/responses.yaml#/Not-found '409': - description: A revision ID conflict would result from updating this document revision. + description: A conflict would result from updating this document revision. tags: - Document operationId: put_keyspace-_local-docid diff --git a/docs/api/paths/public/keyspace-docid.yaml b/docs/api/paths/public/keyspace-docid.yaml index fd5e4945ec..5e408fe38c 100644 --- a/docs/api/paths/public/keyspace-docid.yaml +++ b/docs/api/paths/public/keyspace-docid.yaml @@ -40,7 +40,7 @@ get: description: The revision ID of the document. type: string _cv: - description: The current version of version vector of the document. + description: The Current Version of the document. type: string additionalProperties: true example: @@ -78,7 +78,7 @@ put: - $ref: ../../components/parameters.yaml#/replicator2 - $ref: ../../components/parameters.yaml#/new_edits - $ref: ../../components/parameters.yaml#/rev - - $ref: ../../components/parameters.yaml#/If-Match + - $ref: ../../components/parameters.yaml#/Document-If-Match requestBody: content: application/json: @@ -115,7 +115,7 @@ delete: A revision ID either in the header or on the query parameters is required. parameters: - $ref: ../../components/parameters.yaml#/rev - - $ref: ../../components/parameters.yaml#/If-Match + - $ref: ../../components/parameters.yaml#/Document-If-Match responses: '200': $ref: ../../components/responses.yaml#/New-revision diff --git a/rest/api_test.go b/rest/api_test.go index c53a8986fc..73ce33da72 100644 --- a/rest/api_test.go +++ b/rest/api_test.go @@ -3244,3 +3244,45 @@ func TestHLVUpdateOnRevReplicatorPut(t *testing.T) { assert.Equal(t, base.HexCasToUint64(syncData.Cas), syncData.HLV.Version) assert.Equal(t, base.HexCasToUint64(syncData.Cas), syncData.HLV.CurrentVersionCAS) } + +func TestDocCRUDWithCV(t *testing.T) { + rt := NewRestTester(t, nil) + defer rt.Close() + + const docID = "doc1" + createVersion := rt.PutDoc(docID, `{"create":true}`) + + getDocVersion, _ := rt.GetDoc(docID) + require.Equal(t, createVersion, getDocVersion) + + updateVersion := rt.UpdateDoc(docID, createVersion, `{"update":true}`) + require.NotEqual(t, createVersion, updateVersion) + assert.Greaterf(t, updateVersion.CV.Value, createVersion.CV.Value, "Expected CV Value to be bumped on update") + assert.Greaterf(t, updateVersion.RevIDGeneration(), createVersion.RevIDGeneration(), "Expected revision generation to be bumped on update") + + getDocVersion, _ = rt.GetDoc(docID) + require.Equal(t, updateVersion, getDocVersion) + + // fetch by CV (using the first create version to test cache retrieval) + resp := rt.SendAdminRequest(http.MethodGet, fmt.Sprintf("/{{.keyspace}}/%s?rev=%s", docID, createVersion.CV.String()), "") + RequireStatus(t, resp, http.StatusOK) + resp.DumpBody() + assert.NotContains(t, resp.BodyString(), `"update":true`) + assert.Contains(t, resp.BodyString(), `"create":true`) + assert.Contains(t, resp.BodyString(), `"_cv":"`+createVersion.CV.String()+`"`) + assert.Contains(t, resp.BodyString(), `"_rev":"`+createVersion.RevTreeID+`"`) + + // fetch by CV - updated version + resp = rt.SendAdminRequest(http.MethodGet, fmt.Sprintf("/{{.keyspace}}/%s?rev=%s", docID, updateVersion.CV.String()), "") + RequireStatus(t, resp, http.StatusOK) + resp.DumpBody() + assert.NotContains(t, resp.BodyString(), `"create":true`) + assert.Contains(t, resp.BodyString(), `"update":true`) + assert.Contains(t, resp.BodyString(), `"_cv":"`+updateVersion.CV.String()+`"`) + assert.Contains(t, resp.BodyString(), `"_rev":"`+updateVersion.RevTreeID+`"`) + + deleteVersion := rt.DeleteDoc(docID, updateVersion) + require.NotEqual(t, updateVersion, deleteVersion) + assert.Greaterf(t, deleteVersion.CV.Value, updateVersion.CV.Value, "Expected CV Value to be bumped on delete") + assert.Greaterf(t, deleteVersion.RevIDGeneration(), updateVersion.RevIDGeneration(), "Expected revision generation to be bumped on delete") +} diff --git a/rest/bootstrap_test.go b/rest/bootstrap_test.go index 189b8a94cd..1f5fc40936 100644 --- a/rest/bootstrap_test.go +++ b/rest/bootstrap_test.go @@ -75,7 +75,8 @@ func TestBootstrapRESTAPISetup(t *testing.T) { resp = BootstrapAdminRequest(t, sc, http.MethodGet, "/db1/doc1", ``) resp.RequireStatus(http.StatusNotFound) resp = BootstrapAdminRequest(t, sc, http.MethodPut, "/db1/doc1", `{"foo":"bar"}`) - resp.RequireResponse(http.StatusCreated, `{"id":"doc1","ok":true,"rev":"1-cd809becc169215072fd567eebd8b8de"}`) + resp.RequireStatus(http.StatusCreated) + require.Contains(t, resp.Body, `"id":"doc1","ok":true`) resp = BootstrapAdminRequest(t, sc, http.MethodGet, "/db1/doc1", ``) resp.RequireStatus(http.StatusOK) assert.Contains(t, resp.Body, `"foo":"bar"`) diff --git a/rest/doc_api.go b/rest/doc_api.go index 0204a0fd81..6fc1986a49 100644 --- a/rest/doc_api.go +++ b/rest/doc_api.go @@ -331,6 +331,58 @@ func (h *handler) handleGetAttachment() error { } +// getOCCValue retrieves the optimistic concurrency control value for a document (Revision ID or CV) from several possible sources: +// - Query parameter "rev" +// - If-Match header +// - Body field "_rev" or "_cv" (if the body is provided in the request) +// It also validates that the provided OCC value exactly matches the corresponding body field if it exists in multiple places. +func (h *handler) getOCCValue(optionalBody db.Body) (occValue string, occValueType occVersionType, err error) { + // skipBodyMatchValidation can skip the validation of the occValue inside the body, since in some cases we're pulling that value out of the body anyway. + var skipBodyMatchValidation bool + + // occValue is the optimistic concurrency control value, which can be either the current rev ID or CV - used to prevent lost updates. + // we grab occValue from either query param, Etag header, or request body (in that order) + if revQuery := h.getQuery("rev"); revQuery != "" { + occValue = revQuery + occValueType = guessOCCVersionTypeFromValue(occValue) + } else if ifMatch, err := h.getEtag("If-Match"); err != nil { + return "", 0, err + } else if ifMatch != "" { + occValue = ifMatch + occValueType = guessOCCVersionTypeFromValue(occValue) + } else if bodyCV, ok := optionalBody[db.BodyCV]; ok { + if bodyCVStr, ok := bodyCV.(string); ok { + occValue = bodyCVStr + occValueType = VersionTypeCV + skipBodyMatchValidation = true + } + } else if bodyRev, ok := optionalBody[db.BodyRev]; ok { + if bodyRevStr, ok := bodyRev.(string); ok { + occValue = bodyRevStr + occValueType = VersionTypeRevTreeID + skipBodyMatchValidation = true + } + } + + // ensure the value provided matches exactly the one that may also be supplied in the body + if !skipBodyMatchValidation { + switch occValueType { + case VersionTypeRevTreeID: + if optionalBody[db.BodyRev] != nil && occValue != optionalBody[db.BodyRev] { + return "", 0, base.HTTPErrorf(http.StatusBadRequest, "Revision IDs provided do not match") + } + case VersionTypeCV: + if optionalBody[db.BodyCV] != nil && occValue != optionalBody[db.BodyCV] { + return "", 0, base.HTTPErrorf(http.StatusBadRequest, "CVs provided do not match") + } + default: + return "", 0, base.HTTPErrorf(http.StatusBadRequest, "Unknown version type provided: %q", occValue) + } + } + + return occValue, occValueType, nil +} + // HTTP handler for a PUT of an attachment func (h *handler) handlePutAttachment() error { @@ -344,30 +396,27 @@ func (h *handler) handlePutAttachment() error { if attachmentContentType == "" { attachmentContentType = "application/octet-stream" } - revid := h.getQuery("rev") - if revid == "" { - var err error - revid, err = h.getEtag("If-Match") - if err != nil { - return err - } + + occValue, occValueType, err := h.getOCCValue(nil) + if err != nil { + return err } + attachmentData, err := h.readBody() if err != nil { return err } - body, err := h.collection.Get1xRevBody(h.ctx(), docid, revid, false, nil) + body, err := h.collection.Get1xRevBody(h.ctx(), docid, occValue, false, nil) if err != nil { if base.IsDocNotFoundError(err) { - // couchdb creates empty body on attachment PUT - // for non-existent doc id - body = db.Body{db.BodyRev: revid} + // couchdb creates empty body on attachment PUT for non-existent doc id + body = db.Body{bodyKeyForOCCVersionType(occValueType): occValue} } else if err != nil { return err } } else if body != nil { - if revid == "" { + if occValue == "" { // If a revid is not specified and an active revision was found, // return a conflict now, rather than letting db.Put do it further down... return base.HTTPErrorf(http.StatusConflict, "Cannot modify attachments without a specific rev ID") @@ -389,13 +438,13 @@ func (h *handler) handlePutAttachment() error { attachments[attachmentName] = attachment body[db.BodyAttachments] = attachments - newRev, _, err := h.collection.Put(h.ctx(), docid, body) + newRev, doc, err := h.collection.Put(h.ctx(), docid, body) if err != nil { return err } h.setEtag(newRev) - h.writeRawJSONStatus(http.StatusCreated, []byte(`{"id":`+base.ConvertToJSONString(docid)+`,"ok":true,"rev":"`+newRev+`"}`)) + h.writeRawJSONStatus(http.StatusCreated, []byte(`{"id":`+base.ConvertToJSONString(docid)+`,"ok":true,"rev":"`+newRev+`","cv":"`+doc.HLV.GetCurrentVersionString()+`"}`)) return nil } @@ -406,16 +455,13 @@ func (h *handler) handleDeleteAttachment() error { docid := h.PathVar("docid") attachmentName := h.PathVar("attach") - revid := h.getQuery("rev") - if revid == "" { - var err error - revid, err = h.getEtag("If-Match") - if err != nil { - return err - } + + occValue, _, err := h.getOCCValue(nil) + if err != nil { + return err } - body, err := h.collection.Get1xRevBody(h.ctx(), docid, revid, false, nil) + body, err := h.collection.Get1xRevBody(h.ctx(), docid, occValue, false, nil) if err != nil { if base.IsDocNotFoundError(err) { // Check here if error is relating to incorrect revid, if so return 409 code else return 404 code @@ -428,7 +474,7 @@ func (h *handler) handleDeleteAttachment() error { return err } } else if body != nil { - if revid == "" { + if occValue == "" { // If a revid is not specified and an active revision was found, // return a conflict now, rather than letting db.Put do it further down... return base.HTTPErrorf(http.StatusConflict, "Cannot modify attachments without a specific rev ID") @@ -444,16 +490,44 @@ func (h *handler) handleDeleteAttachment() error { delete(attachments, attachmentName) body[db.BodyAttachments] = attachments - newRev, _, err := h.collection.Put(h.ctx(), docid, body) + newRev, doc, err := h.collection.Put(h.ctx(), docid, body) if err != nil { return err } h.setEtag(newRev) - h.writeRawJSONStatus(http.StatusOK, []byte(`{"id":`+base.ConvertToJSONString(docid)+`,"ok":true,"rev":"`+newRev+`"}`)) + h.writeRawJSONStatus(http.StatusOK, []byte(`{"id":`+base.ConvertToJSONString(docid)+`,"ok":true,"rev":"`+newRev+`","cv":"`+doc.HLV.GetCurrentVersionString()+`"}`)) return nil } +// occVersionType is a type used to represent the type of document version in optimistic concurrency control (OCC). Can be Revision Tree ID or a CV. +type occVersionType uint8 + +const ( + VersionTypeRevTreeID occVersionType = iota // Revision Tree ID (RevTreeID / RevID) + VersionTypeCV // HLV/Version Vector CV +) + +// guessOCCVersionTypeFromValue returns the type of document version based on the string value. Either a RevTree ID or a CV. +func guessOCCVersionTypeFromValue(s string) occVersionType { + if base.IsRevTreeID(s) { + return VersionTypeRevTreeID + } + // anything else we'll assume is a CV + // we _could_ check for a well-formatted CV, but given the usage for OCC, we only need an exact string match + return VersionTypeCV +} + +func bodyKeyForOCCVersionType(versionType occVersionType) string { + switch versionType { + case VersionTypeRevTreeID: + return db.BodyRev + case VersionTypeCV: + return db.BodyCV + } + return "" +} + // HTTP handler for a PUT of a document func (h *handler) handlePutDoc() error { @@ -500,23 +574,24 @@ func (h *handler) handlePutDoc() error { if h.getQuery("new_edits") != "false" { // Regular PUT: - bodyRev := body[db.BodyRev] - if oldRev := h.getQuery("rev"); oldRev != "" { - body[db.BodyRev] = oldRev - } else if ifMatch, _ := h.getEtag("If-Match"); ifMatch != "" { - body[db.BodyRev] = ifMatch - } - if bodyRev != nil && bodyRev != body[db.BodyRev] { - return base.HTTPErrorf(http.StatusBadRequest, "Revision IDs provided do not match") + + // occValue is the optimistic concurrency control value, which can be either the current rev ID or CV - used to prevent lost updates. + // we grab occValue from either query param, Etag header, or request body (in that order) + occValue, occValueType, err := h.getOCCValue(body) + if err != nil { + return err } + // set OCC version body value for Put + body[bodyKeyForOCCVersionType(occValueType)] = occValue + newRev, doc, err = h.collection.Put(h.ctx(), docid, body) if err != nil { return err } h.setEtag(newRev) } else { - // Replicator-style PUT with new_edits=false: + // Replicator-style PUT (allow new revisions/conflicts to be pushed) with new_edits=false: revisions := db.ParseRevisions(h.ctx(), body) if revisions == nil { return base.HTTPErrorf(http.StatusBadRequest, "Bad _revisions") @@ -527,13 +602,25 @@ func (h *handler) handlePutDoc() error { } } + respBody := []byte(`{"id":` + base.ConvertToJSONString(docid) + `,"ok":true,"rev":"` + newRev + `"}`) + + // if this was an idempotent update that didn't need to write an update, returned doc is nil, and we can't pull CV out of what we have available here. + // Accept this as an edge case that we don't need to support for CVs (this can only happen if the request is using new_edits=false, which is not the case for most clients, and definitely none that use CV) + if doc != nil { + respBody, err = base.InjectJSONProperties(respBody, base.KVPair{Key: "cv", Val: doc.HLV.GetCurrentVersionString()}) + if err != nil { + base.AssertfCtx(h.ctx(), "couldn't inject CV into response body: %v", err) + // safe to continue + } + } + if doc != nil && roundTrip { if err := h.collection.WaitForSequenceNotSkipped(h.ctx(), doc.Sequence); err != nil { return err } } - h.writeRawJSONStatus(http.StatusCreated, []byte(`{"id":`+base.ConvertToJSONString(docid)+`,"ok":true,"rev":"`+newRev+`"}`)) + h.writeRawJSONStatus(http.StatusCreated, respBody) return nil } @@ -648,10 +735,24 @@ func (h *handler) handlePostDoc() error { h.setHeader("Location", docid) h.setEtag(newRev) - h.writeRawJSON([]byte(`{"id":"` + docid + `","ok":true,"rev":"` + newRev + `"}`)) + + h.writeRawJSONStatus(http.StatusOK, []byte(`{"id":`+base.ConvertToJSONString(docid)+`,"ok":true,"rev":"`+newRev+`","cv":"`+doc.HLV.GetCurrentVersionString()+`"}`)) return nil } +func docVerisonFromOCCValue(occValue string, occValueType occVersionType) (docVersion db.DocVersion, err error) { + switch occValueType { + case VersionTypeRevTreeID: + docVersion.RevTreeID = occValue + case VersionTypeCV: + docVersion.CV, err = db.ParseVersion(occValue) + if err != nil { + return DocVersion{}, base.HTTPErrorf(http.StatusBadRequest, "Invalid CV: %v", err) + } + } + return docVersion, nil +} + // HTTP handler for a DELETE of a document func (h *handler) handleDeleteDoc() error { if h.db.DatabaseContext.Options.UnsupportedOptions != nil && h.db.DatabaseContext.Options.UnsupportedOptions.RejectWritesWithSkippedSequences { @@ -663,19 +764,35 @@ func (h *handler) handleDeleteDoc() error { } docid := h.PathVar("docid") - revid := h.getQuery("rev") - if revid == "" { - var err error - revid, err = h.getEtag("If-Match") + + // occValue is the optimistic concurrency control value, which can be either the current rev ID or CV - used to prevent lost updates. + // we grab occValue from either query param, Etag header, or request body (in that order) + occValue, occValueType, err := h.getOCCValue(nil) + if err != nil { + return fmt.Errorf("couldn't get OCC value from request: %w", err) + } + + docVersion, err := docVerisonFromOCCValue(occValue, occValueType) + if err != nil { + return fmt.Errorf("couldn't build document version from OCC value: %w", err) + } + + newRev, doc, err := h.collection.DeleteDoc(h.ctx(), docid, docVersion) + if err != nil { + return err + } + + respBody := []byte(`{"id":` + base.ConvertToJSONString(docid) + `,"ok":true,"rev":"` + newRev + `"}`) + if doc != nil { + respBody, err = base.InjectJSONProperties(respBody, base.KVPair{Key: "cv", Val: doc.HLV.GetCurrentVersionString()}) if err != nil { - return err + base.AssertfCtx(h.ctx(), "Failed to inject JSON properties: %v", err) + // safe to continue - we'll just have no deleted in response } } - newRev, _, err := h.collection.DeleteDoc(h.ctx(), docid, revid) - if err == nil { - h.writeRawJSONStatus(http.StatusOK, []byte(`{"id":`+base.ConvertToJSONString(docid)+`,"ok":true,"rev":"`+newRev+`"}`)) - } - return err + + h.writeRawJSONStatus(http.StatusOK, respBody) + return nil } // ////// LOCAL DOCS: diff --git a/rest/utilities_testing.go b/rest/utilities_testing.go index bc20086393..00f3e5c4d2 100644 --- a/rest/utilities_testing.go +++ b/rest/utilities_testing.go @@ -1138,6 +1138,17 @@ func (rt *RestTester) GetDocumentSequence(key string) (sequence uint64) { return rawResponse.Xattrs.Sync.Sequence } +// GetRawDoc returns the raw document response for a given document ID using the _raw endpoint. +func (rt *RestTester) GetRawDoc(key string) RawDocResponse { + response := rt.SendAdminRequest("GET", fmt.Sprintf("/{{.keyspace}}/_raw/%s", key), "") + require.Equal(rt.TB(), http.StatusOK, response.Code, "Error getting raw document %s", response.Body.String()) + response.DumpBody() + var rawResponse RawDocResponse + require.NoError(rt.TB(), base.JSONUnmarshal(response.BodyBytes(), &rawResponse)) + log.Printf("rawResponse: %#v", rawResponse) + return rawResponse +} + // ReplacePerBucketCredentials replaces buckets defined on StartupConfig.BucketCredentials then recreates the couchbase // cluster to pick up the changes func (rt *RestTester) ReplacePerBucketCredentials(config base.PerBucketCredentialsConfig) { @@ -2395,16 +2406,22 @@ func NewDocVersionFromFakeRev(fakeRev string) DocVersion { return DocVersion{RevTreeID: fakeRev} } -// DocVersionFromPutResponse returns a DocRevisionID from the given response to PUT /{, or fails the given test if a rev ID was not found. +// DocVersionFromPutResponse returns a DocVersion from the given response, or fails the given test if a version was not returned. func DocVersionFromPutResponse(t testing.TB, response *TestResponse) DocVersion { var r struct { DocID *string `json:"id"` RevID *string `json:"rev"` - } - require.NoError(t, json.Unmarshal(response.BodyBytes(), &r)) - require.NotNil(t, r.RevID, "expecting non-nil rev ID from response: %s", string(response.BodyBytes())) - require.NotEqual(t, "", *r.RevID, "expecting non-empty rev ID from response: %s", string(response.BodyBytes())) - return DocVersion{RevTreeID: *r.RevID} + CV *string `json:"cv"` + } + respBody := response.BodyString() + require.NoErrorf(t, json.Unmarshal(response.BodyBytes(), &r), "error unmarshalling response body: %s", respBody) + require.NotNilf(t, r.RevID, "expecting non-nil 'rev' from response: %s", respBody) + require.NotNilf(t, r.CV, "expecting non-nil 'cv' from response: %s", respBody) + require.NotEqualf(t, "", *r.RevID, "expecting non-empty 'rev' from response: %s", respBody) + require.NotEqualf(t, "", *r.CV, "expecting non-empty 'cv' from response: %s", respBody) + cv, err := db.ParseVersion(*r.CV) + require.NoErrorf(t, err, "error parsing CV %q: %v", *r.CV, err) + return DocVersion{RevTreeID: *r.RevID, CV: cv} } func MarshalConfig(t *testing.T, config db.ReplicationConfig) string { diff --git a/rest/utilities_testing_resttester.go b/rest/utilities_testing_resttester.go index aee3f630a1..f5669d22d2 100644 --- a/rest/utilities_testing_resttester.go +++ b/rest/utilities_testing_resttester.go @@ -15,6 +15,7 @@ import ( "net/http/httptest" "net/url" "slices" + "strings" "sync/atomic" "testing" "time" @@ -106,20 +107,42 @@ func (rt *RestTester) UpdateDocRev(docID, revID string, body string) string { // UpdateDoc updates a document at a specific version and returns the new version. func (rt *RestTester) UpdateDoc(docID string, version DocVersion, body string) DocVersion { - resource := fmt.Sprintf("/%s/%s?rev=%s", rt.GetSingleKeyspace(), docID, version.RevTreeID) - rawResponse := rt.SendAdminRequest(http.MethodPut, resource, body) - RequireStatus(rt.TB(), rawResponse, http.StatusCreated) - return DocVersionFromPutResponse(rt.TB(), rawResponse) + occValue := version.RevTreeID + if !version.CV.IsEmpty() { + occValue = version.CV.String() + } + resource := fmt.Sprintf("/%s/%s?rev=%s", rt.GetSingleKeyspace(), docID, occValue) + resp := rt.SendAdminRequest(http.MethodPut, resource, body) + if isRespUseRevTreeIDInstead(resp) { + // trying to update a document in-conflict with a CV - try again with RevTreeID + // this is a pretty narrow edge-case and one that customers would deal with in the same way (get the document out of conflict using RevTreeID before using CV) + resp = rt.SendAdminRequest(http.MethodPut, fmt.Sprintf("/%s/%s?rev=%s", rt.GetSingleKeyspace(), docID, version.RevTreeID), body) + } + RequireStatus(rt.TB(), resp, http.StatusCreated) + return DocVersionFromPutResponse(rt.TB(), resp) } // DeleteDoc deletes a document at a specific version. The test will fail if the revision does not exist. -func (rt *RestTester) DeleteDoc(docID string, docVersion DocVersion) DocVersion { - resp := rt.SendAdminRequest(http.MethodDelete, - fmt.Sprintf("/%s/%s?rev=%s", rt.GetSingleKeyspace(), docID, docVersion.RevTreeID), "") +func (rt *RestTester) DeleteDoc(docID string, version DocVersion) DocVersion { + occValue := version.RevTreeID + if !version.CV.IsEmpty() { + occValue = version.CV.String() + } + resp := rt.SendAdminRequest(http.MethodDelete, fmt.Sprintf("/%s/%s?rev=%s", rt.GetSingleKeyspace(), docID, occValue), "") + if isRespUseRevTreeIDInstead(resp) { + // trying to update a document in-conflict with a CV - try again with RevTreeID + // this is a pretty narrow edge-case and one that customers would deal with in the same way (get the document out of conflict using RevTreeID before using CV) + resp = rt.SendAdminRequest(http.MethodDelete, fmt.Sprintf("/%s/%s?rev=%s", rt.GetSingleKeyspace(), docID, version.RevTreeID), "") + } RequireStatus(rt.TB(), resp, http.StatusOK) return DocVersionFromPutResponse(rt.TB(), resp) } +// isRespUseRevTreeIDInstead returns true if the response indicates that a RevTree ID should be used instead of a CV for modifying a document in conflict. +func isRespUseRevTreeIDInstead(resp *TestResponse) bool { + return resp.Code == http.StatusBadRequest && strings.Contains(resp.BodyString(), "Cannot use CV to modify a document in conflict - resolve first with RevTree ID") +} + // DeleteDocRev removes a document at a specific revision. Deprecated for DeleteDoc. func (rt *RestTester) DeleteDocRev(docID, revID string) { rt.DeleteDoc(docID, DocVersion{RevTreeID: revID}) @@ -471,8 +494,7 @@ func (rt *RestTester) UpdateDocDirectly(docID string, version DocVersion, body d func (rt *RestTester) DeleteDocDirectly(docID string, version DocVersion) DocVersion { collection, ctx := rt.GetSingleTestDatabaseCollectionWithUser() - // TODO: CBG-4426 - DeleteDocDirectly does not support CV - rev, doc, err := collection.DeleteDoc(ctx, docID, version.RevTreeID) + rev, doc, err := collection.DeleteDoc(ctx, docID, version) require.NoError(rt.TB(), err) return DocVersion{RevTreeID: rev, CV: db.Version{SourceID: doc.HLV.SourceID, Value: doc.HLV.Version}} } diff --git a/topologytest/sync_gateway_peer_test.go b/topologytest/sync_gateway_peer_test.go index f2656636e6..b97208fd49 100644 --- a/topologytest/sync_gateway_peer_test.go +++ b/topologytest/sync_gateway_peer_test.go @@ -137,11 +137,9 @@ func (p *SyncGatewayPeer) WriteDocument(dsName sgbucket.DataStoreName, docID str func (p *SyncGatewayPeer) DeleteDocument(dsName sgbucket.DataStoreName, docID string) DocMetadata { collection, ctx := p.getCollection(dsName) doc, err := collection.GetDocument(ctx, docID, db.DocUnmarshalAll) - var revID string - if err == nil { - revID = doc.CurrentRev - } - _, doc, err = collection.DeleteDoc(ctx, docID, revID) + require.NoError(p.TB(), err) + require.NotNil(p.TB(), doc) + _, doc, err = collection.DeleteDoc(ctx, docID, doc.ExtractDocVersion()) require.NoError(p.TB(), err) docMeta := DocMetadataFromDocument(doc) p.TB().Logf("%s: Deleted document %s with %#+v", p, docID, docMeta) From d18431d6d963b0138d72a2753ec56221f8fa4e74 Mon Sep 17 00:00:00 2001 From: Ben Brooks Date: Wed, 20 Aug 2025 14:58:30 +0100 Subject: [PATCH 02/10] Change moved code to just use int64 - almost certainly not needed for Rev generation but makes the code simpler --- db/document.go | 4 ++-- rest/utilities_testing_blip_client.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/db/document.go b/db/document.go index 694149e934..3d5d7c4255 100644 --- a/db/document.go +++ b/db/document.go @@ -1523,7 +1523,7 @@ func (v DocVersion) GetRev(useHLV bool) string { } // RevIDGeneration returns the Rev ID generation for the current version -func (v *DocVersion) RevIDGeneration() int { +func (v *DocVersion) RevIDGeneration() int64 { if v == nil { return 0 } @@ -1532,7 +1532,7 @@ func (v *DocVersion) RevIDGeneration() int { base.AssertfCtx(context.TODO(), "Error parsing generation from rev ID %q: %v", v.RevTreeID, err) return 0 } - return int(gen) + return gen } // RevIDDigest returns the Rev ID digest for the current version diff --git a/rest/utilities_testing_blip_client.go b/rest/utilities_testing_blip_client.go index 4bed1530b5..837ccfe0f5 100644 --- a/rest/utilities_testing_blip_client.go +++ b/rest/utilities_testing_blip_client.go @@ -1531,7 +1531,7 @@ func (btcc *BlipTesterCollectionClient) upsertDoc(docID string, parentVersion *D doc = newClientDocument(docID, 0, nil) } - newGen := 1 + var newGen int64 = 1 var hlv db.HybridLogicalVector if parentVersion != nil { // grab latest version for this doc and make sure we're doing an upsert on top of it to avoid branching revisions @@ -1613,7 +1613,7 @@ func (btc *BlipTesterClient) GetDocVersion(docID string) DocVersion { return DocVersion{RevTreeID: doc.CurrentRev, CV: db.Version{SourceID: doc.HLV.SourceID, Value: doc.HLV.Version}} } -func (btcc *BlipTesterCollectionClient) ProcessInlineAttachments(inputBody []byte, revGen int) (outputBody []byte) { +func (btcc *BlipTesterCollectionClient) ProcessInlineAttachments(inputBody []byte, revGen int64) (outputBody []byte) { if !bytes.Contains(inputBody, []byte(db.BodyAttachments)) { return inputBody } From 8a1646ceb27b668c8f8e6cfabff702c12fc25b66 Mon Sep 17 00:00:00 2001 From: Ben Brooks Date: Wed, 20 Aug 2025 17:28:42 +0100 Subject: [PATCH 03/10] Remove log.Printf in test util --- rest/utilities_testing.go | 1 - 1 file changed, 1 deletion(-) diff --git a/rest/utilities_testing.go b/rest/utilities_testing.go index 00f3e5c4d2..f2aeda662e 100644 --- a/rest/utilities_testing.go +++ b/rest/utilities_testing.go @@ -1145,7 +1145,6 @@ func (rt *RestTester) GetRawDoc(key string) RawDocResponse { response.DumpBody() var rawResponse RawDocResponse require.NoError(rt.TB(), base.JSONUnmarshal(response.BodyBytes(), &rawResponse)) - log.Printf("rawResponse: %#v", rawResponse) return rawResponse } From ad00f8359b0e598218b58b2bef6c0de0df0e7b63 Mon Sep 17 00:00:00 2001 From: Ben Brooks Date: Wed, 20 Aug 2025 17:34:02 +0100 Subject: [PATCH 04/10] fix typo --- rest/doc_api.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/rest/doc_api.go b/rest/doc_api.go index 6fc1986a49..f590f5c131 100644 --- a/rest/doc_api.go +++ b/rest/doc_api.go @@ -740,7 +740,8 @@ func (h *handler) handlePostDoc() error { return nil } -func docVerisonFromOCCValue(occValue string, occValueType occVersionType) (docVersion db.DocVersion, err error) { +// docVersionFromOCCValue converts an OCC value and type into a DocVersion struct. +func docVersionFromOCCValue(occValue string, occValueType occVersionType) (docVersion db.DocVersion, err error) { switch occValueType { case VersionTypeRevTreeID: docVersion.RevTreeID = occValue @@ -772,7 +773,7 @@ func (h *handler) handleDeleteDoc() error { return fmt.Errorf("couldn't get OCC value from request: %w", err) } - docVersion, err := docVerisonFromOCCValue(occValue, occValueType) + docVersion, err := docVersionFromOCCValue(occValue, occValueType) if err != nil { return fmt.Errorf("couldn't build document version from OCC value: %w", err) } From 169f03481024e042bb26e0394fdc0a3394a30f8d Mon Sep 17 00:00:00 2001 From: Ben Brooks Date: Thu, 21 Aug 2025 15:37:13 +0100 Subject: [PATCH 05/10] Cleanup of `DocVersion` --- db/crud.go | 8 ++--- db/document.go | 47 +++++++------------------- rest/api_test.go | 9 +++-- rest/attachment_test.go | 8 +++-- rest/replicatortest/replicator_test.go | 2 +- rest/utilities_testing_blip_client.go | 16 +++++---- 6 files changed, 37 insertions(+), 53 deletions(-) diff --git a/db/crud.go b/db/crud.go index 5fd73053c2..8fe8e50eac 100644 --- a/db/crud.go +++ b/db/crud.go @@ -2941,12 +2941,8 @@ func (db *DatabaseCollectionWithUser) Post(ctx context.Context, body Body) (doci // Deletes a document, by adding a new revision whose _deleted property is true. func (db *DatabaseCollectionWithUser) DeleteDoc(ctx context.Context, docid string, docVersion DocVersion) (string, *Document, error) { - body := Body{BodyDeleted: true} - if !docVersion.CV.IsEmpty() { - body[BodyCV] = docVersion.CV.String() - } else { - body[BodyRev] = docVersion.RevTreeID - } + versionKey, versionStr := docVersion.Body1xKVPair() + body := Body{BodyDeleted: true, versionKey: versionStr} newRevID, doc, err := db.Put(ctx, docid, body) return newRevID, doc, err } diff --git a/db/document.go b/db/document.go index 3d5d7c4255..ba348ea136 100644 --- a/db/document.go +++ b/db/document.go @@ -19,7 +19,6 @@ import ( "math" "net/http" "strconv" - "strings" "time" sgbucket "github.com/couchbase/sg-bucket" @@ -1487,6 +1486,7 @@ func (doc *Document) ExtractDocVersion() DocVersion { } // DocVersion represents a specific version of a document in an revID/HLV agnostic manner. +// Users should access the properties via the CV and RevTreeID methods, to ensure there's a check that the specific version type is not empty. type DocVersion struct { RevTreeID string CV Version @@ -1502,43 +1502,20 @@ func (v DocVersion) GoString() string { return fmt.Sprintf("DocVersion{RevTreeID:%s,CV:%#v}", v.RevTreeID, v.CV) } -// DocVersionRevTreeEqual compares two DocVersions based on their RevTreeID. -func (v DocVersion) DocVersionRevTreeEqual(o DocVersion) bool { - if v.RevTreeID != o.RevTreeID { - return false - } - return true -} - -// GetRev returns the revision ID for the DocVersion. -func (v DocVersion) GetRev(useHLV bool) string { - if useHLV { - if v.CV.SourceID == "" { - return "" - } - return v.CV.String() - } else { - return v.RevTreeID - } +// GetRevTreeID returns the Revision Tree ID of the document version, and a bool indicating whether it is present. +func (v DocVersion) GetRevTreeID() (string, bool) { + return v.RevTreeID, v.RevTreeID != "" } -// RevIDGeneration returns the Rev ID generation for the current version -func (v *DocVersion) RevIDGeneration() int64 { - if v == nil { - return 0 - } - gen, err := strconv.ParseInt(strings.Split(v.RevTreeID, "-")[0], 10, 64) - if err != nil { - base.AssertfCtx(context.TODO(), "Error parsing generation from rev ID %q: %v", v.RevTreeID, err) - return 0 - } - return gen +// GetCV returns the Current Version of the document, and a bool indicating whether it is present. +func (v DocVersion) GetCV() (Version, bool) { + return v.CV, !v.CV.IsEmpty() } -// RevIDDigest returns the Rev ID digest for the current version -func (v *DocVersion) RevIDDigest() string { - if v == nil { - return "" +// Body1xKVPair returns the key and value to use in a 1.x-style document body for the given DocVersion. +func (d DocVersion) Body1xKVPair() (bodyVersionKey, bodyVersionStr string) { + if cv, ok := d.GetCV(); ok { + return BodyCV, cv.String() } - return strings.Split(v.RevTreeID, "-")[1] + return BodyRev, d.RevTreeID } diff --git a/rest/api_test.go b/rest/api_test.go index 73ce33da72..bedf8c52d5 100644 --- a/rest/api_test.go +++ b/rest/api_test.go @@ -3255,10 +3255,15 @@ func TestDocCRUDWithCV(t *testing.T) { getDocVersion, _ := rt.GetDoc(docID) require.Equal(t, createVersion, getDocVersion) + revIDGen := func(v DocVersion) int { + gen, _ := db.ParseRevID(base.TestCtx(t), v.RevTreeID) + return gen + } + updateVersion := rt.UpdateDoc(docID, createVersion, `{"update":true}`) require.NotEqual(t, createVersion, updateVersion) assert.Greaterf(t, updateVersion.CV.Value, createVersion.CV.Value, "Expected CV Value to be bumped on update") - assert.Greaterf(t, updateVersion.RevIDGeneration(), createVersion.RevIDGeneration(), "Expected revision generation to be bumped on update") + assert.Greaterf(t, revIDGen(updateVersion), revIDGen(createVersion), "Expected revision generation to be bumped on update") getDocVersion, _ = rt.GetDoc(docID) require.Equal(t, updateVersion, getDocVersion) @@ -3284,5 +3289,5 @@ func TestDocCRUDWithCV(t *testing.T) { deleteVersion := rt.DeleteDoc(docID, updateVersion) require.NotEqual(t, updateVersion, deleteVersion) assert.Greaterf(t, deleteVersion.CV.Value, updateVersion.CV.Value, "Expected CV Value to be bumped on delete") - assert.Greaterf(t, deleteVersion.RevIDGeneration(), updateVersion.RevIDGeneration(), "Expected revision generation to be bumped on delete") + assert.Greaterf(t, revIDGen(deleteVersion), revIDGen(updateVersion), "Expected revision generation to be bumped on delete") } diff --git a/rest/attachment_test.go b/rest/attachment_test.go index 06e71829ac..3c9838dee4 100644 --- a/rest/attachment_test.go +++ b/rest/attachment_test.go @@ -727,7 +727,8 @@ func TestConflictWithInvalidAttachment(t *testing.T) { require.NoError(t, base.JSONUnmarshal(response.Body.Bytes(), &body)) // Modify Doc - parentRevList := [3]string{"foo3", "foo2", version.RevIDDigest()} + _, versionDigest := db.ParseRevID(base.TestCtx(t), version.RevTreeID) + parentRevList := [3]string{"foo3", "foo2", versionDigest} body["_rev"] = "3-foo3" body["rev"] = "3-foo3" body["_revisions"].(map[string]interface{})["ids"] = parentRevList @@ -794,16 +795,17 @@ func TestConflictingBranchAttachments(t *testing.T) { // Create a document version := rt.CreateTestDoc("doc1") + _, versionDigest := db.ParseRevID(base.TestCtx(t), version.RevTreeID) // //Create diverging tree - reqBodyRev2 := `{"_rev": "2-two", "_revisions": {"ids": ["two", "` + version.RevIDDigest() + `"], "start": 2}}` + reqBodyRev2 := `{"_rev": "2-two", "_revisions": {"ids": ["two", "` + versionDigest + `"], "start": 2}}` response := rt.SendAdminRequest("PUT", "/{{.keyspace}}/doc1?new_edits=false", reqBodyRev2) RequireStatus(t, response, http.StatusCreated) docVersion2 := DocVersionFromPutResponse(t, response) - reqBodyRev2a := `{"_rev": "2-two", "_revisions": {"ids": ["twoa", "` + version.RevIDDigest() + `"], "start": 2}}` + reqBodyRev2a := `{"_rev": "2-two", "_revisions": {"ids": ["twoa", "` + versionDigest + `"], "start": 2}}` response = rt.SendAdminRequest("PUT", "/{{.keyspace}}/doc1?new_edits=false", reqBodyRev2a) RequireStatus(t, response, http.StatusCreated) docVersion2a := DocVersionFromPutResponse(t, response) diff --git a/rest/replicatortest/replicator_test.go b/rest/replicatortest/replicator_test.go index a24c896e27..0b9d50796f 100644 --- a/rest/replicatortest/replicator_test.go +++ b/rest/replicatortest/replicator_test.go @@ -4324,7 +4324,7 @@ func TestActiveReplicatorPushAndPullConflict(t *testing.T) { // Validate results on the remote (rt2) rt2Since := remoteDoc.Sequence - if test.expectedVersion.DocVersionRevTreeEqual(test.remoteVersion) { + if test.expectedVersion.RevTreeID == test.remoteVersion.RevTreeID { // no changes should have been pushed back up to rt2, because this rev won. rt2Since = 0 } diff --git a/rest/utilities_testing_blip_client.go b/rest/utilities_testing_blip_client.go index 837ccfe0f5..f1429f7a86 100644 --- a/rest/utilities_testing_blip_client.go +++ b/rest/utilities_testing_blip_client.go @@ -1531,7 +1531,7 @@ func (btcc *BlipTesterCollectionClient) upsertDoc(docID string, parentVersion *D doc = newClientDocument(docID, 0, nil) } - var newGen int64 = 1 + var newGen int = 1 var hlv db.HybridLogicalVector if parentVersion != nil { // grab latest version for this doc and make sure we're doing an upsert on top of it to avoid branching revisions @@ -1541,7 +1541,8 @@ func (btcc *BlipTesterCollectionClient) upsertDoc(docID string, parentVersion *D if btcc.UseHLV() { hlv = latestRev.HLV } else { - newGen = parentVersion.RevIDGeneration() + 1 + parentGen, _ := db.ParseRevID(btcc.ctx, parentVersion.RevTreeID) + newGen = parentGen + 1 } } @@ -1613,7 +1614,7 @@ func (btc *BlipTesterClient) GetDocVersion(docID string) DocVersion { return DocVersion{RevTreeID: doc.CurrentRev, CV: db.Version{SourceID: doc.HLV.SourceID, Value: doc.HLV.Version}} } -func (btcc *BlipTesterCollectionClient) ProcessInlineAttachments(inputBody []byte, revGen int64) (outputBody []byte) { +func (btcc *BlipTesterCollectionClient) ProcessInlineAttachments(inputBody []byte, revGen int) (outputBody []byte) { if !bytes.Contains(inputBody, []byte(db.BodyAttachments)) { return inputBody } @@ -2083,11 +2084,14 @@ func (btcc *BlipTesterCollectionClient) getAllRevisions(docID string) []DocVersi return versions } -func (btc *BlipTesterClient) AssertDeltaSrcProperty(t *testing.T, msg *blip.Message, docVersion DocVersion) { +func (btc *BlipTesterClient) AssertDeltaSrcProperty(t *testing.T, msg *blip.Message, expectedVersion DocVersion) { subProtocol, err := db.ParseSubprotocolString(btc.SupportedBLIPProtocols[0]) require.NoError(t, err) - rev := docVersion.GetRev(subProtocol >= db.CBMobileReplicationV4) - assert.Equal(t, rev, msg.Properties[db.RevMessageDeltaSrc]) + expectedDeltaSrcRev := expectedVersion.RevTreeID + if subProtocol >= db.CBMobileReplicationV4 { + expectedDeltaSrcRev = expectedVersion.CV.String() + } + assert.Equal(t, expectedDeltaSrcRev, msg.Properties[db.RevMessageDeltaSrc]) } // getHLVFromRevMessage extracts the full HLV from a rev message. This will fail the test if the message does not contain a valid HLV. From abb7c1b19f352343e336bb9670f1a0b9bc2852b8 Mon Sep 17 00:00:00 2001 From: Ben Brooks Date: Thu, 21 Aug 2025 15:55:02 +0100 Subject: [PATCH 06/10] Add zero-value for occVersionType enum and more error handling --- rest/doc_api.go | 43 +++++++++++++++++++++------- rest/utilities_testing.go | 1 - rest/utilities_testing_resttester.go | 4 +-- 3 files changed, 34 insertions(+), 14 deletions(-) diff --git a/rest/doc_api.go b/rest/doc_api.go index f590f5c131..c7185e051b 100644 --- a/rest/doc_api.go +++ b/rest/doc_api.go @@ -362,6 +362,14 @@ func (h *handler) getOCCValue(optionalBody db.Body) (occValue string, occValueTy occValueType = VersionTypeRevTreeID skipBodyMatchValidation = true } + } else { + // empty occValue - treat as a create operation without any parent + return "", VersionTypeRevTreeID, nil + } + + // defensive measure against falling out of above without a type set + if occValueType == VersionTypeUnknown { + return "", 0, base.HTTPErrorf(http.StatusBadRequest, "Invalid version type for OCC value: %q", occValue) } // ensure the value provided matches exactly the one that may also be supplied in the body @@ -410,8 +418,12 @@ func (h *handler) handlePutAttachment() error { body, err := h.collection.Get1xRevBody(h.ctx(), docid, occValue, false, nil) if err != nil { if base.IsDocNotFoundError(err) { + bodyKey, err := bodyKeyForOCCVersionType(occValueType) + if err != nil { + return base.HTTPErrorf(http.StatusBadRequest, "Invalid OCC version type: %v", err) + } // couchdb creates empty body on attachment PUT for non-existent doc id - body = db.Body{bodyKeyForOCCVersionType(occValueType): occValue} + body = db.Body{bodyKey: occValue} } else if err != nil { return err } @@ -504,8 +516,9 @@ func (h *handler) handleDeleteAttachment() error { type occVersionType uint8 const ( - VersionTypeRevTreeID occVersionType = iota // Revision Tree ID (RevTreeID / RevID) - VersionTypeCV // HLV/Version Vector CV + VersionTypeUnknown occVersionType = iota + VersionTypeRevTreeID // Revision Tree ID (RevTreeID / RevID) + VersionTypeCV // HLV/Version Vector CV ) // guessOCCVersionTypeFromValue returns the type of document version based on the string value. Either a RevTree ID or a CV. @@ -513,19 +526,21 @@ func guessOCCVersionTypeFromValue(s string) occVersionType { if base.IsRevTreeID(s) { return VersionTypeRevTreeID } - // anything else we'll assume is a CV - // we _could_ check for a well-formatted CV, but given the usage for OCC, we only need an exact string match - return VersionTypeCV + if _, err := db.ParseVersion(s); err == nil { + return VersionTypeCV + } + return VersionTypeUnknown } -func bodyKeyForOCCVersionType(versionType occVersionType) string { +func bodyKeyForOCCVersionType(versionType occVersionType) (string, error) { switch versionType { case VersionTypeRevTreeID: - return db.BodyRev + return db.BodyRev, nil case VersionTypeCV: - return db.BodyCV + return db.BodyCV, nil + default: + return "", fmt.Errorf("unknown occVersionType %d", versionType) } - return "" } // HTTP handler for a PUT of a document @@ -583,7 +598,11 @@ func (h *handler) handlePutDoc() error { } // set OCC version body value for Put - body[bodyKeyForOCCVersionType(occValueType)] = occValue + bodyKey, err := bodyKeyForOCCVersionType(occValueType) + if err != nil { + return base.HTTPErrorf(http.StatusBadRequest, "Invalid OCC version type: %v", err) + } + body[bodyKey] = occValue newRev, doc, err = h.collection.Put(h.ctx(), docid, body) if err != nil { @@ -750,6 +769,8 @@ func docVersionFromOCCValue(occValue string, occValueType occVersionType) (docVe if err != nil { return DocVersion{}, base.HTTPErrorf(http.StatusBadRequest, "Invalid CV: %v", err) } + default: + return DocVersion{}, base.HTTPErrorf(http.StatusBadRequest, "Unknown OCC version type: %d", occValueType) } return docVersion, nil } diff --git a/rest/utilities_testing.go b/rest/utilities_testing.go index f2aeda662e..af99175b8f 100644 --- a/rest/utilities_testing.go +++ b/rest/utilities_testing.go @@ -1142,7 +1142,6 @@ func (rt *RestTester) GetDocumentSequence(key string) (sequence uint64) { func (rt *RestTester) GetRawDoc(key string) RawDocResponse { response := rt.SendAdminRequest("GET", fmt.Sprintf("/{{.keyspace}}/_raw/%s", key), "") require.Equal(rt.TB(), http.StatusOK, response.Code, "Error getting raw document %s", response.Body.String()) - response.DumpBody() var rawResponse RawDocResponse require.NoError(rt.TB(), base.JSONUnmarshal(response.BodyBytes(), &rawResponse)) return rawResponse diff --git a/rest/utilities_testing_resttester.go b/rest/utilities_testing_resttester.go index f5669d22d2..9821087458 100644 --- a/rest/utilities_testing_resttester.go +++ b/rest/utilities_testing_resttester.go @@ -105,7 +105,7 @@ func (rt *RestTester) UpdateDocRev(docID, revID string, body string) string { return version.RevTreeID } -// UpdateDoc updates a document at a specific version and returns the new version. +// UpdateDoc updates a document at a specific version and returns the new version. Uses CV for REST API if present in DocVersion, otherwise fall back to RevTreeID. func (rt *RestTester) UpdateDoc(docID string, version DocVersion, body string) DocVersion { occValue := version.RevTreeID if !version.CV.IsEmpty() { @@ -122,7 +122,7 @@ func (rt *RestTester) UpdateDoc(docID string, version DocVersion, body string) D return DocVersionFromPutResponse(rt.TB(), resp) } -// DeleteDoc deletes a document at a specific version. The test will fail if the revision does not exist. +// DeleteDoc deletes a document at a specific version. The test will fail if the revision does not exist. Uses CV for REST API if present in DocVersion, otherwise fall back to RevTreeID. func (rt *RestTester) DeleteDoc(docID string, version DocVersion) DocVersion { occValue := version.RevTreeID if !version.CV.IsEmpty() { From 79edf1281acbf7e323293869ef078727761e7e87 Mon Sep 17 00:00:00 2001 From: Ben Brooks Date: Thu, 21 Aug 2025 18:19:05 +0100 Subject: [PATCH 07/10] Escape rev query param for CV in RestTester helper functions --- rest/utilities_testing_resttester.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rest/utilities_testing_resttester.go b/rest/utilities_testing_resttester.go index 9821087458..f349e8b2a9 100644 --- a/rest/utilities_testing_resttester.go +++ b/rest/utilities_testing_resttester.go @@ -111,7 +111,7 @@ func (rt *RestTester) UpdateDoc(docID string, version DocVersion, body string) D if !version.CV.IsEmpty() { occValue = version.CV.String() } - resource := fmt.Sprintf("/%s/%s?rev=%s", rt.GetSingleKeyspace(), docID, occValue) + resource := fmt.Sprintf("/%s/%s?rev=%s", rt.GetSingleKeyspace(), docID, url.QueryEscape(occValue)) resp := rt.SendAdminRequest(http.MethodPut, resource, body) if isRespUseRevTreeIDInstead(resp) { // trying to update a document in-conflict with a CV - try again with RevTreeID @@ -128,7 +128,7 @@ func (rt *RestTester) DeleteDoc(docID string, version DocVersion) DocVersion { if !version.CV.IsEmpty() { occValue = version.CV.String() } - resp := rt.SendAdminRequest(http.MethodDelete, fmt.Sprintf("/%s/%s?rev=%s", rt.GetSingleKeyspace(), docID, occValue), "") + resp := rt.SendAdminRequest(http.MethodDelete, fmt.Sprintf("/%s/%s?rev=%s", rt.GetSingleKeyspace(), docID, url.QueryEscape(occValue)), "") if isRespUseRevTreeIDInstead(resp) { // trying to update a document in-conflict with a CV - try again with RevTreeID // this is a pretty narrow edge-case and one that customers would deal with in the same way (get the document out of conflict using RevTreeID before using CV) From f17891cebe446f98f120084a86bf0ac7f2376881 Mon Sep 17 00:00:00 2001 From: Ben Brooks Date: Thu, 21 Aug 2025 18:54:16 +0100 Subject: [PATCH 08/10] Detect unescaped rev values - improve documentation and cover with test. --- docs/api/components/parameters.yaml | 6 ++++-- rest/doc_api.go | 6 ++++++ rest/doc_api_test.go | 28 ++++++++++++++++++++++++++++ 3 files changed, 38 insertions(+), 2 deletions(-) diff --git a/docs/api/components/parameters.yaml b/docs/api/components/parameters.yaml index 24ab42881c..a64fe5fc4b 100644 --- a/docs/api/components/parameters.yaml +++ b/docs/api/components/parameters.yaml @@ -341,8 +341,10 @@ rev: required: false schema: type: string - example: 2-5145e1086bb8d1d71a531e9f6b543c58 - description: The document revision to target. + examples: + - 2-5145e1086bb8d1d71a531e9f6b543c58 + - 185dd4a2b4490000%404EZjEgl1AKEj8qh%2BvXS7OQ + description: The document revision to target. If this is a CV value, ensure the query parameter is URL encoded (`+`->`%2B`, `@`->`%40`, etc.) revs_from: name: revs_from in: query diff --git a/rest/doc_api.go b/rest/doc_api.go index c7185e051b..712c4f3248 100644 --- a/rest/doc_api.go +++ b/rest/doc_api.go @@ -344,6 +344,12 @@ func (h *handler) getOCCValue(optionalBody db.Body) (occValue string, occValueTy // we grab occValue from either query param, Etag header, or request body (in that order) if revQuery := h.getQuery("rev"); revQuery != "" { occValue = revQuery + // try to detect occ Values that are not URL Query escaped + // - `+` which can appear in base64 strings is converted to a space when not escaped properly + // other characters are difficult to correctly detect, since the value is already unescaped + if strings.ContainsAny(occValue, " ") { + return "", 0, base.HTTPErrorf(http.StatusBadRequest, "Bad rev query parameter: %q - ensure this query parameter value is URL Encoded", occValue) + } occValueType = guessOCCVersionTypeFromValue(occValue) } else if ifMatch, err := h.getEtag("If-Match"); err != nil { return "", 0, err diff --git a/rest/doc_api_test.go b/rest/doc_api_test.go index 77f1f034f3..7666188a6a 100644 --- a/rest/doc_api_test.go +++ b/rest/doc_api_test.go @@ -290,3 +290,31 @@ func readMultiPartBody(t *testing.T, response *TestResponse) []string { } return output } + +func TestCVUnescapedRevQueryParam(t *testing.T) { + tests := []struct { + revValue string + expectError bool + }{ + {revValue: "1-abc"}, // Normal Rev ID (doesn't need escaping) + {revValue: "185dd4a2b4490000%404EZjEgl1AKEj8qh%2BvXS7OQ"}, // CV escaped + {revValue: "185dd4a2b4490000@4EZjEgl1AKEj8qh+vXS7OQ", expectError: true}, // CV unescaped + } + + rt := NewRestTesterPersistentConfig(t) + defer rt.Close() + + for _, test := range tests { + t.Run(test.revValue, func(t *testing.T) { + resp := rt.SendAdminRequest(http.MethodPut, "/{{.keyspace}}/testdoc?rev="+test.revValue, `{"foo":"bar"}`) + if test.expectError { + RequireStatus(t, resp, http.StatusBadRequest) + assert.Contains(t, resp.BodyString(), "Bad rev query parameter") + } else { + // this is "successful" since there isn't a doc that exists with that rev but the request made it through + RequireStatus(t, resp, http.StatusConflict) + assert.Contains(t, resp.BodyString(), `Document revision conflict`) + } + }) + } +} From e5b70676f937f07b681182bfb10d0a7b837618bf Mon Sep 17 00:00:00 2001 From: Ben Brooks Date: Thu, 21 Aug 2025 19:01:59 +0100 Subject: [PATCH 09/10] Remove multi-examples --- docs/api/components/parameters.yaml | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/docs/api/components/parameters.yaml b/docs/api/components/parameters.yaml index a64fe5fc4b..6232e9fca8 100644 --- a/docs/api/components/parameters.yaml +++ b/docs/api/components/parameters.yaml @@ -341,9 +341,7 @@ rev: required: false schema: type: string - examples: - - 2-5145e1086bb8d1d71a531e9f6b543c58 - - 185dd4a2b4490000%404EZjEgl1AKEj8qh%2BvXS7OQ + example: 2-5145e1086bb8d1d71a531e9f6b543c58 description: The document revision to target. If this is a CV value, ensure the query parameter is URL encoded (`+`->`%2B`, `@`->`%40`, etc.) revs_from: name: revs_from From da06d680cb29255ad24d186761e06e474fc29b0d Mon Sep 17 00:00:00 2001 From: Ben Brooks Date: Thu, 21 Aug 2025 20:00:29 +0100 Subject: [PATCH 10/10] Fix TestDocCRUDWithCV escaping --- rest/api_test.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/rest/api_test.go b/rest/api_test.go index bedf8c52d5..09b1282160 100644 --- a/rest/api_test.go +++ b/rest/api_test.go @@ -21,6 +21,7 @@ import ( "mime/multipart" "net/http" "net/http/httptest" + "net/url" "os" "path/filepath" "sort" @@ -3269,18 +3270,16 @@ func TestDocCRUDWithCV(t *testing.T) { require.Equal(t, updateVersion, getDocVersion) // fetch by CV (using the first create version to test cache retrieval) - resp := rt.SendAdminRequest(http.MethodGet, fmt.Sprintf("/{{.keyspace}}/%s?rev=%s", docID, createVersion.CV.String()), "") + resp := rt.SendAdminRequest(http.MethodGet, fmt.Sprintf("/{{.keyspace}}/%s?rev=%s", docID, url.QueryEscape(createVersion.CV.String())), "") RequireStatus(t, resp, http.StatusOK) - resp.DumpBody() assert.NotContains(t, resp.BodyString(), `"update":true`) assert.Contains(t, resp.BodyString(), `"create":true`) assert.Contains(t, resp.BodyString(), `"_cv":"`+createVersion.CV.String()+`"`) assert.Contains(t, resp.BodyString(), `"_rev":"`+createVersion.RevTreeID+`"`) // fetch by CV - updated version - resp = rt.SendAdminRequest(http.MethodGet, fmt.Sprintf("/{{.keyspace}}/%s?rev=%s", docID, updateVersion.CV.String()), "") + resp = rt.SendAdminRequest(http.MethodGet, fmt.Sprintf("/{{.keyspace}}/%s?rev=%s", docID, url.QueryEscape(updateVersion.CV.String())), "") RequireStatus(t, resp, http.StatusOK) - resp.DumpBody() assert.NotContains(t, resp.BodyString(), `"create":true`) assert.Contains(t, resp.BodyString(), `"update":true`) assert.Contains(t, resp.BodyString(), `"_cv":"`+updateVersion.CV.String()+`"`)