From e60ae2351fae23f000abf65260ae675a88be0352 Mon Sep 17 00:00:00 2001 From: Abin Simon Date: Wed, 15 Feb 2023 23:57:45 +0530 Subject: [PATCH] Set proper state and paths for moves in OneDrive (#2501) ## Description Ties up the final piece of https://github.com/alcionai/corso/issues/2123. Add handling of moves for folder / files in delta response. ## Does this PR need a docs update or release note? - [ ] :white_check_mark: Yes, it's included - [x] :clock1: Yes, but in a later PR - [ ] :no_entry: No ## Type of change - [x] :sunflower: Feature - [ ] :bug: Bugfix - [ ] :world_map: Documentation - [ ] :robot: Test - [ ] :computer: CI/Deployment - [ ] :broom: Tech Debt/Cleanup ## Issue(s) * https://github.com/alcionai/corso/issues/2123 ## Test Plan - [ ] :muscle: Manual - [x] :zap: Unit test - [ ] :green_heart: E2E --- src/internal/connector/onedrive/collection.go | 5 + .../connector/onedrive/collections.go | 138 +++++++-- .../connector/onedrive/collections_test.go | 272 ++++++++++++++---- src/pkg/path/path.go | 3 + src/pkg/path/resource_path.go | 23 ++ src/pkg/path/resource_path_test.go | 75 +++++ 6 files changed, 440 insertions(+), 76 deletions(-) diff --git a/src/internal/connector/onedrive/collection.go b/src/internal/connector/onedrive/collection.go index 08efa9994..3538c8dd2 100644 --- a/src/internal/connector/onedrive/collection.go +++ b/src/internal/connector/onedrive/collection.go @@ -151,6 +151,11 @@ func (oc Collection) PreviousPath() path.Path { return oc.prevPath } +func (oc *Collection) SetFullPath(curPath path.Path) { + oc.folderPath = curPath + oc.state = data.StateOf(oc.prevPath, curPath) +} + func (oc Collection) State() data.CollectionState { return oc.state } diff --git a/src/internal/connector/onedrive/collections.go b/src/internal/connector/onedrive/collections.go index 11e0d9a30..e6f33ea9f 100644 --- a/src/internal/connector/onedrive/collections.go +++ b/src/internal/connector/onedrive/collections.go @@ -12,6 +12,7 @@ import ( "github.com/pkg/errors" "golang.org/x/exp/maps" + "github.com/alcionai/clues" "github.com/alcionai/corso/src/internal/connector/graph" "github.com/alcionai/corso/src/internal/connector/support" "github.com/alcionai/corso/src/internal/data" @@ -366,6 +367,54 @@ func (c *Collections) Get( return collections, excludedItems, nil } +func updateCollectionPaths( + id string, + cmap map[string]data.BackupCollection, + curPath path.Path, +) (bool, error) { + var initialCurPath path.Path + + col, found := cmap[id] + if found { + ocol, ok := col.(*Collection) + if !ok { + return found, clues.New("unable to cast onedrive collection") + } + + initialCurPath = ocol.FullPath() + if initialCurPath.String() == curPath.String() { + return found, nil + } + + ocol.SetFullPath(curPath) + } + + if initialCurPath == nil { + return found, nil + } + + for i, c := range cmap { + if i == id { + continue + } + + ocol, ok := c.(*Collection) + if !ok { + return found, clues.New("unable to cast onedrive collection") + } + + colPath := c.FullPath() + + // Only updates if initialCurPath parent of colPath + updated := colPath.UpdateParent(initialCurPath, curPath) + if updated { + ocol.SetFullPath(colPath) + } + } + + return found, nil +} + // UpdateCollections initializes and adds the provided drive items to Collections // A new collection is created for every drive folder (or package). // oldPaths is the unchanged data that was loaded from the metadata file. @@ -381,6 +430,11 @@ func (c *Collections) UpdateCollections( invalidPrevDelta bool, ) error { for _, item := range items { + var ( + prevPath path.Path + prevCollectionPath path.Path + ) + if item.GetRoot() != nil { // Skip the root item continue @@ -413,14 +467,21 @@ func (c *Collections) UpdateCollections( switch { case item.GetFolder() != nil, item.GetPackage() != nil: + prevPathStr, ok := oldPaths[*item.GetId()] + if ok { + prevPath, err = path.FromDataLayerPath(prevPathStr, false) + if err != nil { + return clues.Wrap(err, "invalid previous path").WithAll("path_string", prevPathStr) + } + } + if item.GetDeleted() != nil { // Nested folders also return deleted delta results so we don't have to // worry about doing a prefix search in the map to remove the subtree of // the deleted folder/package. delete(newPaths, *item.GetId()) - prevColPath, ok := oldPaths[*item.GetId()] - if !ok { + if prevPath == nil { // It is possible that an item was created and // deleted between two delta invocations. In // that case, it will only produce a single @@ -428,12 +489,6 @@ func (c *Collections) UpdateCollections( continue } - prevPath, err := path.FromDataLayerPath(prevColPath, false) - if err != nil { - logger.Ctx(ctx).Errorw("invalid previous path for deleted item", "error", err) - return err - } - col := NewCollection( c.itemClient, nil, @@ -463,12 +518,34 @@ func (c *Collections) UpdateCollections( // Moved folders don't cause delta results for any subfolders nested in // them. We need to go through and update paths to handle that. We only // update newPaths so we don't accidentally clobber previous deletes. - // - // TODO(ashmrtn): Since we're also getting notifications about folder - // moves we may need to handle updates to a path of a collection we've - // already created and partially populated. updatePath(newPaths, *item.GetId(), folderPath.String()) + found, err := updateCollectionPaths(*item.GetId(), c.CollectionMap, folderPath) + if err != nil { + return err + } + + if !found { + // We only create collections for folder that are not + // new. This is so as to not create collections for + // new folders without any files within them. + if prevPath != nil { + col := NewCollection( + c.itemClient, + folderPath, + prevPath, + driveID, + c.service, + c.statusUpdater, + c.source, + c.ctrl, + invalidPrevDelta, + ) + c.CollectionMap[*item.GetId()] = col + c.NumContainers++ + } + } + if c.source != OneDriveSource { continue } @@ -476,8 +553,15 @@ func (c *Collections) UpdateCollections( fallthrough case item.GetFile() != nil: - if item.GetDeleted() != nil { + if !invalidPrevDelta && item.GetFile() != nil { + // Always add a file to the excluded list. If it was + // deleted, we want to avoid it. If it was + // renamed/moved/modified, we still have to drop the + // original one and download a fresh copy. excluded[*item.GetId()] = struct{}{} + } + + if item.GetDeleted() != nil { // Exchange counts items streamed through it which includes deletions so // add that here too. c.NumFiles++ @@ -486,17 +570,31 @@ func (c *Collections) UpdateCollections( continue } - // TODO(ashmrtn): Figure what when an item was moved (maybe) and add it to - // the exclude list. + oneDrivePath, err := path.ToOneDrivePath(collectionPath) + if err != nil { + return clues.Wrap(err, "invalid path for backup") + } + + if len(oneDrivePath.Folders) == 0 { + // path for root will never change + prevCollectionPath = collectionPath + } else { + prevCollectionPathStr, ok := oldPaths[collectionID] + if ok { + prevCollectionPath, err = path.FromDataLayerPath(prevCollectionPathStr, false) + if err != nil { + return clues.Wrap(err, "invalid previous path").WithAll("path_string", prevCollectionPathStr) + } + } + + } col, found := c.CollectionMap[collectionID] if !found { - // TODO(ashmrtn): Compare old and new path and set collection state - // accordingly. col = NewCollection( c.itemClient, collectionPath, - nil, + prevCollectionPath, driveID, c.service, c.statusUpdater, @@ -509,6 +607,10 @@ func (c *Collections) UpdateCollections( c.NumContainers++ } + // TODO(meain): If a folder gets renamed/moved multiple + // times within a single delta response, we might end up + // storing the permissions multiple times. Switching the + // files to IDs should fix this. collection := col.(*Collection) collection.Add(item) diff --git a/src/internal/connector/onedrive/collections_test.go b/src/internal/connector/onedrive/collections_test.go index 12c2cb1e1..c5e4acc87 100644 --- a/src/internal/connector/onedrive/collections_test.go +++ b/src/internal/connector/onedrive/collections_test.go @@ -33,20 +33,38 @@ type statePath struct { func getExpectedStatePathGenerator( t *testing.T, tenant, user, base string, -) func(data.CollectionState, string) statePath { - return func(state data.CollectionState, pth string) statePath { - p, err := GetCanonicalPath(base+pth, tenant, user, OneDriveSource) - require.NoError(t, err) - +) func(data.CollectionState, ...string) statePath { + return func(state data.CollectionState, pths ...string) statePath { var ( - cp path.Path - pp path.Path + p1 path.Path + p2 path.Path + pp path.Path + cp path.Path + err error ) - if state == data.NewState { - cp = p + if state != data.MovedState { + require.Len(t, pths, 1, "invalid number of paths to getExpectedStatePathGenerator") } else { - pp = p + require.Len(t, pths, 2, "invalid number of paths to getExpectedStatePathGenerator") + p2, err = GetCanonicalPath(base+pths[1], tenant, user, OneDriveSource) + require.NoError(t, err) + } + + p1, err = GetCanonicalPath(base+pths[0], tenant, user, OneDriveSource) + require.NoError(t, err) + + switch state { + case data.NewState: + cp = p1 + case data.NotMovedState: + cp = p1 + pp = p1 + case data.DeletedState: + pp = p1 + case data.MovedState: + pp = p2 + cp = p1 } return statePath{ @@ -169,14 +187,14 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { scope: anyFolder, expect: assert.NoError, expectedCollectionIDs: map[string]statePath{ - "root": expectedStatePath(data.NewState, ""), + "root": expectedStatePath(data.NotMovedState, ""), }, expectedItemCount: 1, expectedFileCount: 1, expectedContainerCount: 1, // Root folder is skipped since it's always present. expectedMetadataPaths: map[string]string{}, - expectedExcludes: map[string]struct{}{}, + expectedExcludes: map[string]struct{}{"file": {}}, }, { testCase: "Single Folder", @@ -188,7 +206,7 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { scope: anyFolder, expect: assert.NoError, expectedCollectionIDs: map[string]statePath{ - "root": expectedStatePath(data.NewState, ""), + "root": expectedStatePath(data.NotMovedState, ""), }, expectedMetadataPaths: map[string]string{ "folder": expectedPath("/folder"), @@ -207,7 +225,7 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { scope: anyFolder, expect: assert.NoError, expectedCollectionIDs: map[string]statePath{ - "root": expectedStatePath(data.NewState, ""), + "root": expectedStatePath(data.NotMovedState, ""), }, expectedMetadataPaths: map[string]string{ "package": expectedPath("/package"), @@ -230,7 +248,7 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { scope: anyFolder, expect: assert.NoError, expectedCollectionIDs: map[string]statePath{ - "root": expectedStatePath(data.NewState, ""), + "root": expectedStatePath(data.NotMovedState, ""), "folder": expectedStatePath(data.NewState, folder), "package": expectedStatePath(data.NewState, pkg), }, @@ -241,7 +259,7 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { "folder": expectedPath("/folder"), "package": expectedPath("/package"), }, - expectedExcludes: map[string]struct{}{}, + expectedExcludes: map[string]struct{}{"fileInRoot": {}, "fileInFolder": {}, "fileInPackage": {}}, }, { testCase: "contains folder selector", @@ -273,7 +291,7 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { "subfolder": expectedPath("/folder/subfolder"), "folder2": expectedPath("/folder/subfolder/folder"), }, - expectedExcludes: map[string]struct{}{}, + expectedExcludes: map[string]struct{}{"fileInFolder": {}, "fileInFolder2": {}}, }, { testCase: "prefix subfolder selector", @@ -302,7 +320,7 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { expectedMetadataPaths: map[string]string{ "folder2": expectedPath("/folder/subfolder/folder"), }, - expectedExcludes: map[string]struct{}{}, + expectedExcludes: map[string]struct{}{"fileInFolder2": {}}, }, { testCase: "match subfolder selector", @@ -327,7 +345,7 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { expectedContainerCount: 1, // No child folders for subfolder so nothing here. expectedMetadataPaths: map[string]string{}, - expectedExcludes: map[string]struct{}{}, + expectedExcludes: map[string]struct{}{"fileInSubfolder": {}}, }, { testCase: "not moved folder tree", @@ -342,11 +360,12 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { scope: anyFolder, expect: assert.NoError, expectedCollectionIDs: map[string]statePath{ - "root": expectedStatePath(data.NewState, ""), + "root": expectedStatePath(data.NotMovedState, ""), + "folder": expectedStatePath(data.NotMovedState, "/folder"), }, expectedItemCount: 1, expectedFileCount: 0, - expectedContainerCount: 1, + expectedContainerCount: 2, expectedMetadataPaths: map[string]string{ "folder": expectedPath("/folder"), "subfolder": expectedPath("/folder/subfolder"), @@ -366,17 +385,87 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { scope: anyFolder, expect: assert.NoError, expectedCollectionIDs: map[string]statePath{ - "root": expectedStatePath(data.NewState, ""), + "root": expectedStatePath(data.NotMovedState, ""), + "folder": expectedStatePath(data.MovedState, "/folder", "/a-folder"), }, expectedItemCount: 1, expectedFileCount: 0, - expectedContainerCount: 1, + expectedContainerCount: 2, expectedMetadataPaths: map[string]string{ "folder": expectedPath("/folder"), "subfolder": expectedPath("/folder/subfolder"), }, expectedExcludes: map[string]struct{}{}, }, + { + testCase: "moved folder tree with file with file first", + items: []models.DriveItemable{ + driveRootItem("root"), + driveItem("file", "file", testBaseDrivePath+"/folder", "folder", true, false, false), + driveItem("folder", "folder", testBaseDrivePath, "root", false, true, false), + }, + inputFolderMap: map[string]string{ + "folder": expectedPath("/folder"), + }, + scope: anyFolder, + expect: assert.NoError, + expectedCollectionIDs: map[string]statePath{ + "root": expectedStatePath(data.NotMovedState, ""), + "folder": expectedStatePath(data.NotMovedState, "/folder"), + }, + expectedItemCount: 2, + expectedFileCount: 1, + expectedContainerCount: 2, + expectedMetadataPaths: map[string]string{ + "folder": expectedPath("/folder"), + }, + expectedExcludes: map[string]struct{}{"file": {}}, + }, + { + testCase: "moved folder tree with file no previous", + items: []models.DriveItemable{ + driveRootItem("root"), + driveItem("folder", "folder", testBaseDrivePath, "root", false, true, false), + driveItem("file", "file", testBaseDrivePath+"/folder", "folder", true, false, false), + driveItem("folder", "folder2", testBaseDrivePath, "root", false, true, false), + }, + inputFolderMap: map[string]string{}, + scope: anyFolder, + expect: assert.NoError, + expectedCollectionIDs: map[string]statePath{ + "root": expectedStatePath(data.NotMovedState, ""), + "folder": expectedStatePath(data.NewState, "/folder2"), + }, + expectedItemCount: 3, // permissions gets saved twice for folder + expectedFileCount: 1, + expectedContainerCount: 2, + expectedMetadataPaths: map[string]string{ + "folder": expectedPath("/folder2"), + }, + expectedExcludes: map[string]struct{}{"file": {}}, + }, + { + testCase: "moved folder tree with file no previous 1", + items: []models.DriveItemable{ + driveRootItem("root"), + driveItem("folder", "folder", testBaseDrivePath, "root", false, true, false), + driveItem("file", "file", testBaseDrivePath+"/folder", "folder", true, false, false), + }, + inputFolderMap: map[string]string{}, + scope: anyFolder, + expect: assert.NoError, + expectedCollectionIDs: map[string]statePath{ + "root": expectedStatePath(data.NotMovedState, ""), + "folder": expectedStatePath(data.NewState, "/folder"), + }, + expectedItemCount: 2, + expectedFileCount: 1, + expectedContainerCount: 2, + expectedMetadataPaths: map[string]string{ + "folder": expectedPath("/folder"), + }, + expectedExcludes: map[string]struct{}{"file": {}}, + }, { testCase: "moved folder tree and subfolder 1", items: []models.DriveItemable{ @@ -391,11 +480,13 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { scope: anyFolder, expect: assert.NoError, expectedCollectionIDs: map[string]statePath{ - "root": expectedStatePath(data.NewState, ""), + "root": expectedStatePath(data.NotMovedState, ""), + "folder": expectedStatePath(data.MovedState, "/folder", "/a-folder"), + "subfolder": expectedStatePath(data.MovedState, "/subfolder", "/a-folder/subfolder"), }, expectedItemCount: 2, expectedFileCount: 0, - expectedContainerCount: 1, + expectedContainerCount: 3, expectedMetadataPaths: map[string]string{ "folder": expectedPath("/folder"), "subfolder": expectedPath("/subfolder"), @@ -416,17 +507,59 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { scope: anyFolder, expect: assert.NoError, expectedCollectionIDs: map[string]statePath{ - "root": expectedStatePath(data.NewState, ""), + "root": expectedStatePath(data.NotMovedState, ""), + "folder": expectedStatePath(data.MovedState, "/folder", "/a-folder"), + "subfolder": expectedStatePath(data.MovedState, "/subfolder", "/a-folder/subfolder"), }, expectedItemCount: 2, expectedFileCount: 0, - expectedContainerCount: 1, + expectedContainerCount: 3, expectedMetadataPaths: map[string]string{ "folder": expectedPath("/folder"), "subfolder": expectedPath("/subfolder"), }, expectedExcludes: map[string]struct{}{}, }, + { + testCase: "move subfolder when moving parent", + items: []models.DriveItemable{ + driveRootItem("root"), + driveItem("folder2", "folder2", testBaseDrivePath, "root", false, true, false), + driveItem("itemInFolder2", "itemInFolder2", testBaseDrivePath+"/folder2", "folder2", true, false, false), + driveItem("subfolder", "subfolder", testBaseDrivePath+"/a-folder", "folder", false, true, false), + driveItem( + "itemInSubfolder", + "itemInSubfolder", + testBaseDrivePath+"/a-folder/subfolder", + "subfolder", + true, + false, + false, + ), + driveItem("folder", "folder", testBaseDrivePath, "root", false, true, false), + }, + inputFolderMap: map[string]string{ + "folder": expectedPath("/a-folder"), + "subfolder": expectedPath("/a-folder/subfolder"), + }, + scope: anyFolder, + expect: assert.NoError, + expectedCollectionIDs: map[string]statePath{ + "root": expectedStatePath(data.NotMovedState, ""), + "folder": expectedStatePath(data.MovedState, "/folder", "/a-folder"), + "folder2": expectedStatePath(data.NewState, "/folder2"), + "subfolder": expectedStatePath(data.MovedState, "/folder/subfolder", "/a-folder/subfolder"), + }, + expectedItemCount: 5, + expectedFileCount: 2, + expectedContainerCount: 4, + expectedMetadataPaths: map[string]string{ + "folder": expectedPath("/folder"), + "folder2": expectedPath("/folder2"), + "subfolder": expectedPath("/folder/subfolder"), + }, + expectedExcludes: map[string]struct{}{"itemInSubfolder": {}, "itemInFolder2": {}}, + }, { testCase: "deleted folder and package", items: []models.DriveItemable{ @@ -450,6 +583,22 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { expectedMetadataPaths: map[string]string{}, expectedExcludes: map[string]struct{}{}, }, + { + testCase: "delete folder without previous", + items: []models.DriveItemable{ + driveRootItem("root"), + delItem("folder", testBaseDrivePath, "root", false, true, false), + }, + inputFolderMap: map[string]string{}, + scope: anyFolder, + expect: assert.NoError, + expectedCollectionIDs: map[string]statePath{}, + expectedItemCount: 0, + expectedFileCount: 0, + expectedContainerCount: 0, + expectedMetadataPaths: map[string]string{}, + expectedExcludes: map[string]struct{}{}, + }, { testCase: "delete folder tree move subfolder", items: []models.DriveItemable{ @@ -464,12 +613,13 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { scope: anyFolder, expect: assert.NoError, expectedCollectionIDs: map[string]statePath{ - "root": expectedStatePath(data.NewState, ""), - "folder": expectedStatePath(data.DeletedState, folder), + "root": expectedStatePath(data.NotMovedState, ""), + "folder": expectedStatePath(data.DeletedState, folder), + "subfolder": expectedStatePath(data.MovedState, "/subfolder", "/folder/subfolder"), }, expectedItemCount: 1, expectedFileCount: 0, - expectedContainerCount: 1, + expectedContainerCount: 2, expectedMetadataPaths: map[string]string{ "subfolder": expectedPath("/subfolder"), }, @@ -528,14 +678,14 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { assert.Equal(t, tt.expectedContainerCount, c.NumContainers, "container count") for id, sp := range tt.expectedCollectionIDs { - assert.Contains(t, c.CollectionMap, id, "contains collection with id") - assert.Equal(t, sp.state, c.CollectionMap[id].State(), "state for collection") - assert.Equal(t, sp.curPath, c.CollectionMap[id].FullPath(), "current path for collection") - assert.Equal(t, sp.prevPath, c.CollectionMap[id].PreviousPath(), "prev path for collection") + assert.Containsf(t, c.CollectionMap, id, "contains collection with id %s", id) + assert.Equalf(t, sp.state, c.CollectionMap[id].State(), "state for collection %s", id) + assert.Equalf(t, sp.curPath, c.CollectionMap[id].FullPath(), "current path for collection %s", id) + assert.Equalf(t, sp.prevPath, c.CollectionMap[id].PreviousPath(), "prev path for collection %s", id) } - assert.Equal(t, tt.expectedMetadataPaths, outputFolderMap) - assert.Equal(t, tt.expectedExcludes, excludes) + assert.Equal(t, tt.expectedMetadataPaths, outputFolderMap, "metadata paths") + assert.Equal(t, tt.expectedExcludes, excludes, "exclude list") }) } } @@ -1041,7 +1191,7 @@ func (suite *OneDriveCollectionsSuite) TestGet() { }, errCheck: assert.NoError, expectedCollections: map[string]map[data.CollectionState][]string{ - expectedPath1(""): {data.NewState: {"file"}}, + expectedPath1(""): {data.NotMovedState: {"file"}}, }, expectedDeltaURLs: map[string]string{ driveID1: delta, @@ -1051,7 +1201,7 @@ func (suite *OneDriveCollectionsSuite) TestGet() { // token for this drive is valid. driveID1: {}, }, - expectedDelList: map[string]struct{}{}, + expectedDelList: map[string]struct{}{"file": {}}, }, { name: "OneDrive_OneItemPage_NoErrors", @@ -1071,7 +1221,7 @@ func (suite *OneDriveCollectionsSuite) TestGet() { errCheck: assert.NoError, expectedCollections: map[string]map[data.CollectionState][]string{ folderPath1: {data.NewState: {"file"}}, - rootFolderPath1: {data.NewState: {"folder"}}, + rootFolderPath1: {data.NotMovedState: {"folder"}}, }, expectedDeltaURLs: map[string]string{ driveID1: delta, @@ -1081,7 +1231,7 @@ func (suite *OneDriveCollectionsSuite) TestGet() { "folder": folderPath1, }, }, - expectedDelList: map[string]struct{}{}, + expectedDelList: map[string]struct{}{"file": {}}, }, { name: "OneDrive_OneItemPage_EmptyDelta_NoErrors", @@ -1101,11 +1251,11 @@ func (suite *OneDriveCollectionsSuite) TestGet() { errCheck: assert.NoError, expectedCollections: map[string]map[data.CollectionState][]string{ folderPath1: {data.NewState: {"file"}}, - rootFolderPath1: {data.NewState: {"folder"}}, + rootFolderPath1: {data.NotMovedState: {"folder"}}, }, expectedDeltaURLs: map[string]string{}, expectedFolderPaths: map[string]map[string]string{}, - expectedDelList: map[string]struct{}{}, + expectedDelList: map[string]struct{}{"file": {}}, }, { name: "OneDrive_TwoItemPages_NoErrors", @@ -1133,7 +1283,7 @@ func (suite *OneDriveCollectionsSuite) TestGet() { errCheck: assert.NoError, expectedCollections: map[string]map[data.CollectionState][]string{ folderPath1: {data.NewState: {"file", "file2"}}, - rootFolderPath1: {data.NewState: {"folder"}}, + rootFolderPath1: {data.NotMovedState: {"folder"}}, }, expectedDeltaURLs: map[string]string{ driveID1: delta, @@ -1143,7 +1293,7 @@ func (suite *OneDriveCollectionsSuite) TestGet() { "folder": folderPath1, }, }, - expectedDelList: map[string]struct{}{}, + expectedDelList: map[string]struct{}{"file": {}, "file2": {}}, }, { name: "TwoDrives_OneItemPageEach_NoErrors", @@ -1165,9 +1315,9 @@ func (suite *OneDriveCollectionsSuite) TestGet() { driveID2: { { items: []models.DriveItemable{ - driveRootItem("root"), - driveItem("folder", "folder", driveBasePath2, "root", false, true, false), - driveItem("file", "file", driveBasePath2+"/folder", "folder", true, false, false), + driveRootItem("root2"), + driveItem("folder2", "folder", driveBasePath2, "root2", false, true, false), + driveItem("file2", "file", driveBasePath2+"/folder", "folder2", true, false, false), }, deltaLink: &delta2, }, @@ -1176,9 +1326,9 @@ func (suite *OneDriveCollectionsSuite) TestGet() { errCheck: assert.NoError, expectedCollections: map[string]map[data.CollectionState][]string{ folderPath1: {data.NewState: {"file"}}, - folderPath2: {data.NewState: {"file"}}, - rootFolderPath1: {data.NewState: {"folder"}}, - rootFolderPath2: {data.NewState: {"folder"}}, + folderPath2: {data.NewState: {"file2"}}, + rootFolderPath1: {data.NotMovedState: {"folder"}}, + rootFolderPath2: {data.NotMovedState: {"folder2"}}, }, expectedDeltaURLs: map[string]string{ driveID1: delta, @@ -1189,10 +1339,10 @@ func (suite *OneDriveCollectionsSuite) TestGet() { "folder": folderPath1, }, driveID2: { - "folder": folderPath2, + "folder2": folderPath2, }, }, - expectedDelList: map[string]struct{}{}, + expectedDelList: map[string]struct{}{"file": {}, "file2": {}}, }, { name: "OneDrive_OneItemPage_Errors", @@ -1229,7 +1379,7 @@ func (suite *OneDriveCollectionsSuite) TestGet() { }, errCheck: assert.NoError, expectedCollections: map[string]map[data.CollectionState][]string{ - expectedPath1(""): {data.NewState: {"file"}}, + expectedPath1(""): {data.NotMovedState: {"file"}}, }, expectedDeltaURLs: map[string]string{ driveID1: delta, @@ -1269,7 +1419,7 @@ func (suite *OneDriveCollectionsSuite) TestGet() { }, errCheck: assert.NoError, expectedCollections: map[string]map[data.CollectionState][]string{ - expectedPath1(""): {data.NewState: {"file", "folder"}}, + expectedPath1(""): {data.NotMovedState: {"file", "folder"}}, expectedPath1("/folder"): {data.NewState: {"file"}}, }, expectedDeltaURLs: map[string]string{ @@ -1305,7 +1455,7 @@ func (suite *OneDriveCollectionsSuite) TestGet() { }, errCheck: assert.NoError, expectedCollections: map[string]map[data.CollectionState][]string{ - expectedPath1(""): {data.NewState: {"file", "folder"}}, + expectedPath1(""): {data.NotMovedState: {"file", "folder"}}, expectedPath1("/folder"): {data.NewState: {"file"}}, }, expectedDeltaURLs: map[string]string{ @@ -1314,7 +1464,7 @@ func (suite *OneDriveCollectionsSuite) TestGet() { expectedFolderPaths: map[string]map[string]string{ driveID1: {"folder": folderPath1}, }, - expectedDelList: map[string]struct{}{}, + expectedDelList: map[string]struct{}{"file": {}}, doNotMergeItems: false, }, } @@ -1422,11 +1572,17 @@ func (suite *OneDriveCollectionsSuite) TestGet() { itemIDs = append(itemIDs, id) } - assert.ElementsMatch(t, test.expectedCollections[folderPath][baseCol.State()], itemIDs) + assert.ElementsMatchf( + t, + test.expectedCollections[folderPath][baseCol.State()], + itemIDs, + "items in collection %s", + folderPath, + ) assert.Equal(t, test.doNotMergeItems, baseCol.DoNotMergeItems(), "DoNotMergeItems") } - assert.Equal(t, test.expectedDelList, delList) + assert.Equal(t, test.expectedDelList, delList, "del list") }) } } diff --git a/src/pkg/path/path.go b/src/pkg/path/path.go index 6db2ae0e9..8b0aade93 100644 --- a/src/pkg/path/path.go +++ b/src/pkg/path/path.go @@ -89,6 +89,9 @@ type Path interface { Folder(bool) string Folders() []string Item() string + // UpdateParent updates parent from old to new if the item/folder was + // parented by old path + UpdateParent(prev, cur Path) bool // PopFront returns a Builder object with the first element (left-side) // removed. As the resulting set of elements is no longer a valid resource // path a Builder is returned instead. diff --git a/src/pkg/path/resource_path.go b/src/pkg/path/resource_path.go index f5384a4ff..77dcaacf3 100644 --- a/src/pkg/path/resource_path.go +++ b/src/pkg/path/resource_path.go @@ -271,3 +271,26 @@ func (rp dataLayerResourcePath) ToBuilder() *Builder { // Safe to directly return the Builder because Builders are immutable. return &rp.Builder } + +func (rp *dataLayerResourcePath) UpdateParent(prev, cur Path) bool { + if prev == cur || len(prev.Elements()) > len(rp.Elements()) { + return false + } + + parent := true + + for i, e := range prev.Elements() { + if rp.elements[i] != e { + parent = false + break + } + } + + if !parent { + return false + } + + rp.elements = append(cur.Elements(), rp.elements[len(prev.Elements()):]...) + + return true +} diff --git a/src/pkg/path/resource_path_test.go b/src/pkg/path/resource_path_test.go index 644eb4592..3979a9c04 100644 --- a/src/pkg/path/resource_path_test.go +++ b/src/pkg/path/resource_path_test.go @@ -532,3 +532,78 @@ func (suite *PopulatedDataLayerResourcePath) TestAppend() { }) } } + +func (suite *PopulatedDataLayerResourcePath) TestUpdateParent() { + cases := []struct { + name string + item string + prev string + cur string + expected string + updated bool + }{ + { + name: "basic", + item: "folder/item", + prev: "folder", + cur: "new-folder", + expected: "new-folder/item", + updated: true, + }, + { + name: "long path", + item: "folder/folder1/folder2/item", + prev: "folder/folder1", + cur: "new-folder/new-folder1", + expected: "new-folder/new-folder1/folder2/item", + updated: true, + }, + { + name: "change to shorter path", + item: "folder/folder1/folder2/item", + prev: "folder/folder1/folder2", + cur: "new-folder", + expected: "new-folder/item", + updated: true, + }, + { + name: "change to longer path", + item: "folder/item", + prev: "folder", + cur: "folder/folder1/folder2/folder3", + expected: "folder/folder1/folder2/folder3/item", + updated: true, + }, + { + name: "not parent", + item: "folder/folder1/folder2/item", + prev: "folder1", + cur: "new-folder1", + expected: "dummy", + updated: false, + }, + } + + buildPath := func(t *testing.T, pth string, isItem bool) path.Path { + pathBuilder := path.Builder{}.Append(strings.Split(pth, "/")...) + item, err := pathBuilder.ToDataLayerOneDrivePath("tenant", "user", isItem) + require.NoError(t, err, "err building path") + + return item + } + + for _, tc := range cases { + suite.T().Run(tc.name, func(t *testing.T) { + item := buildPath(t, tc.item, true) + prev := buildPath(t, tc.prev, false) + cur := buildPath(t, tc.cur, false) + expected := buildPath(t, tc.expected, true) + + updated := item.UpdateParent(prev, cur) + assert.Equal(t, tc.updated, updated, "path updated") + if tc.updated { + assert.Equal(t, expected, item, "modified path") + } + }) + } +}