From 147f96e69b36879693b45b3de73629ea4dccd73c Mon Sep 17 00:00:00 2001 From: ashmrtn Date: Wed, 16 Nov 2022 12:41:39 -0800 Subject: [PATCH] Limit container resolver recursion depth (#1524) ## Description Now that the container resolver may not have all folders in-memory (we're relaxing the limits on when we return an error), add a recursion depth limit. Limit aligns with currently supported M365 service folder hierarchy depth limits Exchange limitations for [mail folders](https://learn.microsoft.com/en-us/office365/servicedescriptions/exchange-online-service-description/exchange-online-limits#mailbox-folder-limits) (search "Maximum folder hierarchy depth") [OneDrive](https://support.microsoft.com/en-us/office/restrictions-and-limitations-in-onedrive-and-sharepoint-64883a5d-228e-48f5-b3d2-eb39e07630fa) limit on path length (search "file name and path lengths") Minor refactoring of test code to make it easier to spin up tests for multiple cases ## Type of change - [ ] :sunflower: Feature - [x] :bug: Bugfix - [ ] :world_map: Documentation - [ ] :robot: Test - [ ] :computer: CI/Deployment - [ ] :hamster: Trivial/Minor ## Issue(s) * closes #1519 merge after * #1523 ## Test Plan - [ ] :muscle: Manual - [x] :zap: Unit test - [ ] :green_heart: E2E --- .../connector/exchange/container_resolver.go | 19 +++- .../exchange/container_resolver_test.go | 86 +++++++++++++------ 2 files changed, 77 insertions(+), 28 deletions(-) diff --git a/src/internal/connector/exchange/container_resolver.go b/src/internal/connector/exchange/container_resolver.go index f8b097d75..2f2d4535b 100644 --- a/src/internal/connector/exchange/container_resolver.go +++ b/src/internal/connector/exchange/container_resolver.go @@ -10,6 +10,11 @@ import ( "github.com/alcionai/corso/src/pkg/path" ) +// Exchange has a limit of 300 for folder depth. OneDrive has a limit on path +// length of 400 characters (including separators) which would be roughly 200 +// folders if each folder is only a single character. +const maxIterations = 300 + func newContainerResolver() *containerResolver { return &containerResolver{ cache: map[string]graph.CachedContainer{}, @@ -24,6 +29,18 @@ func (cr *containerResolver) IDToPath( ctx context.Context, folderID string, ) (*path.Builder, error) { + return cr.idToPath(ctx, folderID, 0) +} + +func (cr *containerResolver) idToPath( + ctx context.Context, + folderID string, + depth int, +) (*path.Builder, error) { + if depth >= maxIterations { + return nil, errors.New("path contains cycle or is too tall") + } + c, ok := cr.cache[folderID] if !ok { return nil, errors.Errorf("folder %s not cached", folderID) @@ -34,7 +51,7 @@ func (cr *containerResolver) IDToPath( return p, nil } - parentPath, err := cr.IDToPath(ctx, *c.GetParentFolderId()) + parentPath, err := cr.idToPath(ctx, *c.GetParentFolderId(), depth+1) if err != nil { return nil, errors.Wrap(err, "retrieving parent folder") } diff --git a/src/internal/connector/exchange/container_resolver_test.go b/src/internal/connector/exchange/container_resolver_test.go index 3282d2c33..defab2494 100644 --- a/src/internal/connector/exchange/container_resolver_test.go +++ b/src/internal/connector/exchange/container_resolver_test.go @@ -2,7 +2,6 @@ package exchange import ( stdpath "path" - "strings" "testing" "github.com/google/uuid" @@ -256,6 +255,34 @@ func (m *mockCachedContainer) SetPath(newPath *path.Builder) { m.p = newPath } +func resolverWithContainers(numContainers int) (*containerResolver, []*mockCachedContainer) { + containers := make([]*mockCachedContainer, 0, numContainers) + + for i := 0; i < numContainers; i++ { + containers = append(containers, newMockCachedContainer("a")) + } + + // Base case for the recursive lookup. + containers[0].p = path.Builder{}.Append(containers[0].displayName) + containers[0].expectedPath = containers[0].displayName + + for i := 1; i < len(containers); i++ { + containers[i].parentID = containers[i-1].id + containers[i].expectedPath = stdpath.Join( + containers[i-1].expectedPath, + containers[i].displayName, + ) + } + + resolver := newContainerResolver() + + for _, c := range containers { + resolver.cache[c.id] = c + } + + return resolver, containers +} + // TestConfiguredFolderCacheUnitSuite cannot run its tests in parallel. type ConfiguredFolderCacheUnitSuite struct { suite.Suite @@ -266,38 +293,43 @@ type ConfiguredFolderCacheUnitSuite struct { } func (suite *ConfiguredFolderCacheUnitSuite) SetupTest() { - suite.allContainers = []*mockCachedContainer{} - - for i := 0; i < 4; i++ { - suite.allContainers = append( - suite.allContainers, - newMockCachedContainer(strings.Repeat("sub", i)+"folder"), - ) - } - - // Base case for the recursive lookup. - suite.allContainers[0].p = path.Builder{}.Append(suite.allContainers[0].displayName) - suite.allContainers[0].expectedPath = suite.allContainers[0].displayName - - for i := 1; i < len(suite.allContainers); i++ { - suite.allContainers[i].parentID = suite.allContainers[i-1].id - suite.allContainers[i].expectedPath = stdpath.Join( - suite.allContainers[i-1].expectedPath, - suite.allContainers[i].displayName, - ) - } - - suite.fc = newContainerResolver() - - for _, c := range suite.allContainers { - suite.fc.cache[c.id] = c - } + suite.fc, suite.allContainers = resolverWithContainers(4) } func TestConfiguredFolderCacheUnitSuite(t *testing.T) { suite.Run(t, new(ConfiguredFolderCacheUnitSuite)) } +func (suite *ConfiguredFolderCacheUnitSuite) TestDepthLimit() { + ctx, flush := tester.NewContext() + defer flush() + + table := []struct { + name string + numContainers int + check assert.ErrorAssertionFunc + }{ + { + name: "AtLimit", + numContainers: maxIterations, + check: assert.NoError, + }, + { + name: "OverLimit", + numContainers: maxIterations + 1, + check: assert.Error, + }, + } + + for _, test := range table { + suite.T().Run(test.name, func(t *testing.T) { + resolver, containers := resolverWithContainers(test.numContainers) + _, err := resolver.IDToPath(ctx, containers[len(containers)-1].id) + test.check(t, err) + }) + } +} + func (suite *ConfiguredFolderCacheUnitSuite) TestPopulatePaths() { ctx, flush := tester.NewContext() defer flush()