diff --git a/src/internal/m365/onedrive/handlers.go b/src/internal/m365/onedrive/handlers.go index a2b539025..0bdd1edfa 100644 --- a/src/internal/m365/onedrive/handlers.go +++ b/src/internal/m365/onedrive/handlers.go @@ -117,7 +117,7 @@ type GetItemsByCollisionKeyser interface { GetItemsInContainerByCollisionKey( ctx context.Context, driveID, containerID string, - ) (map[string]string, error) + ) (map[string]api.DriveCollisionItem, error) } type NewItemContentUploader interface { diff --git a/src/internal/m365/onedrive/item_handler.go b/src/internal/m365/onedrive/item_handler.go index c3011aa46..69ce9fe59 100644 --- a/src/internal/m365/onedrive/item_handler.go +++ b/src/internal/m365/onedrive/item_handler.go @@ -164,7 +164,7 @@ func (h itemRestoreHandler) DeleteItemPermission( func (h itemRestoreHandler) GetItemsInContainerByCollisionKey( ctx context.Context, driveID, containerID string, -) (map[string]string, error) { +) (map[string]api.DriveCollisionItem, error) { m, err := h.ac.GetItemsInContainerByCollisionKey(ctx, driveID, containerID) if err != nil { return nil, err diff --git a/src/internal/m365/onedrive/mock/handlers.go b/src/internal/m365/onedrive/mock/handlers.go index 709e98a03..8de3bf78b 100644 --- a/src/internal/m365/onedrive/mock/handlers.go +++ b/src/internal/m365/onedrive/mock/handlers.go @@ -233,7 +233,7 @@ func (m GetsItemPermission) GetItemPermission( type RestoreHandler struct { ItemInfo details.ItemInfo - CollisionKeyMap map[string]string + CollisionKeyMap map[string]api.DriveCollisionItem CalledDeleteItem bool CalledDeleteItemOn string @@ -258,7 +258,7 @@ func (h *RestoreHandler) AugmentItemInfo( func (h *RestoreHandler) GetItemsInContainerByCollisionKey( context.Context, string, string, -) (map[string]string, error) { +) (map[string]api.DriveCollisionItem, error) { return h.CollisionKeyMap, nil } diff --git a/src/internal/m365/onedrive/restore.go b/src/internal/m365/onedrive/restore.go index 932eb9384..a2d829cb1 100644 --- a/src/internal/m365/onedrive/restore.go +++ b/src/internal/m365/onedrive/restore.go @@ -12,6 +12,7 @@ import ( "sync/atomic" "github.com/alcionai/clues" + "github.com/microsoftgraph/msgraph-sdk-go/models" "github.com/pkg/errors" "github.com/alcionai/corso/src/internal/common/ptr" @@ -36,7 +37,7 @@ const ( ) type restoreCaches struct { - collisionKeyToItemID map[string]string + collisionKeyToItemID map[string]api.DriveCollisionItem DriveIDToRootFolderID map[string]string Folders *folderCache OldLinkShareIDToNewID map[string]string @@ -48,7 +49,7 @@ type restoreCaches struct { func NewRestoreCaches() *restoreCaches { return &restoreCaches{ - collisionKeyToItemID: map[string]string{}, + collisionKeyToItemID: map[string]api.DriveCollisionItem{}, DriveIDToRootFolderID: map[string]string{}, Folders: NewFolderCache(), OldLinkShareIDToNewID: map[string]string{}, @@ -468,10 +469,10 @@ func restoreV0File( fibn data.FetchItemByNamer, restoreFolderID string, copyBuffer []byte, - collisionKeyToItemID map[string]string, + collisionKeyToItemID map[string]api.DriveCollisionItem, itemData data.Stream, ) (details.ItemInfo, error) { - _, itemInfo, err := restoreData( + _, itemInfo, err := restoreFile( ctx, restoreCfg, rh, @@ -504,7 +505,7 @@ func restoreV1File( ) (details.ItemInfo, error) { trimmedName := strings.TrimSuffix(itemData.UUID(), metadata.DataFileSuffix) - itemID, itemInfo, err := restoreData( + itemID, itemInfo, err := restoreFile( ctx, restoreCfg, rh, @@ -587,7 +588,7 @@ func restoreV6File( return details.ItemInfo{}, clues.New("item with empty name") } - itemID, itemInfo, err := restoreData( + itemID, itemInfo, err := restoreFile( ctx, restoreCfg, rh, @@ -696,11 +697,11 @@ func createRestoreFolders( "drive_id", drivePath.DriveID, "root_folder_id", parentFolderID) - for _, folder := range folders { - location = location.Append(folder) + for _, folderName := range folders { + location = location.Append(folderName) ictx := clues.Add( ctx, - "creating_restore_folder", folder, + "creating_restore_folder", folderName, "restore_folder_location", location, "parent_of_restore_folder", parentFolderID) @@ -710,30 +711,15 @@ func createRestoreFolders( continue } - folderItem, err := fr.GetFolderByName(ictx, driveID, parentFolderID, folder) - if err != nil && !errors.Is(err, api.ErrFolderNotFound) { - return "", clues.Wrap(err, "getting folder by display name") - } - - // folder found, moving to next child - if err == nil { - parentFolderID = ptr.Val(folderItem.GetId()) - caches.Folders.set(location, folderItem) - - continue - } - - // create the folder if not found - // the Replace collision policy is used since collisions on that - // policy will no-op and return the existing folder. This has two - // benefits: first, we get to treat the post as idempotent; and - // second, we don't have to worry about race conditions. - folderItem, err = fr.PostItemInContainer( + // we assume this folder creation always uses the Replace + // conflict policy, which means it will act as a GET if the + // folder already exists. + folderItem, err := createFolder( ictx, + fr, driveID, parentFolderID, - newItem(folder, true), - control.Replace) + folderName) if err != nil { return "", clues.Wrap(err, "creating folder") } @@ -747,6 +733,50 @@ func createRestoreFolders( return parentFolderID, nil } +func createFolder( + ctx context.Context, + piic PostItemInContainerer, + driveID, parentFolderID, folderName string, +) (models.DriveItemable, error) { + // create the folder if not found + // the Replace collision policy is used since collisions on that + // policy will no-op and return the existing folder. This has two + // benefits: first, we get to treat the post as idempotent; and + // second, we don't have to worry about race conditions. + item, err := piic.PostItemInContainer( + ctx, + driveID, + parentFolderID, + newItem(folderName, true), + control.Replace) + + // ErrItemAlreadyExistsConflict can only occur for folders if the + // item being replaced is a file, not another folder. + if err != nil && !errors.Is(err, graph.ErrItemAlreadyExistsConflict) { + return nil, clues.Wrap(err, "creating folder") + } + + if err == nil { + return item, err + } + + // if we made it here, then we tried to replace a file with a folder and + // hit a conflict. An unlikely occurrence, and we can try again with a copy + // conflict behavior setting and probably succeed, though that will change + // the location name of the restore. + item, err = piic.PostItemInContainer( + ctx, + driveID, + parentFolderID, + newItem(folderName, true), + control.Copy) + if err != nil { + return nil, clues.Wrap(err, "creating folder") + } + + return item, err +} + type itemRestorer interface { DeleteItemer ItemInfoAugmenter @@ -754,8 +784,8 @@ type itemRestorer interface { PostItemInContainerer } -// restoreData will create a new item in the specified `parentFolderID` and upload the data.Stream -func restoreData( +// restoreFile will create a new item in the specified `parentFolderID` and upload the data.Stream +func restoreFile( ctx context.Context, restoreCfg control.RestoreConfig, ir itemRestorer, @@ -763,7 +793,7 @@ func restoreData( name string, itemData data.Stream, driveID, parentFolderID string, - collisionKeyToItemID map[string]string, + collisionKeyToItemID map[string]api.DriveCollisionItem, copyBuffer []byte, ) (string, details.ItemInfo, error) { ctx, end := diagnostics.Span(ctx, "gc:oneDrive:restoreItem", diagnostics.Label("item_uuid", itemData.UUID())) @@ -780,11 +810,11 @@ func restoreData( var ( item = newItem(name, false) collisionKey = api.DriveItemCollisionKey(item) - collisionID string + collision api.DriveCollisionItem replace bool ) - if id, ok := collisionKeyToItemID[collisionKey]; ok { + if dci, ok := collisionKeyToItemID[collisionKey]; ok { log := logger.Ctx(ctx).With("collision_key", clues.Hide(collisionKey)) log.Debug("item collision") @@ -793,8 +823,8 @@ func restoreData( return "", details.ItemInfo{}, graph.ErrItemAlreadyExistsConflict } - collisionID = id - replace = restoreCfg.OnCollision == control.Replace + collision = dci + replace = restoreCfg.OnCollision == control.Replace && !dci.IsFolder } // drive items do not support PUT requests on the drive item data, so @@ -805,7 +835,7 @@ func restoreData( // 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, collisionID); err != nil { + if err := ir.DeleteItem(ctx, driveID, collision.ItemID); err != nil { return "", details.ItemInfo{}, clues.New("deleting colliding item") } } @@ -820,6 +850,12 @@ func restoreData( driveID, parentFolderID, item, + // notes on forced copy: + // 1. happy path: any non-colliding item will restore as if no collision had occurred + // 2. if a file-container collision is present, we assume the item being restored + // will get generated according to server-side copy rules. + // 3. if restoreCfg specifies replace and a file-container collision is present, we + // make no changes to the original file, and do not delete it. control.Copy) if err != nil { return "", details.ItemInfo{}, err diff --git a/src/internal/m365/onedrive/restore_test.go b/src/internal/m365/onedrive/restore_test.go index 1fc7c5d59..69dee3045 100644 --- a/src/internal/m365/onedrive/restore_test.go +++ b/src/internal/m365/onedrive/restore_test.go @@ -1,6 +1,7 @@ package onedrive import ( + "context" "testing" "github.com/alcionai/clues" @@ -18,6 +19,7 @@ import ( "github.com/alcionai/corso/src/internal/version" "github.com/alcionai/corso/src/pkg/control" "github.com/alcionai/corso/src/pkg/path" + "github.com/alcionai/corso/src/pkg/services/m365/api" ) type RestoreUnitSuite struct { @@ -328,14 +330,14 @@ func (suite *RestoreUnitSuite) TestRestoreItem_collisionHandling() { table := []struct { name string - collisionKeys map[string]string + collisionKeys map[string]api.DriveCollisionItem onCollision control.CollisionPolicy expectSkipped assert.BoolAssertionFunc expectMock func(*testing.T, *mock.RestoreHandler) }{ { name: "no collision, copy", - collisionKeys: map[string]string{}, + collisionKeys: map[string]api.DriveCollisionItem{}, onCollision: control.Copy, expectSkipped: assert.False, expectMock: func(t *testing.T, rh *mock.RestoreHandler) { @@ -345,7 +347,7 @@ func (suite *RestoreUnitSuite) TestRestoreItem_collisionHandling() { }, { name: "no collision, replace", - collisionKeys: map[string]string{}, + collisionKeys: map[string]api.DriveCollisionItem{}, onCollision: control.Replace, expectSkipped: assert.False, expectMock: func(t *testing.T, rh *mock.RestoreHandler) { @@ -355,7 +357,7 @@ func (suite *RestoreUnitSuite) TestRestoreItem_collisionHandling() { }, { name: "no collision, skip", - collisionKeys: map[string]string{}, + collisionKeys: map[string]api.DriveCollisionItem{}, onCollision: control.Skip, expectSkipped: assert.False, expectMock: func(t *testing.T, rh *mock.RestoreHandler) { @@ -364,8 +366,10 @@ func (suite *RestoreUnitSuite) TestRestoreItem_collisionHandling() { }, }, { - name: "collision, copy", - collisionKeys: map[string]string{mock.DriveItemFileName: mndiID}, + name: "collision, copy", + collisionKeys: map[string]api.DriveCollisionItem{ + mock.DriveItemFileName: {ItemID: mndiID}, + }, onCollision: control.Copy, expectSkipped: assert.False, expectMock: func(t *testing.T, rh *mock.RestoreHandler) { @@ -374,8 +378,10 @@ func (suite *RestoreUnitSuite) TestRestoreItem_collisionHandling() { }, }, { - name: "collision, replace", - collisionKeys: map[string]string{mock.DriveItemFileName: mndiID}, + name: "collision, replace", + collisionKeys: map[string]api.DriveCollisionItem{ + mock.DriveItemFileName: {ItemID: mndiID}, + }, onCollision: control.Replace, expectSkipped: assert.False, expectMock: func(t *testing.T, rh *mock.RestoreHandler) { @@ -385,8 +391,55 @@ func (suite *RestoreUnitSuite) TestRestoreItem_collisionHandling() { }, }, { - name: "collision, skip", - collisionKeys: map[string]string{mock.DriveItemFileName: mndiID}, + name: "collision, skip", + collisionKeys: map[string]api.DriveCollisionItem{ + mock.DriveItemFileName: {ItemID: mndiID}, + }, + onCollision: control.Skip, + expectSkipped: assert.True, + expectMock: func(t *testing.T, rh *mock.RestoreHandler) { + assert.False(t, rh.CalledPostItem, "new item posted") + assert.False(t, rh.CalledDeleteItem, "new item deleted") + }, + }, + { + name: "file-folder collision, copy", + collisionKeys: map[string]api.DriveCollisionItem{ + mock.DriveItemFileName: { + ItemID: mndiID, + IsFolder: true, + }, + }, + onCollision: control.Copy, + expectSkipped: assert.False, + expectMock: func(t *testing.T, rh *mock.RestoreHandler) { + assert.True(t, rh.CalledPostItem, "new item posted") + assert.False(t, rh.CalledDeleteItem, "new item deleted") + }, + }, + { + name: "file-folder collision, replace", + collisionKeys: map[string]api.DriveCollisionItem{ + mock.DriveItemFileName: { + ItemID: mndiID, + IsFolder: true, + }, + }, + onCollision: control.Replace, + expectSkipped: assert.False, + expectMock: func(t *testing.T, rh *mock.RestoreHandler) { + assert.True(t, rh.CalledPostItem, "new item posted") + assert.False(t, rh.CalledDeleteItem, "new item deleted") + }, + }, + { + name: "file-folder collision, skip", + collisionKeys: map[string]api.DriveCollisionItem{ + mock.DriveItemFileName: { + ItemID: mndiID, + IsFolder: true, + }, + }, onCollision: control.Skip, expectSkipped: assert.True, expectMock: func(t *testing.T, rh *mock.RestoreHandler) { @@ -446,3 +499,79 @@ func (suite *RestoreUnitSuite) TestRestoreItem_collisionHandling() { }) } } + +type mockPIIC struct { + i int + errs []error + items []models.DriveItemable +} + +func (m *mockPIIC) PostItemInContainer( + context.Context, + string, string, + models.DriveItemable, + control.CollisionPolicy, +) (models.DriveItemable, error) { + j := m.i + m.i++ + + return m.items[j], m.errs[j] +} + +func (suite *RestoreUnitSuite) TestCreateFolder() { + table := []struct { + name string + mock *mockPIIC + expectErr assert.ErrorAssertionFunc + expectItem assert.ValueAssertionFunc + }{ + { + name: "good", + mock: &mockPIIC{ + errs: []error{nil}, + items: []models.DriveItemable{models.NewDriveItem()}, + }, + expectErr: assert.NoError, + expectItem: assert.NotNil, + }, + { + name: "good with copy", + mock: &mockPIIC{ + errs: []error{graph.ErrItemAlreadyExistsConflict, nil}, + items: []models.DriveItemable{nil, models.NewDriveItem()}, + }, + expectErr: assert.NoError, + expectItem: assert.NotNil, + }, + { + name: "bad", + mock: &mockPIIC{ + errs: []error{assert.AnError}, + items: []models.DriveItemable{nil}, + }, + expectErr: assert.Error, + expectItem: assert.Nil, + }, + { + name: "bad with copy", + mock: &mockPIIC{ + errs: []error{graph.ErrItemAlreadyExistsConflict, assert.AnError}, + items: []models.DriveItemable{nil, nil}, + }, + expectErr: assert.Error, + expectItem: assert.Nil, + }, + } + for _, test := range table { + suite.Run(test.name, func() { + t := suite.T() + + ctx, flush := tester.NewContext(t) + defer flush() + + result, err := createFolder(ctx, test.mock, "d", "pf", "fn") + test.expectErr(t, err, clues.ToCore(err)) + test.expectItem(t, result) + }) + } +} diff --git a/src/internal/m365/sharepoint/library_handler.go b/src/internal/m365/sharepoint/library_handler.go index 2c359d7c1..3bfed48fc 100644 --- a/src/internal/m365/sharepoint/library_handler.go +++ b/src/internal/m365/sharepoint/library_handler.go @@ -190,7 +190,7 @@ func (h libraryRestoreHandler) DeleteItemPermission( func (h libraryRestoreHandler) GetItemsInContainerByCollisionKey( ctx context.Context, driveID, containerID string, -) (map[string]string, error) { +) (map[string]api.DriveCollisionItem, error) { m, err := h.ac.GetItemsInContainerByCollisionKey(ctx, driveID, containerID) if err != nil { return nil, err diff --git a/src/pkg/services/m365/api/drive_pager.go b/src/pkg/services/m365/api/drive_pager.go index e1478cfd1..0b073571b 100644 --- a/src/pkg/services/m365/api/drive_pager.go +++ b/src/pkg/services/m365/api/drive_pager.go @@ -67,10 +67,15 @@ func (p *driveItemPageCtrl) setNext(nextLink string) { p.builder = drives.NewItemItemsItemChildrenRequestBuilder(nextLink, p.gs.Adapter()) } +type DriveCollisionItem struct { + ItemID string + IsFolder bool +} + func (c Drives) GetItemsInContainerByCollisionKey( ctx context.Context, driveID, containerID string, -) (map[string]string, error) { +) (map[string]DriveCollisionItem, error) { ctx = clues.Add(ctx, "container_id", containerID) pager := c.NewDriveItemPager(driveID, containerID, idAnd("name")...) @@ -79,10 +84,13 @@ func (c Drives) GetItemsInContainerByCollisionKey( return nil, graph.Wrap(ctx, err, "enumerating drive items") } - m := map[string]string{} + m := map[string]DriveCollisionItem{} for _, item := range items { - m[DriveItemCollisionKey(item)] = ptr.Val(item.GetId()) + m[DriveItemCollisionKey(item)] = DriveCollisionItem{ + ItemID: ptr.Val(item.GetId()), + IsFolder: item.GetFolder() != nil, + } } return m, nil diff --git a/src/pkg/services/m365/api/drive_pager_test.go b/src/pkg/services/m365/api/drive_pager_test.go index e44968ab9..ccf581188 100644 --- a/src/pkg/services/m365/api/drive_pager_test.go +++ b/src/pkg/services/m365/api/drive_pager_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/suite" "github.com/alcionai/corso/src/internal/tester" + "github.com/alcionai/corso/src/pkg/services/m365/api" ) type DrivePagerIntgSuite struct { @@ -63,7 +64,7 @@ func (suite *DrivePagerIntgSuite) TestDrives_GetItemsInContainerByCollisionKey() require.NoError(t, err, clues.ToCore(err)) ims := items.GetValue() - expect := make([]string, 0, len(ims)) + expect := make([]api.DriveCollisionItem, 0, len(ims)) assert.NotEmptyf( t, @@ -81,8 +82,9 @@ func (suite *DrivePagerIntgSuite) TestDrives_GetItemsInContainerByCollisionKey() } for _, e := range expect { - _, ok := results[e] + r, ok := results[e.ItemID] assert.Truef(t, ok, "expected results to contain collision key: %s", e) + assert.Equal(t, e, r) } }) } diff --git a/src/pkg/services/m365/api/drive_test.go b/src/pkg/services/m365/api/drive_test.go index 397d997c1..f25801c9c 100644 --- a/src/pkg/services/m365/api/drive_test.go +++ b/src/pkg/services/m365/api/drive_test.go @@ -1,6 +1,7 @@ package api_test import ( + "fmt" "testing" "github.com/alcionai/clues" @@ -8,6 +9,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" + "golang.org/x/exp/slices" "github.com/alcionai/corso/src/internal/common/ptr" "github.com/alcionai/corso/src/internal/m365/graph" @@ -16,24 +18,24 @@ import ( "github.com/alcionai/corso/src/pkg/control/testdata" ) -type DriveAPISuite struct { +type DriveAPIIntgSuite struct { tester.Suite its intgTesterSetup } -func (suite *DriveAPISuite) SetupSuite() { +func (suite *DriveAPIIntgSuite) SetupSuite() { suite.its = newIntegrationTesterSetup(suite.T()) } func TestDriveAPIs(t *testing.T) { - suite.Run(t, &DriveAPISuite{ + suite.Run(t, &DriveAPIIntgSuite{ Suite: tester.NewIntegrationSuite( t, [][]string{tester.M365AcctCredEnvs}), }) } -func (suite *DriveAPISuite) TestDrives_CreatePagerAndGetPage() { +func (suite *DriveAPIIntgSuite) TestDrives_CreatePagerAndGetPage() { t := suite.T() ctx, flush := tester.NewContext(t) @@ -61,7 +63,7 @@ func newItem(name string, folder bool) *models.DriveItem { return itemToCreate } -func (suite *DriveAPISuite) TestDrives_PostItemInContainer() { +func (suite *DriveAPIIntgSuite) TestDrives_PostItemInContainer() { t := suite.T() ctx, flush := tester.NewContext(t) @@ -218,3 +220,88 @@ func (suite *DriveAPISuite) TestDrives_PostItemInContainer() { }) } } + +// purpose: ensure that creating a new folder with "replace" conflict behavior +// makes no changes to the items which exist in that folder. +func (suite *DriveAPIIntgSuite) TestDrives_PostItemInContainer_replaceFolderRegression() { + t := suite.T() + + ctx, flush := tester.NewContext(t) + defer flush() + + var ( + rc = testdata.DefaultRestoreConfig("drive_folder_replace_regression") + acd = suite.its.ac.Drives() + files = make([]models.DriveItemable, 0, 5) + ) + + // generate a folder for the test data + folder, err := acd.PostItemInContainer( + ctx, + suite.its.userDriveID, + suite.its.userDriveRootFolderID, + newItem(rc.Location, true), + // skip instead of replace here to get + // an ErrItemAlreadyExistsConflict, just in case. + control.Skip) + require.NoError(t, err, clues.ToCore(err)) + + // generate items within that folder + for i := 0; i < 5; i++ { + file := newItem(fmt.Sprintf("collision_%d.txt", i), false) + f, err := acd.PostItemInContainer( + ctx, + suite.its.userDriveID, + ptr.Val(folder.GetId()), + file, + control.Copy) + require.NoError(t, err, clues.ToCore(err)) + + files = append(files, f) + } + + resultFolder, err := acd.PostItemInContainer( + ctx, + suite.its.userDriveID, + ptr.Val(folder.GetParentReference().GetId()), + newItem(rc.Location, true), + control.Replace) + require.NoError(t, err, clues.ToCore(err)) + require.NotEmpty(t, ptr.Val(resultFolder.GetId())) + require.Equal(t, ptr.Val(folder.GetId()), ptr.Val(resultFolder.GetId())) + + resultFileColl, err := acd.Stable. + Client(). + Drives(). + ByDriveId(suite.its.userDriveID). + Items(). + ByDriveItemId(ptr.Val(resultFolder.GetId())). + Children(). + Get(ctx, nil) + err = graph.Stack(ctx, err).OrNil() + require.NoError(t, err, clues.ToCore(err)) + + resultFiles := resultFileColl.GetValue() + + // asserting that no file changes have occurred as a result of the + // "replacement" of the owning folder. + for _, rf := range resultFiles { + var ( + rID = ptr.Val(rf.GetId()) + rName = ptr.Val(rf.GetName()) + rMod = ptr.Val(rf.GetLastModifiedDateTime()) + ) + + check := func(expect models.DriveItemable) bool { + var ( + eID = ptr.Val(expect.GetId()) + eName = ptr.Val(expect.GetName()) + eMod = ptr.Val(expect.GetLastModifiedDateTime()) + ) + + return eID == rID && eName == rName && eMod.Equal(rMod) + } + + assert.True(t, slices.ContainsFunc(files, check)) + } +}