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 1 commit
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.hasFlag(channels.Deleted) {
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

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

The hasFlag method is not exported and is being called on SyncData. Based on the code changes, IsDeleted() method was added to SyncData which should be used instead for better encapsulation and consistency.

Suggested change
} else if parentRevID == "" && syncData.hasFlag(channels.Deleted) {
} else if parentRevID == "" && syncData.IsDeleted() {

Copilot uses AI. Check for mistakes.
// 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 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()
_, doc3, err = collection.Put(ctx, "doc3", Body{"_rev": doc3Rev1, "_deleted": true})

Check failure on line 2310 in db/crud_test.go

View workflow job for this annotation

GitHub Actions / lint 

SA4006: this value of doc3 is never used (staticcheck)

Check failure on line 2310 in db/crud_test.go

View workflow job for this annotation

GitHub Actions / lint 

SA4006: this value of doc3 is never used (staticcheck)
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)
})
}
}
39 changes: 31 additions & 8 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 @@
type DocumentUnmarshalLevel uint8

const (
DocUnmarshalAll = DocumentUnmarshalLevel(iota) // Unmarshals metadata and body

Check failure on line 40 in db/document.go

View workflow job for this annotation

GitHub Actions / lint 

File is not properly formatted (goimports)

Check failure on line 40 in db/document.go

View workflow job for this annotation

GitHub Actions / lint 

File is not properly formatted (goimports)
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 @@
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 @@
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) 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 @@
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 @@
doc.SyncData = SyncData{}
}
doc._rawBody = data
case DocUnmarshalRevAndFlags:
// Unmarshal rev ,cas and flags from sync metadata
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

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

Corrected spacing after 'rev' in comment.

Suggested change
// Unmarshal rev ,cas and flags from sync metadata
// Unmarshal rev, cas and flags from sync metadata

Copilot uses AI. Check for mistakes.
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 (DocUnmarshalRev). Error: %v", base.UD(doc.ID), unmarshalErr))
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

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

Error message references 'DocUnmarshalRev' but this case is for 'DocUnmarshalRevAndFlags'. Update the error message to accurately reflect the unmarshal level being used.

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

Copilot uses AI. Check for mistakes.
}
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