larryh/corso
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

handle file-folder collision cases (#3688)

Browse Source 
handle onedrive item collision cases where a file to be restored is in conflict with an existing folder. Skip and Copy will function normally.  For Replace behavior, we'll defer to Copy in this situation, so that the file gets restored without changing or deleting the folder.  If a folder creation attempts to replace an item, we do a similar action and make a renamed copy of the folder instead.

---

#### Does this PR need a docs update or release note?

- [x] ⛔ No

#### Type of change

- [x] 🧹 Tech Debt/Cleanup

#### Issue(s)

* #3562

#### Test Plan

- [x] ⚡ Unit test
- [x] 💚 E2E
This commit is contained in:
Keepers 2023-07-03 20:34:53 -06:00 committed by GitHub
parent 411be17b17
commit 662d626809
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 325 additions and 63 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
src/internal/m365/onedrive/handlers.go
View File

@ -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 {

2
src/internal/m365/onedrive/item_handler.go
View File

@ -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

4
src/internal/m365/onedrive/mock/handlers.go
View File

@ -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
}

112
src/internal/m365/onedrive/restore.go
View File

@ -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

143
src/internal/m365/onedrive/restore_test.go
View File

@ -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) {
@ -365,7 +367,9 @@ func (suite *RestoreUnitSuite) TestRestoreItem_collisionHandling() {
},
{
name: "collision, copy",
collisionKeys: map[string]string{mock.DriveItemFileName: mndiID},
collisionKeys: map[string]api.DriveCollisionItem{
mock.DriveItemFileName: {ItemID: mndiID},
},
onCollision: control.Copy,
expectSkipped: assert.False,
expectMock: func(t *testing.T, rh *mock.RestoreHandler) {
@ -375,7 +379,9 @@ func (suite *RestoreUnitSuite) TestRestoreItem_collisionHandling() {
},
{
name: "collision, replace",
collisionKeys: map[string]string{mock.DriveItemFileName: mndiID},
collisionKeys: map[string]api.DriveCollisionItem{
mock.DriveItemFileName: {ItemID: mndiID},
},
onCollision: control.Replace,
expectSkipped: assert.False,
expectMock: func(t *testing.T, rh *mock.RestoreHandler) {
@ -386,7 +392,54 @@ func (suite *RestoreUnitSuite) TestRestoreItem_collisionHandling() {
},
{
name: "collision, skip",
collisionKeys: map[string]string{mock.DriveItemFileName: mndiID},
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)
})
}
}

2
src/internal/m365/sharepoint/library_handler.go
View File

@ -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

14
src/pkg/services/m365/api/drive_pager.go
View File

@ -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

6
src/pkg/services/m365/api/drive_pager_test.go
View File

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

97
src/pkg/services/m365/api/drive_test.go
View File

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