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

Merged
merged 4 commits into from
Nov 14, 2025
+214 −21
Merged

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

Show file tree
Hide file tree
Changes from 3 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
94 changes: 94 additions & 0 deletions db/crud_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2282,3 +2282,97 @@ func TestSyncDataCVEqual(t *testing.T) {
})
}
}

func TestProposedRev(t *testing.T) {

base.SetUpTestLogging(t, base.LevelDebug, base.KeyAll)
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.

nitpicks:

I think this line is useful for debugging, but LevelDebug with KeyAll displays a lot of logging for rosmar that isn't helpful. When committing this, I think it's fine for this test to remove all the logging.


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"
)

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

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

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

testCases := []struct {
name string
revID string
parentRevID string
expectedStatus ProposedRevStatus
currentRev string
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
currentRev string
expectedCurrentRev string

This indicates it is the return value of the struct

docID string
}{
{
name: "no existing document",
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.

General comment, but I think this is worth addressing for this PR.

When writing a test, you want to think about the way the code is called, not just the implementation. Some of the idea of testing is to cover the cases that don't have different code pathways today but do have different code pathways in the future.

So there are several to account for and in a fast test it would be great to hit all of the possibilities

  • type of document: (no document, multiple rev, tombstone)
  • arguments (revID, parentRevID)
  • revID possibilities:
    • revID doesn't exist
    • revID does exist and is current rev
    • revID does exist and is in history
  • parentRevID
    • parentRevID doesn't exist
    • parentRevID does exist, and is the immediate parent
    • parentRevID does exist in the history but isn't the immediate parent. (In this case, you can test this by having the documents have three revisions so you can test for a past parent)

I'm not going to list all these permutations, but given how fast this test runs I think it would be fine to hit most of them. I think this is 3 * 3 * 3 which is a manageable number. If it was too many, then I think consider skip more, but this gives more coverage to this function. If this does really blow up because I did the math wrong, we definitely can consider shortening the number of cases.

the first example that strikes me is what happens when you have:

  • no existing document
  • rev: 2-def
  • parentRev: 1-def

That's a common scenario where Couchbase Lite is pushing a document that has two local revisions and Sync Gateway has none.

revID: "1-abc",
parentRevID: "",
expectedStatus: ProposedRev_OK_IsNew,
currentRev: "",
docID: "doc",
},
{
name: "existing revision with no previous version",
revID: doc1Rev,
parentRevID: "",
expectedStatus: ProposedRev_Exists,
currentRev: "",
docID: "doc1",
},
{
name: "existing revision with previous version",
revID: doc2Rev2,
parentRevID: doc2Rev1,
expectedStatus: ProposedRev_Exists,
currentRev: "",
docID: "doc2",
},
{
name: "new revision with previous version",
revID: "2-abc",
parentRevID: doc1Rev,
expectedStatus: ProposedRev_OK,
currentRev: "",
docID: "doc1",
},
{
name: "new revision with previous revision as tombstone",
revID: "1-abc",
parentRevID: "",
expectedStatus: ProposedRev_OK,
currentRev: "",
docID: "doc3",
},
{
name: "conflicting revision with previous version",
revID: "2-abc",
parentRevID: doc2Rev1,
expectedStatus: ProposedRev_Conflict,
currentRev: doc2Rev2,
docID: "doc2",
},
}

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.currentRev, rev)
})
}
}
53 changes: 38 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,14 @@ func (sd *SyncData) SetCV(hlv *HybridLogicalVector) {
sd.RevAndVersion.CurrentVersion = string(base.Uint64CASToLittleEndianHex(hlv.Version))
}

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

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 +315,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 +345,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 +764,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 +1342,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