From 9a8ec099cb288fb2cec0291e6dbfb59238bfca20 Mon Sep 17 00:00:00 2001 From: ashmrtn Date: Fri, 21 Apr 2023 09:48:12 -0700 Subject: [PATCH] Handle subfolder moves and parent deletions right (#3186) Properly merge items if a subfolder is moved, the original parent is deleted and recreated, and the subfolder is moved back to where it started See linked issue for a more detailed example Manually tested original issue and fix on OneDrive backup --- #### 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 #3185 #### Test Plan - [x] :muscle: Manual - [x] :zap: Unit test - [ ] :green_heart: E2E --- CHANGELOG.md | 1 + src/internal/kopia/upload.go | 10 +- src/internal/kopia/upload_test.go | 146 +++++++++++++++++++++++++++++ src/internal/kopia/wrapper_test.go | 3 +- 4 files changed, 158 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 14637004d..833d9397f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - ParentPath of json output for Exchange calendar now shows names instead of IDs. - Fixed failure when downloading huge amount of attachments - Graph API requests that return an ECONNRESET error are now retried. +- Fixed edge case in incremental backups where moving a subfolder, deleting and recreating the subfolder's original parent folder, and moving the subfolder back to where it started would skip backing up unchanged items in the subfolder. ### Known Issues - Restoring a OneDrive or SharePoint file with the same name as a file with that name as its M365 ID may restore both items. diff --git a/src/internal/kopia/upload.go b/src/internal/kopia/upload.go index 1e1f85e96..df9e40136 100644 --- a/src/internal/kopia/upload.go +++ b/src/internal/kopia/upload.go @@ -727,7 +727,7 @@ func inflateCollectionTree( toMerge *mergeDetails, ) (map[string]*treeMap, map[string]path.Path, error) { roots := make(map[string]*treeMap) - // Contains the old path for collections that have been moved or renamed. + // Contains the old path for collections that are not new. // Allows resolving what the new path should be when walking the base // snapshot(s)'s hierarchy. Nil represents a collection that was deleted. updatedPaths := make(map[string]path.Path) @@ -776,6 +776,14 @@ func inflateCollectionTree( if err := addMergeLocation(s, toMerge); err != nil { return nil, nil, clues.Wrap(err, "adding merge location").WithClues(ictx) } + case data.NotMovedState: + p := s.PreviousPath().String() + if _, ok := updatedPaths[p]; ok { + return nil, nil, clues.New("multiple previous state changes to collection"). + WithClues(ictx) + } + + updatedPaths[p] = s.FullPath() } if s.FullPath() == nil || len(s.FullPath().Elements()) == 0 { diff --git a/src/internal/kopia/upload_test.go b/src/internal/kopia/upload_test.go index 157e8b80c..56c0f4181 100644 --- a/src/internal/kopia/upload_test.go +++ b/src/internal/kopia/upload_test.go @@ -1102,6 +1102,7 @@ func (suite *HierarchyBuilderUnitSuite) TestBuildDirectoryTreeSingleSubtree() { name: "AddsNewItems", inputCollections: func() []data.BackupCollection { mc := exchMock.NewCollection(storePath, locPath, 1) + mc.PrevPath = storePath mc.Names[0] = testFileName2 mc.Data[0] = testFileData2 mc.ColState = data.NotMovedState @@ -1137,6 +1138,7 @@ func (suite *HierarchyBuilderUnitSuite) TestBuildDirectoryTreeSingleSubtree() { name: "SkipsUpdatedItems", inputCollections: func() []data.BackupCollection { mc := exchMock.NewCollection(storePath, locPath, 1) + mc.PrevPath = storePath mc.Names[0] = testFileName mc.Data[0] = testFileData2 mc.ColState = data.NotMovedState @@ -2054,6 +2056,150 @@ func (suite *HierarchyBuilderUnitSuite) TestBuildDirectoryTreeMultipleSubdirecto }, ), }, + { + // This could happen if a subfolder is moved out of the parent, the parent + // is deleted, a new folder at the same location as the parent is created, + // and then the subfolder is moved back to the same location. + name: "Delete Parent But Child Marked Not Moved Explicit New Parent", + inputCollections: func(t *testing.T) []data.BackupCollection { + inbox := exchMock.NewCollection(nil, inboxLocPath, 0) + inbox.PrevPath = inboxStorePath + inbox.ColState = data.DeletedState + + inbox2 := exchMock.NewCollection(inboxStorePath, inboxLocPath, 1) + inbox2.PrevPath = nil + inbox2.ColState = data.NewState + inbox2.Names[0] = workFileName1 + + personal := exchMock.NewCollection(personalStorePath, personalLocPath, 0) + personal.PrevPath = personalStorePath + personal.ColState = data.NotMovedState + + return []data.BackupCollection{inbox, inbox2, personal} + }, + expected: expectedTreeWithChildren( + []string{ + testTenant, + service, + testUser, + category, + }, + []*expectedNode{ + { + name: testInboxID, + children: []*expectedNode{ + { + name: workFileName1, + children: []*expectedNode{}, + }, + { + name: personalID, + children: []*expectedNode{ + { + name: personalFileName1, + children: []*expectedNode{}, + }, + { + name: personalFileName2, + children: []*expectedNode{}, + }, + }, + }, + }, + }, + }, + ), + }, + { + // This could happen if a subfolder is moved out of the parent, the parent + // is deleted, a new folder at the same location as the parent is created, + // and then the subfolder is moved back to the same location. + name: "Delete Parent But Child Marked Not Moved Implicit New Parent", + inputCollections: func(t *testing.T) []data.BackupCollection { + inbox := exchMock.NewCollection(nil, inboxLocPath, 0) + inbox.PrevPath = inboxStorePath + inbox.ColState = data.DeletedState + + // New folder not explicitly listed as it may not have had new items. + personal := exchMock.NewCollection(personalStorePath, personalLocPath, 0) + personal.PrevPath = personalStorePath + personal.ColState = data.NotMovedState + + return []data.BackupCollection{inbox, personal} + }, + expected: expectedTreeWithChildren( + []string{ + testTenant, + service, + testUser, + category, + }, + []*expectedNode{ + { + name: testInboxID, + children: []*expectedNode{ + { + name: personalID, + children: []*expectedNode{ + { + name: personalFileName1, + children: []*expectedNode{}, + }, + { + name: personalFileName2, + children: []*expectedNode{}, + }, + }, + }, + }, + }, + }, + ), + }, + { + // This could happen if a subfolder is moved out of the parent, the parent + // is deleted, a new folder at the same location as the parent is created, + // and then the subfolder is moved back to the same location. + name: "Delete Parent But Child Marked Not Moved Implicit New Parent Child Do Not Merge", + inputCollections: func(t *testing.T) []data.BackupCollection { + inbox := exchMock.NewCollection(nil, inboxLocPath, 0) + inbox.PrevPath = inboxStorePath + inbox.ColState = data.DeletedState + + // New folder not explicitly listed as it may not have had new items. + personal := exchMock.NewCollection(personalStorePath, personalLocPath, 1) + personal.PrevPath = personalStorePath + personal.ColState = data.NotMovedState + personal.DoNotMerge = true + personal.Names[0] = workFileName1 + + return []data.BackupCollection{inbox, personal} + }, + expected: expectedTreeWithChildren( + []string{ + testTenant, + service, + testUser, + category, + }, + []*expectedNode{ + { + name: testInboxID, + children: []*expectedNode{ + { + name: personalID, + children: []*expectedNode{ + { + name: workFileName1, + children: []*expectedNode{}, + }, + }, + }, + }, + }, + }, + ), + }, } for _, test := range table { diff --git a/src/internal/kopia/wrapper_test.go b/src/internal/kopia/wrapper_test.go index 684d3fae2..1da2f1a84 100644 --- a/src/internal/kopia/wrapper_test.go +++ b/src/internal/kopia/wrapper_test.go @@ -620,7 +620,7 @@ func (c mockBackupCollection) FullPath() path.Path { } func (c mockBackupCollection) PreviousPath() path.Path { - return nil + return c.path } func (c mockBackupCollection) LocationPath() *path.Builder { @@ -1034,6 +1034,7 @@ func (suite *KopiaSimpleRepoIntegrationSuite) TestBackupExcludeItem() { suite.testPath1, 1) c.ColState = data.NotMovedState + c.PrevPath = suite.testPath1 return []data.BackupCollection{c} },