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

RIT3shSapata · 2025-11-12T09:16:40Z

CBG-3689

Describe your PR here...

CheckProposedRev does not unmarshall the entire history to check if the current version is deleted or not
Added a new level for unmarshal that will unmarshall only the current version and the flags
Added unit test to test CheckProposedRev

Pre-review checklist

Removed debug logging (fmt.Print, log.Print, ...)
Logging sensitive data? Make sure it's tagged (e.g. base.UD(docID), base.MD(dbName))
Updated relevant information in the API specifications (such as endpoint descriptions, schemas, ...) in docs/api

Dependencies (if applicable)

Link upstream PRs
Update Go module dependencies when merged

Integration Tests

GSI=true,xattrs=true https://jenkins.sgwdev.com/job/SyncGatewayIntegration/162/

Copilot

Pull Request Overview

This PR optimizes CheckProposedRev by introducing a new unmarshal level (DocUnmarshalRevAndFlags) that only unmarshals the current revision and flags, avoiding the expensive operation of unmarshalling the entire document history when checking if a document is deleted.

Key Changes:

Added DocUnmarshalRevAndFlags constant for selective unmarshalling
Moved IsDeleted() and hasFlag() methods from Document to SyncData struct
Simplified CheckProposedRev to use the new unmarshal level instead of conditionally loading full history

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
db/document.go	Added new unmarshal level and moved deletion checking methods to SyncData
db/crud.go	Simplified CheckProposedRev to use the new unmarshal level
db/crud_test.go	Added comprehensive unit tests for CheckProposedRev functionality

Copilot · 2025-11-12T09:17:27Z

db/document.go

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


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 · 2025-11-12T09:17:27Z

db/document.go

 		}
 		doc._rawBody = data
+	case DocUnmarshalRevAndFlags:
+		// Unmarshal rev ,cas and flags from sync metadata


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 · 2025-11-12T09:17:27Z

db/crud.go

 		// 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) {


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() {

torcolvin

LGTM in general, I have left some more comments to pick up on go, Sync Gateway style, and the level of testing I prefer. Some of this is idealism but I'd rather note it here when it is easy to achieve and we can discuss how much to do on future tickets when this is harder to achieve.

torcolvin · 2025-11-12T13:14:18Z

db/crud_test.go

+
+func TestProposedRev(t *testing.T) {
+
+	base.SetUpTestLogging(t, base.LevelDebug, base.KeyAll)


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.

torcolvin · 2025-11-12T13:16:36Z

db/crud_test.go

+	defer db.Close(ctx)
+	collection, ctx := GetSingleDatabaseCollectionWithUser(ctx, t, db)
+
+	// create 3 documents


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

torcolvin · 2025-11-12T13:17:27Z

db/crud_test.go

+		revID          string
+		parentRevID    string
+		expectedStatus ProposedRevStatus
+		currentRev     string


Suggested change

currentRev string

expectedCurrentRev string

This indicates it is the return value of the struct

torcolvin · 2025-11-12T13:27:07Z

db/crud_test.go

+		docID          string
+	}{
+		{
+			name:           "no existing document",


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.

db/document.go

torcolvin · 2025-11-12T13:29:40Z

db/document.go

 	CurrentRev channels.RevAndVersion `json:"rev"`
 }

+type revAndFlagsSyncData struct {


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 {

torcolvin · 2025-11-12T13:31:39Z

db/document.go

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


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)

use flags to check if version is deleted

f6a0add

RIT3shSapata requested a review from torcolvin November 12, 2025 09:16

RIT3shSapata assigned torcolvin Nov 12, 2025

Copilot AI review requested due to automatic review settings November 12, 2025 09:16

Copilot AI reviewed Nov 12, 2025

View reviewed changes

RIT3shSapata added 2 commits November 12, 2025 14:49

lint fix

fb2c95e

fix few nits

19d48a5

torcolvin reviewed Nov 12, 2025

View reviewed changes

torcolvin assigned RIT3shSapata and unassigned torcolvin Nov 12, 2025

fixes based on pr comments and added new test cases

713a601

RIT3shSapata requested a review from torcolvin November 13, 2025 15:33

RIT3shSapata assigned torcolvin and unassigned RIT3shSapata Nov 13, 2025

torcolvin approved these changes Nov 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

RIT3shSapata commented Nov 12, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Nov 12, 2025

Uh oh!

Copilot AI Nov 12, 2025

Uh oh!

Copilot AI Nov 12, 2025

Uh oh!

torcolvin left a comment

Uh oh!

torcolvin Nov 12, 2025

Uh oh!

torcolvin Nov 12, 2025

Uh oh!

torcolvin Nov 12, 2025

Uh oh!

torcolvin Nov 12, 2025

Uh oh!

Uh oh!

Uh oh!

torcolvin Nov 12, 2025

Uh oh!

torcolvin Nov 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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

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

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


		func TestProposedRev(t *testing.T) {

		base.SetUpTestLogging(t, base.LevelDebug, base.KeyAll)

	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 {

	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)

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

Are you sure you want to change the base?

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

Conversation

RIT3shSapata commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-review checklist

Dependencies (if applicable)

Integration Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

torcolvin left a comment

Choose a reason for hiding this comment

Uh oh!

torcolvin Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

torcolvin Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

torcolvin Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

torcolvin Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

torcolvin Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

torcolvin Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

RIT3shSapata commented Nov 12, 2025 •

edited

Loading