From 1b417af5bb1e6f250adb7e21216a731e9decb909 Mon Sep 17 00:00:00 2001 From: Keepers Date: Thu, 11 May 2023 19:29:17 -0600 Subject: [PATCH] minor updates to the path.Path interface (#3384) Swaps path.Append from a single item method to accept a variadic list of string elements. Since 95% of all calls to path.Append were items, also adds a shorthand AppendItem func to the interface for easy clarity. Finally, adds a Last() method to elements for getting the last element in the slice. --- #### Does this PR need a docs update or release note? - [x] :no_entry: No #### Type of change - [x] :broom: Tech Debt/Cleanup #### Test Plan - [x] :zap: Unit test - [x] :green_heart: E2E --- .../connector/exchange/service_restore.go | 2 +- .../connector/onedrive/collections.go | 2 +- src/internal/connector/onedrive/restore.go | 8 ++++---- src/internal/connector/sharepoint/restore.go | 4 ++-- src/internal/kopia/upload.go | 6 +++--- src/internal/kopia/wrapper_test.go | 14 ++++++------- src/internal/operations/manifests_test.go | 12 +++++------ src/pkg/backup/details/testdata/testdata.go | 2 +- src/pkg/path/elements.go | 9 +++++++++ src/pkg/path/path.go | 4 +++- src/pkg/path/path_test.go | 20 +++++++++++++++++++ src/pkg/path/resource_path.go | 8 ++++++-- src/pkg/path/resource_path_test.go | 2 +- 13 files changed, 64 insertions(+), 29 deletions(-) diff --git a/src/internal/connector/exchange/service_restore.go b/src/internal/connector/exchange/service_restore.go index 8ac120619..9e293ce5d 100644 --- a/src/internal/connector/exchange/service_restore.go +++ b/src/internal/connector/exchange/service_restore.go @@ -435,7 +435,7 @@ func restoreCollection( metrics.Bytes += int64(len(byteArray)) metrics.Successes++ - itemPath, err := dc.FullPath().Append(itemData.UUID(), true) + itemPath, err := dc.FullPath().AppendItem(itemData.UUID()) if err != nil { errs.AddRecoverable(clues.Wrap(err, "building full path with item").WithClues(ctx)) continue diff --git a/src/internal/connector/onedrive/collections.go b/src/internal/connector/onedrive/collections.go index 4cb1944f7..52f29f879 100644 --- a/src/internal/connector/onedrive/collections.go +++ b/src/internal/connector/onedrive/collections.go @@ -677,7 +677,7 @@ func (c *Collections) getCollectionPath( return nil, clues.New("folder with empty name") } - collectionPath, err = collectionPath.Append(name, false) + collectionPath, err = collectionPath.Append(false, name) if err != nil { return nil, clues.Wrap(err, "making non-root folder path") } diff --git a/src/internal/connector/onedrive/restore.go b/src/internal/connector/onedrive/restore.go index 3f34cc9c4..41d037b13 100644 --- a/src/internal/connector/onedrive/restore.go +++ b/src/internal/connector/onedrive/restore.go @@ -231,7 +231,7 @@ func RestoreCollection( return metrics, nil } - itemPath, err := dc.FullPath().Append(itemData.UUID(), true) + itemPath, err := dc.FullPath().AppendItem(itemData.UUID()) if err != nil { el.AddRecoverable(clues.Wrap(err, "appending item to full path").WithClues(ctx)) continue @@ -852,7 +852,7 @@ func AugmentRestorePaths( el := p.StoragePath.Elements() if backupVersion >= version.OneDrive6NameInMeta { - mPath, err := p.StoragePath.Append(".dirmeta", true) + mPath, err := p.StoragePath.AppendItem(".dirmeta") if err != nil { return nil, err } @@ -861,7 +861,7 @@ func AugmentRestorePaths( paths, path.RestorePaths{StoragePath: mPath, RestorePath: p.RestorePath}) } else if backupVersion >= version.OneDrive4DirIncludesPermissions { - mPath, err := p.StoragePath.Append(el[len(el)-1]+".dirmeta", true) + mPath, err := p.StoragePath.AppendItem(el.Last() + ".dirmeta") if err != nil { return nil, err } @@ -875,7 +875,7 @@ func AugmentRestorePaths( return nil, err } - mPath, err := pp.Append(el[len(el)-1]+".dirmeta", true) + mPath, err := pp.AppendItem(el.Last() + ".dirmeta") if err != nil { return nil, err } diff --git a/src/internal/connector/sharepoint/restore.go b/src/internal/connector/sharepoint/restore.go index 642e9fd32..2f64454da 100644 --- a/src/internal/connector/sharepoint/restore.go +++ b/src/internal/connector/sharepoint/restore.go @@ -251,7 +251,7 @@ func RestoreListCollection( metrics.Bytes += itemInfo.SharePoint.Size - itemPath, err := dc.FullPath().Append(itemData.UUID(), true) + itemPath, err := dc.FullPath().AppendItem(itemData.UUID()) if err != nil { el.AddRecoverable(clues.Wrap(err, "appending item to full path").WithClues(ctx)) continue @@ -339,7 +339,7 @@ func RestorePageCollection( metrics.Bytes += itemInfo.SharePoint.Size - itemPath, err := dc.FullPath().Append(itemData.UUID(), true) + itemPath, err := dc.FullPath().AppendItem(itemData.UUID()) if err != nil { el.AddRecoverable(clues.Wrap(err, "appending item to full path").WithClues(ctx)) continue diff --git a/src/internal/kopia/upload.go b/src/internal/kopia/upload.go index 6f7f5388c..a1cc0bed2 100644 --- a/src/internal/kopia/upload.go +++ b/src/internal/kopia/upload.go @@ -347,7 +347,7 @@ func collectionEntries( seen[encodedName] = struct{}{} // For now assuming that item IDs don't need escaping. - itemPath, err := streamedEnts.FullPath().Append(e.UUID(), true) + itemPath, err := streamedEnts.FullPath().AppendItem(e.UUID()) if err != nil { err = clues.Wrap(err, "getting full item path") progress.errs.AddRecoverable(err) @@ -464,7 +464,7 @@ func streamBaseEntries( } // For now assuming that item IDs don't need escaping. - itemPath, err := curPath.Append(entName, true) + itemPath, err := curPath.AppendItem(entName) if err != nil { return clues.Wrap(err, "getting full item path for base entry") } @@ -473,7 +473,7 @@ func streamBaseEntries( // backup details. If the item moved and we had only the new path, we'd be // unable to find it in the old backup details because we wouldn't know what // to look for. - prevItemPath, err := prevPath.Append(entName, true) + prevItemPath, err := prevPath.AppendItem(entName) if err != nil { return clues.Wrap(err, "getting previous full item path for base entry") } diff --git a/src/internal/kopia/wrapper_test.go b/src/internal/kopia/wrapper_test.go index abe96fdc2..48041cd91 100644 --- a/src/internal/kopia/wrapper_test.go +++ b/src/internal/kopia/wrapper_test.go @@ -73,7 +73,7 @@ func testForFiles( for s := range c.Items(ctx, fault.New(true)) { count++ - fullPath, err := c.FullPath().Append(s.UUID(), true) + fullPath, err := c.FullPath().AppendItem(s.UUID()) require.NoError(t, err, clues.ToCore(err)) expected, ok := expected[fullPath.String()] @@ -689,10 +689,10 @@ func (suite *KopiaIntegrationSuite) TestRestoreAfterCompressionChange() { dc1 := exchMock.NewCollection(suite.storePath1, suite.locPath1, 1) dc2 := exchMock.NewCollection(suite.storePath2, suite.locPath2, 1) - fp1, err := suite.storePath1.Append(dc1.Names[0], true) + fp1, err := suite.storePath1.AppendItem(dc1.Names[0]) require.NoError(t, err, clues.ToCore(err)) - fp2, err := suite.storePath2.Append(dc2.Names[0], true) + fp2, err := suite.storePath2.AppendItem(dc2.Names[0]) require.NoError(t, err, clues.ToCore(err)) stats, _, _, err := w.ConsumeBackupCollections( @@ -838,7 +838,7 @@ func (suite *KopiaIntegrationSuite) TestBackupCollections_ReaderError() { // 5 file and 2 folder entries. assert.Len(t, deets.Details().Entries, 5+2) - failedPath, err := suite.storePath2.Append(testFileName4, true) + failedPath, err := suite.storePath2.AppendItem(testFileName4) require.NoError(t, err, clues.ToCore(err)) ic := i64counter{} @@ -987,7 +987,7 @@ func (suite *KopiaSimpleRepoIntegrationSuite) SetupSuite() { } for _, item := range filesInfo { - pth, err := item.parentPath.Append(item.name, true) + pth, err := item.parentPath.AppendItem(item.name) require.NoError(suite.T(), err, clues.ToCore(err)) mapKey := item.parentPath.String() @@ -1439,7 +1439,7 @@ func (suite *KopiaSimpleRepoIntegrationSuite) TestProduceRestoreCollections_Path item, ok := suite.filesByPath[pth.StoragePath.String()] require.True(t, ok, "getting expected file data") - itemPath, err := pth.RestorePath.Append(pth.StoragePath.Item(), true) + itemPath, err := pth.RestorePath.AppendItem(pth.StoragePath.Item()) require.NoError(t, err, "getting expected item path") expected[itemPath.String()] = item.data @@ -1532,7 +1532,7 @@ func (suite *KopiaSimpleRepoIntegrationSuite) TestProduceRestoreCollections_Fetc } func (suite *KopiaSimpleRepoIntegrationSuite) TestProduceRestoreCollections_Errors() { - itemPath, err := suite.testPath1.Append(testFileName, true) + itemPath, err := suite.testPath1.AppendItem(testFileName) require.NoError(suite.T(), err, clues.ToCore(err)) table := []struct { diff --git a/src/internal/operations/manifests_test.go b/src/internal/operations/manifests_test.go index aa481ade7..ccef6e248 100644 --- a/src/internal/operations/manifests_test.go +++ b/src/internal/operations/manifests_test.go @@ -140,7 +140,7 @@ func (suite *OperationsManifestsUnitSuite) TestCollectMetadata() { ps := make([]path.Path, 0, len(files)) for _, f := range files { - p, err := emailPath.Append(f, true) + p, err := emailPath.AppendItem(f) assert.NoError(t, err, clues.ToCore(err)) ps = append(ps, p) } @@ -163,7 +163,7 @@ func (suite *OperationsManifestsUnitSuite) TestCollectMetadata() { ps := make([]path.Path, 0, len(files)) for _, f := range files { - p, err := emailPath.Append(f, true) + p, err := emailPath.AppendItem(f) assert.NoError(t, err, clues.ToCore(err)) ps = append(ps, p) } @@ -191,10 +191,10 @@ func (suite *OperationsManifestsUnitSuite) TestCollectMetadata() { ps := make([]path.Path, 0, len(files)) for _, f := range files { - p, err := emailPath.Append(f, true) + p, err := emailPath.AppendItem(f) assert.NoError(t, err, clues.ToCore(err)) ps = append(ps, p) - p, err = contactPath.Append(f, true) + p, err = contactPath.AppendItem(f) assert.NoError(t, err, clues.ToCore(err)) ps = append(ps, p) } @@ -222,10 +222,10 @@ func (suite *OperationsManifestsUnitSuite) TestCollectMetadata() { ps := make([]path.Path, 0, len(files)) for _, f := range files { - p, err := emailPath.Append(f, true) + p, err := emailPath.AppendItem(f) assert.NoError(t, err, clues.ToCore(err)) ps = append(ps, p) - p, err = contactPath.Append(f, true) + p, err = contactPath.AppendItem(f) assert.NoError(t, err, clues.ToCore(err)) ps = append(ps, p) } diff --git a/src/pkg/backup/details/testdata/testdata.go b/src/pkg/backup/details/testdata/testdata.go index a406d838a..0d98ec7df 100644 --- a/src/pkg/backup/details/testdata/testdata.go +++ b/src/pkg/backup/details/testdata/testdata.go @@ -25,7 +25,7 @@ func mustParsePath(ref string, isItem bool) path.Path { // path with the element appended to it. Panics if the path cannot be parsed. // Useful for simple variable assignments. func mustAppendPath(p path.Path, newElement string, isItem bool) path.Path { - newP, err := p.Append(newElement, isItem) + newP, err := p.Append(isItem, newElement) if err != nil { panic(err) } diff --git a/src/pkg/path/elements.go b/src/pkg/path/elements.go index 0a55bd8e4..a77ea3345 100644 --- a/src/pkg/path/elements.go +++ b/src/pkg/path/elements.go @@ -86,3 +86,12 @@ func (el Elements) String() string { func (el Elements) PlainString() string { return join(el) } + +// Last returns the last element. Returns "" if empty. +func (el Elements) Last() string { + if len(el) == 0 { + return "" + } + + return el[len(el)-1] +} diff --git a/src/pkg/path/path.go b/src/pkg/path/path.go index 33fae1763..189e24449 100644 --- a/src/pkg/path/path.go +++ b/src/pkg/path/path.go @@ -106,7 +106,9 @@ type Path interface { // Append returns a new Path object with the given element added to the end of // the old Path if possible. If the old Path is an item Path then Append // returns an error. - Append(element string, isItem bool) (Path, error) + Append(isItem bool, elems ...string) (Path, error) + // AppendItem is a shorthand for Append(true, someItem) + AppendItem(item string) (Path, error) // ShortRef returns a short reference representing this path. The short // reference is guaranteed to be unique. No guarantees are made about whether // a short reference can be converted back into the Path that generated it. diff --git a/src/pkg/path/path_test.go b/src/pkg/path/path_test.go index 21631f7bf..be43d3732 100644 --- a/src/pkg/path/path_test.go +++ b/src/pkg/path/path_test.go @@ -245,6 +245,26 @@ func (suite *PathUnitSuite) TestAppend() { } } +func (suite *PathUnitSuite) TestAppendItem() { + t := suite.T() + + p, err := Build("t", "ro", ExchangeService, EmailCategory, false, "foo", "bar") + require.NoError(t, err, clues.ToCore(err)) + + pb := p.ToBuilder() + assert.Equal(t, pb.String(), p.String()) + + pb = pb.Append("qux") + + p, err = p.AppendItem("qux") + + require.NoError(t, err, clues.ToCore(err)) + assert.Equal(t, pb.String(), p.String()) + + _, err = p.AppendItem("fnords") + require.Error(t, err, clues.ToCore(err)) +} + func (suite *PathUnitSuite) TestUnescapeAndAppend() { table := append(append([]testData{}, genericCases...), basicEscapedInputs...) for _, test := range table { diff --git a/src/pkg/path/resource_path.go b/src/pkg/path/resource_path.go index 47d481a46..923d66453 100644 --- a/src/pkg/path/resource_path.go +++ b/src/pkg/path/resource_path.go @@ -253,21 +253,25 @@ func (rp dataLayerResourcePath) Dir() (Path, error) { } func (rp dataLayerResourcePath) Append( - element string, isItem bool, + elems ...string, ) (Path, error) { if rp.hasItem { return nil, clues.New("appending to an item path") } return &dataLayerResourcePath{ - Builder: *rp.Builder.Append(element), + Builder: *rp.Builder.Append(elems...), service: rp.service, category: rp.category, hasItem: isItem, }, nil } +func (rp dataLayerResourcePath) AppendItem(item string) (Path, error) { + return rp.Append(true, item) +} + func (rp dataLayerResourcePath) ToBuilder() *Builder { // Safe to directly return the Builder because Builders are immutable. return &rp.Builder diff --git a/src/pkg/path/resource_path_test.go b/src/pkg/path/resource_path_test.go index 3453737e6..e49f797e2 100644 --- a/src/pkg/path/resource_path_test.go +++ b/src/pkg/path/resource_path_test.go @@ -547,7 +547,7 @@ func (suite *PopulatedDataLayerResourcePath) TestAppend() { suite.Run(test.name, func() { t := suite.T() - newPath, err := suite.paths[m.isItem].Append(newElement, test.hasItem) + newPath, err := suite.paths[m.isItem].Append(test.hasItem, newElement) // Items don't allow appending. if m.isItem {