From a6ec08375fd914108aa8de94bebf647b62cf6846 Mon Sep 17 00:00:00 2001 From: ashmrtn <3891298+ashmrtn@users.noreply.github.com> Date: Thu, 18 Jan 2024 15:50:50 -0800 Subject: [PATCH] Fix in-place contacts restore when restore destination is empty (#5053) Fix bug where we tried to create folders with an empty display name when the user did a restore with `/` or `""` as the restore destination for contacts This PR is safe with the current flow for backing up contact folders because nested folders aren't backed up at all (at least nesting beyond the first level). It is unclear, though possible, that this patch will continue to work if we backup nested contact folders Also adds basic smoke tests for this issue for all exchange data types --- #### Does this PR need a docs update or release note? - [x] :white_check_mark: Yes, it's included - [ ] :clock1: Yes, but in a later PR - [ ] :no_entry: No #### Type of change - [ ] :sunflower: Feature - [x] :bug: Bugfix - [ ] :world_map: Documentation - [ ] :robot: Supportability/Tests - [ ] :computer: CI/Deployment - [ ] :broom: Tech Debt/Cleanup #### Test Plan - [x] :muscle: Manual - [x] :zap: Unit test - [ ] :green_heart: E2E --- CHANGELOG.md | 3 + .../collection/exchange/contacts_restore.go | 23 ++- .../collection/exchange/events_restore.go | 12 +- .../m365/collection/exchange/handlers.go | 4 + .../m365/collection/exchange/mail_restore.go | 12 +- src/internal/m365/restore_test.go | 176 ++++++++++++++++++ src/internal/m365/service/exchange/restore.go | 47 +++-- website/docs/support/known-issues.md | 6 + 8 files changed, 267 insertions(+), 16 deletions(-) create mode 100644 src/internal/m365/restore_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 9687ee84d..0babdb791 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Retry transient 400 "invalidRequest" errors during onedrive & sharepoint backup. - Backup attachments associated with group mailbox items. - Groups and Teams backups no longer fail when a resource has no display name. +- Contacts in-place restore failed if the restore destination was empty. ### Changed - When running `backup details` on an empty backup returns a more helpful error message. @@ -21,6 +22,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Known issues - Backing up a group mailbox item may fail if it has a very large number of attachments (500+). - Event description for exchange exports might look slightly different for certain events. +- Exchange in-place restore may restore items in well-known folders to different folders if the user has well-known folder names change based on locale and has updated the locale since the backup was created. +- In-place Exchange contacts restore will merge items in folders named "Contacts" or "contacts" into the default folder. ## [v0.18.0] (beta) - 2024-01-02 diff --git a/src/internal/m365/collection/exchange/contacts_restore.go b/src/internal/m365/collection/exchange/contacts_restore.go index 40fccb037..52c5225b9 100644 --- a/src/internal/m365/collection/exchange/contacts_restore.go +++ b/src/internal/m365/collection/exchange/contacts_restore.go @@ -19,7 +19,10 @@ import ( "github.com/alcionai/corso/src/pkg/services/m365/api/graph" ) -var _ itemRestorer = &contactRestoreHandler{} +var ( + _ itemRestorer = &contactRestoreHandler{} + _ restoreHandler = &contactRestoreHandler{} +) type contactRestoreHandler struct { ac api.Contacts @@ -41,11 +44,25 @@ func (h contactRestoreHandler) NewContainerCache(userID string) graph.ContainerR } } +func (h contactRestoreHandler) ShouldSetContainerToDefaultRoot( + restoreFolderPath string, + collectionPath path.Path, +) bool { + return len(collectionPath.Folders()) == 1 && + restoreFolderPath == h.DefaultRootContainer() +} + func (h contactRestoreHandler) FormatRestoreDestination( destinationContainerName string, - _ path.Path, // contact folders cannot be nested + collectionFullPath path.Path, // contact folders cannot be nested ) *path.Builder { - return path.Builder{}.Append(destinationContainerName) + // User passed in some location to restore to, use that. + if len(destinationContainerName) > 0 { + return path.Builder{}.Append(destinationContainerName) + } + + // FIXME(ashmrtn): Make sure this plays ok with nested folder creation. + return path.Builder{}.Append(collectionFullPath.Folders()...) } func (h contactRestoreHandler) CreateContainer( diff --git a/src/internal/m365/collection/exchange/events_restore.go b/src/internal/m365/collection/exchange/events_restore.go index 01330d380..9aeada8bb 100644 --- a/src/internal/m365/collection/exchange/events_restore.go +++ b/src/internal/m365/collection/exchange/events_restore.go @@ -20,7 +20,10 @@ import ( "github.com/alcionai/corso/src/pkg/services/m365/api/graph" ) -var _ itemRestorer = &eventRestoreHandler{} +var ( + _ itemRestorer = &eventRestoreHandler{} + _ restoreHandler = &eventRestoreHandler{} +) type eventRestoreHandler struct { ac api.Events @@ -42,6 +45,13 @@ func (h eventRestoreHandler) NewContainerCache(userID string) graph.ContainerRes } } +func (h eventRestoreHandler) ShouldSetContainerToDefaultRoot( + restoreFolderPath string, + collectionPath path.Path, +) bool { + return false +} + func (h eventRestoreHandler) FormatRestoreDestination( destinationContainerName string, _ path.Path, // ignored because calendars cannot be nested diff --git a/src/internal/m365/collection/exchange/handlers.go b/src/internal/m365/collection/exchange/handlers.go index e11cedb73..2da773a3d 100644 --- a/src/internal/m365/collection/exchange/handlers.go +++ b/src/internal/m365/collection/exchange/handlers.go @@ -66,6 +66,10 @@ type restoreHandler interface { containerAPI getItemsByCollisionKeyser NewContainerCache(userID string) graph.ContainerResolver + ShouldSetContainerToDefaultRoot( + restoreFolderPath string, + collectionPath path.Path, + ) bool FormatRestoreDestination( destinationContainerName string, collectionFullPath path.Path, diff --git a/src/internal/m365/collection/exchange/mail_restore.go b/src/internal/m365/collection/exchange/mail_restore.go index a50b733ad..8bb6309da 100644 --- a/src/internal/m365/collection/exchange/mail_restore.go +++ b/src/internal/m365/collection/exchange/mail_restore.go @@ -20,7 +20,10 @@ import ( "github.com/alcionai/corso/src/pkg/services/m365/api/graph" ) -var _ itemRestorer = &mailRestoreHandler{} +var ( + _ itemRestorer = &mailRestoreHandler{} + _ restoreHandler = &mailRestoreHandler{} +) type mailRestoreHandler struct { ac api.Mail @@ -42,6 +45,13 @@ func (h mailRestoreHandler) NewContainerCache(userID string) graph.ContainerReso } } +func (h mailRestoreHandler) ShouldSetContainerToDefaultRoot( + restoreFolderPath string, + collectionPath path.Path, +) bool { + return false +} + func (h mailRestoreHandler) FormatRestoreDestination( destinationContainerName string, collectionFullPath path.Path, diff --git a/src/internal/m365/restore_test.go b/src/internal/m365/restore_test.go new file mode 100644 index 000000000..8ed749a72 --- /dev/null +++ b/src/internal/m365/restore_test.go @@ -0,0 +1,176 @@ +package m365 + +import ( + "testing" + + "github.com/alcionai/clues" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/stretchr/testify/suite" + + "github.com/alcionai/corso/src/internal/data" + exchMock "github.com/alcionai/corso/src/internal/m365/service/exchange/mock" + "github.com/alcionai/corso/src/internal/operations/inject" + "github.com/alcionai/corso/src/internal/tester" + "github.com/alcionai/corso/src/internal/tester/tconfig" + "github.com/alcionai/corso/src/internal/version" + "github.com/alcionai/corso/src/pkg/control" + controlTD "github.com/alcionai/corso/src/pkg/control/testdata" + "github.com/alcionai/corso/src/pkg/count" + "github.com/alcionai/corso/src/pkg/fault" + "github.com/alcionai/corso/src/pkg/path" + "github.com/alcionai/corso/src/pkg/selectors" + "github.com/alcionai/corso/src/pkg/services/m365/api" +) + +type RestoreIntgSuite struct { + tester.Suite +} + +func TestRestoreIntgSuite(t *testing.T) { + suite.Run(t, &RestoreIntgSuite{ + Suite: tester.NewIntegrationSuite( + t, + [][]string{tconfig.M365AcctCredEnvs}), + }) +} + +// TestRestoreCollections_HandlesEmptyRestoreLocation checks to make sure that +// even if the restore location is empty we fallback to using the collection +// path as the folder, resulting in an in-place restore. It doesn't attempt to +// retore any items because that would bloat the data set in the test user. +func (suite *RestoreIntgSuite) TestRestoreCollections_HandlesEmptyRestoreLocation() { + acct := tconfig.NewM365Account(suite.T()) + + table := []struct { + service path.ServiceType + category path.CategoryType + selector func(*testing.T) selectors.Selector + defaultPathFolders func() []string + secondaryPathFolders func(location string) []string + }{ + { + service: path.ExchangeService, + category: path.EmailCategory, + selector: func(t *testing.T) selectors.Selector { + sel := selectors.NewExchangeRestore([]string{tconfig.M365UserID(t)}) + sel.Include(sel.Mails(selectors.Any(), selectors.Any())) + + return sel.Selector + }, + defaultPathFolders: func() []string { + return []string{api.MailInbox} + }, + secondaryPathFolders: func(location string) []string { + return []string{location} + }, + }, + { + service: path.ExchangeService, + category: path.EventsCategory, + selector: func(t *testing.T) selectors.Selector { + sel := selectors.NewExchangeRestore([]string{tconfig.M365UserID(t)}) + sel.Include(sel.Events(selectors.Any(), selectors.Any())) + + return sel.Selector + }, + defaultPathFolders: func() []string { + return []string{api.DefaultCalendar} + }, + secondaryPathFolders: func(location string) []string { + return []string{location} + }, + }, + { + service: path.ExchangeService, + category: path.ContactsCategory, + selector: func(t *testing.T) selectors.Selector { + sel := selectors.NewExchangeRestore([]string{tconfig.M365UserID(t)}) + sel.Include(sel.Contacts(selectors.Any(), selectors.Any())) + + return sel.Selector + }, + defaultPathFolders: func() []string { + return []string{api.DefaultContacts} + }, + secondaryPathFolders: func(location string) []string { + return []string{location} + }, + }, + } + + for _, test := range table { + suite.Run(test.service.HumanString()+test.category.HumanString(), func() { + t := suite.T() + + ctx, flush := tester.NewContext(t) + defer flush() + + controller, err := NewController( + ctx, + acct, + test.service, + control.DefaultOptions(), + count.New()) + require.NoError(t, err, clues.ToCore(err)) + + handler, err := controller.NewServiceHandler(test.service) + require.NoError(t, err, clues.ToCore(err)) + + restoreConfig := controlTD.DefaultRestoreConfig("restore_in_place") + restoreConfig.OnCollision = control.Copy + + // Create 2 empty collections so we don't bloat the data set. + path1, err := path.Build( + tconfig.M365TenantID(t), + tconfig.M365UserID(t), + test.service, + test.category, + false, + test.defaultPathFolders()...) + require.NoError(t, err, clues.ToCore(err)) + + path2, err := path.Build( + tconfig.M365TenantID(t), + tconfig.M365UserID(t), + test.service, + test.category, + false, + test.secondaryPathFolders(restoreConfig.Location)...) + require.NoError(t, err, clues.ToCore(err)) + + cols := []data.RestoreCollection{ + data.NoFetchRestoreCollection{ + Collection: exchMock.NewCollection( + path1, + path1, + 0), + }, + data.NoFetchRestoreCollection{ + Collection: exchMock.NewCollection( + path2, + path2, + 0), + }, + } + + restoreConfig.Location = "" + + sel := test.selector(t) + + _, _, err = handler.ConsumeRestoreCollections( + ctx, + inject.RestoreConsumerConfig{ + BackupVersion: version.Backup, + Options: control.DefaultOptions(), + ProtectedResource: sel, + RestoreConfig: restoreConfig, + Selector: sel, + }, + cols, + fault.New(true), + count.New()) + assert.NoError(t, err, clues.ToCore(err)) + }) + } +} diff --git a/src/internal/m365/service/exchange/restore.go b/src/internal/m365/service/exchange/restore.go index b42619b20..3a72aebea 100644 --- a/src/internal/m365/service/exchange/restore.go +++ b/src/internal/m365/service/exchange/restore.go @@ -12,6 +12,7 @@ import ( "github.com/alcionai/corso/src/pkg/backup/details" "github.com/alcionai/corso/src/pkg/count" "github.com/alcionai/corso/src/pkg/fault" + "github.com/alcionai/corso/src/pkg/logger" "github.com/alcionai/corso/src/pkg/path" "github.com/alcionai/corso/src/pkg/services/m365/api/graph" ) @@ -77,19 +78,43 @@ func (h *exchangeHandler) ConsumeRestoreCollections( directoryCache[category] = gcr } - containerID, gcc, err := exchange.CreateDestination( - ictx, - handler, - handler.FormatRestoreDestination(rcc.RestoreConfig.Location, dc.FullPath()), - resourceID, - directoryCache[category], - errs) - if err != nil { - el.AddRecoverable(ictx, err) - continue + restoreFolderPath := handler.FormatRestoreDestination(rcc.RestoreConfig.Location, dc.FullPath()) + + ictx = clues.Add(ictx, "restore_folder_path", restoreFolderPath) + + var containerID string + + // Only attempt to create a new folder if it's not the default contacts + // folder. Contacts is weird in that it allows creating sub folders with the + // same name as the default contacts folder, but which are technically + // nested folders. + // + // Without this check we'll end up restoring to Contacts/Contacts instead of + // Contacts if in-place restore is requested and the root Contacts folder + // had items. + if handler.ShouldSetContainerToDefaultRoot(restoreFolderPath.String(), dc.FullPath()) { + logger.Ctx(ictx).Info("using default contact folder") + + containerID = handler.DefaultRootContainer() + } else { + logger.Ctx(ictx).Info("creating restore folder") + + newContainerID, gcc, err := exchange.CreateDestination( + ictx, + handler, + restoreFolderPath, + resourceID, + directoryCache[category], + errs) + if err != nil { + el.AddRecoverable(ictx, err) + continue + } + + directoryCache[category] = gcc + containerID = newContainerID } - directoryCache[category] = gcc ictx = clues.Add(ictx, "restore_destination_id", containerID) collisionKeyToItemID, err := handler.GetItemsInContainerByCollisionKey(ictx, resourceID, containerID) diff --git a/website/docs/support/known-issues.md b/website/docs/support/known-issues.md index 52ac0ccd3..76ad7dc50 100644 --- a/website/docs/support/known-issues.md +++ b/website/docs/support/known-issues.md @@ -37,3 +37,9 @@ Below is a list of known Corso issues and limitations: * Restoring the data into a different Group from the one it was backed up from isn't currently supported. * Backing up a group mailbox item may fail if it has a large number of attachments (500+). + +* Exchange in-place restore may restore items in well-known folders to different + folders if the user has well-known folder names change based on locale and has + updated the locale since the backup was created. + +* In-place Exchange contacts restore will merge items in folders named "Contacts" or "contacts" into the default folder.