Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions db/change_cache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,7 @@ func TestChannelCacheBackfill(t *testing.T) {
assert.Equal(t, &ChangeEntry{
Seq: SequenceID{Seq: 1, TriggeredBy: 0, LowSeq: 2},
ID: "doc-1",
Changes: []ChangeRev{{"rev": "1-a"}},
Changes: []ChangeByVersionType{{"rev": "1-a"}},
collectionID: collectionID}, changes[0])

lastSeq := changes[len(changes)-1].Seq
Expand Down Expand Up @@ -568,7 +568,7 @@ func TestChannelCacheBackfill(t *testing.T) {
assert.Equal(t, &ChangeEntry{
Seq: SequenceID{Seq: 3, LowSeq: 3},
ID: "doc-3",
Changes: []ChangeRev{{"rev": "1-a"}},
Changes: []ChangeByVersionType{{"rev": "1-a"}},
collectionID: collectionID,
}, changes[0])

Expand Down Expand Up @@ -722,7 +722,7 @@ func TestLowSequenceHandling(t *testing.T) {
require.Equal(t, &ChangeEntry{
Seq: SequenceID{Seq: 1, TriggeredBy: 0, LowSeq: 2},
ID: "doc-1",
Changes: []ChangeRev{{"rev": "1-a"}},
Changes: []ChangeByVersionType{{"rev": "1-a"}},
collectionID: collectionID}, changes[0])

// Test backfill clear - sequence numbers go back to standard handling
Expand Down
172 changes: 121 additions & 51 deletions db/changes.go

Large diffs are not rendered by default.

10 changes: 5 additions & 5 deletions db/changes_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ func TestChangesAfterChannelAdded(t *testing.T) {

require.Len(t, changes, 1)
assert.Equal(t, "doc2", changes[0].ID)
assert.Equal(t, []ChangeRev{{"rev": revid}}, changes[0].Changes)
assert.Equal(t, []ChangeByVersionType{{"rev": revid}}, changes[0].Changes)

// validate from zero
changes = getChanges(t, collection, base.SetOf("*"), getChangesOptionsWithZeroSeq(t))
Expand Down Expand Up @@ -234,7 +234,7 @@ func TestDocDeletionFromChannelCoalescedRemoved(t *testing.T) {
require.Equal(t, &ChangeEntry{
Seq: SequenceID{Seq: 1},
ID: "alpha",
Changes: []ChangeRev{{"rev": revid}},
Changes: []ChangeByVersionType{{"rev": revid}},
collectionID: collectionID}, changes[0])

lastSeq := getLastSeq(changes)
Expand Down Expand Up @@ -279,7 +279,7 @@ func TestDocDeletionFromChannelCoalescedRemoved(t *testing.T) {
ID: "alpha",
Removed: base.SetOf("A"),
allRemoved: true,
Changes: []ChangeRev{{"rev": "2-e99405a23fa102238fa8c3fd499b15bc"}},
Changes: []ChangeByVersionType{{"rev": "2-e99405a23fa102238fa8c3fd499b15bc"}},
collectionID: collectionID}, changes[0])

printChanges(changes)
Expand Down Expand Up @@ -352,7 +352,7 @@ func TestDocDeletionFromChannelCoalesced(t *testing.T) {
require.Equal(t, &ChangeEntry{
Seq: SequenceID{Seq: 1},
ID: "alpha",
Changes: []ChangeRev{{"rev": revid}},
Changes: []ChangeByVersionType{{"rev": revid}},
collectionID: collectionID}, changes[0])

lastSeq := getLastSeq(changes)
Expand Down Expand Up @@ -392,7 +392,7 @@ func TestDocDeletionFromChannelCoalesced(t *testing.T) {
require.Equal(t, &ChangeEntry{
Seq: SequenceID{Seq: 3},
ID: "alpha",
Changes: []ChangeRev{{"rev": "3-e99405a23fa102238fa8c3fd499b15bc"}},
Changes: []ChangeByVersionType{{"rev": "3-e99405a23fa102238fa8c3fd499b15bc"}},
collectionID: collectionID}, changes[0])

printChanges(changes)
Expand Down
34 changes: 25 additions & 9 deletions db/crud.go
Original file line number Diff line number Diff line change
Expand Up @@ -354,27 +354,43 @@ func (db *DatabaseCollectionWithUser) Get1xRevBodyWithHistory(ctx context.Contex

// Underlying revision retrieval used by Get1xRevBody, Get1xRevBodyWithHistory, GetRevCopy.
// Returns the revision of a document using the revision cache.
// - revid may be "", meaning the current revision.
// - revOrCV may be "", meaning the current revision. It can be a RevTree ID or a HLV CV.
// - maxHistory is >0 if the caller wants a revision history; it's the max length of the history.
// - historyFrom is an optional list of revIDs the client already has. If any of these are found
// in the revision's history, it will be trimmed after that revID.
// - attachmentsSince is nil to return no attachment bodies, otherwise a (possibly empty) list of
// revisions for which the client already has attachments and doesn't need bodies. Any attachment
// that hasn't changed since one of those revisions will be returned as a stub.
func (db *DatabaseCollectionWithUser) getRev(ctx context.Context, docid, revid string, maxHistory int, historyFrom []string) (revision DocumentRevision, err error) {
if revid != "" {
func (db *DatabaseCollectionWithUser) getRev(ctx context.Context, docid, revOrCV string, maxHistory int, historyFrom []string) (DocumentRevision, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some general thoughts that aren't actionable necessarily:

I don't love that this could be revOrCV here. I think a totally more correct way would be to pass a DocVersion here that is only used in tests. I experimented a bit with this and I couldn't make it work without refactoring. We don't have a db.Version for DocVersion.CV in the calling functions here. I didn't have time to consider whether it would be more appropriate to have ChangeEntry.Changes actually have a non string version of the CV.

I don't think we have time for this for a release, but it might be worth thinking about in general.

Other places in crud the API is revTreeID string, cv *db.Version as in documentRevisionForRequest and so changing this would involve downstream changes outside of the scope of this PR.

Copy link
Member Author

@bbrks bbrks Aug 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of this is tied to CBG-4776 because there will be a new requirement for passing "one of cv or revid" down from REST APIs in a fetch (which eventually reaches this codepath)

The problem is all of these areas of rev/cv have been added piecemeal with no real design or consistency - which I completely agree is a problem, but one that doesn't need solving right now.

string is dumb but achieves this "oneOf" requirement easily and with sufficient naming and documentation I don't see it being a problem in reality. If we're accidentally passing in docID or something then we'll find out about that pretty quickly in our tests.

var (
revID *string
cv *Version
)

var (
revision DocumentRevision
getErr error
)
if revOrCV != "" {
// Get a specific revision body and history from the revision cache
// (which will load them if necessary, by calling revCacheLoader, above)
revision, err = db.revisionCache.GetWithRev(ctx, docid, revid, RevCacheOmitDelta)
if currentVersion, parseErr := ParseVersion(revOrCV); parseErr != nil {
// try as a rev ID
revID = &revOrCV
revision, getErr = db.revisionCache.GetWithRev(ctx, docid, *revID, RevCacheOmitDelta)
} else {
cv = &currentVersion
revision, getErr = db.revisionCache.GetWithCV(ctx, docid, cv, RevCacheOmitDelta)
}
} else {
// No rev ID given, so load active revision
revision, err = db.revisionCache.GetActive(ctx, docid)
// No rev given, so load active revision
revision, getErr = db.revisionCache.GetActive(ctx, docid)
}
if err != nil {
return DocumentRevision{}, err
if getErr != nil {
return DocumentRevision{}, getErr
}

return db.documentRevisionForRequest(ctx, docid, revision, &revid, nil, maxHistory, historyFrom)
return db.documentRevisionForRequest(ctx, docid, revision, revID, cv, maxHistory, historyFrom)
}

// documentRevisionForRequest processes the given DocumentRevision and returns a version of it for a given client request, depending on access, deleted, etc.
Expand Down
4 changes: 2 additions & 2 deletions db/database_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1718,7 +1718,7 @@ func TestConflicts(t *testing.T) {
assert.Equal(t, &ChangeEntry{
Seq: SequenceID{Seq: 3},
ID: "doc",
Changes: []ChangeRev{{"rev": "2-b"}, {"rev": "2-a"}},
Changes: []ChangeByVersionType{{"rev": "2-b"}, {"rev": "2-a"}},
branched: true,
collectionID: collectionID,
CurrentVersion: &Version{SourceID: source, Value: version},
Expand Down Expand Up @@ -1753,7 +1753,7 @@ func TestConflicts(t *testing.T) {
assert.Equal(t, &ChangeEntry{
Seq: SequenceID{Seq: 4},
ID: "doc",
Changes: []ChangeRev{{"rev": "2-a"}, {"rev": rev3}},
Changes: []ChangeByVersionType{{"rev": "2-a"}, {"rev": rev3}},
branched: true,
collectionID: collectionID,
CurrentVersion: &Version{SourceID: bucketUUID, Value: doc.Cas},
Expand Down
4 changes: 4 additions & 0 deletions db/revision_cache_interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,10 @@ func (rev *DocumentRevision) Inject1xBodyProperties(ctx context.Context, db *Dat
{Key: BodyRev, Val: rev.RevID},
}

if rev.CV != nil {
kvPairs = append(kvPairs, base.KVPair{Key: BodyCV, Val: rev.CV.String()})
}

if requestedHistory != nil {
kvPairs = append(kvPairs, base.KVPair{Key: BodyRevisions, Val: requestedHistory})
}
Expand Down
4 changes: 2 additions & 2 deletions db/sequence_id.go
Original file line number Diff line number Diff line change
Expand Up @@ -120,11 +120,11 @@ func parseIntegerSequenceID(str string) (SequenceID, error) {
return SequenceID{}, err
}
} else {
return SequenceID{}, base.HTTPErrorf(400, "Invalid sequence")
return SequenceID{}, base.HTTPErrorf(400, "Invalid sequence: %q", str)
}

if err != nil {
return SequenceID{}, base.HTTPErrorf(400, "Invalid sequence")
return SequenceID{}, base.HTTPErrorf(400, "Invalid sequence: %q", str)
}
return s, nil
}
Expand Down
20 changes: 14 additions & 6 deletions docs/api/components/schemas.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -545,14 +545,22 @@ Changes-feed:
description: The document ID the change happened on.
type: string
changes:
description: List of document leafs with each leaf containing only a `rev` field.
description: List of document leafs with each leaf containing only a `rev` or `cv` field.
type: array
items:
type: object
properties:
rev:
description: The new revision that was caused by that change.
type: string
oneOf:
- type: object
properties:
rev:
description: The Revision Tree ID associated with the change. This may be returned even when the `version_type` preference is set to `cv` if this document was an old revision written prior to upgrading to SG 4.0+
type: string
title: Revision Tree ID
- 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+
type: string
title: HLV Current Version
uniqueItems: true
uniqueItems: true
last_seq:
Expand Down
22 changes: 22 additions & 0 deletions docs/api/paths/admin/keyspace-_changes.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,18 @@ get:
schema:
type: boolean
default: false
- name: version_type
in: query
description: The preferred type of document versioning to use for the changes feed.
schema:
type: string
default: rev
enum:
- rev
- cv
x-enumDescriptions:
rev: 'Revision Tree IDs. For example: 1-293a80ce8f4874724732f27d35b3959a13cd96e0'
cv: 'HLV Current Version. For example: 1854e4e557cc0000@zTWkmBiYZgNQo7BHVZrB/Q'
responses:
'200':
$ref: ../../components/responses.yaml#/changes-feed
Expand Down Expand Up @@ -166,6 +178,16 @@ post:
description: 'When true, ensures all valid documents written prior to the request being issued are included in the response. This is only applicable for non-continuous feeds.'
type: boolean
default: false
version_type:
description: The preferred type of document versioning to use for the changes feed.
type: string
default: rev
enum:
- rev
- cv
x-enumDescriptions:
rev: 'Revision Tree IDs. For example: 1-293a80ce8f4874724732f27d35b3959a13cd96e0'
cv: 'HLV Current Version. For example: 1854e4e557cc0000@zTWkmBiYZgNQo7BHVZrB/Q'
responses:
'200':
$ref: ../../components/responses.yaml#/changes-feed
Expand Down
14 changes: 13 additions & 1 deletion docs/api/paths/public/keyspace-_changes.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ get:
minimum: 0
- name: feed
in: query
description: 'The type of changes feed to use. '
description: 'The type of changes feed to use.'
schema:
type: string
default: normal
Expand All @@ -90,6 +90,18 @@ get:
- longpoll
- continuous
- websocket
- name: version_type
in: query
description: 'The preferred type of document versioning to use for the changes feed.'
schema:
type: string
default: rev
enum:
- rev
- cv
x-enumDescriptions:
rev: 'Revision Tree IDs. For example: 1-293a80ce8f4874724732f27d35b3959a13cd96e0'
cv: 'HLV Current Version. For example: 1854e4e557cc0000@zTWkmBiYZgNQo7BHVZrB/Q'
responses:
'200':
$ref: ../../components/responses.yaml#/changes-feed
Expand Down
12 changes: 6 additions & 6 deletions rest/blip_api_crud_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2342,7 +2342,7 @@ func TestRemovedMessageWithAlternateAccess(t *testing.T) {

changes := rt.WaitForChanges(1, "/{{.keyspace}}/_changes?since=0&revocations=true", "user", true)
assert.Equal(t, "doc", changes.Results[0].ID)
RequireChangeRevVersion(t, version, changes.Results[0].Changes[0])
RequireChangeRev(t, version, changes.Results[0].Changes[0], db.ChangesVersionTypeRevTreeID)

btcRunner.StartOneshotPull(btc.id)
_ = btcRunner.WaitForVersion(btc.id, docID, version)
Expand All @@ -2351,7 +2351,7 @@ func TestRemovedMessageWithAlternateAccess(t *testing.T) {

changes = rt.WaitForChanges(1, fmt.Sprintf("/{{.keyspace}}/_changes?since=%s&revocations=true", changes.Last_Seq), "user", true)
assert.Equal(t, docID, changes.Results[0].ID)
RequireChangeRevVersion(t, version, changes.Results[0].Changes[0])
RequireChangeRev(t, version, changes.Results[0].Changes[0], db.ChangesVersionTypeRevTreeID)

btcRunner.StartOneshotPull(btc.id)
_ = btcRunner.WaitForVersion(btc.id, docID, version)
Expand All @@ -2362,11 +2362,11 @@ func TestRemovedMessageWithAlternateAccess(t *testing.T) {

changes = rt.WaitForChanges(2, fmt.Sprintf("/{{.keyspace}}/_changes?since=%s&revocations=true", changes.Last_Seq), "user", true)
assert.Equal(t, "doc", changes.Results[0].ID)
RequireChangeRevVersion(t, version, changes.Results[0].Changes[0])
RequireChangeRev(t, version, changes.Results[0].Changes[0], db.ChangesVersionTypeRevTreeID)
assert.Equal(t, "3-1bc9dd04c8a257ba28a41eaad90d32de", changes.Results[0].Changes[0]["rev"])
assert.False(t, changes.Results[0].Revoked)
assert.Equal(t, "docmarker", changes.Results[1].ID)
RequireChangeRevVersion(t, docMarkerVersion, changes.Results[1].Changes[0])
RequireChangeRev(t, docMarkerVersion, changes.Results[1].Changes[0], db.ChangesVersionTypeRevTreeID)
assert.Equal(t, "1-999bcad4aab47f0a8a24bd9d3598060c", changes.Results[1].Changes[0]["rev"])
assert.False(t, changes.Results[1].Revoked)

Expand Down Expand Up @@ -2427,7 +2427,7 @@ func TestRemovedMessageWithAlternateAccessAndChannelFilteredReplication(t *testi

changes := rt.WaitForChanges(1, "/{{.keyspace}}/_changes?since=0&revocations=true", "user", true)
assert.Equal(t, docID, changes.Results[0].ID)
RequireChangeRevVersion(t, version, changes.Results[0].Changes[0])
RequireChangeRev(t, version, changes.Results[0].Changes[0], db.ChangesVersionTypeRevTreeID)

btcRunner.StartOneshotPull(btc.id)
_ = btcRunner.WaitForVersion(btc.id, docID, version)
Expand All @@ -2438,7 +2438,7 @@ func TestRemovedMessageWithAlternateAccessAndChannelFilteredReplication(t *testi
// At this point changes should send revocation, as document isn't in any of the user's channels
changes = rt.WaitForChanges(1, "/{{.keyspace}}/_changes?filter=sync_gateway/bychannel&channels=A&since=0&revocations=true", "user", true)
assert.Equal(t, docID, changes.Results[0].ID)
RequireChangeRevVersion(t, version, changes.Results[0].Changes[0])
RequireChangeRev(t, version, changes.Results[0].Changes[0], db.ChangesVersionTypeRevTreeID)

btcRunner.StartPullSince(btc.id, BlipTesterPullOptions{Channels: "A", Continuous: false})
_ = btcRunner.WaitForVersion(btc.id, docID, version)
Expand Down
20 changes: 10 additions & 10 deletions rest/blip_channel_filter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func TestChannelFilterRemovalFromChannel(t *testing.T) {

client := btcRunner.SingleCollection(btc.id)
const docID = "doc1"
version1 := rt.PutDoc("doc1", `{"channels":["A"]}`)
version1 := rt.PutDocDirectly("doc1", JsonToMap(t, `{"channels":["A"]}`))
rt.WaitForPendingChanges()

response := rt.SendUserRequest("GET", "/{{.keyspace}}/_changes?since=0&channels=A&include_docs=true", "", "alice")
Expand All @@ -56,20 +56,20 @@ func TestChannelFilterRemovalFromChannel(t *testing.T) {
{
"results": [
{"seq":1, "id": "_user/alice", "changes":[]},
{"seq":3, "id": "doc1", "doc": {"_id": "doc1", "_rev":"%s", "channels": ["A"]}, "changes": [{"rev":"%s"}]}
{"seq":3, "id": "doc1", "doc": {"_id": "doc1", "_rev":"%s", "_cv":"%s", "channels": ["A"]}, "changes": [{"rev":"%s"}]}
],
"last_seq": "3"
}`, version1.RevTreeID, version1.RevTreeID)
}`, version1.RevTreeID, version1.CV.String(), version1.RevTreeID)
require.JSONEq(t, expectedChanges1, string(response.BodyBytes()))

client.StartPullSince(BlipTesterPullOptions{Continuous: false, Since: "0", Channels: "A"})

btcRunner.WaitForVersion(btc.id, docID, version1)

// remove channel A from doc1
version2 := rt.UpdateDoc(docID, version1, `{"channels":["B"]}`)
version2 := rt.UpdateDocDirectly(docID, version1, JsonToMap(t, `{"channels":["B"]}`))
markerDocID := "marker"
markerDocVersion := rt.PutDoc(markerDocID, `{"channels":["A"]}`)
markerDocVersion := rt.PutDocDirectly(markerDocID, JsonToMap(t, `{"channels":["A"]}`))
rt.WaitForPendingChanges()

// alice will see doc1 rev2 with body
Expand All @@ -79,11 +79,11 @@ func TestChannelFilterRemovalFromChannel(t *testing.T) {
aliceExpectedChanges2 := fmt.Sprintf(`
{
"results": [
{"seq":4, "id": "%s", "doc": {"_id": "%s", "_rev":"%s", "channels": ["B"]}, "changes": [{"rev":"%s"}]},
{"seq":5, "id": "%s", "doc": {"_id": "%s", "_rev":"%s", "channels": ["A"]}, "changes": [{"rev":"%s"}]}
{"seq":4, "id": "%s", "doc": {"_id": "%s", "_rev":"%s", "_cv":"%s", "channels": ["B"]}, "changes": [{"rev":"%s"}]},
{"seq":5, "id": "%s", "doc": {"_id": "%s", "_rev":"%s", "_cv":"%s", "channels": ["A"]}, "changes": [{"rev":"%s"}]}
],
"last_seq": "5"
}`, docID, docID, version2.RevTreeID, version2.RevTreeID, markerDocID, markerDocID, markerDocVersion.RevTreeID, markerDocVersion.RevTreeID)
}`, docID, docID, version2.RevTreeID, version2.CV.String(), version2.RevTreeID, markerDocID, markerDocID, markerDocVersion.RevTreeID, markerDocVersion.CV.String(), markerDocVersion.RevTreeID)
require.JSONEq(t, aliceExpectedChanges2, string(response.BodyBytes()))

client.StartPullSince(BlipTesterPullOptions{Continuous: false, Since: "0", Channels: "A"})
Expand All @@ -105,10 +105,10 @@ func TestChannelFilterRemovalFromChannel(t *testing.T) {
{
"results": [
{"seq":4, "id": "doc1", "removed":["A"], "doc": {"_id": "doc1", "_rev":"%s", "_removed": true}, "changes": [{"rev":"%s"}]},
{"seq":5, "id": "%s", "doc": {"_id": "%s", "_rev":"%s", "channels": ["A"]}, "changes": [{"rev":"%s"}]}
{"seq":5, "id": "%s", "doc": {"_id": "%s", "_rev":"%s", "_cv": "%s", "channels": ["A"]}, "changes": [{"rev":"%s"}]}
],
"last_seq": "5"
}`, version2.RevTreeID, version2.RevTreeID, markerDocID, markerDocID, markerDocVersion.RevTreeID, markerDocVersion.RevTreeID)
}`, version2.RevTreeID, version2.RevTreeID, markerDocID, markerDocID, markerDocVersion.RevTreeID, markerDocVersion.CV.String(), markerDocVersion.RevTreeID)
require.JSONEq(t, bobExpectedChanges2, string(response.BodyBytes()))
})
}
Expand Down
Loading
Loading