diff --git a/CHANGELOG.md b/CHANGELOG.md index 3d49a12c8..3140e6ea1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - OneDrive backups no longer include a user's non-default drives. - OneDrive and SharePoint file downloads will properly redirect from 3xx responses. - Refined oneDrive rate limiter controls to reduce throttling errors. +- Fix handling of duplicate folders at the same hierarchy level in Exchange. Duplicate folders will be merged during restore operations. + +### Known Issues +- Restore operations will merge duplicate Exchange folders at the same hierarchy level into a single folder. ## [v0.7.0] (beta) - 2023-05-02 diff --git a/src/internal/connector/exchange/data_collections_test.go b/src/internal/connector/exchange/data_collections_test.go index e2c460cb8..2c23747df 100644 --- a/src/internal/connector/exchange/data_collections_test.go +++ b/src/internal/connector/exchange/data_collections_test.go @@ -282,9 +282,18 @@ func (suite *DataCollectionsIntegrationSuite) TestMailFetch() { } require.NotEmpty(t, c.FullPath().Folder(false)) - folder := c.FullPath().Folder(false) - delete(test.folderNames, folder) + // TODO(ashmrtn): Remove when LocationPath is made part of BackupCollection + // interface. + if !assert.Implements(t, (*data.LocationPather)(nil), c) { + continue + } + + loc := c.(data.LocationPather).LocationPath().String() + + require.NotEmpty(t, loc) + + delete(test.folderNames, loc) } assert.Empty(t, test.folderNames) @@ -525,7 +534,16 @@ func (suite *DataCollectionsIntegrationSuite) TestContactSerializationRegression continue } - assert.Equal(t, edc.FullPath().Folder(false), DefaultContactFolder) + // TODO(ashmrtn): Remove when LocationPath is made part of BackupCollection + // interface. + if !assert.Implements(t, (*data.LocationPather)(nil), edc) { + continue + } + + assert.Equal( + t, + edc.(data.LocationPather).LocationPath().String(), + DefaultContactFolder) assert.NotZero(t, count) } diff --git a/src/internal/connector/exchange/service_functions.go b/src/internal/connector/exchange/service_functions.go index cad25cdd8..52d46ba42 100644 --- a/src/internal/connector/exchange/service_functions.go +++ b/src/internal/connector/exchange/service_functions.go @@ -137,21 +137,15 @@ func includeContainer( directory = locPath.Folder(false) } - var ( - ok bool - pathRes path.Path - ) + var ok bool switch category { case path.EmailCategory: ok = scope.Matches(selectors.ExchangeMailFolder, directory) - pathRes = locPath case path.ContactsCategory: ok = scope.Matches(selectors.ExchangeContactFolder, directory) - pathRes = locPath case path.EventsCategory: ok = scope.Matches(selectors.ExchangeEventCalendar, directory) - pathRes = dirPath default: return nil, nil, false } @@ -162,5 +156,5 @@ func includeContainer( "matches_input", directory, ).Debug("backup folder selection filter") - return pathRes, loc, ok + return dirPath, loc, ok } diff --git a/src/internal/connector/exchange/service_iterators.go b/src/internal/connector/exchange/service_iterators.go index 34ea37d3f..9f707df21 100644 --- a/src/internal/connector/exchange/service_iterators.go +++ b/src/internal/connector/exchange/service_iterators.go @@ -56,10 +56,6 @@ func filterContainersAndFillCollections( // deleted from this map, leaving only the deleted folders behind tombstones = makeTombstones(dps) category = qp.Category - - // Stop-gap: Track folders by LocationPath and if there's duplicates pick - // the one with the lexicographically larger ID. - dupPaths = map[string]string{} ) logger.Ctx(ctx).Infow("filling collections", "len_deltapaths", len(dps)) @@ -108,53 +104,6 @@ func filterContainersAndFillCollections( continue } - // This is a duplicate collection. Either the collection we're examining now - // should be skipped or the collection we previously added should be - // skipped. - // - // Calendars is already using folder IDs so we don't need to pick the - // "newest" folder for that. - if oldCID := dupPaths[locPath.String()]; category != path.EventsCategory && len(oldCID) > 0 { - if cID < oldCID { - logger.Ctx(ictx).Infow( - "skipping duplicate folder with lesser ID", - "previous_folder_id", clues.Hide(oldCID), - "current_folder_id", clues.Hide(cID), - "duplicate_path", locPath) - - // Readd this entry to the tombstone map because we remove it first off. - if oldDP, ok := dps[cID]; ok { - tombstones[cID] = oldDP.path - } - - // Continuing here ensures we don't add anything to the paths map or the - // delta map which is the behavior we want. - continue - } - - logger.Ctx(ictx).Infow( - "switching duplicate folders as newer folder found", - "previous_folder_id", clues.Hide(oldCID), - "current_folder_id", clues.Hide(cID), - "duplicate_path", locPath) - - // Remove the previous collection from the maps. This will make us think - // it's a new item and properly populate it if it ever: - // * moves - // * replaces the current entry (current entry moves/is deleted) - delete(collections, oldCID) - delete(deltaURLs, oldCID) - delete(currPaths, oldCID) - - // Re-add the tombstone entry for the old folder so that it can be marked - // as deleted if need. - if oldDP, ok := dps[oldCID]; ok { - tombstones[oldCID] = oldDP.path - } - } - - dupPaths[locPath.String()] = cID - if len(prevPathStr) > 0 { if prevPath, err = pathFromPrevString(prevPathStr); err != nil { logger.CtxErr(ictx, err).Error("parsing prev path") diff --git a/src/internal/connector/exchange/service_iterators_test.go b/src/internal/connector/exchange/service_iterators_test.go index d7a355122..5b4d11940 100644 --- a/src/internal/connector/exchange/service_iterators_test.go +++ b/src/internal/connector/exchange/service_iterators_test.go @@ -384,6 +384,7 @@ func (suite *ServiceIteratorsSuite) TestFilterContainersAndFillCollections_Dupli ResourceOwner: inMock.NewProvider("user_id", "user_name"), Credentials: suite.creds, } + statusUpdater = func(*support.ConnectorOperationStatus) {} dataTypes = []scopeCat{ @@ -395,6 +396,10 @@ func (suite *ServiceIteratorsSuite) TestFilterContainersAndFillCollections_Dupli scope: selectors.NewExchangeBackup(nil).ContactFolders(selectors.Any())[0], cat: path.ContactsCategory, }, + { + scope: selectors.NewExchangeBackup(nil).EventCalendars(selectors.Any())[0], + cat: path.EventsCategory, + }, } location = path.Builder{}.Append("foo", "bar") @@ -448,8 +453,20 @@ func (suite *ServiceIteratorsSuite) TestFilterContainersAndFillCollections_Dupli return res } - locPath := func(t *testing.T, cat path.CategoryType) path.Path { - res, err := location.ToDataLayerPath( + idPath1 := func(t *testing.T, cat path.CategoryType) path.Path { + res, err := path.Builder{}.Append("1").ToDataLayerPath( + suite.creds.AzureTenantID, + qp.ResourceOwner.ID(), + path.ExchangeService, + cat, + false) + require.NoError(t, err, clues.ToCore(err)) + + return res + } + + idPath2 := func(t *testing.T, cat path.CategoryType) path.Path { + res, err := path.Builder{}.Append("2").ToDataLayerPath( suite.creds.AzureTenantID, qp.ResourceOwner.ID(), path.ExchangeService, @@ -467,8 +484,6 @@ func (suite *ServiceIteratorsSuite) TestFilterContainersAndFillCollections_Dupli inputMetadata func(t *testing.T, cat path.CategoryType) DeltaPaths expectNewColls int expectDeleted int - expectAdded []string - expectRemoved []string expectMetadata func(t *testing.T, cat path.CategoryType) DeltaPaths }{ { @@ -486,49 +501,19 @@ func (suite *ServiceIteratorsSuite) TestFilterContainersAndFillCollections_Dupli }, "2": DeltaPath{ delta: "old_delta", - path: locPath(t, cat).String(), + path: idPath2(t, cat).String(), }, } }, - expectDeleted: 1, - expectAdded: result2.added, - expectRemoved: result2.removed, expectMetadata: func(t *testing.T, cat path.CategoryType) DeltaPaths { - return DeltaPaths{ - "2": DeltaPath{ - delta: "delta_url2", - path: locPath(t, cat).String(), - }, - } - }, - }, - { - name: "1 moved to duplicate, other order", - getter: map[string]mockGetterResults{ - "1": result1, - "2": result2, - }, - resolver: newMockResolver(container2, container1), - inputMetadata: func(t *testing.T, cat path.CategoryType) DeltaPaths { return DeltaPaths{ "1": DeltaPath{ - delta: "old_delta", - path: oldPath1(t, cat).String(), + delta: "delta_url", + path: idPath1(t, cat).String(), }, - "2": DeltaPath{ - delta: "old_delta", - path: locPath(t, cat).String(), - }, - } - }, - expectDeleted: 1, - expectAdded: result2.added, - expectRemoved: result2.removed, - expectMetadata: func(t *testing.T, cat path.CategoryType) DeltaPaths { - return DeltaPaths{ "2": DeltaPath{ delta: "delta_url2", - path: locPath(t, cat).String(), + path: idPath2(t, cat).String(), }, } }, @@ -552,14 +537,15 @@ func (suite *ServiceIteratorsSuite) TestFilterContainersAndFillCollections_Dupli }, } }, - expectDeleted: 1, - expectAdded: result2.added, - expectRemoved: result2.removed, expectMetadata: func(t *testing.T, cat path.CategoryType) DeltaPaths { return DeltaPaths{ + "1": DeltaPath{ + delta: "delta_url", + path: idPath1(t, cat).String(), + }, "2": DeltaPath{ delta: "delta_url2", - path: locPath(t, cat).String(), + path: idPath2(t, cat).String(), }, } }, @@ -574,14 +560,16 @@ func (suite *ServiceIteratorsSuite) TestFilterContainersAndFillCollections_Dupli inputMetadata: func(t *testing.T, cat path.CategoryType) DeltaPaths { return DeltaPaths{} }, - expectNewColls: 1, - expectAdded: result2.added, - expectRemoved: result2.removed, + expectNewColls: 2, expectMetadata: func(t *testing.T, cat path.CategoryType) DeltaPaths { return DeltaPaths{ + "1": DeltaPath{ + delta: "delta_url", + path: idPath1(t, cat).String(), + }, "2": DeltaPath{ delta: "delta_url2", - path: locPath(t, cat).String(), + path: idPath2(t, cat).String(), }, } }, @@ -596,19 +584,17 @@ func (suite *ServiceIteratorsSuite) TestFilterContainersAndFillCollections_Dupli return DeltaPaths{ "2": DeltaPath{ delta: "old_delta", - path: locPath(t, cat).String(), + path: idPath2(t, cat).String(), }, } }, expectNewColls: 1, expectDeleted: 1, - expectAdded: result1.added, - expectRemoved: result1.removed, expectMetadata: func(t *testing.T, cat path.CategoryType) DeltaPaths { return DeltaPaths{ "1": DeltaPath{ delta: "delta_url", - path: locPath(t, cat).String(), + path: idPath1(t, cat).String(), }, } }, @@ -633,7 +619,7 @@ func (suite *ServiceIteratorsSuite) TestFilterContainersAndFillCollections_Dupli statusUpdater, test.resolver, sc.scope, - test.inputMetadata(t, sc.cat), + test.inputMetadata(t, qp.Category), control.Options{FailureHandling: control.FailFast}, fault.New(true)) require.NoError(t, err, "getting collections", clues.ToCore(err)) @@ -649,21 +635,30 @@ func (suite *ServiceIteratorsSuite) TestFilterContainersAndFillCollections_Dupli if c.FullPath().Service() == path.ExchangeMetadataService { metadatas++ - checkMetadata(t, ctx, sc.cat, test.expectMetadata(t, sc.cat), c) + checkMetadata(t, ctx, qp.Category, test.expectMetadata(t, qp.Category), c) continue } if c.State() == data.NewState { news++ } + } - exColl, ok := c.(*Collection) - require.True(t, ok, "collection is an *exchange.Collection") + assert.Equal(t, test.expectDeleted, deleteds, "deleted collections") + assert.Equal(t, test.expectNewColls, news, "new collections") + assert.Equal(t, 1, metadatas, "metadata collections") - if exColl.LocationPath() != nil { - assert.Equal(t, location.String(), exColl.LocationPath().String()) + // items in collections assertions + for k, expect := range test.getter { + coll := collections[k] + + if coll == nil { + continue } + exColl, ok := coll.(*Collection) + require.True(t, ok, "collection is an *exchange.Collection") + ids := [][]string{ make([]string, 0, len(exColl.added)), make([]string, 0, len(exColl.removed)), @@ -675,268 +670,15 @@ func (suite *ServiceIteratorsSuite) TestFilterContainersAndFillCollections_Dupli } } - assert.ElementsMatch(t, test.expectAdded, ids[0], "added items") - assert.ElementsMatch(t, test.expectRemoved, ids[1], "removed items") + assert.ElementsMatch(t, expect.added, ids[0], "added items") + assert.ElementsMatch(t, expect.removed, ids[1], "removed items") } - - assert.Equal(t, test.expectDeleted, deleteds, "deleted collections") - assert.Equal(t, test.expectNewColls, news, "new collections") - assert.Equal(t, 1, metadatas, "metadata collections") }) } }) } } -func (suite *ServiceIteratorsSuite) TestFilterContainersAndFillCollections_DuplicateFolders_Events() { - var ( - qp = graph.QueryParams{ - ResourceOwner: inMock.NewProvider("user_id", "user_name"), - Category: path.EventsCategory, - Credentials: suite.creds, - } - statusUpdater = func(*support.ConnectorOperationStatus) {} - - scope = selectors.NewExchangeBackup(nil).EventCalendars(selectors.Any())[0] - - location = path.Builder{}.Append("foo", "bar") - - result1 = mockGetterResults{ - added: []string{"a1", "a2", "a3"}, - removed: []string{"r1", "r2", "r3"}, - newDelta: api.DeltaUpdate{URL: "delta_url"}, - } - result2 = mockGetterResults{ - added: []string{"a4", "a5", "a6"}, - removed: []string{"r4", "r5", "r6"}, - newDelta: api.DeltaUpdate{URL: "delta_url2"}, - } - - container1 = mockContainer{ - id: strPtr("1"), - displayName: strPtr("bar"), - p: path.Builder{}.Append("1"), - l: location, - } - container2 = mockContainer{ - id: strPtr("2"), - displayName: strPtr("bar"), - p: path.Builder{}.Append("2"), - l: location, - } - ) - - oldPath1, err := location.Append("1").ToDataLayerPath( - suite.creds.AzureTenantID, - qp.ResourceOwner.ID(), - path.ExchangeService, - qp.Category, - false) - require.NoError(suite.T(), err, clues.ToCore(err)) - - oldPath2, err := location.Append("2").ToDataLayerPath( - suite.creds.AzureTenantID, - qp.ResourceOwner.ID(), - path.ExchangeService, - qp.Category, - false) - require.NoError(suite.T(), err, clues.ToCore(err)) - - idPath1, err := path.Builder{}.Append("1").ToDataLayerPath( - suite.creds.AzureTenantID, - qp.ResourceOwner.ID(), - path.ExchangeService, - qp.Category, - false) - require.NoError(suite.T(), err, clues.ToCore(err)) - - idPath2, err := path.Builder{}.Append("2").ToDataLayerPath( - suite.creds.AzureTenantID, - qp.ResourceOwner.ID(), - path.ExchangeService, - qp.Category, - false) - require.NoError(suite.T(), err, clues.ToCore(err)) - - table := []struct { - name string - getter mockGetter - resolver graph.ContainerResolver - inputMetadata DeltaPaths - expectNewColls int - expectDeleted int - expectMetadata DeltaPaths - }{ - { - name: "1 moved to duplicate", - getter: map[string]mockGetterResults{ - "1": result1, - "2": result2, - }, - resolver: newMockResolver(container1, container2), - inputMetadata: DeltaPaths{ - "1": DeltaPath{ - delta: "old_delta", - path: oldPath1.String(), - }, - "2": DeltaPath{ - delta: "old_delta", - path: idPath2.String(), - }, - }, - expectMetadata: DeltaPaths{ - "1": DeltaPath{ - delta: "delta_url", - path: idPath1.String(), - }, - "2": DeltaPath{ - delta: "delta_url2", - path: idPath2.String(), - }, - }, - }, - { - name: "both move to duplicate", - getter: map[string]mockGetterResults{ - "1": result1, - "2": result2, - }, - resolver: newMockResolver(container1, container2), - inputMetadata: DeltaPaths{ - "1": DeltaPath{ - delta: "old_delta", - path: oldPath1.String(), - }, - "2": DeltaPath{ - delta: "old_delta", - path: oldPath2.String(), - }, - }, - expectMetadata: DeltaPaths{ - "1": DeltaPath{ - delta: "delta_url", - path: idPath1.String(), - }, - "2": DeltaPath{ - delta: "delta_url2", - path: idPath2.String(), - }, - }, - }, - { - name: "both new", - getter: map[string]mockGetterResults{ - "1": result1, - "2": result2, - }, - resolver: newMockResolver(container1, container2), - inputMetadata: DeltaPaths{}, - expectNewColls: 2, - expectMetadata: DeltaPaths{ - "1": DeltaPath{ - delta: "delta_url", - path: idPath1.String(), - }, - "2": DeltaPath{ - delta: "delta_url2", - path: idPath2.String(), - }, - }, - }, - { - name: "add 1 remove 2", - getter: map[string]mockGetterResults{ - "1": result1, - }, - resolver: newMockResolver(container1), - inputMetadata: DeltaPaths{ - "2": DeltaPath{ - delta: "old_delta", - path: idPath2.String(), - }, - }, - expectNewColls: 1, - expectDeleted: 1, - expectMetadata: DeltaPaths{ - "1": DeltaPath{ - delta: "delta_url", - path: idPath1.String(), - }, - }, - }, - } - for _, test := range table { - suite.Run(test.name, func() { - t := suite.T() - - ctx, flush := tester.NewContext() - defer flush() - - collections, err := filterContainersAndFillCollections( - ctx, - qp, - test.getter, - statusUpdater, - test.resolver, - scope, - test.inputMetadata, - control.Options{FailureHandling: control.FailFast}, - fault.New(true)) - require.NoError(t, err, "getting collections", clues.ToCore(err)) - - // collection assertions - - deleteds, news, metadatas := 0, 0, 0 - for _, c := range collections { - if c.State() == data.DeletedState { - deleteds++ - continue - } - - if c.FullPath().Service() == path.ExchangeMetadataService { - metadatas++ - checkMetadata(t, ctx, qp.Category, test.expectMetadata, c) - continue - } - - if c.State() == data.NewState { - news++ - } - } - - assert.Equal(t, test.expectDeleted, deleteds, "deleted collections") - assert.Equal(t, test.expectNewColls, news, "new collections") - assert.Equal(t, 1, metadatas, "metadata collections") - - // items in collections assertions - for k, expect := range test.getter { - coll := collections[k] - - if coll == nil { - continue - } - - exColl, ok := coll.(*Collection) - require.True(t, ok, "collection is an *exchange.Collection") - - ids := [][]string{ - make([]string, 0, len(exColl.added)), - make([]string, 0, len(exColl.removed)), - } - - for i, cIDs := range []map[string]struct{}{exColl.added, exColl.removed} { - for id := range cIDs { - ids[i] = append(ids[i], id) - } - } - - assert.ElementsMatch(t, expect.added, ids[0], "added items") - assert.ElementsMatch(t, expect.removed, ids[1], "removed items") - } - }) - } -} - func (suite *ServiceIteratorsSuite) TestFilterContainersAndFillCollections_repeatedItems() { newDelta := api.DeltaUpdate{URL: "delta_url"} diff --git a/src/internal/connector/graph_connector_helper_test.go b/src/internal/connector/graph_connector_helper_test.go index 99043e5bc..3aa309f04 100644 --- a/src/internal/connector/graph_connector_helper_test.go +++ b/src/internal/connector/graph_connector_helper_test.go @@ -918,7 +918,36 @@ func checkHasCollections( } for _, g := range got { - gotNames = append(gotNames, g.FullPath().String()) + // TODO(ashmrtn): Remove when LocationPath is made part of BackupCollection + // interface. + if !assert.Implements(t, (*data.LocationPather)(nil), g) { + continue + } + + fp := g.FullPath() + loc := g.(data.LocationPather).LocationPath() + + if fp.Service() == path.OneDriveService || + (fp.Service() == path.SharePointService && fp.Category() == path.LibrariesCategory) { + dp, err := path.ToDrivePath(fp) + if !assert.NoError(t, err, clues.ToCore(err)) { + continue + } + + loc = path.BuildDriveLocation(dp.DriveID, loc.Elements()...) + } + + p, err := loc.ToDataLayerPath( + fp.Tenant(), + fp.ResourceOwner(), + fp.Service(), + fp.Category(), + false) + if !assert.NoError(t, err, clues.ToCore(err)) { + continue + } + + gotNames = append(gotNames, p.String()) } assert.ElementsMatch(t, expectedNames, gotNames, "returned collections") diff --git a/src/internal/operations/backup_integration_test.go b/src/internal/operations/backup_integration_test.go index 310036a64..ddc59e6ce 100644 --- a/src/internal/operations/backup_integration_test.go +++ b/src/internal/operations/backup_integration_test.go @@ -1111,8 +1111,10 @@ func (suite *BackupOpIntegrationSuite) TestBackup_Run_incrementalExchange() { } } }, - itemsRead: 0, // containers are not counted as reads - itemsWritten: 4, // two items per category + itemsRead: 0, // containers are not counted as reads + // Renaming a folder doesn't cause kopia changes as the folder ID doesn't + // change. + itemsWritten: 0, }, { name: "add a new item",