From 5ea194dc87faaa14ea47e133e0e013736665776b Mon Sep 17 00:00:00 2001 From: Keepers Date: Thu, 6 Jul 2023 16:24:26 -0600 Subject: [PATCH] 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_entry: No #### Type of change - [x] :bug: Bugfix #### Issue(s) * #3562 #### Test Plan - [x] :muscle: Manual --- src/cli/restore/restore.go | 7 ++- .../m365/exchange/contacts_restore.go | 15 ++++-- .../m365/exchange/contacts_restore_test.go | 13 +++++ .../m365/exchange/container_resolver_test.go | 5 +- src/internal/m365/exchange/events_restore.go | 10 ++-- .../m365/exchange/events_restore_test.go | 13 +++++ src/internal/m365/exchange/handlers.go | 7 +-- src/internal/m365/exchange/mail_restore.go | 5 +- .../m365/exchange/mail_restore_test.go | 13 +++++ src/internal/m365/exchange/restore.go | 28 ++++------ src/internal/m365/graph/middleware.go | 5 +- src/internal/m365/onedrive/restore.go | 14 ++--- src/internal/m365/onedrive/restore_test.go | 21 +++++++- src/internal/operations/restore.go | 2 +- src/pkg/backup/details/details.go | 12 ++--- src/pkg/services/m365/api/config.go | 4 +- src/pkg/services/m365/api/contacts_pager.go | 6 +++ src/pkg/services/m365/api/events.go | 52 +++++++++++-------- 18 files changed, 150 insertions(+), 82 deletions(-) diff --git a/src/cli/restore/restore.go b/src/cli/restore/restore.go index ff1d8897a..787de551e 100644 --- a/src/cli/restore/restore.go +++ b/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 } diff --git a/src/internal/m365/exchange/contacts_restore.go b/src/internal/m365/exchange/contacts_restore.go index 9a1e2ac02..7f7ffc498 100644 --- a/src/internal/m365/exchange/contacts_restore.go +++ b/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) orRootContainer(c string) string { - return c +func (h contactRestoreHandler) defaultRootContainer() string { + return api.DefaultContacts } 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") } } diff --git a/src/internal/m365/exchange/contacts_restore_test.go b/src/internal/m365/exchange/contacts_restore_test.go index 6d03c501a..0476cd35b 100644 --- a/src/internal/m365/exchange/contacts_restore_test.go +++ b/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() { diff --git a/src/internal/m365/exchange/container_resolver_test.go b/src/internal/m365/exchange/container_resolver_test.go index 908aa6c9b..8b5fa7c95 100644 --- a/src/internal/m365/exchange/container_resolver_test.go +++ b/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)) diff --git a/src/internal/m365/exchange/events_restore.go b/src/internal/m365/exchange/events_restore.go index 76ca30084..b8eb03e1f 100644 --- a/src/internal/m365/exchange/events_restore.go +++ b/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 { - return c +func (h eventRestoreHandler) defaultRootContainer() string { + 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") } } diff --git a/src/internal/m365/exchange/events_restore_test.go b/src/internal/m365/exchange/events_restore_test.go index 916f73c84..e280a0686 100644 --- a/src/internal/m365/exchange/events_restore_test.go +++ b/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() { diff --git a/src/internal/m365/exchange/handlers.go b/src/internal/m365/exchange/handlers.go index 51e193876..55ab7d42a 100644 --- a/src/internal/m365/exchange/handlers.go +++ b/src/internal/m365/exchange/handlers.go @@ -95,12 +95,7 @@ type containerAPI interface { ctx context.Context, userID, parentContainerID, containerName string, ) (graph.Container, error) - - // 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 + defaultRootContainer() string } type containerByNamer interface { diff --git a/src/internal/m365/exchange/mail_restore.go b/src/internal/m365/exchange/mail_restore.go index 5aa7f88db..67010c55d 100644 --- a/src/internal/m365/exchange/mail_restore.go +++ b/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) orRootContainer(string) string { +func (h mailRestoreHandler) defaultRootContainer() 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") } } diff --git a/src/internal/m365/exchange/mail_restore_test.go b/src/internal/m365/exchange/mail_restore_test.go index 8cc11d639..ec6fd69cc 100644 --- a/src/internal/m365/exchange/mail_restore_test.go +++ b/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() { diff --git a/src/internal/m365/exchange/restore.go b/src/internal/m365/exchange/restore.go index 37696604d..b65c21f99 100644 --- a/src/internal/m365/exchange/restore.go +++ b/src/internal/m365/exchange/restore.go @@ -55,9 +55,8 @@ func ConsumeRestoreCollections( } var ( - isNewCache bool - category = dc.FullPath().Category() - ictx = clues.Add( + category = dc.FullPath().Category() + ictx = clues.Add( ctx, "restore_category", category, "restore_full_path", dc.FullPath()) @@ -70,8 +69,12 @@ func ConsumeRestoreCollections( } if directoryCache[category] == nil { - directoryCache[category] = handler.newContainerCache(userID) - isNewCache = true + gcr := handler.newContainerCache(userID) + 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") } diff --git a/src/internal/m365/graph/middleware.go b/src/internal/m365/graph/middleware.go index 2e053f9d9..80d74144d 100644 --- a/src/internal/m365/graph/middleware.go +++ b/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 diff --git a/src/internal/m365/onedrive/restore.go b/src/internal/m365/onedrive/restore.go index 4daf1e0e9..66b4c6a56 100644 --- a/src/internal/m365/onedrive/restore.go +++ b/src/internal/m365/onedrive/restore.go @@ -824,10 +824,10 @@ func restoreFile( } var ( - item = newItem(name, false) - collisionKey = api.DriveItemCollisionKey(item) - collision api.DriveCollisionItem - replace bool + item = newItem(name, false) + collisionKey = api.DriveItemCollisionKey(item) + collision api.DriveCollisionItem + 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 err := ir.DeleteItem(ctx, driveID, collision.ItemID); err != nil { + if shouldDeleteOriginal { + if err := ir.DeleteItem(ctx, driveID, collision.ItemID); err != nil && !graph.IsErrDeletedInFlight(err) { return "", details.ItemInfo{}, clues.New("deleting colliding item") } } diff --git a/src/internal/m365/onedrive/restore_test.go b/src/internal/m365/onedrive/restore_test.go index 8a027175d..b9ff87715 100644 --- a/src/internal/m365/onedrive/restore_test.go +++ b/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{ @@ -460,8 +474,11 @@ func (suite *RestoreUnitSuite) TestRestoreItem_collisionHandling() { mndi.SetId(ptr.To(mndiID)) var ( - caches = NewRestoreCaches() - rh = &mock.RestoreHandler{PostItemResp: mndi} + caches = NewRestoreCaches() + rh = &mock.RestoreHandler{ + PostItemResp: models.NewDriveItem(), + DeleteItemErr: test.deleteErr, + } restoreCfg = control.RestoreConfig{OnCollision: test.onCollision} dpb = odConsts.DriveFolderPrefixBuilder("driveID1") ) diff --git a/src/internal/operations/restore.go b/src/internal/operations/restore.go index edc0cf1b6..4e5f8d8af 100644 --- a/src/internal/operations/restore.go +++ b/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() diff --git a/src/pkg/backup/details/details.go b/src/pkg/backup/details/details.go index bd2dbb909..4a5a7e4fd 100644 --- a/src/pkg/backup/details/details.go +++ b/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 && - !print.DisplayJSONFormat() && - !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 { + if len(ents) <= maxPrintLimit || + print.DisplayJSONFormat() || + print.DisplayVerbose() { printEntries(ctx, ents) } } diff --git a/src/pkg/services/m365/api/config.go b/src/pkg/services/m365/api/config.go index bdc221329..9e2247279 100644 --- a/src/pkg/services/m365/api/config.go +++ b/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" diff --git a/src/pkg/services/m365/api/contacts_pager.go b/src/pkg/services/m365/api/contacts_pager.go index b3e0ab7dd..69caa110a 100644 --- a/src/pkg/services/m365/api/contacts_pager.go +++ b/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(). diff --git a/src/pkg/services/m365/api/events.go b/src/pkg/services/m365/api/events.go index d1d5f8088..b907199bc 100644 --- a/src/pkg/services/m365/api/events.go +++ b/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() - a string - oftype = ptr.Val(item.GetType()) - t = oftype.String() - start = item.GetStart() - s string - end = item.GetEnd() - e string + oftype = ptr.Val(item.GetType()).String() + startTime = item.GetStart() + start string + endTime = item.GetEnd() + end string recurs = item.GetRecurrence() - r string + recur string + cancelled = ptr.Val(item.GetIsCancelled()) + draft = ptr.Val(item.GetIsDraft()) ) - for _, att := range attendees { - if att.GetEmailAddress() != nil { - a += ptr.Val(att.GetEmailAddress().GetAddress()) - } + if startTime != nil { + start = ptr.Val(startTime.GetDateTime()) } - if start != nil { - s = ptr.Val(start.GetDateTime()) - } - - if end != nil { - e = ptr.Val(end.GetDateTime()) + if endTime != nil { + end = ptr.Val(endTime.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) }