From 60f6d4a0359eab19b8aac261ebc0df0829e18daf Mon Sep 17 00:00:00 2001 From: Keepers Date: Fri, 12 May 2023 15:23:52 -0600 Subject: [PATCH] renaming/cleanup for sharepoint perms pt.1 (#3330) No logic changes, just renaming and minor cleanup. PRs to follow: 1. collect various maps in onedrive collections into a single cache. 2. logic changes and tests to produce sharepoint permissions backup/restore e2e. 3. extend permission identity-type retention. --- #### Does this PR need a docs update or release note? - [x] :no_entry: No #### Type of change - [x] :broom: Tech Debt/Cleanup #### Issue(s) * #3135 #### Test Plan - [x] :zap: Unit test --- src/cli/restore/onedrive.go | 2 +- src/internal/connector/graph/middleware.go | 9 +- src/internal/connector/onedrive/api/drive.go | 20 +++ src/internal/connector/onedrive/collection.go | 2 +- .../connector/onedrive/collections.go | 6 +- src/internal/connector/onedrive/drive_test.go | 12 +- src/internal/connector/onedrive/item_test.go | 2 +- src/internal/connector/onedrive/permission.go | 63 ++++---- src/internal/connector/onedrive/restore.go | 153 +++++++++--------- .../connector/onedrive/service_test.go | 2 + src/internal/connector/sharepoint/restore.go | 22 ++- src/internal/operations/backup.go | 4 +- .../operations/backup_integration_test.go | 17 +- src/internal/operations/restore.go | 4 +- 14 files changed, 172 insertions(+), 146 deletions(-) diff --git a/src/cli/restore/onedrive.go b/src/cli/restore/onedrive.go index 66e1f697e..1b0ebf410 100644 --- a/src/cli/restore/onedrive.go +++ b/src/cli/restore/onedrive.go @@ -52,7 +52,7 @@ corso restore onedrive --backup 1234abcd-12ab-cd34-56de-1234abcd --file 98765abc # Restore the file with ID 98765abcdef along with its associated permissions corso restore onedrive --backup 1234abcd-12ab-cd34-56de-1234abcd --file 98765abcdef --restore-permissions -# Restore files named "FY2021 Planning.xlsx in "Documents/Finance Reports" +# Restore files named "FY2021 Planning.xlsx" in "Documents/Finance Reports" corso restore onedrive --backup 1234abcd-12ab-cd34-56de-1234abcd \ --file "FY2021 Planning.xlsx" --folder "Documents/Finance Reports" diff --git a/src/internal/connector/graph/middleware.go b/src/internal/connector/graph/middleware.go index 108f03cac..25a6468c8 100644 --- a/src/internal/connector/graph/middleware.go +++ b/src/internal/connector/graph/middleware.go @@ -399,6 +399,10 @@ func (mw *MetricsMiddleware) Intercept( status = "nil-resp" ) + if resp == nil { + return resp, err + } + if resp != nil { status = resp.Status } @@ -410,11 +414,6 @@ func (mw *MetricsMiddleware) Intercept( // track the graph "resource cost" for each call (if not provided, assume 1) - // nil-pointer guard - if len(resp.Header) == 0 { - resp.Header = http.Header{} - } - // from msoft throttling documentation: // x-ms-resource-unit - Indicates the resource unit used for this request. Values are positive integer xmru := resp.Header.Get(xmruHeader) diff --git a/src/internal/connector/onedrive/api/drive.go b/src/internal/connector/onedrive/api/drive.go index d87546830..ab45e60bd 100644 --- a/src/internal/connector/onedrive/api/drive.go +++ b/src/internal/connector/onedrive/api/drive.go @@ -418,3 +418,23 @@ func GetFolderByName( return foundItem, nil } + +func PostItemPermissionUpdate( + ctx context.Context, + service graph.Servicer, + driveID, itemID string, + body *drive.ItemsItemInvitePostRequestBody, +) (drives.ItemItemsItemInviteResponseable, error) { + ctx = graph.ConsumeNTokens(ctx, graph.PermissionsLC) + + itm, err := service.Client(). + DrivesById(driveID). + ItemsById(itemID). + Invite(). + Post(ctx, body, nil) + if err != nil { + return nil, graph.Wrap(ctx, err, "posting permissions") + } + + return itm, nil +} diff --git a/src/internal/connector/onedrive/collection.go b/src/internal/connector/onedrive/collection.go index 26fd41283..c339efb07 100644 --- a/src/internal/connector/onedrive/collection.go +++ b/src/internal/connector/onedrive/collection.go @@ -478,7 +478,7 @@ func (oc *Collection) populateItems(ctx context.Context, errs *fault.Bus) { ctx = clues.Add( ctx, "item_id", itemID, - "item_name", itemName, + "item_name", clues.Hide(itemName), "item_size", itemSize) item.SetParentReference(setName(item.GetParentReference(), oc.driveName)) diff --git a/src/internal/connector/onedrive/collections.go b/src/internal/connector/onedrive/collections.go index 52f29f879..d7503949e 100644 --- a/src/internal/connector/onedrive/collections.go +++ b/src/internal/connector/onedrive/collections.go @@ -646,7 +646,7 @@ func (c *Collections) getCollectionPath( if item.GetParentReference() == nil || item.GetParentReference().GetPath() == nil { err := clues.New("no parent reference"). - With("item_name", ptr.Val(item.GetName())) + With("item_name", clues.Hide(ptr.Val(item.GetName()))) return nil, err } @@ -711,7 +711,7 @@ func (c *Collections) UpdateCollections( var ( itemID = ptr.Val(item.GetId()) itemName = ptr.Val(item.GetName()) - ictx = clues.Add(ctx, "item_id", itemID, "item_name", itemName) + ictx = clues.Add(ctx, "item_id", itemID, "item_name", clues.Hide(itemName)) isFolder = item.GetFolder() != nil || item.GetPackage() != nil ) @@ -758,7 +758,7 @@ func (c *Collections) UpdateCollections( // Skip items that don't match the folder selectors we were given. if shouldSkipDrive(ctx, collectionPath, c.matcher, driveName) { - logger.Ctx(ictx).Debugw("Skipping drive path", "skipped_path", collectionPath.String()) + logger.Ctx(ictx).Debugw("path not selected", "skipped_path", collectionPath.String()) continue } diff --git a/src/internal/connector/onedrive/drive_test.go b/src/internal/connector/onedrive/drive_test.go index 2a5d4b5a8..d1008ee37 100644 --- a/src/internal/connector/onedrive/drive_test.go +++ b/src/internal/connector/onedrive/drive_test.go @@ -335,8 +335,16 @@ func (suite *OneDriveIntgSuite) TestCreateGetDeleteFolder() { require.NoError(t, err, clues.ToCore(err)) restoreFolders := path.Builder{}.Append(folderElements...) + drivePath := path.DrivePath{ + DriveID: driveID, + Root: "root:", + Folders: folderElements, + } - folderID, err := CreateRestoreFolders(ctx, gs, driveID, ptr.Val(rootFolder.GetId()), restoreFolders, NewFolderCache()) + caches := NewRestoreCaches() + caches.DriveIDToRootFolderID[driveID] = ptr.Val(rootFolder.GetId()) + + folderID, err := CreateRestoreFolders(ctx, gs, &drivePath, restoreFolders, caches) require.NoError(t, err, clues.ToCore(err)) folderIDs = append(folderIDs, folderID) @@ -344,7 +352,7 @@ func (suite *OneDriveIntgSuite) TestCreateGetDeleteFolder() { folderName2 := "Corso_Folder_Test_" + dttm.FormatNow(dttm.SafeForTesting) restoreFolders = restoreFolders.Append(folderName2) - folderID, err = CreateRestoreFolders(ctx, gs, driveID, ptr.Val(rootFolder.GetId()), restoreFolders, NewFolderCache()) + folderID, err = CreateRestoreFolders(ctx, gs, &drivePath, restoreFolders, caches) require.NoError(t, err, clues.ToCore(err)) folderIDs = append(folderIDs, folderID) diff --git a/src/internal/connector/onedrive/item_test.go b/src/internal/connector/onedrive/item_test.go index 47feea0ff..79caff036 100644 --- a/src/internal/connector/onedrive/item_test.go +++ b/src/internal/connector/onedrive/item_test.go @@ -283,7 +283,7 @@ func TestItemUnitTestSuite(t *testing.T) { suite.Run(t, &ItemUnitTestSuite{Suite: tester.NewUnitSuite(t)}) } -func (suite *ItemUnitTestSuite) TestOneDrivePermissionsFilter() { +func (suite *ItemUnitTestSuite) TestDrivePermissionsFilter() { permID := "fakePermId" userID := "fakeuser@provider.com" userID2 := "fakeuser2@provider.com" diff --git a/src/internal/connector/onedrive/permission.go b/src/internal/connector/onedrive/permission.go index b67973be0..39ad546e7 100644 --- a/src/internal/connector/onedrive/permission.go +++ b/src/internal/connector/onedrive/permission.go @@ -4,11 +4,12 @@ import ( "context" "github.com/alcionai/clues" - msdrive "github.com/microsoftgraph/msgraph-sdk-go/drive" + "github.com/microsoftgraph/msgraph-sdk-go/drive" "github.com/microsoftgraph/msgraph-sdk-go/models" "github.com/alcionai/corso/src/internal/common/ptr" "github.com/alcionai/corso/src/internal/connector/graph" + "github.com/alcionai/corso/src/internal/connector/onedrive/api" "github.com/alcionai/corso/src/internal/connector/onedrive/metadata" "github.com/alcionai/corso/src/internal/data" "github.com/alcionai/corso/src/internal/version" @@ -18,9 +19,9 @@ import ( func getParentMetadata( parentPath path.Path, - metas map[string]metadata.Metadata, + parentDirToMeta map[string]metadata.Metadata, ) (metadata.Metadata, error) { - parentMeta, ok := metas[parentPath.String()] + parentMeta, ok := parentDirToMeta[parentPath.String()] if !ok { drivePath, err := path.ToDrivePath(parentPath) if err != nil { @@ -41,7 +42,7 @@ func getCollectionMetadata( ctx context.Context, drivePath *path.DrivePath, dc data.RestoreCollection, - metas map[string]metadata.Metadata, + caches *restoreCaches, backupVersion int, restorePerms bool, ) (metadata.Metadata, error) { @@ -60,7 +61,7 @@ func getCollectionMetadata( } if backupVersion < version.OneDrive4DirIncludesPermissions { - colMeta, err := getParentMetadata(collectionPath, metas) + colMeta, err := getParentMetadata(collectionPath, caches.ParentDirToMeta) if err != nil { return metadata.Metadata{}, clues.Wrap(err, "collection metadata") } @@ -85,12 +86,13 @@ func getCollectionMetadata( } // computeParentPermissions computes the parent permissions by -// traversing folderMetas and finding the first item with custom -// permissions. folderMetas is expected to have all the parent +// traversing parentMetas and finding the first item with custom +// permissions. parentMetas is expected to have all the parent // directory metas for this to work. func computeParentPermissions( - itemPath path.Path, - folderMetas map[string]metadata.Metadata, + originDir path.Path, + // map parent dir -> parent's metadata + parentMetas map[string]metadata.Metadata, ) (metadata.Metadata, error) { var ( parent path.Path @@ -100,7 +102,7 @@ func computeParentPermissions( ok bool ) - parent = itemPath + parent = originDir for { parent, err = parent.Dir() @@ -110,14 +112,14 @@ func computeParentPermissions( drivePath, err := path.ToDrivePath(parent) if err != nil { - return metadata.Metadata{}, clues.New("get parent path") + return metadata.Metadata{}, clues.New("transforming dir to drivePath") } if len(drivePath.Folders) == 0 { return metadata.Metadata{}, nil } - meta, ok = folderMetas[parent.String()] + meta, ok = parentMetas[parent.String()] if !ok { return metadata.Metadata{}, clues.New("no parent meta") } @@ -137,7 +139,7 @@ func UpdatePermissions( driveID string, itemID string, permAdded, permRemoved []metadata.Permission, - permissionIDMappings map[string]string, + oldPermIDToNewID map[string]string, ) error { // The ordering of the operations is important here. We first // remove all the removed permissions and then add the added ones. @@ -151,7 +153,7 @@ func UpdatePermissions( return graph.Wrap(ctx, err, "creating delete client") } - pid, ok := permissionIDMappings[p.ID] + pid, ok := oldPermIDToNewID[p.ID] if !ok { return clues.New("no new permission id").WithClues(ctx) } @@ -182,7 +184,7 @@ func UpdatePermissions( continue } - pbody := msdrive.NewItemsItemInvitePostRequestBody() + pbody := drive.NewItemsItemInvitePostRequestBody() pbody.SetRoles(roles) if p.Expiration != nil { @@ -207,16 +209,12 @@ func UpdatePermissions( pbody.SetRecipients([]models.DriveRecipientable{rec}) - np, err := service.Client(). - DrivesById(driveID). - ItemsById(itemID). - Invite(). - Post(graph.ConsumeNTokens(ctx, graph.PermissionsLC), pbody, nil) + newPerm, err := api.PostItemPermissionUpdate(ctx, service, driveID, itemID, pbody) if err != nil { - return graph.Wrap(ctx, err, "setting permissions") + return clues.Stack(err) } - permissionIDMappings[p.ID] = ptr.Val(np.GetValue()[0].GetId()) + oldPermIDToNewID[p.ID] = ptr.Val(newPerm.GetValue()[0].GetId()) } return nil @@ -233,22 +231,29 @@ func RestorePermissions( driveID string, itemID string, itemPath path.Path, - meta metadata.Metadata, - folderMetas map[string]metadata.Metadata, - permissionIDMappings map[string]string, + current metadata.Metadata, + caches *restoreCaches, ) error { - if meta.SharingMode == metadata.SharingModeInherited { + if current.SharingMode == metadata.SharingModeInherited { return nil } ctx = clues.Add(ctx, "permission_item_id", itemID) - parentPermissions, err := computeParentPermissions(itemPath, folderMetas) + parents, err := computeParentPermissions(itemPath, caches.ParentDirToMeta) if err != nil { return clues.Wrap(err, "parent permissions").WithClues(ctx) } - permAdded, permRemoved := metadata.DiffPermissions(parentPermissions.Permissions, meta.Permissions) + permAdded, permRemoved := metadata.DiffPermissions(parents.Permissions, current.Permissions) - return UpdatePermissions(ctx, creds, service, driveID, itemID, permAdded, permRemoved, permissionIDMappings) + return UpdatePermissions( + ctx, + creds, + service, + driveID, + itemID, + permAdded, + permRemoved, + caches.OldPermIDToNewID) } diff --git a/src/internal/connector/onedrive/restore.go b/src/internal/connector/onedrive/restore.go index 41d037b13..36f8d5d2d 100644 --- a/src/internal/connector/onedrive/restore.go +++ b/src/internal/connector/onedrive/restore.go @@ -33,6 +33,22 @@ import ( // https://docs.microsoft.com/en-us/graph/api/driveitem-createuploadsession?view=graph-rest-1.0#best-practices const copyBufferSize = 5 * 1024 * 1024 +type restoreCaches struct { + Folders *folderCache + ParentDirToMeta map[string]metadata.Metadata + OldPermIDToNewID map[string]string + DriveIDToRootFolderID map[string]string +} + +func NewRestoreCaches() *restoreCaches { + return &restoreCaches{ + Folders: NewFolderCache(), + ParentDirToMeta: map[string]metadata.Metadata{}, + OldPermIDToNewID: map[string]string{}, + DriveIDToRootFolderID: map[string]string{}, + } +} + // RestoreCollections will restore the specified data collections into OneDrive func RestoreCollections( ctx context.Context, @@ -47,14 +63,8 @@ func RestoreCollections( ) (*support.ConnectorOperationStatus, error) { var ( restoreMetrics support.CollectionMetrics - metrics support.CollectionMetrics - folderMetas = map[string]metadata.Metadata{} - - // permissionIDMappings is used to map between old and new id - // of permissions as we restore them - permissionIDMappings = map[string]string{} - fc = NewFolderCache() - rootIDCache = map[string]string{} + caches = NewRestoreCaches() + el = errs.Local() ) ctx = clues.Add( @@ -68,8 +78,6 @@ func RestoreCollections( return dcs[i].FullPath().String() < dcs[j].FullPath().String() }) - el := errs.Local() - // Iterate through the data collections and restore the contents of each for _, dc := range dcs { if el.Failure() != nil { @@ -77,8 +85,9 @@ func RestoreCollections( } var ( - err error - ictx = clues.Add( + err error + metrics support.CollectionMetrics + ictx = clues.Add( ctx, "resource_owner", clues.Hide(dc.FullPath().ResourceOwner()), "category", dc.FullPath().Category(), @@ -91,10 +100,7 @@ func RestoreCollections( backupVersion, service, dc, - folderMetas, - permissionIDMappings, - fc, - rootIDCache, + caches, OneDriveSource, dest.ContainerName, deets, @@ -132,10 +138,7 @@ func RestoreCollection( backupVersion int, service graph.Servicer, dc data.RestoreCollection, - folderMetas map[string]metadata.Metadata, - permissionIDMappings map[string]string, - fc *folderCache, - rootIDCache map[string]string, // map of drive id -> root folder ID + caches *restoreCaches, source driveSource, restoreContainerName string, deets *details.Builder, @@ -157,17 +160,13 @@ func RestoreCollection( return metrics, clues.Wrap(err, "creating drive path").WithClues(ctx) } - if rootIDCache == nil { - rootIDCache = map[string]string{} - } - - if _, ok := rootIDCache[drivePath.DriveID]; !ok { + if _, ok := caches.DriveIDToRootFolderID[drivePath.DriveID]; !ok { root, err := api.GetDriveRoot(ctx, service, drivePath.DriveID) if err != nil { return metrics, clues.Wrap(err, "getting drive root id") } - rootIDCache[drivePath.DriveID] = ptr.Val(root.GetId()) + caches.DriveIDToRootFolderID[drivePath.DriveID] = ptr.Val(root.GetId()) } // Assemble folder hierarchy we're going to restore into (we recreate the folder hierarchy @@ -189,7 +188,7 @@ func RestoreCollection( ctx, drivePath, dc, - folderMetas, + caches, backupVersion, restorePerms) if err != nil { @@ -202,19 +201,16 @@ func RestoreCollection( creds, service, drivePath, - rootIDCache[drivePath.DriveID], restoreFolderElements, dc.FullPath(), colMeta, - folderMetas, - fc, - permissionIDMappings, + caches, restorePerms) if err != nil { return metrics, clues.Wrap(err, "creating folders for restore") } - folderMetas[dc.FullPath().String()] = colMeta + caches.ParentDirToMeta[dc.FullPath().String()] = colMeta items := dc.Items(ctx, errs) for { @@ -231,14 +227,16 @@ func RestoreCollection( return metrics, nil } + ictx := clues.Add(ctx, "restore_item_id", itemData.UUID()) + itemPath, err := dc.FullPath().AppendItem(itemData.UUID()) if err != nil { - el.AddRecoverable(clues.Wrap(err, "appending item to full path").WithClues(ctx)) + el.AddRecoverable(clues.Wrap(err, "appending item to full path").WithClues(ictx)) continue } itemInfo, skipped, err := restoreItem( - ctx, + ictx, creds, dc, backupVersion, @@ -247,8 +245,7 @@ func RestoreCollection( drivePath, restoreFolderID, copyBuffer, - folderMetas, - permissionIDMappings, + caches, restorePerms, itemData, itemPath) @@ -265,7 +262,7 @@ func RestoreCollection( } if skipped { - logger.Ctx(ctx).With("item_path", itemPath).Debug("did not restore item") + logger.Ctx(ictx).With("item_path", itemPath).Debug("did not restore item") continue } @@ -276,7 +273,7 @@ func RestoreCollection( itemInfo) if err != nil { // Not critical enough to need to stop restore operation. - logger.CtxErr(ctx, err).Infow("adding restored item to details") + logger.CtxErr(ictx, err).Infow("adding restored item to details") } metrics.Successes++ @@ -298,8 +295,7 @@ func restoreItem( drivePath *path.DrivePath, restoreFolderID string, copyBuffer []byte, - folderMetas map[string]metadata.Metadata, - permissionIDMappings map[string]string, + caches *restoreCaches, restorePerms bool, itemData data.Stream, itemPath path.Path, @@ -348,7 +344,7 @@ func restoreItem( } trimmedPath := strings.TrimSuffix(itemPath.String(), metadata.DirMetaFileSuffix) - folderMetas[trimmedPath] = meta + caches.ParentDirToMeta[trimmedPath] = meta return details.ItemInfo{}, true, nil } @@ -366,8 +362,7 @@ func restoreItem( restoreFolderID, copyBuffer, restorePerms, - folderMetas, - permissionIDMappings, + caches, itemPath, itemData) if err != nil { @@ -389,8 +384,7 @@ func restoreItem( restoreFolderID, copyBuffer, restorePerms, - folderMetas, - permissionIDMappings, + caches, itemPath, itemData) if err != nil { @@ -439,8 +433,7 @@ func restoreV1File( restoreFolderID string, copyBuffer []byte, restorePerms bool, - folderMetas map[string]metadata.Metadata, - permissionIDMappings map[string]string, + caches *restoreCaches, itemPath path.Path, itemData data.Stream, ) (details.ItemInfo, error) { @@ -481,8 +474,7 @@ func restoreV1File( itemID, itemPath, meta, - folderMetas, - permissionIDMappings) + caches) if err != nil { return details.ItemInfo{}, clues.Wrap(err, "restoring item permissions") } @@ -500,8 +492,7 @@ func restoreV6File( restoreFolderID string, copyBuffer []byte, restorePerms bool, - folderMetas map[string]metadata.Metadata, - permissionIDMappings map[string]string, + caches *restoreCaches, itemPath path.Path, itemData data.Stream, ) (details.ItemInfo, error) { @@ -515,6 +506,11 @@ func restoreV6File( return details.ItemInfo{}, clues.Wrap(err, "restoring file") } + ctx = clues.Add( + ctx, + "count_perms", len(meta.Permissions), + "restore_item_name", clues.Hide(meta.FileName)) + if err != nil { return details.ItemInfo{}, clues.Wrap(err, "deserializing item metadata") } @@ -553,8 +549,7 @@ func restoreV6File( itemID, itemPath, meta, - folderMetas, - permissionIDMappings) + caches) if err != nil { return details.ItemInfo{}, clues.Wrap(err, "restoring item permissions") } @@ -572,22 +567,18 @@ func createRestoreFoldersWithPermissions( creds account.M365Config, service graph.Servicer, drivePath *path.DrivePath, - driveRootID string, restoreFolders *path.Builder, folderPath path.Path, folderMetadata metadata.Metadata, - folderMetas map[string]metadata.Metadata, - fc *folderCache, - permissionIDMappings map[string]string, + caches *restoreCaches, restorePerms bool, ) (string, error) { id, err := CreateRestoreFolders( ctx, service, - drivePath.DriveID, - driveRootID, + drivePath, restoreFolders, - fc) + caches) if err != nil { return "", err } @@ -609,8 +600,7 @@ func createRestoreFoldersWithPermissions( id, folderPath, folderMetadata, - folderMetas, - permissionIDMappings) + caches) return id, err } @@ -621,16 +611,22 @@ func createRestoreFoldersWithPermissions( func CreateRestoreFolders( ctx context.Context, service graph.Servicer, - driveID, driveRootID string, - restoreFolders *path.Builder, - fc *folderCache, + drivePath *path.DrivePath, + restoreDir *path.Builder, + caches *restoreCaches, ) (string, error) { var ( - location = &path.Builder{} - parentFolderID = driveRootID - folders = restoreFolders.Elements() + driveID = drivePath.DriveID + folders = restoreDir.Elements() + location = path.Builder{}.Append(driveID) + parentFolderID = caches.DriveIDToRootFolderID[drivePath.DriveID] ) + ctx = clues.Add( + ctx, + "drive_id", drivePath.DriveID, + "root_folder_id", parentFolderID) + for _, folder := range folders { location = location.Append(folder) ictx := clues.Add( @@ -639,7 +635,7 @@ func CreateRestoreFolders( "restore_folder_location", location, "parent_of_restore_folder", parentFolderID) - if fl, ok := fc.get(location); ok { + if fl, ok := caches.Folders.get(location); ok { parentFolderID = ptr.Val(fl.GetId()) // folder was already created, move on to the child continue @@ -647,27 +643,27 @@ func CreateRestoreFolders( folderItem, err := api.GetFolderByName(ictx, service, driveID, parentFolderID, folder) if err != nil && !errors.Is(err, api.ErrFolderNotFound) { - return "", clues.Wrap(err, "getting folder by display name").WithClues(ctx) + return "", clues.Wrap(err, "getting folder by display name") } // folder found, moving to next child if err == nil { parentFolderID = ptr.Val(folderItem.GetId()) - fc.set(location, folderItem) + caches.Folders.set(location, folderItem) continue } // create the folder if not found - folderItem, err = CreateItem(ctx, service, driveID, parentFolderID, newItem(folder, true)) + folderItem, err = CreateItem(ictx, service, driveID, parentFolderID, newItem(folder, true)) if err != nil { return "", clues.Wrap(err, "creating folder") } parentFolderID = ptr.Val(folderItem.GetId()) - fc.set(location, folderItem) + caches.Folders.set(location, folderItem) - logger.Ctx(ctx).Debug("resolved restore destination") + logger.Ctx(ictx).Debug("resolved restore destination") } return parentFolderID, nil @@ -686,10 +682,7 @@ func restoreData( ctx, end := diagnostics.Span(ctx, "gc:oneDrive:restoreItem", diagnostics.Label("item_uuid", itemData.UUID())) defer end() - ctx = clues.Add(ctx, "item_name", itemData.UUID()) - - itemName := itemData.UUID() - trace.Log(ctx, "gc:oneDrive:restoreItem", itemName) + trace.Log(ctx, "gc:oneDrive:restoreItem", itemData.UUID()) // Get the stream size (needed to create the upload session) ss, ok := itemData.(data.StreamSize) @@ -700,13 +693,13 @@ func restoreData( // Create Item newItem, err := CreateItem(ctx, service, driveID, parentFolderID, newItem(name, false)) if err != nil { - return "", details.ItemInfo{}, clues.Wrap(err, "creating item") + return "", details.ItemInfo{}, err } // Get a drive item writer w, err := driveItemWriter(ctx, service, driveID, ptr.Val(newItem.GetId()), ss.Size()) if err != nil { - return "", details.ItemInfo{}, clues.Wrap(err, "creating item writer") + return "", details.ItemInfo{}, err } iReader := itemData.ToReader() diff --git a/src/internal/connector/onedrive/service_test.go b/src/internal/connector/onedrive/service_test.go index 94aac53b3..9c27d7dde 100644 --- a/src/internal/connector/onedrive/service_test.go +++ b/src/internal/connector/onedrive/service_test.go @@ -23,6 +23,8 @@ func (ms *MockGraphService) Adapter() *msgraphsdk.GraphRequestAdapter { return nil } +var _ graph.Servicer = &oneDriveService{} + // TODO(ashmrtn): Merge with similar structs in graph and exchange packages. type oneDriveService struct { client msgraphsdk.GraphServiceClient diff --git a/src/internal/connector/sharepoint/restore.go b/src/internal/connector/sharepoint/restore.go index 2f64454da..c9eab4889 100644 --- a/src/internal/connector/sharepoint/restore.go +++ b/src/internal/connector/sharepoint/restore.go @@ -2,6 +2,7 @@ package sharepoint import ( "context" + "errors" "fmt" "io" "runtime/trace" @@ -12,7 +13,6 @@ import ( "github.com/alcionai/corso/src/internal/common/ptr" "github.com/alcionai/corso/src/internal/connector/graph" "github.com/alcionai/corso/src/internal/connector/onedrive" - "github.com/alcionai/corso/src/internal/connector/onedrive/metadata" "github.com/alcionai/corso/src/internal/connector/sharepoint/api" "github.com/alcionai/corso/src/internal/connector/support" "github.com/alcionai/corso/src/internal/data" @@ -51,20 +51,25 @@ func RestoreCollections( errs *fault.Bus, ) (*support.ConnectorOperationStatus, error) { var ( - err error restoreMetrics support.CollectionMetrics + caches = onedrive.NewRestoreCaches() + el = errs.Local() ) // Iterate through the data collections and restore the contents of each for _, dc := range dcs { + if el.Failure() != nil { + break + } + var ( + err error category = dc.FullPath().Category() metrics support.CollectionMetrics ictx = clues.Add(ctx, "category", category, "destination", clues.Hide(dest.ContainerName), "resource_owner", clues.Hide(dc.FullPath().ResourceOwner())) - driveFolderCache = onedrive.NewFolderCache() ) switch dc.FullPath().Category() { @@ -75,10 +80,7 @@ func RestoreCollections( backupVersion, service, dc, - map[string]metadata.Metadata{}, // Currently permission data is not stored for sharepoint - map[string]string{}, - driveFolderCache, - nil, + caches, onedrive.SharePointSource, dest.ContainerName, deets, @@ -110,6 +112,10 @@ func RestoreCollections( restoreMetrics = support.CombineMetrics(restoreMetrics, metrics) if err != nil { + el.AddRecoverable(err) + } + + if errors.Is(err, context.Canceled) { break } } @@ -121,7 +127,7 @@ func RestoreCollections( restoreMetrics, dest.ContainerName) - return status, err + return status, el.Failure() } // restoreListItem utility function restores a List to the siteID. diff --git a/src/internal/operations/backup.go b/src/internal/operations/backup.go index c7e8bcfeb..033fca8bb 100644 --- a/src/internal/operations/backup.go +++ b/src/internal/operations/backup.go @@ -199,9 +199,7 @@ func (op *BackupOperation) Run(ctx context.Context) (err error) { op.Results.BackupID) if err != nil { // No return here! We continue down to persistResults, even in case of failure. - logger.Ctx(ctx). - With("err", err). - Errorw("running backup", clues.InErr(err).Slice()...) + logger.CtxErr(ctx, err).Error("running backup") op.Errors.Fail(clues.Wrap(err, "running backup")) } diff --git a/src/internal/operations/backup_integration_test.go b/src/internal/operations/backup_integration_test.go index c7e48f001..cf191dc2c 100644 --- a/src/internal/operations/backup_integration_test.go +++ b/src/internal/operations/backup_integration_test.go @@ -1605,9 +1605,8 @@ func runDriveIncrementalTest( *newFile.GetId(), []metadata.Permission{}, []metadata.Permission{writePerm}, - permissionIDMappings, - ) - require.NoErrorf(t, err, "adding permission to file %v", clues.ToCore(err)) + permissionIDMappings) + require.NoErrorf(t, err, "removing permission from file %v", clues.ToCore(err)) // no expectedDeets: metadata isn't tracked }, itemsRead: 1, // .data file for newitem @@ -1629,10 +1628,9 @@ func runDriveIncrementalTest( targetContainer, []metadata.Permission{writePerm}, []metadata.Permission{}, - permissionIDMappings, - ) - require.NoErrorf(t, err, "adding permission to file %v", clues.ToCore(err)) - // no expectedDeets: metadata isn't tracked5tgb + permissionIDMappings) + require.NoErrorf(t, err, "adding permission to container %v", clues.ToCore(err)) + // no expectedDeets: metadata isn't tracked }, itemsRead: 0, itemsWritten: 1, // .dirmeta for collection @@ -1653,9 +1651,8 @@ func runDriveIncrementalTest( targetContainer, []metadata.Permission{}, []metadata.Permission{writePerm}, - permissionIDMappings, - ) - require.NoErrorf(t, err, "adding permission to file %v", clues.ToCore(err)) + permissionIDMappings) + require.NoErrorf(t, err, "removing permission from container %v", clues.ToCore(err)) // no expectedDeets: metadata isn't tracked }, itemsRead: 0, diff --git a/src/internal/operations/restore.go b/src/internal/operations/restore.go index 30bd08363..c0c8ef8f7 100644 --- a/src/internal/operations/restore.go +++ b/src/internal/operations/restore.go @@ -166,9 +166,7 @@ func (op *RestoreOperation) Run(ctx context.Context) (restoreDetails *details.De deets, err := op.do(ctx, &opStats, sstore, start) if err != nil { // No return here! We continue down to persistResults, even in case of failure. - logger.Ctx(ctx). - With("err", err). - Errorw("running restore", clues.InErr(err).Slice()...) + logger.CtxErr(ctx, err).Error("running restore") op.Errors.Fail(clues.Wrap(err, "running restore")) }