From b63995c5ed9ce24739d5f0cf23b35d9930acdb52 Mon Sep 17 00:00:00 2001 From: Ben Brooks Date: Tue, 7 Nov 2023 18:07:15 +0000 Subject: [PATCH] Address PR comments (move logic into Unmarshal only - and don't set channel info for winning rev) --- db/blip_sync_context.go | 2 +- db/crud.go | 11 ++++++++--- db/database.go | 8 +++++--- db/document.go | 5 ++++- db/revtree.go | 8 ++++---- db/revtree_test.go | 40 +++++++++++++++++++++++++--------------- 6 files changed, 47 insertions(+), 27 deletions(-) diff --git a/db/blip_sync_context.go b/db/blip_sync_context.go index 841fe5d7fd..3afba0e6e9 100644 --- a/db/blip_sync_context.go +++ b/db/blip_sync_context.go @@ -371,7 +371,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) } diff --git a/db/crud.go b/db/crud.go index d5f974ce2a..c168d5b79c 100644 --- a/db/crud.go +++ b/db/crud.go @@ -1772,7 +1772,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 } @@ -1799,7 +1800,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 } @@ -2024,7 +2025,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, diff --git a/db/database.go b/db/database.go index 32d36daf6b..b99572e62b 100644 --- a/db/database.go +++ b/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) diff --git a/db/document.go b/db/document.go index 92d541b0f7..54b3de226c 100644 --- a/db/document.go +++ b/db/document.go @@ -975,7 +975,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 { @@ -1004,6 +1004,9 @@ func (doc *Document) updateChannels(ctx context.Context, newChannels base.Set) ( doc.updateChannelHistory(channel, doc.Sequence, true) } } + 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) diff --git a/db/revtree.go b/db/revtree.go index 5616879db9..42367bd763 100644 --- a/db/revtree.go +++ b/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) diff --git a/db/revtree_test.go b/db/revtree_test.go index 3bfc9e03ac..da9d699ad8 100644 --- a/db/revtree_test.go +++ b/db/revtree_test.go @@ -229,17 +229,18 @@ 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"], @@ -247,14 +248,16 @@ func TestRevTreeUnmarshalChannelMap(t *testing.T) { "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"}`), @@ -262,7 +265,7 @@ func TestRevTreeUnmarshalChannelMap(t *testing.T) { }) 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")