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?

- [ ] ✅ Yes, it's included
- [ ] 🕐 Yes, but in a later PR
- [x] ⛔ No

#### Type of change

- [ ] 🌻 Feature
- [ ] 🐛 Bugfix
- [ ] 🗺️ Documentation
- [ ] 🤖 Supportability/Tests
- [ ] 💻 CI/Deployment
- [x] 🧹 Tech Debt/Cleanup

#### Test Plan

- [ ] 💪 Manual
- [x] ⚡ Unit test
- [ ] 💚 E2E

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,

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.

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{}
+			collections, err := filterContainersAndFillCollections(
-
-			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{}
+					collections, err := filterContainersAndFillCollections(
-
-					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{}
+			collections, err := filterContainersAndFillCollections(
-
-			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{}
+			collections, err := filterContainersAndFillCollections(
-
-			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{}
+			collections, err := filterContainersAndFillCollections(
-
-			err := filterContainersAndFillCollections(
 				ctx,
 				qp,
 				test.getter,
-				collections,
 				statusUpdater,
 				test.resolver,
 				allScope,