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