From b68336a4660296f42791825e6794cd5a23ba47e0 Mon Sep 17 00:00:00 2001 From: Abhishek Pandey Date: Tue, 20 Jun 2023 12:51:00 -0700 Subject: [PATCH] Address review feedback --- src/internal/m365/onedrive/collection.go | 33 ++++++++++--------- src/internal/m365/onedrive/collection_test.go | 16 ++------- src/internal/m365/onedrive/collections.go | 6 ++-- .../m365/onedrive/collections_test.go | 9 +++-- src/internal/m365/onedrive/url_cache.go | 4 +-- src/internal/m365/onedrive/url_cache_test.go | 3 -- 6 files changed, 28 insertions(+), 43 deletions(-) diff --git a/src/internal/m365/onedrive/collection.go b/src/internal/m365/onedrive/collection.go index b06d7da44..1da2e4673 100644 --- a/src/internal/m365/onedrive/collection.go +++ b/src/internal/m365/onedrive/collection.go @@ -85,7 +85,7 @@ type Collection struct { // should only be true if the old delta token expired doNotMergeItems bool - cache urlCacher + urlCache getItemPropertyer } func pathToLocation(p path.Path) (*path.Builder, error) { @@ -111,7 +111,7 @@ func NewCollection( ctrlOpts control.Options, colScope collectionScope, doNotMergeItems bool, - cache urlCacher, + urlCache getItemPropertyer, ) (*Collection, error) { // TODO(ashmrtn): If OneDrive switches to using folder IDs then this will need // to be changed as we won't be able to extract path information from the @@ -136,7 +136,7 @@ func NewCollection( ctrlOpts, colScope, doNotMergeItems, - cache) + urlCache) c.locPath = locPath c.prevLocPath = prevLocPath @@ -153,7 +153,7 @@ func newColl( ctrlOpts control.Options, colScope collectionScope, doNotMergeItems bool, - cache urlCacher, + urlCache getItemPropertyer, ) *Collection { c := &Collection{ handler: handler, @@ -167,7 +167,7 @@ func newColl( state: data.StateOf(prevPath, currPath), scope: colScope, doNotMergeItems: doNotMergeItems, - cache: cache, + urlCache: urlCache, } return c @@ -273,7 +273,7 @@ func (oc *Collection) getDriveItemContent( el = errs.Local() ) - itemData, err := oc.downloadContent(ctx, oc.handler, item, oc.driveID) + itemData, err := downloadContent(ctx, oc.handler, oc.urlCache, item, oc.driveID) if err != nil { if clues.HasLabel(err, graph.LabelsMalware) || (item != nil && item.GetMalware() != nil) { logger.CtxErr(ctx, err).With("skipped_reason", fault.SkipMalware).Info("item flagged as malware") @@ -323,9 +323,10 @@ type itemAndAPIGetter interface { // downloadContent attempts to fetch the item content. If the content url // is expired (ie, returns a 401), it re-fetches the item to get a new download // url and tries again. -func (oc *Collection) downloadContent( +func downloadContent( ctx context.Context, iaag itemAndAPIGetter, + uc getItemPropertyer, item models.DriveItemable, driveID string, ) (io.ReadCloser, error) { @@ -343,9 +344,9 @@ func (oc *Collection) downloadContent( // token, and that we've overrun the available window to // download the file. Get a fresh url from the cache and attempt to // download again. - content, err = oc.readFromCache(ctx, iaag, itemID) + content, err = readItemContents(ctx, iaag, uc, itemID) if err == nil { - logger.Ctx(ctx).Debug("found item in cache", itemID) + logger.Ctx(ctx).Debug("found item in url cache") return content, nil } @@ -353,7 +354,7 @@ func (oc *Collection) downloadContent( // Consider cache errors(including deleted items) as cache misses. This is // to preserve existing behavior. Fallback to refetching the item using the // API. - logger.CtxErr(ctx, err).Info("cache miss. refetching from API") + logger.CtxErr(ctx, err).Info("url cache miss: refetching from API") di, err := iaag.GetItem(ctx, driveID, ptr.Val(item.GetId())) if err != nil { @@ -368,18 +369,19 @@ func (oc *Collection) downloadContent( return content, nil } -// readFromCache fetches latest download URL from the cache and attempts to +// readItemContents fetches latest download URL from the cache and attempts to // download the file using the new URL. -func (oc *Collection) readFromCache( +func readItemContents( ctx context.Context, iaag itemAndAPIGetter, + uc getItemPropertyer, itemID string, ) (io.ReadCloser, error) { - if oc.cache == nil { - return nil, clues.New("nil cache") + if uc == nil { + return nil, clues.New("nil url cache") } - props, err := oc.cache.getItemProperties(ctx, itemID) + props, err := uc.getItemProperties(ctx, itemID) if err != nil { return nil, err } @@ -387,7 +389,6 @@ func (oc *Collection) readFromCache( // Handle newly deleted items if props.isDeleted { logger.Ctx(ctx).Info("item deleted in cache") - return nil, graph.ErrDeletedInFlight } diff --git a/src/internal/m365/onedrive/collection_test.go b/src/internal/m365/onedrive/collection_test.go index b90172ea7..4b8cc53b6 100644 --- a/src/internal/m365/onedrive/collection_test.go +++ b/src/internal/m365/onedrive/collection_test.go @@ -605,7 +605,7 @@ func (suite *GetDriveItemUnitTestSuite) TestGetDriveItem_error() { } } -var _ urlCacher = &mockURLCache{} +var _ getItemPropertyer = &mockURLCache{} type mockURLCache struct { Get func(ctx context.Context, itemID string) (itemProps, error) @@ -765,19 +765,7 @@ func (suite *GetDriveItemUnitTestSuite) TestDownloadContent() { mbh.GetResps = resps mbh.GetErrs = test.getErr - coll, err := NewCollection( - mbh, - nil, - nil, - driveID, - nil, - control.Options{ToggleFeatures: control.Toggles{}}, - CollectionScopeFolder, - true, - test.muc) - require.NoError(t, err, clues.ToCore(err)) - - r, err := coll.downloadContent(ctx, mbh, item, driveID) + r, err := downloadContent(ctx, mbh, test.muc, item, driveID) test.expect(t, r) test.expectErr(t, err, clues.ToCore(err)) }) diff --git a/src/internal/m365/onedrive/collections.go b/src/internal/m365/onedrive/collections.go index c36d89f58..2f270ad0a 100644 --- a/src/internal/m365/onedrive/collections.go +++ b/src/internal/m365/onedrive/collections.go @@ -385,7 +385,7 @@ func (c *Collections) Get( // Only create a drive cache if there are less than 300k items in the drive. if numDriveItems < urlCacheDriveItemThreshold { - logger.Ctx(ictx).Info("adding url cache for drive ", driveID) + logger.Ctx(ictx).Info("adding url cache for drive") err = c.addURLCacheToDriveCollections( ictx, @@ -457,7 +457,7 @@ func (c *Collections) Get( return collections, canUsePreviousBackup, nil } -// addURLCacheToDriveCollections adds a URL cache to all collections belonging to +// addURLCacheToDriveCollections adds an URL cache to all collections belonging to // a drive. func (c *Collections) addURLCacheToDriveCollections( ctx context.Context, @@ -476,7 +476,7 @@ func (c *Collections) addURLCacheToDriveCollections( // Set the URL cache for all collections in this drive for _, driveColls := range c.CollectionMap { for _, coll := range driveColls { - coll.cache = uc + coll.urlCache = uc } } diff --git a/src/internal/m365/onedrive/collections_test.go b/src/internal/m365/onedrive/collections_test.go index bd63e0668..f6350937a 100644 --- a/src/internal/m365/onedrive/collections_test.go +++ b/src/internal/m365/onedrive/collections_test.go @@ -2733,14 +2733,13 @@ func (suite *OneDriveCollectionsUnitSuite) TestURLCacheIntegration() { require.NoError(t, err, clues.ToCore(err)) c.CollectionMap[driveID][strconv.Itoa(i)] = coll - require.Equal(t, nil, coll.cache, "cache not nil") + require.Equal(t, nil, coll.urlCache, "cache not nil") } err := c.addURLCacheToDriveCollections( ctx, driveID, fault.New(true)) - require.NoError(t, err, clues.ToCore(err)) // Check that all collections have the same cache instance attached @@ -2748,11 +2747,11 @@ func (suite *OneDriveCollectionsUnitSuite) TestURLCacheIntegration() { var uc *urlCache for _, driveColls := range c.CollectionMap { for _, coll := range driveColls { - require.NotNil(t, coll.cache, "cache is nil") + require.NotNil(t, coll.urlCache, "cache is nil") if uc == nil { - uc = coll.cache.(*urlCache) + uc = coll.urlCache.(*urlCache) } else { - require.Equal(t, uc, coll.cache, "cache not equal") + require.Equal(t, uc, coll.urlCache, "cache not equal") } } } diff --git a/src/internal/m365/onedrive/url_cache.go b/src/internal/m365/onedrive/url_cache.go index 01a36039a..babd92c97 100644 --- a/src/internal/m365/onedrive/url_cache.go +++ b/src/internal/m365/onedrive/url_cache.go @@ -15,7 +15,7 @@ import ( "github.com/alcionai/corso/src/pkg/services/m365/api" ) -type urlCacher interface { +type getItemPropertyer interface { getItemProperties( ctx context.Context, itemID string, @@ -27,7 +27,7 @@ type itemProps struct { isDeleted bool } -var _ urlCacher = &urlCache{} +var _ getItemPropertyer = &urlCache{} // urlCache caches download URLs for drive items type urlCache struct { diff --git a/src/internal/m365/onedrive/url_cache_test.go b/src/internal/m365/onedrive/url_cache_test.go index 6e5da998c..1632c165a 100644 --- a/src/internal/m365/onedrive/url_cache_test.go +++ b/src/internal/m365/onedrive/url_cache_test.go @@ -118,9 +118,6 @@ func (suite *URLCacheIntegrationSuite) TestURLCacheBasic() { require.NoError(t, err, clues.ToCore(err)) - err = cache.refreshCache(ctx) - require.NoError(t, err, clues.ToCore(err)) - // Launch parallel requests to the cache, one per item var wg sync.WaitGroup for i := 0; i < len(items); i++ {