From 971d8744622cc0fd0a61511af000802ccccc3e84 Mon Sep 17 00:00:00 2001 From: Keepers Date: Wed, 3 Jan 2024 11:13:41 -0700 Subject: [PATCH] introduce errs/core (#4897) first step towards having a centralized set of error sentinels that can be passed around corso instead of re-using low level error sentinels like those found in the graph/errors.go file. This PR works as a standalone, and only handles the lowest hanging fruit. SDK consumers will need to change their error enum references, but all api behavior should remain the same. --- #### Does this PR need a docs update or release note? - [x] :no_entry: No #### Type of change - [x] :robot: Supportability/Tests #### Issue(s) * #4685 #### Test Plan - [x] :zap: Unit test - [x] :green_heart: E2E --- src/cli/backup/backup.go | 4 +- src/internal/m365/backup.go | 3 +- src/internal/m365/controller.go | 5 +- src/internal/m365/service/onedrive/enabled.go | 3 +- src/internal/operations/backup.go | 3 +- src/internal/operations/restore.go | 4 +- src/pkg/errs/core/core.go | 106 ++++++++++++++++++ src/pkg/errs/core/core_test.go | 91 +++++++++++++++ src/pkg/errs/errs.go | 54 ++++----- src/pkg/errs/errs_test.go | 84 +++++--------- src/pkg/services/m365/api/graph/errors.go | 30 ++--- .../services/m365/api/graph/errors_test.go | 8 +- .../services/m365/api/graph/http_wrapper.go | 3 +- src/pkg/services/m365/api/graph/service.go | 3 +- src/pkg/services/m365/api/groups.go | 3 +- src/pkg/services/m365/api/groups_test.go | 5 +- src/pkg/services/m365/api/sites.go | 5 +- src/pkg/services/m365/api/sites_test.go | 6 +- src/pkg/services/m365/api/users.go | 3 +- src/pkg/services/m365/api/users_test.go | 3 +- src/pkg/services/m365/groups_test.go | 6 +- src/pkg/services/m365/sites.go | 3 +- src/pkg/services/m365/sites_test.go | 3 +- 23 files changed, 301 insertions(+), 137 deletions(-) create mode 100644 src/pkg/errs/core/core.go create mode 100644 src/pkg/errs/core/core_test.go diff --git a/src/cli/backup/backup.go b/src/cli/backup/backup.go index 9debea9d6..2d5aa9993 100644 --- a/src/cli/backup/backup.go +++ b/src/cli/backup/backup.go @@ -20,11 +20,11 @@ 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/errs/core" "github.com/alcionai/corso/src/pkg/logger" "github.com/alcionai/corso/src/pkg/path" "github.com/alcionai/corso/src/pkg/repository" "github.com/alcionai/corso/src/pkg/selectors" - "github.com/alcionai/corso/src/pkg/services/m365/api/graph" "github.com/alcionai/corso/src/pkg/store" ) @@ -210,7 +210,7 @@ func genericCreateCommand( err = bo.Run(ictx) if err != nil { - if errors.Is(err, graph.ErrServiceNotEnabled) { + if errors.Is(err, core.ErrServiceNotEnabled) { logger.Ctx(ctx).Infow("service not enabled", "resource_owner_id", bo.ResourceOwner.ID(), "service", serviceName) diff --git a/src/internal/m365/backup.go b/src/internal/m365/backup.go index a9b6527f0..06af7b9bd 100644 --- a/src/internal/m365/backup.go +++ b/src/internal/m365/backup.go @@ -17,6 +17,7 @@ import ( bupMD "github.com/alcionai/corso/src/pkg/backup/metadata" "github.com/alcionai/corso/src/pkg/control" "github.com/alcionai/corso/src/pkg/count" + "github.com/alcionai/corso/src/pkg/errs/core" "github.com/alcionai/corso/src/pkg/fault" "github.com/alcionai/corso/src/pkg/filters" "github.com/alcionai/corso/src/pkg/path" @@ -170,7 +171,7 @@ func verifyBackupInputs(sels selectors.Selector, cachedIDs []string) error { } if !filters.Contains(ids).Compare(sels.ID()) { - return clues.Stack(graph.ErrResourceOwnerNotFound). + return clues.Stack(core.ErrResourceOwnerNotFound). With("selector_protected_resource", sels.DiscreteOwner) } diff --git a/src/internal/m365/controller.go b/src/internal/m365/controller.go index 4b152bf0d..fc81ea2ef 100644 --- a/src/internal/m365/controller.go +++ b/src/internal/m365/controller.go @@ -16,6 +16,7 @@ import ( "github.com/alcionai/corso/src/pkg/backup/details" "github.com/alcionai/corso/src/pkg/control" "github.com/alcionai/corso/src/pkg/count" + "github.com/alcionai/corso/src/pkg/errs/core" "github.com/alcionai/corso/src/pkg/path" "github.com/alcionai/corso/src/pkg/services/m365/api" "github.com/alcionai/corso/src/pkg/services/m365/api/graph" @@ -255,7 +256,7 @@ func (r resourceGetter) GetResourceIDAndNameFrom( id, name, err = r.getter.GetIDAndName(ctx, owner, api.CallConfig{}) if err != nil { if graph.IsErrUserNotFound(err) { - return nil, clues.Stack(graph.ErrResourceOwnerNotFound, err) + return nil, clues.Stack(core.ErrResourceOwnerNotFound, err) } if graph.IsErrResourceLocked(err) { @@ -266,7 +267,7 @@ func (r resourceGetter) GetResourceIDAndNameFrom( } if len(id) == 0 || len(name) == 0 { - return nil, clues.Stack(graph.ErrResourceOwnerNotFound) + return nil, clues.Stack(core.ErrResourceOwnerNotFound) } return idname.NewProvider(id, name), nil diff --git a/src/internal/m365/service/onedrive/enabled.go b/src/internal/m365/service/onedrive/enabled.go index 68b57ed6d..42b8d7b69 100644 --- a/src/internal/m365/service/onedrive/enabled.go +++ b/src/internal/m365/service/onedrive/enabled.go @@ -6,6 +6,7 @@ import ( "github.com/alcionai/clues" "github.com/microsoftgraph/msgraph-sdk-go/models" + "github.com/alcionai/corso/src/pkg/errs/core" "github.com/alcionai/corso/src/pkg/services/m365/api/graph" ) @@ -27,7 +28,7 @@ func IsServiceEnabled( } if graph.IsErrUserNotFound(err) { - return false, clues.Stack(graph.ErrResourceOwnerNotFound, err) + return false, clues.Stack(core.ErrResourceOwnerNotFound, err) } if graph.IsErrResourceLocked(err) { diff --git a/src/internal/operations/backup.go b/src/internal/operations/backup.go index 95fd95c26..c91c3b43b 100644 --- a/src/internal/operations/backup.go +++ b/src/internal/operations/backup.go @@ -28,6 +28,7 @@ import ( "github.com/alcionai/corso/src/pkg/control" "github.com/alcionai/corso/src/pkg/count" "github.com/alcionai/corso/src/pkg/dttm" + "github.com/alcionai/corso/src/pkg/errs/core" "github.com/alcionai/corso/src/pkg/fault" "github.com/alcionai/corso/src/pkg/logger" "github.com/alcionai/corso/src/pkg/path" @@ -231,7 +232,7 @@ func (op *BackupOperation) Run(ctx context.Context) (err error) { if !enabled { // Return named error so that we can check for it in caller. - err = clues.Wrap(graph.ErrServiceNotEnabled, "service not enabled for backup") + err = clues.Stack(core.ErrServiceNotEnabled) op.Errors.Fail(err) return err diff --git a/src/internal/operations/restore.go b/src/internal/operations/restore.go index a2c122ad6..48551be78 100644 --- a/src/internal/operations/restore.go +++ b/src/internal/operations/restore.go @@ -27,11 +27,11 @@ import ( "github.com/alcionai/corso/src/pkg/control" "github.com/alcionai/corso/src/pkg/count" "github.com/alcionai/corso/src/pkg/dttm" + "github.com/alcionai/corso/src/pkg/errs/core" "github.com/alcionai/corso/src/pkg/fault" "github.com/alcionai/corso/src/pkg/logger" "github.com/alcionai/corso/src/pkg/path" "github.com/alcionai/corso/src/pkg/selectors" - "github.com/alcionai/corso/src/pkg/services/m365/api/graph" "github.com/alcionai/corso/src/pkg/store" ) @@ -246,7 +246,7 @@ func (op *RestoreOperation) do( } if !enabled { - return nil, clues.WrapWC(ctx, graph.ErrServiceNotEnabled, "service not enabled for restore") + return nil, clues.StackWC(ctx, core.ErrServiceNotEnabled) } pcfg := observe.ProgressCfg{ diff --git a/src/pkg/errs/core/core.go b/src/pkg/errs/core/core.go new file mode 100644 index 000000000..16a55e4a7 --- /dev/null +++ b/src/pkg/errs/core/core.go @@ -0,0 +1,106 @@ +package core + +import "errors" + +// ----------------------------------------------------------------------------------------------- +// core.Err sentinels are provided to maintain a reference of commonplace errors throughout Corso. +// +// The general idea is that these errors allow the repo (and consumers of its CLI and SDK apis) +// to communicate clearly about the central identity of an error (ie: its "core"), without leaking +// service-specific details and imports from low-level apis, clients, and other packages. +// +// In order to maintain sanity here, a couple rules should be followed. +// +// 1. Sentinels should have generic messages. No references to downstream concepts. +// Basic cleanliness here. Downstream references contaminate the sentinel purpose. +// +// 2. Maintain coarseness. +// We won't need a core.Err version of every lower-level error. Try, where possible, +// to group concepts into broad categories. Ex: prefer "resource not found" over +// "user not found" or "site not found". +// +// 3. Always Stack/Wrap core.Errs. Only once. +// `return core.ErrFoo` should be avoided. Also, if you're handling a error returned +// by some internal package, do your due diligence and make sure it isn't already +// identified by a core.Err at a lower level. +// +// 4. Stacking/Wrapping is the lowest layer's job. +// We prefer to returning sentinels at lower layers instead of parsing errors at +// higher layers. This ensures higher layers only need to run errors.Is and .As +// checks, without needing take on low-level error details. +// +// 5. Add comments to explain the sentinels. +// Future maintainers may not easily grok the intent behind an existing sentinel. +// Because we want to keep the error messages themselves small and clean, a short +// explanation in the comments, even a basic one, can help a lot. +// +// 6. This package gets more important at higher layers. +// The goal is to make life easier for layers that are the most detached from low- +// level and internal packages. The closer that code gets to those lower layers, +// the less important it is to strictly use this package. But since most errors +// bubble up to the SDK and CLI APIs, it is eventually a critical issue that we +// categorize our errors smartly for those end users. +// ----------------------------------------------------------------------------------------------- + +type Err struct { + msg string +} + +func (e Err) Error() string { + return e.msg +} + +var ( + // currently we have no internal throttling controls. We only try to match + // external throttling requirements. This sentinel assumes that an external + // server has returned one or more throttling errors which has stopped + // operation progress. + ErrApplicationThrottled = &Err{msg: "application throttled"} + // about what it sounds like: we tried to look for a backup by ID, but the + // storage layer couldn't find anything for that ID. + ErrBackupNotFound = &Err{msg: "backup not found"} + // a catch-all for downstream api auth issues. doesn't matter which api. + ErrInsufficientAuthorization = &Err{msg: "insufficient authorization"} + // specifically for repository creation: if we tried to create a repo and + // it already exists with those credentials, we return this error. + ErrRepoAlreadyExists = &Err{msg: "repository already exists"} + // use this when a resource (user, etc; whatever owner is used to own the + // data in the given backup) is unable to be used for backup or restore. + // some nuance here: this is not the same as a broad-scale auth issue. + // it is also not the same as a "not found" issue. it's specific to + // cases where we can find the resource, and have authorization to access + // it, but are told by the external system that the resource is somehow + // unusable. + ErrResourceNotAccessible = &Err{msg: "resource not accesible"} + // use this when a resource (user, etc; whatever owner is used to own the + // data in the given backup) cannot be found in the system by the ID that + // the end user provided. + ErrResourceOwnerNotFound = &Err{msg: "resource owner not found"} + // a service is the set of application data within a given provider. eg: + // if m365 is the provider, then exchange is a service, so is oneDrive. + // this sentinel is used to indicate that the service in question is not + // accessible to the user. this is not the same as an auth error. more + // often its a license issue. as in: the tenant hasn't purchased the use + // of this service (but may have purchased the use of other services in + // the same provider). + ErrServiceNotEnabled = &Err{msg: "service not enabled"} +) + +// As is a quality-of-life wrapper around errors.As, to retrieve the core.Err +// out of any arbitrary error. +func As(err error) (*Err, bool) { + if err == nil { + return nil, false + } + + var ( + ce *Err + ok = errors.As(err, &ce) + ) + + if !ok { + return nil, ok + } + + return ce, ok +} diff --git a/src/pkg/errs/core/core_test.go b/src/pkg/errs/core/core_test.go new file mode 100644 index 000000000..1ae6a4f9c --- /dev/null +++ b/src/pkg/errs/core/core_test.go @@ -0,0 +1,91 @@ +package core_test + +import ( + "testing" + + "github.com/alcionai/clues" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/suite" + + "github.com/alcionai/corso/src/internal/tester" + "github.com/alcionai/corso/src/pkg/errs/core" +) + +type ErrUnitSuite struct { + tester.Suite +} + +func TestErrUnitSuite(t *testing.T) { + suite.Run(t, &ErrUnitSuite{Suite: tester.NewUnitSuite(t)}) +} + +func (suite *ErrUnitSuite) TestAs() { + // shorthand reference for ease of reading + cErr := core.ErrApplicationThrottled + adHoc := &core.Err{} + + table := []struct { + name string + err error + expectOK assert.BoolAssertionFunc + expectErr func(t *testing.T, ce *core.Err) + }{ + { + name: "nil", + err: nil, + expectOK: assert.False, + expectErr: func(t *testing.T, ce *core.Err) { + assert.Nil(t, ce) + }, + }, + { + name: "non-matching", + err: assert.AnError, + expectOK: assert.False, + expectErr: func(t *testing.T, ce *core.Err) { + assert.Nil(t, ce) + }, + }, + { + name: "matching", + err: cErr, + expectOK: assert.True, + expectErr: func(t *testing.T, ce *core.Err) { + assert.Equal(t, cErr, ce) + }, + }, + { + name: "adHoc", + err: adHoc, + expectOK: assert.True, + expectErr: func(t *testing.T, ce *core.Err) { + assert.Equal(t, adHoc, ce) + }, + }, + { + name: "stacked", + err: clues.Stack(assert.AnError, cErr, assert.AnError), + expectOK: assert.True, + expectErr: func(t *testing.T, ce *core.Err) { + assert.Equal(t, cErr, ce) + }, + }, + { + name: "wrapped", + err: clues.Wrap(cErr, "wrapper"), + expectOK: assert.True, + expectErr: func(t *testing.T, ce *core.Err) { + assert.Equal(t, cErr, ce) + }, + }, + } + for _, test := range table { + suite.Run(test.name, func() { + t := suite.T() + err, ok := core.As(test.err) + + test.expectOK(t, ok) + test.expectErr(t, err) + }) + } +} diff --git a/src/pkg/errs/errs.go b/src/pkg/errs/errs.go index de7256aad..ef973b123 100644 --- a/src/pkg/errs/errs.go +++ b/src/pkg/errs/errs.go @@ -3,35 +3,17 @@ package errs import ( "errors" + "github.com/alcionai/corso/src/pkg/errs/core" "github.com/alcionai/corso/src/pkg/repository" "github.com/alcionai/corso/src/pkg/services/m365/api/graph" ) -// expose enums, rather than errors, for Is checks. The enum should -// map to a specific internal error that can be used for the actual -// errors.Is comparison. -type errEnum string - -const ( - ApplicationThrottled errEnum = "application-throttled" - BackupNotFound errEnum = "backup-not-found" - InsufficientAuthorization errEnum = "insufficient-authorization" - RepoAlreadyExists errEnum = "repository-already-exists" - ResourceNotAccessible errEnum = "resource-not-accesible" - ResourceOwnerNotFound errEnum = "resource-owner-not-found" - ServiceNotEnabled errEnum = "service-not-enabled" -) - // 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 externalToInternal = map[errEnum][]error{ - ApplicationThrottled: {graph.ErrApplicationThrottled}, - BackupNotFound: {repository.ErrorBackupNotFound}, - RepoAlreadyExists: {repository.ErrorRepoAlreadyExists}, - ResourceNotAccessible: {graph.ErrResourceLocked}, - ResourceOwnerNotFound: {graph.ErrResourceOwnerNotFound}, - ServiceNotEnabled: {graph.ErrServiceNotEnabled}, +// internal errors. +var externalToInternal = map[*core.Err][]error{ + core.ErrBackupNotFound: {repository.ErrorBackupNotFound}, + core.ErrRepoAlreadyExists: {repository.ErrorRepoAlreadyExists}, + core.ErrResourceNotAccessible: {graph.ErrResourceLocked}, } type ErrCheck func(error) bool @@ -41,23 +23,27 @@ type ErrCheck func(error) bool // 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}, - InsufficientAuthorization: {graph.IsErrInsufficientAuthorization}, +var externalToInternalCheck = map[*core.Err][]ErrCheck{ + core.ErrApplicationThrottled: {graph.IsErrApplicationThrottled}, + core.ErrResourceNotAccessible: {graph.IsErrResourceLocked}, + core.ErrResourceOwnerNotFound: {graph.IsErrItemNotFound}, + core.ErrInsufficientAuthorization: {graph.IsErrInsufficientAuthorization}, } // 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] +func Internal(ce *core.Err) ([]error, []ErrCheck) { + return externalToInternal[ce], externalToInternalCheck[ce] } // 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 := externalToInternal[enum] +func Is(err error, ce *core.Err) bool { + if errors.Is(err, ce) { + return true + } + + internalErrs, ok := externalToInternal[ce] if ok { for _, target := range internalErrs { if errors.Is(err, target) { @@ -66,7 +52,7 @@ func Is(err error, enum errEnum) bool { } } - internalChecks, ok := externalToInternalCheck[enum] + internalChecks, ok := externalToInternalCheck[ce] if ok { for _, check := range internalChecks { if check(err) { diff --git a/src/pkg/errs/errs_test.go b/src/pkg/errs/errs_test.go index 7846609de..c2ccb1813 100644 --- a/src/pkg/errs/errs_test.go +++ b/src/pkg/errs/errs_test.go @@ -8,6 +8,7 @@ import ( "github.com/stretchr/testify/suite" "github.com/alcionai/corso/src/internal/tester" + "github.com/alcionai/corso/src/pkg/errs/core" "github.com/alcionai/corso/src/pkg/repository" "github.com/alcionai/corso/src/pkg/services/m365/api/graph" graphTD "github.com/alcionai/corso/src/pkg/services/m365/api/graph/testdata" @@ -23,36 +24,24 @@ func TestErrUnitSuite(t *testing.T) { func (suite *ErrUnitSuite) TestInternal_errs() { table := []struct { - get errEnum + get *core.Err expect []error }{ { - get: ApplicationThrottled, - expect: []error{graph.ErrApplicationThrottled}, - }, - { - get: RepoAlreadyExists, + get: core.ErrRepoAlreadyExists, expect: []error{repository.ErrorRepoAlreadyExists}, }, { - get: BackupNotFound, + get: core.ErrBackupNotFound, expect: []error{repository.ErrorBackupNotFound}, }, { - get: ServiceNotEnabled, - expect: []error{graph.ErrServiceNotEnabled}, - }, - { - get: ResourceOwnerNotFound, - expect: []error{graph.ErrResourceOwnerNotFound}, - }, - { - get: ResourceNotAccessible, + get: core.ErrResourceNotAccessible, expect: []error{graph.ErrResourceLocked}, }, } for _, test := range table { - suite.Run(string(test.get), func() { + suite.Run(test.get.Error(), func() { // can't compare func signatures errs, _ := Internal(test.get) assert.ElementsMatch(suite.T(), test.expect, errs) @@ -62,57 +51,56 @@ func (suite *ErrUnitSuite) TestInternal_errs() { func (suite *ErrUnitSuite) TestInternal_checks() { table := []struct { - get errEnum + get *core.Err err error expectHasChecks assert.ValueAssertionFunc expect assert.BoolAssertionFunc }{ { - get: ApplicationThrottled, - err: graph.ErrApplicationThrottled, + get: core.ErrApplicationThrottled, + err: graphTD.ODataErr(string(graph.ApplicationThrottled)), expectHasChecks: assert.NotEmpty, expect: assert.True, }, { - get: RepoAlreadyExists, - err: graph.ErrApplicationThrottled, + get: core.ErrRepoAlreadyExists, + err: graphTD.ODataErr(string(graph.ApplicationThrottled)), expectHasChecks: assert.Empty, expect: assert.False, }, { - get: BackupNotFound, + get: core.ErrBackupNotFound, 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, + get: core.ErrResourceOwnerNotFound, + err: graphTD.ODataErr(string(graph.ItemNotFound)), expectHasChecks: assert.NotEmpty, - expect: assert.False, + expect: assert.True, }, { - get: ResourceNotAccessible, + get: core.ErrResourceOwnerNotFound, + err: graphTD.ODataErr(string(graph.ErrorItemNotFound)), + expectHasChecks: assert.NotEmpty, + expect: assert.True, + }, + { + get: core.ErrResourceNotAccessible, err: graph.ErrResourceLocked, expectHasChecks: assert.NotEmpty, expect: assert.True, }, { - get: InsufficientAuthorization, + get: core.ErrInsufficientAuthorization, err: graphTD.ODataErr(string(graph.AuthorizationRequestDenied)), expectHasChecks: assert.NotEmpty, expect: assert.True, }, } for _, test := range table { - suite.Run(string(test.get), func() { + suite.Run(test.get.Error(), func() { t := suite.T() _, checks := Internal(test.get) @@ -135,40 +123,28 @@ func (suite *ErrUnitSuite) TestInternal_checks() { func (suite *ErrUnitSuite) TestIs() { table := []struct { - target errEnum + target *core.Err err error }{ { - target: ApplicationThrottled, - err: graph.ErrApplicationThrottled, - }, - { - target: RepoAlreadyExists, + target: core.ErrRepoAlreadyExists, err: repository.ErrorRepoAlreadyExists, }, { - target: BackupNotFound, + target: core.ErrBackupNotFound, err: repository.ErrorBackupNotFound, }, { - target: ServiceNotEnabled, - err: graph.ErrServiceNotEnabled, - }, - { - target: ResourceOwnerNotFound, - err: graph.ErrResourceOwnerNotFound, - }, - { - target: ResourceNotAccessible, + target: core.ErrResourceNotAccessible, err: graph.ErrResourceLocked, }, { - target: InsufficientAuthorization, + target: core.ErrInsufficientAuthorization, err: graphTD.ODataErr(string(graph.AuthorizationRequestDenied)), }, } for _, test := range table { - suite.Run(string(test.target), func() { + suite.Run(test.target.Error(), func() { var ( w = clues.Wrap(test.err, "wrap") s = clues.Stack(test.err) diff --git a/src/pkg/services/m365/api/graph/errors.go b/src/pkg/services/m365/api/graph/errors.go index c84bfcc98..76ed542a4 100644 --- a/src/pkg/services/m365/api/graph/errors.go +++ b/src/pkg/services/m365/api/graph/errors.go @@ -30,7 +30,7 @@ import ( type errorCode string const ( - applicationThrottled errorCode = "ApplicationThrottled" + ApplicationThrottled errorCode = "ApplicationThrottled" // this authN error is a catch-all used by graph in a variety of cases: // users without licenses, bad jwts, missing account permissions, etc. AuthenticationError errorCode = "AuthenticationError" @@ -43,7 +43,7 @@ const ( cannotOpenFileAttachment errorCode = "ErrorCannotOpenFileAttachment" emailFolderNotFound errorCode = "ErrorSyncFolderNotFound" ErrorAccessDenied errorCode = "ErrorAccessDenied" - errorItemNotFound errorCode = "ErrorItemNotFound" + ErrorItemNotFound errorCode = "ErrorItemNotFound" // This error occurs when an email is enumerated but retrieving it fails // - we believe - due to it pre-dating mailbox creation. Possible explanations // are mailbox creation racing with email receipt or a similar issue triggered @@ -57,7 +57,7 @@ const ( // that doesn't exist. invalidUser errorCode = "ErrorInvalidUser" invalidAuthenticationToken errorCode = "InvalidAuthenticationToken" - itemNotFound errorCode = "itemNotFound" + ItemNotFound errorCode = "itemNotFound" MailboxNotEnabledForRESTAPI errorCode = "MailboxNotEnabledForRESTAPI" malwareDetected errorCode = "malwareDetected" // nameAlreadyExists occurs when a request with @@ -104,11 +104,10 @@ const ( LabelsSkippable = "skippable_errors" ) +// These errors are graph specific. That means they don't have a clear parallel in +// pkg/errs/core. If these errors need to trickle outward to non-m365 layers, we +// need to find a sufficiently coarse errs/core sentinel to use as transformation. var ( - // ErrApplicationThrottled occurs if throttling retries are exhausted and completely - // fails out. - ErrApplicationThrottled = clues.New("application throttled") - // The folder or item was deleted between the time we identified // it and when we tried to fetch data for it. ErrDeletedInFlight = clues.New("deleted in flight") @@ -131,18 +130,11 @@ var ( // This makes the resource inaccessible for any Corso operations. ErrResourceLocked = clues.New("resource has been locked and must be unlocked by an administrator") - // ErrServiceNotEnabled identifies that a resource owner does not have - // access to a given service. - ErrServiceNotEnabled = clues.New("service is not enabled for that resource owner") - - ErrResourceOwnerNotFound = clues.New("resource owner not found in tenant") - ErrTokenExpired = clues.New("jwt token expired") ) func IsErrApplicationThrottled(err error) bool { - return errors.Is(err, ErrApplicationThrottled) || - parseODataErr(err).hasErrorCode(err, applicationThrottled) + return parseODataErr(err).hasErrorCode(err, ApplicationThrottled) } func IsErrAuthenticationError(err error) bool { @@ -160,8 +152,8 @@ func IsErrDeletedInFlight(err error) bool { if parseODataErr(err).hasErrorCode( err, - errorItemNotFound, - itemNotFound, + ErrorItemNotFound, + ItemNotFound, syncFolderNotFound) { return true } @@ -170,7 +162,7 @@ func IsErrDeletedInFlight(err error) bool { } func IsErrItemNotFound(err error) bool { - return parseODataErr(err).hasErrorCode(err, itemNotFound, errorItemNotFound) + return parseODataErr(err).hasErrorCode(err, ItemNotFound, ErrorItemNotFound) } func IsErrInvalidDelta(err error) bool { @@ -188,7 +180,7 @@ func IsErrQuotaExceeded(err error) bool { func IsErrExchangeMailFolderNotFound(err error) bool { // Not sure if we can actually see a resourceNotFound error here. I've only // seen the latter two. - return parseODataErr(err).hasErrorCode(err, ResourceNotFound, errorItemNotFound, MailboxNotEnabledForRESTAPI) + return parseODataErr(err).hasErrorCode(err, ResourceNotFound, ErrorItemNotFound, MailboxNotEnabledForRESTAPI) } func IsErrUserNotFound(err error) bool { diff --git a/src/pkg/services/m365/api/graph/errors_test.go b/src/pkg/services/m365/api/graph/errors_test.go index 8de476abd..129bffb2e 100644 --- a/src/pkg/services/m365/api/graph/errors_test.go +++ b/src/pkg/services/m365/api/graph/errors_test.go @@ -79,7 +79,7 @@ func (suite *GraphErrorsUnitSuite) TestIsErrApplicationThrottled() { }, { name: "applicationThrottled oDataErr", - err: graphTD.ODataErr(string(applicationThrottled)), + err: graphTD.ODataErr(string(ApplicationThrottled)), expect: assert.True, }, } @@ -186,7 +186,7 @@ func (suite *GraphErrorsUnitSuite) TestIsErrDeletedInFlight() { }, { name: "not-found oDataErr", - err: graphTD.ODataErr(string(errorItemNotFound)), + err: graphTD.ODataErr(string(ErrorItemNotFound)), expect: assert.True, }, { @@ -858,12 +858,12 @@ func (suite *GraphErrorsUnitSuite) TestIsErrItemNotFound() { }, { name: "item not found oDataErr", - err: graphTD.ODataErr(string(itemNotFound)), + err: graphTD.ODataErr(string(ItemNotFound)), expect: assert.True, }, { name: "error item not found oDataErr", - err: graphTD.ODataErr(string(errorItemNotFound)), + err: graphTD.ODataErr(string(ErrorItemNotFound)), expect: assert.True, }, } diff --git a/src/pkg/services/m365/api/graph/http_wrapper.go b/src/pkg/services/m365/api/graph/http_wrapper.go index 8093738a0..789ec30d4 100644 --- a/src/pkg/services/m365/api/graph/http_wrapper.go +++ b/src/pkg/services/m365/api/graph/http_wrapper.go @@ -14,6 +14,7 @@ import ( "github.com/alcionai/corso/src/internal/events" "github.com/alcionai/corso/src/internal/version" "github.com/alcionai/corso/src/pkg/count" + "github.com/alcionai/corso/src/pkg/errs/core" "github.com/alcionai/corso/src/pkg/logger" ) @@ -120,7 +121,7 @@ func (hw httpWrapper) Request( } if IsErrApplicationThrottled(err) { - return nil, Stack(ctx, clues.Stack(ErrApplicationThrottled, err)) + return nil, Stack(ctx, clues.Stack(core.ErrApplicationThrottled, err)) } var http2StreamErr http2.StreamError diff --git a/src/pkg/services/m365/api/graph/service.go b/src/pkg/services/m365/api/graph/service.go index e005030bd..685af34ec 100644 --- a/src/pkg/services/m365/api/graph/service.go +++ b/src/pkg/services/m365/api/graph/service.go @@ -18,6 +18,7 @@ import ( "github.com/alcionai/corso/src/internal/common/idname" "github.com/alcionai/corso/src/internal/events" "github.com/alcionai/corso/src/pkg/count" + "github.com/alcionai/corso/src/pkg/errs/core" "github.com/alcionai/corso/src/pkg/filters" "github.com/alcionai/corso/src/pkg/logger" "github.com/alcionai/corso/src/pkg/path" @@ -395,7 +396,7 @@ func (aw *adapterWrap) Send( // those retries are well handled in middleware already. We want to ensure // that the error gets wrapped with the appropriate sentinel here. if IsErrApplicationThrottled(err) { - return nil, clues.StackWC(ictx, ErrApplicationThrottled, err).WithTrace(1) + return nil, clues.StackWC(ictx, core.ErrApplicationThrottled, err).WithTrace(1) } // exit most errors without retry diff --git a/src/pkg/services/m365/api/groups.go b/src/pkg/services/m365/api/groups.go index 3419ad63f..1c289a93f 100644 --- a/src/pkg/services/m365/api/groups.go +++ b/src/pkg/services/m365/api/groups.go @@ -15,6 +15,7 @@ import ( "github.com/alcionai/corso/src/internal/common/ptr" "github.com/alcionai/corso/src/internal/common/str" "github.com/alcionai/corso/src/internal/common/tform" + "github.com/alcionai/corso/src/pkg/errs/core" "github.com/alcionai/corso/src/pkg/fault" "github.com/alcionai/corso/src/pkg/logger" "github.com/alcionai/corso/src/pkg/services/m365/api/graph" @@ -204,7 +205,7 @@ func getGroupFromResponse(ctx context.Context, resp models.GroupCollectionRespon vs := resp.GetValue() if len(vs) == 0 { - return nil, clues.StackWC(ctx, graph.ErrResourceOwnerNotFound) + return nil, clues.StackWC(ctx, core.ErrResourceOwnerNotFound) } else if len(vs) > 1 { return nil, clues.StackWC(ctx, graph.ErrMultipleResultsMatchIdentifier) } diff --git a/src/pkg/services/m365/api/groups_test.go b/src/pkg/services/m365/api/groups_test.go index 5cd2bb299..f48496484 100644 --- a/src/pkg/services/m365/api/groups_test.go +++ b/src/pkg/services/m365/api/groups_test.go @@ -14,6 +14,7 @@ import ( "github.com/alcionai/corso/src/internal/common/ptr" "github.com/alcionai/corso/src/internal/tester" "github.com/alcionai/corso/src/internal/tester/tconfig" + "github.com/alcionai/corso/src/pkg/errs/core" "github.com/alcionai/corso/src/pkg/fault" "github.com/alcionai/corso/src/pkg/services/m365/api/graph" graphTD "github.com/alcionai/corso/src/pkg/services/m365/api/graph/testdata" @@ -201,7 +202,7 @@ func (suite *GroupsIntgSuite) TestGroups_GetByID() { name: "invalid id", id: uuid.NewString(), expectErr: func(t *testing.T, err error) { - assert.ErrorIs(t, err, graph.ErrResourceOwnerNotFound, clues.ToCore(err)) + assert.ErrorIs(t, err, core.ErrResourceOwnerNotFound, clues.ToCore(err)) }, }, { @@ -215,7 +216,7 @@ func (suite *GroupsIntgSuite) TestGroups_GetByID() { name: "invalid displayName", id: "jabberwocky", expectErr: func(t *testing.T, err error) { - assert.ErrorIs(t, err, graph.ErrResourceOwnerNotFound, clues.ToCore(err)) + assert.ErrorIs(t, err, core.ErrResourceOwnerNotFound, clues.ToCore(err)) }, }, } diff --git a/src/pkg/services/m365/api/sites.go b/src/pkg/services/m365/api/sites.go index 2736ef1fd..7ce927239 100644 --- a/src/pkg/services/m365/api/sites.go +++ b/src/pkg/services/m365/api/sites.go @@ -13,6 +13,7 @@ import ( "github.com/microsoftgraph/msgraph-sdk-go/sites" "github.com/alcionai/corso/src/internal/common/ptr" + "github.com/alcionai/corso/src/pkg/errs/core" "github.com/alcionai/corso/src/pkg/fault" "github.com/alcionai/corso/src/pkg/services/m365/api/graph" ) @@ -153,7 +154,7 @@ func (c Sites) GetByID( // a 404 when getting sites by ID returns an itemNotFound // error code, instead of something more sensible. if graph.IsErrItemNotFound(err) { - err = clues.Stack(graph.ErrResourceOwnerNotFound, err) + err = clues.Stack(core.ErrResourceOwnerNotFound, err) } if graph.IsErrResourceLocked(err) { @@ -199,7 +200,7 @@ func (c Sites) GetByID( // a 404 when getting sites by ID returns an itemNotFound // error code, instead of something more sensible. if graph.IsErrItemNotFound(err) { - err = clues.Stack(graph.ErrResourceOwnerNotFound, err) + err = clues.Stack(core.ErrResourceOwnerNotFound, err) } if graph.IsErrResourceLocked(err) { diff --git a/src/pkg/services/m365/api/sites_test.go b/src/pkg/services/m365/api/sites_test.go index 5a4e4c938..aa42c60c3 100644 --- a/src/pkg/services/m365/api/sites_test.go +++ b/src/pkg/services/m365/api/sites_test.go @@ -14,8 +14,8 @@ import ( "github.com/alcionai/corso/src/internal/common/ptr" "github.com/alcionai/corso/src/internal/tester" "github.com/alcionai/corso/src/internal/tester/tconfig" + "github.com/alcionai/corso/src/pkg/errs/core" "github.com/alcionai/corso/src/pkg/fault" - "github.com/alcionai/corso/src/pkg/services/m365/api/graph" ) type SitesUnitSuite struct { @@ -181,7 +181,7 @@ func (suite *SitesIntgSuite) TestSites_GetByID() { name: "random id", id: uuid.NewString() + "," + uuid.NewString(), expectErr: func(t *testing.T, err error) bool { - assert.ErrorIs(t, err, graph.ErrResourceOwnerNotFound, clues.ToCore(err)) + assert.ErrorIs(t, err, core.ErrResourceOwnerNotFound, clues.ToCore(err)) return true }, }, @@ -213,7 +213,7 @@ func (suite *SitesIntgSuite) TestSites_GetByID() { name: "well formed url, no sites match", id: modifiedSiteURL, expectErr: func(t *testing.T, err error) bool { - assert.ErrorIs(t, err, graph.ErrResourceOwnerNotFound, clues.ToCore(err)) + assert.ErrorIs(t, err, core.ErrResourceOwnerNotFound, clues.ToCore(err)) return true }, }, diff --git a/src/pkg/services/m365/api/users.go b/src/pkg/services/m365/api/users.go index c7489b423..a48b250f0 100644 --- a/src/pkg/services/m365/api/users.go +++ b/src/pkg/services/m365/api/users.go @@ -13,6 +13,7 @@ import ( "github.com/alcionai/corso/src/internal/common/idname" "github.com/alcionai/corso/src/internal/common/ptr" + "github.com/alcionai/corso/src/pkg/errs/core" "github.com/alcionai/corso/src/pkg/fault" "github.com/alcionai/corso/src/pkg/services/m365/api/graph" ) @@ -198,7 +199,7 @@ func EvaluateMailboxError(err error) error { // must occur before MailFolderNotFound, due to overlapping cases. if graph.IsErrUserNotFound(err) { - return clues.Stack(graph.ErrResourceOwnerNotFound, err) + return clues.Stack(core.ErrResourceOwnerNotFound, err) } if graph.IsErrResourceLocked(err) { diff --git a/src/pkg/services/m365/api/users_test.go b/src/pkg/services/m365/api/users_test.go index 2fdf6056b..eab3520ab 100644 --- a/src/pkg/services/m365/api/users_test.go +++ b/src/pkg/services/m365/api/users_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/suite" "github.com/alcionai/corso/src/internal/tester" + "github.com/alcionai/corso/src/pkg/errs/core" "github.com/alcionai/corso/src/pkg/services/m365/api/graph" graphTD "github.com/alcionai/corso/src/pkg/services/m365/api/graph/testdata" ) @@ -82,7 +83,7 @@ func (suite *UsersUnitSuite) TestEvaluateMailboxError() { name: "mail inbox err - user not found", err: graphTD.ODataErr(string(graph.RequestResourceNotFound)), expect: func(t *testing.T, err error) { - assert.ErrorIs(t, err, graph.ErrResourceOwnerNotFound, clues.ToCore(err)) + assert.ErrorIs(t, err, core.ErrResourceOwnerNotFound, clues.ToCore(err)) }, }, { diff --git a/src/pkg/services/m365/groups_test.go b/src/pkg/services/m365/groups_test.go index 806abac06..a76753782 100644 --- a/src/pkg/services/m365/groups_test.go +++ b/src/pkg/services/m365/groups_test.go @@ -12,8 +12,8 @@ import ( "github.com/alcionai/corso/src/internal/tester" "github.com/alcionai/corso/src/internal/tester/tconfig" "github.com/alcionai/corso/src/pkg/errs" + "github.com/alcionai/corso/src/pkg/errs/core" "github.com/alcionai/corso/src/pkg/fault" - "github.com/alcionai/corso/src/pkg/services/m365/api/graph" ) type GroupsIntgSuite struct { @@ -108,8 +108,8 @@ func (suite *GroupsIntgSuite) TestGroupByID_notFound() { group, err := suite.cli.GroupByID(ctx, uuid.NewString()) require.Nil(t, group) - require.ErrorIs(t, err, graph.ErrResourceOwnerNotFound, clues.ToCore(err)) - require.True(t, errs.Is(err, errs.ResourceOwnerNotFound)) + require.ErrorIs(t, err, core.ErrResourceOwnerNotFound, clues.ToCore(err)) + require.True(t, errs.Is(err, core.ErrResourceOwnerNotFound)) } func (suite *GroupsIntgSuite) TestGroups() { diff --git a/src/pkg/services/m365/sites.go b/src/pkg/services/m365/sites.go index d5b91ccab..fc3d3666d 100644 --- a/src/pkg/services/m365/sites.go +++ b/src/pkg/services/m365/sites.go @@ -10,6 +10,7 @@ import ( "github.com/alcionai/corso/src/internal/common/ptr" "github.com/alcionai/corso/src/internal/common/str" "github.com/alcionai/corso/src/internal/common/tform" + "github.com/alcionai/corso/src/pkg/errs/core" "github.com/alcionai/corso/src/pkg/fault" "github.com/alcionai/corso/src/pkg/logger" "github.com/alcionai/corso/src/pkg/services/m365/api" @@ -87,7 +88,7 @@ func getAllSites( sites, err := ga.GetAll(ctx, fault.New(true)) if err != nil { if clues.HasLabel(err, graph.LabelsNoSharePointLicense) { - return nil, clues.Stack(graph.ErrServiceNotEnabled, err) + return nil, clues.Stack(core.ErrServiceNotEnabled, err) } return nil, clues.Wrap(err, "retrieving sites") diff --git a/src/pkg/services/m365/sites_test.go b/src/pkg/services/m365/sites_test.go index 3fa68d21c..e9da2f080 100644 --- a/src/pkg/services/m365/sites_test.go +++ b/src/pkg/services/m365/sites_test.go @@ -14,6 +14,7 @@ import ( "github.com/alcionai/corso/src/internal/common/ptr" "github.com/alcionai/corso/src/internal/tester" "github.com/alcionai/corso/src/internal/tester/tconfig" + "github.com/alcionai/corso/src/pkg/errs/core" "github.com/alcionai/corso/src/pkg/fault" "github.com/alcionai/corso/src/pkg/services/m365/api" "github.com/alcionai/corso/src/pkg/services/m365/api/graph" @@ -145,7 +146,7 @@ func (suite *siteUnitSuite) TestGetAllSites() { return mockGASites{nil, graph.Stack(ctx, odErr)} }, expectErr: func(t *testing.T, err error) { - assert.ErrorIs(t, err, graph.ErrServiceNotEnabled, clues.ToCore(err)) + assert.ErrorIs(t, err, core.ErrServiceNotEnabled, clues.ToCore(err)) }, }, {