From 8a81624b98ddc5216d553499ce83acd57cb6e984 Mon Sep 17 00:00:00 2001 From: ashmrtn Date: Tue, 14 Mar 2023 15:32:06 -0700 Subject: [PATCH] Change to storing files by ID instead of display name (#2496) ## Description Change to storing OneDrive files by ID instead of their OneDrive display name. This allows delta token-based incrementals to use an exclude list to remove previously backed up items from a backup during hierarchy merging Also updates the following: * selectors to match on file display name instead of kopia file name * ShortRefs for OneDrive files to update when the file display name is updated ## 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 - [x] :sunflower: Feature - [ ] :bug: Bugfix - [ ] :world_map: Documentation - [ ] :robot: Test - [ ] :computer: CI/Deployment - [ ] :broom: Tech Debt/Cleanup ## Issue(s) * closes #1535 ## Test Plan - [x] :muscle: Manual - [x] :zap: Unit test - [x] :green_heart: E2E --- src/cli/utils/testdata/opts.go | 62 ++++++ .../connector/exchange/service_restore.go | 6 +- .../connector/graph_connector_helper_test.go | 5 + .../graph_connector_onedrive_test.go | 27 ++- src/internal/connector/onedrive/collection.go | 8 +- .../connector/onedrive/collection_test.go | 6 +- src/internal/connector/onedrive/restore.go | 16 +- src/internal/connector/sharepoint/restore.go | 13 +- src/internal/kopia/upload.go | 15 +- src/internal/operations/backup.go | 5 +- src/internal/streamstore/details_test.go | 14 +- src/internal/version/backup.go | 10 +- src/pkg/backup/details/details.go | 48 ++++- src/pkg/backup/details/details_test.go | 177 ++++++++++++++++++ src/pkg/selectors/exchange.go | 11 +- src/pkg/selectors/exchange_test.go | 10 +- src/pkg/selectors/helpers_test.go | 7 +- src/pkg/selectors/onedrive.go | 15 +- src/pkg/selectors/onedrive_test.go | 22 ++- src/pkg/selectors/scopes.go | 12 +- src/pkg/selectors/scopes_test.go | 4 +- src/pkg/selectors/sharepoint.go | 26 ++- src/pkg/selectors/sharepoint_test.go | 27 ++- 23 files changed, 478 insertions(+), 68 deletions(-) diff --git a/src/cli/utils/testdata/opts.go b/src/cli/utils/testdata/opts.go index 1af633e40..2f0891432 100644 --- a/src/cli/utils/testdata/opts.go +++ b/src/cli/utils/testdata/opts.go @@ -387,6 +387,37 @@ var ( }, }, }, + { + Name: "SingleItem", + Expected: []details.DetailsEntry{testdata.OneDriveItems[0]}, + Opts: utils.OneDriveOpts{ + FileNames: []string{ + testdata.OneDriveItems[0].OneDrive.ItemName, + }, + }, + }, + { + Name: "MultipleItems", + Expected: []details.DetailsEntry{ + testdata.OneDriveItems[0], + testdata.OneDriveItems[1], + }, + Opts: utils.OneDriveOpts{ + FileNames: []string{ + testdata.OneDriveItems[0].OneDrive.ItemName, + testdata.OneDriveItems[1].OneDrive.ItemName, + }, + }, + }, + { + Name: "NoSelectRepoItemName", + Expected: []details.DetailsEntry{}, + Opts: utils.OneDriveOpts{ + FileNames: []string{ + testdata.OneDriveItemPath1.Item(), + }, + }, + }, { Name: "CreatedBefore", Expected: []details.DetailsEntry{testdata.OneDriveItems[1]}, @@ -475,6 +506,37 @@ var ( }, }, }, + { + Name: "SingleItem", + Expected: []details.DetailsEntry{testdata.SharePointLibraryItems[0]}, + Opts: utils.SharePointOpts{ + FileNames: []string{ + testdata.SharePointLibraryItems[0].SharePoint.ItemName, + }, + }, + }, + { + Name: "MultipleItems", + Expected: []details.DetailsEntry{ + testdata.SharePointLibraryItems[0], + testdata.SharePointLibraryItems[1], + }, + Opts: utils.SharePointOpts{ + FileNames: []string{ + testdata.SharePointLibraryItems[0].SharePoint.ItemName, + testdata.SharePointLibraryItems[1].SharePoint.ItemName, + }, + }, + }, + { + Name: "NoSelectRepoItemName", + Expected: []details.DetailsEntry{}, + Opts: utils.SharePointOpts{ + FileNames: []string{ + testdata.SharePointLibraryItemPath1.Item(), + }, + }, + }, // { // Name: "CreatedBefore", // Expected: []details.DetailsEntry{testdata.SharePointLibraryItems[1]}, diff --git a/src/internal/connector/exchange/service_restore.go b/src/internal/connector/exchange/service_restore.go index c832c012b..8f49a01b6 100644 --- a/src/internal/connector/exchange/service_restore.go +++ b/src/internal/connector/exchange/service_restore.go @@ -449,7 +449,7 @@ func restoreCollection( locationRef = itemPath.Folder(false) } - deets.Add( + err = deets.Add( itemPath.String(), itemPath.ShortRef(), "", @@ -458,6 +458,10 @@ func restoreCollection( details.ItemInfo{ Exchange: info, }) + if err != nil { + // Not critical enough to need to stop restore operation. + logger.Ctx(ctx).Infow("accounting for restored item", "error", err) + } colProgress <- struct{}{} } diff --git a/src/internal/connector/graph_connector_helper_test.go b/src/internal/connector/graph_connector_helper_test.go index c1b0e62b4..2392319a9 100644 --- a/src/internal/connector/graph_connector_helper_test.go +++ b/src/internal/connector/graph_connector_helper_test.go @@ -754,6 +754,11 @@ func compareOneDriveItem( } expectedData := expected[name] + + if strings.HasSuffix(name, onedrive.MetaFileSuffix) { + expectedData = expected[itemMeta.FileName] + } + if !assert.NotNil( t, expectedData, diff --git a/src/internal/connector/graph_connector_onedrive_test.go b/src/internal/connector/graph_connector_onedrive_test.go index feba7759d..c2e961e92 100644 --- a/src/internal/connector/graph_connector_onedrive_test.go +++ b/src/internal/connector/graph_connector_onedrive_test.go @@ -80,7 +80,7 @@ func onedriveItemWithData( func onedriveMetadata( t *testing.T, - fileName, itemID string, + fileName, itemID, lookupKey string, perm permData, permUseID bool, ) itemInfo { @@ -94,7 +94,7 @@ func onedriveMetadata( return itemInfo{ name: itemID, data: testMetaJSON, - lookupKey: itemID, + lookupKey: lookupKey, } } @@ -205,6 +205,24 @@ func (c *onedriveCollection) withFile(name string, fileData []byte, perm permDat c.t, "", name+onedrive.MetaFileSuffix, + name+onedrive.MetaFileSuffix, + perm, + c.backupVersion >= versionPermissionSwitchedToID) + c.items = append(c.items, metadata) + c.aux = append(c.aux, metadata) + + case version.OneDrive6NameInMeta: + c.items = append(c.items, onedriveItemWithData( + c.t, + name+onedrive.DataFileSuffix, + name+onedrive.DataFileSuffix, + fileData)) + + metadata := onedriveMetadata( + c.t, + name, + name+onedrive.MetaFileSuffix, + name, perm, c.backupVersion >= versionPermissionSwitchedToID) c.items = append(c.items, metadata) @@ -219,7 +237,8 @@ func (c *onedriveCollection) withFile(name string, fileData []byte, perm permDat func (c *onedriveCollection) withFolder(name string, perm permData) *onedriveCollection { switch c.backupVersion { - case 0, version.OneDrive4DirIncludesPermissions, version.OneDrive5DirMetaNoName: + case 0, version.OneDrive4DirIncludesPermissions, version.OneDrive5DirMetaNoName, + version.OneDrive6NameInMeta: return c case version.OneDrive1DataAndMetaFiles, 2, version.OneDrive3IsMetaMarker: @@ -229,6 +248,7 @@ func (c *onedriveCollection) withFolder(name string, perm permData) *onedriveCol c.t, "", name+onedrive.DirMetaFileSuffix, + name+onedrive.DirMetaFileSuffix, perm, c.backupVersion >= versionPermissionSwitchedToID)) @@ -264,6 +284,7 @@ func (c *onedriveCollection) withPermissions(perm permData) *onedriveCollection c.t, name, metaName+onedrive.DirMetaFileSuffix, + metaName+onedrive.DirMetaFileSuffix, perm, c.backupVersion >= versionPermissionSwitchedToID) diff --git a/src/internal/connector/onedrive/collection.go b/src/internal/connector/onedrive/collection.go index e9886809c..9b262ddd2 100644 --- a/src/internal/connector/onedrive/collection.go +++ b/src/internal/connector/onedrive/collection.go @@ -357,7 +357,7 @@ func (oc *Collection) populateItems(ctx context.Context, errs *fault.Bus) { if isFile { atomic.AddInt64(&itemsFound, 1) - metaFileName = itemName + metaFileName = itemID metaSuffix = MetaFileSuffix } else { atomic.AddInt64(&dirsFound, 1) @@ -443,7 +443,7 @@ func (oc *Collection) populateItems(ctx context.Context, errs *fault.Bus) { ctx, itemData, observe.ItemBackupMsg, - observe.PII(itemName+dataSuffix), + observe.PII(itemID+dataSuffix), itemSize) go closer() @@ -451,7 +451,7 @@ func (oc *Collection) populateItems(ctx context.Context, errs *fault.Bus) { }) oc.data <- &Item{ - id: itemName + dataSuffix, + id: itemID + dataSuffix, data: itemReader, info: itemInfo, } @@ -461,7 +461,7 @@ func (oc *Collection) populateItems(ctx context.Context, errs *fault.Bus) { metaReader := lazy.NewLazyReadCloser(func() (io.ReadCloser, error) { progReader, closer := observe.ItemProgress( ctx, itemMeta, observe.ItemBackupMsg, - observe.PII(itemName+metaSuffix), int64(itemMetaSize)) + observe.PII(metaFileName+metaSuffix), int64(itemMetaSize)) go closer() return progReader, nil }) diff --git a/src/internal/connector/onedrive/collection_test.go b/src/internal/connector/onedrive/collection_test.go index 1fdcb73a7..3be7f7951 100644 --- a/src/internal/connector/onedrive/collection_test.go +++ b/src/internal/connector/onedrive/collection_test.go @@ -254,9 +254,9 @@ func (suite *CollectionUnitTestSuite) TestCollection() { readItemInfo := readItem.(data.StreamInfo) if test.source == OneDriveSource { - assert.Equal(t, testItemName+DataFileSuffix, readItem.UUID()) + assert.Equal(t, testItemID+DataFileSuffix, readItem.UUID()) } else { - assert.Equal(t, testItemName, readItem.UUID()) + assert.Equal(t, testItemID, readItem.UUID()) } require.Implements(t, (*data.StreamModTime)(nil), readItem) @@ -283,7 +283,7 @@ func (suite *CollectionUnitTestSuite) TestCollection() { if test.source == OneDriveSource { readItemMeta := readItems[1] - assert.Equal(t, testItemName+MetaFileSuffix, readItemMeta.UUID()) + assert.Equal(t, testItemID+MetaFileSuffix, readItemMeta.UUID()) readMetaData, err := io.ReadAll(readItemMeta.ToReader()) require.NoError(t, err) diff --git a/src/internal/connector/onedrive/restore.go b/src/internal/connector/onedrive/restore.go index f4cf25193..1a483a0d2 100644 --- a/src/internal/connector/onedrive/restore.go +++ b/src/internal/connector/onedrive/restore.go @@ -229,7 +229,7 @@ func RestoreCollection( err error ) - if backupVersion < version.OneDriveXNameInMeta { + if backupVersion < version.OneDrive6NameInMeta { itemInfo, err = restoreV1File( ctx, source, @@ -262,13 +262,18 @@ func RestoreCollection( continue } - deets.Add( + err = deets.Add( itemPath.String(), itemPath.ShortRef(), "", "", // TODO: implement locationRef true, itemInfo) + if err != nil { + // Not critical enough to need to stop restore operation. + logger.Ctx(ctx).Infow("accounting for restored item", "error", err) + } + metrics.Successes++ } else if strings.HasSuffix(name, MetaFileSuffix) { // Just skip this for the moment since we moved the code to the above @@ -315,13 +320,18 @@ func RestoreCollection( continue } - deets.Add( + err = deets.Add( itemPath.String(), itemPath.ShortRef(), "", "", // TODO: implement locationRef true, itemInfo) + if err != nil { + // Not critical enough to need to stop restore operation. + logger.Ctx(ctx).Infow("accounting for restored item", "error", err) + } + metrics.Successes++ } } diff --git a/src/internal/connector/sharepoint/restore.go b/src/internal/connector/sharepoint/restore.go index b349dd9c7..44bb31302 100644 --- a/src/internal/connector/sharepoint/restore.go +++ b/src/internal/connector/sharepoint/restore.go @@ -21,6 +21,7 @@ import ( "github.com/alcionai/corso/src/pkg/backup/details" "github.com/alcionai/corso/src/pkg/control" "github.com/alcionai/corso/src/pkg/fault" + "github.com/alcionai/corso/src/pkg/logger" "github.com/alcionai/corso/src/pkg/path" ) @@ -261,13 +262,17 @@ func RestoreListCollection( continue } - deets.Add( + err = deets.Add( itemPath.String(), itemPath.ShortRef(), "", "", // TODO: implement locationRef true, itemInfo) + if err != nil { + // Not critical enough to need to stop restore operation. + logger.Ctx(ctx).Infow("accounting for restored item", "error", err) + } metrics.Successes++ } @@ -344,13 +349,17 @@ func RestorePageCollection( continue } - deets.Add( + err = deets.Add( itemPath.String(), itemPath.ShortRef(), "", "", // TODO: implement locationRef true, itemInfo) + if err != nil { + // Not critical enough to need to stop restore operation. + logger.Ctx(ctx).Infow("accounting for restored item", "error", err) + } metrics.Successes++ } diff --git a/src/internal/kopia/upload.go b/src/internal/kopia/upload.go index 08064ad08..3b0fa41c6 100644 --- a/src/internal/kopia/upload.go +++ b/src/internal/kopia/upload.go @@ -187,7 +187,8 @@ func (cp *corsoProgress) FinishedFile(relativePath string, err error) { With( "service", d.repoPath.Service().String(), "category", d.repoPath.Category().String(), - )) + ). + Label(fault.LabelForceNoBackupCreation)) return } @@ -221,13 +222,23 @@ func (cp *corsoProgress) FinishedFile(relativePath string, err error) { } } - cp.deets.Add( + err = cp.deets.Add( d.repoPath.String(), d.repoPath.ShortRef(), parent.ShortRef(), locationFolders, !d.cached, *d.info) + if err != nil { + cp.errs.AddRecoverable(clues.New("adding item to details"). + With( + "service", d.repoPath.Service().String(), + "category", d.repoPath.Category().String(), + ). + Label(fault.LabelForceNoBackupCreation)) + + return + } folders := details.FolderEntriesForPath(parent, locPB) cp.deets.AddFoldersForItem( diff --git a/src/internal/operations/backup.go b/src/internal/operations/backup.go index c6355f0aa..fc435f205 100644 --- a/src/internal/operations/backup.go +++ b/src/internal/operations/backup.go @@ -587,13 +587,16 @@ func mergeDetails( itemUpdated = itemUpdated || newLocStr != entry.LocationRef } - deets.Add( + err = deets.Add( newPath.String(), newPath.ShortRef(), newPath.ToBuilder().Dir().ShortRef(), newLocStr, itemUpdated, item) + if err != nil { + return clues.Wrap(err, "adding item to details") + } folders := details.FolderEntriesForPath(newPath.ToBuilder().Dir(), locBuilder) deets.AddFoldersForItem(folders, item, itemUpdated) diff --git a/src/internal/streamstore/details_test.go b/src/internal/streamstore/details_test.go index 118626d77..e14c371d4 100644 --- a/src/internal/streamstore/details_test.go +++ b/src/internal/streamstore/details_test.go @@ -47,12 +47,14 @@ func (suite *StreamDetailsIntegrationSuite) TestDetails() { deetsBuilder := &details.Builder{} - deetsBuilder.Add("ref", "shortref", "parentref", "locationRef", true, - details.ItemInfo{ - Exchange: &details.ExchangeInfo{ - Subject: "hello world", - }, - }) + require.NoError( + t, + deetsBuilder.Add("ref", "shortref", "parentref", "locationRef", true, + details.ItemInfo{ + Exchange: &details.ExchangeInfo{ + Subject: "hello world", + }, + })) var ( deets = deetsBuilder.Details() diff --git a/src/internal/version/backup.go b/src/internal/version/backup.go index e99e7627b..0ea734aca 100644 --- a/src/internal/version/backup.go +++ b/src/internal/version/backup.go @@ -1,8 +1,6 @@ package version -import "math" - -const Backup = 5 +const Backup = 6 // Various labels to refer to important version changes. // Labels don't need 1:1 service:version representation. Add a new @@ -30,10 +28,8 @@ const ( // during incremental backups. OneDrive5DirMetaNoName = 5 - // OneDriveXNameInMeta points to the backup format version where we begin + // OneDrive6NameInMeta points to the backup format version where we begin // storing files in kopia with their item ID instead of their OneDrive file // name. - // TODO(ashmrtn): Update this to a real value when we merge the file name - // change. Set to MAXINT for now to keep the if-check using it working. - OneDriveXNameInMeta = math.MaxInt + OneDrive6NameInMeta = 6 ) diff --git a/src/pkg/backup/details/details.go b/src/pkg/backup/details/details.go index 8ec5c1328..fff392f7f 100644 --- a/src/pkg/backup/details/details.go +++ b/src/pkg/backup/details/details.go @@ -152,10 +152,11 @@ func (b *Builder) Add( repoRef, shortRef, parentRef, locationRef string, updated bool, info ItemInfo, -) { +) error { b.mu.Lock() defer b.mu.Unlock() - b.d.add(repoRef, shortRef, parentRef, locationRef, updated, info) + + return b.d.add(repoRef, shortRef, parentRef, locationRef, updated, info) } func (b *Builder) Details() *Details { @@ -283,15 +284,52 @@ func (d *Details) add( repoRef, shortRef, parentRef, locationRef string, updated bool, info ItemInfo, -) { - d.Entries = append(d.Entries, DetailsEntry{ +) error { + entry := DetailsEntry{ RepoRef: repoRef, ShortRef: shortRef, ParentRef: parentRef, LocationRef: locationRef, Updated: updated, ItemInfo: info, - }) + } + + // Use the item name and the path for the ShortRef. This ensures that renames + // within a directory generate unique ShortRefs. + if info.infoType() == OneDriveItem || info.infoType() == SharePointLibrary { + p, err := path.FromDataLayerPath(repoRef, true) + if err != nil { + return clues.Wrap(err, "munging OneDrive or SharePoint ShortRef") + } + + if info.OneDrive == nil && info.SharePoint == nil { + return clues.New("item is not SharePoint or OneDrive type") + } + + filename := "" + if info.OneDrive != nil { + filename = info.OneDrive.ItemName + } else if info.SharePoint != nil { + filename = info.SharePoint.ItemName + } + + // Make the new path contain all display names and then the M365 item ID. + // This ensures the path will be unique, thus ensuring the ShortRef will be + // unique. + // + // If we appended the file's display name to the path then it's possible + // for a folder in the parent directory to have the same display name as the + // M365 ID of this file and also have a subfolder in the folder with a + // display name that matches the file's display name. That would result in + // duplicate ShortRefs, which we can't allow. + elements := p.Elements() + elements = append(elements[:len(elements)-1], filename, p.Item()) + entry.ShortRef = path.Builder{}.Append(elements...).ShortRef() + } + + d.Entries = append(d.Entries, entry) + + return nil } // addFolder adds an entry for the given folder. diff --git a/src/pkg/backup/details/details_test.go b/src/pkg/backup/details/details_test.go index 323d2770b..651341e6f 100644 --- a/src/pkg/backup/details/details_test.go +++ b/src/pkg/backup/details/details_test.go @@ -365,6 +365,183 @@ func (suite *DetailsUnitSuite) TestDetailsModel_FilterMetaFiles() { assert.Len(t, d.Entries, 3) } +func (suite *DetailsUnitSuite) TestDetails_Add_ShortRefs() { + itemNames := []string{ + "item1", + "item2", + } + + table := []struct { + name string + service path.ServiceType + category path.CategoryType + itemInfoFunc func(name string) ItemInfo + expectedUniqueRefs int + }{ + { + name: "OneDrive", + service: path.OneDriveService, + category: path.FilesCategory, + itemInfoFunc: func(name string) ItemInfo { + return ItemInfo{ + OneDrive: &OneDriveInfo{ + ItemType: OneDriveItem, + ItemName: name, + }, + } + }, + expectedUniqueRefs: len(itemNames), + }, + { + name: "SharePoint", + service: path.SharePointService, + category: path.LibrariesCategory, + itemInfoFunc: func(name string) ItemInfo { + return ItemInfo{ + SharePoint: &SharePointInfo{ + ItemType: SharePointLibrary, + ItemName: name, + }, + } + }, + expectedUniqueRefs: len(itemNames), + }, + { + name: "SharePoint List", + service: path.SharePointService, + category: path.ListsCategory, + itemInfoFunc: func(name string) ItemInfo { + return ItemInfo{ + SharePoint: &SharePointInfo{ + ItemType: SharePointList, + ItemName: name, + }, + } + }, + // Should all end up as the starting shortref. + expectedUniqueRefs: 1, + }, + { + name: "Exchange no change", + service: path.ExchangeService, + category: path.EmailCategory, + itemInfoFunc: func(name string) ItemInfo { + return ItemInfo{ + Exchange: &ExchangeInfo{ + ItemType: ExchangeMail, + Sender: "a-person@foo.com", + Subject: name, + Recipient: []string{"another-person@bar.com"}, + }, + } + }, + // Should all end up as the starting shortref. + expectedUniqueRefs: 1, + }, + } + + for _, test := range table { + suite.Run(test.name, func() { + t := suite.T() + + b := Builder{} + + for _, name := range itemNames { + item := test.itemInfoFunc(name) + itemPath := makeItemPath( + suite.T(), + test.service, + test.category, + "a-tenant", + "a-user", + []string{ + "drive-id", + "root:", + "folder", + name + "-id", + }, + ) + + require.NoError(t, b.Add( + itemPath.String(), + "deadbeef", + itemPath.ToBuilder().Dir().String(), + itemPath.String(), + false, + item, + )) + } + + deets := b.Details() + shortRefs := map[string]struct{}{} + + for _, d := range deets.Items() { + shortRefs[d.ShortRef] = struct{}{} + } + + assert.Len(t, shortRefs, test.expectedUniqueRefs, "items don't have unique ShortRefs") + }) + } +} + +func (suite *DetailsUnitSuite) TestDetails_Add_ShortRefs_Unique_From_Folder() { + t := suite.T() + + b := Builder{} + name := "itemName" + info := ItemInfo{ + OneDrive: &OneDriveInfo{ + ItemType: OneDriveItem, + ItemName: name, + }, + } + + itemPath := makeItemPath( + t, + path.OneDriveService, + path.FilesCategory, + "a-tenant", + "a-user", + []string{ + "drive-id", + "root:", + "folder", + name + "-id", + }, + ) + + otherItemPath := makeItemPath( + t, + path.OneDriveService, + path.FilesCategory, + "a-tenant", + "a-user", + []string{ + "drive-id", + "root:", + "folder", + name + "-id", + name, + }, + ) + + err := b.Add( + itemPath.String(), + "deadbeef", + itemPath.ToBuilder().Dir().String(), + itemPath.String(), + false, + info) + require.NoError(t, err) + + items := b.Details().Items() + require.Len(t, items, 1) + + // If the ShortRefs match then it means it's possible for the user to + // construct folder names such that they'll generate a ShortRef collision. + assert.NotEqual(t, otherItemPath.ShortRef(), items[0].ShortRef, "same ShortRef as subfolder item") +} + func (suite *DetailsUnitSuite) TestDetails_AddFolders() { itemTime := time.Date(2022, 10, 21, 10, 0, 0, 0, time.UTC) folderTimeOlderThanItem := time.Date(2022, 9, 21, 10, 0, 0, 0, time.UTC) diff --git a/src/pkg/selectors/exchange.go b/src/pkg/selectors/exchange.go index cc69fac58..3eda3acac 100644 --- a/src/pkg/selectors/exchange.go +++ b/src/pkg/selectors/exchange.go @@ -4,6 +4,8 @@ import ( "context" "strconv" + "github.com/alcionai/clues" + "github.com/alcionai/corso/src/internal/common" "github.com/alcionai/corso/src/pkg/backup/details" "github.com/alcionai/corso/src/pkg/fault" @@ -580,7 +582,10 @@ func (ec exchangeCategory) isLeaf() bool { // Example: // [tenantID, service, userPN, category, mailFolder, mailID] // => {exchMailFolder: mailFolder, exchMail: mailID} -func (ec exchangeCategory) pathValues(repo path.Path, ent details.DetailsEntry) map[categorizer][]string { +func (ec exchangeCategory) pathValues( + repo path.Path, + ent details.DetailsEntry, +) (map[categorizer][]string, error) { var folderCat, itemCat categorizer switch ec { @@ -594,7 +599,7 @@ func (ec exchangeCategory) pathValues(repo path.Path, ent details.DetailsEntry) folderCat, itemCat = ExchangeMailFolder, ExchangeMail default: - return map[categorizer][]string{} + return nil, clues.New("bad exchanageCategory").With("category", ec) } result := map[categorizer][]string{ @@ -606,7 +611,7 @@ func (ec exchangeCategory) pathValues(repo path.Path, ent details.DetailsEntry) result[folderCat] = append(result[folderCat], ent.LocationRef) } - return result + return result, nil } // pathKeys returns the path keys recognized by the receiver's leaf type. diff --git a/src/pkg/selectors/exchange_test.go b/src/pkg/selectors/exchange_test.go index 817b3ee25..09b9ebeaa 100644 --- a/src/pkg/selectors/exchange_test.go +++ b/src/pkg/selectors/exchange_test.go @@ -764,7 +764,9 @@ func (suite *ExchangeSelectorSuite) TestExchangeScope_MatchesPath() { scopes := setScopesToDefault(test.scope) var aMatch bool for _, scope := range scopes { - pvs := ExchangeMail.pathValues(repo, ent) + pvs, err := ExchangeMail.pathValues(repo, ent) + require.NoError(t, err) + if matchesPathValues(scope, ExchangeMail, pvs) { aMatch = true break @@ -1345,7 +1347,8 @@ func (suite *ExchangeSelectorSuite) TestPasses() { suite.Run(test.name, func() { t := suite.T() - pvs := cat.pathValues(repo, ent) + pvs, err := cat.pathValues(repo, ent) + require.NoError(t, err) result := passes( cat, @@ -1486,7 +1489,8 @@ func (suite *ExchangeSelectorSuite) TestExchangeCategory_PathValues() { ShortRef: "short", } - pvs := test.cat.pathValues(test.path, ent) + pvs, err := test.cat.pathValues(test.path, ent) + require.NoError(t, err) assert.Equal(t, test.expect, pvs) }) } diff --git a/src/pkg/selectors/helpers_test.go b/src/pkg/selectors/helpers_test.go index a62ff1499..1921b8b4f 100644 --- a/src/pkg/selectors/helpers_test.go +++ b/src/pkg/selectors/helpers_test.go @@ -55,11 +55,14 @@ func (mc mockCategorizer) isLeaf() bool { return mc == leafCatStub } -func (mc mockCategorizer) pathValues(repo path.Path, ent details.DetailsEntry) map[categorizer][]string { +func (mc mockCategorizer) pathValues( + repo path.Path, + ent details.DetailsEntry, +) (map[categorizer][]string, error) { return map[categorizer][]string{ rootCatStub: {"root"}, leafCatStub: {"leaf"}, - } + }, nil } func (mc mockCategorizer) pathKeys() []categorizer { diff --git a/src/pkg/selectors/onedrive.go b/src/pkg/selectors/onedrive.go index dde3b9784..e7d4c590a 100644 --- a/src/pkg/selectors/onedrive.go +++ b/src/pkg/selectors/onedrive.go @@ -3,6 +3,8 @@ package selectors import ( "context" + "github.com/alcionai/clues" + "github.com/alcionai/corso/src/internal/common" "github.com/alcionai/corso/src/pkg/backup/details" "github.com/alcionai/corso/src/pkg/fault" @@ -376,20 +378,27 @@ func (c oneDriveCategory) isLeaf() bool { // Example: // [tenantID, service, userPN, category, folder, fileID] // => {odFolder: folder, odFileID: fileID} -func (c oneDriveCategory) pathValues(repo path.Path, ent details.DetailsEntry) map[categorizer][]string { +func (c oneDriveCategory) pathValues( + repo path.Path, + ent details.DetailsEntry, +) (map[categorizer][]string, error) { + if ent.OneDrive == nil { + return nil, clues.New("no OneDrive ItemInfo in details") + } + // Ignore `drives//root:` for folder comparison rFld := path.Builder{}.Append(repo.Folders()...).PopFront().PopFront().PopFront().String() result := map[categorizer][]string{ OneDriveFolder: {rFld}, - OneDriveItem: {repo.Item(), ent.ShortRef}, + OneDriveItem: {ent.OneDrive.ItemName, ent.ShortRef}, } if len(ent.LocationRef) > 0 { result[OneDriveFolder] = append(result[OneDriveFolder], ent.LocationRef) } - return result + return result, nil } // pathKeys returns the path keys recognized by the receiver's leaf type. diff --git a/src/pkg/selectors/onedrive_test.go b/src/pkg/selectors/onedrive_test.go index 55bdbff23..69a583537 100644 --- a/src/pkg/selectors/onedrive_test.go +++ b/src/pkg/selectors/onedrive_test.go @@ -175,6 +175,7 @@ func (suite *OneDriveSelectorSuite) TestOneDriveRestore_Reduce() { ItemInfo: details.ItemInfo{ OneDrive: &details.OneDriveInfo{ ItemType: details.OneDriveItem, + ItemName: "fileName", }, }, }, @@ -183,6 +184,7 @@ func (suite *OneDriveSelectorSuite) TestOneDriveRestore_Reduce() { ItemInfo: details.ItemInfo{ OneDrive: &details.OneDriveInfo{ ItemType: details.OneDriveItem, + ItemName: "fileName2", }, }, }, @@ -191,6 +193,7 @@ func (suite *OneDriveSelectorSuite) TestOneDriveRestore_Reduce() { ItemInfo: details.ItemInfo{ OneDrive: &details.OneDriveInfo{ ItemType: details.OneDriveItem, + ItemName: "fileName3", }, }, }, @@ -223,7 +226,7 @@ func (suite *OneDriveSelectorSuite) TestOneDriveRestore_Reduce() { deets, func() *OneDriveRestore { odr := NewOneDriveRestore(Any()) - odr.Include(odr.Items(Any(), []string{"file2"})) + odr.Include(odr.Items(Any(), []string{"fileName2"})) return odr }, arr(file2), @@ -257,21 +260,30 @@ func (suite *OneDriveSelectorSuite) TestOneDriveRestore_Reduce() { func (suite *OneDriveSelectorSuite) TestOneDriveCategory_PathValues() { t := suite.T() - elems := []string{"drive", "driveID", "root:", "dir1", "dir2", "file"} + fileName := "file" + shortRef := "short" + elems := []string{"drive", "driveID", "root:", "dir1", "dir2", fileName + "-id"} + filePath, err := path.Build("tenant", "user", path.OneDriveService, path.FilesCategory, true, elems...) require.NoError(t, err) expected := map[categorizer][]string{ OneDriveFolder: {"dir1/dir2"}, - OneDriveItem: {"file", "short"}, + OneDriveItem: {fileName, shortRef}, } ent := details.DetailsEntry{ RepoRef: filePath.String(), - ShortRef: "short", + ShortRef: shortRef, + ItemInfo: details.ItemInfo{ + OneDrive: &details.OneDriveInfo{ + ItemName: fileName, + }, + }, } - r := OneDriveItem.pathValues(filePath, ent) + r, err := OneDriveItem.pathValues(filePath, ent) + require.NoError(t, err) assert.Equal(t, expected, r) } diff --git a/src/pkg/selectors/scopes.go b/src/pkg/selectors/scopes.go index 32076f049..44166c4ce 100644 --- a/src/pkg/selectors/scopes.go +++ b/src/pkg/selectors/scopes.go @@ -88,7 +88,7 @@ type ( // folderCat: folder, // itemCat: itemID, // } - pathValues(path.Path, details.DetailsEntry) map[categorizer][]string + pathValues(path.Path, details.DetailsEntry) (map[categorizer][]string, error) // pathKeys produces a list of categorizers that can be used as keys in the pathValues // map. The combination of the two funcs generically interprets the context of the @@ -328,9 +328,11 @@ func reduce[T scopeT, C categoryT]( // for each entry, compare that entry against the scopes of the same data type for _, ent := range deets.Items() { + ictx := clues.Add(ctx, "short_ref", ent.ShortRef) + repoPath, err := path.FromDataLayerPath(ent.RepoRef, true) if err != nil { - el.AddRecoverable(clues.Wrap(err, "transforming repoRef to path").WithClues(ctx)) + el.AddRecoverable(clues.Wrap(err, "transforming repoRef to path").WithClues(ictx)) continue } @@ -351,7 +353,11 @@ func reduce[T scopeT, C categoryT]( continue } - pv := dc.pathValues(repoPath, *ent) + pv, err := dc.pathValues(repoPath, *ent) + if err != nil { + el.AddRecoverable(clues.Wrap(err, "getting path values").WithClues(ictx)) + continue + } passed := passes(dc, pv, *ent, e, f, i) if passed { diff --git a/src/pkg/selectors/scopes_test.go b/src/pkg/selectors/scopes_test.go index 8c033763c..430e06256 100644 --- a/src/pkg/selectors/scopes_test.go +++ b/src/pkg/selectors/scopes_test.go @@ -360,9 +360,11 @@ func (suite *SelectorScopesSuite) TestPasses() { entry = details.DetailsEntry{ RepoRef: pth.String(), } - pvs = cat.pathValues(pth, entry) ) + pvs, err := cat.pathValues(pth, entry) + require.NoError(suite.T(), err) + for _, test := range reduceTestTable { suite.Run(test.name, func() { t := suite.T() diff --git a/src/pkg/selectors/sharepoint.go b/src/pkg/selectors/sharepoint.go index 5ff36fcfa..0a06de432 100644 --- a/src/pkg/selectors/sharepoint.go +++ b/src/pkg/selectors/sharepoint.go @@ -3,6 +3,8 @@ package selectors import ( "context" + "github.com/alcionai/clues" + "github.com/alcionai/corso/src/internal/common" "github.com/alcionai/corso/src/pkg/backup/details" "github.com/alcionai/corso/src/pkg/fault" @@ -496,30 +498,44 @@ func (c sharePointCategory) isLeaf() bool { // Example: // [tenantID, service, siteID, category, folder, itemID] // => {spFolder: folder, spItemID: itemID} -func (c sharePointCategory) pathValues(repo path.Path, ent details.DetailsEntry) map[categorizer][]string { - var folderCat, itemCat categorizer +func (c sharePointCategory) pathValues( + repo path.Path, + ent details.DetailsEntry, +) (map[categorizer][]string, error) { + var ( + folderCat, itemCat categorizer + itemName = repo.Item() + ) switch c { case SharePointLibraryFolder, SharePointLibraryItem: + if ent.SharePoint == nil { + return nil, clues.New("no SharePoint ItemInfo in details") + } + folderCat, itemCat = SharePointLibraryFolder, SharePointLibraryItem + itemName = ent.SharePoint.ItemName + case SharePointList, SharePointListItem: folderCat, itemCat = SharePointList, SharePointListItem + case SharePointPage, SharePointPageFolder: folderCat, itemCat = SharePointPageFolder, SharePointPage + default: - return map[categorizer][]string{} + return nil, clues.New("unrecognized sharePointCategory").With("category", c) } result := map[categorizer][]string{ folderCat: {repo.Folder(false)}, - itemCat: {repo.Item(), ent.ShortRef}, + itemCat: {itemName, ent.ShortRef}, } if len(ent.LocationRef) > 0 { result[folderCat] = append(result[folderCat], ent.LocationRef) } - return result + return result, nil } // pathKeys returns the path keys recognized by the receiver's leaf type. diff --git a/src/pkg/selectors/sharepoint_test.go b/src/pkg/selectors/sharepoint_test.go index 812db2609..f89ac8f11 100644 --- a/src/pkg/selectors/sharepoint_test.go +++ b/src/pkg/selectors/sharepoint_test.go @@ -215,6 +215,7 @@ func (suite *SharePointSelectorSuite) TestSharePointRestore_Reduce() { ItemInfo: details.ItemInfo{ SharePoint: &details.SharePointInfo{ ItemType: details.SharePointLibrary, + ItemName: "itemName", }, }, }, @@ -223,6 +224,7 @@ func (suite *SharePointSelectorSuite) TestSharePointRestore_Reduce() { ItemInfo: details.ItemInfo{ SharePoint: &details.SharePointInfo{ ItemType: details.SharePointLibrary, + ItemName: "itemName2", }, }, }, @@ -231,6 +233,7 @@ func (suite *SharePointSelectorSuite) TestSharePointRestore_Reduce() { ItemInfo: details.ItemInfo{ SharePoint: &details.SharePointInfo{ ItemType: details.SharePointLibrary, + ItemName: "itemName3", }, }, }, @@ -239,6 +242,7 @@ func (suite *SharePointSelectorSuite) TestSharePointRestore_Reduce() { ItemInfo: details.ItemInfo{ SharePoint: &details.SharePointInfo{ ItemType: details.SharePointPage, + ItemName: "itemName4", }, }, }, @@ -247,6 +251,7 @@ func (suite *SharePointSelectorSuite) TestSharePointRestore_Reduce() { ItemInfo: details.ItemInfo{ SharePoint: &details.SharePointInfo{ ItemType: details.SharePointPage, + ItemName: "itemName5", }, }, }, @@ -279,7 +284,7 @@ func (suite *SharePointSelectorSuite) TestSharePointRestore_Reduce() { deets: deets, makeSelector: func() *SharePointRestore { odr := NewSharePointRestore(Any()) - odr.Include(odr.LibraryItems(Any(), []string{"item2"})) + odr.Include(odr.LibraryItems(Any(), []string{"itemName2"})) return odr }, expect: arr(item2), @@ -321,6 +326,10 @@ func (suite *SharePointSelectorSuite) TestSharePointRestore_Reduce() { } func (suite *SharePointSelectorSuite) TestSharePointCategory_PathValues() { + itemName := "item" + shortRef := "short" + elems := []string{"dir1", "dir2", itemName + "-id"} + table := []struct { name string sc sharePointCategory @@ -331,7 +340,7 @@ func (suite *SharePointSelectorSuite) TestSharePointCategory_PathValues() { sc: SharePointLibraryItem, expected: map[categorizer][]string{ SharePointLibraryFolder: {"dir1/dir2"}, - SharePointLibraryItem: {"item", "short"}, + SharePointLibraryItem: {itemName, shortRef}, }, }, { @@ -339,7 +348,7 @@ func (suite *SharePointSelectorSuite) TestSharePointCategory_PathValues() { sc: SharePointListItem, expected: map[categorizer][]string{ SharePointList: {"dir1/dir2"}, - SharePointListItem: {"item", "short"}, + SharePointListItem: {"item-id", shortRef}, }, }, } @@ -354,15 +363,21 @@ func (suite *SharePointSelectorSuite) TestSharePointCategory_PathValues() { path.SharePointService, test.sc.PathType(), true, - "dir1", "dir2", "item") + elems...) require.NoError(t, err) ent := details.DetailsEntry{ RepoRef: itemPath.String(), - ShortRef: "short", + ShortRef: shortRef, + ItemInfo: details.ItemInfo{ + SharePoint: &details.SharePointInfo{ + ItemName: itemName, + }, + }, } - pv := test.sc.pathValues(itemPath, ent) + pv, err := test.sc.pathValues(itemPath, ent) + require.NoError(t, err) assert.Equal(t, test.expected, pv) }) }