OneDrive backup cleanup (#2618)

## Description

General cleanup/consolidation of code for getting OneDrive collections during backup. Few minor changes to behavior:
* always return a collection for the root folder even if it's empty
* add items to the exclude list only after they've been added to a collection. This gives a better chance of preserving the item even if we encounter an error parsing paths or something. If there's a failure, the old version of the item is most likely to be preserved (this varies based on whether the item is returned in multiple delta results during the backup)

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

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

## Type of change

- [ ] 🌻 Feature
- [ ] 🐛 Bugfix
- [ ] 🗺️ Documentation
- [ ] 🤖 Test
- [ ] 💻 CI/Deployment
- [x] 🧹 Tech Debt/Cleanup

## Issue(s)

* #2447

## Test Plan

- [ ] 💪 Manual
- [x] ⚡ Unit test
- [ ] 💚 E2E

src/internal/connector/data_collections_test.go

@@ -349,7 +349,10 @@ func (suite *ConnectorCreateSharePointCollectionIntegrationSuite) TestCreateShar
 
 	for _, collection := range cols {
 		t.Logf("Path: %s\n", collection.FullPath().String())
-		assert.Equal(t, path.SharePointMetadataService, collection.FullPath().Service())
+		assert.Equal(
+			t,
+			path.SharePointMetadataService.String(),
+			collection.FullPath().Service().String())
 	}
 }
 

src/internal/connector/onedrive/collections.go

@@ -70,7 +70,7 @@ type Collections struct {
 
 	// collectionMap allows lookup of the data.BackupCollection
 	// for a OneDrive folder
-	CollectionMap map[string]data.BackupCollection
+	CollectionMap map[string]*Collection
 
 	// Not the most ideal, but allows us to change the pager function for testing
 	// as needed. This will allow us to mock out some scenarios during testing.
@@ -107,7 +107,7 @@ func NewCollections(
 		resourceOwner:  resourceOwner,
 		source:         source,
 		matcher:        matcher,
-		CollectionMap:  map[string]data.BackupCollection{},
+		CollectionMap:  map[string]*Collection{},
 		drivePagerFunc: PagerForSource,
 		itemPagerFunc:  defaultItemPager,
 		service:        service,
@@ -433,24 +433,19 @@ func (c *Collections) Get(
 
 func updateCollectionPaths(
 	id string,
-	cmap map[string]data.BackupCollection,
+	cmap map[string]*Collection,
 	curPath path.Path,
 ) (bool, error) {
 	var initialCurPath path.Path
 
 	col, found := cmap[id]
 	if found {
-		ocol, ok := col.(*Collection)
+		initialCurPath = col.FullPath()
-		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)
+		col.SetFullPath(curPath)
 	}
 
 	if initialCurPath == nil {
@@ -462,23 +457,134 @@ func updateCollectionPaths(
 			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)
+			c.SetFullPath(colPath)
 		}
 	}
 
 	return found, nil
 }
 
+func (c *Collections) handleDelete(
+	itemID, driveID string,
+	oldPaths, newPaths map[string]string,
+	isFolder bool,
+	excluded map[string]struct{},
+) error {
+	if !isFolder {
+		excluded[itemID+DataFileSuffix] = struct{}{}
+		excluded[itemID+MetaFileSuffix] = struct{}{}
+		// Exchange counts items streamed through it which includes deletions so
+		// add that here too.
+		c.NumFiles++
+		c.NumItems++
+
+		return nil
+	}
+
+	var prevPath path.Path
+
+	prevPathStr, ok := oldPaths[itemID]
+	if ok {
+		var err error
+
+		prevPath, err = path.FromDataLayerPath(prevPathStr, false)
+		if err != nil {
+			return clues.Wrap(err, "invalid previous path").
+				With("collection_id", itemID, "path_string", prevPathStr)
+		}
+	}
+
+	// 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, itemID)
+
+	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
+		// delete entry in the delta response.
+		return nil
+	}
+
+	col := NewCollection(
+		c.itemClient,
+		nil,
+		prevPath,
+		driveID,
+		c.service,
+		c.statusUpdater,
+		c.source,
+		c.ctrl,
+		// DoNotMerge is not checked for deleted items.
+		false,
+	)
+
+	c.CollectionMap[itemID] = col
+
+	return nil
+}
+
+func (c *Collections) getCollectionPath(
+	driveID string,
+	item models.DriveItemable,
+) (path.Path, error) {
+	var (
+		collectionPathStr string
+		isRoot            = item.GetRoot() != nil
+		isFile            = item.GetFile() != nil
+	)
+
+	if isRoot {
+		collectionPathStr = fmt.Sprintf(rootDrivePattern, driveID)
+	} else {
+		if item.GetParentReference() == nil ||
+			item.GetParentReference().GetPath() == nil {
+			err := clues.New("no parent reference").
+				With("item_name", ptr.Val(item.GetName()))
+
+			return nil, err
+		}
+
+		collectionPathStr = ptr.Val(item.GetParentReference().GetPath())
+	}
+
+	collectionPath, err := GetCanonicalPath(
+		collectionPathStr,
+		c.tenant,
+		c.resourceOwner,
+		c.source,
+	)
+	if err != nil {
+		return nil, clues.Wrap(err, "making item path")
+	}
+
+	if isRoot || isFile {
+		return collectionPath, nil
+	}
+
+	// Append folder name to path since we want the path for the collection, not
+	// the path for the parent of the collection. The root and files don't need
+	// to append an extra element because the root already refers to itself and
+	// the collection containing the item is the parent path.
+	name := ptr.Val(item.GetName())
+	if len(name) == 0 {
+		return nil, clues.New("folder with empty name")
+	}
+
+	collectionPath, err = collectionPath.Append(name, false)
+	if err != nil {
+		return nil, clues.Wrap(err, "making non-root folder path")
+	}
+
+	return collectionPath, 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.
@@ -503,97 +609,43 @@ func (c *Collections) UpdateCollections(
 		}
 
 		var (
-			prevPath           path.Path
+			itemID   = ptr.Val(item.GetId())
-			prevCollectionPath path.Path
+			ictx     = clues.Add(ctx, "update_item_id", itemID)
-			ok                 bool
+			isFolder = item.GetFolder() != nil || item.GetPackage() != nil
 		)
 
-		if item.GetRoot() != nil {
+		// Deleted file or folder.
-			rootPath, err := GetCanonicalPath(
+		if item.GetDeleted() != nil {
-				fmt.Sprintf(rootDrivePattern, driveID),
+			if err := c.handleDelete(
-				c.tenant,
+				itemID,
-				c.resourceOwner,
+				driveID,
-				c.source,
+				oldPaths,
-			)
+				newPaths,
-			if err != nil {
+				isFolder,
-				return err
+				excluded,
+			); err != nil {
+				return clues.Stack(err).WithClues(ictx)
 			}
 
-			// 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
 		}
 
-		var (
+		collectionPath, err := c.getCollectionPath(driveID, item)
-			itemID = ptr.Val(item.GetId())
-			ictx   = clues.Add(ctx, "update_item_id", itemID)
-		)
-
-		if item.GetParentReference() == nil ||
-			item.GetParentReference().GetId() == nil ||
-			(item.GetDeleted() == nil && item.GetParentReference().GetPath() == nil) {
-			et.Add(clues.New("item missing parent reference").
-				WithClues(ictx).
-				With("item_id", itemID, "item_name", ptr.Val(item.GetName())).
-				Label(fault.LabelForceNoBackupCreation))
-
-			continue
-		}
-
-		// Create a collection for the parent of this item
-		collectionID := ptr.Val(item.GetParentReference().GetId())
-		ictx = clues.Add(ictx, "collection_id", collectionID)
-
-		var collectionPathStr string
-		if item.GetDeleted() == nil {
-			collectionPathStr = ptr.Val(item.GetParentReference().GetPath())
-		} else {
-			collectionPathStr, ok = oldPaths[ptr.Val(item.GetParentReference().GetId())]
-			if !ok {
-				// This collection was created and destroyed in
-				// between the current and previous invocation
-				continue
-			}
-		}
-
-		collectionPath, err := GetCanonicalPath(
-			collectionPathStr,
-			c.tenant,
-			c.resourceOwner,
-			c.source)
 		if err != nil {
 			return clues.Stack(err).WithClues(ictx)
 		}
 
-		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, itemPath, c.matcher, driveName) &&
+		if shouldSkipDrive(ctx, collectionPath, c.matcher, driveName) {
-			shouldSkipDrive(ctx, collectionPath, c.matcher, driveName) {
 			logger.Ctx(ictx).Infow("Skipping path", "skipped_path", collectionPath.String())
 			continue
 		}
 
 		switch {
-		case item.GetFolder() != nil, item.GetPackage() != nil:
+		case isFolder:
+			// Deletions are handled above so this is just moves/renames.
+			var prevPath path.Path
+
 			prevPathStr, ok := oldPaths[itemID]
 			if ok {
 				prevPath, err = path.FromDataLayerPath(prevPathStr, false)
@@ -602,172 +654,103 @@ func (c *Collections) UpdateCollections(
 						WithClues(ictx).
 						With("path_string", prevPathStr))
 				}
-			}
+			} else if item.GetRoot() != nil {
-
+				// Root doesn't move or get renamed.
-			if item.GetDeleted() != nil {
+				prevPath = collectionPath
-				// 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, itemID)
-
-				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
-					// delete entry in the delta response.
-					continue
-				}
-
-				col := NewCollection(
-					c.itemClient,
-					nil,
-					prevPath,
-					driveID,
-					c.service,
-					c.statusUpdater,
-					c.source,
-					c.ctrl,
-					invalidPrevDelta)
-
-				c.CollectionMap[itemID] = col
-
-				break
 			}
 
 			// 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(), itemPath.String())
+			updatePath(newPaths, itemID, collectionPath.String())
 
-			found, err := updateCollectionPaths(*item.GetId(), c.CollectionMap, itemPath)
+			found, err := updateCollectionPaths(itemID, c.CollectionMap, collectionPath)
 			if err != nil {
-				return clues.Stack(err).WithClues(ctx)
+				return clues.Stack(err).WithClues(ictx)
 			}
 
-			if !found {
+			if found {
-				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
 			}
 
-			if col := c.CollectionMap[*item.GetId()]; col != nil {
+			col := NewCollection(
-				// Add an entry to fetch permissions into this collection. This assumes
+				c.itemClient,
-				// that OneDrive always returns all folders on the path of an item
+				collectionPath,
-				// before the item. This seems to hold true for now at least.
+				prevPath,
-				collection := col.(*Collection)
+				driveID,
-				if collection.Add(item) {
+				c.service,
-					c.NumItems++
+				c.statusUpdater,
-				}
+				c.source,
+				c.ctrl,
+				invalidPrevDelta,
+			)
+			c.CollectionMap[itemID] = col
+			c.NumContainers++
+
+			if c.source != OneDriveSource || item.GetRoot() != nil {
+				continue
+			}
+
+			// 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.
+			if col.Add(item) {
+				c.NumItems++
 			}
 
 		case item.GetFile() != nil:
-			if !invalidPrevDelta && item.GetFile() != nil {
+			// Deletions are handled above so this is just moves/renames.
-				// Always add a file to the excluded list. If it was
+			if len(ptr.Val(item.GetParentReference().GetId())) == 0 {
-				// deleted, we want to avoid it. If it was
+				return clues.New("file without parent ID").WithClues(ictx)
-				// renamed/moved/modified, we still have to drop the
-				// original one and download a fresh copy.
-				excluded[itemID+DataFileSuffix] = struct{}{}
-				excluded[itemID+MetaFileSuffix] = struct{}{}
 			}
 
-			if item.GetDeleted() != nil {
+			// Get the collection for this item.
-				// Exchange counts items streamed through it which includes deletions so
+			collectionID := ptr.Val(item.GetParentReference().GetId())
-				// add that here too.
+			ictx = clues.Add(ictx, "collection_id", collectionID)
-				c.NumFiles++
-				c.NumItems++
 
-				continue
+			collection, found := c.CollectionMap[collectionID]
-			}
-
-			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").With("path_string", prevCollectionPathStr)
-					}
-				}
-
-			}
-
-			col, found := c.CollectionMap[collectionID]
 			if !found {
-				// TODO(ashmrtn): We should probably tighten the restrictions on this
+				return clues.New("item seen before parent folder").WithClues(ictx)
-				// 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,
-					prevCollectionPath,
-					driveID,
-					c.service,
-					c.statusUpdater,
-					c.source,
-					c.ctrl,
-					invalidPrevDelta,
-				)
-
-				c.CollectionMap[collectionID] = col
-				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.
-
 			// Delete the file from previous collection. This will
 			// only kick in if the file was moved multiple times
 			// within a single delta query
-			itemColID, found := itemCollection[*item.GetId()]
+			itemColID, found := itemCollection[itemID]
 			if found {
-				pcol, found := c.CollectionMap[itemColID]
+				pcollection, found := c.CollectionMap[itemColID]
 				if !found {
-					return clues.New("previous collection not found").With("item_id", *item.GetId())
+					return clues.New("previous collection not found").WithClues(ictx)
 				}
 
-				pcollection := pcol.(*Collection)
-
 				removed := pcollection.Remove(item)
 				if !removed {
-					return clues.New("removing from prev collection").With("item_id", *item.GetId())
+					return clues.New("removing from prev collection").WithClues(ictx)
 				}
 			}
 
-			itemCollection[*item.GetId()] = collectionID
+			itemCollection[itemID] = collectionID
-			collection := col.(*Collection)
 
 			if collection.Add(item) {
 				c.NumItems++
 				c.NumFiles++
 			}
 
+			// Do this after adding the file to the collection so if we fail to add
+			// the item to the collection for some reason and we're using best effort
+			// we don't just end up deleting the item in the resulting backup. The
+			// resulting backup will be slightly incorrect, but it will have the most
+			// data that we were able to preserve.
+			if !invalidPrevDelta {
+				// Always add a file to the excluded list. The file may have been
+				// renamed/moved/modified, so we still have to drop the
+				// original one and download a fresh copy.
+				excluded[itemID+DataFileSuffix] = struct{}{}
+				excluded[itemID+MetaFileSuffix] = struct{}{}
+			}
+
 		default:
-			return clues.New("item type not supported").WithClues(ctx)
+			return clues.New("item type not supported").WithClues(ictx)
 		}
 	}
 
@@ -775,10 +758,6 @@ 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)
 }

src/internal/connector/onedrive/collections_test.go

@@ -187,6 +187,10 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
 			inputFolderMap: map[string]string{},
 			scope:          anyFolder,
 			expect:         assert.Error,
+			expectedCollectionIDs: map[string]statePath{
+				"root": expectedStatePath(data.NotMovedState, ""),
+			},
+			expectedContainerCount: 1,
 			expectedMetadataPaths: map[string]string{
 				"root": expectedPath(""),
 			},
@@ -223,6 +227,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{
@@ -230,7 +235,7 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
 				"folder": expectedPath("/folder"),
 			},
 			expectedItemCount:      1,
-			expectedContainerCount: 1,
+			expectedContainerCount: 2,
 			expectedExcludes:       map[string]struct{}{},
 		},
 		{
@@ -243,6 +248,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{
@@ -250,7 +256,7 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
 				"package": expectedPath("/package"),
 			},
 			expectedItemCount:      1,
-			expectedContainerCount: 1,
+			expectedContainerCount: 2,
 			expectedExcludes:       map[string]struct{}{},
 		},
 		{
@@ -308,7 +314,6 @@ 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(""),
 				"folder":    expectedPath(folder),
 				"subfolder": expectedPath(folderSub),
 				"folder2":   expectedPath(folderSub + folder),
@@ -340,7 +345,6 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
 			expectedFileCount:      1,
 			expectedContainerCount: 2,
 			expectedMetadataPaths: map[string]string{
-				"root":      expectedPath(""),
 				"subfolder": expectedPath(folderSub),
 				"folder2":   expectedPath(folderSub + folder),
 			},
@@ -369,7 +373,6 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
 			expectedContainerCount: 1,
 			// No child folders for subfolder so nothing here.
 			expectedMetadataPaths: map[string]string{
-				"root":      expectedPath(""),
 				"subfolder": expectedPath(folderSub),
 			},
 			expectedExcludes: getDelList("fileInSubfolder"),
@@ -387,11 +390,12 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
 			scope:  anyFolder,
 			expect: assert.NoError,
 			expectedCollectionIDs: map[string]statePath{
+				"root":   expectedStatePath(data.NotMovedState, ""),
 				"folder": expectedStatePath(data.NotMovedState, folder),
 			},
 			expectedItemCount:      1,
 			expectedFileCount:      0,
-			expectedContainerCount: 1,
+			expectedContainerCount: 2,
 			expectedMetadataPaths: map[string]string{
 				"root":      expectedPath(""),
 				"folder":    expectedPath(folder),
@@ -412,11 +416,12 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
 			scope:  anyFolder,
 			expect: assert.NoError,
 			expectedCollectionIDs: map[string]statePath{
+				"root":   expectedStatePath(data.NotMovedState, ""),
 				"folder": expectedStatePath(data.MovedState, folder, "/a-folder"),
 			},
 			expectedItemCount:      1,
 			expectedFileCount:      0,
-			expectedContainerCount: 1,
+			expectedContainerCount: 2,
 			expectedMetadataPaths: map[string]string{
 				"root":      expectedPath(""),
 				"folder":    expectedPath(folder),
@@ -424,30 +429,6 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
 			},
 			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{
-				"folder": expectedStatePath(data.NotMovedState, folder),
-			},
-			expectedItemCount:      2,
-			expectedFileCount:      1,
-			expectedContainerCount: 1,
-			expectedMetadataPaths: map[string]string{
-				"root":   expectedPath(""),
-				"folder": expectedPath(folder),
-			},
-			expectedExcludes: getDelList("file"),
-		},
 		{
 			testCase: "moved folder tree with file no previous",
 			items: []models.DriveItemable{
@@ -460,11 +441,12 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
 			scope:          anyFolder,
 			expect:         assert.NoError,
 			expectedCollectionIDs: map[string]statePath{
+				"root":   expectedStatePath(data.NotMovedState, ""),
 				"folder": expectedStatePath(data.NewState, "/folder2"),
 			},
 			expectedItemCount:      2,
 			expectedFileCount:      1,
-			expectedContainerCount: 1,
+			expectedContainerCount: 2,
 			expectedMetadataPaths: map[string]string{
 				"root":   expectedPath(""),
 				"folder": expectedPath("/folder2"),
@@ -482,11 +464,12 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
 			scope:          anyFolder,
 			expect:         assert.NoError,
 			expectedCollectionIDs: map[string]statePath{
+				"root":   expectedStatePath(data.NotMovedState, ""),
 				"folder": expectedStatePath(data.NewState, folder),
 			},
 			expectedItemCount:      2,
 			expectedFileCount:      1,
-			expectedContainerCount: 1,
+			expectedContainerCount: 2,
 			expectedMetadataPaths: map[string]string{
 				"root":   expectedPath(""),
 				"folder": expectedPath(folder),
@@ -507,12 +490,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"),
 				"subfolder": expectedStatePath(data.MovedState, "/subfolder", "/a-folder/subfolder"),
 			},
 			expectedItemCount:      2,
 			expectedFileCount:      0,
-			expectedContainerCount: 2,
+			expectedContainerCount: 3,
 			expectedMetadataPaths: map[string]string{
 				"root":      expectedPath(""),
 				"folder":    expectedPath(folder),
@@ -534,12 +518,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"),
 				"subfolder": expectedStatePath(data.MovedState, "/subfolder", "/a-folder/subfolder"),
 			},
 			expectedItemCount:      2,
 			expectedFileCount:      0,
-			expectedContainerCount: 2,
+			expectedContainerCount: 3,
 			expectedMetadataPaths: map[string]string{
 				"root":      expectedPath(""),
 				"folder":    expectedPath(folder),
@@ -575,13 +560,14 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
 			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, folderSub, "/a-folder/subfolder"),
 			},
 			expectedItemCount:      5,
 			expectedFileCount:      2,
-			expectedContainerCount: 3,
+			expectedContainerCount: 4,
 			expectedMetadataPaths: map[string]string{
 				"root":      expectedPath(""),
 				"folder":    expectedPath("/folder"),
@@ -605,11 +591,12 @@ 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:      2,
 			expectedFileCount:      1,
-			expectedContainerCount: 1,
+			expectedContainerCount: 2,
 			expectedMetadataPaths: map[string]string{
 				"root":      expectedPath(""),
 				"folder":    expectedPath("/folder2"),
@@ -632,12 +619,13 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
 			scope:  anyFolder,
 			expect: assert.NoError,
 			expectedCollectionIDs: map[string]statePath{
+				"root":    expectedStatePath(data.NotMovedState, ""),
 				"folder":  expectedStatePath(data.DeletedState, folder),
 				"package": expectedStatePath(data.DeletedState, pkg),
 			},
 			expectedItemCount:      0,
 			expectedFileCount:      0,
-			expectedContainerCount: 0,
+			expectedContainerCount: 1,
 			expectedMetadataPaths: map[string]string{
 				"root": expectedPath(""),
 			},
@@ -652,12 +640,14 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
 			inputFolderMap: map[string]string{
 				"root": expectedPath(""),
 			},
-			scope:                  anyFolder,
+			scope:  anyFolder,
-			expect:                 assert.NoError,
+			expect: assert.NoError,
-			expectedCollectionIDs:  map[string]statePath{},
+			expectedCollectionIDs: map[string]statePath{
+				"root": expectedStatePath(data.NotMovedState, ""),
+			},
 			expectedItemCount:      0,
 			expectedFileCount:      0,
-			expectedContainerCount: 0,
+			expectedContainerCount: 1,
 			expectedMetadataPaths: map[string]string{
 				"root": expectedPath(""),
 			},
@@ -678,12 +668,13 @@ 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", folderSub),
 			},
 			expectedItemCount:      1,
 			expectedFileCount:      0,
-			expectedContainerCount: 1,
+			expectedContainerCount: 2,
 			expectedMetadataPaths: map[string]string{
 				"root":      expectedPath(""),
 				"subfolder": expectedPath("/subfolder"),
@@ -693,21 +684,46 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
 		{
 			testCase: "delete file",
 			items: []models.DriveItemable{
+				driveRootItem("root"),
 				delItem("item", testBaseDrivePath, "root", true, false, false),
 			},
 			inputFolderMap: map[string]string{
 				"root": expectedPath(""),
 			},
-			scope:                  anyFolder,
+			scope:  anyFolder,
-			expect:                 assert.NoError,
+			expect: assert.NoError,
+			expectedCollectionIDs: map[string]statePath{
+				"root": expectedStatePath(data.NotMovedState, ""),
+			},
 			expectedItemCount:      1,
 			expectedFileCount:      1,
-			expectedContainerCount: 0,
+			expectedContainerCount: 1,
 			expectedMetadataPaths: map[string]string{
 				"root": expectedPath(""),
 			},
 			expectedExcludes: getDelList("item"),
 		},
+		{
+			testCase: "item before parent errors",
+			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{},
+			scope:          anyFolder,
+			expect:         assert.Error,
+			expectedCollectionIDs: map[string]statePath{
+				"root": expectedStatePath(data.NotMovedState, ""),
+			},
+			expectedItemCount:      0,
+			expectedFileCount:      0,
+			expectedContainerCount: 1,
+			expectedMetadataPaths: map[string]string{
+				"root": expectedPath(""),
+			},
+			expectedExcludes: map[string]struct{}{},
+		},
 	}
 
 	for _, tt := range tests {
@@ -1241,7 +1257,9 @@ func (suite *OneDriveCollectionsSuite) TestGet() {
 			prevFolderPaths: map[string]map[string]string{
 				driveID1: {"root": rootFolderPath1},
 			},
-			expectedCollections: map[string]map[data.CollectionState][]string{},
+			expectedCollections: map[string]map[data.CollectionState][]string{
+				rootFolderPath1: {data.NotMovedState: {}},
+			},
 			expectedDeltaURLs: map[string]string{
 				driveID1: delta,
 			},
@@ -1266,10 +1284,10 @@ func (suite *OneDriveCollectionsSuite) TestGet() {
 			},
 			errCheck: assert.NoError,
 			prevFolderPaths: map[string]map[string]string{
-				driveID1: {"root": expectedPath1("")},
+				driveID1: {"root": rootFolderPath1},
 			},
 			expectedCollections: map[string]map[data.CollectionState][]string{
-				expectedPath1(""): {data.NotMovedState: {"file"}},
+				rootFolderPath1: {data.NotMovedState: {"file"}},
 			},
 			expectedDeltaURLs: map[string]string{
 				driveID1: delta,
@@ -1299,7 +1317,8 @@ func (suite *OneDriveCollectionsSuite) TestGet() {
 				driveID1: {},
 			},
 			expectedCollections: map[string]map[data.CollectionState][]string{
-				folderPath1: {data.NewState: {"folder", "file"}},
+				rootFolderPath1: {data.NewState: {}},
+				folderPath1:     {data.NewState: {"folder", "file"}},
 			},
 			expectedDeltaURLs: map[string]string{
 				driveID1: delta,
@@ -1333,7 +1352,8 @@ func (suite *OneDriveCollectionsSuite) TestGet() {
 				driveID1: {},
 			},
 			expectedCollections: map[string]map[data.CollectionState][]string{
-				folderPath1: {data.NewState: {"folder", "file"}},
+				rootFolderPath1: {data.NewState: {}},
+				folderPath1:     {data.NewState: {"folder", "file"}},
 			},
 			expectedDeltaURLs: map[string]string{
 				driveID1: delta,
@@ -1401,7 +1421,8 @@ func (suite *OneDriveCollectionsSuite) TestGet() {
 				driveID1: {},
 			},
 			expectedCollections: map[string]map[data.CollectionState][]string{
-				folderPath1: {data.NewState: {"folder", "file"}},
+				rootFolderPath1: {data.NewState: {}},
+				folderPath1:     {data.NewState: {"folder", "file"}},
 			},
 			expectedDeltaURLs:   map[string]string{},
 			expectedFolderPaths: map[string]map[string]string{},
@@ -1435,7 +1456,8 @@ func (suite *OneDriveCollectionsSuite) TestGet() {
 				driveID1: {},
 			},
 			expectedCollections: map[string]map[data.CollectionState][]string{
-				folderPath1: {data.NewState: {"folder", "file", "file2"}},
+				rootFolderPath1: {data.NewState: {}},
+				folderPath1:     {data.NewState: {"folder", "file", "file2"}},
 			},
 			expectedDeltaURLs: map[string]string{
 				driveID1: delta,
@@ -1482,8 +1504,10 @@ func (suite *OneDriveCollectionsSuite) TestGet() {
 				driveID2: {},
 			},
 			expectedCollections: map[string]map[data.CollectionState][]string{
-				folderPath1: {data.NewState: {"folder", "file"}},
+				rootFolderPath1: {data.NewState: {}},
-				folderPath2: {data.NewState: {"folder2", "file2"}},
+				folderPath1:     {data.NewState: {"folder", "file"}},
+				rootFolderPath2: {data.NewState: {}},
+				folderPath2:     {data.NewState: {"folder2", "file2"}},
 			},
 			expectedDeltaURLs: map[string]string{
 				driveID1: delta,
@@ -1539,7 +1563,7 @@ func (suite *OneDriveCollectionsSuite) TestGet() {
 			},
 			errCheck: assert.NoError,
 			expectedCollections: map[string]map[data.CollectionState][]string{
-				expectedPath1(""): {data.NotMovedState: {"file"}},
+				rootFolderPath1: {data.NotMovedState: {"file"}},
 			},
 			expectedDeltaURLs: map[string]string{
 				driveID1: delta,
@@ -1579,7 +1603,7 @@ func (suite *OneDriveCollectionsSuite) TestGet() {
 			},
 			errCheck: assert.NoError,
 			expectedCollections: map[string]map[data.CollectionState][]string{
-				expectedPath1(""):        {data.NotMovedState: {"file"}},
+				rootFolderPath1:          {data.NotMovedState: {"file"}},
 				expectedPath1("/folder"): {data.NewState: {"folder", "file2"}},
 			},
 			expectedDeltaURLs: map[string]string{
@@ -1621,7 +1645,7 @@ func (suite *OneDriveCollectionsSuite) TestGet() {
 				driveID1: {},
 			},
 			expectedCollections: map[string]map[data.CollectionState][]string{
-				expectedPath1(""):        {data.NotMovedState: {"file"}},
+				rootFolderPath1:          {data.NotMovedState: {"file"}},
 				expectedPath1("/folder"): {data.NewState: {"folder", "file2"}},
 			},
 			expectedDeltaURLs: map[string]string{
@@ -1662,6 +1686,7 @@ func (suite *OneDriveCollectionsSuite) TestGet() {
 				},
 			},
 			expectedCollections: map[string]map[data.CollectionState][]string{
+				rootFolderPath1:           {data.NewState: {}},
 				expectedPath1("/folder"):  {data.DeletedState: {}},
 				expectedPath1("/folder2"): {data.NewState: {"folder2", "file"}},
 			},
@@ -1703,6 +1728,7 @@ func (suite *OneDriveCollectionsSuite) TestGet() {
 				},
 			},
 			expectedCollections: map[string]map[data.CollectionState][]string{
+				rootFolderPath1:          {data.NewState: {}},
 				expectedPath1("/folder"): {data.NewState: {"folder2", "file"}},
 			},
 			expectedDeltaURLs: map[string]string{
@@ -1894,6 +1920,7 @@ func driveRootItem(id string) models.DriveItemable {
 	item.SetName(&name)
 	item.SetId(&id)
 	item.SetRoot(models.NewRoot())
+	item.SetFolder(models.NewFolder())
 
 	return item
 }

src/internal/connector/sharepoint/data_collections_test.go

@@ -156,6 +156,7 @@ func driveRootItem(id string) models.DriveItemable {
 	item.SetName(&name)
 	item.SetId(&id)
 	item.SetRoot(models.NewRoot())
+	item.SetFolder(models.NewFolder())
 
 	return item
 }