CBG-3203: Don't store channel history for non-leaf revisions #6368
Conversation
bbrks
force-pushed
the
CBG-3203
branch
2 times, most recently
from
97e6e10
to
ce69d5a
Compare
|
@adamcfraser I think this PR is finally good for a fresh review! Thanks |
| // As a special case, you don't need channel access to see a deletion revision, | ||
| // otherwise the client's replicator can't process the deletion (since deletions | ||
| // usually aren't on any channels at all!) But don't show the full body. (See #59) | ||
| // Update: this applies to non-deletions too, since the client may have lost access to | ||
| // the channel and gotten a "removed" entry in the _changes feed. It then needs to | ||
| // incorporate that tombstone and for that it needs to see the _revisions property. | ||
| if revid == "" || doc.History[revid] == nil { | ||
| if revid == "" || doc.History[revid] == nil || errors.Is(err, ErrMissing) { |
There was a problem hiding this comment.
From the comment on line 700, it seems like we want to bypass the security check for tombstones or removals (so that we successfully set bodyBytes to either EmptyDocument or RemovedRedactedDocument below). I'm wondering if the ErrMissing handling is going to break this, in particular for removals. I went looking for a test that covers this path - TestGetRemovedAsUser seems close but I don't think it purges the removal from the rev cache before attempting to retrieve. I think it would be good to extend TestGetRemovedAsUser for this scenario to see whether it changes before/after this PR:
- removal revision is non-leaf
- removal revision is not resident in the rev cache
- verify removal is still returned
There was a problem hiding this comment.
The same might be true for tombstones, although non-leaf tombstones are more of a corner case. It would be common for removals to be non-leaf revisions.
There was a problem hiding this comment.
My interpretation of the comment was that an old rev is neither removed from a channel, nor a tombstone. Is that incorrect?
An old revision we don't have channel info for any more seems the same as somebody requesting a pruned revision, or doc1 / 123-invalid which would return ErrMissing.
There was a problem hiding this comment.
I was thinking of the usage of IsChannelRemoval in revCacheLoaderForDocument , where we check the channel history to identify removals for revisions that aren't in the rev tree, and return a removal instead of 404 for those. I wasn't clear about the interaction (if any) between that handling and the changes here.
There was a problem hiding this comment.
As discussed, will follow up and get test coverage for a removal that isn't cached and ensure behaviour doesn't change with this PR.
…hannels in particular)
…ct and isn't storing non-leaf channelmap entries)
…ored elsewhere outside of the RevTree)
…hannel info for winning rev)
CBG-3203
_sync.Channelsto build set of activeChannels returned byIsChannelRemovalIntegration Tests
GSI=true,xattrs=truehttps://jenkins.sgwdev.com/job/SyncGateway-Integration/2182/