From 416383a99c77583dbba5e10691fc598b69f2921f Mon Sep 17 00:00:00 2001 From: Keepers Date: Thu, 15 Jun 2023 18:07:09 -0600 Subject: [PATCH] cascade restoreCfg collision policy into onedrive (#3623) Adds collision policy handling to onedrive item posts. This allows us to override the default "replace" behavior that currently returns a 409 for the creation endpoint, in case we want to use skip (ie: fail) or copy handling. --- #### Does this PR need a docs update or release note? - [x] :no_entry: No #### Type of change - [x] :sunflower: Feature #### Issue(s) * #3562 #### Test Plan - [x] :muscle: Manual - [x] :zap: Unit test - [x] :green_heart: E2E --- .../m365/exchange/mock/collections.go | 1 + src/internal/m365/graph/errors.go | 28 ++- src/internal/m365/onedrive/handlers.go | 2 + src/internal/m365/onedrive/item_handler.go | 4 +- src/internal/m365/onedrive/item_test.go | 7 +- src/internal/m365/onedrive/mock/handlers.go | 67 ++++++ src/internal/m365/onedrive/mock/item.go | 85 +++++++ src/internal/m365/onedrive/restore.go | 62 +++++- src/internal/m365/onedrive/restore_test.go | 76 +++++++ src/internal/m365/onedrive/url_cache_test.go | 7 +- src/internal/m365/restore.go | 1 + .../m365/sharepoint/library_handler.go | 4 +- src/internal/m365/sharepoint/restore.go | 2 +- .../operations/backup_integration_test.go | 3 +- src/pkg/services/m365/api/drive.go | 28 ++- src/pkg/services/m365/api/drive_test.go | 207 +++++++++++++++++- 16 files changed, 549 insertions(+), 35 deletions(-) diff --git a/src/internal/m365/exchange/mock/collections.go b/src/internal/m365/exchange/mock/collections.go index 36de3cfd1..0e601da3d 100644 --- a/src/internal/m365/exchange/mock/collections.go +++ b/src/internal/m365/exchange/mock/collections.go @@ -138,6 +138,7 @@ func (medc *DataCollection) Items( return res } +// TODO: move to data/mock for service-agnostic mocking // Data represents a single item retrieved from exchange type Data struct { ID string diff --git a/src/internal/m365/graph/errors.go b/src/internal/m365/graph/errors.go index 65150868b..cbc82080c 100644 --- a/src/internal/m365/graph/errors.go +++ b/src/internal/m365/graph/errors.go @@ -33,13 +33,16 @@ const ( itemNotFoundShort errorCode = "itemNotFound" mailboxNotEnabledForRESTAPI errorCode = "MailboxNotEnabledForRESTAPI" malwareDetected errorCode = "malwareDetected" - requestResourceNotFound errorCode = "Request_ResourceNotFound" - quotaExceeded errorCode = "ErrorQuotaExceeded" - resourceNotFound errorCode = "ResourceNotFound" - resyncRequired errorCode = "ResyncRequired" // alt: resyncRequired - syncFolderNotFound errorCode = "ErrorSyncFolderNotFound" - syncStateInvalid errorCode = "SyncStateInvalid" - syncStateNotFound errorCode = "SyncStateNotFound" + // nameAlreadyExists occurs when a request with + // @microsoft.graph.conflictBehavior=fail finds a conflicting file. + nameAlreadyExists errorCode = "nameAlreadyExists" + quotaExceeded errorCode = "ErrorQuotaExceeded" + requestResourceNotFound errorCode = "Request_ResourceNotFound" + resourceNotFound errorCode = "ResourceNotFound" + resyncRequired errorCode = "ResyncRequired" // alt: resyncRequired + syncFolderNotFound errorCode = "ErrorSyncFolderNotFound" + syncStateInvalid errorCode = "SyncStateInvalid" + syncStateNotFound errorCode = "SyncStateNotFound" // This error occurs when an attempt is made to create a folder that has // the same name as another folder in the same parent. Such duplicate folder // names are not allowed by graph. @@ -79,6 +82,12 @@ var ( // https://learn.microsoft.com/en-us/graph/errors#code-property ErrInvalidDelta = clues.New("invalid delta token") + // ErrItemAlreadyExistsConflict denotes that a post or put attempted to create + // an item which already exists by some unique identifier. The identifier is + // not always the id. For example, in onedrive, this error can be produced + // when filenames collide in a @microsoft.graph.conflictBehavior=fail request. + ErrItemAlreadyExistsConflict = clues.New("item already exists") + // ErrServiceNotEnabled identifies that a resource owner does not have // access to a given service. ErrServiceNotEnabled = clues.New("service is not enabled for that resource owner") @@ -162,6 +171,11 @@ func IsErrUnauthorized(err error) bool { return clues.HasLabel(err, LabelStatus(http.StatusUnauthorized)) } +func IsErrItemAlreadyExistsConflict(err error) bool { + return hasErrorCode(err, nameAlreadyExists) || + errors.Is(err, ErrItemAlreadyExistsConflict) +} + // LabelStatus transforms the provided statusCode into // a standard label that can be attached to a clues error // and later reviewed when checking error statuses. diff --git a/src/internal/m365/onedrive/handlers.go b/src/internal/m365/onedrive/handlers.go index 079bcd727..3090703d9 100644 --- a/src/internal/m365/onedrive/handlers.go +++ b/src/internal/m365/onedrive/handlers.go @@ -7,6 +7,7 @@ import ( "github.com/microsoftgraph/msgraph-sdk-go/models" "github.com/alcionai/corso/src/pkg/backup/details" + "github.com/alcionai/corso/src/pkg/control" "github.com/alcionai/corso/src/pkg/path" "github.com/alcionai/corso/src/pkg/services/m365/api" ) @@ -117,6 +118,7 @@ type PostItemInContainerer interface { ctx context.Context, driveID, parentFolderID string, newItem models.DriveItemable, + onCollision control.CollisionPolicy, ) (models.DriveItemable, error) } diff --git a/src/internal/m365/onedrive/item_handler.go b/src/internal/m365/onedrive/item_handler.go index 904a20bd6..a23e07c61 100644 --- a/src/internal/m365/onedrive/item_handler.go +++ b/src/internal/m365/onedrive/item_handler.go @@ -11,6 +11,7 @@ import ( "github.com/alcionai/corso/src/internal/common/ptr" odConsts "github.com/alcionai/corso/src/internal/m365/onedrive/consts" "github.com/alcionai/corso/src/pkg/backup/details" + "github.com/alcionai/corso/src/pkg/control" "github.com/alcionai/corso/src/pkg/path" "github.com/alcionai/corso/src/pkg/selectors" "github.com/alcionai/corso/src/pkg/services/m365/api" @@ -172,8 +173,9 @@ func (h itemRestoreHandler) PostItemInContainer( ctx context.Context, driveID, parentFolderID string, newItem models.DriveItemable, + onCollision control.CollisionPolicy, ) (models.DriveItemable, error) { - return h.ac.PostItemInContainer(ctx, driveID, parentFolderID, newItem) + return h.ac.PostItemInContainer(ctx, driveID, parentFolderID, newItem, onCollision) } func (h itemRestoreHandler) GetFolderByName( diff --git a/src/internal/m365/onedrive/item_test.go b/src/internal/m365/onedrive/item_test.go index d862e6edf..44b3005db 100644 --- a/src/internal/m365/onedrive/item_test.go +++ b/src/internal/m365/onedrive/item_test.go @@ -15,6 +15,7 @@ import ( "github.com/alcionai/corso/src/internal/common/dttm" "github.com/alcionai/corso/src/internal/common/ptr" "github.com/alcionai/corso/src/internal/tester" + "github.com/alcionai/corso/src/pkg/control" "github.com/alcionai/corso/src/pkg/control/testdata" "github.com/alcionai/corso/src/pkg/fault" "github.com/alcionai/corso/src/pkg/selectors" @@ -167,7 +168,8 @@ func (suite *ItemIntegrationSuite) TestItemWriter() { ctx, test.driveID, ptr.Val(root.GetId()), - newItem(newFolderName, true)) + newItem(newFolderName, true), + control.Copy) require.NoError(t, err, clues.ToCore(err)) require.NotNil(t, newFolder.GetId()) @@ -178,7 +180,8 @@ func (suite *ItemIntegrationSuite) TestItemWriter() { ctx, test.driveID, ptr.Val(newFolder.GetId()), - newItem(newItemName, false)) + newItem(newItemName, false), + control.Copy) require.NoError(t, err, clues.ToCore(err)) require.NotNil(t, newItem.GetId()) diff --git a/src/internal/m365/onedrive/mock/handlers.go b/src/internal/m365/onedrive/mock/handlers.go index bafda5022..23ef8a4d5 100644 --- a/src/internal/m365/onedrive/mock/handlers.go +++ b/src/internal/m365/onedrive/mock/handlers.go @@ -5,10 +5,12 @@ import ( "net/http" "github.com/alcionai/clues" + "github.com/microsoftgraph/msgraph-sdk-go/drives" "github.com/microsoftgraph/msgraph-sdk-go/models" odConsts "github.com/alcionai/corso/src/internal/m365/onedrive/consts" "github.com/alcionai/corso/src/pkg/backup/details" + "github.com/alcionai/corso/src/pkg/control" "github.com/alcionai/corso/src/pkg/path" "github.com/alcionai/corso/src/pkg/services/m365/api" ) @@ -223,3 +225,68 @@ func (m GetsItemPermission) GetItemPermission( ) (models.PermissionCollectionResponseable, error) { return m.Perm, m.Err } + +// --------------------------------------------------------------------------- +// Restore Handler +// --------------------------------------------------------------------------- + +type RestoreHandler struct { + ItemInfo details.ItemInfo + + PostItemResp models.DriveItemable + PostItemErr error +} + +func (h RestoreHandler) AugmentItemInfo( + details.ItemInfo, + models.DriveItemable, + int64, + *path.Builder, +) details.ItemInfo { + return h.ItemInfo +} + +func (h RestoreHandler) NewItemContentUpload( + context.Context, + string, string, +) (models.UploadSessionable, error) { + return nil, clues.New("not implemented") +} + +func (h RestoreHandler) DeleteItemPermission( + context.Context, + string, string, string, +) error { + return clues.New("not implemented") +} + +func (h RestoreHandler) PostItemPermissionUpdate( + context.Context, + string, string, + *drives.ItemItemsItemInvitePostRequestBody, +) (drives.ItemItemsItemInviteResponseable, error) { + return nil, clues.New("not implemented") +} + +func (h RestoreHandler) PostItemInContainer( + context.Context, + string, string, + models.DriveItemable, + control.CollisionPolicy, +) (models.DriveItemable, error) { + return h.PostItemResp, h.PostItemErr +} + +func (h RestoreHandler) GetFolderByName( + context.Context, + string, string, string, +) (models.DriveItemable, error) { + return nil, clues.New("not implemented") +} + +func (h RestoreHandler) GetRootFolder( + context.Context, + string, +) (models.DriveItemable, error) { + return nil, clues.New("not implemented") +} diff --git a/src/internal/m365/onedrive/mock/item.go b/src/internal/m365/onedrive/mock/item.go index dcd86e11c..4e54f179c 100644 --- a/src/internal/m365/onedrive/mock/item.go +++ b/src/internal/m365/onedrive/mock/item.go @@ -1,5 +1,90 @@ package mock +import ( + "bytes" + "context" + "io" + "time" + + "github.com/alcionai/corso/src/internal/data" + "github.com/alcionai/corso/src/pkg/backup/details" +) + +// --------------------------------------------------------------------------- +// data.Stream +// --------------------------------------------------------------------------- + +var _ data.Stream = &Data{} + +// TODO: move to data/mock for service-agnostic mocking +// Data represents a single item retrieved from, or restored to, onedrive +type Data struct { + ID string + Reader io.ReadCloser + ReadErr error + size int64 + modifiedTime time.Time + deleted bool +} + +func (d *Data) UUID() string { return d.ID } +func (d *Data) Deleted() bool { return d.deleted } +func (d *Data) Size() int64 { return d.size } +func (d *Data) ModTime() time.Time { return d.modifiedTime } + +func (d *Data) ToReader() io.ReadCloser { + if d.ReadErr != nil { + return io.NopCloser(errReader{d.ReadErr}) + } + + return d.Reader +} + +func (d *Data) Info() details.ItemInfo { + return details.ItemInfo{ + OneDrive: &details.OneDriveInfo{ + ItemType: details.OneDriveItem, + ItemName: "test.txt", + Size: 1, + }, + } +} + +type errReader struct { + readErr error +} + +func (er errReader) Read([]byte) (int, error) { + return 0, er.readErr +} + +// --------------------------------------------------------------------------- +// FetchItemByNamer +// --------------------------------------------------------------------------- + +var _ data.FetchItemByNamer = &FetchItemByName{} + +type FetchItemByName struct { + Item data.Stream + Err error +} + +func (f FetchItemByName) FetchItemByName(context.Context, string) (data.Stream, error) { + return f.Item, f.Err +} + +// --------------------------------------------------------------------------- +// stub payload +// --------------------------------------------------------------------------- + +func FileRespReadCloser(pl string) io.ReadCloser { + return io.NopCloser(bytes.NewReader([]byte(pl))) +} + +const DriveFileMetaData = `{ + "fileName": "fnords.txt" +}` + //nolint:lll const DriveFilePayloadData = `{ "@odata.context": "https://graph.microsoft.com/v1.0/$metadata#drives('b%22-8wC6Jt04EWvKr1fQUDOyw5Gk8jIUJdEjzqonlSRf48i67LJdwopT4-6kiycJ5AV')/items/$entity", diff --git a/src/internal/m365/onedrive/restore.go b/src/internal/m365/onedrive/restore.go index da3f83d92..0edbaf62a 100644 --- a/src/internal/m365/onedrive/restore.go +++ b/src/internal/m365/onedrive/restore.go @@ -76,10 +76,7 @@ func ConsumeRestoreCollections( el = errs.Local() ) - ctx = clues.Add( - ctx, - "backup_version", backupVersion, - "restore_location", restoreCfg.Location) + ctx = clues.Add(ctx, "backup_version", backupVersion) // Reorder collections so that the parents directories are created // before the child directories; a requirement for permissions. @@ -97,7 +94,6 @@ func ConsumeRestoreCollections( ictx = clues.Add( ctx, "category", dc.FullPath().Category(), - "destination", clues.Hide(restoreCfg.Location), "resource_owner", clues.Hide(dc.FullPath().ResourceOwner()), "full_path", dc.FullPath()) ) @@ -105,10 +101,10 @@ func ConsumeRestoreCollections( metrics, err = RestoreCollection( ictx, rh, + restoreCfg, backupVersion, dc, caches, - restoreCfg.Location, deets, opts.RestorePermissions, errs) @@ -141,12 +137,12 @@ func ConsumeRestoreCollections( func RestoreCollection( ctx context.Context, rh RestoreHandler, + restoreCfg control.RestoreConfig, backupVersion int, dc data.RestoreCollection, caches *restoreCaches, - restoreContainerName string, deets *details.Builder, - restorePerms bool, + restorePerms bool, // TODD: move into restoreConfig errs *fault.Bus, ) (support.CollectionMetrics, error) { var ( @@ -181,7 +177,13 @@ func RestoreCollection( // from the backup under this the restore folder instead of root) // i.e. Restore into `/` // the drive into which this folder gets restored is tracked separately in drivePath. - restoreDir := path.Builder{}.Append(restoreContainerName).Append(drivePath.Folders...) + restoreDir := &path.Builder{} + + if len(restoreCfg.Location) > 0 { + restoreDir = restoreDir.Append(restoreCfg.Location) + } + + restoreDir = restoreDir.Append(drivePath.Folders...) ctx = clues.Add( ctx, @@ -280,6 +282,7 @@ func RestoreCollection( itemInfo, skipped, err := restoreItem( ictx, rh, + restoreCfg, dc, backupVersion, drivePath, @@ -328,6 +331,7 @@ func RestoreCollection( func restoreItem( ctx context.Context, rh RestoreHandler, + restoreCfg control.RestoreConfig, fibn data.FetchItemByNamer, backupVersion int, drivePath *path.DrivePath, @@ -345,12 +349,17 @@ func restoreItem( itemInfo, err := restoreV0File( ctx, rh, + restoreCfg, drivePath, fibn, restoreFolderID, copyBuffer, itemData) if err != nil { + if errors.Is(err, graph.ErrItemAlreadyExistsConflict) && restoreCfg.OnCollision == control.Skip { + return details.ItemInfo{}, true, nil + } + return details.ItemInfo{}, false, clues.Wrap(err, "v0 restore") } @@ -394,6 +403,7 @@ func restoreItem( itemInfo, err := restoreV1File( ctx, rh, + restoreCfg, drivePath, fibn, restoreFolderID, @@ -403,6 +413,10 @@ func restoreItem( itemPath, itemData) if err != nil { + if errors.Is(err, graph.ErrItemAlreadyExistsConflict) && restoreCfg.OnCollision == control.Skip { + return details.ItemInfo{}, true, nil + } + return details.ItemInfo{}, false, clues.Wrap(err, "v1 restore") } @@ -414,6 +428,7 @@ func restoreItem( itemInfo, err := restoreV6File( ctx, rh, + restoreCfg, drivePath, fibn, restoreFolderID, @@ -423,6 +438,10 @@ func restoreItem( itemPath, itemData) if err != nil { + if errors.Is(err, graph.ErrItemAlreadyExistsConflict) && restoreCfg.OnCollision == control.Skip { + return details.ItemInfo{}, true, nil + } + return details.ItemInfo{}, false, clues.Wrap(err, "v6 restore") } @@ -432,6 +451,7 @@ func restoreItem( func restoreV0File( ctx context.Context, rh RestoreHandler, + restoreCfg control.RestoreConfig, drivePath *path.DrivePath, fibn data.FetchItemByNamer, restoreFolderID string, @@ -440,6 +460,7 @@ func restoreV0File( ) (details.ItemInfo, error) { _, itemInfo, err := restoreData( ctx, + restoreCfg, rh, fibn, itemData.UUID(), @@ -457,6 +478,7 @@ func restoreV0File( func restoreV1File( ctx context.Context, rh RestoreHandler, + restoreCfg control.RestoreConfig, drivePath *path.DrivePath, fibn data.FetchItemByNamer, restoreFolderID string, @@ -470,6 +492,7 @@ func restoreV1File( itemID, itemInfo, err := restoreData( ctx, + restoreCfg, rh, fibn, trimmedName, @@ -513,6 +536,7 @@ func restoreV1File( func restoreV6File( ctx context.Context, rh RestoreHandler, + restoreCfg control.RestoreConfig, drivePath *path.DrivePath, fibn data.FetchItemByNamer, restoreFolderID string, @@ -550,6 +574,7 @@ func restoreV6File( itemID, itemInfo, err := restoreData( ctx, + restoreCfg, rh, fibn, meta.FileName, @@ -683,7 +708,16 @@ func createRestoreFolders( } // create the folder if not found - folderItem, err = fr.PostItemInContainer(ictx, driveID, parentFolderID, newItem(folder, true)) + // 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( + ictx, + driveID, + parentFolderID, + newItem(folder, true), + control.Replace) if err != nil { return "", clues.Wrap(err, "creating folder") } @@ -706,6 +740,7 @@ type itemRestorer interface { // restoreData will create a new item in the specified `parentFolderID` and upload the data.Stream func restoreData( ctx context.Context, + restoreCfg control.RestoreConfig, ir itemRestorer, fibn data.FetchItemByNamer, name string, @@ -725,7 +760,12 @@ func restoreData( } // Create Item - newItem, err := ir.PostItemInContainer(ctx, driveID, parentFolderID, newItem(name, false)) + newItem, err := ir.PostItemInContainer( + ctx, + driveID, + parentFolderID, + newItem(name, false), + restoreCfg.OnCollision) 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 0af13eccb..e88216721 100644 --- a/src/internal/m365/onedrive/restore_test.go +++ b/src/internal/m365/onedrive/restore_test.go @@ -4,12 +4,17 @@ import ( "testing" "github.com/alcionai/clues" + "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" + "github.com/alcionai/corso/src/internal/m365/graph" + odConsts "github.com/alcionai/corso/src/internal/m365/onedrive/consts" + "github.com/alcionai/corso/src/internal/m365/onedrive/mock" "github.com/alcionai/corso/src/internal/tester" "github.com/alcionai/corso/src/internal/version" + "github.com/alcionai/corso/src/pkg/control" "github.com/alcionai/corso/src/pkg/path" ) @@ -315,3 +320,74 @@ func (suite *RestoreUnitSuite) TestAugmentRestorePaths_DifferentRestorePath() { }) } } + +func (suite *RestoreUnitSuite) TestRestoreItem_errItemAlreadyExists() { + table := []struct { + name string + onCollision control.CollisionPolicy + expectErr func(*testing.T, error) + expectSkipped assert.BoolAssertionFunc + }{ + { + name: "skip", + onCollision: control.Skip, + expectErr: func(t *testing.T, err error) { + require.NoError(t, err, clues.ToCore(err)) + }, + expectSkipped: assert.True, + }, + { + name: "replace", + onCollision: control.Replace, + expectErr: func(t *testing.T, err error) { + require.ErrorIs(t, err, graph.ErrItemAlreadyExistsConflict, clues.ToCore(err)) + }, + expectSkipped: assert.False, + }, + } + for _, test := range table { + suite.Run(test.name, func() { + t := suite.T() + + ctx, flush := tester.NewContext(t) + defer flush() + + var ( + rh = mock.RestoreHandler{ + PostItemErr: graph.ErrItemAlreadyExistsConflict, + } + restoreCfg = control.RestoreConfig{ + OnCollision: test.onCollision, + } + dpb = odConsts.DriveFolderPrefixBuilder("driveID1") + ) + + dpp, err := dpb.ToDataLayerOneDrivePath("t", "u", false) + require.NoError(t, err) + + dp, err := path.ToDrivePath(dpp) + require.NoError(t, err) + + _, skip, err := restoreItem( + ctx, + rh, + restoreCfg, + mock.FetchItemByName{ + Item: &mock.Data{ + Reader: mock.FileRespReadCloser(mock.DriveFileMetaData), + }, + }, + version.Backup, + dp, + "", + []byte{}, + NewRestoreCaches(), + false, + &mock.Data{ID: uuid.NewString()}, + nil) + + test.expectErr(t, err) + test.expectSkipped(t, skip) + }) + } +} diff --git a/src/internal/m365/onedrive/url_cache_test.go b/src/internal/m365/onedrive/url_cache_test.go index a7bafb68e..6e5da998c 100644 --- a/src/internal/m365/onedrive/url_cache_test.go +++ b/src/internal/m365/onedrive/url_cache_test.go @@ -17,6 +17,7 @@ import ( "github.com/alcionai/corso/src/internal/common/ptr" "github.com/alcionai/corso/src/internal/m365/graph" "github.com/alcionai/corso/src/internal/tester" + "github.com/alcionai/corso/src/pkg/control" "github.com/alcionai/corso/src/pkg/control/testdata" "github.com/alcionai/corso/src/pkg/fault" "github.com/alcionai/corso/src/pkg/services/m365/api" @@ -81,7 +82,8 @@ func (suite *URLCacheIntegrationSuite) TestURLCacheBasic() { ctx, driveID, ptr.Val(root.GetId()), - newItem(newFolderName, true)) + newItem(newFolderName, true), + control.Copy) require.NoError(t, err, clues.ToCore(err)) require.NotNil(t, newFolder.GetId()) @@ -97,7 +99,8 @@ func (suite *URLCacheIntegrationSuite) TestURLCacheBasic() { ctx, driveID, nfid, - newItem(newItemName, false)) + newItem(newItemName, false), + control.Copy) if err != nil { // Something bad happened, skip this item continue diff --git a/src/internal/m365/restore.go b/src/internal/m365/restore.go index 07d4cd968..18c35060c 100644 --- a/src/internal/m365/restore.go +++ b/src/internal/m365/restore.go @@ -34,6 +34,7 @@ func (ctrl *Controller) ConsumeRestoreCollections( defer end() ctx = graph.BindRateLimiterConfig(ctx, graph.LimiterCfg{Service: sels.PathService()}) + ctx = clues.Add(ctx, "restore_config", restoreCfg) // TODO(rkeepers): needs PII control var ( status *support.ControllerOperationStatus diff --git a/src/internal/m365/sharepoint/library_handler.go b/src/internal/m365/sharepoint/library_handler.go index eff8a9bff..a51621f7b 100644 --- a/src/internal/m365/sharepoint/library_handler.go +++ b/src/internal/m365/sharepoint/library_handler.go @@ -12,6 +12,7 @@ import ( "github.com/alcionai/corso/src/internal/m365/onedrive" odConsts "github.com/alcionai/corso/src/internal/m365/onedrive/consts" "github.com/alcionai/corso/src/pkg/backup/details" + "github.com/alcionai/corso/src/pkg/control" "github.com/alcionai/corso/src/pkg/path" "github.com/alcionai/corso/src/pkg/selectors" "github.com/alcionai/corso/src/pkg/services/m365/api" @@ -198,8 +199,9 @@ func (h libraryRestoreHandler) PostItemInContainer( ctx context.Context, driveID, parentFolderID string, newItem models.DriveItemable, + onCollision control.CollisionPolicy, ) (models.DriveItemable, error) { - return h.ac.PostItemInContainer(ctx, driveID, parentFolderID, newItem) + return h.ac.PostItemInContainer(ctx, driveID, parentFolderID, newItem, onCollision) } func (h libraryRestoreHandler) GetFolderByName( diff --git a/src/internal/m365/sharepoint/restore.go b/src/internal/m365/sharepoint/restore.go index 772515fd4..191ac5f96 100644 --- a/src/internal/m365/sharepoint/restore.go +++ b/src/internal/m365/sharepoint/restore.go @@ -68,10 +68,10 @@ func ConsumeRestoreCollections( metrics, err = onedrive.RestoreCollection( ictx, libraryRestoreHandler{ac.Drives()}, + restoreCfg, backupVersion, dc, caches, - restoreCfg.Location, deets, opts.RestorePermissions, errs) diff --git a/src/internal/operations/backup_integration_test.go b/src/internal/operations/backup_integration_test.go index 771c77122..36ec0cfa3 100644 --- a/src/internal/operations/backup_integration_test.go +++ b/src/internal/operations/backup_integration_test.go @@ -1564,7 +1564,8 @@ func runDriveIncrementalTest( ctx, driveID, targetContainer, - driveItem) + driveItem, + control.Copy) require.NoErrorf(t, err, "creating new file %v", clues.ToCore(err)) newFileID = ptr.Val(newFile.GetId()) diff --git a/src/pkg/services/m365/api/drive.go b/src/pkg/services/m365/api/drive.go index 478da708e..d1dd93fc3 100644 --- a/src/pkg/services/m365/api/drive.go +++ b/src/pkg/services/m365/api/drive.go @@ -9,6 +9,7 @@ import ( "github.com/microsoftgraph/msgraph-sdk-go/models" "github.com/alcionai/corso/src/internal/m365/graph" + "github.com/alcionai/corso/src/pkg/control" ) // --------------------------------------------------------------------------- @@ -123,21 +124,44 @@ func (c Drives) NewItemContentUpload( return r, nil } -const itemChildrenRawURLFmt = "https://graph.microsoft.com/v1.0/drives/%s/items/%s/children" +//nolint:lll +const itemChildrenRawURLFmt = "https://graph.microsoft.com/v1.0/drives/%s/items/%s/children?@microsoft.graph.conflictBehavior=%s" + +const ( + conflictBehaviorFail = "fail" + conflictBehaviorRename = "rename" + conflictBehaviorReplace = "replace" +) // PostItemInContainer creates a new item in the specified folder func (c Drives) PostItemInContainer( ctx context.Context, driveID, parentFolderID string, newItem models.DriveItemable, + onCollision control.CollisionPolicy, ) (models.DriveItemable, error) { + // graph api has no policy for Skip; instead we wrap the same-name failure + // as a graph.ErrItemAlreadyExistsConflict. + conflictBehavior := conflictBehaviorFail + + switch onCollision { + case control.Replace: + conflictBehavior = conflictBehaviorReplace + case control.Copy: + conflictBehavior = conflictBehaviorRename + } + // Graph SDK doesn't yet provide a POST method for `/children` so we set the `rawUrl` ourselves as recommended // here: https://github.com/microsoftgraph/msgraph-sdk-go/issues/155#issuecomment-1136254310 - rawURL := fmt.Sprintf(itemChildrenRawURLFmt, driveID, parentFolderID) + rawURL := fmt.Sprintf(itemChildrenRawURLFmt, driveID, parentFolderID, conflictBehavior) builder := drives.NewItemItemsRequestBuilder(rawURL, c.Stable.Adapter()) newItem, err := builder.Post(ctx, newItem, nil) if err != nil { + if graph.IsErrItemAlreadyExistsConflict(err) { + return nil, clues.Stack(graph.ErrItemAlreadyExistsConflict, err) + } + return nil, graph.Wrap(ctx, err, "creating item in folder") } diff --git a/src/pkg/services/m365/api/drive_test.go b/src/pkg/services/m365/api/drive_test.go index 22d6d71e6..12329427c 100644 --- a/src/pkg/services/m365/api/drive_test.go +++ b/src/pkg/services/m365/api/drive_test.go @@ -4,23 +4,35 @@ import ( "testing" "github.com/alcionai/clues" + "github.com/microsoftgraph/msgraph-sdk-go/models" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" + "github.com/alcionai/corso/src/internal/common/ptr" + "github.com/alcionai/corso/src/internal/m365/graph" "github.com/alcionai/corso/src/internal/tester" "github.com/alcionai/corso/src/pkg/account" + "github.com/alcionai/corso/src/pkg/control" + "github.com/alcionai/corso/src/pkg/control/testdata" "github.com/alcionai/corso/src/pkg/services/m365/api" ) -type OneDriveAPISuite struct { +type DriveAPISuite struct { tester.Suite - creds account.M365Config - ac api.Client + creds account.M365Config + ac api.Client + driveID string + rootFolderID string } -func (suite *OneDriveAPISuite) SetupSuite() { +func (suite *DriveAPISuite) SetupSuite() { t := suite.T() + + ctx, flush := tester.NewContext(t) + defer flush() + + userID := tester.M365UserID(t) a := tester.NewM365Account(t) creds, err := a.M365Config() require.NoError(t, err, clues.ToCore(err)) @@ -28,17 +40,27 @@ func (suite *OneDriveAPISuite) SetupSuite() { suite.creds = creds suite.ac, err = api.NewClient(creds) require.NoError(t, err, clues.ToCore(err)) + + drive, err := suite.ac.Users().GetDefaultDrive(ctx, userID) + require.NoError(t, err, clues.ToCore(err)) + + suite.driveID = ptr.Val(drive.GetId()) + + rootFolder, err := suite.ac.Drives().GetRootFolder(ctx, suite.driveID) + require.NoError(t, err, clues.ToCore(err)) + + suite.rootFolderID = ptr.Val(rootFolder.GetId()) } -func TestOneDriveAPIs(t *testing.T) { - suite.Run(t, &OneDriveAPISuite{ +func TestDriveAPIs(t *testing.T) { + suite.Run(t, &DriveAPISuite{ Suite: tester.NewIntegrationSuite( t, [][]string{tester.M365AcctCredEnvs}), }) } -func (suite *OneDriveAPISuite) TestCreatePagerAndGetPage() { +func (suite *DriveAPISuite) TestDrives_CreatePagerAndGetPage() { t := suite.T() ctx, flush := tester.NewContext(t) @@ -51,3 +73,174 @@ func (suite *OneDriveAPISuite) TestCreatePagerAndGetPage() { assert.NoError(t, err, clues.ToCore(err)) assert.NotNil(t, a) } + +// newItem initializes a `models.DriveItemable` that can be used as input to `createItem` +func newItem(name string, folder bool) *models.DriveItem { + itemToCreate := models.NewDriveItem() + itemToCreate.SetName(&name) + + if folder { + itemToCreate.SetFolder(models.NewFolder()) + } else { + itemToCreate.SetFile(models.NewFile()) + } + + return itemToCreate +} + +func (suite *DriveAPISuite) TestDrives_PostItemInContainer() { + t := suite.T() + + ctx, flush := tester.NewContext(t) + defer flush() + + rc := testdata.DefaultRestoreConfig("drive_api_post_item") + + // generate a parent for the test data + parent, err := suite.ac.Drives().PostItemInContainer( + ctx, + suite.driveID, + suite.rootFolderID, + newItem(rc.Location, true), + control.Replace) + require.NoError(t, err, clues.ToCore(err)) + + // generate a folder to use for collision testing + folder := newItem("collision", true) + origFolder, err := suite.ac.Drives().PostItemInContainer( + ctx, + suite.driveID, + ptr.Val(parent.GetId()), + folder, + control.Copy) + require.NoError(t, err, clues.ToCore(err)) + + // generate an item to use for collision testing + file := newItem("collision.txt", false) + origFile, err := suite.ac.Drives().PostItemInContainer( + ctx, + suite.driveID, + ptr.Val(parent.GetId()), + file, + control.Copy) + require.NoError(t, err, clues.ToCore(err)) + + table := []struct { + name string + onCollision control.CollisionPolicy + postItem models.DriveItemable + expectErr func(t *testing.T, err error) + expectItem func(t *testing.T, i models.DriveItemable) + }{ + { + name: "fail folder", + onCollision: control.Skip, + postItem: folder, + expectErr: func(t *testing.T, err error) { + assert.ErrorIs(t, err, graph.ErrItemAlreadyExistsConflict, clues.ToCore(err)) + }, + expectItem: func(t *testing.T, i models.DriveItemable) { + assert.Nil(t, i) + }, + }, + { + name: "rename folder", + onCollision: control.Copy, + postItem: folder, + expectErr: func(t *testing.T, err error) { + assert.NoError(t, err, clues.ToCore(err)) + }, + expectItem: func(t *testing.T, i models.DriveItemable) { + assert.NotEqual( + t, + ptr.Val(origFolder.GetId()), + ptr.Val(i.GetId()), + "renamed item should have a different id") + assert.NotEqual( + t, + ptr.Val(origFolder.GetName()), + ptr.Val(i.GetName()), + "renamed item should have a different name") + }, + }, + { + name: "replace folder", + onCollision: control.Replace, + postItem: folder, + expectErr: func(t *testing.T, err error) { + assert.NoError(t, err, clues.ToCore(err)) + }, + expectItem: func(t *testing.T, i models.DriveItemable) { + assert.Equal( + t, + ptr.Val(origFolder.GetId()), + ptr.Val(i.GetId()), + "replaced item should have the same id") + assert.Equal( + t, + ptr.Val(origFolder.GetName()), + ptr.Val(i.GetName()), + "replaced item should have the same name") + }, + }, + { + name: "fail file", + onCollision: control.Skip, + postItem: file, + expectErr: func(t *testing.T, err error) { + assert.ErrorIs(t, err, graph.ErrItemAlreadyExistsConflict, clues.ToCore(err)) + }, + expectItem: func(t *testing.T, i models.DriveItemable) { + assert.Nil(t, i) + }, + }, + { + name: "rename file", + onCollision: control.Copy, + postItem: file, + expectErr: func(t *testing.T, err error) { + assert.NoError(t, err, clues.ToCore(err)) + }, + expectItem: func(t *testing.T, i models.DriveItemable) { + assert.NotEqual( + t, + ptr.Val(origFile.GetId()), + ptr.Val(i.GetId()), + "renamed item should have a different id") + assert.NotEqual( + t, + ptr.Val(origFolder.GetName()), + ptr.Val(i.GetName()), + "renamed item should have a different name") + }, + }, + // FIXME: this *should* behave the same as folder collision, but there's either a + // bug or a deviation in graph api behavior. + // See open ticket: https://github.com/OneDrive/onedrive-api-docs/issues/1702 + { + name: "replace file", + onCollision: control.Replace, + postItem: file, + expectErr: func(t *testing.T, err error) { + assert.ErrorIs(t, err, graph.ErrItemAlreadyExistsConflict, clues.ToCore(err)) + }, + expectItem: func(t *testing.T, i models.DriveItemable) { + assert.Nil(t, i) + }, + }, + } + for _, test := range table { + suite.Run(test.name, func() { + t := suite.T() + i, err := suite.ac.Drives().PostItemInContainer( + ctx, + suite.driveID, + ptr.Val(parent.GetId()), + test.postItem, + test.onCollision) + + test.expectErr(t, err) + test.expectItem(t, i) + }) + } +}