From 1fe37e4ba9b621d987c42b0b9fa7c3f76c27847c Mon Sep 17 00:00:00 2001 From: ashmrtn <3891298+ashmrtn@users.noreply.github.com> Date: Tue, 12 Sep 2023 14:36:22 -0700 Subject: [PATCH] Update item info api (#4183) Update the API for Item.Info to return an error. This can then be leveraged to add lazy readers to exchange backups See #2023 for more info on how to add lazy readers to exchange --- #### 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 - [ ] :robot: Supportability/Tests - [ ] :computer: CI/Deployment - [x] :broom: Tech Debt/Cleanup #### Issue(s) * #2023 #### Test Plan - [ ] :muscle: Manual - [x] :zap: Unit test - [x] :green_heart: E2E --- src/internal/data/collection.go | 2 +- src/internal/data/mock/collection.go | 9 ++++-- src/internal/kopia/upload.go | 12 +++----- src/internal/kopia/upload_test.go | 30 +++++++++---------- .../m365/collection/drive/collection.go | 10 +++---- .../m365/collection/drive/collection_test.go | 4 ++- .../m365/collection/exchange/collection.go | 4 +-- .../m365/collection/groups/collection.go | 4 +-- .../m365/collection/site/collection.go | 4 +-- .../m365/collection/site/collection_test.go | 10 +++++-- src/internal/m365/helper_test.go | 12 +++++--- 11 files changed, 55 insertions(+), 46 deletions(-) diff --git a/src/internal/data/collection.go b/src/internal/data/collection.go index e542b38d1..996fe7a5b 100644 --- a/src/internal/data/collection.go +++ b/src/internal/data/collection.go @@ -93,7 +93,7 @@ type PreviousLocationPather interface { // ItemInfo returns the details.ItemInfo for the item. type ItemInfo interface { - Info() details.ItemInfo + Info() (details.ItemInfo, error) } // ItemSize returns the size of the item in bytes. diff --git a/src/internal/data/mock/collection.go b/src/internal/data/mock/collection.go index bae0f75e5..7d9519dd9 100644 --- a/src/internal/data/mock/collection.go +++ b/src/internal/data/mock/collection.go @@ -17,7 +17,10 @@ import ( // Item // --------------------------------------------------------------------------- -var _ data.Item = &Item{} +var ( + _ data.Item = &Item{} + _ data.ItemInfo = &Item{} +) type Item struct { DeletedFlag bool @@ -45,8 +48,8 @@ func (s *Item) ToReader() io.ReadCloser { return s.Reader } -func (s *Item) Info() details.ItemInfo { - return s.ItemInfo +func (s *Item) Info() (details.ItemInfo, error) { + return s.ItemInfo, nil } func (s *Item) Size() int64 { diff --git a/src/internal/kopia/upload.go b/src/internal/kopia/upload.go index 85b95bae2..5a28760eb 100644 --- a/src/internal/kopia/upload.go +++ b/src/internal/kopia/upload.go @@ -132,7 +132,7 @@ func (rw *restoreStreamReader) Read(p []byte) (n int, err error) { } type itemDetails struct { - infoFunc func() (details.ItemInfo, error) + infoer data.ItemInfo repoPath path.Path prevPath path.Path locationPath *path.Builder @@ -204,7 +204,7 @@ func (cp *corsoProgress) FinishedFile(relativePath string, err error) { // These items were sourced from a base snapshot or were cached in kopia so we // never had to materialize their details in-memory. - if d.infoFunc == nil || d.cached { + if d.infoer == nil || d.cached { if d.prevPath == nil { cp.errs.AddRecoverable(ctx, clues.New("finished file sourced from previous backup with no previous path"). WithClues(ctx). @@ -230,7 +230,7 @@ func (cp *corsoProgress) FinishedFile(relativePath string, err error) { return } - info, err := d.infoFunc() + info, err := d.infoer.Info() if err != nil { cp.errs.AddRecoverable(ctx, clues.Wrap(err, "getting ItemInfo"). WithClues(ctx). @@ -412,11 +412,7 @@ func collectionEntries( // element. Add to pending set before calling the callback to avoid race // conditions when the item is completed. d := &itemDetails{ - // TODO(ashmrtn): Update API in data package to return an error and - // then remove this wrapper. - infoFunc: func() (details.ItemInfo, error) { - return ei.Info(), nil - }, + infoer: ei, repoPath: itemPath, // Also use the current path as the previous path for this item. This // is so that if the item is marked as cached and we need to merge diff --git a/src/internal/kopia/upload_test.go b/src/internal/kopia/upload_test.go index 61fcc2de7..51d8c7410 100644 --- a/src/internal/kopia/upload_test.go +++ b/src/internal/kopia/upload_test.go @@ -373,6 +373,18 @@ func (suite *CorsoProgressUnitSuite) SetupSuite() { suite.targetFileName = suite.targetFilePath.ToBuilder().Dir().String() } +var _ data.ItemInfo = &mockExchangeMailInfoer{} + +type mockExchangeMailInfoer struct{} + +func (m mockExchangeMailInfoer) Info() (details.ItemInfo, error) { + return details.ItemInfo{ + Exchange: &details.ExchangeInfo{ + ItemType: details.ExchangeMail, + }, + }, nil +} + type testInfo struct { info *itemDetails err error @@ -394,13 +406,7 @@ var finishedFileTable = []struct { return map[string]testInfo{ fname: { info: &itemDetails{ - infoFunc: func() (details.ItemInfo, error) { - return details.ItemInfo{ - Exchange: &details.ExchangeInfo{ - ItemType: details.ExchangeMail, - }, - }, nil - }, + infoer: mockExchangeMailInfoer{}, repoPath: fpath, locationPath: path.Builder{}.Append(fpath.Folders()...), }, @@ -432,13 +438,7 @@ var finishedFileTable = []struct { return map[string]testInfo{ fname: { info: &itemDetails{ - infoFunc: func() (details.ItemInfo, error) { - return details.ItemInfo{ - Exchange: &details.ExchangeInfo{ - ItemType: details.ExchangeMail, - }, - }, nil - }, + infoer: mockExchangeMailInfoer{}, repoPath: fpath, }, err: assert.AnError, @@ -530,7 +530,7 @@ func (suite *CorsoProgressUnitSuite) TestFinishedFile() { } if cachedTest.dropInfo { - v.info.infoFunc = nil + v.info.infoer = nil } } diff --git a/src/internal/m365/collection/drive/collection.go b/src/internal/m365/collection/drive/collection.go index 60614280a..4c6be1ab7 100644 --- a/src/internal/m365/collection/drive/collection.go +++ b/src/internal/m365/collection/drive/collection.go @@ -255,11 +255,11 @@ type Item struct { // Deleted implements an interface function. However, OneDrive items are marked // as deleted by adding them to the exclude list so this can always return // false. -func (i Item) Deleted() bool { return false } -func (i *Item) ID() string { return i.id } -func (i *Item) ToReader() io.ReadCloser { return i.data } -func (i *Item) Info() details.ItemInfo { return i.info } -func (i *Item) ModTime() time.Time { return i.info.Modified() } +func (i Item) Deleted() bool { return false } +func (i *Item) ID() string { return i.id } +func (i *Item) ToReader() io.ReadCloser { return i.data } +func (i *Item) Info() (details.ItemInfo, error) { return i.info, nil } +func (i *Item) ModTime() time.Time { return i.info.Modified() } // getDriveItemContent fetch drive item's contents with retries func (oc *Collection) getDriveItemContent( diff --git a/src/internal/m365/collection/drive/collection_test.go b/src/internal/m365/collection/drive/collection_test.go index ca52b70e6..2ec7ea77d 100644 --- a/src/internal/m365/collection/drive/collection_test.go +++ b/src/internal/m365/collection/drive/collection_test.go @@ -980,7 +980,9 @@ func (suite *CollectionUnitTestSuite) TestItemExtensions() { ei, ok := collItem.(data.ItemInfo) assert.True(t, ok) - itemInfo := ei.Info() + + itemInfo, err := ei.Info() + require.NoError(t, err, clues.ToCore(err)) _, err = io.ReadAll(collItem.ToReader()) test.expectReadErr(t, err, clues.ToCore(err)) diff --git a/src/internal/m365/collection/exchange/collection.go b/src/internal/m365/collection/exchange/collection.go index 3137d5b07..98cfb3e00 100644 --- a/src/internal/m365/collection/exchange/collection.go +++ b/src/internal/m365/collection/exchange/collection.go @@ -320,8 +320,8 @@ func (i Item) Deleted() bool { return i.deleted } -func (i *Item) Info() details.ItemInfo { - return details.ItemInfo{Exchange: i.info} +func (i *Item) Info() (details.ItemInfo, error) { + return details.ItemInfo{Exchange: i.info}, nil } func (i *Item) ModTime() time.Time { diff --git a/src/internal/m365/collection/groups/collection.go b/src/internal/m365/collection/groups/collection.go index 2ca70c3f0..23d0c8512 100644 --- a/src/internal/m365/collection/groups/collection.go +++ b/src/internal/m365/collection/groups/collection.go @@ -168,8 +168,8 @@ func (i Item) Deleted() bool { return i.deleted } -func (i *Item) Info() details.ItemInfo { - return details.ItemInfo{Groups: i.info} +func (i *Item) Info() (details.ItemInfo, error) { + return details.ItemInfo{Groups: i.info}, nil } func (i *Item) ModTime() time.Time { diff --git a/src/internal/m365/collection/site/collection.go b/src/internal/m365/collection/site/collection.go index a6196a4ed..b183cb9e2 100644 --- a/src/internal/m365/collection/site/collection.go +++ b/src/internal/m365/collection/site/collection.go @@ -149,8 +149,8 @@ func (sd Item) Deleted() bool { return sd.deleted } -func (sd *Item) Info() details.ItemInfo { - return details.ItemInfo{SharePoint: sd.info} +func (sd *Item) Info() (details.ItemInfo, error) { + return details.ItemInfo{SharePoint: sd.info}, nil } func (sd *Item) ModTime() time.Time { diff --git a/src/internal/m365/collection/site/collection_test.go b/src/internal/m365/collection/site/collection_test.go index 93525770b..d7b369326 100644 --- a/src/internal/m365/collection/site/collection_test.go +++ b/src/internal/m365/collection/site/collection_test.go @@ -183,9 +183,13 @@ func (suite *SharePointCollectionSuite) TestCollection_Items() { item := readItems[0] shareInfo, ok := item.(data.ItemInfo) require.True(t, ok) - require.NotNil(t, shareInfo.Info()) - require.NotNil(t, shareInfo.Info().SharePoint) - assert.Equal(t, test.itemName, shareInfo.Info().SharePoint.ItemName) + + info, err := shareInfo.Info() + require.NoError(t, err, clues.ToCore(err)) + + assert.NotNil(t, info) + assert.NotNil(t, info.SharePoint) + assert.Equal(t, test.itemName, info.SharePoint.ItemName) }) } } diff --git a/src/internal/m365/helper_test.go b/src/internal/m365/helper_test.go index 3036f4c87..cc0f7bba1 100644 --- a/src/internal/m365/helper_test.go +++ b/src/internal/m365/helper_test.go @@ -732,16 +732,20 @@ func compareDriveItem( if !isMeta { oitem := item.(*drive.Item) - info := oitem.Info() + + info, err := oitem.Info() + if !assert.NoError(t, err, clues.ToCore(err)) { + return true + } if info.OneDrive != nil { - displayName = oitem.Info().OneDrive.ItemName + displayName = info.OneDrive.ItemName // Don't need to check SharePoint because it was added after we stopped // adding meta files to backup details. - assert.False(t, oitem.Info().OneDrive.IsMeta, "meta marker for non meta item %s", name) + assert.False(t, info.OneDrive.IsMeta, "meta marker for non meta item %s", name) } else if info.SharePoint != nil { - displayName = oitem.Info().SharePoint.ItemName + displayName = info.SharePoint.ItemName } else { assert.Fail(t, "ItemInfo is not SharePoint or OneDrive") }