From 53a0525bfdd704815e5963a1d95128675a295f73 Mon Sep 17 00:00:00 2001 From: Abhishek Pandey Date: Mon, 5 Feb 2024 15:10:09 -0800 Subject: [PATCH] Handle previous paths for group mailbox (#5154) * Tombstone collections weren't being added because we were not processing prev paths for group mailbox category. * Hence deleted conversations were being carried forward even if `--disable-incrementals` was used. --- #### Does this PR need a docs update or release note? - [ ] :white_check_mark: Yes, it's included - [ ] :clock1: Yes, but in a later PR - [x] :no_entry: No #### Type of change - [ ] :sunflower: Feature - [x] :bug: Bugfix - [ ] :world_map: Documentation - [ ] :robot: Supportability/Tests - [ ] :computer: CI/Deployment - [ ] :broom: Tech Debt/Cleanup #### Issue(s) * # #### Test Plan - [x] :muscle: Manual - [x] :zap: Unit test - [ ] :green_heart: E2E --- .../m365/collection/groups/backup_test.go | 521 +++++++++++++++++- .../m365/collection/groups/metadata.go | 9 +- .../groups/testdata/conversations.go | 52 ++ 3 files changed, 551 insertions(+), 31 deletions(-) create mode 100644 src/internal/m365/collection/groups/testdata/conversations.go diff --git a/src/internal/m365/collection/groups/backup_test.go b/src/internal/m365/collection/groups/backup_test.go index b52d5644d..760287691 100644 --- a/src/internal/m365/collection/groups/backup_test.go +++ b/src/internal/m365/collection/groups/backup_test.go @@ -39,22 +39,24 @@ import ( // mocks // --------------------------------------------------------------------------- -var _ backupHandler[models.Channelable, models.ChatMessageable] = &mockBackupHandler{} +var _ backupHandler[models.Channelable, models.ChatMessageable] = &mockChannelsBH{} //lint:ignore U1000 false linter issue due to generics -type mockBackupHandler struct { +type mockChannelsBH struct { channels []models.Channelable + conversations []models.Conversationable messageIDs []string deletedMsgIDs []string messagesErr error messages map[string]models.ChatMessageable + posts map[string]models.Postable info map[string]*details.GroupsInfo getMessageErr map[string]error doNotInclude bool } //lint:ignore U1000 false linter issue due to generics -func (bh mockBackupHandler) augmentItemInfo( +func (bh mockChannelsBH) augmentItemInfo( *details.GroupsInfo, models.Channelable, ) { @@ -62,15 +64,15 @@ func (bh mockBackupHandler) augmentItemInfo( } //lint:ignore U1000 false linter issue due to generics -func (bh mockBackupHandler) supportsItemMetadata() bool { +func (bh mockChannelsBH) supportsItemMetadata() bool { return false } -func (bh mockBackupHandler) canMakeDeltaQueries() bool { +func (bh mockChannelsBH) canMakeDeltaQueries() bool { return true } -func (bh mockBackupHandler) containers() []container[models.Channelable] { +func (bh mockChannelsBH) containers() []container[models.Channelable] { containers := make([]container[models.Channelable], 0, len(bh.channels)) for _, ch := range bh.channels { @@ -81,14 +83,14 @@ func (bh mockBackupHandler) containers() []container[models.Channelable] { } //lint:ignore U1000 required for interface compliance -func (bh mockBackupHandler) getContainers( +func (bh mockChannelsBH) getContainers( context.Context, api.CallConfig, ) ([]container[models.Channelable], error) { return bh.containers(), nil } -func (bh mockBackupHandler) getContainerItemIDs( +func (bh mockChannelsBH) getContainerItemIDs( _ context.Context, _ path.Elements, _ string, @@ -111,14 +113,14 @@ func (bh mockBackupHandler) getContainerItemIDs( } //lint:ignore U1000 required for interface compliance -func (bh mockBackupHandler) includeContainer( +func (bh mockChannelsBH) includeContainer( models.Channelable, selectors.GroupsScope, ) bool { return !bh.doNotInclude } -func (bh mockBackupHandler) canonicalPath( +func (bh mockChannelsBH) canonicalPath( storageDirFolders path.Elements, tenantID string, ) (path.Path, error) { @@ -133,7 +135,7 @@ func (bh mockBackupHandler) canonicalPath( } //lint:ignore U1000 false linter issue due to generics -func (bh mockBackupHandler) getItem( +func (bh mockChannelsBH) getItem( _ context.Context, _ string, _ path.Elements, @@ -142,7 +144,7 @@ func (bh mockBackupHandler) getItem( return bh.messages[itemID], bh.info[itemID], bh.getMessageErr[itemID] } -func (bh mockBackupHandler) getItemMetadata( +func (bh mockChannelsBH) getItemMetadata( _ context.Context, _ models.Channelable, ) (io.ReadCloser, int, error) { @@ -150,7 +152,7 @@ func (bh mockBackupHandler) getItemMetadata( } //lint:ignore U1000 false linter issue due to generics -func (bh mockBackupHandler) makeTombstones( +func (bh mockChannelsBH) makeTombstones( dps metadata.DeltaPaths, ) (map[string]string, error) { return makeTombstones(dps), nil @@ -176,6 +178,10 @@ func (suite *BackupUnitSuite) SetupSuite() { suite.creds = m365 } +// --------------------------------------------------------------------------- +// Channels tests +// --------------------------------------------------------------------------- + func (suite *BackupUnitSuite) TestPopulateCollections() { var ( qp = graph.QueryParams{ @@ -188,7 +194,7 @@ func (suite *BackupUnitSuite) TestPopulateCollections() { table := []struct { name string - mock mockBackupHandler + mock mockChannelsBH expectErr require.ErrorAssertionFunc expectColls int expectNewColls int @@ -196,7 +202,7 @@ func (suite *BackupUnitSuite) TestPopulateCollections() { }{ { name: "happy path, one container", - mock: mockBackupHandler{ + mock: mockChannelsBH{ channels: testdata.StubChannels("one"), messageIDs: []string{"msg-one"}, }, @@ -207,7 +213,7 @@ func (suite *BackupUnitSuite) TestPopulateCollections() { }, { name: "happy path, one container, only deleted messages", - mock: mockBackupHandler{ + mock: mockChannelsBH{ channels: testdata.StubChannels("one"), deletedMsgIDs: []string{"msg-one"}, }, @@ -218,7 +224,7 @@ func (suite *BackupUnitSuite) TestPopulateCollections() { }, { name: "happy path, many containers", - mock: mockBackupHandler{ + mock: mockChannelsBH{ channels: testdata.StubChannels("one", "two"), messageIDs: []string{"msg-one"}, }, @@ -229,7 +235,7 @@ func (suite *BackupUnitSuite) TestPopulateCollections() { }, { name: "no containers pass scope", - mock: mockBackupHandler{ + mock: mockChannelsBH{ channels: testdata.StubChannels("one"), doNotInclude: true, }, @@ -240,7 +246,7 @@ func (suite *BackupUnitSuite) TestPopulateCollections() { }, { name: "no channels", - mock: mockBackupHandler{}, + mock: mockChannelsBH{}, expectErr: require.NoError, expectColls: 1, expectNewColls: 0, @@ -248,7 +254,7 @@ func (suite *BackupUnitSuite) TestPopulateCollections() { }, { name: "no channel messages", - mock: mockBackupHandler{ + mock: mockChannelsBH{ channels: testdata.StubChannels("one"), }, expectErr: require.NoError, @@ -258,7 +264,7 @@ func (suite *BackupUnitSuite) TestPopulateCollections() { }, { name: "err: deleted in flight", - mock: mockBackupHandler{ + mock: mockChannelsBH{ channels: testdata.StubChannels("one"), messagesErr: core.ErrNotFound, }, @@ -269,7 +275,7 @@ func (suite *BackupUnitSuite) TestPopulateCollections() { }, { name: "err: other error", - mock: mockBackupHandler{ + mock: mockChannelsBH{ channels: testdata.StubChannels("one"), messagesErr: assert.AnError, }, @@ -348,7 +354,7 @@ func (suite *BackupUnitSuite) TestPopulateCollections_incremental() { table := []struct { name string - mock mockBackupHandler + mock mockChannelsBH deltaPaths metadata.DeltaPaths expectErr require.ErrorAssertionFunc expectColls int @@ -358,7 +364,7 @@ func (suite *BackupUnitSuite) TestPopulateCollections_incremental() { }{ { name: "non incremental", - mock: mockBackupHandler{ + mock: mockChannelsBH{ channels: testdata.StubChannels("chan"), messageIDs: []string{"msg"}, }, @@ -371,7 +377,7 @@ func (suite *BackupUnitSuite) TestPopulateCollections_incremental() { }, { name: "incremental", - mock: mockBackupHandler{ + mock: mockChannelsBH{ channels: testdata.StubChannels("chan"), deletedMsgIDs: []string{"msg"}, }, @@ -389,7 +395,7 @@ func (suite *BackupUnitSuite) TestPopulateCollections_incremental() { }, { name: "incremental no new messages", - mock: mockBackupHandler{ + mock: mockChannelsBH{ channels: testdata.StubChannels("chan"), }, deltaPaths: metadata.DeltaPaths{ @@ -406,7 +412,7 @@ func (suite *BackupUnitSuite) TestPopulateCollections_incremental() { }, { name: "incremental deleted channel", - mock: mockBackupHandler{ + mock: mockChannelsBH{ channels: testdata.StubChannels(), }, deltaPaths: metadata.DeltaPaths{ @@ -423,7 +429,7 @@ func (suite *BackupUnitSuite) TestPopulateCollections_incremental() { }, { name: "incremental new and deleted channel", - mock: mockBackupHandler{ + mock: mockChannelsBH{ channels: testdata.StubChannels("chan2"), messageIDs: []string{"msg"}, }, @@ -493,6 +499,465 @@ func (suite *BackupUnitSuite) TestPopulateCollections_incremental() { } } +// --------------------------------------------------------------------------- +// Conversations tests +// --------------------------------------------------------------------------- + +var _ backupHandler[models.Conversationable, models.Postable] = &mockConversationsBH{} + +//lint:ignore U1000 false linter issue due to generics +type mockConversationsBH struct { + conversations []models.Conversationable + // Assume all conversations have the same thread object under them for simplicty. + // It doesn't impact the tests. + thread models.ConversationThreadable + postIDs []string + deletedPostIDs []string + PostsErr error + Posts map[string]models.Postable + info map[string]*details.GroupsInfo + getPostErr map[string]error + doNotInclude bool +} + +//lint:ignore U1000 false linter issue due to generics +func (bh mockConversationsBH) augmentItemInfo( + *details.GroupsInfo, + models.Conversationable, +) { + // no-op +} + +func (bh mockConversationsBH) canMakeDeltaQueries() bool { + return false +} + +func (bh mockConversationsBH) containers() []container[models.Conversationable] { + containers := make([]container[models.Conversationable], 0, len(bh.conversations)) + + for _, ch := range bh.conversations { + containers = append(containers, conversationThreadContainer(ch, bh.thread)) + } + + return containers +} + +//lint:ignore U1000 required for interface compliance +func (bh mockConversationsBH) getContainers( + context.Context, + api.CallConfig, +) ([]container[models.Conversationable], error) { + return bh.containers(), nil +} + +func (bh mockConversationsBH) getContainerItemIDs( + _ context.Context, + _ path.Elements, + _ string, + _ api.CallConfig, +) (pagers.AddedAndRemoved, error) { + idRes := make(map[string]time.Time, len(bh.postIDs)) + + for _, id := range bh.postIDs { + idRes[id] = time.Time{} + } + + aar := pagers.AddedAndRemoved{ + Added: idRes, + Removed: bh.deletedPostIDs, + ValidModTimes: true, + DU: pagers.DeltaUpdate{}, + } + + return aar, bh.PostsErr +} + +//lint:ignore U1000 required for interface compliance +func (bh mockConversationsBH) includeContainer( + models.Conversationable, + selectors.GroupsScope, +) bool { + return !bh.doNotInclude +} + +func (bh mockConversationsBH) canonicalPath( + storageDirFolders path.Elements, + tenantID string, +) (path.Path, error) { + return storageDirFolders. + Builder(). + ToDataLayerPath( + tenantID, + "protectedResource", + path.GroupsService, + path.ConversationPostsCategory, + false) +} + +//lint:ignore U1000 false linter issue due to generics +func (bh mockConversationsBH) getItem( + _ context.Context, + _ string, + _ path.Elements, + itemID string, +) (models.Postable, *details.GroupsInfo, error) { + return bh.Posts[itemID], bh.info[itemID], bh.getPostErr[itemID] +} + +//lint:ignore U1000 false linter issue due to generics +func (bh mockConversationsBH) supportsItemMetadata() bool { + return true +} + +func (bh mockConversationsBH) getItemMetadata( + _ context.Context, + _ models.Conversationable, +) (io.ReadCloser, int, error) { + return nil, 0, nil +} + +//lint:ignore U1000 false linter issue due to generics +func (bh mockConversationsBH) makeTombstones( + dps metadata.DeltaPaths, +) (map[string]string, error) { + r := make(map[string]string, len(dps)) + + for id, v := range dps { + elems := path.Split(id) + if len(elems) != 2 { + return nil, clues.New("invalid prev path") + } + + r[elems[0]] = v.Path + } + + return r, nil +} + +func (suite *BackupUnitSuite) TestPopulateCollections_Conversations() { + var ( + qp = graph.QueryParams{ + Category: path.ConversationPostsCategory, // doesn't matter which one we use. + ProtectedResource: inMock.NewProvider("group_id", "user_name"), + TenantID: suite.creds.AzureTenantID, + } + statusUpdater = func(*support.ControllerOperationStatus) {} + ) + + table := []struct { + name string + mock mockConversationsBH + expectErr require.ErrorAssertionFunc + expectColls int + expectNewColls int + expectMetadataColls int + }{ + { + name: "happy path, one container", + mock: mockConversationsBH{ + conversations: testdata.StubConversations("one"), + thread: testdata.StubConversationThreads("t-one")[0], + postIDs: []string{"msg-one"}, + }, + expectErr: require.NoError, + expectColls: 2, + expectNewColls: 1, + expectMetadataColls: 1, + }, + { + name: "happy path, one container, only deleted messages", + mock: mockConversationsBH{ + conversations: testdata.StubConversations("one"), + thread: testdata.StubConversationThreads("t-one")[0], + deletedPostIDs: []string{"msg-one"}, + }, + expectErr: require.NoError, + expectColls: 2, + expectNewColls: 1, + expectMetadataColls: 1, + }, + { + name: "happy path, many containers", + mock: mockConversationsBH{ + conversations: testdata.StubConversations("one", "two"), + thread: testdata.StubConversationThreads("t-one")[0], + postIDs: []string{"msg-one"}, + }, + expectErr: require.NoError, + expectColls: 3, + expectNewColls: 2, + expectMetadataColls: 1, + }, + { + name: "no containers pass scope", + mock: mockConversationsBH{ + conversations: testdata.StubConversations("one"), + thread: testdata.StubConversationThreads("t-one")[0], + doNotInclude: true, + }, + expectErr: require.NoError, + expectColls: 1, + expectNewColls: 0, + expectMetadataColls: 1, + }, + { + name: "no conversations", + mock: mockConversationsBH{}, + expectErr: require.NoError, + expectColls: 1, + expectNewColls: 0, + expectMetadataColls: 1, + }, + { + name: "no conv posts", + mock: mockConversationsBH{ + conversations: testdata.StubConversations("one"), + thread: testdata.StubConversationThreads("t-one")[0], + }, + expectErr: require.NoError, + expectColls: 2, + expectNewColls: 1, + expectMetadataColls: 1, + }, + { + name: "err: deleted in flight", + mock: mockConversationsBH{ + conversations: testdata.StubConversations("one"), + thread: testdata.StubConversationThreads("t-one")[0], + PostsErr: core.ErrNotFound, + }, + expectErr: require.Error, + expectColls: 1, + expectNewColls: 0, + expectMetadataColls: 1, + }, + { + name: "err: other error", + mock: mockConversationsBH{ + conversations: testdata.StubConversations("one"), + thread: testdata.StubConversationThreads("t-one")[0], + PostsErr: assert.AnError, + }, + expectErr: require.Error, + expectColls: 1, + expectNewColls: 0, + expectMetadataColls: 1, + }, + } + for _, test := range table { + suite.Run(test.name, func() { + t := suite.T() + + ctx, flush := tester.NewContext(t) + defer flush() + + ctrlOpts := control.Options{FailureHandling: control.FailFast} + + collections, err := populateCollections( + ctx, + qp, + test.mock, + statusUpdater, + test.mock.containers(), + selectors.NewGroupsBackup(nil).Channels(selectors.Any())[0], + nil, + false, + ctrlOpts, + count.New(), + fault.New(true)) + test.expectErr(t, err, clues.ToCore(err)) + assert.Len(t, collections, test.expectColls, "number of collections") + + // collection assertions + + deleteds, news, metadatas, doNotMerges := 0, 0, 0, 0 + for _, c := range collections { + if c.FullPath().Service() == path.GroupsMetadataService { + metadatas++ + continue + } + + if c.State() == data.DeletedState { + deleteds++ + } + + if c.State() == data.NewState { + news++ + } + + if c.DoNotMergeItems() { + doNotMerges++ + } + } + + assert.Zero(t, deleteds, "deleted collections") + assert.Equal(t, test.expectNewColls, news, "new collections") + assert.Equal(t, test.expectMetadataColls, metadatas, "metadata collections") + }) + } +} + +func (suite *BackupUnitSuite) TestPopulateCollections_ConversationsIncremental() { + var ( + qp = graph.QueryParams{ + Category: path.ConversationPostsCategory, // doesn't matter which one we use. + ProtectedResource: inMock.NewProvider("group_id", "user_name"), + TenantID: suite.creds.AzureTenantID, + } + statusUpdater = func(*support.ControllerOperationStatus) {} + allScope = selectors.NewGroupsBackup(nil).Conversation(selectors.Any())[0] + ) + + convPath, err := path.Build("t", "g", path.GroupsService, path.ConversationPostsCategory, false, "conv0", "thread0") + require.NoError(suite.T(), err, clues.ToCore(err)) + + table := []struct { + name string + mock mockConversationsBH + deltaPaths metadata.DeltaPaths + expectErr require.ErrorAssertionFunc + expectColls int + expectNewColls int + expectTombstoneCols int + expectMetadataColls int + }{ + { + name: "non incremental", + mock: mockConversationsBH{ + conversations: testdata.StubConversations("conv0"), + thread: testdata.StubConversationThreads("t0")[0], + postIDs: []string{"msg"}, + }, + deltaPaths: metadata.DeltaPaths{}, + expectErr: require.NoError, + expectColls: 2, + expectNewColls: 1, + expectTombstoneCols: 0, + expectMetadataColls: 1, + }, + { + name: "incremental", + mock: mockConversationsBH{ + conversations: testdata.StubConversations("conv0"), + thread: testdata.StubConversationThreads("t0")[0], + postIDs: []string{"msg"}, + }, + deltaPaths: metadata.DeltaPaths{ + "conv0/thread0": { + Path: convPath.String(), + }, + }, + expectErr: require.NoError, + expectColls: 2, + expectNewColls: 1, // No delta support + expectTombstoneCols: 0, + expectMetadataColls: 1, + }, + { + name: "incremental no new posts", + mock: mockConversationsBH{ + conversations: testdata.StubConversations("conv0"), + thread: testdata.StubConversationThreads("t0")[0], + }, + deltaPaths: metadata.DeltaPaths{ + "conv0/thread0": { + Path: convPath.String(), + }, + }, + expectErr: require.NoError, + expectColls: 2, + expectNewColls: 1, // No delta support + expectTombstoneCols: 0, + expectMetadataColls: 1, + }, + { + name: "incremental deleted conversation", + mock: mockConversationsBH{ + conversations: testdata.StubConversations(), + }, + deltaPaths: metadata.DeltaPaths{ + "conv0/thread0": { + Path: convPath.String(), + }, + }, + expectErr: require.NoError, + expectColls: 2, + expectNewColls: 0, + expectTombstoneCols: 1, + expectMetadataColls: 1, + }, + { + name: "incremental new and deleted conversations", + mock: mockConversationsBH{ + conversations: testdata.StubConversations("conv1"), + thread: testdata.StubConversationThreads("t1")[0], + postIDs: []string{"msg"}, + }, + deltaPaths: metadata.DeltaPaths{ + "conv0/thread0": { + Path: convPath.String(), + }, + }, + expectErr: require.NoError, + expectColls: 3, + expectNewColls: 1, + expectTombstoneCols: 1, + expectMetadataColls: 1, + }, + } + for _, test := range table { + suite.Run(test.name, func() { + t := suite.T() + + ctx, flush := tester.NewContext(t) + defer flush() + + ctrlOpts := control.Options{FailureHandling: control.FailFast} + + collections, err := populateCollections( + ctx, + qp, + test.mock, + statusUpdater, + test.mock.containers(), + allScope, + test.deltaPaths, + false, + ctrlOpts, + count.New(), + fault.New(true)) + test.expectErr(t, err, clues.ToCore(err)) + assert.Len(t, collections, test.expectColls, "number of collections") + + // collection assertions + + tombstones, news, metadatas, doNotMerges := 0, 0, 0, 0 + for _, c := range collections { + if c.FullPath() != nil && c.FullPath().Service() == path.GroupsMetadataService { + metadatas++ + continue + } + + if c.State() == data.DeletedState { + tombstones++ + } + + if c.State() == data.NewState { + news++ + } + + if c.DoNotMergeItems() { + doNotMerges++ + } + } + + assert.Equal(t, test.expectNewColls, news, "new collections") + assert.Equal(t, test.expectTombstoneCols, tombstones, "tombstone collections") + assert.Equal(t, test.expectMetadataColls, metadatas, "metadata collections") + }) + } +} + // --------------------------------------------------------------------------- // Integration tests // --------------------------------------------------------------------------- diff --git a/src/internal/m365/collection/groups/metadata.go b/src/internal/m365/collection/groups/metadata.go index 4f123945f..d328216ca 100644 --- a/src/internal/m365/collection/groups/metadata.go +++ b/src/internal/m365/collection/groups/metadata.go @@ -21,13 +21,15 @@ func parseMetadataCollections( ) (metadata.CatDeltaPaths, bool, error) { // cdp stores metadata cdp := metadata.CatDeltaPaths{ - path.ChannelMessagesCategory: {}, + path.ChannelMessagesCategory: {}, + path.ConversationPostsCategory: {}, } // found tracks the metadata we've loaded, to make sure we don't // fetch overlapping copies. found := map[path.CategoryType]map[string]struct{}{ - path.ChannelMessagesCategory: {}, + path.ChannelMessagesCategory: {}, + path.ConversationPostsCategory: {}, } // errors from metadata items should not stop the backup, @@ -104,7 +106,8 @@ func parseMetadataCollections( logger.CtxErr(ctx, errs.Failure()).Info("reading metadata collection items") return metadata.CatDeltaPaths{ - path.ChannelMessagesCategory: {}, + path.ChannelMessagesCategory: {}, + path.ConversationPostsCategory: {}, }, false, nil } diff --git a/src/internal/m365/collection/groups/testdata/conversations.go b/src/internal/m365/collection/groups/testdata/conversations.go new file mode 100644 index 000000000..d49fb90ef --- /dev/null +++ b/src/internal/m365/collection/groups/testdata/conversations.go @@ -0,0 +1,52 @@ +package testdata + +import ( + "github.com/google/uuid" + "github.com/microsoftgraph/msgraph-sdk-go/models" + + "github.com/alcionai/corso/src/internal/common/ptr" +) + +func StubConversations(ids ...string) []models.Conversationable { + sl := make([]models.Conversationable, 0, len(ids)) + + for _, id := range ids { + c := models.NewConversation() + c.SetId(ptr.To(id)) + + sl = append(sl, c) + } + + return sl +} + +func StubConversationThreads(ids ...string) []models.ConversationThreadable { + sl := make([]models.ConversationThreadable, 0, len(ids)) + + for _, id := range ids { + ct := models.NewConversationThread() + ct.SetId(ptr.To(id)) + + sl = append(sl, ct) + } + + return sl +} + +func StubPosts(ids ...string) []models.Postable { + sl := make([]models.Postable, 0, len(ids)) + + for _, id := range ids { + p := models.NewPost() + p.SetId(ptr.To(uuid.NewString())) + + body := models.NewItemBody() + body.SetContent(ptr.To(id)) + + p.SetBody(body) + + sl = append(sl, p) + } + + return sl +}