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

@@ -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 `<restoreContainerName>/<original folder path>`
@@ -1215,51 +1221,63 @@ func ensureDriveExists(
 	pdagrf PostDriveAndGetRootFolderer,
 	caches *restoreCaches,
 	drivePath *path.DrivePath,
-	protectedResourceID, driveName string,
+	protectedResourceID, fallbackDriveName string,
-) error {
+) (driveInfo, error) {
 	driveID := drivePath.DriveID
 
-	// the drive might already be cached
+	// the drive might already be cached by ID.  it's okay
-	if _, ok := caches.DriveIDToDriveInfo[driveID]; ok {
+	// if the name has changed.  the ID is a better reference
-		return nil
+	// 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 {
+	// if the drive wasn't found by ID, maybe we can find a
-		newDriveName = fmt.Sprintf("%s %d", driveName, 1)
+	// 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
+	nextDriveName := newDriveName
-	// 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
-	//
 	// 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
+	// names with document libraries (they're the same type, actually).
-	// to compare the names of drives; we also need to continue this
+	// In those cases we need to rename the drive until we can create
-	// loop until we can create a drive without error.
+	// one without a collision.
-	for i := 2; ; i++ {
+	for i := 1; ; i++ {
-		ictx := clues.Add(ctx, "new_drive_name", clues.Hide(newDriveName))
+		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
 }

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),
+			rc:           populatedCache(driveID),
-			expectErr:  require.NoError,
+			expectErr:    require.NoError,
-			expectName: name,
+			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),
+			rc:           idSwitchedCache(),
-			expectErr:  require.NoError,
+			expectErr:    require.NoError,
-			expectName: name,
+			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"),
+			rc:           populatedCache("beaux"),
-			expectErr:  require.NoError,
+			expectErr:    require.NoError,
-			expectName: name + " 1",
+			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),
+			rc:           NewRestoreCaches(nil),
-			expectErr:  require.NoError,
+			expectErr:    require.NoError,
-			expectName: name + " 1",
+			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"),
+			rc:           populatedCache(driveID),
-			expectErr:  require.NoError,
+			expectErr:    require.NoError,
-			expectName: name + " 2",
+			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")
 			}