diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c88de28c..7e15da9f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Fixed permissions restore in latest backup version +- 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. ## [v0.6.1] (beta) - 2023-03-21 diff --git a/src/go.sum b/src/go.sum index ee1467596..e8f661fa7 100644 --- a/src/go.sum +++ b/src/go.sum @@ -266,8 +266,6 @@ github.com/matttproud/golang_protobuf_extensions v1.0.2 h1:hAHbPm5IJGijwng3PWk09 github.com/matttproud/golang_protobuf_extensions v1.0.2/go.mod h1:BSXmuO+STAnVfrANrmjBb36TMTDstsz7MSK+HVaYKv4= github.com/mgutz/ansi v0.0.0-20200706080929-d51e80ef957d h1:5PJl274Y63IEHC+7izoQE9x6ikvDFZS2mDVS3drnohI= github.com/mgutz/ansi v0.0.0-20200706080929-d51e80ef957d/go.mod h1:01TrycV0kFyexm33Z7vhZRXopbI8J3TDReVlkTgMUxE= -github.com/microsoft/kiota-abstractions-go v0.17.3 h1:RmQ9RXzXFkQh8QnFcQ31QM2ou8MJksQe0/H8shz8CNI= -github.com/microsoft/kiota-abstractions-go v0.17.3/go.mod h1:0lbPErVO6Rj3HHpntNYW/OFmHhJJ1ewPdsi1xPxYIMc= github.com/microsoft/kiota-abstractions-go v0.18.0 h1:H1kQE5hAq/7Q8gENPJ1Y7DuvG9QqKCpglN8D7TJi9qY= github.com/microsoft/kiota-abstractions-go v0.18.0/go.mod h1:0lbPErVO6Rj3HHpntNYW/OFmHhJJ1ewPdsi1xPxYIMc= github.com/microsoft/kiota-authentication-azure-go v0.6.0 h1:Il9bLO34J6D8DY89xYAXoGh9muvlphayqG4eihyT6B8= diff --git a/src/internal/connector/onedrive/collection.go b/src/internal/connector/onedrive/collection.go index 8d5ce97e4..aa55dcf5b 100644 --- a/src/internal/connector/onedrive/collection.go +++ b/src/internal/connector/onedrive/collection.go @@ -178,13 +178,13 @@ func (oc *Collection) Add(item models.DriveItemable) bool { } // Remove removes a item from the collection -func (oc *Collection) Remove(item models.DriveItemable) bool { - _, found := oc.driveItems[ptr.Val(item.GetId())] +func (oc *Collection) Remove(itemID string) bool { + _, found := oc.driveItems[itemID] if !found { return false } - delete(oc.driveItems, ptr.Val(item.GetId())) + delete(oc.driveItems, itemID) return true } diff --git a/src/internal/connector/onedrive/collections.go b/src/internal/connector/onedrive/collections.go index dc39a9469..a482de3d7 100644 --- a/src/internal/connector/onedrive/collections.go +++ b/src/internal/connector/onedrive/collections.go @@ -347,9 +347,14 @@ func (c *Collections) Get( logger.Ctx(ictx).Infow( "persisted metadata for drive", "num_paths_entries", len(paths), - "num_deltas_entries", numDeltas) + "num_deltas_entries", numDeltas, + "delta_reset", delta.Reset) - if !delta.Reset { + // For both cases we don't need to do set difference on folder map if the + // delta token was valid because we should see all the changes. + if !delta.Reset && len(excluded) == 0 { + continue + } else if !delta.Reset { p, err := GetCanonicalPath( fmt.Sprintf(rootDrivePattern, driveID), c.tenant, @@ -376,8 +381,11 @@ func (c *Collections) Get( // Set all folders in previous backup but not in the current // one with state deleted modifiedPaths := map[string]struct{}{} + for _, p := range c.CollectionMap[driveID] { - modifiedPaths[p.FullPath().String()] = struct{}{} + if p.FullPath() != nil { + modifiedPaths[p.FullPath().String()] = struct{}{} + } } for fldID, p := range oldPaths { @@ -493,6 +501,7 @@ func (c *Collections) handleDelete( oldPaths, newPaths map[string]string, isFolder bool, excluded map[string]struct{}, + invalidPrevDelta bool, ) error { if !isFolder { excluded[itemID+DataFileSuffix] = struct{}{} @@ -526,11 +535,18 @@ func (c *Collections) handleDelete( // 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. + if prevPath == nil || invalidPrevDelta { + // 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. + // + // It's also possible the item was made and deleted while getting the delta + // results or our delta token expired and the folder was seen and now is + // marked as deleted. If either of those is the case we should try to delete + // the collection with this ID so it doesn't show up with items. For the + // latter case, we rely on the set difference in the Get() function to find + // folders that need to be marked as deleted and make collections for them. + delete(c.CollectionMap[driveID], itemID) return nil } @@ -659,6 +675,7 @@ func (c *Collections) UpdateCollections( newPaths, isFolder, excluded, + invalidPrevDelta, ); err != nil { return clues.Stack(err).WithClues(ictx) } @@ -671,6 +688,8 @@ func (c *Collections) UpdateCollections( el.AddRecoverable(clues.Stack(err). WithClues(ictx). Label(fault.LabelForceNoBackupCreation)) + + continue } // Skip items that don't match the folder selectors we were given. @@ -763,7 +782,7 @@ func (c *Collections) UpdateCollections( return clues.New("previous collection not found").WithClues(ictx) } - removed := pcollection.Remove(item) + removed := pcollection.Remove(itemID) if !removed { return clues.New("removing from prev collection").WithClues(ictx) } diff --git a/src/internal/connector/onedrive/collections_test.go b/src/internal/connector/onedrive/collections_test.go index 6ded474fd..25448abd6 100644 --- a/src/internal/connector/onedrive/collections_test.go +++ b/src/internal/connector/onedrive/collections_test.go @@ -1912,6 +1912,98 @@ func (suite *OneDriveCollectionsUnitSuite) TestGet() { }, expectedSkippedCount: 2, }, + { + name: "One Drive Delta Error Deleted Folder In New Results", + drives: []models.Driveable{drive1}, + items: map[string][]deltaPagerResult{ + driveID1: { + { + err: getDeltaError(), + }, + { + items: []models.DriveItemable{ + driveRootItem("root"), + driveItem("folder", "folder", driveBasePath1, "root", false, true, false), + driveItem("file", "file", driveBasePath1+"/folder", "folder", true, false, false), + driveItem("folder2", "folder2", driveBasePath1, "root", false, true, false), + driveItem("file2", "file2", driveBasePath1+"/folder2", "folder2", true, false, false), + }, + nextLink: &next, + }, + { + items: []models.DriveItemable{ + driveRootItem("root"), + delItem("folder2", driveBasePath1, "root", false, true, false), + delItem("file2", driveBasePath1, "root", true, false, false), + }, + deltaLink: &delta2, + }, + }, + }, + errCheck: assert.NoError, + prevFolderPaths: map[string]map[string]string{ + driveID1: { + "root": rootFolderPath1, + "folder": folderPath1, + "folder2": expectedPath1("/folder2"), + }, + }, + expectedCollections: map[string]map[data.CollectionState][]string{ + rootFolderPath1: {data.NewState: {}}, + folderPath1: {data.NotMovedState: {"folder", "file"}}, + expectedPath1("/folder2"): {data.DeletedState: {}}, + }, + expectedDeltaURLs: map[string]string{ + driveID1: delta2, + }, + expectedFolderPaths: map[string]map[string]string{ + driveID1: { + "root": rootFolderPath1, + "folder": folderPath1, + }, + }, + expectedDelList: map[string]map[string]struct{}{}, + doNotMergeItems: true, + }, + { + name: "One Drive Delta Error Random Folder Delete", + drives: []models.Driveable{drive1}, + items: map[string][]deltaPagerResult{ + driveID1: { + { + err: getDeltaError(), + }, + { + items: []models.DriveItemable{ + driveRootItem("root"), + delItem("folder", driveBasePath1, "root", false, true, false), + }, + deltaLink: &delta, + }, + }, + }, + errCheck: assert.NoError, + prevFolderPaths: map[string]map[string]string{ + driveID1: { + "root": rootFolderPath1, + "folder": folderPath1, + }, + }, + expectedCollections: map[string]map[data.CollectionState][]string{ + rootFolderPath1: {data.NewState: {}}, + folderPath1: {data.DeletedState: {}}, + }, + expectedDeltaURLs: map[string]string{ + driveID1: delta, + }, + expectedFolderPaths: map[string]map[string]string{ + driveID1: { + "root": rootFolderPath1, + }, + }, + expectedDelList: map[string]map[string]struct{}{}, + doNotMergeItems: true, + }, } for _, test := range table { suite.Run(test.name, func() {