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? - [ ] ✅ Yes, it's included - [ ] 🕐 Yes, but in a later PR - [x] ⛔ No #### Type of change - [ ] 🌻 Feature - [ ] 🐛 Bugfix - [ ] 🗺️ Documentation - [ ] 🤖 Supportability/Tests - [ ] 💻 CI/Deployment - [x] 🧹 Tech Debt/Cleanup #### Issue(s) * closes #3716 #### Test Plan - [x] 💪 Manual - [x] ⚡ Unit test - [ ] 💚 E2E
2023-08-23 12:16:42 -07:00 · 2023-08-23 12:16:42 -07:00 · 7972ff6e8f
commit 7972ff6e8f
parent a39526f64e
4 changed files with 129 additions and 49 deletions
--- 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")
--- 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 {
--- 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 {
--- 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)