diff --git a/src/internal/connector/graph_connector_helper_test.go b/src/internal/connector/graph_connector_helper_test.go index 5a5a8eb99..7c9dbcba7 100644 --- a/src/internal/connector/graph_connector_helper_test.go +++ b/src/internal/connector/graph_connector_helper_test.go @@ -704,11 +704,18 @@ func compareOneDriveItem( t *testing.T, expected map[string][]byte, item data.Stream, + dest control.RestoreDestination, restorePermissions bool, -) { +) bool { + // Skip OneDrive permissions in the folder that used to be the root. We don't + // have a good way to materialize these in the test right now. + if item.UUID() == dest.ContainerName+onedrive.DirMetaFileSuffix { + return false + } + buf, err := io.ReadAll(item.ToReader()) if !assert.NoError(t, err) { - return + return true } name := item.UUID() @@ -722,7 +729,7 @@ func compareOneDriveItem( err = json.Unmarshal(buf, &itemMeta) if !assert.NoErrorf(t, err, "unmarshalling retrieved metadata for file %s", name) { - return + return true } expectedData := expected[name] @@ -732,12 +739,12 @@ func compareOneDriveItem( "unexpected metadata file with name %s", name, ) { - return + return true } err = json.Unmarshal(expectedData, &expectedMeta) if !assert.NoError(t, err, "unmarshalling expected metadata") { - return + return true } // Only compare file names if we're using a version that expects them to be @@ -748,7 +755,7 @@ func compareOneDriveItem( if !restorePermissions { assert.Equal(t, 0, len(itemMeta.Permissions)) - return + return true } testElementsMatch( @@ -758,19 +765,19 @@ func compareOneDriveItem( permissionEqual, ) - return + return true } var fileData testOneDriveData err = json.Unmarshal(buf, &fileData) if !assert.NoErrorf(t, err, "unmarshalling file data for file %s", name) { - return + return true } expectedData := expected[fileData.FileName] if !assert.NotNil(t, expectedData, "unexpected file with name %s", name) { - return + return true } // OneDrive data items are just byte buffers of the data. Nothing special to @@ -779,16 +786,22 @@ func compareOneDriveItem( // Compare against the version with the file name embedded because that's what // the auto-generated expected data has. assert.Equal(t, expectedData, buf) + + return true } +// compareItem compares the data returned by backup with the expected data. +// Returns true if a comparison was done else false. Bool return is mostly used +// to exclude OneDrive permissions for the root right now. func compareItem( t *testing.T, expected map[string][]byte, service path.ServiceType, category path.CategoryType, item data.Stream, + dest control.RestoreDestination, restorePermissions bool, -) { +) bool { if mt, ok := item.(data.StreamModTime); ok { assert.NotZero(t, mt.ModTime()) } @@ -807,11 +820,13 @@ func compareItem( } case path.OneDriveService: - compareOneDriveItem(t, expected, item, restorePermissions) + return compareOneDriveItem(t, expected, item, dest, restorePermissions) default: assert.FailNowf(t, "unexpected service: %s", service.String()) } + + return true } func checkHasCollections( @@ -832,7 +847,7 @@ func checkHasCollections( gotNames = append(gotNames, g.FullPath().String()) } - assert.ElementsMatch(t, expectedNames, gotNames) + assert.ElementsMatch(t, expectedNames, gotNames, "returned collections") } //revive:disable:context-as-argument @@ -842,6 +857,7 @@ func checkCollections( expectedItems int, expected map[string]map[string][]byte, got []data.BackupCollection, + dest control.RestoreDestination, restorePermissions bool, ) int { //revive:enable:context-as-argument @@ -851,10 +867,12 @@ func checkCollections( gotItems := 0 for _, returned := range got { - startingItems := gotItems - service := returned.FullPath().Service() - category := returned.FullPath().Category() - expectedColData := expected[returned.FullPath().String()] + var ( + hasItems bool + service = returned.FullPath().Service() + category = returned.FullPath().Category() + expectedColData = expected[returned.FullPath().String()] + ) // Need to iterate through all items even if we don't expect to find a match // because otherwise we'll deadlock waiting for GC status. Unexpected or @@ -872,16 +890,19 @@ func checkCollections( continue } + hasItems = true gotItems++ if expectedColData == nil { continue } - compareItem(t, expectedColData, service, category, item, restorePermissions) + if !compareItem(t, expectedColData, service, category, item, dest, restorePermissions) { + gotItems-- + } } - if gotItems != startingItems { + if hasItems { collectionsWithItems = append(collectionsWithItems, returned) } } diff --git a/src/internal/connector/graph_connector_onedrive_test.go b/src/internal/connector/graph_connector_onedrive_test.go index 2efff2cb1..f0682cc00 100644 --- a/src/internal/connector/graph_connector_onedrive_test.go +++ b/src/internal/connector/graph_connector_onedrive_test.go @@ -173,7 +173,7 @@ func (c *onedriveCollection) withFile( name+onedrive.DataFileSuffix, fileData)) - case 1, 2, 3: + case version.OneDrive1DataAndMetaFiles, 2, version.OneDrive3IsMetaMarker, version.OneDrive4DirIncludesPermissions: c.items = append(c.items, onedriveItemWithData( c.t, name+onedrive.DataFileSuffix, @@ -202,10 +202,10 @@ func (c *onedriveCollection) withFolder( roles []string, ) *onedriveCollection { switch c.backupVersion { - case 0: + case 0, version.OneDrive4DirIncludesPermissions: return c - case 1, 2, 3: + case version.OneDrive1DataAndMetaFiles, 2, version.OneDrive3IsMetaMarker: c.items = append( c.items, onedriveMetadata( @@ -230,7 +230,7 @@ func (c *onedriveCollection) withPermissions( ) *onedriveCollection { // These versions didn't store permissions for the folder or didn't store them // in the folder's collection. - if c.backupVersion < version.OneDriveXIncludesPermissions { + if c.backupVersion < version.OneDrive4DirIncludesPermissions { return c } @@ -240,15 +240,15 @@ func (c *onedriveCollection) withPermissions( return c } - c.items = append( - c.items, - onedriveMetadata( - c.t, - name, - name+onedrive.DirMetaFileSuffix, - user, - roles), - ) + metadata := onedriveMetadata( + c.t, + name, + name+onedrive.DirMetaFileSuffix, + user, + roles) + + c.items = append(c.items, metadata) + c.aux = append(c.aux, metadata) return c } @@ -349,7 +349,7 @@ func (suite *GraphConnectorOneDriveIntegrationSuite) TestRestoreAndBackup_Multip table := []onedriveTest{ { name: "WithMetadata", - startVersion: 1, + startVersion: version.OneDrive1DataAndMetaFiles, cols: []onedriveColInfo{ { pathElements: rootPath, @@ -375,6 +375,10 @@ func (suite *GraphConnectorOneDriveIntegrationSuite) TestRestoreAndBackup_Multip }, { pathElements: folderBPath, + perms: permData{ + user: suite.secondaryUser, + roles: readPerm, + }, files: []itemData{ { name: fileName, @@ -516,7 +520,7 @@ func (suite *GraphConnectorOneDriveIntegrationSuite) TestPermissionsRestoreAndBa folderBName, } - startVersion := 1 + startVersion := version.OneDrive1DataAndMetaFiles table := []onedriveTest{ { @@ -731,7 +735,7 @@ func (suite *GraphConnectorOneDriveIntegrationSuite) TestPermissionsBackupAndNoR suite.user, ) - startVersion := 1 + startVersion := version.OneDrive1DataAndMetaFiles table := []onedriveTest{ { @@ -891,6 +895,12 @@ func (suite *GraphConnectorOneDriveIntegrationSuite) TestPermissionsRestoreAndNo "", nil, ). + // Call this to generate a meta file with the folder name that we can + // check. + withPermissions( + "", + nil, + ). collection(), }, } diff --git a/src/internal/connector/graph_connector_test.go b/src/internal/connector/graph_connector_test.go index 4d20c5b69..032a1a7f3 100644 --- a/src/internal/connector/graph_connector_test.go +++ b/src/internal/connector/graph_connector_test.go @@ -493,7 +493,14 @@ func runBackupAndCompare( // Pull the data prior to waiting for the status as otherwise it will // deadlock. - skipped := checkCollections(t, ctx, totalKopiaItems, expectedData, dcs, config.opts.RestorePermissions) + skipped := checkCollections( + t, + ctx, + totalKopiaItems, + expectedData, + dcs, + config.dest, + config.opts.RestorePermissions) status := backupGC.AwaitStatus() @@ -1000,7 +1007,15 @@ func (suite *GraphConnectorIntegrationSuite) TestMultiFolderBackupDifferentNames // Pull the data prior to waiting for the status as otherwise it will // deadlock. - skipped := checkCollections(t, ctx, allItems, allExpectedData, dcs, true) + skipped := checkCollections( + t, + ctx, + allItems, + allExpectedData, + dcs, + // Alright to be empty, needed for OneDrive. + control.RestoreDestination{}, + true) status := backupGC.AwaitStatus() assert.Equal(t, allItems+skipped, status.ObjectCount, "status.ObjectCount") diff --git a/src/internal/connector/onedrive/collections.go b/src/internal/connector/onedrive/collections.go index 2a1707a67..e82fbd645 100644 --- a/src/internal/connector/onedrive/collections.go +++ b/src/internal/connector/onedrive/collections.go @@ -13,6 +13,7 @@ import ( "github.com/pkg/errors" "golang.org/x/exp/maps" + "github.com/alcionai/corso/src/internal/common/ptr" "github.com/alcionai/corso/src/internal/connector/graph" "github.com/alcionai/corso/src/internal/connector/support" "github.com/alcionai/corso/src/internal/data" @@ -562,8 +563,26 @@ func (c *Collections) UpdateCollections( return err } + var ( + itemPath path.Path + isFolder = item.GetFolder() != nil || item.GetPackage() != nil + ) + + if item.GetDeleted() == nil { + name := ptr.Val(item.GetName()) + if len(name) == 0 { + return clues.New("non-deleted item with empty name").With("item_id", name) + } + + itemPath, err = collectionPath.Append(name, !isFolder) + if err != nil { + return err + } + } + // Skip items that don't match the folder selectors we were given. - if shouldSkipDrive(ctx, collectionPath, c.matcher, driveName) { + if shouldSkipDrive(ctx, itemPath, c.matcher, driveName) && + shouldSkipDrive(ctx, collectionPath, c.matcher, driveName) { logger.Ctx(ctx).Infof("Skipping path %s", collectionPath.String()) continue } @@ -584,13 +603,6 @@ func (c *Collections) UpdateCollections( // the deleted folder/package. delete(newPaths, *item.GetId()) - // TODO(meain): Directory metadata files should be - // moved into the directory instead of having a - // `.dirmeta` file at the same level as the - // directory. This way we can make sure it is moved - // and deleted along with the directory and don't have - // to be handled separately. - if prevPath == nil { // It is possible that an item was created and // deleted between two delta invocations. In @@ -616,51 +628,45 @@ func (c *Collections) UpdateCollections( break } - // Deletions of folders are handled in this case so we may as well start - // off by saving the path.Path of the item instead of just the OneDrive - // parentRef or such. - folderPath, err := collectionPath.Append(*item.GetName(), false) - if err != nil { - logger.Ctx(ctx).Errorw("failed building collection path", "error", err) - return err - } - // 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. - updatePath(newPaths, *item.GetId(), folderPath.String()) + updatePath(newPaths, *item.GetId(), itemPath.String()) - found, err := updateCollectionPaths(*item.GetId(), c.CollectionMap, folderPath) + found, err := updateCollectionPaths(*item.GetId(), c.CollectionMap, itemPath) 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++ - } + col := NewCollection( + c.itemClient, + itemPath, + prevPath, + driveID, + c.service, + c.statusUpdater, + c.source, + c.ctrl, + invalidPrevDelta, + ) + c.CollectionMap[*item.GetId()] = col + c.NumContainers++ } if c.source != OneDriveSource { continue } - fallthrough + if col := c.CollectionMap[*item.GetId()]; col != nil { + // Add an entry to fetch permissions into this collection. This assumes + // that OneDrive always returns all folders on the path of an item + // before the item. This seems to hold true for now at least. + collection := col.(*Collection) + if collection.Add(item) { + c.NumItems++ + } + } case item.GetFile() != nil: if !invalidPrevDelta && item.GetFile() != nil { @@ -702,6 +708,11 @@ func (c *Collections) UpdateCollections( col, found := c.CollectionMap[collectionID] if !found { + // TODO(ashmrtn): We should probably tighten the restrictions on this + // and just make it return an error if the collection doesn't already + // exist. Graph seems pretty consistent about returning all folders on + // the path from the root to the item in question. Removing this will + // also ensure we always add an entry to get the folder metadata. col = NewCollection( c.itemClient, collectionPath, @@ -739,14 +750,6 @@ func (c *Collections) UpdateCollections( if !removed { return clues.New("removing from prev collection").With("item_id", *item.GetId()) } - - // If that was the only item in that collection and is - // not getting added back, delete the collection - if itemColID != collectionID && - pcollection.IsEmpty() && - pcollection.State() == data.NewState { - delete(c.CollectionMap, itemColID) - } } itemCollection[*item.GetId()] = collectionID @@ -754,11 +757,7 @@ func (c *Collections) UpdateCollections( if collection.Add(item) { c.NumItems++ - if item.GetFile() != nil { - // This is necessary as we have a fallthrough for - // folders and packages - c.NumFiles++ - } + c.NumFiles++ } default: @@ -770,6 +769,10 @@ func (c *Collections) UpdateCollections( } func shouldSkipDrive(ctx context.Context, drivePath path.Path, m folderMatcher, driveName string) bool { + if drivePath == nil { + return false + } + return !includePath(ctx, m, drivePath) || (drivePath.Category() == path.LibrariesCategory && restrictedDirectory == driveName) } diff --git a/src/internal/connector/onedrive/collections_test.go b/src/internal/connector/onedrive/collections_test.go index 40bea9c28..d8f4df60d 100644 --- a/src/internal/connector/onedrive/collections_test.go +++ b/src/internal/connector/onedrive/collections_test.go @@ -222,7 +222,7 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { scope: anyFolder, expect: assert.NoError, expectedCollectionIDs: map[string]statePath{ - "root": expectedStatePath(data.NotMovedState, ""), + "folder": expectedStatePath(data.NewState, folder), }, expectedMetadataPaths: map[string]string{ "root": expectedPath(""), @@ -242,7 +242,7 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { scope: anyFolder, expect: assert.NoError, expectedCollectionIDs: map[string]statePath{ - "root": expectedStatePath(data.NotMovedState, ""), + "package": expectedStatePath(data.NewState, pkg), }, expectedMetadataPaths: map[string]string{ "root": expectedPath(""), @@ -301,15 +301,16 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { "subfolder": expectedStatePath(data.NewState, folderSub), "folder2": expectedStatePath(data.NewState, folderSub+folder), }, - expectedItemCount: 4, + expectedItemCount: 5, expectedFileCount: 2, expectedContainerCount: 3, // just "folder" isn't added here because the include check is done on the // parent path since we only check later if something is a folder or not. expectedMetadataPaths: map[string]string{ "root": expectedPath(""), - "subfolder": expectedPath("/folder/subfolder"), - "folder2": expectedPath("/folder/subfolder/folder"), + "folder": expectedPath(folder), + "subfolder": expectedPath(folderSub), + "folder2": expectedPath(folderSub + folder), }, expectedExcludes: getDelList("fileInFolder", "fileInFolder2"), }, @@ -334,12 +335,13 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { "subfolder": expectedStatePath(data.NewState, folderSub), "folder2": expectedStatePath(data.NewState, folderSub+folder), }, - expectedItemCount: 2, + expectedItemCount: 3, expectedFileCount: 1, expectedContainerCount: 2, expectedMetadataPaths: map[string]string{ - "root": expectedPath(""), - "folder2": expectedPath("/folder/subfolder/folder"), + "root": expectedPath(""), + "subfolder": expectedPath(folderSub), + "folder2": expectedPath(folderSub + folder), }, expectedExcludes: getDelList("fileInFolder2"), }, @@ -361,12 +363,13 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { expectedCollectionIDs: map[string]statePath{ "subfolder": expectedStatePath(data.NewState, folderSub), }, - expectedItemCount: 1, + expectedItemCount: 2, expectedFileCount: 1, expectedContainerCount: 1, // No child folders for subfolder so nothing here. expectedMetadataPaths: map[string]string{ - "root": expectedPath(""), + "root": expectedPath(""), + "subfolder": expectedPath(folderSub), }, expectedExcludes: getDelList("fileInSubfolder"), }, @@ -377,22 +380,21 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { driveItem("folder", "folder", testBaseDrivePath, "root", false, true, false), }, inputFolderMap: map[string]string{ - "folder": expectedPath("/folder"), - "subfolder": expectedPath("/folder/subfolder"), + "folder": expectedPath(folder), + "subfolder": expectedPath(folderSub), }, scope: anyFolder, expect: assert.NoError, expectedCollectionIDs: map[string]statePath{ - "root": expectedStatePath(data.NotMovedState, ""), - "folder": expectedStatePath(data.NotMovedState, "/folder"), + "folder": expectedStatePath(data.NotMovedState, folder), }, expectedItemCount: 1, expectedFileCount: 0, - expectedContainerCount: 2, + expectedContainerCount: 1, expectedMetadataPaths: map[string]string{ "root": expectedPath(""), - "folder": expectedPath("/folder"), - "subfolder": expectedPath("/folder/subfolder"), + "folder": expectedPath(folder), + "subfolder": expectedPath(folderSub), }, expectedExcludes: map[string]struct{}{}, }, @@ -409,16 +411,15 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { scope: anyFolder, expect: assert.NoError, expectedCollectionIDs: map[string]statePath{ - "root": expectedStatePath(data.NotMovedState, ""), - "folder": expectedStatePath(data.MovedState, "/folder", "/a-folder"), + "folder": expectedStatePath(data.MovedState, folder, "/a-folder"), }, expectedItemCount: 1, expectedFileCount: 0, - expectedContainerCount: 2, + expectedContainerCount: 1, expectedMetadataPaths: map[string]string{ "root": expectedPath(""), - "folder": expectedPath("/folder"), - "subfolder": expectedPath("/folder/subfolder"), + "folder": expectedPath(folder), + "subfolder": expectedPath(folderSub), }, expectedExcludes: map[string]struct{}{}, }, @@ -430,20 +431,19 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { driveItem("folder", "folder", testBaseDrivePath, "root", false, true, false), }, inputFolderMap: map[string]string{ - "folder": expectedPath("/folder"), + "folder": expectedPath(folder), }, scope: anyFolder, expect: assert.NoError, expectedCollectionIDs: map[string]statePath{ - "root": expectedStatePath(data.NotMovedState, ""), - "folder": expectedStatePath(data.NotMovedState, "/folder"), + "folder": expectedStatePath(data.NotMovedState, folder), }, expectedItemCount: 2, expectedFileCount: 1, - expectedContainerCount: 2, + expectedContainerCount: 1, expectedMetadataPaths: map[string]string{ "root": expectedPath(""), - "folder": expectedPath("/folder"), + "folder": expectedPath(folder), }, expectedExcludes: getDelList("file"), }, @@ -459,12 +459,11 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { 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 + expectedItemCount: 2, expectedFileCount: 1, - expectedContainerCount: 2, + expectedContainerCount: 1, expectedMetadataPaths: map[string]string{ "root": expectedPath(""), "folder": expectedPath("/folder2"), @@ -482,15 +481,14 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { scope: anyFolder, expect: assert.NoError, expectedCollectionIDs: map[string]statePath{ - "root": expectedStatePath(data.NotMovedState, ""), - "folder": expectedStatePath(data.NewState, "/folder"), + "folder": expectedStatePath(data.NewState, folder), }, expectedItemCount: 2, expectedFileCount: 1, - expectedContainerCount: 2, + expectedContainerCount: 1, expectedMetadataPaths: map[string]string{ "root": expectedPath(""), - "folder": expectedPath("/folder"), + "folder": expectedPath(folder), }, expectedExcludes: getDelList("file"), }, @@ -508,16 +506,15 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { scope: anyFolder, expect: assert.NoError, expectedCollectionIDs: map[string]statePath{ - "root": expectedStatePath(data.NotMovedState, ""), - "folder": expectedStatePath(data.MovedState, "/folder", "/a-folder"), + "folder": expectedStatePath(data.MovedState, folder, "/a-folder"), "subfolder": expectedStatePath(data.MovedState, "/subfolder", "/a-folder/subfolder"), }, expectedItemCount: 2, expectedFileCount: 0, - expectedContainerCount: 3, + expectedContainerCount: 2, expectedMetadataPaths: map[string]string{ "root": expectedPath(""), - "folder": expectedPath("/folder"), + "folder": expectedPath(folder), "subfolder": expectedPath("/subfolder"), }, expectedExcludes: map[string]struct{}{}, @@ -536,16 +533,15 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { scope: anyFolder, expect: assert.NoError, expectedCollectionIDs: map[string]statePath{ - "root": expectedStatePath(data.NotMovedState, ""), - "folder": expectedStatePath(data.MovedState, "/folder", "/a-folder"), + "folder": expectedStatePath(data.MovedState, folder, "/a-folder"), "subfolder": expectedStatePath(data.MovedState, "/subfolder", "/a-folder/subfolder"), }, expectedItemCount: 2, expectedFileCount: 0, - expectedContainerCount: 3, + expectedContainerCount: 2, expectedMetadataPaths: map[string]string{ "root": expectedPath(""), - "folder": expectedPath("/folder"), + "folder": expectedPath(folder), "subfolder": expectedPath("/subfolder"), }, expectedExcludes: map[string]struct{}{}, @@ -556,6 +552,9 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { driveRootItem("root"), driveItem("folder2", "folder2", testBaseDrivePath, "root", false, true, false), driveItem("itemInFolder2", "itemInFolder2", testBaseDrivePath+"/folder2", "folder2", true, false, false), + // Need to see the parent folder first (expected since that's what Graph + // consistently returns). + driveItem("folder", "a-folder", testBaseDrivePath, "root", false, true, false), driveItem("subfolder", "subfolder", testBaseDrivePath+"/a-folder", "folder", false, true, false), driveItem( "itemInSubfolder", @@ -575,14 +574,13 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { scope: anyFolder, expect: assert.NoError, expectedCollectionIDs: map[string]statePath{ - "root": expectedStatePath(data.NotMovedState, ""), - "folder": expectedStatePath(data.MovedState, "/folder", "/a-folder"), + "folder": expectedStatePath(data.MovedState, folder, "/a-folder"), "folder2": expectedStatePath(data.NewState, "/folder2"), - "subfolder": expectedStatePath(data.MovedState, "/folder/subfolder", "/a-folder/subfolder"), + "subfolder": expectedStatePath(data.MovedState, folderSub, "/a-folder/subfolder"), }, expectedItemCount: 5, expectedFileCount: 2, - expectedContainerCount: 4, + expectedContainerCount: 3, expectedMetadataPaths: map[string]string{ "root": expectedPath(""), "folder": expectedPath("/folder"), @@ -606,12 +604,11 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { scope: anyFolder, expect: assert.NoError, expectedCollectionIDs: map[string]statePath{ - "root": expectedStatePath(data.NotMovedState, ""), "folder": expectedStatePath(data.MovedState, "/folder2", "/a-folder"), }, - expectedItemCount: 3, + expectedItemCount: 2, expectedFileCount: 1, - expectedContainerCount: 2, + expectedContainerCount: 1, expectedMetadataPaths: map[string]string{ "root": expectedPath(""), "folder": expectedPath("/folder2"), @@ -680,13 +677,12 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { scope: anyFolder, expect: assert.NoError, expectedCollectionIDs: map[string]statePath{ - "root": expectedStatePath(data.NotMovedState, ""), "folder": expectedStatePath(data.DeletedState, folder), - "subfolder": expectedStatePath(data.MovedState, "/subfolder", "/folder/subfolder"), + "subfolder": expectedStatePath(data.MovedState, "/subfolder", folderSub), }, expectedItemCount: 1, expectedFileCount: 0, - expectedContainerCount: 2, + expectedContainerCount: 1, expectedMetadataPaths: map[string]string{ "root": expectedPath(""), "subfolder": expectedPath("/subfolder"), @@ -752,7 +748,11 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { assert.Equal(t, tt.expectedContainerCount, c.NumContainers, "container count") for id, sp := range tt.expectedCollectionIDs { - assert.Containsf(t, c.CollectionMap, id, "contains collection with id %s", id) + if !assert.Containsf(t, c.CollectionMap, id, "missing collection with id %s", id) { + // Skip collections we don't find so we don't get an NPE. + continue + } + 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) @@ -1299,8 +1299,7 @@ func (suite *OneDriveCollectionsSuite) TestGet() { driveID1: {}, }, expectedCollections: map[string]map[data.CollectionState][]string{ - folderPath1: {data.NewState: {"file"}}, - rootFolderPath1: {data.NotMovedState: {"folder"}}, + folderPath1: {data.NewState: {"folder", "file"}}, }, expectedDeltaURLs: map[string]string{ driveID1: delta, @@ -1334,8 +1333,7 @@ func (suite *OneDriveCollectionsSuite) TestGet() { driveID1: {}, }, expectedCollections: map[string]map[data.CollectionState][]string{ - folderPath1: {data.NewState: {"file"}}, - rootFolderPath1: {data.NotMovedState: {"folder"}}, + folderPath1: {data.NewState: {"folder", "file"}}, }, expectedDeltaURLs: map[string]string{ driveID1: delta, @@ -1369,7 +1367,8 @@ func (suite *OneDriveCollectionsSuite) TestGet() { driveID1: {}, }, expectedCollections: map[string]map[data.CollectionState][]string{ - rootFolderPath1: {data.NotMovedState: {"folder", "file"}}, + rootFolderPath1: {data.NotMovedState: {"file"}}, + folderPath1: {data.NewState: {"folder"}}, }, expectedDeltaURLs: map[string]string{ driveID1: delta, @@ -1402,8 +1401,7 @@ func (suite *OneDriveCollectionsSuite) TestGet() { driveID1: {}, }, expectedCollections: map[string]map[data.CollectionState][]string{ - folderPath1: {data.NewState: {"file"}}, - rootFolderPath1: {data.NotMovedState: {"folder"}}, + folderPath1: {data.NewState: {"folder", "file"}}, }, expectedDeltaURLs: map[string]string{}, expectedFolderPaths: map[string]map[string]string{}, @@ -1437,8 +1435,7 @@ func (suite *OneDriveCollectionsSuite) TestGet() { driveID1: {}, }, expectedCollections: map[string]map[data.CollectionState][]string{ - folderPath1: {data.NewState: {"file", "file2"}}, - rootFolderPath1: {data.NotMovedState: {"folder"}}, + folderPath1: {data.NewState: {"folder", "file", "file2"}}, }, expectedDeltaURLs: map[string]string{ driveID1: delta, @@ -1485,10 +1482,8 @@ func (suite *OneDriveCollectionsSuite) TestGet() { driveID2: {}, }, expectedCollections: map[string]map[data.CollectionState][]string{ - folderPath1: {data.NewState: {"file"}}, - folderPath2: {data.NewState: {"file2"}}, - rootFolderPath1: {data.NotMovedState: {"folder"}}, - rootFolderPath2: {data.NotMovedState: {"folder2"}}, + folderPath1: {data.NewState: {"folder", "file"}}, + folderPath2: {data.NewState: {"folder2", "file2"}}, }, expectedDeltaURLs: map[string]string{ driveID1: delta, @@ -1584,8 +1579,8 @@ func (suite *OneDriveCollectionsSuite) TestGet() { }, errCheck: assert.NoError, expectedCollections: map[string]map[data.CollectionState][]string{ - expectedPath1(""): {data.NotMovedState: {"file", "folder"}}, - expectedPath1("/folder"): {data.NewState: {"file2"}}, + expectedPath1(""): {data.NotMovedState: {"file"}}, + expectedPath1("/folder"): {data.NewState: {"folder", "file2"}}, }, expectedDeltaURLs: map[string]string{ driveID1: delta, @@ -1626,8 +1621,8 @@ func (suite *OneDriveCollectionsSuite) TestGet() { driveID1: {}, }, expectedCollections: map[string]map[data.CollectionState][]string{ - expectedPath1(""): {data.NotMovedState: {"file", "folder"}}, - expectedPath1("/folder"): {data.NewState: {"file2"}}, + expectedPath1(""): {data.NotMovedState: {"file"}}, + expectedPath1("/folder"): {data.NewState: {"folder", "file2"}}, }, expectedDeltaURLs: map[string]string{ driveID1: delta, @@ -1667,9 +1662,8 @@ func (suite *OneDriveCollectionsSuite) TestGet() { }, }, expectedCollections: map[string]map[data.CollectionState][]string{ - expectedPath1(""): {data.NotMovedState: {"folder2"}}, expectedPath1("/folder"): {data.DeletedState: {}}, - expectedPath1("/folder2"): {data.NewState: {"file"}}, + expectedPath1("/folder2"): {data.NewState: {"folder2", "file"}}, }, expectedDeltaURLs: map[string]string{ driveID1: delta, @@ -1709,8 +1703,7 @@ func (suite *OneDriveCollectionsSuite) TestGet() { }, }, expectedCollections: map[string]map[data.CollectionState][]string{ - expectedPath1(""): {data.NotMovedState: {"folder2"}}, - expectedPath1("/folder"): {data.NewState: {"file"}}, + expectedPath1("/folder"): {data.NewState: {"folder2", "file"}}, }, expectedDeltaURLs: map[string]string{ driveID1: delta, @@ -1842,9 +1835,9 @@ func (suite *OneDriveCollectionsSuite) TestGet() { t, test.expectedCollections[folderPath][baseCol.State()], itemIDs, - "items in collection %s", - folderPath, - ) + "state: %d, path: %s", + baseCol.State(), + folderPath) assert.Equal(t, test.doNotMergeItems, baseCol.DoNotMergeItems(), "DoNotMergeItems") } diff --git a/src/internal/connector/onedrive/permission.go b/src/internal/connector/onedrive/permission.go index b5527f5c1..814a99c94 100644 --- a/src/internal/connector/onedrive/permission.go +++ b/src/internal/connector/onedrive/permission.go @@ -4,11 +4,14 @@ import ( "context" "github.com/alcionai/clues" - "github.com/alcionai/corso/src/internal/connector/graph" - "github.com/alcionai/corso/src/pkg/path" msdrive "github.com/microsoftgraph/msgraph-sdk-go/drive" "github.com/microsoftgraph/msgraph-sdk-go/models" "github.com/pkg/errors" + + "github.com/alcionai/corso/src/internal/connector/graph" + "github.com/alcionai/corso/src/internal/data" + "github.com/alcionai/corso/src/internal/version" + "github.com/alcionai/corso/src/pkg/path" ) func getParentPermissions( @@ -33,16 +36,23 @@ func getParentPermissions( } func getParentAndCollectionPermissions( + ctx context.Context, drivePath *path.DrivePath, - collectionPath path.Path, + dc data.RestoreCollection, permissions map[string][]UserPermission, + backupVersion int, restorePerms bool, ) ([]UserPermission, []UserPermission, error) { - if !restorePerms { + if !restorePerms || backupVersion < version.OneDrive1DataAndMetaFiles { return nil, nil, nil } - var parentPerms []UserPermission + var ( + err error + parentPerms []UserPermission + colPerms []UserPermission + collectionPath = dc.FullPath() + ) // Only get parent permissions if we're not restoring the root. if len(drivePath.Folders) > 0 { @@ -57,11 +67,24 @@ func getParentAndCollectionPermissions( } } - // TODO(ashmrtn): For versions after this pull the permissions from the - // current collection with Fetch(). - colPerms, err := getParentPermissions(collectionPath, permissions) - if err != nil { - return nil, nil, clues.Wrap(err, "getting collection permissions") + if backupVersion < version.OneDrive4DirIncludesPermissions { + colPerms, err = getParentPermissions(collectionPath, permissions) + if err != nil { + return nil, nil, clues.Wrap(err, "getting collection permissions") + } + } else if len(drivePath.Folders) > 0 { + // Root folder doesn't have a metadata file associated with it. + folders := collectionPath.Folders() + + meta, err := fetchAndReadMetadata( + ctx, + dc, + folders[len(folders)-1]+DirMetaFileSuffix) + if err != nil { + return nil, nil, clues.Wrap(err, "collection permissions") + } + + colPerms = meta.Permissions } return parentPerms, colPerms, nil diff --git a/src/internal/connector/onedrive/restore.go b/src/internal/connector/onedrive/restore.go index 36d2a5baf..759edb53b 100644 --- a/src/internal/connector/onedrive/restore.go +++ b/src/internal/connector/onedrive/restore.go @@ -170,9 +170,11 @@ func RestoreCollection( logger.Ctx(ctx).Info("restoring onedrive collection") parentPerms, colPerms, err := getParentAndCollectionPermissions( + ctx, drivePath, - dc.FullPath(), + dc, parentPermissions, + backupVersion, restorePerms) if err != nil { return metrics, folderPerms, permissionIDMappings, clues.Wrap(err, "getting permissions").WithClues(ctx) @@ -278,7 +280,10 @@ func RestoreCollection( // RestoreOp, so we still need to handle them in some way. continue } else if strings.HasSuffix(name, DirMetaFileSuffix) { - if !restorePerms { + // Only the version.OneDrive1DataAndMetaFiles needed to deserialize the + // permission for child folders here. Later versions can request + // permissions inline when processing the collection. + if !restorePerms || backupVersion >= version.OneDrive4DirIncludesPermissions { continue } diff --git a/src/internal/version/backup.go b/src/internal/version/backup.go index 25b719fd9..a3afdc8c1 100644 --- a/src/internal/version/backup.go +++ b/src/internal/version/backup.go @@ -2,7 +2,7 @@ package version import "math" -const Backup = 3 +const Backup = 4 // Various labels to refer to important version changes. // Labels don't need 1:1 service:version representation. Add a new @@ -21,12 +21,9 @@ const ( // specifies if the file is a meta file or a data file. OneDrive3IsMetaMarker = 3 - // OneDrive4IncludesPermissions includes permissions in the backup. - // Note that this is larger than the current backup version. That's - // because it isn't implemented yet. But we have tests based on this, - // so maybe we just keep bumping the verson ahead of the backup until - // it gets implemented. - OneDriveXIncludesPermissions = Backup + 1 + // OneDrive4IncludesPermissions includes permissions for folders in the same + // collection as the folder itself. + OneDrive4DirIncludesPermissions = 4 // OneDriveXNameInMeta points to the backup format version where we begin // storing files in kopia with their item ID instead of their OneDrive file