Switch to folder IDs in Exchange (#3373)

Store all folders from Exchange by folder ID
in kopia. Remove the old duplicate folder
name stop-gap measure as well since it's no
longer required

Update tests to check LocationPath instead
of FullPath since LocationPath still has
display name info

---

#### Does this PR need a docs update or release note?

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

#### Type of change

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

#### Issue(s)

* closes #3197

#### Test Plan

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

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
 

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)
 			}
 

src/internal/connector/exchange/service_functions.go

@@ -137,21 +137,15 @@ func includeContainer(
 		directory = locPath.Folder(false)
 	}
 
-	var (
+	var ok bool
-		ok      bool
-		pathRes path.Path
-	)
 
 	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
 }

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")

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 {
+	idPath1 := func(t *testing.T, cat path.CategoryType) path.Path {
-		res, err := location.ToDataLayerPath(
+		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",
+						delta: "delta_url",
-						path:  oldPath1(t, cat).String(),
+						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,
+			expectNewColls: 2,
-			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(),
 					},
 				}
 			},
@@ -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,253 +635,7 @@ 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")
-
-						if exColl.LocationPath() != nil {
-							assert.Equal(t, location.String(), exColl.LocationPath().String())
-						}
-
-						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, test.expectAdded, ids[0], "added items")
-						assert.ElementsMatch(t, test.expectRemoved, 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
 						}
 
@@ -935,6 +675,8 @@ func (suite *ServiceIteratorsSuite) TestFilterContainersAndFillCollections_Dupli
 					}
 				})
 			}
+		})
+	}
 }
 
 func (suite *ServiceIteratorsSuite) TestFilterContainersAndFillCollections_repeatedItems() {

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")

src/internal/operations/backup_integration_test.go

@@ -1112,7 +1112,9 @@ func (suite *BackupOpIntegrationSuite) TestBackup_Run_incrementalExchange() {
 				}
 			},
 			itemsRead: 0, // containers are not counted as reads
-			itemsWritten: 4, // two items per category
+			// Renaming a folder doesn't cause kopia changes as the folder ID doesn't
+			// change.
+			itemsWritten: 0,
 		},
 		{
 			name: "add a new item",