From d1ce661b8fa03805e57b21ea7abcfb75d01a47ac Mon Sep 17 00:00:00 2001 From: ashmrtn Date: Wed, 22 Mar 2023 16:31:46 -0700 Subject: [PATCH] Fixup item deletion edge cases (#2920) Mostly centers around handling item deletion when the item is both created and deleted during item enumeration --- #### Does this PR need a docs update or release note? - [x] :white_check_mark: Yes, it's included - [ ] :clock1: Yes, but in a later PR - [ ] :no_entry: No #### Type of change - [ ] :sunflower: Feature - [x] :bug: Bugfix - [ ] :world_map: Documentation - [ ] :robot: Supportability/Tests - [ ] :computer: CI/Deployment - [ ] :broom: Tech Debt/Cleanup #### Issue(s) * closes #2917 #### Test Plan - [ ] :muscle: Manual - [x] :zap: Unit test - [ ] :green_heart: E2E --- CHANGELOG.md | 1 + .../connector/onedrive/collections.go | 19 ++ .../connector/onedrive/collections_test.go | 186 ++++++++++++++++++ 3 files changed, 206 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e61c092e3..e13731c30 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Incremental OneDrive backups could panic if the delta token expired and a folder was seen and deleted in the course of item enumeration for the backup. - Incorrectly moving subfolder hierarchy from a deleted folder to a new folder at the same path during OneDrive incremental backup. - Handle calendar events with no body. +- Items not being deleted if they were created and deleted during item enumeration of a OneDrive backup. ## [v0.6.1] (beta) - 2023-03-21 diff --git a/src/internal/connector/onedrive/collections.go b/src/internal/connector/onedrive/collections.go index 411154891..e90c9e354 100644 --- a/src/internal/connector/onedrive/collections.go +++ b/src/internal/connector/onedrive/collections.go @@ -495,9 +495,27 @@ func (c *Collections) handleDelete( oldPaths, newPaths map[string]string, isFolder bool, excluded map[string]struct{}, + itemCollection map[string]map[string]string, invalidPrevDelta bool, ) error { if !isFolder { + // Try to remove the item from the Collection if an entry exists for this + // item. This handles cases where an item was created and deleted during the + // same delta query. + if parentID, ok := itemCollection[driveID][itemID]; ok { + if col := c.CollectionMap[driveID][parentID]; col != nil { + col.Remove(itemID) + } + + delete(itemCollection[driveID], itemID) + } + + // Don't need to add to exclude list if the delta is invalid since the + // exclude list only matters if we're merging with a base. + if invalidPrevDelta { + return nil + } + excluded[itemID+DataFileSuffix] = struct{}{} excluded[itemID+MetaFileSuffix] = struct{}{} // Exchange counts items streamed through it which includes deletions so @@ -669,6 +687,7 @@ func (c *Collections) UpdateCollections( newPaths, isFolder, excluded, + itemCollection, invalidPrevDelta, ); err != nil { return clues.Stack(err).WithClues(ictx) diff --git a/src/internal/connector/onedrive/collections_test.go b/src/internal/connector/onedrive/collections_test.go index c12d1b390..054d89aa0 100644 --- a/src/internal/connector/onedrive/collections_test.go +++ b/src/internal/connector/onedrive/collections_test.go @@ -2009,6 +2009,192 @@ func (suite *OneDriveCollectionsUnitSuite) TestGet() { expectedDelList: map[string]map[string]struct{}{}, doNotMergeItems: true, }, + { + name: "One Drive Delta Error Random Item Delete", + drives: []models.Driveable{drive1}, + items: map[string][]deltaPagerResult{ + driveID1: { + { + err: getDeltaError(), + }, + { + items: []models.DriveItemable{ + driveRootItem("root"), + delItem("file", driveBasePath1, "root", true, false, false), + }, + deltaLink: &delta, + }, + }, + }, + errCheck: assert.NoError, + prevFolderPaths: map[string]map[string]string{ + driveID1: { + "root": rootFolderPath1, + }, + }, + expectedCollections: map[string]map[data.CollectionState][]string{ + rootFolderPath1: {data.NewState: {}}, + }, + expectedDeltaURLs: map[string]string{ + driveID1: delta, + }, + expectedFolderPaths: map[string]map[string]string{ + driveID1: { + "root": rootFolderPath1, + }, + }, + expectedDelList: map[string]map[string]struct{}{}, + doNotMergeItems: true, + }, + { + name: "One Drive Folder Made And Deleted", + drives: []models.Driveable{drive1}, + items: map[string][]deltaPagerResult{ + driveID1: { + { + items: []models.DriveItemable{ + driveRootItem("root"), + driveItem("folder", "folder", driveBasePath1, "root", false, true, false), + driveItem("file", "file", driveBasePath1+"/folder", "folder", true, false, false), + }, + nextLink: &next, + }, + { + items: []models.DriveItemable{ + driveRootItem("root"), + delItem("folder", driveBasePath1, "root", false, true, false), + delItem("file", driveBasePath1, "root", true, false, false), + }, + deltaLink: &delta2, + }, + }, + }, + errCheck: assert.NoError, + prevFolderPaths: map[string]map[string]string{ + driveID1: {}, + }, + expectedCollections: map[string]map[data.CollectionState][]string{ + rootFolderPath1: {data.NewState: {}}, + }, + expectedDeltaURLs: map[string]string{ + driveID1: delta2, + }, + expectedFolderPaths: map[string]map[string]string{ + driveID1: { + "root": rootFolderPath1, + }, + }, + expectedDelList: map[string]map[string]struct{}{ + rootFolderPath1: getDelList("file"), + }, + }, + { + name: "One Drive Item Made And Deleted", + drives: []models.Driveable{drive1}, + items: map[string][]deltaPagerResult{ + driveID1: { + { + items: []models.DriveItemable{ + driveRootItem("root"), + driveItem("folder", "folder", driveBasePath1, "root", false, true, false), + driveItem("file", "file", driveBasePath1+"/folder", "folder", true, false, false), + }, + nextLink: &next, + }, + { + items: []models.DriveItemable{ + driveRootItem("root"), + delItem("file", driveBasePath1, "root", true, false, false), + }, + deltaLink: &delta, + }, + }, + }, + errCheck: assert.NoError, + prevFolderPaths: map[string]map[string]string{ + driveID1: {}, + }, + expectedCollections: map[string]map[data.CollectionState][]string{ + rootFolderPath1: {data.NewState: {}}, + folderPath1: {data.NewState: {"folder"}}, + }, + expectedDeltaURLs: map[string]string{ + driveID1: delta, + }, + expectedFolderPaths: map[string]map[string]string{ + driveID1: { + "root": rootFolderPath1, + "folder": folderPath1, + }, + }, + expectedDelList: map[string]map[string]struct{}{ + rootFolderPath1: getDelList("file"), + }, + }, + { + name: "One Drive Random Folder Delete", + drives: []models.Driveable{drive1}, + items: map[string][]deltaPagerResult{ + driveID1: { + { + items: []models.DriveItemable{ + driveRootItem("root"), + delItem("folder", driveBasePath1, "root", false, true, false), + }, + deltaLink: &delta, + }, + }, + }, + errCheck: assert.NoError, + prevFolderPaths: map[string]map[string]string{ + driveID1: {}, + }, + expectedCollections: map[string]map[data.CollectionState][]string{ + rootFolderPath1: {data.NewState: {}}, + }, + expectedDeltaURLs: map[string]string{ + driveID1: delta, + }, + expectedFolderPaths: map[string]map[string]string{ + driveID1: { + "root": rootFolderPath1, + }, + }, + expectedDelList: map[string]map[string]struct{}{}, + }, + { + name: "One Drive Random Item Delete", + drives: []models.Driveable{drive1}, + items: map[string][]deltaPagerResult{ + driveID1: { + { + items: []models.DriveItemable{ + driveRootItem("root"), + delItem("file", driveBasePath1, "root", true, false, false), + }, + deltaLink: &delta, + }, + }, + }, + errCheck: assert.NoError, + prevFolderPaths: map[string]map[string]string{ + driveID1: {}, + }, + expectedCollections: map[string]map[data.CollectionState][]string{ + rootFolderPath1: {data.NewState: {}}, + }, + expectedDeltaURLs: map[string]string{ + driveID1: delta, + }, + expectedFolderPaths: map[string]map[string]string{ + driveID1: { + "root": rootFolderPath1, + }, + }, + expectedDelList: map[string]map[string]struct{}{ + rootFolderPath1: getDelList("file"), + }, + }, } for _, test := range table { suite.Run(test.name, func() {