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 <!--- Please check the type of change your PR introduces: ---> - [ ] 🌻 Feature - [x] 🐛 Bugfix - [ ] 🗺️ Documentation - [ ] 🤖 Test - [ ] 💻 CI/Deployment - [ ] 🐹 Trivial/Minor ## Issue(s) * closes #1519 merge after * #1523 ## Test Plan <!-- How will this be tested prior to merging.--> - [ ] 💪 Manual - [x] ⚡ Unit test - [ ] 💚 E2E
This commit is contained in:
parent
16e9f07d1e
commit
147f96e69b
@ -10,6 +10,11 @@ import (
|
|||||||
"github.com/alcionai/corso/src/pkg/path"
|
"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 {
|
func newContainerResolver() *containerResolver {
|
||||||
return &containerResolver{
|
return &containerResolver{
|
||||||
cache: map[string]graph.CachedContainer{},
|
cache: map[string]graph.CachedContainer{},
|
||||||
@ -24,6 +29,18 @@ func (cr *containerResolver) IDToPath(
|
|||||||
ctx context.Context,
|
ctx context.Context,
|
||||||
folderID string,
|
folderID string,
|
||||||
) (*path.Builder, error) {
|
) (*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]
|
c, ok := cr.cache[folderID]
|
||||||
if !ok {
|
if !ok {
|
||||||
return nil, errors.Errorf("folder %s not cached", folderID)
|
return nil, errors.Errorf("folder %s not cached", folderID)
|
||||||
@ -34,7 +51,7 @@ func (cr *containerResolver) IDToPath(
|
|||||||
return p, nil
|
return p, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
parentPath, err := cr.IDToPath(ctx, *c.GetParentFolderId())
|
parentPath, err := cr.idToPath(ctx, *c.GetParentFolderId(), depth+1)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, errors.Wrap(err, "retrieving parent folder")
|
return nil, errors.Wrap(err, "retrieving parent folder")
|
||||||
}
|
}
|
||||||
|
|||||||
@ -2,7 +2,6 @@ package exchange
|
|||||||
|
|
||||||
import (
|
import (
|
||||||
stdpath "path"
|
stdpath "path"
|
||||||
"strings"
|
|
||||||
"testing"
|
"testing"
|
||||||
|
|
||||||
"github.com/google/uuid"
|
"github.com/google/uuid"
|
||||||
@ -256,6 +255,34 @@ func (m *mockCachedContainer) SetPath(newPath *path.Builder) {
|
|||||||
m.p = newPath
|
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.
|
// TestConfiguredFolderCacheUnitSuite cannot run its tests in parallel.
|
||||||
type ConfiguredFolderCacheUnitSuite struct {
|
type ConfiguredFolderCacheUnitSuite struct {
|
||||||
suite.Suite
|
suite.Suite
|
||||||
@ -266,38 +293,43 @@ type ConfiguredFolderCacheUnitSuite struct {
|
|||||||
}
|
}
|
||||||
|
|
||||||
func (suite *ConfiguredFolderCacheUnitSuite) SetupTest() {
|
func (suite *ConfiguredFolderCacheUnitSuite) SetupTest() {
|
||||||
suite.allContainers = []*mockCachedContainer{}
|
suite.fc, suite.allContainers = resolverWithContainers(4)
|
||||||
|
|
||||||
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
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
func TestConfiguredFolderCacheUnitSuite(t *testing.T) {
|
func TestConfiguredFolderCacheUnitSuite(t *testing.T) {
|
||||||
suite.Run(t, new(ConfiguredFolderCacheUnitSuite))
|
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() {
|
func (suite *ConfiguredFolderCacheUnitSuite) TestPopulatePaths() {
|
||||||
ctx, flush := tester.NewContext()
|
ctx, flush := tester.NewContext()
|
||||||
defer flush()
|
defer flush()
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user