From 8db03d4cd7dd4ebe164de1154b767a9153047849 Mon Sep 17 00:00:00 2001 From: ryanfkeepers Date: Tue, 18 Jul 2023 14:40:21 -0600 Subject: [PATCH] utilize old drive name when restoring utilizes the old drive name in case the drive was deleted between backup and restore. Priority is first to use drives whose ids match the backup id; second, to use drives whose names match the backup drive name; third, to fall back to a new, arbitrary name. --- src/internal/m365/onedrive/restore.go | 66 +++++++---- src/internal/m365/onedrive/restore_test.go | 128 ++++++++++++++++++--- 2 files changed, 151 insertions(+), 43 deletions(-) diff --git a/src/internal/m365/onedrive/restore.go b/src/internal/m365/onedrive/restore.go index 58faa28cd..6d71b389f 100644 --- a/src/internal/m365/onedrive/restore.go +++ b/src/internal/m365/onedrive/restore.go @@ -258,7 +258,7 @@ func RestoreCollection( return metrics, clues.Wrap(err, "creating drive path").WithClues(ctx) } - err = ensureDriveExists( + di, err := ensureDriveExists( ctx, rh, caches, @@ -269,6 +269,12 @@ func RestoreCollection( return metrics, clues.Wrap(err, "ensuring drive exists") } + // clobber the drivePath details with the details retrieved + // in the ensure func, as they might have changed to reflect + // a different drive as a restore location. + drivePath.DriveID = di.id + drivePath.Root = di.rootFolderID + // Assemble folder hierarchy we're going to restore into (we recreate the folder hierarchy // from the backup under this the restore folder instead of root) // i.e. Restore into `/` @@ -1215,51 +1221,63 @@ func ensureDriveExists( pdagrf PostDriveAndGetRootFolderer, caches *restoreCaches, drivePath *path.DrivePath, - protectedResourceID, driveName string, -) error { + protectedResourceID, fallbackDriveName string, +) (driveInfo, error) { driveID := drivePath.DriveID - // the drive might already be cached - if _, ok := caches.DriveIDToDriveInfo[driveID]; ok { - return nil + // the drive might already be cached by ID. it's okay + // if the name has changed. the ID is a better reference + // anyway. + if di, ok := caches.DriveIDToDriveInfo[driveID]; ok { + return di, nil } var ( - newDriveName = driveName + newDriveName = fallbackDriveName newDrive models.Driveable err error ) - if _, ok := caches.DriveNameToDriveInfo[newDriveName]; ok { - newDriveName = fmt.Sprintf("%s %d", driveName, 1) + // if the drive wasn't found by ID, maybe we can find a + // drive with the same name but different ID. + // start by looking up the old drive's name + oldName, ok := caches.BackupDriveIDName.NameOf(driveID) + if ok { + // check for drives that currently have the same name + if di, ok := caches.DriveNameToDriveInfo[oldName]; ok { + return di, nil + } + + // if no current drives have the same name, we'll make + // a new drive with that name. + newDriveName = oldName } - // if not, double check that the name won't collide by looking - // up drives by name until we can make some name like `foo N` that - // doesn't collide with `foo` or other values of N in `foo N`. - // Ex: foo -> foo 1 -> foo 2 -> ... -> foo N - // + nextDriveName := newDriveName + // For sharepoint, document libraries can collide by name with // item types beyond just drive. Lists, for example, cannot share - // names with document libraries. In those cases it's not enough - // to compare the names of drives; we also need to continue this - // loop until we can create a drive without error. - for i := 2; ; i++ { - ictx := clues.Add(ctx, "new_drive_name", clues.Hide(newDriveName)) + // names with document libraries (they're the same type, actually). + // In those cases we need to rename the drive until we can create + // one without a collision. + for i := 1; ; i++ { + ictx := clues.Add(ctx, "new_drive_name", clues.Hide(nextDriveName)) - newDrive, err = pdagrf.PostDrive(ictx, protectedResourceID, newDriveName) + newDrive, err = pdagrf.PostDrive(ictx, protectedResourceID, nextDriveName) if err != nil && !errors.Is(err, graph.ErrItemAlreadyExistsConflict) { - return clues.Wrap(err, "creating new drive") + return driveInfo{}, clues.Wrap(err, "creating new drive") } if err == nil { break } - newDriveName = fmt.Sprintf("%s %d", driveName, i) + nextDriveName = fmt.Sprintf("%s %d", newDriveName, i) } - err = caches.AddDrive(ctx, newDrive, pdagrf) + if err := caches.AddDrive(ctx, newDrive, pdagrf); err != nil { + return driveInfo{}, clues.Wrap(err, "adding drive to cache").OrNil() + } - return clues.Wrap(err, "adding drive to cache").OrNil() + return caches.DriveIDToDriveInfo[ptr.Val(newDrive.GetId())], nil } diff --git a/src/internal/m365/onedrive/restore_test.go b/src/internal/m365/onedrive/restore_test.go index e8b41a45b..dbb7317c9 100644 --- a/src/internal/m365/onedrive/restore_test.go +++ b/src/internal/m365/onedrive/restore_test.go @@ -11,6 +11,7 @@ import ( "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" + "github.com/alcionai/corso/src/internal/common/idname" "github.com/alcionai/corso/src/internal/common/ptr" "github.com/alcionai/corso/src/internal/m365/graph" odConsts "github.com/alcionai/corso/src/internal/m365/onedrive/consts" @@ -827,7 +828,9 @@ func (m *mockPDAGRF) GetRootFolder( func (suite *RestoreUnitSuite) TestEnsureDriveExists() { rfID := "this-is-id" driveID := "another-id" + oldID := "old-id" name := "name" + otherName := "other name" rf := models.NewDriveItem() rf.SetId(&rfID) @@ -848,6 +851,12 @@ func (suite *RestoreUnitSuite) TestEnsureDriveExists() { Folders: path.Elements{}, } + oldDP := &path.DrivePath{ + DriveID: oldID, + Root: "root:", + Folders: path.Elements{}, + } + populatedCache := func(id string) *restoreCaches { rc := NewRestoreCaches(nil) di := driveInfo{ @@ -860,38 +869,91 @@ func (suite *RestoreUnitSuite) TestEnsureDriveExists() { return rc } + oldDriveIDNames := idname.NewCache(nil) + oldDriveIDNames.Add(oldID, name) + + idSwitchedCache := func() *restoreCaches { + rc := NewRestoreCaches(oldDriveIDNames) + di := driveInfo{ + id: "diff", + name: name, + } + rc.DriveIDToDriveInfo["diff"] = di + rc.DriveNameToDriveInfo[name] = di + + return rc + } + table := []struct { name string + dp *path.DrivePath mock *mockPDAGRF rc *restoreCaches expectErr require.ErrorAssertionFunc + fallbackName string expectName string + expectID string skipValueChecks bool }{ { name: "drive already in cache", + dp: dp, mock: &mockPDAGRF{ postResp: []models.Driveable{makeMD()}, postErr: []error{nil}, grf: grf, }, - rc: populatedCache(driveID), - expectErr: require.NoError, - expectName: name, + rc: populatedCache(driveID), + expectErr: require.NoError, + fallbackName: name, + expectName: name, + expectID: driveID, }, { - name: "drive created", + name: "drive with same name but different id exists", + dp: oldDP, mock: &mockPDAGRF{ postResp: []models.Driveable{makeMD()}, postErr: []error{nil}, grf: grf, }, - rc: NewRestoreCaches(nil), - expectErr: require.NoError, - expectName: name, + rc: idSwitchedCache(), + expectErr: require.NoError, + fallbackName: otherName, + expectName: name, + expectID: "diff", + }, + { + name: "drive created with old name", + dp: oldDP, + mock: &mockPDAGRF{ + postResp: []models.Driveable{makeMD()}, + postErr: []error{nil}, + grf: grf, + }, + rc: NewRestoreCaches(oldDriveIDNames), + expectErr: require.NoError, + fallbackName: otherName, + expectName: name, + expectID: driveID, + }, + { + name: "drive created with fallback name", + dp: dp, + mock: &mockPDAGRF{ + postResp: []models.Driveable{makeMD()}, + postErr: []error{nil}, + grf: grf, + }, + rc: NewRestoreCaches(nil), + expectErr: require.NoError, + fallbackName: otherName, + expectName: otherName, + expectID: driveID, }, { name: "error creating drive", + dp: dp, mock: &mockPDAGRF{ postResp: []models.Driveable{nil}, postErr: []error{assert.AnError}, @@ -899,41 +961,66 @@ func (suite *RestoreUnitSuite) TestEnsureDriveExists() { }, rc: NewRestoreCaches(nil), expectErr: require.Error, + fallbackName: name, expectName: "", skipValueChecks: true, + expectID: driveID, }, { name: "drive name already exists", + dp: dp, mock: &mockPDAGRF{ postResp: []models.Driveable{makeMD()}, postErr: []error{nil}, grf: grf, }, - rc: populatedCache("beaux"), - expectErr: require.NoError, - expectName: name + " 1", + rc: populatedCache("beaux"), + expectErr: require.NoError, + fallbackName: name, + expectName: name, + expectID: driveID, }, { name: "list with name already exists", + dp: dp, mock: &mockPDAGRF{ postResp: []models.Driveable{nil, makeMD()}, postErr: []error{graph.ErrItemAlreadyExistsConflict, nil}, grf: grf, }, - rc: NewRestoreCaches(nil), - expectErr: require.NoError, - expectName: name + " 1", + rc: NewRestoreCaches(nil), + expectErr: require.NoError, + fallbackName: name, + expectName: name + " 1", + expectID: driveID, + }, + { + name: "list with old name already exists", + dp: oldDP, + mock: &mockPDAGRF{ + postResp: []models.Driveable{nil, makeMD()}, + postErr: []error{graph.ErrItemAlreadyExistsConflict, nil}, + grf: grf, + }, + rc: NewRestoreCaches(oldDriveIDNames), + expectErr: require.NoError, + fallbackName: name, + expectName: name + " 1", + expectID: driveID, }, { name: "drive and list with name already exist", + dp: dp, mock: &mockPDAGRF{ postResp: []models.Driveable{nil, makeMD()}, postErr: []error{graph.ErrItemAlreadyExistsConflict, nil}, grf: grf, }, - rc: populatedCache("regard"), - expectErr: require.NoError, - expectName: name + " 2", + rc: populatedCache(driveID), + expectErr: require.NoError, + fallbackName: name, + expectName: name, + expectID: driveID, }, } for _, test := range table { @@ -945,16 +1032,19 @@ func (suite *RestoreUnitSuite) TestEnsureDriveExists() { rc := test.rc - err := ensureDriveExists( + di, err := ensureDriveExists( ctx, test.mock, rc, - dp, + test.dp, "prID", - name) + test.fallbackName) test.expectErr(t, err, clues.ToCore(err)) if !test.skipValueChecks { + assert.Equal(t, test.expectName, di.name, "ensured drive has expected name") + assert.Equal(t, test.expectID, di.id, "ensured drive has expected id") + nameResult := rc.DriveNameToDriveInfo[test.expectName] assert.Equal(t, test.expectName, nameResult.name, "found drive entry with expected name") }