diff --git a/src/internal/m365/service/exchange/enabled.go b/src/internal/m365/service/exchange/enabled.go index d15d01bab..fc3daea04 100644 --- a/src/internal/m365/service/exchange/enabled.go +++ b/src/internal/m365/service/exchange/enabled.go @@ -29,7 +29,7 @@ func IsServiceEnabled( return false, clues.Stack(err) } - logger.Ctx(ctx).Info("resource owner does not have a mailbox enabled") + logger.CtxErr(ctx, err).Info("resource owner does not have a mailbox enabled") return false, nil } @@ -61,7 +61,7 @@ func GetMailboxInfo( return mi, clues.Stack(err) } - logger.Ctx(ctx).Info("resource owner does not have a mailbox enabled") + logger.CtxErr(ctx, err).Info("resource owner does not have a mailbox enabled") mi.ErrGetMailBoxSetting = append( mi.ErrGetMailBoxSetting, diff --git a/src/pkg/errs/core/core.go b/src/pkg/errs/core/core.go index 259d8a2a4..253901e59 100644 --- a/src/pkg/errs/core/core.go +++ b/src/pkg/errs/core/core.go @@ -16,8 +16,8 @@ import "errors" // // 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". +// to group concepts into broad categories. Ex: prefer "not found" over "resource not +// found", and "resource not found" over "user not found". // // 3. Always Stack/Wrap core.Errs. Only once. // `return core.ErrFoo` should be avoided. Also, if you're handling a error returned diff --git a/src/pkg/services/m365/api/graph/errors.go b/src/pkg/services/m365/api/graph/errors.go index 2c5ddb408..9f8336648 100644 --- a/src/pkg/services/m365/api/graph/errors.go +++ b/src/pkg/services/m365/api/graph/errors.go @@ -127,7 +127,11 @@ var ( // error categorization // --------------------------------------------------------------------------- -var ErrUserNotFound = clues.New("user not found") +// ErrResourceNotFound is a special case handler to differentiate from the +// more generic core.ErrNotFound. Two rules for usage, to preserve sanity: +// 1. it should get stacked on top of a clues.NotFound error. +// 2. it should not be investigated above the api layer. +var ErrResourceNotFound = clues.New("resource not found") func stackWithCoreErr(ctx context.Context, err error, traceDepth int) error { if err == nil { @@ -142,11 +146,11 @@ func stackWithCoreErr(ctx context.Context, err error, traceDepth int) error { err = clues.Stack(core.ErrAuthTokenExpired) case isErrApplicationThrottled(ode, err): err = clues.Stack(core.ErrApplicationThrottled, err) - case isErrUserNotFound(ode, err): - // stack both userNotFound and notFound to ensure some graph api + case isErrResourceNotFound(ode, err): + // stack both resourceNotFound and notFound to ensure some graph api // internals can distinguish between the two cases (where possible). // layers above the api should still handle only the core notFound. - err = clues.Stack(ErrUserNotFound, core.ErrNotFound, err) + err = clues.Stack(ErrResourceNotFound, core.ErrNotFound, err) case isErrResourceLocked(ode, err): err = clues.Stack(core.ErrResourceNotAccessible, err) case isErrInsufficientAuthorization(ode, err): @@ -209,7 +213,7 @@ func isErrNotFound(ode oDataErr, err error) bool { notFound) } -func isErrUserNotFound(ode oDataErr, err error) bool { +func isErrResourceNotFound(ode oDataErr, err error) bool { if ode.hasErrorCode(err, RequestResourceNotFound, invalidUser) { return true } diff --git a/src/pkg/services/m365/api/graph/errors_test.go b/src/pkg/services/m365/api/graph/errors_test.go index 47c6b3904..5ab39eeb0 100644 --- a/src/pkg/services/m365/api/graph/errors_test.go +++ b/src/pkg/services/m365/api/graph/errors_test.go @@ -549,7 +549,7 @@ func (suite *GraphErrorsUnitSuite) TestIsErrUserNotFound() { for _, test := range table { suite.Run(test.name, func() { ode := parseODataErr(test.err) - test.expect(suite.T(), isErrUserNotFound(ode, test.err)) + test.expect(suite.T(), isErrResourceNotFound(ode, test.err)) }) } } @@ -1176,7 +1176,7 @@ func (suite *GraphErrorsUnitSuite) TestStackWithCoreErr() { { name: "user not found", err: graphTD.ODataErrWithMsg(string(ResourceNotFound), "User not found"), - expect: []error{ErrUserNotFound, core.ErrNotFound}, + expect: []error{ErrResourceNotFound, core.ErrNotFound}, }, { name: "resource locked", diff --git a/src/pkg/services/m365/api/users.go b/src/pkg/services/m365/api/users.go index 2f0a6e790..27eb9c6ff 100644 --- a/src/pkg/services/m365/api/users.go +++ b/src/pkg/services/m365/api/users.go @@ -185,12 +185,13 @@ func appendIfErr(errs []error, err error) []error { return append(errs, err) } -// IsMailboxErrorIgnorable checks whether the provided error can be interpreted -// as "user does not have a mailbox", or whether it is some other error. If -// the former (no mailbox), returns nil, otherwise returns an error. +// IsMailboxErrorIgnorable checks whether the provided error (which is assumed to +// have originated from a call to retrieve a user's mailbox) can be safely ignored +// or not. In particular, the igorable cases are when the mail folder is not found +// and when an authentication issue occurs. func IsMailboxErrorIgnorable(err error) bool { // must occur before MailFolderNotFound, due to overlapping cases. - if errors.Is(err, core.ErrResourceNotAccessible) || errors.Is(err, graph.ErrUserNotFound) { + if errors.Is(err, core.ErrResourceNotAccessible) || errors.Is(err, graph.ErrResourceNotFound) { return false } diff --git a/src/pkg/services/m365/api/users_test.go b/src/pkg/services/m365/api/users_test.go index 48ff275b4..5117ed5bc 100644 --- a/src/pkg/services/m365/api/users_test.go +++ b/src/pkg/services/m365/api/users_test.go @@ -79,7 +79,7 @@ func (suite *UsersUnitSuite) TestEvaluateMailboxError() { }, { name: "user not found - corso sentinel", - err: graph.ErrUserNotFound, + err: graph.ErrResourceNotFound, expect: assert.False, }, {