Address PR comments (move logic into Unmarshal only - and don't set channel info for winning rev)

Author: bbrks

db/blip_sync_context.go

@@ -364,7 +364,7 @@ func (bsc *BlipSyncContext) handleChangesResponse(sender *blip.Sender, response
 				sentSeqs = append(sentSeqs, seq)
 			}
 		} else {
-			base.DebugfCtx(bsc.loggingCtx, base.KeySync, "Peer didn't want revision %s / %s (seq:%v)", base.UD(docID), revID, seq)
+			base.DebugfCtx(bsc.loggingCtx, base.KeySync, "Peer didn't want revision %s/%s (seq:%v)", base.UD(docID), revID, seq)
 			if collectionCtx.sgr2PushAlreadyKnownSeqsCallback != nil {
 				alreadyKnownSeqs = append(alreadyKnownSeqs, seq)
 			}

db/crud.go

@@ -1770,7 +1770,8 @@ func (col *DatabaseCollectionWithUser) documentUpdateFunc(ctx context.Context, d
 		return
 	}
 
-	if len(channelSet) > 0 {
+	isWinningRev := doc.CurrentRev == newRevID
+	if len(channelSet) > 0 && !isWinningRev {
 		doc.History[newRevID].Channels = channelSet
 	}
 
@@ -1797,7 +1798,7 @@ func (col *DatabaseCollectionWithUser) documentUpdateFunc(ctx context.Context, d
 				return
 			}
 		}
-		_, err = doc.updateChannels(ctx, channelSet)
+		_, err = doc.updateChannels(ctx, isWinningRev, channelSet)
 		if err != nil {
 			return
 		}
@@ -2011,7 +2012,11 @@ func (db *DatabaseCollectionWithUser) updateAndReturnDoc(ctx context.Context, do
 			return nil, "", err
 		}
 
-		revChannels := doc.History[newRevID].Channels
+		revChannels, ok := doc.channelsForRev(newRevID)
+		if !ok {
+			// Should be unreachable, as we've already checked History[newRevID] above ...
+			return nil, "", base.RedactErrorf("unable to determine channels for %s/%s", base.UD(docid), newRevID)
+		}
 		documentRevision := DocumentRevision{
 			DocID:            docid,
 			RevID:            newRevID,

db/database.go

@@ -1734,9 +1734,11 @@ func (db *DatabaseCollectionWithUser) getResyncedDocument(ctx context.Context, d
 			access = nil
 			channels = nil
 		}
-		rev.Channels = channels
 
-		if rev.ID == doc.CurrentRev {
+		isWinningRev := rev.ID == doc.CurrentRev
+		if !isWinningRev {
+			rev.Channels = channels
+		} else {
 			if regenerateSequences {
 				updatedUnusedSequences, err = db.assignSequence(ctx, 0, doc, unusedSequences)
 				if err != nil {
@@ -1745,7 +1747,7 @@ func (db *DatabaseCollectionWithUser) getResyncedDocument(ctx context.Context, d
 				forceUpdate = true
 			}
 
-			changedChannels, err := doc.updateChannels(ctx, channels)
+			changedChannels, err := doc.updateChannels(ctx, isWinningRev, channels)
 			changed = len(doc.Access.updateAccess(ctx, doc, access)) +
 				len(doc.RoleAccess.updateAccess(ctx, doc, roles)) +
 				len(changedChannels)

db/document.go

@@ -973,7 +973,7 @@ func (doc *Document) addToChannelSetHistory(channelName string, historyEntry Cha
 
 // Updates the Channels property of a document object with current & past channels.
 // Returns the set of channels that have changed (document joined or left in this revision)
-func (doc *Document) updateChannels(ctx context.Context, newChannels base.Set) (changedChannels base.Set, err error) {
+func (doc *Document) updateChannels(ctx context.Context, isWinningRev bool, newChannels base.Set) (changedChannels base.Set, err error) {
 	var changed []string
 	oldChannels := doc.Channels
 	if oldChannels == nil {
@@ -1002,7 +1002,9 @@ func (doc *Document) updateChannels(ctx context.Context, newChannels base.Set) (
 			doc.updateChannelHistory(channel, doc.Sequence, true)
 		}
 	}
-	doc.currentRevChannels = newChannels
+	if isWinningRev {
+		doc.SyncData.currentRevChannels = newChannels
+	}
 	if changed != nil {
 		base.InfofCtx(ctx, base.KeyCRUD, "\tDoc %q / %q in channels %q", base.UD(doc.ID), doc.CurrentRev, base.UD(newChannels))
 		changedChannels, err = channels.SetFromArray(changed, channels.KeepStar)

db/revtree.go

@@ -63,8 +63,6 @@ func (tree RevTree) MarshalJSON() ([]byte, error) {
 	}
 	revIndexes := map[string]int{"": -1}
 
-	winner, _, _ := tree.winningRevision(context.TODO())
-
 	i := 0
 	for _, info := range tree {
 		revIndexes[info.ID] = i
@@ -86,7 +84,7 @@ func (tree RevTree) MarshalJSON() ([]byte, error) {
 		}
 
 		// for non-winning leaf revisions we'll store channel information
-		if winner != info.ID && len(info.Channels) > 0 {
+		if len(info.Channels) > 0 {
 			if rep.ChannelsMap == nil {
 				rep.ChannelsMap = make(map[string]base.Set, 1)
 			}
@@ -192,10 +190,12 @@ func (tree *RevTree) UnmarshalJSON(inputjson []byte) (err error) {
 
 	// we shouldn't be in a situation where we have both channels and channelsMap populated, but we still need to handle the old format
 	if len(rep.Channels_Old) > 0 {
+		winner, _, _ := tree.winningRevision(context.TODO())
 		leaves := tree.Leaves()
 		for i, channels := range rep.Channels_Old {
 			info := (*tree)[rep.Revs[i]]
-			if _, isLeaf := leaves[info.ID]; isLeaf {
+			// Ensure this leaf is not a winner (channels not stored in revtree for winning revision)
+			if _, isLeaf := leaves[info.ID]; isLeaf && info.ID != winner {
 				// set only if we've not already populated from ChannelsMap (we shouldn't ever have both)
 				if info.Channels != nil {
 					return fmt.Errorf("RevTree leaf %q had channels set already (from channelsMap)", info.ID)

db/revtree_test.go

@@ -229,40 +229,43 @@ func TestRevTreeUnmarshalOldFormat(t *testing.T) {
 	"bodies": ["{\"foo\":\"bar\"}", "", ""],
 	"channels": [["ABC", "CBS"], null, ["ABC"]]
 }`
-	tree := testmap.copy()
+	expected := testmap.copy()
 
-	// set desired channels
-	ri, err := tree.getInfo("3-three")
+	// winning rev channel information no longer present in revtree,
+	// expect to drop it at unmarshal time if it's still in the raw JSON
+	ri, err := expected.getInfo("3-three")
 	require.NoError(t, err)
-	ri.Channels = base.SetOf("ABC", "CBS")
+	ri.Channels = nil
 
-	assertRevTreeUnmarshal(t, testJSON, tree)
+	assertRevTreeUnmarshal(t, testJSON, expected)
 }
 
-func TestRevTreeUnmarshalChannelMap(t *testing.T) {
+func TestRevTreeUnmarshalOldFormatNonWinningRev(t *testing.T) {
 	// we moved non-winning leaf revision channel information into a 'channelMap' property instead to handle the case where documents are in conflict.
 	const testJSON = `{
 	"revs": ["3-drei", "3-three", "2-two", "1-one"],
 	"parents": [2, 2, 3, -1],
 	"bodies": ["{\"foo\":\"buzz\"}", "{\"foo\":\"bar\"}", "", ""],
 	"channels": [["DE"], ["EN"], null, ["ABC"]]
 }`
-	tree := testmap.copy()
+	expected := testmap.copy()
 
-	// set desired channels
-	ri, err := tree.getInfo("3-three")
+	// winning rev channel information no longer present in revtree,
+	// expect to drop it at unmarshal time if it's still in the raw JSON
+	ri, err := expected.getInfo("3-three")
 	require.NoError(t, err)
-	ri.Channels = base.SetOf("EN")
+	ri.Channels = nil
 
-	err = tree.addRevision("", RevInfo{
+	// non-winning revisions do retain channel information, so populate this for the expected expected
+	err = expected.addRevision("", RevInfo{
 		ID:       "3-drei",
 		Parent:   "2-two",
 		Body:     []byte(`{"foo":"buzz"}`),
 		Channels: base.SetOf("DE"),
 	})
 	require.NoError(t, err)
 
-	assertRevTreeUnmarshal(t, testJSON, tree)
+	assertRevTreeUnmarshal(t, testJSON, expected)
 }
 
 func TestRevTreeUnmarshal(t *testing.T) {
@@ -390,15 +393,22 @@ func TestRevTreeChannelMapLeafOnly(t *testing.T) {
 	//	                       └─│ 3-drei (DE) │
 	//	                         └─────────────┘
 	tree := branchymap.copy()
-	err := tree.addRevision(t.Name(), RevInfo{
+
+	ri, err := tree.getInfo("3-three")
+	require.NoError(t, err)
+	// this wouldn't have been set normally (winning rev),
+	// but let's force it to ensure we're not storing old revs in ChannelsMap
+	ri.Channels = base.SetOf("EN")
+
+	err = tree.addRevision(t.Name(), RevInfo{
 		ID:       "4-four",
 		Parent:   "3-three",
-		Channels: base.SetOf("EN-us", "EN-gb"),
+		Channels: nil, // we don't store channels for winning revs
 	})
 	require.NoError(t, err)
 
 	// insert channel into tree - we don't store it in the globals because each test requires different channel data.
-	ri, err := tree.getInfo("3-drei")
+	ri, err = tree.getInfo("3-drei")
 	require.NoError(t, err)
 	ri.Channels = base.SetOf("DE")