From 7f2a8735efdae9a821b5f288af029cbb85efd69b Mon Sep 17 00:00:00 2001 From: Keepers Date: Tue, 7 Feb 2023 14:10:34 -0700 Subject: [PATCH] add fault to repository repository funcs (#2364) ## Description adds fault.Errors to backupDetails() and backups(). ## Does this PR need a docs update or release note? - [x] :no_entry: No ## Type of change - [x] :broom: Tech Debt/Cleanup ## Issue(s) * #1970 ## Test Plan - [x] :zap: Unit test - [x] :green_heart: E2E --- src/cli/backup/exchange.go | 34 +++++++++----------- src/cli/backup/exchange_integration_test.go | 10 +++--- src/cli/backup/exchange_test.go | 6 ++-- src/cli/backup/onedrive.go | 34 +++++++++----------- src/cli/backup/onedrive_test.go | 6 ++-- src/cli/backup/sharepoint.go | 34 +++++++++----------- src/cli/backup/sharepoint_test.go | 6 ++-- src/cli/restore/exchange_integration_test.go | 6 ++-- src/cli/utils/testdata/opts.go | 14 +++++--- src/pkg/repository/repository.go | 26 ++++++++------- src/pkg/repository/repository_load_test.go | 8 +++-- 11 files changed, 93 insertions(+), 91 deletions(-) diff --git a/src/cli/backup/exchange.go b/src/cli/backup/exchange.go index 1f6f6ae81..5a12dba87 100644 --- a/src/cli/backup/exchange.go +++ b/src/cli/backup/exchange.go @@ -16,7 +16,6 @@ import ( "github.com/alcionai/corso/src/internal/model" "github.com/alcionai/corso/src/pkg/backup" "github.com/alcionai/corso/src/pkg/backup/details" - "github.com/alcionai/corso/src/pkg/fault" "github.com/alcionai/corso/src/pkg/path" "github.com/alcionai/corso/src/pkg/repository" "github.com/alcionai/corso/src/pkg/selectors" @@ -309,9 +308,10 @@ func createExchangeCmd(cmd *cobra.Command, args []string) error { bIDs = append(bIDs, bo.Results.BackupID) } - bups, err := r.Backups(ctx, bIDs) - if err != nil { - return Only(ctx, errors.Wrap(err, "Unable to retrieve backup results from storage")) + bups, ferrs := r.Backups(ctx, bIDs) + // TODO: print/log recoverable errors + if ferrs.Err() != nil { + return Only(ctx, errors.Wrap(ferrs.Err(), "Unable to retrieve backup results from storage")) } backup.PrintAll(ctx, bups) @@ -471,10 +471,9 @@ func detailsExchangeCmd(cmd *cobra.Command, args []string) error { defer utils.CloseRepo(ctx, r) - ds, errs := runDetailsExchangeCmd(ctx, r, backupID, opts) - if errs.Err() != nil { - // TODO: log/display iterated errors - return Only(ctx, errs.Err()) + ds, err := runDetailsExchangeCmd(ctx, r, backupID, opts) + if err != nil { + return Only(ctx, err) } if len(ds.Entries) == 0 { @@ -495,26 +494,25 @@ func runDetailsExchangeCmd( r repository.BackupGetter, backupID string, opts utils.ExchangeOpts, -) (*details.Details, *fault.Errors) { - errs := fault.New(false) - +) (*details.Details, error) { if err := utils.ValidateExchangeRestoreFlags(backupID, opts); err != nil { - return nil, errs.Fail(err) + return nil, err } - d, _, err := r.BackupDetails(ctx, backupID) - if err != nil { - if errors.Is(err, kopia.ErrNotFound) { - return nil, errs.Fail(errors.Errorf("No backup exists with the id %s", backupID)) + d, _, errs := r.BackupDetails(ctx, backupID) + // TODO: log/track recoverable errors + if errs.Err() != nil { + if errors.Is(errs.Err(), kopia.ErrNotFound) { + return nil, errors.Errorf("No backup exists with the id %s", backupID) } - return nil, errs.Fail(errors.Wrap(err, "Failed to get backup details in the repository")) + return nil, errors.Wrap(errs.Err(), "Failed to get backup details in the repository") } sel := utils.IncludeExchangeRestoreDataSelectors(opts) utils.FilterExchangeRestoreInfoSelectors(sel, opts) - return sel.Reduce(ctx, d, errs), errs + return sel.Reduce(ctx, d, errs), nil } // ------------------------------------------------------------------------------------------------ diff --git a/src/cli/backup/exchange_integration_test.go b/src/cli/backup/exchange_integration_test.go index 4e8f63ef4..1b2a36466 100644 --- a/src/cli/backup/exchange_integration_test.go +++ b/src/cli/backup/exchange_integration_test.go @@ -296,8 +296,9 @@ func (suite *PreparedBackupExchangeIntegrationSuite) SetupSuite() { b, err := suite.repo.Backup(ctx, bop.Results.BackupID) require.NoError(t, err, "retrieving recent backup by ID") require.Equal(t, bIDs, string(b.ID), "repo backup matches results id") - _, b, err = suite.repo.BackupDetails(ctx, bIDs) - require.NoError(t, err, "retrieving recent backup details by ID") + _, b, errs := suite.repo.BackupDetails(ctx, bIDs) + require.NoError(t, errs.Err(), "retrieving recent backup details by ID") + require.Empty(t, errs.Errs(), "retrieving recent backup details by ID") require.Equal(t, bIDs, string(b.ID), "repo details matches results id") suite.backupOps[set] = string(b.ID) @@ -396,8 +397,9 @@ func (suite *PreparedBackupExchangeIntegrationSuite) TestExchangeDetailsCmd() { bID := suite.backupOps[set] // fetch the details from the repo first - deets, _, err := suite.repo.BackupDetails(ctx, string(bID)) - require.NoError(t, err) + deets, _, errs := suite.repo.BackupDetails(ctx, string(bID)) + require.NoError(t, errs.Err()) + require.Empty(t, errs.Errs()) cmd := tester.StubRootCmd( "backup", "details", "exchange", diff --git a/src/cli/backup/exchange_test.go b/src/cli/backup/exchange_test.go index c67a5c15b..40a1f9b2c 100644 --- a/src/cli/backup/exchange_test.go +++ b/src/cli/backup/exchange_test.go @@ -224,8 +224,7 @@ func (suite *ExchangeSuite) TestExchangeBackupDetailsSelectors() { test.BackupGetter, "backup-ID", test.Opts) - assert.NoError(t, err.Err(), "failure") - assert.Empty(t, err.Errs(), "recovered errors") + assert.NoError(t, err, "failure") assert.ElementsMatch(t, test.Expected, output.Entries) }) } @@ -242,8 +241,7 @@ func (suite *ExchangeSuite) TestExchangeBackupDetailsSelectorsBadFormats() { test.BackupGetter, "backup-ID", test.Opts) - assert.Error(t, err.Err(), "failure") - assert.Empty(t, err.Errs(), "recovered errors") + assert.Error(t, err, "failure") assert.Empty(t, output) }) } diff --git a/src/cli/backup/onedrive.go b/src/cli/backup/onedrive.go index 517477661..2b60432ff 100644 --- a/src/cli/backup/onedrive.go +++ b/src/cli/backup/onedrive.go @@ -16,7 +16,6 @@ import ( "github.com/alcionai/corso/src/internal/model" "github.com/alcionai/corso/src/pkg/backup" "github.com/alcionai/corso/src/pkg/backup/details" - "github.com/alcionai/corso/src/pkg/fault" "github.com/alcionai/corso/src/pkg/path" "github.com/alcionai/corso/src/pkg/repository" "github.com/alcionai/corso/src/pkg/selectors" @@ -232,9 +231,10 @@ func createOneDriveCmd(cmd *cobra.Command, args []string) error { bIDs = append(bIDs, bo.Results.BackupID) } - bups, err := r.Backups(ctx, bIDs) - if err != nil { - return Only(ctx, errors.Wrap(err, "Unable to retrieve backup results from storage")) + bups, ferrs := r.Backups(ctx, bIDs) + // TODO: print/log recoverable errors + if ferrs.Err() != nil { + return Only(ctx, errors.Wrap(ferrs.Err(), "Unable to retrieve backup results from storage")) } backup.PrintAll(ctx, bups) @@ -363,10 +363,9 @@ func detailsOneDriveCmd(cmd *cobra.Command, args []string) error { Populated: utils.GetPopulatedFlags(cmd), } - ds, errs := runDetailsOneDriveCmd(ctx, r, backupID, opts) - if errs.Err() != nil { - // TODO: log/display iterated errors - return Only(ctx, errs.Err()) + ds, err := runDetailsOneDriveCmd(ctx, r, backupID, opts) + if err != nil { + return Only(ctx, err) } if len(ds.Entries) == 0 { @@ -387,26 +386,25 @@ func runDetailsOneDriveCmd( r repository.BackupGetter, backupID string, opts utils.OneDriveOpts, -) (*details.Details, *fault.Errors) { - errs := fault.New(false) - +) (*details.Details, error) { if err := utils.ValidateOneDriveRestoreFlags(backupID, opts); err != nil { - return nil, errs.Fail(err) + return nil, err } - d, _, err := r.BackupDetails(ctx, backupID) - if err != nil { - if errors.Is(err, kopia.ErrNotFound) { - return nil, errs.Fail(errors.Errorf("no backup exists with the id %s", backupID)) + d, _, errs := r.BackupDetails(ctx, backupID) + // TODO: log/track recoverable errors + if errs.Err() != nil { + if errors.Is(errs.Err(), kopia.ErrNotFound) { + return nil, errors.Errorf("no backup exists with the id %s", backupID) } - return nil, errs.Fail(errors.Wrap(err, "Failed to get backup details in the repository")) + return nil, errors.Wrap(errs.Err(), "Failed to get backup details in the repository") } sel := utils.IncludeOneDriveRestoreDataSelectors(opts) utils.FilterOneDriveRestoreInfoSelectors(sel, opts) - return sel.Reduce(ctx, d, errs), errs + return sel.Reduce(ctx, d, errs), nil } // `corso backup delete onedrive [...]` diff --git a/src/cli/backup/onedrive_test.go b/src/cli/backup/onedrive_test.go index 7fb2a38e3..8dde53bd8 100644 --- a/src/cli/backup/onedrive_test.go +++ b/src/cli/backup/onedrive_test.go @@ -99,8 +99,7 @@ func (suite *OneDriveSuite) TestOneDriveBackupDetailsSelectors() { test.BackupGetter, "backup-ID", test.Opts) - assert.NoError(t, err.Err()) - assert.Empty(t, err.Errs()) + assert.NoError(t, err) assert.ElementsMatch(t, test.Expected, output.Entries) }) } @@ -117,8 +116,7 @@ func (suite *OneDriveSuite) TestOneDriveBackupDetailsSelectorsBadFormats() { test.BackupGetter, "backup-ID", test.Opts) - assert.Error(t, err.Err()) - assert.Empty(t, err.Errs()) + assert.Error(t, err) assert.Empty(t, output) }) } diff --git a/src/cli/backup/sharepoint.go b/src/cli/backup/sharepoint.go index e8d65752a..c2a155334 100644 --- a/src/cli/backup/sharepoint.go +++ b/src/cli/backup/sharepoint.go @@ -18,7 +18,6 @@ import ( "github.com/alcionai/corso/src/internal/model" "github.com/alcionai/corso/src/pkg/backup" "github.com/alcionai/corso/src/pkg/backup/details" - "github.com/alcionai/corso/src/pkg/fault" "github.com/alcionai/corso/src/pkg/path" "github.com/alcionai/corso/src/pkg/repository" "github.com/alcionai/corso/src/pkg/selectors" @@ -252,9 +251,10 @@ func createSharePointCmd(cmd *cobra.Command, args []string) error { bIDs = append(bIDs, bo.Results.BackupID) } - bups, err := r.Backups(ctx, bIDs) - if err != nil { - return Only(ctx, errors.Wrap(err, "Unable to retrieve backup results from storage")) + bups, ferrs := r.Backups(ctx, bIDs) + // TODO: print/log recoverable errors + if ferrs.Err() != nil { + return Only(ctx, errors.Wrap(ferrs.Err(), "Unable to retrieve backup results from storage")) } backup.PrintAll(ctx, bups) @@ -482,10 +482,9 @@ func detailsSharePointCmd(cmd *cobra.Command, args []string) error { Populated: utils.GetPopulatedFlags(cmd), } - ds, errs := runDetailsSharePointCmd(ctx, r, backupID, opts) - if errs.Err() != nil { - // TODO: log/display iterated errors - return Only(ctx, errs.Err()) + ds, err := runDetailsSharePointCmd(ctx, r, backupID, opts) + if err != nil { + return Only(ctx, err) } if len(ds.Entries) == 0 { @@ -506,24 +505,23 @@ func runDetailsSharePointCmd( r repository.BackupGetter, backupID string, opts utils.SharePointOpts, -) (*details.Details, *fault.Errors) { - errs := fault.New(false) - +) (*details.Details, error) { if err := utils.ValidateSharePointRestoreFlags(backupID, opts); err != nil { - return nil, errs.Fail(err) + return nil, err } - d, _, err := r.BackupDetails(ctx, backupID) - if err != nil { - if errors.Is(err, kopia.ErrNotFound) { - return nil, errs.Fail(errors.Errorf("no backup exists with the id %s", backupID)) + d, _, errs := r.BackupDetails(ctx, backupID) + // TODO: log/track recoverable errors + if errs.Err() != nil { + if errors.Is(errs.Err(), kopia.ErrNotFound) { + return nil, errors.Errorf("no backup exists with the id %s", backupID) } - return nil, errs.Fail(errors.Wrap(err, "Failed to get backup details in the repository")) + return nil, errors.Wrap(errs.Err(), "Failed to get backup details in the repository") } sel := utils.IncludeSharePointRestoreDataSelectors(opts) utils.FilterSharePointRestoreInfoSelectors(sel, opts) - return sel.Reduce(ctx, d, errs), errs + return sel.Reduce(ctx, d, errs), nil } diff --git a/src/cli/backup/sharepoint_test.go b/src/cli/backup/sharepoint_test.go index a46deeeff..e5b206ff6 100644 --- a/src/cli/backup/sharepoint_test.go +++ b/src/cli/backup/sharepoint_test.go @@ -214,8 +214,7 @@ func (suite *SharePointSuite) TestSharePointBackupDetailsSelectors() { test.BackupGetter, "backup-ID", test.Opts) - assert.NoError(t, err.Err()) - assert.Empty(t, err.Errs()) + assert.NoError(t, err) assert.ElementsMatch(t, test.Expected, output.Entries) }) } @@ -232,8 +231,7 @@ func (suite *SharePointSuite) TestSharePointBackupDetailsSelectorsBadFormats() { test.BackupGetter, "backup-ID", test.Opts) - assert.Error(t, err.Err()) - assert.Empty(t, err.Errs()) + assert.Error(t, err) assert.Empty(t, output) }) } diff --git a/src/cli/restore/exchange_integration_test.go b/src/cli/restore/exchange_integration_test.go index 23649b23b..4ef62e58b 100644 --- a/src/cli/restore/exchange_integration_test.go +++ b/src/cli/restore/exchange_integration_test.go @@ -110,8 +110,10 @@ func (suite *RestoreExchangeIntegrationSuite) SetupSuite() { // sanity check, ensure we can find the backup and its details immediately _, err = suite.repo.Backup(ctx, bop.Results.BackupID) require.NoError(t, err, "retrieving recent backup by ID") - _, _, err = suite.repo.BackupDetails(ctx, string(bop.Results.BackupID)) - require.NoError(t, err, "retrieving recent backup details by ID") + + _, _, errs := suite.repo.BackupDetails(ctx, string(bop.Results.BackupID)) + require.NoError(t, errs.Err(), "retrieving recent backup details by ID") + require.Empty(t, errs.Errs(), "retrieving recent backup details by ID") } } diff --git a/src/cli/utils/testdata/opts.go b/src/cli/utils/testdata/opts.go index 68a33911a..e958fc5c5 100644 --- a/src/cli/utils/testdata/opts.go +++ b/src/cli/utils/testdata/opts.go @@ -10,6 +10,7 @@ import ( "github.com/alcionai/corso/src/internal/model" "github.com/alcionai/corso/src/pkg/backup" "github.com/alcionai/corso/src/pkg/backup/details" + "github.com/alcionai/corso/src/pkg/fault" "github.com/alcionai/corso/src/pkg/selectors" "github.com/alcionai/corso/src/pkg/selectors/testdata" "github.com/alcionai/corso/src/pkg/store" @@ -497,8 +498,11 @@ func (MockBackupGetter) Backup( return nil, errors.New("unexpected call to mock") } -func (MockBackupGetter) Backups(context.Context, []model.StableID) ([]*backup.Backup, error) { - return nil, errors.New("unexpected call to mock") +func (MockBackupGetter) Backups( + context.Context, + []model.StableID, +) ([]*backup.Backup, *fault.Errors) { + return nil, fault.New(false).Fail(errors.New("unexpected call to mock")) } func (MockBackupGetter) BackupsByTag( @@ -511,10 +515,10 @@ func (MockBackupGetter) BackupsByTag( func (bg *MockBackupGetter) BackupDetails( ctx context.Context, backupID string, -) (*details.Details, *backup.Backup, error) { +) (*details.Details, *backup.Backup, *fault.Errors) { if bg == nil { - return testdata.GetDetailsSet(), nil, nil + return testdata.GetDetailsSet(), nil, fault.New(true) } - return nil, nil, errors.New("unexpected call to mock") + return nil, nil, fault.New(false).Fail(errors.New("unexpected call to mock")) } diff --git a/src/pkg/repository/repository.go b/src/pkg/repository/repository.go index a8f1e2827..429b0e7a7 100644 --- a/src/pkg/repository/repository.go +++ b/src/pkg/repository/repository.go @@ -6,7 +6,6 @@ import ( "github.com/alcionai/clues" "github.com/google/uuid" - "github.com/hashicorp/go-multierror" "github.com/pkg/errors" "github.com/alcionai/corso/src/internal/events" @@ -19,6 +18,7 @@ import ( "github.com/alcionai/corso/src/pkg/backup" "github.com/alcionai/corso/src/pkg/backup/details" "github.com/alcionai/corso/src/pkg/control" + "github.com/alcionai/corso/src/pkg/fault" "github.com/alcionai/corso/src/pkg/logger" "github.com/alcionai/corso/src/pkg/selectors" "github.com/alcionai/corso/src/pkg/storage" @@ -31,12 +31,12 @@ var ErrorRepoAlreadyExists = errors.New("a repository was already initialized wi // repository. type BackupGetter interface { Backup(ctx context.Context, id model.StableID) (*backup.Backup, error) - Backups(ctx context.Context, ids []model.StableID) ([]*backup.Backup, error) + Backups(ctx context.Context, ids []model.StableID) ([]*backup.Backup, *fault.Errors) BackupsByTag(ctx context.Context, fs ...store.FilterOption) ([]*backup.Backup, error) BackupDetails( ctx context.Context, backupID string, - ) (*details.Details, *backup.Backup, error) + ) (*details.Details, *backup.Backup, *fault.Errors) } type Repository interface { @@ -282,23 +282,23 @@ func (r repository) Backup(ctx context.Context, id model.StableID) (*backup.Back // BackupsByID lists backups by ID. Returns as many backups as possible with // errors for the backups it was unable to retrieve. -func (r repository) Backups(ctx context.Context, ids []model.StableID) ([]*backup.Backup, error) { +func (r repository) Backups(ctx context.Context, ids []model.StableID) ([]*backup.Backup, *fault.Errors) { var ( - errs *multierror.Error bups []*backup.Backup + errs = fault.New(false) sw = store.NewKopiaStore(r.modelStore) ) for _, id := range ids { b, err := sw.GetBackup(ctx, id) if err != nil { - errs = multierror.Append(errs, err) + errs.Add(clues.Stack(err).With("backup_id", id)) } bups = append(bups, b) } - return bups, errs.ErrorOrNil() + return bups, errs } // backups lists backups in a repository @@ -308,12 +308,16 @@ func (r repository) BackupsByTag(ctx context.Context, fs ...store.FilterOption) } // BackupDetails returns the specified backup details object -func (r repository) BackupDetails(ctx context.Context, backupID string) (*details.Details, *backup.Backup, error) { +func (r repository) BackupDetails( + ctx context.Context, + backupID string, +) (*details.Details, *backup.Backup, *fault.Errors) { sw := store.NewKopiaStore(r.modelStore) + errs := fault.New(false) dID, b, err := sw.GetDetailsIDFromBackupID(ctx, model.StableID(backupID)) if err != nil { - return nil, nil, err + return nil, nil, errs.Fail(err) } deets, err := streamstore.New( @@ -321,10 +325,10 @@ func (r repository) BackupDetails(ctx context.Context, backupID string) (*detail r.Account.ID(), b.Selector.PathService()).ReadBackupDetails(ctx, dID) if err != nil { - return nil, nil, err + return nil, nil, errs.Fail(err) } - return deets, b, nil + return deets, b, errs } // DeleteBackup removes the backup from both the model store and the backup storage. diff --git a/src/pkg/repository/repository_load_test.go b/src/pkg/repository/repository_load_test.go index 36714df71..a080d916c 100644 --- a/src/pkg/repository/repository_load_test.go +++ b/src/pkg/repository/repository_load_test.go @@ -19,6 +19,7 @@ import ( "github.com/alcionai/corso/src/pkg/backup" "github.com/alcionai/corso/src/pkg/backup/details" "github.com/alcionai/corso/src/pkg/control" + "github.com/alcionai/corso/src/pkg/fault" "github.com/alcionai/corso/src/pkg/path" "github.com/alcionai/corso/src/pkg/repository" "github.com/alcionai/corso/src/pkg/selectors" @@ -241,17 +242,18 @@ func runBackupDetailsLoadTest( t.Run("backup_details_"+name, func(t *testing.T) { var ( - err error + errs *fault.Errors b *backup.Backup ds *details.Details labels = pprof.Labels("details_load_test", name) ) pprof.Do(ctx, labels, func(ctx context.Context) { - ds, b, err = r.BackupDetails(ctx, backupID) + ds, b, errs = r.BackupDetails(ctx, backupID) }) - require.NoError(t, err, "retrieving details in backup "+backupID) + require.NoError(t, errs.Err(), "retrieving details in backup "+backupID) + require.Empty(t, errs.Errs(), "retrieving details in backup "+backupID) require.NotNil(t, ds, "backup details must exist") require.NotNil(t, b, "backup must exist")