From dd19b484c8f89461354485826633a1f21b6c6b7f Mon Sep 17 00:00:00 2001 From: Abhishek Pandey Date: Thu, 15 Jun 2023 00:48:03 -0700 Subject: [PATCH] Add more tests for url cache (#3593) PR contents 1. Address a corner case where cache may be half filled ( e.g. scenario: delta query failing after a few pages). Empty the cache on any delta failures 2. Add unit tests --- #### 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 - [x] :robot: Supportability/Tests - [ ] :computer: CI/Deployment - [ ] :broom: Tech Debt/Cleanup #### Issue(s) * # #### Test Plan - [ ] :muscle: Manual - [x] :zap: Unit test - [ ] :green_heart: E2E --- .../m365/onedrive/collections_test.go | 16 + src/internal/m365/onedrive/url_cache.go | 25 +- src/internal/m365/onedrive/url_cache_test.go | 374 ++++++++++++++++++ 3 files changed, 405 insertions(+), 10 deletions(-) diff --git a/src/internal/m365/onedrive/collections_test.go b/src/internal/m365/onedrive/collections_test.go index 5a5805179..d18ad0f4f 100644 --- a/src/internal/m365/onedrive/collections_test.go +++ b/src/internal/m365/onedrive/collections_test.go @@ -2494,6 +2494,22 @@ func driveItem( return coreItem(id, name, parentPath, parentID, isFile, isFolder, isPackage) } +func fileItem( + id, name, parentPath, parentID, url string, + deleted bool, +) models.DriveItemable { + di := driveItem(id, name, parentPath, parentID, true, false, false) + di.SetAdditionalData(map[string]interface{}{ + "@microsoft.graph.downloadUrl": url, + }) + + if deleted { + di.SetDeleted(models.NewDeleted()) + } + + return di +} + func malwareItem( id string, name string, diff --git a/src/internal/m365/onedrive/url_cache.go b/src/internal/m365/onedrive/url_cache.go index 4370136db..bb5e61b94 100644 --- a/src/internal/m365/onedrive/url_cache.go +++ b/src/internal/m365/onedrive/url_cache.go @@ -9,6 +9,7 @@ import ( "github.com/microsoftgraph/msgraph-sdk-go/models" "github.com/alcionai/corso/src/internal/common/ptr" + "github.com/alcionai/corso/src/internal/common/str" "github.com/alcionai/corso/src/pkg/fault" "github.com/alcionai/corso/src/pkg/logger" "github.com/alcionai/corso/src/pkg/services/m365/api" @@ -33,7 +34,7 @@ type urlCache struct { itemPager api.DriveItemEnumerator - errors *fault.Bus + errs *fault.Bus } // newURLache creates a new URL cache for the specified drive ID @@ -41,7 +42,7 @@ func newURLCache( driveID string, refreshInterval time.Duration, itemPager api.DriveItemEnumerator, - errors *fault.Bus, + errs *fault.Bus, ) (*urlCache, error) { err := validateCacheParams( driveID, @@ -57,7 +58,7 @@ func newURLCache( driveID: driveID, refreshInterval: refreshInterval, itemPager: itemPager, - errors: errors, + errs: errs, }, nil } @@ -72,7 +73,7 @@ func validateCacheParams( return clues.New("drive id is empty") } - if refreshInterval <= 1*time.Second { + if refreshInterval < 1*time.Second { return clues.New("invalid refresh interval") } @@ -94,7 +95,6 @@ func (uc *urlCache) getItemProperties( ctx = clues.Add(ctx, "drive_id", uc.driveID) - // Lazy refresh if uc.needsRefresh() { err := uc.refreshCache(ctx) if err != nil { @@ -146,6 +146,9 @@ func (uc *urlCache) refreshCache( err := uc.deltaQuery(ctx) if err != nil { + // clear cache + uc.idToProps = make(map[string]itemProps) + return err } @@ -171,7 +174,7 @@ func (uc *urlCache) deltaQuery( uc.updateCache, map[string]string{}, "", - uc.errors) + uc.errs) if err != nil { return clues.Wrap(err, "delta query") } @@ -224,12 +227,14 @@ func (uc *urlCache) updateCache( continue } - var url string + var ( + url string + ad = item.GetAdditionalData() + ) for _, key := range downloadURLKeys { - tmp, ok := item.GetAdditionalData()[key].(*string) - if ok { - url = ptr.Val(tmp) + if v, err := str.AnyValueToString(key, ad); err == nil { + url = v break } } diff --git a/src/internal/m365/onedrive/url_cache_test.go b/src/internal/m365/onedrive/url_cache_test.go index c3674c8c7..6a6c696b8 100644 --- a/src/internal/m365/onedrive/url_cache_test.go +++ b/src/internal/m365/onedrive/url_cache_test.go @@ -1,6 +1,8 @@ package onedrive import ( + "errors" + "math/rand" "net/http" "sync" "testing" @@ -152,3 +154,375 @@ func (suite *URLCacheIntegrationSuite) TestURLCacheBasic() { // Validate that <= 1 delta queries were made require.LessOrEqual(t, cache.deltaQueryCount, 1) } + +type URLCacheUnitSuite struct { + tester.Suite +} + +func TestURLCacheUnitSuite(t *testing.T) { + suite.Run(t, &URLCacheUnitSuite{Suite: tester.NewUnitSuite(t)}) +} + +func (suite *URLCacheUnitSuite) TestGetItemProperties() { + deltaString := "delta" + next := "next" + driveID := "drive1" + + table := []struct { + name string + pagerResult map[string][]deltaPagerResult + expectedItemProps map[string]itemProps + expectedErr require.ErrorAssertionFunc + cacheAssert func(*urlCache, time.Time) + }{ + { + name: "single item in cache", + pagerResult: map[string][]deltaPagerResult{ + driveID: { + { + items: []models.DriveItemable{ + fileItem("1", "file1", "root", "root", "https://dummy1.com", false), + }, + deltaLink: &deltaString, + }, + }, + }, + expectedItemProps: map[string]itemProps{ + "1": { + downloadURL: "https://dummy1.com", + isDeleted: false, + }, + }, + expectedErr: require.NoError, + cacheAssert: func(uc *urlCache, startTime time.Time) { + require.Greater(suite.T(), uc.lastRefreshTime, startTime) + require.Equal(suite.T(), 1, uc.deltaQueryCount) + require.Equal(suite.T(), 1, len(uc.idToProps)) + }, + }, + { + name: "multiple items in cache", + pagerResult: map[string][]deltaPagerResult{ + driveID: { + { + items: []models.DriveItemable{ + fileItem("1", "file1", "root", "root", "https://dummy1.com", false), + fileItem("2", "file2", "root", "root", "https://dummy2.com", false), + fileItem("3", "file3", "root", "root", "https://dummy3.com", false), + fileItem("4", "file4", "root", "root", "https://dummy4.com", false), + fileItem("5", "file5", "root", "root", "https://dummy5.com", false), + }, + deltaLink: &deltaString, + }, + }, + }, + expectedItemProps: map[string]itemProps{ + "1": { + downloadURL: "https://dummy1.com", + isDeleted: false, + }, + "2": { + downloadURL: "https://dummy2.com", + isDeleted: false, + }, + "3": { + downloadURL: "https://dummy3.com", + isDeleted: false, + }, + "4": { + downloadURL: "https://dummy4.com", + isDeleted: false, + }, + "5": { + downloadURL: "https://dummy5.com", + isDeleted: false, + }, + }, + expectedErr: require.NoError, + cacheAssert: func(uc *urlCache, startTime time.Time) { + require.Greater(suite.T(), uc.lastRefreshTime, startTime) + require.Equal(suite.T(), 1, uc.deltaQueryCount) + require.Equal(suite.T(), 5, len(uc.idToProps)) + }, + }, + { + name: "duplicate items with potentially new urls", + pagerResult: map[string][]deltaPagerResult{ + driveID: { + { + items: []models.DriveItemable{ + fileItem("1", "file1", "root", "root", "https://dummy1.com", false), + fileItem("2", "file2", "root", "root", "https://dummy2.com", false), + fileItem("3", "file3", "root", "root", "https://dummy3.com", false), + fileItem("1", "file1", "root", "root", "https://test1.com", false), + fileItem("2", "file2", "root", "root", "https://test2.com", false), + }, + deltaLink: &deltaString, + }, + }, + }, + expectedItemProps: map[string]itemProps{ + "1": { + downloadURL: "https://test1.com", + isDeleted: false, + }, + "2": { + downloadURL: "https://test2.com", + isDeleted: false, + }, + "3": { + downloadURL: "https://dummy3.com", + isDeleted: false, + }, + }, + expectedErr: require.NoError, + cacheAssert: func(uc *urlCache, startTime time.Time) { + require.Greater(suite.T(), uc.lastRefreshTime, startTime) + require.Equal(suite.T(), 1, uc.deltaQueryCount) + require.Equal(suite.T(), 3, len(uc.idToProps)) + }, + }, + { + name: "deleted items", + pagerResult: map[string][]deltaPagerResult{ + driveID: { + { + items: []models.DriveItemable{ + fileItem("1", "file1", "root", "root", "https://dummy1.com", false), + fileItem("2", "file2", "root", "root", "https://dummy2.com", false), + fileItem("1", "file1", "root", "root", "https://dummy1.com", true), + }, + deltaLink: &deltaString, + }, + }, + }, + expectedItemProps: map[string]itemProps{ + "1": { + downloadURL: "", + isDeleted: true, + }, + "2": { + downloadURL: "https://dummy2.com", + isDeleted: false, + }, + }, + expectedErr: require.NoError, + cacheAssert: func(uc *urlCache, startTime time.Time) { + require.Greater(suite.T(), uc.lastRefreshTime, startTime) + require.Equal(suite.T(), 1, uc.deltaQueryCount) + require.Equal(suite.T(), 2, len(uc.idToProps)) + }, + }, + { + name: "item not found in cache", + pagerResult: map[string][]deltaPagerResult{ + driveID: { + { + items: []models.DriveItemable{ + fileItem("1", "file1", "root", "root", "https://dummy1.com", false), + }, + deltaLink: &deltaString, + }, + }, + }, + expectedItemProps: map[string]itemProps{ + "2": {}, + }, + expectedErr: require.Error, + cacheAssert: func(uc *urlCache, startTime time.Time) { + require.Greater(suite.T(), uc.lastRefreshTime, startTime) + require.Equal(suite.T(), 1, uc.deltaQueryCount) + require.Equal(suite.T(), 1, len(uc.idToProps)) + }, + }, + { + name: "multi-page delta query error", + pagerResult: map[string][]deltaPagerResult{ + driveID: { + { + items: []models.DriveItemable{ + fileItem("1", "file1", "root", "root", "https://dummy1.com", false), + }, + nextLink: &next, + }, + { + items: []models.DriveItemable{ + fileItem("2", "file2", "root", "root", "https://dummy2.com", false), + }, + deltaLink: &deltaString, + err: errors.New("delta query error"), + }, + }, + }, + expectedItemProps: map[string]itemProps{ + "1": {}, + "2": {}, + }, + expectedErr: require.Error, + cacheAssert: func(uc *urlCache, _ time.Time) { + require.Equal(suite.T(), time.Time{}, uc.lastRefreshTime) + require.Equal(suite.T(), 0, uc.deltaQueryCount) + require.Equal(suite.T(), 0, len(uc.idToProps)) + }, + }, + + { + name: "folder item", + pagerResult: map[string][]deltaPagerResult{ + driveID: { + { + items: []models.DriveItemable{ + fileItem("1", "file1", "root", "root", "https://dummy1.com", false), + driveItem("2", "folder2", "root", "root", false, true, false), + }, + deltaLink: &deltaString, + }, + }, + }, + expectedItemProps: map[string]itemProps{ + "2": {}, + }, + expectedErr: require.Error, + cacheAssert: func(uc *urlCache, startTime time.Time) { + require.Greater(suite.T(), uc.lastRefreshTime, startTime) + require.Equal(suite.T(), 1, uc.deltaQueryCount) + require.Equal(suite.T(), 1, len(uc.idToProps)) + }, + }, + } + + for _, test := range table { + suite.Run(test.name, func() { + t := suite.T() + ctx, flush := tester.NewContext(t) + defer flush() + + itemPager := &mockItemPager{ + toReturn: test.pagerResult[driveID], + } + + cache, err := newURLCache( + driveID, + 1*time.Hour, + itemPager, + fault.New(true)) + + require.NoError(suite.T(), err, clues.ToCore(err)) + + numConcurrentReq := 100 + var wg sync.WaitGroup + wg.Add(numConcurrentReq) + + startTime := time.Now() + + for i := 0; i < numConcurrentReq; i++ { + go func() { + defer wg.Done() + + for id, expected := range test.expectedItemProps { + time.Sleep(time.Duration(rand.Intn(100)) * time.Millisecond) + + props, err := cache.getItemProperties(ctx, id) + + test.expectedErr(suite.T(), err, clues.ToCore(err)) + require.Equal(suite.T(), expected, props) + } + }() + } + + wg.Wait() + + test.cacheAssert(cache, startTime) + }) + } +} + +// Test needsRefresh +func (suite *URLCacheUnitSuite) TestNeedsRefresh() { + driveID := "drive1" + t := suite.T() + refreshInterval := 1 * time.Second + + cache, err := newURLCache( + driveID, + refreshInterval, + &mockItemPager{}, + fault.New(true)) + + require.NoError(t, err, clues.ToCore(err)) + + // cache is empty + require.True(t, cache.needsRefresh()) + + // cache is not empty, but refresh interval has passed + cache.idToProps["1"] = itemProps{ + downloadURL: "https://dummy1.com", + isDeleted: false, + } + + time.Sleep(refreshInterval) + require.True(t, cache.needsRefresh()) + + // none of the above + cache.lastRefreshTime = time.Now() + require.False(t, cache.needsRefresh()) +} + +// Test newURLCache +func (suite *URLCacheUnitSuite) TestNewURLCache() { + // table driven tests + table := []struct { + name string + driveID string + refreshInt time.Duration + itemPager api.DriveItemEnumerator + errors *fault.Bus + expectedErr require.ErrorAssertionFunc + }{ + { + name: "invalid driveID", + driveID: "", + refreshInt: 1 * time.Hour, + itemPager: &mockItemPager{}, + errors: fault.New(true), + expectedErr: require.Error, + }, + { + name: "invalid refresh interval", + driveID: "drive1", + refreshInt: 100 * time.Millisecond, + itemPager: &mockItemPager{}, + errors: fault.New(true), + expectedErr: require.Error, + }, + { + name: "invalid itemPager", + driveID: "drive1", + refreshInt: 1 * time.Hour, + itemPager: nil, + errors: fault.New(true), + expectedErr: require.Error, + }, + { + name: "valid", + driveID: "drive1", + refreshInt: 1 * time.Hour, + itemPager: &mockItemPager{}, + errors: fault.New(true), + expectedErr: require.NoError, + }, + } + + for _, test := range table { + suite.Run(test.name, func() { + t := suite.T() + _, err := newURLCache( + test.driveID, + test.refreshInt, + test.itemPager, + test.errors) + + test.expectedErr(t, err, clues.ToCore(err)) + }) + } +}