Skip to content

CBG-3689: CheckProposedRev does not unmarshall the entire history #7875

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+214 −21
Open

CBG-3689: CheckProposedRev does not unmarshall the entire history #7875

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f6a0add
use flags to check if version is deleted
RIT3shSapata Nov 12, 2025
fb2c95e
lint fix
RIT3shSapata Nov 12, 2025
19d48a5
fix few nits
RIT3shSapata Nov 12, 2025
713a601
fixes based on pr comments and added new test cases
RIT3shSapata Nov 13, 2025
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
7 changes: 2 additions & 5 deletions db/crud.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3626,10 +3626,7 @@ func (db *DatabaseCollectionWithUser) CheckProposedRev(ctx context.Context, doci
return ProposedRev_OK, "" // Users can't upload design docs, so ignore them
}

level := DocUnmarshalRev
if parentRevID == "" {
level = DocUnmarshalHistory // doc.History only needed in this case (see below)
}
level := DocUnmarshalRevAndFlags
syncData, _, err := db.GetDocSyncDataNoImport(ctx, docid, level)
if err != nil {
if !base.IsDocNotFoundError(err) && !base.IsXattrNotFoundError(err) {
Expand All @@ -3644,7 +3641,7 @@ func (db *DatabaseCollectionWithUser) CheckProposedRev(ctx context.Context, doci
} else if syncData.GetRevTreeID() == parentRevID {
// Proposed rev's parent is my current revision; OK to add:
return ProposedRev_OK, ""
} else if parentRevID == "" && syncData.History[syncData.GetRevTreeID()].Deleted {
} else if parentRevID == "" && syncData.IsDeleted() {
// Proposed rev has no parent and doc is currently deleted; OK to add:
return ProposedRev_OK, ""
} else {
Expand Down
173 changes: 172 additions & 1 deletion db/crud_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1810,7 +1810,7 @@ func TestPutExistingCurrentVersion(t *testing.T) {

// Update the client's HLV to include the latest SGW version.
incomingHLV.PreviousVersions[currentSourceID] = docUpdateVersion
// TODO: because currentRev isn't being updated, storeOldBodyInRevTreeAndUpdateCurrent isn't
// TODO: because expectedCurrentRev isn't being updated, storeOldBodyInRevTreeAndUpdateCurrent isn't
// updating the document body. Need to review whether it makes sense to keep using
// storeOldBodyInRevTreeAndUpdateCurrent, or if this needs a larger overhaul to support VV
doc, cv, _, err = collection.PutExistingCurrentVersion(ctx, opts)
Expand Down Expand Up @@ -2282,3 +2282,174 @@ func TestSyncDataCVEqual(t *testing.T) {
})
}
}

func TestProposedRev(t *testing.T) {
db, ctx := setupTestDB(t)
defer db.Close(ctx)
collection, ctx := GetSingleDatabaseCollectionWithUser(ctx, t, db)

// create 3 documents
Copy link
Collaborator

@torcolvin torcolvin Nov 12, 2025

Choose a reason for hiding this comment

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

Nit:

To make this easier to read and debug in the future I think I would do something like name these documents with what they are:

const (
singleRevDoc = "singleRevDoc"
twoRevDoc = "twoRevDoc"
tombstonedDoc = "tombstonedDoc"
)

const (
SingleRevDoc = "SingleRevDoc"
MultiRevDoc = "MultiRevDoc"
TombstonedDoc = "TombstonedDoc"
)
body := Body{"key1": "value1", "key2": 1234}
_, doc1, err := collection.Put(ctx, SingleRevDoc, body)
require.NoError(t, err)
doc1Rev := doc1.GetRevTreeID()

_, doc2, err := collection.Put(ctx, MultiRevDoc, body)
require.NoError(t, err)
doc2Rev1 := doc2.GetRevTreeID()
_, doc2, err = collection.Put(ctx, MultiRevDoc, Body{"_rev": doc2Rev1, "key1": "value2", "key2": 5678})
require.NoError(t, err)
doc2Rev2 := doc2.GetRevTreeID()
_, doc2, err = collection.Put(ctx, MultiRevDoc, Body{"_rev": doc2Rev2, "key1": "value3", "key2": 91011})
require.NoError(t, err)
doc2Rev3 := doc2.GetRevTreeID()

_, doc3, err := collection.Put(ctx, TombstonedDoc, body)
require.NoError(t, err)
doc3Rev1 := doc3.GetRevTreeID()
_, _, err = collection.Put(ctx, TombstonedDoc, Body{"_rev": doc3Rev1, "_deleted": true})
require.NoError(t, err)

testCases := []struct {
name string
revID string
parentRevID string
expectedStatus ProposedRevStatus
expectedCurrentRev string
docID string
}{
{
name: "no_existing_document-curr_rev-no_parent",
revID: "1-abc",
parentRevID: "",
expectedStatus: ProposedRev_OK_IsNew,
expectedCurrentRev: "",
docID: "doc",
},
{
name: "no_existing_document-curr_rev-with_parent",
revID: "2-def",
parentRevID: "1-abc",
expectedStatus: ProposedRev_OK_IsNew,
expectedCurrentRev: "",
docID: "doc",
},
{
name: "one_rev_doc-curr_rev-without_parent",
revID: doc1Rev,
parentRevID: "",
expectedStatus: ProposedRev_Exists,
expectedCurrentRev: "",
docID: SingleRevDoc,
},
{
name: "one_rev_doc-incorrect_curr_rev-without_parent",
revID: "1-abc",
parentRevID: "",
expectedStatus: ProposedRev_Conflict,
expectedCurrentRev: doc1Rev,
docID: SingleRevDoc,
},
{
name: "one_rev_doc-new_curr_rev-without_parent",
revID: "2-abc",
parentRevID: doc1Rev,
expectedStatus: ProposedRev_OK,
expectedCurrentRev: "",
docID: SingleRevDoc,
},
{
name: "multi_rev_doc-rev1-without_parent",
revID: doc2Rev1,
parentRevID: "",
expectedStatus: ProposedRev_Conflict,
expectedCurrentRev: doc2Rev3,
docID: MultiRevDoc,
},
{
name: "multi_rev_doc-rev2-with_rev1_parent",
revID: doc2Rev2,
parentRevID: doc2Rev1,
expectedStatus: ProposedRev_Conflict,
expectedCurrentRev: doc2Rev3,
docID: MultiRevDoc,
},
{
name: "multi_rev_doc-rev2-with_incorrect_parent",
revID: doc2Rev2,
parentRevID: "1-abc",
expectedStatus: ProposedRev_Conflict,
expectedCurrentRev: doc2Rev3,
docID: MultiRevDoc,
},
{
name: "multi_rev_doc-rev2-without_parent",
revID: doc2Rev2,
parentRevID: "",
expectedStatus: ProposedRev_Conflict,
expectedCurrentRev: doc2Rev3,
docID: MultiRevDoc,
},
{
name: "multi_rev_doc-conflicting_rev2-with_parent",
revID: "2-abc",
parentRevID: doc2Rev1,
expectedStatus: ProposedRev_Conflict,
expectedCurrentRev: doc2Rev3,
docID: MultiRevDoc,
},
{
name: "multi_rev_doc-conflicting_rev3-without_parent",
revID: doc2Rev3,
parentRevID: "",
expectedStatus: ProposedRev_Exists,
expectedCurrentRev: "",
docID: MultiRevDoc,
},
{
name: "multi_rev_doc-conflicting_rev3-with_parent",
revID: doc2Rev3,
parentRevID: doc2Rev2,
expectedStatus: ProposedRev_Exists,
expectedCurrentRev: "",
docID: MultiRevDoc,
},
{
name: "multi_rev_doc-conflicting_rev3-with_incorrect_parent",
revID: doc2Rev3,
parentRevID: doc2Rev1,
expectedStatus: ProposedRev_Exists,
expectedCurrentRev: "",
docID: MultiRevDoc,
},
{
name: "multi_rev_doc-conflicting_incorrect_rev3-with_parent",
revID: "3-abc",
parentRevID: doc2Rev2,
expectedStatus: ProposedRev_Conflict,
expectedCurrentRev: doc2Rev3,
docID: MultiRevDoc,
},
{
name: "new revision with previous revision as tombstone",
revID: "1-abc",
parentRevID: "",
expectedStatus: ProposedRev_OK,
expectedCurrentRev: "",
docID: TombstonedDoc,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
status, rev := collection.CheckProposedRev(ctx, tc.docID, tc.revID, tc.parentRevID)
assert.Equal(t, tc.expectedStatus, status)
assert.Equal(t, tc.expectedCurrentRev, rev)
})
}
}
55 changes: 40 additions & 15 deletions db/document.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,14 @@ const DocumentHistoryMaxEntriesPerChannel = 5
type DocumentUnmarshalLevel uint8

const (
DocUnmarshalAll = DocumentUnmarshalLevel(iota) // Unmarshals metadata and body
DocUnmarshalSync // Unmarshals metadata
DocUnmarshalNoHistory // Unmarshals metadata excluding revtree history
DocUnmarshalHistory // Unmarshals revtree history + rev + CAS only
DocUnmarshalRev // Unmarshals revTreeID + CAS only (no HLV)
DocUnmarshalCAS // Unmarshals CAS (for import check) only
DocUnmarshalNone // No unmarshalling (skips import/upgrade check)
DocUnmarshalAll = DocumentUnmarshalLevel(iota) // Unmarshals metadata and body
DocUnmarshalSync // Unmarshals metadata
DocUnmarshalNoHistory // Unmarshals metadata excluding revtree history
DocUnmarshalHistory // Unmarshals revtree history + rev + CAS only
DocUnmarshalRev // Unmarshals revTreeID + CAS only (no HLV)
DocUnmarshalCAS // Unmarshals CAS (for import check) only
DocUnmarshalNone // No unmarshalling (skips import/upgrade check)
DocUnmarshalRevAndFlags // Unmarshals revTreeID + CAS and Flags (no HLV)
)

const (
Expand Down Expand Up @@ -167,6 +168,16 @@ func (sd *SyncData) SetCV(hlv *HybridLogicalVector) {
sd.RevAndVersion.CurrentVersion = string(base.Uint64CASToLittleEndianHex(hlv.Version))
}

// hasFlag returns true if the document has this bit flag
func (sd *SyncData) hasFlag(flag uint8) bool {
return sd.Flags&flag != 0
}

// IsDeleted returns true if the document metadata indicates it is a tombstone.
func (sd *SyncData) IsDeleted() bool {
return sd.hasFlag(channels.Deleted)
}

// RedactRawGlobalSyncData runs HashRedact on the given global sync data.
func RedactRawGlobalSyncData(syncData []byte, redactSalt string) ([]byte, error) {
if redactSalt == "" {
Expand Down Expand Up @@ -306,6 +317,11 @@ type revOnlySyncData struct {
CurrentRev channels.RevAndVersion `json:"rev"`
}

type revAndFlagsSyncData struct {
Copy link
Collaborator

@torcolvin torcolvin Nov 12, 2025

Choose a reason for hiding this comment

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

Suggested change
type revAndFlagsSyncData struct {
// revAndFlagsSyncData is a limited set of the SyncData suitable for faster unmarshalling. It only contains rev and flags properties
type revAndFlagsSyncData struct {

revOnlySyncData
Flags uint8 `json:"flags"`
}

type casOnlySyncData struct {
Cas string `json:"cas"`
}
Expand All @@ -331,10 +347,6 @@ func (doc *Document) MarshalBodyAndSync() (retBytes []byte, err error) {
}
}

func (doc *Document) IsDeleted() bool {
return doc.hasFlag(channels.Deleted)
}

func (doc *Document) BodyWithSpecialProperties(ctx context.Context) ([]byte, error) {
bodyBytes, err := doc.BodyBytes(ctx)
if err != nil {
Expand Down Expand Up @@ -754,10 +766,6 @@ func (doc *Document) IsSGWrite(ctx context.Context, rawBody []byte) (isSGWrite b
return true, true, false
}

func (doc *Document) hasFlag(flag uint8) bool {
return doc.Flags&flag != 0
}

func (doc *Document) setFlag(flag uint8, state bool) {
if state {
doc.Flags |= flag
Expand Down Expand Up @@ -1336,6 +1344,23 @@ func (doc *Document) UnmarshalWithXattrs(ctx context.Context, data, syncXattrDat
doc.SyncData = SyncData{}
}
doc._rawBody = data
case DocUnmarshalRevAndFlags:
// Unmarshal rev, cas and flags from sync metadata
if syncXattrData != nil {
var revOnlyMeta revAndFlagsSyncData
unmarshalErr := base.JSONUnmarshal(syncXattrData, &revOnlyMeta)
if unmarshalErr != nil {
return pkgerrors.WithStack(base.RedactErrorf("Failed to UnmarshalWithXattrs() doc with id: %s (DocUnmarshalRevAndFlags). Error: %v", base.UD(doc.ID), unmarshalErr))
Copy link
Collaborator

@torcolvin torcolvin Nov 12, 2025

Choose a reason for hiding this comment

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

nit: general go style

pkgerrors WithStack was used before go implemented its own error wrapping in go 1.13. Some of Sync Gateway's code base was written before this existed. Preferred way of doing this is to use %w on base.RedactErrorf or fmt.Errorf to use "native" go error wrapping.

Suggested change
return pkgerrors.WithStack(base.RedactErrorf("Failed to UnmarshalWithXattrs() doc with id: %s (DocUnmarshalRevAndFlags). Error: %v", base.UD(doc.ID), unmarshalErr))
return base.RedactErrorf("Failed to UnmarshalWithXattrs() doc with id: %s (DocUnmarshalRevAndFlags). Error: %w", base.UD(doc.ID), unmarshalErr)

}
doc.SyncData = SyncData{
RevAndVersion: revOnlyMeta.CurrentRev,
Cas: revOnlyMeta.Cas,
Flags: revOnlyMeta.Flags,
}
} else {
doc.SyncData = SyncData{}
}
doc._rawBody = data
}

// If there's no body, but there is an xattr, set deleted flag and initialize an empty body
Expand Down
Loading