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