From 1452e8c0c97c5255eeb6411e827a53b0a9d51f94 Mon Sep 17 00:00:00 2001 From: Keepers Date: Fri, 13 Oct 2023 17:27:43 -0600 Subject: [PATCH] add extra catches for resource locked (#4499) Adds two layers of extra catches for resoruceLocked errors. First, in pkg/errs, adds error check funcs to ensure the source comparators are checked even if they somehow got skipped in the lower layer packages. Second, adds checks for resource locked throughout sites, users, and groups in the api layer. --- #### Does this PR need a docs update or release note? - [x] :no_entry: No #### Type of change - [x] :broom: Tech Debt/Cleanup #### Test Plan - [x] :zap: Unit test - [x] :green_heart: E2E --- src/pkg/errs/errs.go | 41 ++++++--- src/pkg/errs/errs_test.go | 131 +++++++++++++++++++++++++--- src/pkg/services/m365/api/groups.go | 8 ++ src/pkg/services/m365/api/sites.go | 8 ++ src/pkg/services/m365/api/users.go | 7 +- 5 files changed, 171 insertions(+), 24 deletions(-) diff --git a/src/pkg/errs/errs.go b/src/pkg/errs/errs.go index 68f3df0b0..566be9489 100644 --- a/src/pkg/errs/errs.go +++ b/src/pkg/errs/errs.go @@ -24,7 +24,7 @@ const ( // map of enums to errors. We might want to re-use an enum for multiple // internal errors (ex: "ServiceNotEnabled" may exist in both graph and // non-graph producers). -var internalToExternal = map[errEnum][]error{ +var externalToInternal = map[errEnum][]error{ ApplicationThrottled: {graph.ErrApplicationThrottled}, BackupNotFound: {repository.ErrorBackupNotFound}, RepoAlreadyExists: {repository.ErrorRepoAlreadyExists}, @@ -33,22 +33,43 @@ var internalToExternal = map[errEnum][]error{ ServiceNotEnabled: {graph.ErrServiceNotEnabled}, } -// Internal returns the internal errors which match to the public error category. -func Internal(enum errEnum) []error { - return internalToExternal[enum] +type ErrCheck func(error) bool + +// map of enums to error comparators. The above map assumes that we +// always stack or wrap the sentinel error in the returned error. But in +// many places of error handling, we primarily rely on error comparison +// checks. This allows us to apply those comparison checks instead of relying +// only on sentinels. +var externalToInternalCheck = map[errEnum][]ErrCheck{ + ApplicationThrottled: {graph.IsErrApplicationThrottled}, + ResourceNotAccessible: {graph.IsErrResourceLocked}, + ResourceOwnerNotFound: {graph.IsErrItemNotFound}, +} + +// Internal returns the internal errors and error checking functions which +// match to the public error enum. +func Internal(enum errEnum) ([]error, []ErrCheck) { + return externalToInternal[enum], externalToInternalCheck[enum] } // Is checks if the provided error contains an internal error that matches // the public error category. func Is(err error, enum errEnum) bool { - internalErrs, ok := internalToExternal[enum] - if !ok { - return false + internalErrs, ok := externalToInternal[enum] + if ok { + for _, target := range internalErrs { + if errors.Is(err, target) { + return true + } + } } - for _, target := range internalErrs { - if errors.Is(err, target) { - return true + internalChecks, ok := externalToInternalCheck[enum] + if ok { + for _, check := range internalChecks { + if check(err) { + return true + } } } diff --git a/src/pkg/errs/errs_test.go b/src/pkg/errs/errs_test.go index d5d6d5a37..f651c819b 100644 --- a/src/pkg/errs/errs_test.go +++ b/src/pkg/errs/errs_test.go @@ -20,20 +20,108 @@ func TestErrUnitSuite(t *testing.T) { suite.Run(t, &ErrUnitSuite{Suite: tester.NewUnitSuite(t)}) } -func (suite *ErrUnitSuite) TestInternal() { +func (suite *ErrUnitSuite) TestInternal_errs() { table := []struct { get errEnum expect []error }{ - {RepoAlreadyExists, []error{repository.ErrorRepoAlreadyExists}}, - {BackupNotFound, []error{repository.ErrorBackupNotFound}}, - {ServiceNotEnabled, []error{graph.ErrServiceNotEnabled}}, - {ResourceOwnerNotFound, []error{graph.ErrResourceOwnerNotFound}}, - {ResourceNotAccessible, []error{graph.ErrResourceLocked}}, + { + get: ApplicationThrottled, + expect: []error{graph.ErrApplicationThrottled}, + }, + { + get: RepoAlreadyExists, + expect: []error{repository.ErrorRepoAlreadyExists}, + }, + { + get: BackupNotFound, + expect: []error{repository.ErrorBackupNotFound}, + }, + { + get: ServiceNotEnabled, + expect: []error{graph.ErrServiceNotEnabled}, + }, + { + get: ResourceOwnerNotFound, + expect: []error{graph.ErrResourceOwnerNotFound}, + }, + { + get: ResourceNotAccessible, + expect: []error{graph.ErrResourceLocked}, + }, } for _, test := range table { suite.Run(string(test.get), func() { - assert.ElementsMatch(suite.T(), test.expect, Internal(test.get)) + // can't compare func signatures + errs, _ := Internal(test.get) + assert.ElementsMatch(suite.T(), test.expect, errs) + }) + } +} + +func (suite *ErrUnitSuite) TestInternal_checks() { + table := []struct { + get errEnum + err error + expectHasChecks assert.ValueAssertionFunc + expect assert.BoolAssertionFunc + }{ + { + get: ApplicationThrottled, + err: graph.ErrApplicationThrottled, + expectHasChecks: assert.NotEmpty, + expect: assert.True, + }, + { + get: RepoAlreadyExists, + err: graph.ErrApplicationThrottled, + expectHasChecks: assert.Empty, + expect: assert.False, + }, + { + get: BackupNotFound, + err: repository.ErrorBackupNotFound, + expectHasChecks: assert.Empty, + expect: assert.False, + }, + { + get: ServiceNotEnabled, + err: graph.ErrServiceNotEnabled, + expectHasChecks: assert.Empty, + expect: assert.False, + }, + { + get: ResourceOwnerNotFound, + // won't match, checks itemNotFound, which isn't an error enum + err: graph.ErrResourceOwnerNotFound, + expectHasChecks: assert.NotEmpty, + expect: assert.False, + }, + { + get: ResourceNotAccessible, + err: graph.ErrResourceLocked, + expectHasChecks: assert.NotEmpty, + expect: assert.True, + }, + } + for _, test := range table { + suite.Run(string(test.get), func() { + t := suite.T() + + _, checks := Internal(test.get) + + test.expectHasChecks(t, checks) + + var result bool + + for _, check := range checks { + if check(test.err) { + result = true + break + } + } + + test.expect(t, result) }) } } @@ -43,11 +131,30 @@ func (suite *ErrUnitSuite) TestIs() { target errEnum err error }{ - {RepoAlreadyExists, repository.ErrorRepoAlreadyExists}, - {BackupNotFound, repository.ErrorBackupNotFound}, - {ServiceNotEnabled, graph.ErrServiceNotEnabled}, - {ResourceOwnerNotFound, graph.ErrResourceOwnerNotFound}, - {ResourceNotAccessible, graph.ErrResourceLocked}, + { + target: ApplicationThrottled, + err: graph.ErrApplicationThrottled, + }, + { + target: RepoAlreadyExists, + err: repository.ErrorRepoAlreadyExists, + }, + { + target: BackupNotFound, + err: repository.ErrorBackupNotFound, + }, + { + target: ServiceNotEnabled, + err: graph.ErrServiceNotEnabled, + }, + { + target: ResourceOwnerNotFound, + err: graph.ErrResourceOwnerNotFound, + }, + { + target: ResourceNotAccessible, + err: graph.ErrResourceLocked, + }, } for _, test := range table { suite.Run(string(test.target), func() { diff --git a/src/pkg/services/m365/api/groups.go b/src/pkg/services/m365/api/groups.go index b6223dbdd..4dcba24e0 100644 --- a/src/pkg/services/m365/api/groups.go +++ b/src/pkg/services/m365/api/groups.go @@ -128,6 +128,10 @@ func (c Groups) GetByID( return group, nil } + if graph.IsErrResourceLocked(err) { + return nil, graph.Stack(ctx, clues.Stack(graph.ErrResourceLocked, err)) + } + logger.CtxErr(ctx, err).Info("finding group by id, falling back to display name") } @@ -140,6 +144,10 @@ func (c Groups) GetByID( resp, err := service.Client().Groups().Get(ctx, opts) if err != nil { + if graph.IsErrResourceLocked(err) { + err = clues.Stack(graph.ErrResourceLocked, err) + } + return nil, graph.Wrap(ctx, err, "finding group by display name") } diff --git a/src/pkg/services/m365/api/sites.go b/src/pkg/services/m365/api/sites.go index 813f1c1fa..94abfb8c7 100644 --- a/src/pkg/services/m365/api/sites.go +++ b/src/pkg/services/m365/api/sites.go @@ -158,6 +158,10 @@ func (c Sites) GetByID( err = clues.Stack(graph.ErrResourceOwnerNotFound, err) } + if graph.IsErrResourceLocked(err) { + err = clues.Stack(graph.ErrResourceLocked, err) + } + return nil, err } @@ -200,6 +204,10 @@ func (c Sites) GetByID( err = clues.Stack(graph.ErrResourceOwnerNotFound, err) } + if graph.IsErrResourceLocked(err) { + return nil, graph.Stack(ctx, clues.Stack(graph.ErrResourceLocked, err)) + } + return nil, err } diff --git a/src/pkg/services/m365/api/users.go b/src/pkg/services/m365/api/users.go index ccd3f22af..7408c60cd 100644 --- a/src/pkg/services/m365/api/users.go +++ b/src/pkg/services/m365/api/users.go @@ -121,9 +121,12 @@ func (c Users) GetByID(ctx context.Context, identifier string) (models.Userable, ) resp, err = c.Stable.Client().Users().ByUserId(identifier).Get(ctx, nil) - if err != nil { - return nil, graph.Wrap(ctx, err, "getting user") + if graph.IsErrResourceLocked(err) { + err = clues.Stack(graph.ErrResourceLocked, err) + } + + return nil, graph.Stack(ctx, err) } return resp, err