diff --git a/CHANGELOG.md b/CHANGELOG.md index 4860687ea..81b2cfee9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Permissions backup for OneDrive is now out of experimental (By default, only newly backed up items will have their permissions backed up. You will have to run a full backup to ensure all items have their permissions backed up.) +- LocationRef is now populated for all services and data types. It should be used in place of RepoRef if a location for an item is required. ### Fixed - Fixed permissions restore in latest backup version. diff --git a/src/internal/connector/graph_connector_onedrive_test.go b/src/internal/connector/graph_connector_onedrive_test.go index 71fc264c6..f188ecb3e 100644 --- a/src/internal/connector/graph_connector_onedrive_test.go +++ b/src/internal/connector/graph_connector_onedrive_test.go @@ -186,7 +186,7 @@ func (c *onedriveCollection) withFile(name string, fileData []byte, perm permDat c.items = append(c.items, metadata) c.aux = append(c.aux, metadata) - case version.OneDrive6NameInMeta: + case version.OneDrive6NameInMeta, version.OneDrive7LocationRef: c.items = append(c.items, onedriveItemWithData( c.t, name+onedrive.DataFileSuffix, @@ -213,7 +213,7 @@ 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, - version.OneDrive6NameInMeta: + version.OneDrive6NameInMeta, version.OneDrive7LocationRef: return c case version.OneDrive1DataAndMetaFiles, 2, version.OneDrive3IsMetaMarker: diff --git a/src/internal/connector/onedrive/collection.go b/src/internal/connector/onedrive/collection.go index 66c4396af..e98920c87 100644 --- a/src/internal/connector/onedrive/collection.go +++ b/src/internal/connector/onedrive/collection.go @@ -97,6 +97,12 @@ type Collection struct { // moved. It will be empty on its first retrieval. prevPath path.Path + // locPath represents the human-readable location of this collection. + locPath *path.Builder + // prevLocPath represents the human-readable location of this collection in + // the previous backup. + prevLocPath *path.Builder + // Specifies if it new, moved/rename or deleted state data.CollectionState @@ -135,6 +141,19 @@ type itemMetaReaderFunc func( item models.DriveItemable, ) (io.ReadCloser, int, error) +func pathToLocation(p path.Path) (*path.Builder, error) { + if p == nil { + return nil, nil + } + + odp, err := path.ToOneDrivePath(p) + if err != nil { + return nil, err + } + + return path.Builder{}.Append(odp.Root).Append(odp.Folders...), nil +} + // NewCollection creates a Collection func NewCollection( itemClient *http.Client, @@ -147,11 +166,27 @@ func NewCollection( ctrlOpts control.Options, colScope collectionScope, doNotMergeItems bool, -) *Collection { +) (*Collection, error) { + // TODO(ashmrtn): If OneDrive switches to using folder IDs then this will need + // to be changed as we won't be able to extract path information from the + // storage path. In that case, we'll need to start storing the location paths + // like we do the previous path. + locPath, err := pathToLocation(folderPath) + if err != nil { + return nil, clues.Wrap(err, "getting location").With("folder_path", folderPath.String()) + } + + prevLocPath, err := pathToLocation(prevPath) + if err != nil { + return nil, clues.Wrap(err, "getting previous location").With("prev_path", prevPath.String()) + } + c := &Collection{ itemClient: itemClient, folderPath: folderPath, prevPath: prevPath, + locPath: locPath, + prevLocPath: prevLocPath, driveItems: map[string]models.DriveItemable{}, driveID: driveID, source: source, @@ -176,7 +211,7 @@ func NewCollection( c.itemMetaReader = oneDriveItemMetaReader } - return c + return c, nil } // Adds an itemID to the collection. This will make it eligible to be @@ -229,6 +264,28 @@ func (oc *Collection) SetFullPath(curPath path.Path) { oc.state = data.StateOf(oc.prevPath, curPath) } +func (oc Collection) LocationPath() *path.Builder { + return oc.locPath +} + +func (oc Collection) PreviousLocationPath() details.LocationIDer { + if oc.prevLocPath == nil { + return nil + } + + switch oc.source { + case OneDriveSource: + return details.NewOneDriveLocationIDer( + oc.driveID, + oc.prevLocPath.Elements()...) + + default: + return details.NewSharePointLocationIDer( + oc.driveID, + oc.prevLocPath.Elements()...) + } +} + func (oc Collection) State() data.CollectionState { return oc.state } diff --git a/src/internal/connector/onedrive/collection_test.go b/src/internal/connector/onedrive/collection_test.go index 3afa31984..0f69b5455 100644 --- a/src/internal/connector/onedrive/collection_test.go +++ b/src/internal/connector/onedrive/collection_test.go @@ -205,7 +205,7 @@ func (suite *CollectionUnitTestSuite) TestCollection() { driveFolderPath, err := path.GetDriveFolderPath(folderPath) require.NoError(t, err, clues.ToCore(err)) - coll := NewCollection( + coll, err := NewCollection( graph.HTTPClient(graph.NoTimeout()), folderPath, nil, @@ -216,6 +216,7 @@ func (suite *CollectionUnitTestSuite) TestCollection() { control.Options{ToggleFeatures: control.Toggles{}}, CollectionScopeFolder, true) + require.NoError(t, err, clues.ToCore(err)) require.NotNil(t, coll) assert.Equal(t, folderPath, coll.FullPath()) @@ -344,7 +345,7 @@ func (suite *CollectionUnitTestSuite) TestCollectionReadError() { folderPath, err := GetCanonicalPath("drive/driveID1/root:/folderPath", "a-tenant", "a-user", test.source) require.NoError(t, err, clues.ToCore(err)) - coll := NewCollection( + coll, err := NewCollection( graph.HTTPClient(graph.NoTimeout()), folderPath, nil, @@ -355,6 +356,7 @@ func (suite *CollectionUnitTestSuite) TestCollectionReadError() { control.Options{ToggleFeatures: control.Toggles{}}, CollectionScopeFolder, true) + require.NoError(t, err, clues.ToCore(err)) mockItem := models.NewDriveItem() mockItem.SetId(&testItemID) @@ -433,7 +435,7 @@ func (suite *CollectionUnitTestSuite) TestCollectionReadUnauthorizedErrorRetry() folderPath, err := GetCanonicalPath("drive/driveID1/root:/folderPath", "a-tenant", "a-user", test.source) require.NoError(t, err) - coll := NewCollection( + coll, err := NewCollection( graph.HTTPClient(graph.NoTimeout()), folderPath, nil, @@ -444,6 +446,7 @@ func (suite *CollectionUnitTestSuite) TestCollectionReadUnauthorizedErrorRetry() control.Options{ToggleFeatures: control.Toggles{}}, CollectionScopeFolder, true) + require.NoError(t, err, clues.ToCore(err)) mockItem := models.NewDriveItem() mockItem.SetId(&testItemID) @@ -532,7 +535,7 @@ func (suite *CollectionUnitTestSuite) TestCollectionPermissionBackupLatestModTim folderPath, err := GetCanonicalPath("drive/driveID1/root:/folderPath", "a-tenant", "a-user", test.source) require.NoError(t, err, clues.ToCore(err)) - coll := NewCollection( + coll, err := NewCollection( graph.HTTPClient(graph.NoTimeout()), folderPath, nil, @@ -543,6 +546,7 @@ func (suite *CollectionUnitTestSuite) TestCollectionPermissionBackupLatestModTim control.Options{ToggleFeatures: control.Toggles{}}, CollectionScopeFolder, true) + require.NoError(t, err, clues.ToCore(err)) mtime := time.Now().AddDate(0, -1, 0) mockItem := models.NewDriveItem() diff --git a/src/internal/connector/onedrive/collections.go b/src/internal/connector/onedrive/collections.go index 4ab890552..e7c1e782b 100644 --- a/src/internal/connector/onedrive/collections.go +++ b/src/internal/connector/onedrive/collections.go @@ -416,7 +416,7 @@ func (c *Collections) Get( return nil, map[string]map[string]struct{}{}, err } - col := NewCollection( + col, err := NewCollection( c.itemClient, nil, prevPath, @@ -427,6 +427,9 @@ func (c *Collections) Get( c.ctrl, CollectionScopeUnknown, true) + if err != nil { + return nil, map[string]map[string]struct{}{}, clues.Wrap(err, "making collection").WithClues(ictx) + } c.CollectionMap[driveID][fldID] = col } @@ -578,7 +581,7 @@ func (c *Collections) handleDelete( return nil } - col := NewCollection( + col, err := NewCollection( c.itemClient, nil, prevPath, @@ -590,6 +593,12 @@ func (c *Collections) handleDelete( CollectionScopeUnknown, // DoNotMerge is not checked for deleted items. false) + if err != nil { + return clues.Wrap(err, "making collection").With( + "drive_id", driveID, + "item_id", itemID, + "path_string", prevPathStr) + } c.CollectionMap[driveID][itemID] = col @@ -765,7 +774,7 @@ func (c *Collections) UpdateCollections( colScope = CollectionScopePackage } - col := NewCollection( + col, err := NewCollection( c.itemClient, collectionPath, prevPath, @@ -777,6 +786,10 @@ func (c *Collections) UpdateCollections( colScope, invalidPrevDelta, ) + if err != nil { + return clues.Stack(err).WithClues(ictx) + } + col.driveName = driveName c.CollectionMap[driveID][itemID] = col diff --git a/src/internal/connector/onedrive/restore_test.go b/src/internal/connector/onedrive/restore_test.go index f0ecb7580..a05be92c1 100644 --- a/src/internal/connector/onedrive/restore_test.go +++ b/src/internal/connector/onedrive/restore_test.go @@ -24,9 +24,7 @@ func TestRestoreUnitSuite(t *testing.T) { func (suite *RestoreUnitSuite) TestAugmentRestorePaths() { // Adding a simple test here so that we can be sure that this // function gets updated whenever we add a new version. - if version.Backup > version.OneDrive6NameInMeta { - require.Less(suite.T(), version.OneDrive6NameInMeta+1, version.Backup, "unsupported backup version") - } + require.LessOrEqual(suite.T(), version.Backup, version.OneDrive7LocationRef, "unsupported backup version") table := []struct { name string diff --git a/src/internal/kopia/merge_details.go b/src/internal/kopia/merge_details.go index e1bb3cf55..5917892a7 100644 --- a/src/internal/kopia/merge_details.go +++ b/src/internal/kopia/merge_details.go @@ -21,7 +21,7 @@ type DetailsMergeInfoer interface { GetNewPathRefs( oldRef *path.Builder, oldLoc details.LocationIDer, - ) (path.Path, *path.Builder, *path.Builder) + ) (path.Path, *path.Builder, error) } type prevRef struct { @@ -68,36 +68,37 @@ func (m *mergeDetails) addRepoRef( func (m *mergeDetails) GetNewPathRefs( oldRef *path.Builder, oldLoc details.LocationIDer, -) (path.Path, *path.Builder, *path.Builder) { +) (path.Path, *path.Builder, error) { pr, ok := m.repoRefs[oldRef.ShortRef()] if !ok { return nil, nil, nil } - // This was a location specified directly by a collection. Say the prefix is - // the whole oldLoc so other code will replace everything. - // - // TODO(ashmrtn): Should be able to remove the nil check later as we'll be - // able to ensure that old locations actually exist in backup details. - if oldLoc == nil { - return pr.repoRef, nil, pr.locRef - } else if pr.locRef != nil { - return pr.repoRef, oldLoc.InDetails(), pr.locRef + // This was a location specified directly by a collection. + if pr.locRef != nil { + return pr.repoRef, pr.locRef, nil + } else if oldLoc == nil || oldLoc.ID() == nil || len(oldLoc.ID().Elements()) == 0 { + return nil, nil, clues.New("empty location key") } // This is a location that we need to do prefix matching on because we didn't // see the new location of it in a collection. For example, it's a subfolder // whose parent folder was moved. - prefixes := m.locations.longestPrefix(oldLoc.ID()) + prefixes := m.locations.longestPrefix(oldLoc) + newLoc := oldLoc.InDetails() - return pr.repoRef, prefixes.oldLoc, prefixes.newLoc + // Noop if prefix or newPrefix are nil. Them being nil means that the + // LocationRef hasn't changed. + newLoc.UpdateParent(prefixes.oldLoc, prefixes.newLoc) + + return pr.repoRef, newLoc, nil } func (m *mergeDetails) addLocation( oldRef details.LocationIDer, newLoc *path.Builder, ) error { - return m.locations.add(oldRef.ID(), newLoc) + return m.locations.add(oldRef, newLoc) } func newMergeDetails() *mergeDetails { @@ -116,20 +117,25 @@ type locationPrefixMatcher struct { m prefixmatcher.Matcher[locRefs] } -func (m *locationPrefixMatcher) add(oldRef, newLoc *path.Builder) error { - key := oldRef.String() +func (m *locationPrefixMatcher) add( + oldRef details.LocationIDer, + newLoc *path.Builder, +) error { + key := oldRef.ID().String() if _, ok := m.m.Get(key); ok { return clues.New("RepoRef already in matcher").With("repo_ref", oldRef) } - m.m.Add(key, locRefs{oldLoc: oldRef, newLoc: newLoc}) + m.m.Add(key, locRefs{oldLoc: oldRef.InDetails(), newLoc: newLoc}) return nil } -func (m *locationPrefixMatcher) longestPrefix(oldRef *path.Builder) locRefs { - _, v, _ := m.m.LongestPrefix(oldRef.String()) +func (m *locationPrefixMatcher) longestPrefix( + oldRef details.LocationIDer, +) locRefs { + _, v, _ := m.m.LongestPrefix(oldRef.ID().String()) return v } diff --git a/src/internal/kopia/merge_details_test.go b/src/internal/kopia/merge_details_test.go index b1b90c1f6..6dfee6381 100644 --- a/src/internal/kopia/merge_details_test.go +++ b/src/internal/kopia/merge_details_test.go @@ -97,10 +97,11 @@ func (suite *DetailsMergeInfoerUnitSuite) TestGetNewPathRefs() { testUser, category, "folder3", - "folder4", + "folder2", }, false) newLoc1 := path.Builder{}.Append(newRef1.Folders()...) + newLoc2 := path.Builder{}.Append(newRef2.Folders()...) oldLoc1 := path.Builder{}.Append(oldRef1.Folders()...) oldLoc2 := path.Builder{}.Append(oldRef2.Folders()...) @@ -120,69 +121,62 @@ func (suite *DetailsMergeInfoerUnitSuite) TestGetNewPathRefs() { require.NoError(t, err, clues.ToCore(err)) table := []struct { - name string - searchRef *path.Builder - searchLoc mockLocationIDer - expectedRef path.Path - prefixFound bool - expectedOldPrefix *path.Builder + name string + searchRef *path.Builder + searchLoc mockLocationIDer + errCheck require.ErrorAssertionFunc + expectedRef path.Path + expectedLoc *path.Builder }{ { - name: "Exact Match With Loc", - searchRef: oldRef1.ToBuilder(), - searchLoc: searchLoc1, - expectedRef: newRef1, - prefixFound: true, - expectedOldPrefix: oldLoc1, - }, - { - name: "Exact Match Without Loc", - searchRef: oldRef1.ToBuilder(), - expectedRef: newRef1, - prefixFound: true, - expectedOldPrefix: nil, - }, - { - name: "Prefix Match", - searchRef: oldRef2.ToBuilder(), - searchLoc: searchLoc2, - expectedRef: newRef2, - prefixFound: true, - expectedOldPrefix: oldLoc1, - }, - { - name: "Not Found", - searchRef: newRef1.ToBuilder(), - expectedRef: nil, - }, - { - name: "Not Found With Loc", - searchRef: newRef1.ToBuilder(), + name: "Exact Match With Loc", + searchRef: oldRef1.ToBuilder(), searchLoc: searchLoc1, - expectedRef: nil, + errCheck: require.NoError, + expectedRef: newRef1, + expectedLoc: newLoc1, }, { - name: "Ref Found Loc Not", + name: "Exact Match Without Loc", + searchRef: oldRef1.ToBuilder(), + errCheck: require.NoError, + expectedRef: newRef1, + expectedLoc: newLoc1, + }, + { + name: "Prefix Match", searchRef: oldRef2.ToBuilder(), - searchLoc: mockLocationIDer{path.Builder{}.Append("foo")}, + searchLoc: searchLoc2, + errCheck: require.NoError, expectedRef: newRef2, + expectedLoc: newLoc2, + }, + { + name: "Would Be Prefix Match Without Old Loc Errors", + searchRef: oldRef2.ToBuilder(), + errCheck: require.Error, + }, + { + name: "Not Found With Old Loc", + searchRef: newRef1.ToBuilder(), + searchLoc: searchLoc2, + errCheck: require.NoError, + }, + { + name: "Not Found Without Old Loc", + searchRef: newRef1.ToBuilder(), + errCheck: require.NoError, }, } for _, test := range table { suite.Run(test.name, func() { t := suite.T() - newRef, oldPrefix, newPrefix := dm.GetNewPathRefs(test.searchRef, test.searchLoc) + newRef, newLoc, err := dm.GetNewPathRefs(test.searchRef, test.searchLoc) + test.errCheck(t, err, clues.ToCore(err)) + assert.Equal(t, test.expectedRef, newRef, "RepoRef") - - if !test.prefixFound { - assert.Nil(t, oldPrefix) - assert.Nil(t, newPrefix) - return - } - - assert.Equal(t, test.expectedOldPrefix, oldPrefix, "old prefix") - assert.Equal(t, newLoc1, newPrefix, "new prefix") + assert.Equal(t, test.expectedLoc, newLoc, "LocationRef") }) } } @@ -197,7 +191,7 @@ func TestLocationPrefixMatcherUnitSuite(t *testing.T) { func (suite *LocationPrefixMatcherUnitSuite) TestAdd_Twice_Fails() { t := suite.T() - p := makePath( + p := mockLocationIDer{makePath( t, []string{ testTenant, @@ -206,7 +200,7 @@ func (suite *LocationPrefixMatcherUnitSuite) TestAdd_Twice_Fails() { category, "folder1", }, - false).ToBuilder() + false).ToBuilder()} loc1 := path.Builder{}.Append("folder1") loc2 := path.Builder{}.Append("folder2") @@ -220,20 +214,20 @@ func (suite *LocationPrefixMatcherUnitSuite) TestAdd_Twice_Fails() { } func (suite *LocationPrefixMatcherUnitSuite) TestAdd_And_Match() { - loc1 := path.Builder{}.Append("folder1") - loc2 := loc1.Append("folder2") - loc3 := path.Builder{}.Append("foo") + loc1 := mockLocationIDer{path.Builder{}.Append("folder1")} + loc2 := mockLocationIDer{loc1.InDetails().Append("folder2")} + loc3 := mockLocationIDer{path.Builder{}.Append("foo")} - res1 := path.Builder{}.Append("1") + res1 := mockLocationIDer{path.Builder{}.Append("1")} lpm := newLocationPrefixMatcher() - err := lpm.add(loc1, res1) + err := lpm.add(loc1, res1.InDetails()) require.NoError(suite.T(), err, clues.ToCore(err)) table := []struct { name string - searchKey *path.Builder + searchKey mockLocationIDer found bool }{ { @@ -265,8 +259,8 @@ func (suite *LocationPrefixMatcherUnitSuite) TestAdd_And_Match() { return } - assert.Equal(t, loc1, prefixes.oldLoc, "old prefix") - assert.Equal(t, res1, prefixes.newLoc, "new prefix") + assert.Equal(t, loc1.InDetails(), prefixes.oldLoc, "old prefix") + assert.Equal(t, res1.InDetails(), prefixes.newLoc, "new prefix") }) } } diff --git a/src/internal/operations/backup.go b/src/internal/operations/backup.go index 05cfe77cb..978c6e2a2 100644 --- a/src/internal/operations/backup.go +++ b/src/internal/operations/backup.go @@ -524,7 +524,8 @@ func matchesReason(reasons []kopia.Reason, p path.Path) bool { func getNewPathRefs( dataFromBackup kopia.DetailsMergeInfoer, entry *details.DetailsEntry, - repoRef *path.Builder, + repoRef path.Path, + backupVersion int, ) (path.Path, *path.Builder, bool, 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 @@ -533,18 +534,52 @@ func getNewPathRefs( // 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. - newPath, _, newLocPrefix := dataFromBackup.GetNewPathRefs(repoRef, nil) - if newPath == nil { - // This entry doesn't need merging. + if repoRef.Service() == path.ExchangeService { + newPath, newLoc, err := dataFromBackup.GetNewPathRefs(repoRef.ToBuilder(), nil) + if err != nil { + return nil, nil, false, clues.Wrap(err, "getting new paths") + } else if newPath == nil { + // This entry doesn't need merging. + return nil, nil, false, nil + } else if newLoc == nil { + return nil, nil, false, clues.New("unable to find new exchange location") + } + + // This is kind of jank cause we're in a transitionary period, but even if + // we're consesrvative here about marking something as updated the RepoRef + // comparison in the caller should catch the change. Calendars is the only + // exception, since it uses IDs for folders, but we should already be + // populating the LocationRef for them. + // + // Without this, all OneDrive items will be marked as updated the first time + // around because OneDrive hasn't been persisting LocationRef before now. + updated := len(entry.LocationRef) > 0 && newLoc.String() != entry.LocationRef + + return newPath, newLoc, updated, nil + } + + // We didn't have an exact entry, so retry with a location. + locRef, err := entry.ToLocationIDer(backupVersion) + if err != nil { + return nil, nil, false, clues.Wrap(err, "getting previous item location") + } + + if locRef == nil { + return nil, nil, false, clues.New("entry with empty LocationRef") + } + + newPath, newLoc, err := dataFromBackup.GetNewPathRefs(repoRef.ToBuilder(), locRef) + if err != nil { + return nil, nil, false, clues.Wrap(err, "getting new paths with old location") + } else if newPath == nil { return nil, nil, false, nil + } else if newLoc == nil { + return nil, nil, false, clues.New("unable to get new paths") } - // OneDrive doesn't return prefixes yet. - if newLocPrefix == nil { - newLocPrefix = &path.Builder{} - } + updated := len(entry.LocationRef) > 0 && newLoc.String() != entry.LocationRef - return newPath, newLocPrefix, newLocPrefix.String() != entry.LocationRef, nil + return newPath, newLoc, updated, nil } func mergeDetails( @@ -582,7 +617,7 @@ func mergeDetails( mctx = clues.Add(mctx, "base_manifest_backup_id", bID) - _, baseDeets, err := getBackupAndDetailsFromID( + baseBackup, baseDeets, err := getBackupAndDetailsFromID( mctx, model.StableID(bID), ms, @@ -615,7 +650,8 @@ func mergeDetails( newPath, newLoc, locUpdated, err := getNewPathRefs( dataFromBackup, entry, - rr.ToBuilder()) + rr, + baseBackup.Version) if err != nil { return clues.Wrap(err, "getting updated info for entry").WithClues(mctx) } @@ -627,9 +663,7 @@ func mergeDetails( // Fixup paths in the item. item := entry.ItemInfo - if err := details.UpdateItem(&item, newPath, newLoc); err != nil { - return clues.Wrap(err, "updating merged item info").WithClues(mctx) - } + details.UpdateItem(&item, newLoc) // TODO(ashmrtn): This may need updated if we start using this merge // strategry for items that were cached in kopia. diff --git a/src/internal/operations/backup_test.go b/src/internal/operations/backup_test.go index 4c8671800..41c4d1849 100644 --- a/src/internal/operations/backup_test.go +++ b/src/internal/operations/backup_test.go @@ -183,36 +183,25 @@ func (mbs mockBackupStorer) Update(context.Context, model.Schema, model.Model) e // ----- model store for backups -type locPair struct { - old *path.Builder - newL *path.Builder -} - type mockDetailsMergeInfoer struct { repoRefs map[string]path.Path - locs map[string]locPair + locs map[string]*path.Builder } -func (m *mockDetailsMergeInfoer) add(oldRef, newRef path.Path, oldPrefix, newLoc *path.Builder) { +func (m *mockDetailsMergeInfoer) add(oldRef, newRef path.Path, newLoc *path.Builder) { oldPB := oldRef.ToBuilder() // Items are indexed individually. m.repoRefs[oldPB.ShortRef()] = newRef - if newLoc != nil { - // Locations are indexed by directory. - m.locs[oldPB.ShortRef()] = locPair{ - old: oldPrefix, - newL: newLoc, - } - } + // Locations are indexed by directory. + m.locs[oldPB.ShortRef()] = newLoc } func (m *mockDetailsMergeInfoer) GetNewPathRefs( oldRef *path.Builder, - oldLoc details.LocationIDer, -) (path.Path, *path.Builder, *path.Builder) { - locs := m.locs[oldRef.ShortRef()] - return m.repoRefs[oldRef.ShortRef()], locs.old, locs.newL + _ details.LocationIDer, +) (path.Path, *path.Builder, error) { + return m.repoRefs[oldRef.ShortRef()], m.locs[oldRef.ShortRef()], nil } func (m *mockDetailsMergeInfoer) ItemsToMerge() int { @@ -226,7 +215,7 @@ func (m *mockDetailsMergeInfoer) ItemsToMerge() int { func newMockDetailsMergeInfoer() *mockDetailsMergeInfoer { return &mockDetailsMergeInfoer{ repoRefs: map[string]path.Path{}, - locs: map[string]locPair{}, + locs: map[string]*path.Builder{}, } } @@ -351,13 +340,13 @@ func makeDetailsEntry( } case path.OneDriveService: - parent, err := path.GetDriveFolderPath(p) - require.NoError(t, err, clues.ToCore(err)) + require.NotNil(t, l) res.OneDrive = &details.OneDriveInfo{ ItemType: details.OneDriveItem, - ParentPath: parent, + ParentPath: l.PopFront().String(), Size: int64(size), + DriveID: "drive-id", } default: @@ -744,7 +733,7 @@ func (suite *BackupOpUnitSuite) TestBackupOperation_MergeBackupDetails_AddsItems name: "BackupIDNotFound", mdm: func() *mockDetailsMergeInfoer { res := newMockDetailsMergeInfoer() - res.add(itemPath1, itemPath1, locationPath1, locationPath1) + res.add(itemPath1, itemPath1, locationPath1) return res }(), @@ -762,7 +751,7 @@ func (suite *BackupOpUnitSuite) TestBackupOperation_MergeBackupDetails_AddsItems name: "DetailsIDNotFound", mdm: func() *mockDetailsMergeInfoer { res := newMockDetailsMergeInfoer() - res.add(itemPath1, itemPath1, locationPath1, locationPath1) + res.add(itemPath1, itemPath1, locationPath1) return res }(), @@ -788,8 +777,8 @@ func (suite *BackupOpUnitSuite) TestBackupOperation_MergeBackupDetails_AddsItems name: "BaseMissingItems", mdm: func() *mockDetailsMergeInfoer { res := newMockDetailsMergeInfoer() - res.add(itemPath1, itemPath1, locationPath1, locationPath1) - res.add(itemPath2, itemPath2, locationPath2, locationPath2) + res.add(itemPath1, itemPath1, locationPath1) + res.add(itemPath2, itemPath2, locationPath2) return res }(), @@ -819,7 +808,7 @@ func (suite *BackupOpUnitSuite) TestBackupOperation_MergeBackupDetails_AddsItems name: "TooManyItems", mdm: func() *mockDetailsMergeInfoer { res := newMockDetailsMergeInfoer() - res.add(itemPath1, itemPath1, locationPath1, locationPath1) + res.add(itemPath1, itemPath1, locationPath1) return res }(), @@ -855,7 +844,7 @@ func (suite *BackupOpUnitSuite) TestBackupOperation_MergeBackupDetails_AddsItems name: "BadBaseRepoRef", mdm: func() *mockDetailsMergeInfoer { res := newMockDetailsMergeInfoer() - res.add(itemPath1, itemPath2, locationPath1, locationPath2) + res.add(itemPath1, itemPath2, locationPath2) return res }(), @@ -917,7 +906,7 @@ func (suite *BackupOpUnitSuite) TestBackupOperation_MergeBackupDetails_AddsItems true, ) - res.add(itemPath1, p, locationPath1, nil) + res.add(itemPath1, p, nil) return res }(), @@ -947,7 +936,7 @@ func (suite *BackupOpUnitSuite) TestBackupOperation_MergeBackupDetails_AddsItems name: "ItemMerged", mdm: func() *mockDetailsMergeInfoer { res := newMockDetailsMergeInfoer() - res.add(itemPath1, itemPath1, locationPath1, locationPath1) + res.add(itemPath1, itemPath1, locationPath1) return res }(), @@ -980,7 +969,7 @@ func (suite *BackupOpUnitSuite) TestBackupOperation_MergeBackupDetails_AddsItems name: "ItemMergedSameLocation", mdm: func() *mockDetailsMergeInfoer { res := newMockDetailsMergeInfoer() - res.add(itemPath1, itemPath1, locationPath1, locationPath1) + res.add(itemPath1, itemPath1, locationPath1) return res }(), @@ -1013,7 +1002,7 @@ func (suite *BackupOpUnitSuite) TestBackupOperation_MergeBackupDetails_AddsItems name: "ItemMergedExtraItemsInBase", mdm: func() *mockDetailsMergeInfoer { res := newMockDetailsMergeInfoer() - res.add(itemPath1, itemPath1, locationPath1, locationPath1) + res.add(itemPath1, itemPath1, locationPath1) return res }(), @@ -1047,7 +1036,7 @@ func (suite *BackupOpUnitSuite) TestBackupOperation_MergeBackupDetails_AddsItems name: "ItemMoved", mdm: func() *mockDetailsMergeInfoer { res := newMockDetailsMergeInfoer() - res.add(itemPath1, itemPath2, locationPath1, locationPath2) + res.add(itemPath1, itemPath2, locationPath2) return res }(), @@ -1080,8 +1069,8 @@ func (suite *BackupOpUnitSuite) TestBackupOperation_MergeBackupDetails_AddsItems name: "MultipleBases", mdm: func() *mockDetailsMergeInfoer { res := newMockDetailsMergeInfoer() - res.add(itemPath1, itemPath1, locationPath1, locationPath1) - res.add(itemPath3, itemPath3, locationPath3, locationPath3) + res.add(itemPath1, itemPath1, locationPath1) + res.add(itemPath3, itemPath3, locationPath3) return res }(), @@ -1132,7 +1121,7 @@ func (suite *BackupOpUnitSuite) TestBackupOperation_MergeBackupDetails_AddsItems name: "SomeBasesIncomplete", mdm: func() *mockDetailsMergeInfoer { res := newMockDetailsMergeInfoer() - res.add(itemPath1, itemPath1, locationPath1, locationPath1) + res.add(itemPath1, itemPath1, locationPath1) return res }(), @@ -1263,7 +1252,7 @@ func (suite *BackupOpUnitSuite) TestBackupOperation_MergeBackupDetails_AddsFolde ) mdm := newMockDetailsMergeInfoer() - mdm.add(itemPath1, itemPath1, locPath1, locPath1) + mdm.add(itemPath1, itemPath1, locPath1) itemDetails := makeDetailsEntry(t, itemPath1, locPath1, itemSize, false) // itemDetails.Exchange.Modified = now diff --git a/src/internal/version/backup.go b/src/internal/version/backup.go index 9ba2d3660..29e697bd8 100644 --- a/src/internal/version/backup.go +++ b/src/internal/version/backup.go @@ -1,6 +1,6 @@ package version -const Backup = 6 +const Backup = 7 // Various labels to refer to important version changes. // Labels don't need 1:1 service:version representation. Add a new @@ -38,5 +38,5 @@ const ( // OneDriveXLocationRef provides LocationRef information for Exchange, // OneDrive, and SharePoint libraries. - OneDriveXLocationRef = Backup + 1 + OneDrive7LocationRef = 7 ) diff --git a/src/pkg/backup/details/details.go b/src/pkg/backup/details/details.go index 8c0e162bd..b77891743 100644 --- a/src/pkg/backup/details/details.go +++ b/src/pkg/backup/details/details.go @@ -457,7 +457,7 @@ func (de DetailsEntry) ToLocationIDer(backupVersion int) (LocationIDer, error) { return de.ItemInfo.uniqueLocation(baseLoc) } - if backupVersion >= version.OneDriveXLocationRef || + if backupVersion >= version.OneDrive7LocationRef || (de.ItemInfo.infoType() != OneDriveItem && de.ItemInfo.infoType() != SharePointLibrary) { return nil, clues.New("no previous location for entry") @@ -578,10 +578,10 @@ const ( FolderItem ItemType = 306 ) -func UpdateItem(item *ItemInfo, repoPath path.Path, locPath *path.Builder) error { +func UpdateItem(item *ItemInfo, newLocPath *path.Builder) { // Only OneDrive and SharePoint have information about parent folders // contained in them. - var updatePath func(repo path.Path, location *path.Builder) error + var updatePath func(newLocPath *path.Builder) switch item.infoType() { case ExchangeContact, ExchangeEvent, ExchangeMail: @@ -591,10 +591,10 @@ func UpdateItem(item *ItemInfo, repoPath path.Path, locPath *path.Builder) error case OneDriveItem: updatePath = item.OneDrive.UpdateParentPath default: - return nil + return } - return updatePath(repoPath, locPath) + updatePath(newLocPath) } // ItemInfo is a oneOf that contains service specific @@ -758,9 +758,8 @@ func (i ExchangeInfo) Values() []string { return []string{} } -func (i *ExchangeInfo) UpdateParentPath(_ path.Path, locPath *path.Builder) error { - i.ParentPath = locPath.String() - return nil +func (i *ExchangeInfo) UpdateParentPath(newLocPath *path.Builder) { + i.ParentPath = newLocPath.String() } func (i *ExchangeInfo) uniqueLocation(baseLoc *path.Builder) (LocationIDer, error) { @@ -812,15 +811,8 @@ func (i SharePointInfo) Values() []string { } } -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) - } - - i.ParentPath = newParent - - return nil +func (i *SharePointInfo) UpdateParentPath(newLocPath *path.Builder) { + i.ParentPath = newLocPath.PopFront().String() } func (i *SharePointInfo) uniqueLocation(baseLoc *path.Builder) (LocationIDer, error) { @@ -864,15 +856,8 @@ func (i OneDriveInfo) Values() []string { } } -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) - } - - i.ParentPath = newParent - - return nil +func (i *OneDriveInfo) UpdateParentPath(newLocPath *path.Builder) { + i.ParentPath = newLocPath.PopFront().String() } func (i *OneDriveInfo) uniqueLocation(baseLoc *path.Builder) (LocationIDer, error) { diff --git a/src/pkg/backup/details/details_test.go b/src/pkg/backup/details/details_test.go index 96c69c151..93620decb 100644 --- a/src/pkg/backup/details/details_test.go +++ b/src/pkg/backup/details/details_test.go @@ -857,47 +857,17 @@ func makeItemPath( func (suite *DetailsUnitSuite) TestUpdateItem() { const ( - tenant = "a-tenant" - resourceOwner = "a-user" - driveID = "abcd" - folder1 = "f1" - folder2 = "f2" - folder3 = "f3" - item = "hello.txt" + folder1 = "f1" + folder2 = "f2" ) - // Making both OneDrive paths is alright because right now they're the same as - // SharePoint path and there's no extra validation. - newOneDrivePath := makeItemPath( - suite.T(), - path.OneDriveService, - path.FilesCategory, - tenant, - resourceOwner, - []string{ - "drives", - driveID, - "root:", - folder2, - item, - }, - ) - newExchangePB := path.Builder{}.Append(folder3) - badOneDrivePath := makeItemPath( - suite.T(), - path.OneDriveService, - path.FilesCategory, - tenant, - resourceOwner, - []string{item}, - ) + newExchangePB := path.Builder{}.Append(folder2) + newOneDrivePB := path.Builder{}.Append("root:", folder2) table := []struct { name string input ItemInfo - repoPath path.Path locPath *path.Builder - errCheck assert.ErrorAssertionFunc expectedItem ItemInfo }{ { @@ -908,13 +878,11 @@ func (suite *DetailsUnitSuite) TestUpdateItem() { ParentPath: folder1, }, }, - repoPath: newOneDrivePath, - locPath: newExchangePB, - errCheck: assert.NoError, + locPath: newExchangePB, expectedItem: ItemInfo{ Exchange: &ExchangeInfo{ ItemType: ExchangeEvent, - ParentPath: folder3, + ParentPath: folder2, }, }, }, @@ -926,13 +894,11 @@ func (suite *DetailsUnitSuite) TestUpdateItem() { ParentPath: folder1, }, }, - repoPath: newOneDrivePath, - locPath: newExchangePB, - errCheck: assert.NoError, + locPath: newExchangePB, expectedItem: ItemInfo{ Exchange: &ExchangeInfo{ ItemType: ExchangeContact, - ParentPath: folder3, + ParentPath: folder2, }, }, }, @@ -944,13 +910,11 @@ func (suite *DetailsUnitSuite) TestUpdateItem() { ParentPath: folder1, }, }, - repoPath: newOneDrivePath, - locPath: newExchangePB, - errCheck: assert.NoError, + locPath: newExchangePB, expectedItem: ItemInfo{ Exchange: &ExchangeInfo{ ItemType: ExchangeMail, - ParentPath: folder3, + ParentPath: folder2, }, }, }, @@ -962,9 +926,7 @@ func (suite *DetailsUnitSuite) TestUpdateItem() { ParentPath: folder1, }, }, - repoPath: newOneDrivePath, - locPath: newExchangePB, - errCheck: assert.NoError, + locPath: newOneDrivePB, expectedItem: ItemInfo{ OneDrive: &OneDriveInfo{ ItemType: OneDriveItem, @@ -980,9 +942,7 @@ func (suite *DetailsUnitSuite) TestUpdateItem() { ParentPath: folder1, }, }, - repoPath: newOneDrivePath, - locPath: newExchangePB, - errCheck: assert.NoError, + locPath: newOneDrivePB, expectedItem: ItemInfo{ SharePoint: &SharePointInfo{ ItemType: SharePointLibrary, @@ -990,44 +950,14 @@ func (suite *DetailsUnitSuite) TestUpdateItem() { }, }, }, - { - name: "OneDriveBadPath", - input: ItemInfo{ - OneDrive: &OneDriveInfo{ - ItemType: OneDriveItem, - ParentPath: folder1, - }, - }, - repoPath: badOneDrivePath, - locPath: newExchangePB, - errCheck: assert.Error, - }, - { - name: "SharePointBadPath", - input: ItemInfo{ - SharePoint: &SharePointInfo{ - ItemType: SharePointLibrary, - ParentPath: folder1, - }, - }, - repoPath: badOneDrivePath, - locPath: newExchangePB, - errCheck: assert.Error, - }, } for _, test := range table { suite.Run(test.name, func() { t := suite.T() + item := test.input - - err := UpdateItem(&item, test.repoPath, test.locPath) - test.errCheck(t, err, clues.ToCore(err)) - - if err != nil { - return - } - + UpdateItem(&item, test.locPath) assert.Equal(t, test.expectedItem, item) }) } @@ -1255,7 +1185,7 @@ func (suite *DetailsUnitSuite) TestLocationIDer_FromEntry() { DriveID: driveID, }, }, - backupVersion: version.OneDriveXLocationRef - 1, + backupVersion: version.OneDrive7LocationRef - 1, expectedErr: require.NoError, expectedUniqueLoc: fmt.Sprintf(expectedUniqueLocFmt, path.FilesCategory), }, @@ -1269,7 +1199,7 @@ func (suite *DetailsUnitSuite) TestLocationIDer_FromEntry() { DriveID: driveID, }, }, - backupVersion: version.OneDriveXLocationRef, + backupVersion: version.OneDrive7LocationRef, hasLocRef: true, expectedErr: require.NoError, expectedUniqueLoc: fmt.Sprintf(expectedUniqueLocFmt, path.FilesCategory), @@ -1284,7 +1214,7 @@ func (suite *DetailsUnitSuite) TestLocationIDer_FromEntry() { DriveID: driveID, }, }, - backupVersion: version.OneDriveXLocationRef, + backupVersion: version.OneDrive7LocationRef, expectedErr: require.Error, }, { @@ -1297,7 +1227,7 @@ func (suite *DetailsUnitSuite) TestLocationIDer_FromEntry() { DriveID: driveID, }, }, - backupVersion: version.OneDriveXLocationRef - 1, + backupVersion: version.OneDrive7LocationRef - 1, expectedErr: require.NoError, expectedUniqueLoc: fmt.Sprintf(expectedUniqueLocFmt, path.LibrariesCategory), }, @@ -1311,7 +1241,7 @@ func (suite *DetailsUnitSuite) TestLocationIDer_FromEntry() { DriveID: driveID, }, }, - backupVersion: version.OneDriveXLocationRef, + backupVersion: version.OneDrive7LocationRef, hasLocRef: true, expectedErr: require.NoError, expectedUniqueLoc: fmt.Sprintf(expectedUniqueLocFmt, path.LibrariesCategory), @@ -1326,7 +1256,7 @@ func (suite *DetailsUnitSuite) TestLocationIDer_FromEntry() { DriveID: driveID, }, }, - backupVersion: version.OneDriveXLocationRef, + backupVersion: version.OneDrive7LocationRef, expectedErr: require.Error, }, { @@ -1338,7 +1268,7 @@ func (suite *DetailsUnitSuite) TestLocationIDer_FromEntry() { ItemType: ExchangeMail, }, }, - backupVersion: version.OneDriveXLocationRef - 1, + backupVersion: version.OneDrive7LocationRef - 1, hasLocRef: true, expectedErr: require.NoError, expectedUniqueLoc: fmt.Sprintf(expectedExchangeUniqueLocFmt, path.EmailCategory), @@ -1352,7 +1282,7 @@ func (suite *DetailsUnitSuite) TestLocationIDer_FromEntry() { ItemType: ExchangeMail, }, }, - backupVersion: version.OneDriveXLocationRef, + backupVersion: version.OneDrive7LocationRef, hasLocRef: true, expectedErr: require.NoError, expectedUniqueLoc: fmt.Sprintf(expectedExchangeUniqueLocFmt, path.EmailCategory), @@ -1366,7 +1296,7 @@ func (suite *DetailsUnitSuite) TestLocationIDer_FromEntry() { ItemType: ExchangeMail, }, }, - backupVersion: version.OneDriveXLocationRef - 1, + backupVersion: version.OneDrive7LocationRef - 1, expectedErr: require.Error, }, { @@ -1378,7 +1308,7 @@ func (suite *DetailsUnitSuite) TestLocationIDer_FromEntry() { ItemType: ExchangeMail, }, }, - backupVersion: version.OneDriveXLocationRef, + backupVersion: version.OneDrive7LocationRef, expectedErr: require.Error, }, }