From 7972ff6e8f35a635f7cbd9ae92aafcc30dc70896 Mon Sep 17 00:00:00 2001 From: ashmrtn <3891298+ashmrtn@users.noreply.github.com> Date: Wed, 23 Aug 2023 12:16:42 -0700 Subject: [PATCH] Shuffle around logic for details merging (#4087) Shuffle around some logic for details merging so that we always attempt to extract a LocationRef from the backup base entry that's currently being examined. A LocationRef should always be available from either the LocationRef field in the details entry (newer backups) or by extracting it from the RepoRef (older backups) Manually tested incremental backups for exchange in the following scenarios: 1. v0.3.0 backup (calendars use IDs in RepoRef) -> incremental with this patch -> incremental with this patch 1. v0.2.0 backup (exchange uses folder names in RepoRef) -> incremental with this patch -> incremental with this patch The above tests should cover the cases where: * base backup details don't have LocationRef for exchange items * base backup details have LocationRef for exchange items --- #### Does this PR need a docs update or release note? - [ ] :white_check_mark: Yes, it's included - [ ] :clock1: Yes, but in a later PR - [x] :no_entry: No #### Type of change - [ ] :sunflower: Feature - [ ] :bug: Bugfix - [ ] :world_map: Documentation - [ ] :robot: Supportability/Tests - [ ] :computer: CI/Deployment - [x] :broom: Tech Debt/Cleanup #### Issue(s) * closes #3716 #### Test Plan - [x] :muscle: Manual - [x] :zap: Unit test - [ ] :green_heart: E2E --- src/internal/operations/backup.go | 25 ------------ src/internal/operations/backup_test.go | 48 ++++++++++++++++++++++ src/pkg/backup/details/details_test.go | 50 +++++++++++++++++++++-- src/pkg/backup/details/entry.go | 55 ++++++++++++++++---------- 4 files changed, 129 insertions(+), 49 deletions(-) diff --git a/src/internal/operations/backup.go b/src/internal/operations/backup.go index d3d4f0a9f..9215bc0d5 100644 --- a/src/internal/operations/backup.go +++ b/src/internal/operations/backup.go @@ -559,31 +559,6 @@ func getNewPathRefs( repoRef path.Path, backupVersion int, ) (path.Path, *path.Builder, error) { - // Right now we can't guarantee that we have an old location in the - // previous details entry so first try a lookup without a location to see - // if it matches so we don't need to try parsing from the old entry. - // - // TODO(ashmrtn): In the future we can remove this first check as we'll be - // able to assume we always have the location in the previous entry. We'll end - // up doing some extra parsing, but it will simplify this code. - if repoRef.Service() == path.ExchangeService { - newPath, newLoc, err := dataFromBackup.GetNewPathRefs( - repoRef.ToBuilder(), - entry.Modified(), - nil) - if err != nil { - return nil, nil, clues.Wrap(err, "getting new paths") - } else if newPath == nil { - // This entry doesn't need merging. - return nil, nil, nil - } else if newLoc == nil { - return nil, nil, clues.New("unable to find new exchange location") - } - - return newPath, newLoc, nil - } - - // We didn't have an exact entry, so retry with a location. locRef, err := entry.ToLocationIDer(backupVersion) if err != nil { return nil, nil, clues.Wrap(err, "getting previous item location") diff --git a/src/internal/operations/backup_test.go b/src/internal/operations/backup_test.go index 5b489e769..e8970ea1c 100644 --- a/src/internal/operations/backup_test.go +++ b/src/internal/operations/backup_test.go @@ -606,6 +606,24 @@ func (suite *BackupOpUnitSuite) TestBackupOperation_MergeBackupDetails_AddsItems time1 = time.Now() time2 = time1.Add(time.Hour) + + exchangeItemPath1 = makePath( + suite.T(), + []string{ + tenant, + path.ExchangeService.String(), + ro, + path.EmailCategory.String(), + "work", + "item1", + }, + true) + exchangeLocationPath1 = path.Builder{}.Append("work-display-name") + exchangePathReason1 = kopia.NewReason( + "", + exchangeItemPath1.ResourceOwner(), + exchangeItemPath1.Service(), + exchangeItemPath1.Category()) ) itemParents1, err := path.GetDriveFolderPath(itemPath1) @@ -803,6 +821,36 @@ func (suite *BackupOpUnitSuite) TestBackupOperation_MergeBackupDetails_AddsItems makeDetailsEntry(suite.T(), itemPath1, locationPath1, 42, false), }, }, + { + name: "ExchangeItemMerged", + mdm: func() *mockDetailsMergeInfoer { + res := newMockDetailsMergeInfoer() + res.add(exchangeItemPath1, exchangeItemPath1, exchangeLocationPath1) + + return res + }(), + inputBackups: []kopia.BackupEntry{ + { + Backup: &backup1, + Reasons: []identity.Reasoner{ + exchangePathReason1, + }, + }, + }, + populatedDetails: map[string]*details.Details{ + backup1.DetailsID: { + DetailsModel: details.DetailsModel{ + Entries: []details.Entry{ + *makeDetailsEntry(suite.T(), exchangeItemPath1, exchangeLocationPath1, 42, false), + }, + }, + }, + }, + errCheck: assert.NoError, + expectedEntries: []*details.Entry{ + makeDetailsEntry(suite.T(), exchangeItemPath1, exchangeLocationPath1, 42, false), + }, + }, { name: "ItemMergedSameLocation", mdm: func() *mockDetailsMergeInfoer { diff --git a/src/pkg/backup/details/details_test.go b/src/pkg/backup/details/details_test.go index 43883ed5a..1d5e57035 100644 --- a/src/pkg/backup/details/details_test.go +++ b/src/pkg/backup/details/details_test.go @@ -1353,7 +1353,7 @@ func (suite *DetailsUnitSuite) TestLocationIDer_FromEntry() { expectedUniqueLoc: fmt.Sprintf(expectedExchangeUniqueLocFmt, path.EmailCategory), }, { - name: "Exchange Email Without LocationRef Old Version Errors", + name: "Exchange Email Without LocationRef Old Version", service: path.ExchangeService.String(), category: path.EmailCategory.String(), itemInfo: ItemInfo{ @@ -1361,11 +1361,13 @@ func (suite *DetailsUnitSuite) TestLocationIDer_FromEntry() { ItemType: ExchangeMail, }, }, - backupVersion: version.OneDrive7LocationRef - 1, - expectedErr: require.Error, + backupVersion: version.OneDrive7LocationRef - 1, + hasLocRef: true, + expectedErr: require.NoError, + expectedUniqueLoc: fmt.Sprintf(expectedExchangeUniqueLocFmt, path.EmailCategory), }, { - name: "Exchange Email Without LocationRef New Version Errors", + name: "Exchange Email Without LocationRef New Version", service: path.ExchangeService.String(), category: path.EmailCategory.String(), itemInfo: ItemInfo{ @@ -1373,9 +1375,49 @@ func (suite *DetailsUnitSuite) TestLocationIDer_FromEntry() { ItemType: ExchangeMail, }, }, + backupVersion: version.OneDrive7LocationRef, + hasLocRef: true, + expectedErr: require.NoError, + expectedUniqueLoc: fmt.Sprintf(expectedExchangeUniqueLocFmt, path.EmailCategory), + }, + { + name: "Exchange Email Bad RepoRef Fails", + service: path.OneDriveService.String(), + category: path.EmailCategory.String(), + itemInfo: ItemInfo{ + Exchange: &ExchangeInfo{ + ItemType: ExchangeMail, + }, + }, backupVersion: version.OneDrive7LocationRef, expectedErr: require.Error, }, + { + name: "Exchange Event Empty LocationRef New Version Fails", + service: path.ExchangeService.String(), + category: path.EventsCategory.String(), + itemInfo: ItemInfo{ + Exchange: &ExchangeInfo{ + ItemType: ExchangeEvent, + }, + }, + backupVersion: 2, + expectedErr: require.Error, + }, + { + name: "Exchange Event Empty LocationRef Old Version", + service: path.ExchangeService.String(), + category: path.EventsCategory.String(), + itemInfo: ItemInfo{ + Exchange: &ExchangeInfo{ + ItemType: ExchangeEvent, + }, + }, + backupVersion: version.OneDrive1DataAndMetaFiles, + hasLocRef: true, + expectedErr: require.NoError, + expectedUniqueLoc: fmt.Sprintf(expectedExchangeUniqueLocFmt, path.EventsCategory), + }, } for _, test := range table { diff --git a/src/pkg/backup/details/entry.go b/src/pkg/backup/details/entry.go index 47e2d5196..cfadd8641 100644 --- a/src/pkg/backup/details/entry.go +++ b/src/pkg/backup/details/entry.go @@ -56,6 +56,9 @@ type Entry struct { // ToLocationIDer takes a backup version and produces the unique location for // this entry if possible. Reasons it may not be possible to produce the unique // location include an unsupported backup version or missing information. +// +// TODO(ashmrtn): Remove this function completely if we ever decide to sunset +// older corso versions that didn't populate LocationRef. func (de Entry) ToLocationIDer(backupVersion int) (LocationIDer, error) { if len(de.LocationRef) > 0 { baseLoc, err := path.Builder{}.SplitUnescapeAppend(de.LocationRef) @@ -68,32 +71,44 @@ func (de Entry) ToLocationIDer(backupVersion int) (LocationIDer, error) { return de.ItemInfo.uniqueLocation(baseLoc) } - if backupVersion >= version.OneDrive7LocationRef || - (de.ItemInfo.infoType() != OneDriveItem && - de.ItemInfo.infoType() != SharePointLibrary) { - return nil, clues.New("no previous location for entry") - } - - // This is a little hacky, but we only want to try to extract the old - // location if it's OneDrive or SharePoint libraries and it's known to - // be an older backup version. - // - // TODO(ashmrtn): Remove this code once OneDrive/SharePoint libraries - // LocationRef code has been out long enough that all delta tokens for - // previous backup versions will have expired. At that point, either - // we'll do a full backup (token expired, no newer backups) or have a - // backup of a higher version with the information we need. rr, err := path.FromDataLayerPath(de.RepoRef, true) if err != nil { - return nil, clues.Wrap(err, "getting item RepoRef") + return nil, clues.Wrap(err, "getting item RepoRef"). + With("repo_ref", de.RepoRef) } - p, err := path.ToDrivePath(rr) - if err != nil { - return nil, clues.New("converting RepoRef to drive path") + var baseLoc *path.Builder + + switch de.ItemInfo.infoType() { + case ExchangeEvent: + if backupVersion >= 2 { + return nil, clues.New("no previous location for calendar entry"). + With("repo_ref", rr) + } + + fallthrough + case ExchangeMail, ExchangeContact: + baseLoc = path.Builder{}.Append(rr.Folders()...) + + case OneDriveItem, SharePointLibrary: + if backupVersion >= version.OneDrive7LocationRef { + return nil, clues.New("no previous location for drive entry"). + With("repo_ref", rr) + } + + p, err := path.ToDrivePath(rr) + if err != nil { + return nil, clues.New("converting RepoRef to drive path"). + With("repo_ref", rr) + } + + baseLoc = path.Builder{}.Append(p.Root).Append(p.Folders...) } - baseLoc := path.Builder{}.Append(p.Root).Append(p.Folders...) + if baseLoc == nil { + return nil, clues.New("unable to extract LocationRef from RepoRef"). + With("repo_ref", rr) + } // Individual services may add additional info to the base and return that. return de.ItemInfo.uniqueLocation(baseLoc)