Deletion related fixes for OneDrive delta incrementals (#2518)

## Description

- parentReference for deleted items will not have path.
- GC was not responding with deleted state
- Deleted items are not streamed by kopia

Borrows some changes from https://github.com/alcionai/corso/pull/2503/files as that might be delayed with on merge. Will rebase that if this gets merged fist.

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

- [ ]  Yes, it's included
- [x] 🕐 Yes, but in a later PR
- [ ]  No 

## Type of change

<!--- Please check the type of change your PR introduces: --->
- [x] 🌻 Feature
- [ ] 🐛 Bugfix
- [ ] 🗺️ Documentation
- [ ] 🤖 Test
- [ ] 💻 CI/Deployment
- [ ] 🧹 Tech Debt/Cleanup

## Issue(s)

<!-- Can reference multiple issues. Use one of the following "magic words" - "closes, fixes" to auto-close the Github issue. -->
* https://github.com/alcionai/corso/issues/2117
* fixes https://github.com/alcionai/corso/issues/2517

## Test Plan

<!-- How will this be tested prior to merging.-->
- [ ] 💪 Manual
- [x]  Unit test
- [ ] 💚 E2E
This commit is contained in:
Abin Simon 2023-02-16 09:49:50 +05:30 committed by GitHub
parent 476a931ccb
commit 55825afdbd
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 153 additions and 55 deletions

View File

@ -225,8 +225,11 @@ func (gc *GraphConnector) OneDriveDataCollections(
maps.Copy(allExcludes, excludes)
}
for range collections {
gc.incrementAwaitingMessages()
for _, c := range collections {
if c.State() != data.DeletedState {
// kopia doesn't stream Items() from deleted collections
gc.incrementAwaitingMessages()
}
}
return collections, allExcludes, errs

View File

@ -8,11 +8,11 @@ import (
"net/http"
"strings"
"github.com/alcionai/clues"
"github.com/microsoftgraph/msgraph-sdk-go/models"
"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"
@ -256,7 +256,7 @@ func (c *Collections) Get(
ctx context.Context,
prevMetadata []data.RestoreCollection,
) ([]data.BackupCollection, map[string]struct{}, error) {
prevDeltas, _, err := deserializeMetadata(ctx, prevMetadata)
prevDeltas, oldPathsByDriveID, err := deserializeMetadata(ctx, prevMetadata)
if err != nil {
return nil, nil, err
}
@ -293,6 +293,7 @@ func (c *Collections) Get(
driveName := *d.GetName()
prevDelta := prevDeltas[driveID]
oldPaths := oldPathsByDriveID[driveID]
delta, paths, excluded, err := collectItems(
ctx,
@ -304,6 +305,7 @@ func (c *Collections) Get(
driveID,
driveName,
c.UpdateCollections,
oldPaths,
prevDelta,
)
if err != nil {
@ -433,24 +435,56 @@ func (c *Collections) UpdateCollections(
var (
prevPath path.Path
prevCollectionPath path.Path
ok bool
)
if item.GetRoot() != nil {
// Skip the root item
rootPath, err := GetCanonicalPath(
fmt.Sprintf(rootDrivePattern, driveID),
c.tenant,
c.resourceOwner,
c.source,
)
if err != nil {
return err
}
// Add root path so as to have root folder information in
// path metadata. Root id -> path map is necessary in
// following delta incremental backups.
updatePath(newPaths, *item.GetId(), rootPath.String())
continue
}
if item.GetParentReference() == nil ||
item.GetParentReference().GetPath() == nil ||
item.GetParentReference().GetId() == nil {
return errors.Errorf("item does not have a parent reference. item name : %s", *item.GetName())
item.GetParentReference().GetId() == nil ||
(item.GetDeleted() == nil && item.GetParentReference().GetPath() == nil) {
err := clues.New("no parent reference").With("item_id", *item.GetId())
if item.GetName() != nil {
err = err.With("item_name", *item.GetName())
}
return err
}
// Create a collection for the parent of this item
collectionID := *item.GetParentReference().GetId()
var collectionPathStr string
if item.GetDeleted() == nil {
collectionPathStr = *item.GetParentReference().GetPath()
} else {
collectionPathStr, ok = oldPaths[*item.GetParentReference().GetId()]
if !ok {
// This collection was created and destroyed in
// between the current and previous invocation
continue
}
}
collectionPath, err := GetCanonicalPath(
*item.GetParentReference().GetPath(),
collectionPathStr,
c.tenant,
c.resourceOwner,
c.source,

View File

@ -171,11 +171,13 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
driveRootItem("root"),
driveItem("item", "item", testBaseDrivePath, "root", false, false, false),
},
inputFolderMap: map[string]string{},
scope: anyFolder,
expect: assert.Error,
expectedMetadataPaths: map[string]string{},
expectedExcludes: map[string]struct{}{},
inputFolderMap: map[string]string{},
scope: anyFolder,
expect: assert.Error,
expectedMetadataPaths: map[string]string{
"root": expectedPath(""),
},
expectedExcludes: map[string]struct{}{},
},
{
testCase: "Single File",
@ -193,8 +195,10 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
expectedFileCount: 1,
expectedContainerCount: 1,
// Root folder is skipped since it's always present.
expectedMetadataPaths: map[string]string{},
expectedExcludes: map[string]struct{}{"file": {}},
expectedMetadataPaths: map[string]string{
"root": expectedPath(""),
},
expectedExcludes: map[string]struct{}{"file": {}},
},
{
testCase: "Single Folder",
@ -209,6 +213,7 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
"root": expectedStatePath(data.NotMovedState, ""),
},
expectedMetadataPaths: map[string]string{
"root": expectedPath(""),
"folder": expectedPath("/folder"),
},
expectedItemCount: 1,
@ -228,6 +233,7 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
"root": expectedStatePath(data.NotMovedState, ""),
},
expectedMetadataPaths: map[string]string{
"root": expectedPath(""),
"package": expectedPath("/package"),
},
expectedItemCount: 1,
@ -256,6 +262,7 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
expectedFileCount: 3,
expectedContainerCount: 3,
expectedMetadataPaths: map[string]string{
"root": expectedPath(""),
"folder": expectedPath("/folder"),
"package": expectedPath("/package"),
},
@ -288,6 +295,7 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
// 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"),
},
@ -318,6 +326,7 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
expectedFileCount: 1,
expectedContainerCount: 2,
expectedMetadataPaths: map[string]string{
"root": expectedPath(""),
"folder2": expectedPath("/folder/subfolder/folder"),
},
expectedExcludes: map[string]struct{}{"fileInFolder2": {}},
@ -344,8 +353,10 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
expectedFileCount: 1,
expectedContainerCount: 1,
// No child folders for subfolder so nothing here.
expectedMetadataPaths: map[string]string{},
expectedExcludes: map[string]struct{}{"fileInSubfolder": {}},
expectedMetadataPaths: map[string]string{
"root": expectedPath(""),
},
expectedExcludes: map[string]struct{}{"fileInSubfolder": {}},
},
{
testCase: "not moved folder tree",
@ -367,6 +378,7 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
expectedFileCount: 0,
expectedContainerCount: 2,
expectedMetadataPaths: map[string]string{
"root": expectedPath(""),
"folder": expectedPath("/folder"),
"subfolder": expectedPath("/folder/subfolder"),
},
@ -392,6 +404,7 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
expectedFileCount: 0,
expectedContainerCount: 2,
expectedMetadataPaths: map[string]string{
"root": expectedPath(""),
"folder": expectedPath("/folder"),
"subfolder": expectedPath("/folder/subfolder"),
},
@ -417,6 +430,7 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
expectedFileCount: 1,
expectedContainerCount: 2,
expectedMetadataPaths: map[string]string{
"root": expectedPath(""),
"folder": expectedPath("/folder"),
},
expectedExcludes: map[string]struct{}{"file": {}},
@ -440,6 +454,7 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
expectedFileCount: 1,
expectedContainerCount: 2,
expectedMetadataPaths: map[string]string{
"root": expectedPath(""),
"folder": expectedPath("/folder2"),
},
expectedExcludes: map[string]struct{}{"file": {}},
@ -462,6 +477,7 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
expectedFileCount: 1,
expectedContainerCount: 2,
expectedMetadataPaths: map[string]string{
"root": expectedPath(""),
"folder": expectedPath("/folder"),
},
expectedExcludes: map[string]struct{}{"file": {}},
@ -488,6 +504,7 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
expectedFileCount: 0,
expectedContainerCount: 3,
expectedMetadataPaths: map[string]string{
"root": expectedPath(""),
"folder": expectedPath("/folder"),
"subfolder": expectedPath("/subfolder"),
},
@ -515,6 +532,7 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
expectedFileCount: 0,
expectedContainerCount: 3,
expectedMetadataPaths: map[string]string{
"root": expectedPath(""),
"folder": expectedPath("/folder"),
"subfolder": expectedPath("/subfolder"),
},
@ -554,6 +572,7 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
expectedFileCount: 2,
expectedContainerCount: 4,
expectedMetadataPaths: map[string]string{
"root": expectedPath(""),
"folder": expectedPath("/folder"),
"folder2": expectedPath("/folder2"),
"subfolder": expectedPath("/folder/subfolder"),
@ -568,6 +587,7 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
delItem("package", testBaseDrivePath, "root", false, false, true),
},
inputFolderMap: map[string]string{
"root": expectedPath(""),
"folder": expectedPath("/folder"),
"package": expectedPath("/package"),
},
@ -580,8 +600,10 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
expectedItemCount: 0,
expectedFileCount: 0,
expectedContainerCount: 0,
expectedMetadataPaths: map[string]string{},
expectedExcludes: map[string]struct{}{},
expectedMetadataPaths: map[string]string{
"root": expectedPath(""),
},
expectedExcludes: map[string]struct{}{},
},
{
testCase: "delete folder without previous",
@ -589,15 +611,19 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
driveRootItem("root"),
delItem("folder", testBaseDrivePath, "root", false, true, false),
},
inputFolderMap: map[string]string{},
inputFolderMap: map[string]string{
"root": expectedPath(""),
},
scope: anyFolder,
expect: assert.NoError,
expectedCollectionIDs: map[string]statePath{},
expectedItemCount: 0,
expectedFileCount: 0,
expectedContainerCount: 0,
expectedMetadataPaths: map[string]string{},
expectedExcludes: map[string]struct{}{},
expectedMetadataPaths: map[string]string{
"root": expectedPath(""),
},
expectedExcludes: map[string]struct{}{},
},
{
testCase: "delete folder tree move subfolder",
@ -607,6 +633,7 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
driveItem("subfolder", "subfolder", testBaseDrivePath, "root", false, true, false),
},
inputFolderMap: map[string]string{
"root": expectedPath(""),
"folder": expectedPath("/folder"),
"subfolder": expectedPath("/folder/subfolder"),
},
@ -621,6 +648,7 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
expectedFileCount: 0,
expectedContainerCount: 2,
expectedMetadataPaths: map[string]string{
"root": expectedPath(""),
"subfolder": expectedPath("/subfolder"),
},
expectedExcludes: map[string]struct{}{},
@ -630,13 +658,17 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
items: []models.DriveItemable{
delItem("item", testBaseDrivePath, "root", true, false, false),
},
inputFolderMap: map[string]string{},
inputFolderMap: map[string]string{
"root": expectedPath(""),
},
scope: anyFolder,
expect: assert.NoError,
expectedItemCount: 1,
expectedFileCount: 1,
expectedContainerCount: 0,
expectedMetadataPaths: map[string]string{},
expectedMetadataPaths: map[string]string{
"root": expectedPath(""),
},
expectedExcludes: map[string]struct{}{
"item": {},
},
@ -1135,10 +1167,11 @@ func (suite *OneDriveCollectionsSuite) TestGet() {
folderPath2 := expectedPath2("/folder")
table := []struct {
name string
drives []models.Driveable
items map[string][]deltaPagerResult
errCheck assert.ErrorAssertionFunc
name string
drives []models.Driveable
items map[string][]deltaPagerResult
errCheck assert.ErrorAssertionFunc
prevFolderPaths map[string]map[string]string
// Collection name -> set of item IDs. We can't check item data because
// that's not mocked out. Metadata is checked separately.
expectedCollections map[string]map[data.CollectionState][]string
@ -1161,15 +1194,16 @@ func (suite *OneDriveCollectionsSuite) TestGet() {
},
},
},
errCheck: assert.NoError,
errCheck: assert.NoError,
prevFolderPaths: map[string]map[string]string{
driveID1: {"root": rootFolderPath1},
},
expectedCollections: map[string]map[data.CollectionState][]string{},
expectedDeltaURLs: map[string]string{
driveID1: delta,
},
expectedFolderPaths: map[string]map[string]string{
// We need an empty map here so deserializing metadata knows the delta
// token for this drive is valid.
driveID1: {},
driveID1: {"root": rootFolderPath1},
},
expectedDelList: map[string]struct{}{
"file": {},
@ -1190,6 +1224,9 @@ func (suite *OneDriveCollectionsSuite) TestGet() {
},
},
errCheck: assert.NoError,
prevFolderPaths: map[string]map[string]string{
driveID1: {"root": expectedPath1("")},
},
expectedCollections: map[string]map[data.CollectionState][]string{
expectedPath1(""): {data.NotMovedState: {"file"}},
},
@ -1197,9 +1234,7 @@ func (suite *OneDriveCollectionsSuite) TestGet() {
driveID1: delta,
},
expectedFolderPaths: map[string]map[string]string{
// We need an empty map here so deserializing metadata knows the delta
// token for this drive is valid.
driveID1: {},
driveID1: {"root": rootFolderPath1},
},
expectedDelList: map[string]struct{}{"file": {}},
},
@ -1219,6 +1254,9 @@ func (suite *OneDriveCollectionsSuite) TestGet() {
},
},
errCheck: assert.NoError,
prevFolderPaths: map[string]map[string]string{
driveID1: {},
},
expectedCollections: map[string]map[data.CollectionState][]string{
folderPath1: {data.NewState: {"file"}},
rootFolderPath1: {data.NotMovedState: {"folder"}},
@ -1228,6 +1266,7 @@ func (suite *OneDriveCollectionsSuite) TestGet() {
},
expectedFolderPaths: map[string]map[string]string{
driveID1: {
"root": rootFolderPath1,
"folder": folderPath1,
},
},
@ -1244,11 +1283,14 @@ func (suite *OneDriveCollectionsSuite) TestGet() {
driveItem("folder", "folder", driveBasePath1, "root", false, true, false),
driveItem("file", "file", driveBasePath1+"/folder", "folder", true, false, false),
},
deltaLink: &empty,
deltaLink: &empty, // probably will never happen with graph
},
},
},
errCheck: assert.NoError,
prevFolderPaths: map[string]map[string]string{
driveID1: {},
},
expectedCollections: map[string]map[data.CollectionState][]string{
folderPath1: {data.NewState: {"file"}},
rootFolderPath1: {data.NotMovedState: {"folder"}},
@ -1281,6 +1323,9 @@ func (suite *OneDriveCollectionsSuite) TestGet() {
},
},
errCheck: assert.NoError,
prevFolderPaths: map[string]map[string]string{
driveID1: {},
},
expectedCollections: map[string]map[data.CollectionState][]string{
folderPath1: {data.NewState: {"file", "file2"}},
rootFolderPath1: {data.NotMovedState: {"folder"}},
@ -1290,6 +1335,7 @@ func (suite *OneDriveCollectionsSuite) TestGet() {
},
expectedFolderPaths: map[string]map[string]string{
driveID1: {
"root": rootFolderPath1,
"folder": folderPath1,
},
},
@ -1324,6 +1370,10 @@ func (suite *OneDriveCollectionsSuite) TestGet() {
},
},
errCheck: assert.NoError,
prevFolderPaths: map[string]map[string]string{
driveID1: {},
driveID2: {},
},
expectedCollections: map[string]map[data.CollectionState][]string{
folderPath1: {data.NewState: {"file"}},
folderPath2: {data.NewState: {"file2"}},
@ -1336,9 +1386,11 @@ func (suite *OneDriveCollectionsSuite) TestGet() {
},
expectedFolderPaths: map[string]map[string]string{
driveID1: {
"root": rootFolderPath1,
"folder": folderPath1,
},
driveID2: {
"root2": rootFolderPath2,
"folder2": folderPath2,
},
},
@ -1354,7 +1406,10 @@ func (suite *OneDriveCollectionsSuite) TestGet() {
},
},
},
errCheck: assert.Error,
errCheck: assert.Error,
prevFolderPaths: map[string]map[string]string{
driveID1: {},
},
expectedCollections: nil,
expectedDeltaURLs: nil,
expectedFolderPaths: nil,
@ -1385,9 +1440,9 @@ func (suite *OneDriveCollectionsSuite) TestGet() {
driveID1: delta,
},
expectedFolderPaths: map[string]map[string]string{
// We need an empty map here so deserializing metadata knows the delta
// token for this drive is valid.
driveID1: {},
driveID1: {
"root": rootFolderPath1,
},
},
expectedDelList: map[string]struct{}{},
doNotMergeItems: true,
@ -1426,7 +1481,10 @@ func (suite *OneDriveCollectionsSuite) TestGet() {
driveID1: delta,
},
expectedFolderPaths: map[string]map[string]string{
driveID1: {"folder": folderPath1},
driveID1: {
"root": rootFolderPath1,
"folder": folderPath1,
},
},
expectedDelList: map[string]struct{}{},
doNotMergeItems: true,
@ -1454,6 +1512,9 @@ func (suite *OneDriveCollectionsSuite) TestGet() {
},
},
errCheck: assert.NoError,
prevFolderPaths: map[string]map[string]string{
driveID1: {},
},
expectedCollections: map[string]map[data.CollectionState][]string{
expectedPath1(""): {data.NotMovedState: {"file", "folder"}},
expectedPath1("/folder"): {data.NewState: {"file"}},
@ -1462,7 +1523,10 @@ func (suite *OneDriveCollectionsSuite) TestGet() {
driveID1: delta,
},
expectedFolderPaths: map[string]map[string]string{
driveID1: {"folder": folderPath1},
driveID1: {
"root": rootFolderPath1,
"folder": folderPath1,
},
},
expectedDelList: map[string]struct{}{"file": {}},
doNotMergeItems: false,
@ -1525,10 +1589,7 @@ func (suite *OneDriveCollectionsSuite) TestGet() {
),
graph.NewMetadataEntry(
graph.PreviousPathFileName,
map[string]map[string]string{
driveID1: {},
driveID2: {},
},
test.prevFolderPaths,
),
},
func(*support.ConnectorOperationStatus) {},
@ -1638,7 +1699,6 @@ func delItem(
item.SetDeleted(models.NewDeleted())
parentReference := models.NewItemReference()
parentReference.SetPath(&parentPath)
parentReference.SetId(&parentID)
item.SetParentReference(parentReference)
@ -1754,6 +1814,7 @@ func (suite *OneDriveCollectionsSuite) TestCollectItems() {
"",
"General",
collectorFunc,
map[string]string{},
test.prevDelta,
)

View File

@ -178,6 +178,7 @@ func defaultItemPager(
"parentReference",
"root",
"size",
"deleted",
},
)
}
@ -189,21 +190,18 @@ func collectItems(
pager itemPager,
driveID, driveName string,
collector itemCollector,
oldPaths map[string]string,
prevDelta string,
) (DeltaUpdate, map[string]string, map[string]struct{}, error) {
var (
newDeltaURL = ""
// TODO(ashmrtn): Eventually this should probably be a parameter so we can
// take in previous paths.
oldPaths = map[string]string{}
newDeltaURL = ""
newPaths = map[string]string{}
excluded = map[string]struct{}{}
invalidPrevDelta = len(prevDelta) == 0
)
maps.Copy(newPaths, oldPaths)
if len(prevDelta) != 0 {
if !invalidPrevDelta {
maps.Copy(newPaths, oldPaths)
pager.SetNext(prevDelta)
}
@ -411,6 +409,7 @@ func GetAllFolders(
return nil
},
map[string]string{},
"",
)
if err != nil {

View File

@ -127,6 +127,7 @@ func (suite *ItemIntegrationSuite) TestItemReader_oneDrive() {
suite.userDriveID,
"General",
itemCollector,
map[string]string{},
"",
)
require.NoError(suite.T(), err)