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] ✅ Yes, it's included
- [ ] 🕐 Yes, but in a later PR
- [ ] ⛔ No

#### Type of change

- [ ] 🌻 Feature
- [x] 🐛 Bugfix
- [ ] 🗺️ Documentation
- [ ] 🤖 Supportability/Tests
- [ ] 💻 CI/Deployment
- [ ] 🧹 Tech Debt/Cleanup

#### Test Plan

- [x] 💪 Manual
- [x] ⚡ Unit test
- [ ] 💚 E2E

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
 

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(

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

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,

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,

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))
+		})
+	}
+}

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)

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.