fix up naming, comments

CHANGELOG.md

@@ -29,6 +29,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Contacts in-place restore failed if the restore destination was empty.
 - Link shares with external users are now backed up and restored as expected
 - Ensure persistent repo config is populated on repo init if repo init failed partway through during the previous init attempt.
+- User-not-found and mailbox-not-found errors no longer collide when checking backup capacity for mailboxes.
 
 ### Changed
 - When running `backup details` on an empty backup returns a more helpful error message.

src/internal/m365/service/exchange/enabled.go

@@ -23,12 +23,13 @@ func IsServiceEnabled(
 ) (bool, error) {
 	_, err := gmi.GetMailInbox(ctx, resource)
 	if err != nil {
-		if err := api.EvaluateMailboxError(err); err != nil {
+		ignorable := api.IsMailboxErrorIgnorable(err)
+		if !ignorable {
 			logger.CtxErr(ctx, err).Error("getting user's mail folder")
 			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
 	}
@@ -54,13 +55,13 @@ func GetMailboxInfo(
 	// First check whether the user is able to access their inbox.
 	inbox, err := gmb.GetMailInbox(ctx, userID)
 	if err != nil {
-		if err := api.EvaluateMailboxError(clues.Stack(err)); err != nil {
+		ignorable := api.IsMailboxErrorIgnorable(clues.Stack(err))
+		if !ignorable {
 			logger.CtxErr(ctx, err).Error("getting user's mail folder")
-
-			return mi, err
+			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,

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

src/pkg/services/m365/api/graph/errors.go

@@ -127,6 +127,12 @@ var (
 // error categorization
 // ---------------------------------------------------------------------------
 
+// 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 {
 		return nil
@@ -140,8 +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):
-		err = clues.Stack(core.ErrNotFound, err)
+	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(ErrResourceNotFound, core.ErrNotFound, err)
 	case isErrResourceLocked(ode, err):
 		err = clues.Stack(core.ErrResourceNotAccessible, err)
 	case isErrInsufficientAuthorization(ode, err):
@@ -204,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
 	}

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))
 		})
 	}
 }
@@ -1111,7 +1111,6 @@ func (suite *GraphErrorsUnitSuite) TestToErrByRespCode() {
 	for _, test := range table {
 		suite.Run(test.name, func() {
 			t := suite.T()
-
 			err := toErrByRespCode(parseODataErr(test.err), test.err)
 
 			if test.expectNoStack {
@@ -1122,3 +1121,96 @@ func (suite *GraphErrorsUnitSuite) TestToErrByRespCode() {
 		})
 	}
 }
+
+func (suite *GraphErrorsUnitSuite) TestIsErrItemAlreadyExists() {
+	table := []struct {
+		name   string
+		err    error
+		expect assert.BoolAssertionFunc
+	}{
+		{
+			name:   "nil",
+			err:    nil,
+			expect: assert.False,
+		},
+		{
+			name:   "non-matching",
+			err:    assert.AnError,
+			expect: assert.False,
+		},
+		{
+			name:   "non-matching oDataErr",
+			err:    graphTD.ODataErrWithMsg("InvalidRequest", "item already exists"),
+			expect: assert.False,
+		},
+		{
+			name:   "matching oDataErr code",
+			err:    graphTD.ODataInner(string(nameAlreadyExists)),
+			expect: assert.True,
+		},
+	}
+	for _, test := range table {
+		suite.Run(test.name, func() {
+			ode := parseODataErr(test.err)
+			test.expect(suite.T(), isErrItemAlreadyExists(ode, test.err))
+		})
+	}
+}
+
+func (suite *GraphErrorsUnitSuite) TestStackWithCoreErr() {
+	table := []struct {
+		name   string
+		err    error
+		expect []error
+	}{
+		{
+			name:   "bad jwt",
+			err:    graphTD.ODataErr(string(invalidAuthenticationToken)),
+			expect: []error{core.ErrAuthTokenExpired},
+		},
+		{
+			name:   "throttled",
+			err:    graphTD.ODataErr(string(ApplicationThrottled)),
+			expect: []error{core.ErrApplicationThrottled},
+		},
+		{
+			name:   "user not found",
+			err:    graphTD.ODataErrWithMsg(string(ResourceNotFound), "User not found"),
+			expect: []error{ErrResourceNotFound, core.ErrNotFound},
+		},
+		{
+			name:   "resource locked",
+			err:    graphTD.ODataErr(string(NotAllowed)),
+			expect: []error{core.ErrResourceNotAccessible},
+		},
+		{
+			name:   "insufficient auth",
+			err:    graphTD.ODataErr(string(AuthorizationRequestDenied)),
+			expect: []error{core.ErrInsufficientAuthorization},
+		},
+		{
+			name:   "already exists",
+			err:    graphTD.ODataInner(string(nameAlreadyExists)),
+			expect: []error{core.ErrAlreadyExists},
+		},
+		{
+			name:   "not found",
+			err:    graphTD.ODataErr(string(ItemNotFound)),
+			expect: []error{core.ErrNotFound},
+		},
+	}
+	for _, test := range table {
+		suite.Run(test.name, func() {
+			t := suite.T()
+
+			ctx, flush := tester.NewContext(t)
+			defer flush()
+
+			result := stackWithCoreErr(ctx, test.err, 1)
+
+			for _, ex := range test.expect {
+				assert.ErrorIs(t, result, ex, clues.ToCore(result))
+			}
+		})
+	}
+}

src/pkg/services/m365/api/users.go

@@ -185,26 +185,19 @@ func appendIfErr(errs []error, err error) []error {
 	return append(errs, err)
 }
 
-// EvaluateMailboxError 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.
-func EvaluateMailboxError(err error) error {
-	if err == nil {
-		return nil
-	}
-
+// 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) {
-		return err
+	if errors.Is(err, core.ErrResourceNotAccessible) || errors.Is(err, graph.ErrResourceNotFound) {
+		return false
 	}
 
-	if errors.Is(err, core.ErrNotFound) ||
+	return err != nil && (errors.Is(err, core.ErrNotFound) ||
 		graph.IsErrExchangeMailFolderNotFound(err) ||
-		graph.IsErrAuthenticationError(err) {
-		return nil
-	}
-
-	return err
+		graph.IsErrAuthenticationError(err))
 }
 
 // IsAnyErrMailboxNotFound inspects the secondary errors inside MailboxInfo and

src/pkg/services/m365/api/users_test.go

@@ -70,54 +70,47 @@ func (suite *UsersUnitSuite) TestEvaluateMailboxError() {
 	table := []struct {
 		name   string
 		err    error
-		expect func(t *testing.T, err error)
+		expect assert.BoolAssertionFunc
 	}{
 		{
-			name: "nil",
-			err:  nil,
-			expect: func(t *testing.T, err error) {
-				assert.NoError(t, err, clues.ToCore(err))
-			},
+			name:   "nil",
+			err:    nil,
+			expect: assert.False,
 		},
 		{
-			name: "mail inbox err - user not found",
-			err:  core.ErrNotFound,
-			expect: func(t *testing.T, err error) {
-				assert.NoError(t, err, clues.ToCore(err))
-			},
+			name:   "user not found - corso sentinel",
+			err:    graph.ErrResourceNotFound,
+			expect: assert.False,
 		},
 		{
-			name: "mail inbox err - resourceLocked",
-			err:  core.ErrResourceNotAccessible,
-			expect: func(t *testing.T, err error) {
-				assert.ErrorIs(t, err, core.ErrResourceNotAccessible, clues.ToCore(err))
-			},
+			name:   "not found - corso sentinel",
+			err:    core.ErrNotFound,
+			expect: assert.True,
 		},
 		{
-			name: "mail inbox err - user not found",
-			err:  graphTD.ODataErr(string(graph.MailboxNotEnabledForRESTAPI)),
-			expect: func(t *testing.T, err error) {
-				assert.NoError(t, err, clues.ToCore(err))
-			},
+			name:   "mailbox not enabled - graph api error",
+			err:    graphTD.ODataErr(string(graph.MailboxNotEnabledForRESTAPI)),
+			expect: assert.True,
 		},
 		{
-			name: "mail inbox err - authenticationError",
-			err:  graphTD.ODataErr(string(graph.AuthenticationError)),
-			expect: func(t *testing.T, err error) {
-				assert.NoError(t, err, clues.ToCore(err))
-			},
+			name:   "resourceLocked",
+			err:    core.ErrResourceNotAccessible,
+			expect: assert.False,
 		},
 		{
-			name: "mail inbox err - other error",
-			err:  graphTD.ODataErrWithMsg("somecode", "somemessage"),
-			expect: func(t *testing.T, err error) {
-				assert.Error(t, err, clues.ToCore(err))
-			},
+			name:   "authenticationError",
+			err:    graphTD.ODataErr(string(graph.AuthenticationError)),
+			expect: assert.True,
+		},
+		{
+			name:   "other error",
+			err:    graphTD.ODataErrWithMsg("somecode", "somemessage"),
+			expect: assert.False,
 		},
 	}
 	for _, test := range table {
 		suite.Run(test.name, func() {
-			test.expect(suite.T(), EvaluateMailboxError(test.err))
+			test.expect(suite.T(), IsMailboxErrorIgnorable(test.err))
 		})
 	}
 }