tweaks and fixes found while testing (#3735)

a variety of small updates that came
from manual testing of restore with various
collision and destination combinations.

---

#### Does this PR need a docs update or release note?

- [x] ⛔ No

#### Type of change

- [x] 🐛 Bugfix

#### Issue(s)

* #3562

#### Test Plan

- [x] 💪 Manual

src/cli/restore/restore.go

@@ -84,14 +84,17 @@ func runRestore(
 		return Only(ctx, clues.Wrap(err, "Failed to run "+serviceName+" restore"))
 	}
 
-	Info(ctx, "Completed Restore:")
+	Info(ctx, "Restore Complete")
 
 	skipped := ro.Counter.Get(count.CollisionSkip)
 	if skipped > 0 {
 		Infof(ctx, "Skipped %d items due to collision", skipped)
 	}
 
-	ds.Items().MaybePrintEntries(ctx)
+	dis := ds.Items()
+
+	Outf(ctx, "Restored %d items", len(dis))
+	dis.MaybePrintEntries(ctx)
 
 	return nil
 }

src/internal/m365/exchange/contacts_restore.go

@@ -60,9 +60,8 @@ func (h contactRestoreHandler) GetContainerByName(
 	return h.ac.GetContainerByName(ctx, userID, "", containerName)
 }
 
-// always returns the provided value
+func (h contactRestoreHandler) defaultRootContainer() string {
-func (h contactRestoreHandler) orRootContainer(c string) string {
+	return api.DefaultContacts
-	return c
 }
 
 func (h contactRestoreHandler) restore(
@@ -100,6 +99,14 @@ func restoreContact(
 	errs *fault.Bus,
 	ctr *count.Bus,
 ) (*details.ExchangeInfo, error) {
+	// contacts has a weird relationship with its default
+	// folder, which is that the folder is treated as invisible
+	// in many cases.  If we're restoring to a blank location,
+	// we can interpret that as the root.
+	if len(destinationID) == 0 {
+		destinationID = api.DefaultContacts
+	}
+
 	contact, err := api.BytesToContactable(body)
 	if err != nil {
 		return nil, graph.Wrap(ctx, err, "creating contact from bytes")
@@ -139,7 +146,7 @@ func restoreContact(
 	// at least we'll have accidentally over-produced data instead of deleting
 	// the user's data.
 	if shouldDeleteOriginal {
-		if err := cr.DeleteItem(ctx, userID, collisionID); err != nil {
+		if err := cr.DeleteItem(ctx, userID, collisionID); err != nil && !graph.IsErrDeletedInFlight(err) {
 			return nil, graph.Wrap(ctx, err, "deleting colliding contact")
 		}
 	}

src/internal/m365/exchange/contacts_restore_test.go

@@ -176,6 +176,19 @@ func (suite *ContactsRestoreIntgSuite) TestRestoreContact() {
 				assert.True(t, m.calledDelete, "old item deleted")
 			},
 		},
+		{
+			name:         "collision: replace - err already deleted",
+			apiMock:      &contactRestoreMock{deleteItemErr: graph.ErrDeletedInFlight},
+			collisionMap: map[string]string{collisionKey: "smarf"},
+			onCollision:  control.Replace,
+			expectErr: func(t *testing.T, err error) {
+				assert.NoError(t, err, clues.ToCore(err))
+			},
+			expectMock: func(t *testing.T, m *contactRestoreMock) {
+				assert.True(t, m.calledPost, "new item posted")
+				assert.True(t, m.calledDelete, "old item deleted")
+			},
+		},
 	}
 	for _, test := range table {
 		suite.Run(test.name, func() {

src/internal/m365/exchange/container_resolver_test.go

@@ -806,6 +806,9 @@ func runCreateDestinationTest(
 		gcc = handler.newContainerCache(userID)
 	)
 
+	err := gcc.Populate(ctx, fault.New(true), handler.defaultRootContainer())
+	require.NoError(t, err, clues.ToCore(err))
+
 	path1, err := path.Build(
 		tenantID,
 		userID,
@@ -821,7 +824,6 @@ func runCreateDestinationTest(
 		handler.formatRestoreDestination(destinationName, path1),
 		userID,
 		gcc,
-		true,
 		fault.New(true))
 	require.NoError(t, err, clues.ToCore(err))
 
@@ -843,7 +845,6 @@ func runCreateDestinationTest(
 		handler.formatRestoreDestination(destinationName, path2),
 		userID,
 		gcc,
-		false,
 		fault.New(true))
 	require.NoError(t, err, clues.ToCore(err))
 

src/internal/m365/exchange/events_restore.go

@@ -44,6 +44,10 @@ func (h eventRestoreHandler) formatRestoreDestination(
 	destinationContainerName string,
 	_ path.Path, // ignored because calendars cannot be nested
 ) *path.Builder {
+	if len(destinationContainerName) == 0 {
+		destinationContainerName = api.DefaultCalendar
+	}
+
 	return path.Builder{}.Append(destinationContainerName)
 }
 
@@ -62,8 +66,8 @@ func (h eventRestoreHandler) GetContainerByName(
 }
 
 // always returns the provided value
-func (h eventRestoreHandler) orRootContainer(c string) string {
+func (h eventRestoreHandler) defaultRootContainer() string {
-	return c
+	return api.DefaultCalendar
 }
 
 func (h eventRestoreHandler) restore(
@@ -151,7 +155,7 @@ func restoreEvent(
 	// at least we'll have accidentally over-produced data instead of deleting
 	// the user's data.
 	if shouldDeleteOriginal {
-		if err := er.DeleteItem(ctx, userID, collisionID); err != nil {
+		if err := er.DeleteItem(ctx, userID, collisionID); err != nil && !graph.IsErrDeletedInFlight(err) {
 			return nil, graph.Wrap(ctx, err, "deleting colliding event")
 		}
 	}

src/internal/m365/exchange/events_restore_test.go

@@ -224,6 +224,19 @@ func (suite *EventsRestoreIntgSuite) TestRestoreEvent() {
 				assert.True(t, m.calledDelete, "old item deleted")
 			},
 		},
+		{
+			name:         "collision: replace - err already deleted",
+			apiMock:      &eventRestoreMock{deleteItemErr: graph.ErrDeletedInFlight},
+			collisionMap: map[string]string{collisionKey: "smarf"},
+			onCollision:  control.Replace,
+			expectErr: func(t *testing.T, err error) {
+				assert.NoError(t, err, clues.ToCore(err))
+			},
+			expectMock: func(t *testing.T, m *eventRestoreMock) {
+				assert.True(t, m.calledPost, "new item posted")
+				assert.True(t, m.calledDelete, "old item deleted")
+			},
+		},
 	}
 	for _, test := range table {
 		suite.Run(test.name, func() {

src/internal/m365/exchange/handlers.go

@@ -95,12 +95,7 @@ type containerAPI interface {
 		ctx context.Context,
 		userID, parentContainerID, containerName string,
 	) (graph.Container, error)
-
+	defaultRootContainer() string
-	// returns either the provided value (assumed to be the root
-	// folder for that cache tree), or the default root container
-	// (if the category uses a root folder that exists above the
-	// restore location path).
-	orRootContainer(string) string
 }
 
 type containerByNamer interface {

src/internal/m365/exchange/mail_restore.go

@@ -65,8 +65,7 @@ func (h mailRestoreHandler) GetContainerByName(
 	return h.ac.GetContainerByName(ctx, userID, parentContainerID, containerName)
 }
 
-// always returns rootFolderAlias
+func (h mailRestoreHandler) defaultRootContainer() string {
-func (h mailRestoreHandler) orRootContainer(string) string {
 	return api.MsgFolderRoot
 }
 
@@ -151,7 +150,7 @@ func restoreMail(
 	// at least we'll have accidentally over-produced data instead of deleting
 	// the user's data.
 	if shouldDeleteOriginal {
-		if err := mr.DeleteItem(ctx, userID, collisionID); err != nil {
+		if err := mr.DeleteItem(ctx, userID, collisionID); err != nil && !graph.IsErrDeletedInFlight(err) {
 			return nil, graph.Wrap(ctx, err, "deleting colliding mail message")
 		}
 	}

src/internal/m365/exchange/mail_restore_test.go

@@ -193,6 +193,19 @@ func (suite *MailRestoreIntgSuite) TestRestoreMail() {
 				assert.True(t, m.calledDelete, "old item deleted")
 			},
 		},
+		{
+			name:         "collision: replace - err already deleted",
+			apiMock:      &mailRestoreMock{deleteItemErr: graph.ErrDeletedInFlight},
+			collisionMap: map[string]string{collisionKey: "smarf"},
+			onCollision:  control.Replace,
+			expectErr: func(t *testing.T, err error) {
+				assert.NoError(t, err, clues.ToCore(err))
+			},
+			expectMock: func(t *testing.T, m *mailRestoreMock) {
+				assert.True(t, m.calledPost, "new item posted")
+				assert.True(t, m.calledDelete, "old item deleted")
+			},
+		},
 	}
 	for _, test := range table {
 		suite.Run(test.name, func() {

src/internal/m365/exchange/restore.go

@@ -55,7 +55,6 @@ func ConsumeRestoreCollections(
 		}
 
 		var (
-			isNewCache bool
 			category = dc.FullPath().Category()
 			ictx     = clues.Add(
 				ctx,
@@ -70,8 +69,12 @@ func ConsumeRestoreCollections(
 		}
 
 		if directoryCache[category] == nil {
-			directoryCache[category] = handler.newContainerCache(userID)
+			gcr := handler.newContainerCache(userID)
-			isNewCache = true
+			if err := gcr.Populate(ctx, errs, handler.defaultRootContainer()); err != nil {
+				return nil, clues.Wrap(err, "populating container cache")
+			}
+
+			directoryCache[category] = gcr
 		}
 
 		containerID, gcc, err := createDestination(
@@ -80,7 +83,6 @@ func ConsumeRestoreCollections(
 			handler.formatRestoreDestination(restoreCfg.Location, dc.FullPath()),
 			userID,
 			directoryCache[category],
-			isNewCache,
 			errs)
 		if err != nil {
 			el.AddRecoverable(ctx, err)
@@ -240,7 +242,6 @@ func createDestination(
 	destination *path.Builder,
 	userID string,
 	gcr graph.ContainerResolver,
-	isNewCache bool,
 	errs *fault.Bus,
 ) (string, graph.ContainerResolver, error) {
 	var (
@@ -254,12 +255,11 @@ func createDestination(
 
 		ictx := clues.Add(
 			ctx,
-			"is_new_cache", isNewCache,
 			"container_parent_id", containerParentID,
 			"container_name", container,
 			"restore_location", restoreLoc)
 
-		fid, err := getOrPopulateContainer(
+		containerID, err := getOrPopulateContainer(
 			ictx,
 			ca,
 			cache,
@@ -267,13 +267,12 @@ func createDestination(
 			userID,
 			containerParentID,
 			container,
-			isNewCache,
 			errs)
 		if err != nil {
 			return "", cache, clues.Stack(err)
 		}
 
-		containerParentID = fid
+		containerParentID = containerID
 	}
 
 	// containerParentID now identifies the last created container,
@@ -287,7 +286,6 @@ func getOrPopulateContainer(
 	gcr graph.ContainerResolver,
 	restoreLoc *path.Builder,
 	userID, containerParentID, containerName string,
-	isNewCache bool,
 	errs *fault.Bus,
 ) (string, error) {
 	cached, ok := gcr.LocationInCache(restoreLoc.String())
@@ -318,12 +316,6 @@ func getOrPopulateContainer(
 
 	folderID := ptr.Val(c.GetId())
 
-	if isNewCache {
-		if err := gcr.Populate(ctx, errs, folderID, ca.orRootContainer(restoreLoc.HeadElem())); err != nil {
-			return "", clues.Wrap(err, "populating container cache")
-		}
-	}
-
 	if err = gcr.AddToCache(ctx, c); err != nil {
 		return "", clues.Wrap(err, "adding container to cache")
 	}

src/internal/m365/graph/middleware.go

@@ -143,12 +143,11 @@ func (mw *LoggingMiddleware) Intercept(
 
 		// special cases where we always dump the response body, since the response
 		// details might be critical to understanding the response when debugging.
-		// * 400-bad-request
-		// * 403-forbidden
 		logBody = logger.DebugAPIFV ||
 			os.Getenv(logGraphRequestsEnvKey) != "" ||
 			resp.StatusCode == http.StatusBadRequest ||
-			resp.StatusCode == http.StatusForbidden
+			resp.StatusCode == http.StatusForbidden ||
+			resp.StatusCode == http.StatusConflict
 	)
 
 	// special case: always info-level status 429 logs

src/internal/m365/onedrive/restore.go

@@ -827,7 +827,7 @@ func restoreFile(
 		item                 = newItem(name, false)
 		collisionKey         = api.DriveItemCollisionKey(item)
 		collision            api.DriveCollisionItem
-		replace      bool
+		shouldDeleteOriginal bool
 	)
 
 	if dci, ok := collisionKeyToItemID[collisionKey]; ok {
@@ -842,7 +842,7 @@ func restoreFile(
 		}
 
 		collision = dci
-		replace = restoreCfg.OnCollision == control.Replace && !dci.IsFolder
+		shouldDeleteOriginal = restoreCfg.OnCollision == control.Replace && !dci.IsFolder
 	}
 
 	// drive items do not support PUT requests on the drive item data, so
@@ -852,8 +852,8 @@ func restoreFile(
 	// conflict replace handling bug gets fixed, we either delete-post, and
 	// risk failures in the middle, or we post w/ copy, then delete, then patch
 	// the name, which could triple our graph calls in the worst case.
-	if replace {
+	if shouldDeleteOriginal {
-		if err := ir.DeleteItem(ctx, driveID, collision.ItemID); err != nil {
+		if err := ir.DeleteItem(ctx, driveID, collision.ItemID); err != nil && !graph.IsErrDeletedInFlight(err) {
 			return "", details.ItemInfo{}, clues.New("deleting colliding item")
 		}
 	}

src/internal/m365/onedrive/restore_test.go

@@ -333,6 +333,7 @@ func (suite *RestoreUnitSuite) TestRestoreItem_collisionHandling() {
 		name          string
 		collisionKeys map[string]api.DriveCollisionItem
 		onCollision   control.CollisionPolicy
+		deleteErr     error
 		expectSkipped assert.BoolAssertionFunc
 		expectMock    func(*testing.T, *mock.RestoreHandler)
 	}{
@@ -391,6 +392,19 @@ func (suite *RestoreUnitSuite) TestRestoreItem_collisionHandling() {
 				assert.Equal(t, mndiID, rh.CalledDeleteItemOn, "deleted the correct item")
 			},
 		},
+		{
+			name: "collision, replace - err already deleted",
+			collisionKeys: map[string]api.DriveCollisionItem{
+				mock.DriveItemFileName: {ItemID: "smarf"},
+			},
+			onCollision:   control.Replace,
+			deleteErr:     graph.ErrDeletedInFlight,
+			expectSkipped: assert.False,
+			expectMock: func(t *testing.T, rh *mock.RestoreHandler) {
+				assert.True(t, rh.CalledPostItem, "new item posted")
+				assert.True(t, rh.CalledDeleteItem, "new item deleted")
+			},
+		},
 		{
 			name: "collision, skip",
 			collisionKeys: map[string]api.DriveCollisionItem{
@@ -461,7 +475,10 @@ func (suite *RestoreUnitSuite) TestRestoreItem_collisionHandling() {
 
 			var (
 				caches = NewRestoreCaches()
-				rh         = &mock.RestoreHandler{PostItemResp: mndi}
+				rh     = &mock.RestoreHandler{
+					PostItemResp:  models.NewDriveItem(),
+					DeleteItemErr: test.deleteErr,
+				}
 				restoreCfg = control.RestoreConfig{OnCollision: test.onCollision}
 				dpb        = odConsts.DriveFolderPrefixBuilder("driveID1")
 			)

src/internal/operations/restore.go

@@ -271,7 +271,7 @@ func (op *RestoreOperation) do(
 		op.Errors,
 		op.Counter)
 	if err != nil {
-		return nil, clues.Wrap(err, "restoring collections")
+		return nil, clues.Stack(err)
 	}
 
 	opStats.ctrl = op.rc.Wait()

src/pkg/backup/details/details.go

@@ -22,7 +22,7 @@ import (
 
 // Max number of items for which we will print details. If there are
 // more than this, then we just show a summary.
-const maxPrintLimit = 15
+const maxPrintLimit = 50
 
 // LocationIDer provides access to location information but guarantees that it
 // can also generate a unique location (among items in the same service but
@@ -504,13 +504,9 @@ func (ents entrySet) PrintEntries(ctx context.Context) {
 // MaybePrintEntries is same as PrintEntries, but only prints if we
 // have less than 15 items or is not json output.
 func (ents entrySet) MaybePrintEntries(ctx context.Context) {
-	if len(ents) > maxPrintLimit &&
+	if len(ents) <= maxPrintLimit ||
-		!print.DisplayJSONFormat() &&
+		print.DisplayJSONFormat() ||
-		!print.DisplayVerbose() {
+		print.DisplayVerbose() {
-		// TODO: Should we detect if the user is piping the output and
-		// print if that is the case?
-		print.Outf(ctx, "Restored %d items.", len(ents))
-	} else {
 		printEntries(ctx, ents)
 	}
 }

src/pkg/services/m365/api/config.go

@@ -17,16 +17,18 @@ const (
 // get easily misspelled.
 // eg: we don't need a const for "id"
 const (
-	attendees         = "attendees"
 	bccRecipients     = "bccRecipients"
 	ccRecipients      = "ccRecipients"
 	createdDateTime   = "createdDateTime"
 	displayName       = "displayName"
 	emailAddresses    = "emailAddresses"
 	givenName         = "givenName"
+	isCancelled       = "isCancelled"
+	isDraft           = "isDraft"
 	mobilePhone       = "mobilePhone"
 	parentFolderID    = "parentFolderId"
 	receivedDateTime  = "receivedDateTime"
+	recurrence        = "recurrence"
 	sentDateTime      = "sentDateTime"
 	surname           = "surname"
 	toRecipients      = "toRecipients"

src/pkg/services/m365/api/contacts_pager.go

@@ -112,6 +112,12 @@ func (c Contacts) NewContactsPager(
 		options.QueryParameters.Select = selectProps
 	}
 
+	// if we have no container ID, we need to fetch the
+	// base contacts container ID.
+	if len(containerID) == 0 {
+		containerID = DefaultContacts
+	}
+
 	builder := c.Stable.
 		Client().
 		Users().

src/pkg/services/m365/api/events.go

@@ -6,6 +6,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
+	"strconv"
 	"strings"
 	"time"
 
@@ -701,7 +702,17 @@ func EventFromMap(ev map[string]any) (models.Eventable, error) {
 }
 
 func eventCollisionKeyProps() []string {
-	return idAnd("subject", "type", "start", "end", "attendees", "recurrence")
+	// Do not use attendees here.  We slice out attendees from the
+	// restored event so that they do not receive an email for every
+	// restoration item.  Attendees will guarantee non-overlapping keys.
+	return idAnd(
+		"subject",
+		"type",
+		"start",
+		"end",
+		recurrence,
+		isCancelled,
+		isDraft)
 }
 
 // EventCollisionKey constructs a key from the eventable's creation time, subject, and organizer.
@@ -713,37 +724,34 @@ func EventCollisionKey(item models.Eventable) string {
 
 	var (
 		subject   = ptr.Val(item.GetSubject())
-		attendees = item.GetAttendees()
+		oftype    = ptr.Val(item.GetType()).String()
-		a         string
+		startTime = item.GetStart()
-		oftype    = ptr.Val(item.GetType())
+		start     string
-		t         = oftype.String()
+		endTime   = item.GetEnd()
-		start     = item.GetStart()
+		end       string
-		s         string
-		end       = item.GetEnd()
-		e         string
 		recurs    = item.GetRecurrence()
-		r         string
+		recur     string
+		cancelled = ptr.Val(item.GetIsCancelled())
+		draft     = ptr.Val(item.GetIsDraft())
 	)
 
-	for _, att := range attendees {
+	if startTime != nil {
-		if att.GetEmailAddress() != nil {
+		start = ptr.Val(startTime.GetDateTime())
-			a += ptr.Val(att.GetEmailAddress().GetAddress())
-		}
 	}
 
-	if start != nil {
+	if endTime != nil {
-		s = ptr.Val(start.GetDateTime())
+		end = ptr.Val(endTime.GetDateTime())
-	}
-
-	if end != nil {
-		e = ptr.Val(end.GetDateTime())
 	}
 
 	if recurs != nil && recurs.GetPattern() != nil {
-		r = ptr.Val(recurs.GetPattern().GetOdataType())
+		recur = ptr.Val(recurs.GetPattern().GetOdataType())
 	}
 
 	// this result gets hashed to ensure that an enormous list of attendees
 	// doesn't generate a multi-kb collision key.
-	return clues.ConcealWith(clues.SHA256, subject+a+t+s+e+r)
+	return subject +
+		oftype +
+		start + end + recur +
+		strconv.FormatBool(draft) +
+		strconv.FormatBool(cancelled)
 }