From a45aeda4a27b7698d3d3da79c48a1411821615d6 Mon Sep 17 00:00:00 2001 From: Vaibhav Kamra Date: Wed, 21 Dec 2022 11:17:24 -0800 Subject: [PATCH] Update folder size and modified time in details (#1881) ## Description Caches folder info added during details construction in the details builder and keeps the size/modified time updated as newer items are added. As part of this, this PR refactors the details package to separate out building `details.Details` from the in-memory representation and model. ## Does this PR need a docs update or release note? - [ ] :white_check_mark: Yes, it's included - [x] :clock1: Yes, but in a later PR - [ ] :no_entry: No ## Type of change - [x] :sunflower: Feature - [ ] :bug: Bugfix - [ ] :world_map: Documentation - [ ] :robot: Test - [ ] :computer: CI/Deployment - [ ] :hamster: Trivial/Minor ## Issue(s) * #1850 ## Test Plan - [ ] :muscle: Manual - [x] :zap: Unit test - [ ] :green_heart: E2E --- .../connector/exchange/service_restore.go | 4 +- src/internal/connector/graph_connector.go | 4 +- src/internal/connector/onedrive/restore.go | 4 +- src/internal/connector/sharepoint/restore.go | 2 +- src/internal/kopia/upload.go | 4 +- src/internal/kopia/upload_test.go | 14 +- src/internal/kopia/wrapper.go | 6 +- src/internal/streamstore/streamstore_test.go | 6 +- src/pkg/backup/details/details.go | 112 ++++++++++---- src/pkg/backup/details/details_test.go | 140 +++++++++++++++++- 10 files changed, 239 insertions(+), 57 deletions(-) diff --git a/src/internal/connector/exchange/service_restore.go b/src/internal/connector/exchange/service_restore.go index ffed09563..7905a41f3 100644 --- a/src/internal/connector/exchange/service_restore.go +++ b/src/internal/connector/exchange/service_restore.go @@ -288,7 +288,7 @@ func RestoreExchangeDataCollections( gs graph.Servicer, dest control.RestoreDestination, dcs []data.Collection, - deets *details.Details, + deets *details.Builder, ) (*support.ConnectorOperationStatus, error) { var ( // map of caches... but not yet... @@ -349,7 +349,7 @@ func restoreCollection( dc data.Collection, folderID string, policy control.CollisionPolicy, - deets *details.Details, + deets *details.Builder, errUpdater func(string, error), ) (support.CollectionMetrics, bool) { ctx, end := D.Span(ctx, "gc:exchange:restoreCollection", D.Label("path", dc.FullPath())) diff --git a/src/internal/connector/graph_connector.go b/src/internal/connector/graph_connector.go index 226be17cc..aeaf8193b 100644 --- a/src/internal/connector/graph_connector.go +++ b/src/internal/connector/graph_connector.go @@ -262,7 +262,7 @@ func (gc *GraphConnector) RestoreDataCollections( var ( status *support.ConnectorOperationStatus err error - deets = &details.Details{} + deets = &details.Builder{} ) switch selector.Service { @@ -279,7 +279,7 @@ func (gc *GraphConnector) RestoreDataCollections( gc.incrementAwaitingMessages() gc.UpdateStatus(status) - return deets, err + return deets.Details(), err } // AwaitStatus waits for all gc tasks to complete and then returns status diff --git a/src/internal/connector/onedrive/restore.go b/src/internal/connector/onedrive/restore.go index ff2efab53..74a9095a2 100644 --- a/src/internal/connector/onedrive/restore.go +++ b/src/internal/connector/onedrive/restore.go @@ -53,7 +53,7 @@ func RestoreCollections( service graph.Servicer, dest control.RestoreDestination, dcs []data.Collection, - deets *details.Details, + deets *details.Builder, ) (*support.ConnectorOperationStatus, error) { var ( restoreMetrics support.CollectionMetrics @@ -95,7 +95,7 @@ func RestoreCollection( dc data.Collection, source driveSource, restoreContainerName string, - deets *details.Details, + deets *details.Builder, errUpdater func(string, error), ) (support.CollectionMetrics, bool) { ctx, end := D.Span(ctx, "gc:oneDrive:restoreCollection", D.Label("path", dc.FullPath())) diff --git a/src/internal/connector/sharepoint/restore.go b/src/internal/connector/sharepoint/restore.go index dd2a2783e..9cfd91512 100644 --- a/src/internal/connector/sharepoint/restore.go +++ b/src/internal/connector/sharepoint/restore.go @@ -20,7 +20,7 @@ func RestoreCollections( service graph.Servicer, dest control.RestoreDestination, dcs []data.Collection, - deets *details.Details, + deets *details.Builder, ) (*support.ConnectorOperationStatus, error) { var ( restoreMetrics support.CollectionMetrics diff --git a/src/internal/kopia/upload.go b/src/internal/kopia/upload.go index 741a86cb7..8647b710d 100644 --- a/src/internal/kopia/upload.go +++ b/src/internal/kopia/upload.go @@ -125,7 +125,7 @@ type itemDetails struct { type corsoProgress struct { snapshotfs.UploadProgress pending map[string]*itemDetails - deets *details.Details + deets *details.Builder mu sync.RWMutex totalBytes int64 } @@ -182,7 +182,7 @@ func (cp *corsoProgress) FinishedFile(relativePath string, err error) { parent = nextParent } - cp.deets.AddFolders(folders) + cp.deets.AddFoldersForItem(folders, d.info) } // Kopia interface function used as a callback when kopia finishes hashing a file. diff --git a/src/internal/kopia/upload_test.go b/src/internal/kopia/upload_test.go index 9f8737bd8..ed8699394 100644 --- a/src/internal/kopia/upload_test.go +++ b/src/internal/kopia/upload_test.go @@ -435,7 +435,7 @@ var finishedFileTable = []struct { func (suite *CorsoProgressUnitSuite) TestFinishedFile() { for _, test := range finishedFileTable { suite.T().Run(test.name, func(t *testing.T) { - bd := &details.Details{} + bd := &details.Builder{} cp := corsoProgress{ UploadProgress: &snapshotfs.NullUploadProgress{}, deets: bd, @@ -455,7 +455,7 @@ func (suite *CorsoProgressUnitSuite) TestFinishedFile() { } assert.Empty(t, cp.pending) - assert.Len(t, bd.Entries, test.expectedNumEntries) + assert.Len(t, bd.Details().Entries, test.expectedNumEntries) }) } } @@ -466,7 +466,7 @@ func (suite *CorsoProgressUnitSuite) TestFinishedFileBuildsHierarchy() { expectedFolderOrder := suite.targetFilePath.ToBuilder().Dir().Elements() // Setup stuff. - bd := &details.Details{} + bd := &details.Builder{} cp := corsoProgress{ UploadProgress: &snapshotfs.NullUploadProgress{}, deets: bd, @@ -485,8 +485,10 @@ func (suite *CorsoProgressUnitSuite) TestFinishedFileBuildsHierarchy() { refToEntry = map[string]*details.DetailsEntry{} ) - for i := 0; i < len(bd.Entries); i++ { - e := &bd.Entries[i] + entries := bd.Details().Entries + + for i := 0; i < len(entries); i++ { + e := &entries[i] if e.Folder == nil { continue } @@ -522,7 +524,7 @@ func (suite *CorsoProgressUnitSuite) TestFinishedFileBuildsHierarchy() { func (suite *CorsoProgressUnitSuite) TestFinishedHashingFile() { for _, test := range finishedFileTable { suite.T().Run(test.name, func(t *testing.T) { - bd := &details.Details{} + bd := &details.Builder{} cp := corsoProgress{ UploadProgress: &snapshotfs.NullUploadProgress{}, deets: bd, diff --git a/src/internal/kopia/wrapper.go b/src/internal/kopia/wrapper.go index 3f1cbbcac..95c46f7a2 100644 --- a/src/internal/kopia/wrapper.go +++ b/src/internal/kopia/wrapper.go @@ -131,12 +131,12 @@ func (w Wrapper) BackupCollections( defer end() if len(collections) == 0 { - return &BackupStats{}, &details.Details{}, nil + return &BackupStats{}, (&details.Builder{}).Details(), nil } progress := &corsoProgress{ pending: map[string]*itemDetails{}, - deets: &details.Details{}, + deets: &details.Builder{}, } // TODO(ashmrtn): Pass previousSnapshots here to enable building the directory @@ -158,7 +158,7 @@ func (w Wrapper) BackupCollections( return nil, nil, err } - return s, progress.deets, nil + return s, progress.deets.Details(), nil } func (w Wrapper) makeSnapshotWithRoot( diff --git a/src/internal/streamstore/streamstore_test.go b/src/internal/streamstore/streamstore_test.go index 5a41bdd7a..843a8ebe5 100644 --- a/src/internal/streamstore/streamstore_test.go +++ b/src/internal/streamstore/streamstore_test.go @@ -46,15 +46,17 @@ func (suite *StreamStoreIntegrationSuite) TestDetails() { defer kw.Close(ctx) - deets := &details.Details{} + deetsBuilder := &details.Builder{} - deets.Add("ref", "shortref", "parentref", true, + deetsBuilder.Add("ref", "shortref", "parentref", true, details.ItemInfo{ Exchange: &details.ExchangeInfo{ Subject: "hello world", }, }) + deets := deetsBuilder.Details() + ss := New(kw, "tenant", path.ExchangeService) id, err := ss.WriteBackupDetails(ctx, deets) diff --git a/src/pkg/backup/details/details.go b/src/pkg/backup/details/details.go index de1b858c5..79f85cc31 100644 --- a/src/pkg/backup/details/details.go +++ b/src/pkg/backup/details/details.go @@ -100,6 +100,78 @@ func (dm DetailsModel) Items() []*DetailsEntry { return res } +// Builder should be used to create a details model. +type Builder struct { + d Details + mu sync.Mutex `json:"-"` + knownFolders map[string]FolderEntry `json:"-"` +} + +func (b *Builder) Add(repoRef, shortRef, parentRef string, updated bool, info ItemInfo) { + b.mu.Lock() + defer b.mu.Unlock() + b.d.add(repoRef, shortRef, parentRef, updated, info) +} + +func (b *Builder) Details() *Details { + b.mu.Lock() + defer b.mu.Unlock() + + // Write the cached folder entries to details + for _, folder := range b.knownFolders { + b.d.addFolder(folder) + } + + return &b.d +} + +// AddFoldersForItem adds entries for the given folders. It skips adding entries that +// have been added by previous calls. +func (b *Builder) AddFoldersForItem(folders []FolderEntry, itemInfo ItemInfo) { + b.mu.Lock() + defer b.mu.Unlock() + + if b.knownFolders == nil { + b.knownFolders = map[string]FolderEntry{} + } + + for _, folder := range folders { + if existing, ok := b.knownFolders[folder.ShortRef]; ok { + // We've seen this folder before for a different item. + // Update the "cached" folder entry + folder = existing + } + + // Update the folder's size and modified time + var ( + itemSize int64 + itemModified time.Time + ) + + switch { + case itemInfo.Exchange != nil: + itemSize = itemInfo.Exchange.Size + itemModified = itemInfo.Exchange.Modified + + case itemInfo.OneDrive != nil: + itemSize = itemInfo.OneDrive.Size + itemModified = itemInfo.OneDrive.Modified + + case itemInfo.SharePoint != nil: + itemSize = itemInfo.SharePoint.Size + itemModified = itemInfo.SharePoint.Modified + } + + folder.Info.Folder.Size += itemSize + + if folder.Info.Folder.Modified.Before(itemModified) { + folder.Info.Folder.Modified = itemModified + } + + b.knownFolders[folder.ShortRef] = folder + } +} + // -------------------------------------------------------------------------------- // Details // -------------------------------------------------------------------------------- @@ -109,15 +181,9 @@ func (dm DetailsModel) Items() []*DetailsEntry { // printing. type Details struct { DetailsModel - - // internal - mu sync.Mutex `json:"-"` - knownFolders map[string]struct{} `json:"-"` } -func (d *Details) Add(repoRef, shortRef, parentRef string, updated bool, info ItemInfo) { - d.mu.Lock() - defer d.mu.Unlock() +func (d *Details) add(repoRef, shortRef, parentRef string, updated bool, info ItemInfo) { d.Entries = append(d.Entries, DetailsEntry{ RepoRef: repoRef, ShortRef: shortRef, @@ -127,30 +193,14 @@ func (d *Details) Add(repoRef, shortRef, parentRef string, updated bool, info It }) } -// AddFolders adds entries for the given folders. It skips adding entries that -// have been added by previous calls. -func (d *Details) AddFolders(folders []FolderEntry) { - d.mu.Lock() - defer d.mu.Unlock() - - if d.knownFolders == nil { - d.knownFolders = map[string]struct{}{} - } - - for _, folder := range folders { - if _, ok := d.knownFolders[folder.ShortRef]; ok { - // Entry already exists, nothing to do. - continue - } - - d.knownFolders[folder.ShortRef] = struct{}{} - d.Entries = append(d.Entries, DetailsEntry{ - RepoRef: folder.RepoRef, - ShortRef: folder.ShortRef, - ParentRef: folder.ParentRef, - ItemInfo: folder.Info, - }) - } +// addFolder adds an entry for the given folder. +func (d *Details) addFolder(folder FolderEntry) { + d.Entries = append(d.Entries, DetailsEntry{ + RepoRef: folder.RepoRef, + ShortRef: folder.ShortRef, + ParentRef: folder.ParentRef, + ItemInfo: folder.Info, + }) } // -------------------------------------------------------------------------------- diff --git a/src/pkg/backup/details/details_test.go b/src/pkg/backup/details/details_test.go index 093eeb886..bd764ae6b 100644 --- a/src/pkg/backup/details/details_test.go +++ b/src/pkg/backup/details/details_test.go @@ -222,10 +222,22 @@ func (suite *DetailsUnitSuite) TestDetailsModel_Items() { } func (suite *DetailsUnitSuite) TestDetails_AddFolders() { + itemTime := time.Date(2022, 10, 21, 10, 0, 0, 0, time.UTC) + folderTimeOlderThanItem := time.Date(2022, 9, 21, 10, 0, 0, 0, time.UTC) + folderTimeNewerThanItem := time.Date(2022, 11, 21, 10, 0, 0, 0, time.UTC) + + itemInfo := details.ItemInfo{ + Exchange: &details.ExchangeInfo{ + Size: 20, + Modified: itemTime, + }, + } + table := []struct { - name string - folders []details.FolderEntry - expectedShortRefs []string + name string + folders []details.FolderEntry + expectedShortRefs []string + expectedFolderInfo map[string]details.FolderInfo }{ { name: "MultipleFolders", @@ -234,14 +246,28 @@ func (suite *DetailsUnitSuite) TestDetails_AddFolders() { RepoRef: "rr1", ShortRef: "sr1", ParentRef: "pr1", + Info: details.ItemInfo{ + Folder: &details.FolderInfo{ + Modified: folderTimeOlderThanItem, + }, + }, }, { RepoRef: "rr2", ShortRef: "sr2", ParentRef: "pr2", + Info: details.ItemInfo{ + Folder: &details.FolderInfo{ + Modified: folderTimeNewerThanItem, + }, + }, }, }, expectedShortRefs: []string{"sr1", "sr2"}, + expectedFolderInfo: map[string]details.FolderInfo{ + "sr1": {Size: 20, Modified: itemTime}, + "sr2": {Size: 20, Modified: folderTimeNewerThanItem}, + }, }, { name: "MultipleFoldersWithRepeats", @@ -250,36 +276,138 @@ func (suite *DetailsUnitSuite) TestDetails_AddFolders() { RepoRef: "rr1", ShortRef: "sr1", ParentRef: "pr1", + Info: details.ItemInfo{ + Folder: &details.FolderInfo{ + Modified: folderTimeOlderThanItem, + }, + }, }, { RepoRef: "rr2", ShortRef: "sr2", ParentRef: "pr2", + Info: details.ItemInfo{ + Folder: &details.FolderInfo{ + Modified: folderTimeOlderThanItem, + }, + }, }, { RepoRef: "rr1", ShortRef: "sr1", ParentRef: "pr1", + Info: details.ItemInfo{ + Folder: &details.FolderInfo{ + Modified: folderTimeOlderThanItem, + }, + }, }, { RepoRef: "rr3", ShortRef: "sr3", ParentRef: "pr3", + Info: details.ItemInfo{ + Folder: &details.FolderInfo{ + Modified: folderTimeNewerThanItem, + }, + }, }, }, expectedShortRefs: []string{"sr1", "sr2", "sr3"}, + expectedFolderInfo: map[string]details.FolderInfo{ + // Two items were added + "sr1": {Size: 40, Modified: itemTime}, + "sr2": {Size: 20, Modified: itemTime}, + "sr3": {Size: 20, Modified: folderTimeNewerThanItem}, + }, }, } for _, test := range table { suite.T().Run(test.name, func(t *testing.T) { - deets := details.Details{} - deets.AddFolders(test.folders) - + builder := details.Builder{} + builder.AddFoldersForItem(test.folders, itemInfo) + deets := builder.Details() assert.Len(t, deets.Entries, len(test.expectedShortRefs)) for _, e := range deets.Entries { assert.Contains(t, test.expectedShortRefs, e.ShortRef) + assert.Equal(t, test.expectedFolderInfo[e.ShortRef].Size, e.Folder.Size) + assert.Equal(t, test.expectedFolderInfo[e.ShortRef].Modified, e.Folder.Modified) } }) } } + +func (suite *DetailsUnitSuite) TestDetails_AddFoldersDifferentServices() { + itemTime := time.Date(2022, 10, 21, 10, 0, 0, 0, time.UTC) + folderTimeOlderThanItem := time.Date(2022, 9, 21, 10, 0, 0, 0, time.UTC) + + table := []struct { + name string + item details.ItemInfo + expectedFolderInfo details.FolderInfo + }{ + { + name: "Exchange", + item: details.ItemInfo{ + Exchange: &details.ExchangeInfo{ + Size: 20, + Modified: itemTime, + }, + }, + expectedFolderInfo: details.FolderInfo{ + Size: 20, + Modified: itemTime, + }, + }, + { + name: "OneDrive", + item: details.ItemInfo{ + OneDrive: &details.OneDriveInfo{ + Size: 20, + Modified: itemTime, + }, + }, + expectedFolderInfo: details.FolderInfo{ + Size: 20, + Modified: itemTime, + }, + }, + { + name: "SharePoint", + item: details.ItemInfo{ + SharePoint: &details.SharePointInfo{ + Size: 20, + Modified: itemTime, + }, + }, + expectedFolderInfo: details.FolderInfo{ + Size: 20, + Modified: itemTime, + }, + }, + } + for _, test := range table { + suite.T().Run(test.name, func(t *testing.T) { + folderEntry := details.FolderEntry{ + RepoRef: "rr1", + ShortRef: "sr1", + ParentRef: "pr1", + Info: details.ItemInfo{ + Folder: &details.FolderInfo{ + Modified: folderTimeOlderThanItem, + }, + }, + } + + builder := details.Builder{} + builder.AddFoldersForItem([]details.FolderEntry{folderEntry}, test.item) + deets := builder.Details() + require.Len(t, deets.Entries, 1) + + got := deets.Entries[0].Folder + + assert.Equal(t, test.expectedFolderInfo, *got) + }) + } +}