From f5365f19c5863a20fcf5c5cabb38d70b5cbf9605 Mon Sep 17 00:00:00 2001 From: ashmrtn Date: Fri, 5 May 2023 14:00:16 -0700 Subject: [PATCH] Return map instead of using in-out param (#3308) Return type should really be switched to []data.BackupCollection but that's a non-trivial change to make since tests rely on being able to lookup things by folder ID. --- #### 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 - [ ] :sunflower: Feature - [ ] :bug: Bugfix - [ ] :world_map: Documentation - [ ] :robot: Supportability/Tests - [ ] :computer: CI/Deployment - [x] :broom: Tech Debt/Cleanup #### Test Plan - [ ] :muscle: Manual - [x] :zap: Unit test - [ ] :green_heart: E2E --- .../connector/exchange/data_collections.go | 6 +---- .../connector/exchange/service_iterators.go | 21 ++++++++++------ .../exchange/service_iterators_test.go | 25 ++++--------------- 3 files changed, 19 insertions(+), 33 deletions(-) diff --git a/src/internal/connector/exchange/data_collections.go b/src/internal/connector/exchange/data_collections.go index 1ff1d47c1..66c02cc9f 100644 --- a/src/internal/connector/exchange/data_collections.go +++ b/src/internal/connector/exchange/data_collections.go @@ -277,9 +277,6 @@ func createCollections( return nil, clues.Stack(err).WithClues(ctx) } - // Create collection of ExchangeDataCollection - collections := make(map[string]data.BackupCollection) - qp := graph.QueryParams{ Category: category, ResourceOwner: user, @@ -297,11 +294,10 @@ func createCollections( return nil, clues.Wrap(err, "populating container cache") } - err = filterContainersAndFillCollections( + collections, err := filterContainersAndFillCollections( ctx, qp, getter, - collections, su, resolver, scope, diff --git a/src/internal/connector/exchange/service_iterators.go b/src/internal/connector/exchange/service_iterators.go index bab7bd5a8..34ea37d3f 100644 --- a/src/internal/connector/exchange/service_iterators.go +++ b/src/internal/connector/exchange/service_iterators.go @@ -31,19 +31,24 @@ type addedAndRemovedItemIDsGetter interface { // into a BackupCollection. Messages outside of those directories are omitted. // @param collection is filled with during this function. // Supports all exchange applications: Contacts, Events, and Mail +// +// TODO(ashmrtn): This should really return []data.BackupCollection but +// unfortunately some of our tests rely on being able to lookup returned +// collections by ID and it would be non-trivial to change them. func filterContainersAndFillCollections( ctx context.Context, qp graph.QueryParams, getter addedAndRemovedItemIDsGetter, - collections map[string]data.BackupCollection, statusUpdater support.StatusUpdater, resolver graph.ContainerResolver, scope selectors.ExchangeScope, dps DeltaPaths, ctrlOpts control.Options, errs *fault.Bus, -) error { +) (map[string]data.BackupCollection, error) { var ( + // folder ID -> BackupCollection. + collections = map[string]data.BackupCollection{} // folder ID -> delta url or folder path lookups deltaURLs = map[string]string{} currPaths = map[string]string{} @@ -64,19 +69,19 @@ func filterContainersAndFillCollections( // But this will work for the short term. ac, err := api.NewClient(qp.Credentials) if err != nil { - return err + return nil, err } ibt, err := itemerByType(ac, category) if err != nil { - return err + return nil, err } el := errs.Local() for _, c := range resolver.Items() { if el.Failure() != nil { - return el.Failure() + return nil, el.Failure() } cID := ptr.Val(c.GetId()) @@ -222,7 +227,7 @@ func filterContainersAndFillCollections( // resolver (which contains all the resource owners' current containers). for id, p := range tombstones { if el.Failure() != nil { - return el.Failure() + return nil, el.Failure() } ictx := clues.Add(ctx, "tombstone_id", id) @@ -274,12 +279,12 @@ func filterContainersAndFillCollections( }, statusUpdater) if err != nil { - return clues.Wrap(err, "making metadata collection") + return nil, clues.Wrap(err, "making metadata collection") } collections["metadata"] = col - return el.Failure() + return collections, el.Failure() } // produces a set of id:path pairs from the deltapaths map. diff --git a/src/internal/connector/exchange/service_iterators_test.go b/src/internal/connector/exchange/service_iterators_test.go index 5c1e14d90..d7a355122 100644 --- a/src/internal/connector/exchange/service_iterators_test.go +++ b/src/internal/connector/exchange/service_iterators_test.go @@ -287,13 +287,10 @@ func (suite *ServiceIteratorsSuite) TestFilterContainersAndFillCollections() { ctx, flush := tester.NewContext() defer flush() - collections := map[string]data.BackupCollection{} - - err := filterContainersAndFillCollections( + collections, err := filterContainersAndFillCollections( ctx, qp, test.getter, - collections, statusUpdater, test.resolver, test.scope, @@ -629,13 +626,10 @@ func (suite *ServiceIteratorsSuite) TestFilterContainersAndFillCollections_Dupli ctx, flush := tester.NewContext() defer flush() - collections := map[string]data.BackupCollection{} - - err := filterContainersAndFillCollections( + collections, err := filterContainersAndFillCollections( ctx, qp, test.getter, - collections, statusUpdater, test.resolver, sc.scope, @@ -878,13 +872,10 @@ func (suite *ServiceIteratorsSuite) TestFilterContainersAndFillCollections_Dupli ctx, flush := tester.NewContext() defer flush() - collections := map[string]data.BackupCollection{} - - err := filterContainersAndFillCollections( + collections, err := filterContainersAndFillCollections( ctx, qp, test.getter, - collections, statusUpdater, test.resolver, scope, @@ -1033,13 +1024,10 @@ func (suite *ServiceIteratorsSuite) TestFilterContainersAndFillCollections_repea require.Equal(t, "user_id", qp.ResourceOwner.ID(), qp.ResourceOwner) require.Equal(t, "user_name", qp.ResourceOwner.Name(), qp.ResourceOwner) - collections := map[string]data.BackupCollection{} - - err := filterContainersAndFillCollections( + collections, err := filterContainersAndFillCollections( ctx, qp, test.getter, - collections, statusUpdater, resolver, allScope, @@ -1398,13 +1386,10 @@ func (suite *ServiceIteratorsSuite) TestFilterContainersAndFillCollections_incre ctx, flush := tester.NewContext() defer flush() - collections := map[string]data.BackupCollection{} - - err := filterContainersAndFillCollections( + collections, err := filterContainersAndFillCollections( ctx, qp, test.getter, - collections, statusUpdater, test.resolver, allScope,