Switch LocationPath return type (#3038)

LocationRefs don't need prefix directories, so using a path.Path is overkill. This will also fit better with changes to come where we need to parse and update the LocationRef of merged backup details entries. --- #### 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) * #2486 #### Test Plan - [x] 💪 Manual - [ ] ⚡ Unit test - [ ] 💚 E2E
2023-04-05 14:10:42 -07:00 · 2023-04-05 14:10:42 -07:00 · 59e17757bf
commit 59e17757bf
parent 7b8a00efc2
11 changed files with 58 additions and 118 deletions
--- a/src/internal/connector/exchange/exchange_data_collection.go
+++ b/src/internal/connector/exchange/exchange_data_collection.go
@ -82,7 +82,7 @@ type Collection struct {

 	// LocationPath contains the path with human-readable display names.
 	// IE: "/Inbox/Important" instead of "/abcdxyz123/algha=lgkhal=t"
-	locationPath path.Path
+	locationPath *path.Builder

 	state data.CollectionState

@ -98,7 +98,8 @@ type Collection struct {
 // or notMoved (if they match).
 func NewCollection(
 	user string,
-	curr, prev, location path.Path,
+	curr, prev path.Path,
+	location *path.Builder,
 	category path.CategoryType,
 	items itemer,
 	statusUpdater support.StatusUpdater,
@ -138,7 +139,7 @@ func (col *Collection) FullPath() path.Path {

 // LocationPath produces the Collection's full path, but with display names
 // instead of IDs in the folders.  Only populated for Calendars.
-func (col *Collection) LocationPath() path.Path {
+func (col *Collection) LocationPath() *path.Builder {
 	return col.locationPath
 }

@ -278,7 +279,7 @@ func (col *Collection) streamItems(ctx context.Context, errs *fault.Bus) {
 			}

 			info.Size = int64(len(data))
-			info.ParentPath = col.locationPath.Folder(true)
+			info.ParentPath = col.locationPath.String()

 			col.data <- &Stream{
 				id:      id,
--- a/src/internal/connector/exchange/exchange_data_collection_test.go
+++ b/src/internal/connector/exchange/exchange_data_collection_test.go
@ -133,34 +133,34 @@ func (suite *ExchangeDataCollectionSuite) TestNewCollection_state() {
 	require.NoError(suite.T(), err, clues.ToCore(err))
 	barP, err := path.Build("t", "u", path.ExchangeService, path.EmailCategory, false, "bar")
 	require.NoError(suite.T(), err, clues.ToCore(err))
-	locP, err := path.Build("t", "u", path.ExchangeService, path.EmailCategory, false, "human-readable")
-	require.NoError(suite.T(), err, clues.ToCore(err))
+
+	locPB := path.Builder{}.Append("human-readable")

 	table := []struct {
 		name   string
 		prev   path.Path
 		curr   path.Path
-		loc    path.Path
+		loc    *path.Builder
 		expect data.CollectionState
 	}{
 		{
 			name:   "new",
 			curr:   fooP,
-			loc:    locP,
+			loc:    locPB,
 			expect: data.NewState,
 		},
 		{
 			name:   "not moved",
 			prev:   fooP,
 			curr:   fooP,
-			loc:    locP,
+			loc:    locPB,
 			expect: data.NotMovedState,
 		},
 		{
 			name:   "moved",
 			prev:   fooP,
 			curr:   barP,
-			loc:    locP,
+			loc:    locPB,
 			expect: data.MovedState,
 		},
 		{
--- a/src/internal/connector/exchange/service_functions.go
+++ b/src/internal/connector/exchange/service_functions.go
@ -95,7 +95,7 @@ func includeContainer(
 	qp graph.QueryParams,
 	c graph.CachedContainer,
 	scope selectors.ExchangeScope,
-) (path.Path, path.Path, bool) {
+) (path.Path, *path.Builder, bool) {
 	var (
 		directory string
 		locPath   path.Path
@ -154,5 +154,5 @@ func includeContainer(
 		return nil, nil, false
 	}

-	return pathRes, locPath, ok
+	return pathRes, loc, ok
 }
--- a/src/internal/data/data_collection.go
+++ b/src/internal/data/data_collection.go
@ -101,7 +101,7 @@ type Stream interface {
 // LocationPather provides a LocationPath describing the path with Display Names
 // instead of canonical IDs
 type LocationPather interface {
-	LocationPath() path.Path
+	LocationPath() *path.Builder
 }

 // StreamInfo is used to provide service specific
--- a/src/internal/kopia/upload.go
+++ b/src/internal/kopia/upload.go
@ -127,7 +127,7 @@ type itemDetails struct {
 	info         *details.ItemInfo
 	repoPath     path.Path
 	prevPath     path.Path
-	locationPath path.Path
+	locationPath *path.Builder
 	cached       bool
 }

@ -205,20 +205,11 @@ func (cp *corsoProgress) FinishedFile(relativePath string, err error) {

 	var (
 		locationFolders string
-		locPB           *path.Builder
 		parent          = d.repoPath.ToBuilder().Dir()
 	)

 	if d.locationPath != nil {
-		locationFolders = d.locationPath.Folder(true)
-
-		locPB = d.locationPath.ToBuilder()
-
-		// folderEntriesForPath assumes the location will
-		// not have an item element appended
-		if len(d.locationPath.Item()) > 0 {
-			locPB = locPB.Dir()
-		}
+		locationFolders = d.locationPath.String()
 	}

 	err = cp.deets.Add(
@ -239,7 +230,7 @@ func (cp *corsoProgress) FinishedFile(relativePath string, err error) {
 		return
 	}

-	folders := details.FolderEntriesForPath(parent, locPB)
+	folders := details.FolderEntriesForPath(parent, d.locationPath)
 	cp.deets.AddFoldersForItem(
 		folders,
 		*d.info,
@ -328,7 +319,7 @@ func collectionEntries(
 	}

 	var (
-		locationPath path.Path
+		locationPath *path.Builder
 		// Track which items have already been seen so we can skip them if we see
 		// them again in the data from the base snapshot.
 		seen  = map[string]struct{}{}
@ -431,7 +422,7 @@ func streamBaseEntries(
 	cb func(context.Context, fs.Entry) error,
 	curPath path.Path,
 	prevPath path.Path,
-	locationPath path.Path,
+	locationPath *path.Builder,
 	dir fs.Directory,
 	encodedSeen map[string]struct{},
 	globalExcludeSet map[string]map[string]struct{},
@ -556,7 +547,7 @@ func getStreamItemFunc(
 			}
 		}

-		var locationPath path.Path
+		var locationPath *path.Builder

 		if lp, ok := streamedEnts.(data.LocationPather); ok {
 			locationPath = lp.LocationPath()
--- a/src/internal/kopia/upload_test.go
+++ b/src/internal/kopia/upload_test.go
@ -345,6 +345,7 @@ func (suite *VersionReadersUnitSuite) TestWriteHandlesShortReads() {
 type CorsoProgressUnitSuite struct {
 	tester.Suite
 	targetFilePath path.Path
+	targetFileLoc  *path.Builder
 	targetFileName string
 }

@ -363,6 +364,7 @@ func (suite *CorsoProgressUnitSuite) SetupSuite() {
 	require.NoError(suite.T(), err, clues.ToCore(err))

 	suite.targetFilePath = p
+	suite.targetFileLoc = path.Builder{}.Append(testInboxDir)
 	suite.targetFileName = suite.targetFilePath.ToBuilder().Dir().String()
 }

@ -596,7 +598,7 @@ func (suite *CorsoProgressUnitSuite) TestFinishedFileBaseItemDoesntBuildHierarch
 	expectedToMerge := map[string]PrevRefs{
 		prevPath.ShortRef(): {
 			Repo:     suite.targetFilePath,
-			Location: suite.targetFilePath,
+			Location: suite.targetFileLoc,
 		},
 	}

@ -614,7 +616,7 @@ func (suite *CorsoProgressUnitSuite) TestFinishedFileBaseItemDoesntBuildHierarch
 		info:         nil,
 		repoPath:     suite.targetFilePath,
 		prevPath:     prevPath,
-		locationPath: suite.targetFilePath,
+		locationPath: suite.targetFileLoc,
 	}

 	cp.put(suite.targetFileName, deets)
--- a/src/internal/kopia/wrapper.go
+++ b/src/internal/kopia/wrapper.go
@ -128,7 +128,7 @@ type IncrementalBase struct {
 // that need to be merged in from prior snapshots.
 type PrevRefs struct {
 	Repo     path.Path
-	Location path.Path
+	Location *path.Builder
 }

 // ConsumeBackupCollections takes a set of collections and creates a kopia snapshot
--- a/src/internal/operations/backup.go
+++ b/src/internal/operations/backup.go
@ -583,12 +583,10 @@ func mergeDetails(
 			var (
 				itemUpdated = newPath.String() != rr.String()
 				newLocStr   string
-				locBuilder  *path.Builder
 			)

 			if newLoc != nil {
-				locBuilder = newLoc.ToBuilder()
-				newLocStr = newLoc.Folder(true)
+				newLocStr = newLoc.String()
 				itemUpdated = itemUpdated || newLocStr != entry.LocationRef
 			}

@ -603,7 +601,7 @@ func mergeDetails(
 				return clues.Wrap(err, "adding item to details")
 			}

-			folders := details.FolderEntriesForPath(newPath.ToBuilder().Dir(), locBuilder)
+			folders := details.FolderEntriesForPath(newPath.ToBuilder().Dir(), newLoc)
 			deets.AddFoldersForItem(folders, item, itemUpdated)

 			// Track how many entries we added so that we know if we got them all when
--- a/src/internal/operations/backup_test.go
+++ b/src/internal/operations/backup_test.go
@ -265,7 +265,7 @@ func makePath(t *testing.T, elements []string, isItem bool) path.Path {
 func makeDetailsEntry(
 	t *testing.T,
 	p path.Path,
-	l path.Path,
+	l *path.Builder,
 	size int,
 	updated bool,
 ) *details.DetailsEntry {
@ -273,7 +273,7 @@ func makeDetailsEntry(

 	var lr string
 	if l != nil {
-		lr = l.PopFront().PopFront().PopFront().PopFront().Dir().String()
+		lr = l.String()
 	}

 	res := &details.DetailsEntry{
@ -298,7 +298,7 @@ func makeDetailsEntry(
 		res.Exchange = &details.ExchangeInfo{
 			ItemType:   details.ExchangeMail,
 			Size:       int64(size),
-			ParentPath: l.Folder(false),
+			ParentPath: l.String(),
 		}

 	case path.OneDriveService:
@ -608,22 +608,8 @@ func (suite *BackupOpUnitSuite) TestBackupOperation_MergeBackupDetails_AddsItems
 			},
 			true,
 		)
-		locationPath1 = makePath(
-			suite.T(),
-			[]string{
-				tenant,
-				path.OneDriveService.String(),
-				ro,
-				path.FilesCategory.String(),
-				"drives",
-				"drive-id",
-				"root:",
-				"work-display-name",
-				"item1",
-			},
-			true,
-		)
-		itemPath2 = makePath(
+		locationPath1 = path.Builder{}.Append("root:", "work-display-name")
+		itemPath2     = makePath(
 			suite.T(),
 			[]string{
 				tenant,
@ -638,22 +624,8 @@ func (suite *BackupOpUnitSuite) TestBackupOperation_MergeBackupDetails_AddsItems
 			},
 			true,
 		)
-		locationPath2 = makePath(
-			suite.T(),
-			[]string{
-				tenant,
-				path.OneDriveService.String(),
-				ro,
-				path.FilesCategory.String(),
-				"drives",
-				"drive-id",
-				"root:",
-				"personal-display-name",
-				"item2",
-			},
-			true,
-		)
-		itemPath3 = makePath(
+		locationPath2 = path.Builder{}.Append("root:", "personal-display-name")
+		itemPath3     = makePath(
 			suite.T(),
 			[]string{
 				tenant,
@ -665,18 +637,7 @@ func (suite *BackupOpUnitSuite) TestBackupOperation_MergeBackupDetails_AddsItems
 			},
 			true,
 		)
-		locationPath3 = makePath(
-			suite.T(),
-			[]string{
-				tenant,
-				path.ExchangeService.String(),
-				ro,
-				path.EmailCategory.String(),
-				"personal-display-name",
-				"item3",
-			},
-			true,
-		)
+		locationPath3 = path.Builder{}.Append("personal-display-name")

 		backup1 = backup.Backup{
 			BaseModel: model.BaseModel{
@ -801,7 +762,7 @@ func (suite *BackupOpUnitSuite) TestBackupOperation_MergeBackupDetails_AddsItems
 				backup1.DetailsID: {
 					DetailsModel: details.DetailsModel{
 						Entries: []details.DetailsEntry{
-							*makeDetailsEntry(suite.T(), itemPath1, itemPath1, 42, false),
+							*makeDetailsEntry(suite.T(), itemPath1, locationPath1, 42, false),
 						},
 					},
 				},
@ -837,7 +798,7 @@ func (suite *BackupOpUnitSuite) TestBackupOperation_MergeBackupDetails_AddsItems
 				backup1.DetailsID: {
 					DetailsModel: details.DetailsModel{
 						Entries: []details.DetailsEntry{
-							*makeDetailsEntry(suite.T(), itemPath1, itemPath1, 42, false),
+							*makeDetailsEntry(suite.T(), itemPath1, locationPath1, 42, false),
 						},
 					},
 				},
@ -926,7 +887,7 @@ func (suite *BackupOpUnitSuite) TestBackupOperation_MergeBackupDetails_AddsItems
 				backup1.DetailsID: {
 					DetailsModel: details.DetailsModel{
 						Entries: []details.DetailsEntry{
-							*makeDetailsEntry(suite.T(), itemPath1, itemPath1, 42, false),
+							*makeDetailsEntry(suite.T(), itemPath1, locationPath1, 42, false),
 						},
 					},
 				},
@ -1003,7 +964,7 @@ func (suite *BackupOpUnitSuite) TestBackupOperation_MergeBackupDetails_AddsItems
 			inputShortRefsFromPrevBackup: map[string]kopia.PrevRefs{
 				itemPath1.ShortRef(): {
 					Repo:     itemPath1,
-					Location: itemPath1,
+					Location: locationPath1,
 				},
 			},
 			inputMans: []*kopia.ManifestEntry{
@ -1021,14 +982,14 @@ func (suite *BackupOpUnitSuite) TestBackupOperation_MergeBackupDetails_AddsItems
 				backup1.DetailsID: {
 					DetailsModel: details.DetailsModel{
 						Entries: []details.DetailsEntry{
-							*makeDetailsEntry(suite.T(), itemPath1, itemPath1, 42, false),
+							*makeDetailsEntry(suite.T(), itemPath1, locationPath1, 42, false),
 						},
 					},
 				},
 			},
 			errCheck: assert.NoError,
 			expectedEntries: []*details.DetailsEntry{
-				makeDetailsEntry(suite.T(), itemPath1, itemPath1, 42, false),
+				makeDetailsEntry(suite.T(), itemPath1, locationPath1, 42, false),
 			},
 		},
 		{
@ -1254,10 +1215,7 @@ func (suite *BackupOpUnitSuite) TestBackupOperation_MergeBackupDetails_AddsFolde
 			pathElems,
 			true)

-		locPath1 = makePath(
-			t,
-			pathElems[:len(pathElems)-1],
-			false)
+		locPath1 = path.Builder{}.Append(pathElems[:len(pathElems)-1]...)

 		backup1 = backup.Backup{
 			BaseModel: model.BaseModel{
@ -1297,7 +1255,7 @@ func (suite *BackupOpUnitSuite) TestBackupOperation_MergeBackupDetails_AddsFolde
 		// later    = now.Add(42 * time.Minute)
 	)

-	itemDetails := makeDetailsEntry(t, itemPath1, itemPath1, itemSize, false)
+	itemDetails := makeDetailsEntry(t, itemPath1, locPath1, itemSize, false)
 	// itemDetails.Exchange.Modified = now

 	populatedDetails := map[string]*details.Details{
--- a/src/pkg/backup/details/details.go
+++ b/src/pkg/backup/details/details.go
@ -468,10 +468,10 @@ const (
 	FolderItem ItemType = 306
 )

-func UpdateItem(item *ItemInfo, repoPath, locPath path.Path) error {
+func UpdateItem(item *ItemInfo, repoPath path.Path, locPath *path.Builder) error {
 	// Only OneDrive and SharePoint have information about parent folders
 	// contained in them.
-	var updatePath func(repo path.Path, location path.Path) error
+	var updatePath func(repo path.Path, location *path.Builder) error

 	switch item.infoType() {
 	case ExchangeContact, ExchangeEvent, ExchangeMail:
@ -632,13 +632,13 @@ func (i ExchangeInfo) Values() []string {
 	return []string{}
 }

-func (i *ExchangeInfo) UpdateParentPath(_, locPath path.Path) error {
+func (i *ExchangeInfo) UpdateParentPath(_ path.Path, locPath *path.Builder) error {
 	// Not all data types have this set yet.
 	if locPath == nil {
 		return nil
 	}

-	i.ParentPath = locPath.Folder(true)
+	i.ParentPath = locPath.String()

 	return nil
 }
@ -677,7 +677,7 @@ func (i SharePointInfo) Values() []string {
 	}
 }

-func (i *SharePointInfo) UpdateParentPath(newPath, _ path.Path) error {
+func (i *SharePointInfo) UpdateParentPath(newPath path.Path, _ *path.Builder) error {
 	newParent, err := path.GetDriveFolderPath(newPath)
 	if err != nil {
 		return clues.Wrap(err, "making sharePoint path").With("path", newPath)
@ -721,7 +721,7 @@ func (i OneDriveInfo) Values() []string {
 	}
 }

-func (i *OneDriveInfo) UpdateParentPath(newPath, _ path.Path) error {
+func (i *OneDriveInfo) UpdateParentPath(newPath path.Path, _ *path.Builder) error {
 	newParent, err := path.GetDriveFolderPath(newPath)
 	if err != nil {
 		return clues.Wrap(err, "making oneDrive path").With("path", newPath)
--- a/src/pkg/backup/details/details_test.go
+++ b/src/pkg/backup/details/details_test.go
@ -880,17 +880,7 @@ func (suite *DetailsUnitSuite) TestUpdateItem() {
 			item,
 		},
 	)
-	newExchangePath := makeItemPath(
-		suite.T(),
-		path.ExchangeService,
-		path.EmailCategory,
-		tenant,
-		resourceOwner,
-		[]string{
-			folder3,
-			item,
-		},
-	)
+	newExchangePB := path.Builder{}.Append(folder3)
 	badOneDrivePath := makeItemPath(
 		suite.T(),
 		path.OneDriveService,
@ -904,7 +894,7 @@ func (suite *DetailsUnitSuite) TestUpdateItem() {
 		name         string
 		input        ItemInfo
 		repoPath     path.Path
-		locPath      path.Path
+		locPath      *path.Builder
 		errCheck     assert.ErrorAssertionFunc
 		expectedItem ItemInfo
 	}{
@ -917,7 +907,7 @@ func (suite *DetailsUnitSuite) TestUpdateItem() {
 				},
 			},
 			repoPath: newOneDrivePath,
-			locPath:  newExchangePath,
+			locPath:  newExchangePB,
 			errCheck: assert.NoError,
 			expectedItem: ItemInfo{
 				Exchange: &ExchangeInfo{
@ -935,7 +925,7 @@ func (suite *DetailsUnitSuite) TestUpdateItem() {
 				},
 			},
 			repoPath: newOneDrivePath,
-			locPath:  newExchangePath,
+			locPath:  newExchangePB,
 			errCheck: assert.NoError,
 			expectedItem: ItemInfo{
 				Exchange: &ExchangeInfo{
@ -953,7 +943,7 @@ func (suite *DetailsUnitSuite) TestUpdateItem() {
 				},
 			},
 			repoPath: newOneDrivePath,
-			locPath:  newExchangePath,
+			locPath:  newExchangePB,
 			errCheck: assert.NoError,
 			expectedItem: ItemInfo{
 				Exchange: &ExchangeInfo{
@ -971,7 +961,7 @@ func (suite *DetailsUnitSuite) TestUpdateItem() {
 				},
 			},
 			repoPath: newOneDrivePath,
-			locPath:  newExchangePath,
+			locPath:  newExchangePB,
 			errCheck: assert.NoError,
 			expectedItem: ItemInfo{
 				OneDrive: &OneDriveInfo{
@ -989,7 +979,7 @@ func (suite *DetailsUnitSuite) TestUpdateItem() {
 				},
 			},
 			repoPath: newOneDrivePath,
-			locPath:  newExchangePath,
+			locPath:  newExchangePB,
 			errCheck: assert.NoError,
 			expectedItem: ItemInfo{
 				SharePoint: &SharePointInfo{
@ -1007,7 +997,7 @@ func (suite *DetailsUnitSuite) TestUpdateItem() {
 				},
 			},
 			repoPath: badOneDrivePath,
-			locPath:  newExchangePath,
+			locPath:  newExchangePB,
 			errCheck: assert.Error,
 		},
 		{
@ -1019,7 +1009,7 @@ func (suite *DetailsUnitSuite) TestUpdateItem() {
 				},
 			},
 			repoPath: badOneDrivePath,
-			locPath:  newExchangePath,
+			locPath:  newExchangePB,
 			errCheck: assert.Error,
 		},
 	}