diff --git a/src/cli/backup/helpers_test.go b/src/cli/backup/helpers_test.go index 775a7ee68..ee4214006 100644 --- a/src/cli/backup/helpers_test.go +++ b/src/cli/backup/helpers_test.go @@ -25,7 +25,6 @@ import ( "github.com/alcionai/corso/src/pkg/repository" "github.com/alcionai/corso/src/pkg/services/m365/api" "github.com/alcionai/corso/src/pkg/services/m365/api/graph" - gmock "github.com/alcionai/corso/src/pkg/services/m365/api/graph/mock" "github.com/alcionai/corso/src/pkg/storage" "github.com/alcionai/corso/src/pkg/storage/testdata" ) @@ -37,12 +36,12 @@ import ( // GockClient produces a new exchange api client that can be // mocked using gock. func gockClient(creds account.M365Config, counter *count.Bus) (api.Client, error) { - s, err := gmock.NewService(creds, counter) + s, err := graph.NewGockService(creds, counter) if err != nil { return api.Client{}, err } - li, err := gmock.NewService(creds, counter, graph.NoTimeout()) + li, err := graph.NewGockService(creds, counter, graph.NoTimeout()) if err != nil { return api.Client{}, err } diff --git a/src/internal/m365/controller.go b/src/internal/m365/controller.go index fc81ea2ef..8da8ec072 100644 --- a/src/internal/m365/controller.go +++ b/src/internal/m365/controller.go @@ -255,15 +255,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(core.ErrResourceOwnerNotFound, err) - } - - if graph.IsErrResourceLocked(err) { - return nil, clues.Stack(graph.ErrResourceLocked, err) - } - - return nil, err + return nil, clues.Stack(err) } if len(id) == 0 || len(name) == 0 { diff --git a/src/internal/m365/service/exchange/enabled_test.go b/src/internal/m365/service/exchange/enabled_test.go index ca40aebc9..c37c61148 100644 --- a/src/internal/m365/service/exchange/enabled_test.go +++ b/src/internal/m365/service/exchange/enabled_test.go @@ -10,6 +10,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" "github.com/alcionai/corso/src/pkg/services/m365/api/graph" graphTD "github.com/alcionai/corso/src/pkg/services/m365/api/graph/testdata" @@ -91,9 +92,10 @@ func (suite *EnabledUnitSuite) TestIsServiceEnabled() { name: "overlapping resourcenotfound", mock: func(ctx context.Context) getMailInboxer { odErr := graphTD.ODataErrWithMsg(string(graph.ResourceNotFound), "User not found") + err := clues.Stack(core.ErrResourceOwnerNotFound, odErr) return mockGMB{ - mailboxErr: graph.Stack(ctx, odErr), + mailboxErr: graph.Stack(ctx, err), } }, expect: assert.False, diff --git a/src/internal/m365/service/onedrive/enabled.go b/src/internal/m365/service/onedrive/enabled.go index 42b8d7b69..1c3fbf3d0 100644 --- a/src/internal/m365/service/onedrive/enabled.go +++ b/src/internal/m365/service/onedrive/enabled.go @@ -6,7 +6,6 @@ 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,14 +26,6 @@ func IsServiceEnabled( return false, nil } - if graph.IsErrUserNotFound(err) { - return false, clues.Stack(core.ErrResourceOwnerNotFound, err) - } - - if graph.IsErrResourceLocked(err) { - return false, clues.Stack(graph.ErrResourceLocked, err) - } - return false, clues.Stack(err) } diff --git a/src/internal/operations/test/m365/helper.go b/src/internal/operations/test/m365/helper.go index 559a0f57a..dc58e5b77 100644 --- a/src/internal/operations/test/m365/helper.go +++ b/src/internal/operations/test/m365/helper.go @@ -34,7 +34,6 @@ import ( "github.com/alcionai/corso/src/pkg/selectors" "github.com/alcionai/corso/src/pkg/services/m365/api" "github.com/alcionai/corso/src/pkg/services/m365/api/graph" - gmock "github.com/alcionai/corso/src/pkg/services/m365/api/graph/mock" ) // --------------------------------------------------------------------------- @@ -44,12 +43,12 @@ import ( // GockClient produces a new exchange api client that can be // mocked using gock. func GockClient(creds account.M365Config, counter *count.Bus) (api.Client, error) { - s, err := gmock.NewService(creds, counter) + s, err := graph.NewGockService(creds, counter) if err != nil { return api.Client{}, err } - li, err := gmock.NewService(creds, counter, graph.NoTimeout()) + li, err := graph.NewGockService(creds, counter, graph.NoTimeout()) if err != nil { return api.Client{}, err } diff --git a/src/pkg/errs/errs.go b/src/pkg/errs/errs.go index ef973b123..3f48b317f 100644 --- a/src/pkg/errs/errs.go +++ b/src/pkg/errs/errs.go @@ -24,10 +24,7 @@ type ErrCheck func(error) bool // checks. This allows us to apply those comparison checks instead of relying // only on sentinels. var externalToInternalCheck = map[*core.Err][]ErrCheck{ - core.ErrApplicationThrottled: {graph.IsErrApplicationThrottled}, - core.ErrResourceNotAccessible: {graph.IsErrResourceLocked}, - core.ErrResourceOwnerNotFound: {graph.IsErrItemNotFound}, - core.ErrInsufficientAuthorization: {graph.IsErrInsufficientAuthorization}, + core.ErrResourceOwnerNotFound: {graph.IsErrItemNotFound}, } // Internal returns the internal errors and error checking functions which diff --git a/src/pkg/errs/errs_test.go b/src/pkg/errs/errs_test.go index c2ccb1813..507ddc8ee 100644 --- a/src/pkg/errs/errs_test.go +++ b/src/pkg/errs/errs_test.go @@ -56,12 +56,6 @@ func (suite *ErrUnitSuite) TestInternal_checks() { expectHasChecks assert.ValueAssertionFunc expect assert.BoolAssertionFunc }{ - { - get: core.ErrApplicationThrottled, - err: graphTD.ODataErr(string(graph.ApplicationThrottled)), - expectHasChecks: assert.NotEmpty, - expect: assert.True, - }, { get: core.ErrRepoAlreadyExists, err: graphTD.ODataErr(string(graph.ApplicationThrottled)), @@ -86,18 +80,6 @@ func (suite *ErrUnitSuite) TestInternal_checks() { expectHasChecks: assert.NotEmpty, expect: assert.True, }, - { - get: core.ErrResourceNotAccessible, - err: graph.ErrResourceLocked, - expectHasChecks: assert.NotEmpty, - expect: assert.True, - }, - { - get: core.ErrInsufficientAuthorization, - err: graphTD.ODataErr(string(graph.AuthorizationRequestDenied)), - expectHasChecks: assert.NotEmpty, - expect: assert.True, - }, } for _, test := range table { suite.Run(test.get.Error(), func() { @@ -138,10 +120,6 @@ func (suite *ErrUnitSuite) TestIs() { target: core.ErrResourceNotAccessible, err: graph.ErrResourceLocked, }, - { - target: core.ErrInsufficientAuthorization, - err: graphTD.ODataErr(string(graph.AuthorizationRequestDenied)), - }, } for _, test := range table { suite.Run(test.target.Error(), func() { diff --git a/src/pkg/services/m365/api/graph/errors.go b/src/pkg/services/m365/api/graph/errors.go index 140c09617..0c6543bba 100644 --- a/src/pkg/services/m365/api/graph/errors.go +++ b/src/pkg/services/m365/api/graph/errors.go @@ -18,6 +18,7 @@ import ( "github.com/alcionai/corso/src/internal/common/jwt" "github.com/alcionai/corso/src/internal/common/ptr" "github.com/alcionai/corso/src/internal/common/str" + "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/services/m365/custom" @@ -105,6 +106,10 @@ const ( LabelsSkippable = "skippable_errors" ) +// --------------------------------------------------------------------------- +// error sentinels & categorization +// --------------------------------------------------------------------------- + // 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. @@ -134,7 +139,26 @@ var ( ErrTokenExpired = clues.New("jwt token expired") ) -func IsErrApplicationThrottled(err error) bool { +func stackWithCoreErr(ctx context.Context, err error, traceDepth int) error { + if err == nil { + return nil + } + + switch { + case isErrApplicationThrottled(err): + err = clues.Stack(core.ErrApplicationThrottled, err) + case isErrResourceLocked(err): + err = clues.Stack(core.ErrResourceNotAccessible, err) + case isErrUserNotFound(err): + err = clues.Stack(core.ErrResourceOwnerNotFound, err) + case isErrInsufficientAuthorization(err): + err = clues.Stack(core.ErrInsufficientAuthorization, err) + } + + return stackWithDepth(ctx, err, 1+traceDepth) +} + +func isErrApplicationThrottled(err error) bool { return parseODataErr(err).hasErrorCode(err, ApplicationThrottled) } @@ -142,7 +166,7 @@ func IsErrAuthenticationError(err error) bool { return parseODataErr(err).hasErrorCode(err, AuthenticationError) } -func IsErrInsufficientAuthorization(err error) bool { +func isErrInsufficientAuthorization(err error) bool { return parseODataErr(err).hasErrorCode(err, AuthorizationRequestDenied) } @@ -189,7 +213,7 @@ func IsErrExchangeMailFolderNotFound(err error) bool { return parseODataErr(err).hasErrorCode(err, ResourceNotFound, ErrorItemNotFound, MailboxNotEnabledForRESTAPI) } -func IsErrUserNotFound(err error) bool { +func isErrUserNotFound(err error) bool { ode := parseODataErr(err) if ode.hasErrorCode(err, RequestResourceNotFound, invalidUser) { @@ -281,7 +305,7 @@ func IsErrSiteNotFound(err error) bool { return parseODataErr(err).hasErrorMessage(err, requestedSiteCouldNotBeFound) } -func IsErrResourceLocked(err error) bool { +func isErrResourceLocked(err error) bool { ode := parseODataErr(err) return errors.Is(err, ErrResourceLocked) || @@ -298,6 +322,10 @@ func IsErrSharingDisabled(err error) bool { return parseODataErr(err).hasInnerErrorCode(err, sharingDisabled) } +// --------------------------------------------------------------------------- +// quality of life wrappers +// --------------------------------------------------------------------------- + // Wrap is a helper function that extracts ODataError metadata from // the error. If the error is not an ODataError type, returns the error. func Wrap(ctx context.Context, e error, msg string) *clues.Err { @@ -324,6 +352,10 @@ func Wrap(ctx context.Context, e error, msg string) *clues.Err { // Stack is a helper function that extracts ODataError metadata from // the error. If the error is not an ODataError type, returns the error. func Stack(ctx context.Context, e error) *clues.Err { + return stackWithDepth(ctx, e, 1) +} + +func stackWithDepth(ctx context.Context, e error, traceDepth int) *clues.Err { if e == nil { return nil } @@ -339,7 +371,7 @@ func Stack(ctx context.Context, e error) *clues.Err { ce := clues.StackWC(ctx, e). With("graph_api_err", ode). - WithTrace(1) + WithTrace(1 + traceDepth) return setLabels(ce, ode) } diff --git a/src/pkg/services/m365/api/graph/errors_test.go b/src/pkg/services/m365/api/graph/errors_test.go index 23dd4ddb7..8e2c4ff5d 100644 --- a/src/pkg/services/m365/api/graph/errors_test.go +++ b/src/pkg/services/m365/api/graph/errors_test.go @@ -85,7 +85,7 @@ func (suite *GraphErrorsUnitSuite) TestIsErrApplicationThrottled() { } for _, test := range table { suite.Run(test.name, func() { - test.expect(suite.T(), IsErrApplicationThrottled(test.err)) + test.expect(suite.T(), isErrApplicationThrottled(test.err)) }) } } @@ -153,7 +153,7 @@ func (suite *GraphErrorsUnitSuite) TestIsErrInsufficientAuthorization() { } for _, test := range table { suite.Run(test.name, func() { - test.expect(suite.T(), IsErrInsufficientAuthorization(test.err)) + test.expect(suite.T(), isErrInsufficientAuthorization(test.err)) }) } } @@ -481,7 +481,7 @@ func (suite *GraphErrorsUnitSuite) TestIsErrUserNotFound() { } for _, test := range table { suite.Run(test.name, func() { - test.expect(suite.T(), IsErrUserNotFound(test.err)) + test.expect(suite.T(), isErrUserNotFound(test.err)) }) } } @@ -968,7 +968,7 @@ func (suite *GraphErrorsUnitSuite) TestIsErrResourceLocked() { } for _, test := range table { suite.Run(test.name, func() { - test.expect(suite.T(), IsErrResourceLocked(test.err)) + test.expect(suite.T(), isErrResourceLocked(test.err)) }) } } diff --git a/src/pkg/services/m365/api/graph/http_wrapper.go b/src/pkg/services/m365/api/graph/http_wrapper.go index 789ec30d4..737d2b61c 100644 --- a/src/pkg/services/m365/api/graph/http_wrapper.go +++ b/src/pkg/services/m365/api/graph/http_wrapper.go @@ -14,7 +14,6 @@ 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" ) @@ -103,7 +102,9 @@ func (hw httpWrapper) Request( // See https://learn.microsoft.com/en-us/sharepoint/dev/general-development/how-to-avoid-getting-throttled-or-blocked-in-sharepoint-online#how-to-decorate-your-http-traffic req.Header.Set("User-Agent", "ISV|Alcion|Corso/"+version.Version) - var resp *http.Response + retriedErrors := []string{} + + var e error // stream errors from http/2 will fail before we reach // client middleware handling, therefore we don't get to @@ -112,36 +113,41 @@ func (hw httpWrapper) Request( // retry in the event of a `stream error`, which is not // a common expectation. for i := 0; i < hw.config.maxConnectionRetries+1; i++ { + if i > 0 { + time.Sleep(3 * time.Second) + } + ctx = clues.Add(ctx, "request_retry_iter", i) - resp, err = hw.client.Do(req) - + resp, err := hw.client.Do(req) if err == nil { - break + logResp(ctx, resp) + return resp, nil } - if IsErrApplicationThrottled(err) { - return nil, Stack(ctx, clues.Stack(core.ErrApplicationThrottled, err)) - } + err = stackWithCoreErr(ctx, err, 1) + e = err var http2StreamErr http2.StreamError if !errors.As(err, &http2StreamErr) { - return nil, Stack(ctx, err) + // exit most errors without retry + break } logger.Ctx(ctx).Debug("http2 stream error") events.Inc(events.APICall, "streamerror") - time.Sleep(3 * time.Second) + retriedErrors = append(retriedErrors, err.Error()) } - if err != nil { - return nil, Stack(ctx, err) - } + e = clues.Stack(e). + With("retried_errors", retriedErrors). + WithTrace(1). + OrNil() - logResp(ctx, resp) - - return resp, nil + // no chance of a non-error return here. + // we handle that inside the loop. + return nil, e } // --------------------------------------------------------------------------- diff --git a/src/pkg/services/m365/api/graph/http_wrapper_test.go b/src/pkg/services/m365/api/graph/http_wrapper_test.go index 604ca5e9d..5e274c35c 100644 --- a/src/pkg/services/m365/api/graph/http_wrapper_test.go +++ b/src/pkg/services/m365/api/graph/http_wrapper_test.go @@ -182,7 +182,6 @@ func (suite *HTTPWrapperUnitSuite) TestNewHTTPWrapper_http2StreamErrorRetries() _, err := hw.Request(ctx, http.MethodGet, url, nil, nil) require.ErrorAs(t, err, &http2.StreamError{}, clues.ToCore(err)) - require.Equal(t, test.expectRetries, tries, "count of retries") }) } diff --git a/src/pkg/services/m365/api/graph/service.go b/src/pkg/services/m365/api/graph/service.go index ca1e6f680..a656b89ad 100644 --- a/src/pkg/services/m365/api/graph/service.go +++ b/src/pkg/services/m365/api/graph/service.go @@ -18,7 +18,6 @@ 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" @@ -381,13 +380,15 @@ func (aw *adapterWrap) Send( requestInfo *abstractions.RequestInformation, constructor serialization.ParsableFactory, errorMappings abstractions.ErrorMappings, -) (sp serialization.Parsable, err error) { +) (sp serialization.Parsable, e error) { defer func() { if crErr := crash.Recovery(ctx, recover(), "graph adapter request"); crErr != nil { - err = Stack(ctx, crErr) + e = Stack(ctx, crErr) } }() + retriedErrors := []string{} + // This external retry wrapper is unsophisticated, but should // only retry under certain circumstances // 1. stream errors from http/2, which will fail before we reach @@ -395,41 +396,47 @@ func (aw *adapterWrap) Send( // 2. jwt token invalidation, which requires a re-auth that's handled // in the Send() call, before reaching client middleware. for i := 0; i < aw.config.maxConnectionRetries+1; i++ { + if i > 0 { + time.Sleep(aw.retryDelay) + } + ictx := clues.Add(ctx, "request_retry_iter", i) - sp, err = aw.RequestAdapter.Send(ictx, requestInfo, constructor, errorMappings) + resp, err := aw.RequestAdapter.Send(ictx, requestInfo, constructor, errorMappings) if err == nil { - break + return resp, nil } - // force an early exit on throttling issues. - // 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, core.ErrApplicationThrottled, err).WithTrace(1) - } + err = stackWithCoreErr(ictx, err, 1) + e = err - // exit most errors without retry - switch { - case IsErrConnectionReset(err) || connectionEnded.Compare(err.Error()): + if IsErrConnectionReset(err) || connectionEnded.Compare(err.Error()) { logger.Ctx(ictx).Debug("http connection error") events.Inc(events.APICall, "connectionerror") - case IsErrBadJWTToken(err): + } else if IsErrBadJWTToken(err) { logger.Ctx(ictx).Debug("bad jwt token") events.Inc(events.APICall, "badjwttoken") - case requestInfo.Method.String() == http.MethodGet && IsErrInvalidRequest(err): + } else if requestInfo.Method.String() == http.MethodGet && IsErrInvalidRequest(err) { // Graph may sometimes return a transient 400 response during onedrive // and sharepoint backup. This is pending investigation on msft end, retry // for now as it's a transient issue. Restrict retries to GET requests only // to limit the scope of this fix. logger.Ctx(ictx).Debug("invalid request") - events.Inc(events.APICall, "invalidrequest") - default: - return nil, clues.StackWC(ictx, err).WithTrace(1) + events.Inc(events.APICall, "invalidgetrequest") + } else { + // exit most errors without retry + break } - time.Sleep(aw.retryDelay) + retriedErrors = append(retriedErrors, err.Error()) } - return sp, clues.Stack(err).OrNil() + e = clues.Stack(e). + With("retried_errors", retriedErrors). + WithTrace(1). + OrNil() + + // no chance of a non-error return here. + // we handle that inside the loop. + return nil, e } diff --git a/src/pkg/services/m365/api/graph/mock/service.go b/src/pkg/services/m365/api/graph/service_mock.go similarity index 55% rename from src/pkg/services/m365/api/graph/mock/service.go rename to src/pkg/services/m365/api/graph/service_mock.go index 0456e3693..96373c198 100644 --- a/src/pkg/services/m365/api/graph/mock/service.go +++ b/src/pkg/services/m365/api/graph/service_mock.go @@ -1,21 +1,21 @@ -package mock +package graph import ( "github.com/alcionai/clues" "github.com/h2non/gock" + abstractions "github.com/microsoft/kiota-abstractions-go" msgraphsdkgo "github.com/microsoftgraph/msgraph-sdk-go" "github.com/alcionai/corso/src/pkg/account" "github.com/alcionai/corso/src/pkg/count" - "github.com/alcionai/corso/src/pkg/services/m365/api/graph" ) -func NewService( +func NewGockService( creds account.M365Config, counter *count.Bus, - opts ...graph.Option, -) (*graph.Service, error) { - a, err := CreateAdapter( + opts ...Option, +) (*Service, error) { + a, err := CreateGockAdapter( creds.AzureTenantID, creds.AzureClientID, creds.AzureClientSecret, @@ -25,29 +25,31 @@ func NewService( return nil, clues.Wrap(err, "generating graph adapter") } - return graph.NewService(a), nil + return NewService(a), nil } -// CreateAdapter is similar to graph.CreateAdapter, but with option to +// CreateGockAdapter is similar to graph.CreateAdapter, but with option to // enable interceptions via gock to make it mockable. -func CreateAdapter( +func CreateGockAdapter( tenant, client, secret string, counter *count.Bus, - opts ...graph.Option, -) (*msgraphsdkgo.GraphRequestAdapter, error) { - auth, err := graph.GetAuth(tenant, client, secret) + opts ...Option, +) (abstractions.RequestAdapter, error) { + auth, err := GetAuth(tenant, client, secret) if err != nil { return nil, err } - httpClient, _ := graph.KiotaHTTPClient(counter, opts...) + httpClient, cc := KiotaHTTPClient(counter, opts...) // This makes sure that we are able to intercept any requests via // gock. Only necessary for testing. gock.InterceptClient(httpClient) - return msgraphsdkgo.NewGraphRequestAdapterWithParseNodeFactoryAndSerializationWriterFactoryAndHttpClient( + ng, err := msgraphsdkgo.NewGraphRequestAdapterWithParseNodeFactoryAndSerializationWriterFactoryAndHttpClient( auth, nil, nil, httpClient) + + return wrapAdapter(ng, cc), clues.Stack(err).OrNil() } diff --git a/src/pkg/services/m365/api/groups.go b/src/pkg/services/m365/api/groups.go index 7e2628ee3..9bdedcb94 100644 --- a/src/pkg/services/m365/api/groups.go +++ b/src/pkg/services/m365/api/groups.go @@ -11,6 +11,7 @@ import ( msgraphgocore "github.com/microsoftgraph/msgraph-sdk-go-core" "github.com/microsoftgraph/msgraph-sdk-go/groups" "github.com/microsoftgraph/msgraph-sdk-go/models" + "github.com/pkg/errors" "github.com/alcionai/corso/src/internal/common/idname" "github.com/alcionai/corso/src/internal/common/ptr" @@ -133,10 +134,6 @@ func (c Groups) GetTeamByID( t, err := c.Stable.Client().Teams().ByTeamId(identifier).Get(ctx, nil) if err != nil { - if graph.IsErrResourceLocked(err) { - return nil, graph.Stack(ctx, clues.Stack(graph.ErrResourceLocked, err)) - } - return nil, graph.Wrap(ctx, err, "finding team by ID") } @@ -172,8 +169,8 @@ func (c Groups) GetByID( return group, nil } - if graph.IsErrResourceLocked(err) { - return nil, graph.Stack(ctx, clues.Stack(graph.ErrResourceLocked, err)) + if errors.Is(err, core.ErrResourceNotAccessible) { + return nil, err } logger.CtxErr(ctx, err).Info("finding group by id, falling back to secondary identifier") @@ -194,8 +191,8 @@ func (c Groups) GetByID( return getGroupFromResponse(ctx, resp) } - if graph.IsErrResourceLocked(err) { - err = clues.Stack(graph.ErrResourceLocked, err) + if errors.Is(err, core.ErrResourceNotAccessible) { + return nil, err } logger.CtxErr(ctx, err).Info("finding group by email, falling back to display name") @@ -211,10 +208,6 @@ func (c Groups) GetByID( resp, err := c.Stable.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/groups_test.go b/src/pkg/services/m365/api/groups_test.go index e3f19f7c7..54871bdb3 100644 --- a/src/pkg/services/m365/api/groups_test.go +++ b/src/pkg/services/m365/api/groups_test.go @@ -7,6 +7,7 @@ import ( "github.com/google/uuid" "github.com/h2non/gock" "github.com/microsoftgraph/msgraph-sdk-go/models" + "github.com/microsoftgraph/msgraph-sdk-go/models/odataerrors" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" @@ -261,57 +262,30 @@ func (suite *GroupsIntgSuite) TestGroups_GetByID_mockResourceLockedErrs() { gID := uuid.NewString() table := []struct { - name string - id string - setup func(t *testing.T) + name string + id string + err *odataerrors.ODataError }{ { name: "by name", id: "g", - setup: func(t *testing.T) { - err := graphTD.ODataErr(string(graph.NotAllowed)) - - interceptV1Path("groups"). - Reply(403). - JSON(graphTD.ParseableToMap(t, err)) - interceptV1Path("teams"). - Reply(403). - JSON(graphTD.ParseableToMap(t, err)) - }, + err: graphTD.ODataErr(string(graph.NotAllowed)), }, { name: "by id", id: gID, - setup: func(t *testing.T) { - err := graphTD.ODataErr(string(graph.NotAllowed)) - - interceptV1Path("groups", gID). - Reply(403). - JSON(graphTD.ParseableToMap(t, err)) - interceptV1Path("teams", gID). - Reply(403). - JSON(graphTD.ParseableToMap(t, err)) - }, + err: graphTD.ODataErr(string(graph.NotAllowed)), }, { name: "by id - matches error message", id: gID, - setup: func(t *testing.T) { - err := graphTD.ODataErrWithMsg( - string(graph.AuthenticationError), - "AADSTS500014: The service principal for resource 'beefe6b7-f5df-413d-ac2d-abf1e3fd9c0b' "+ - "is disabled. This indicate that a subscription within the tenant has lapsed, or that the "+ - "administrator for this tenant has disabled the application, preventing tokens from being "+ - "issued for it. Trace ID: dead78e1-0830-4edf-bea7-f0a445620100 Correlation ID: "+ - "deadbeef-7f1e-4578-8215-36004a2c935c Timestamp: 2023-12-05 19:31:01Z") - - interceptV1Path("groups", gID). - Reply(403). - JSON(graphTD.ParseableToMap(t, err)) - interceptV1Path("teams", gID). - Reply(403). - JSON(graphTD.ParseableToMap(t, err)) - }, + err: graphTD.ODataErrWithMsg( + string(graph.AuthenticationError), + "AADSTS500014: The service principal for resource 'beefe6b7-f5df-413d-ac2d-abf1e3fd9c0b' "+ + "is disabled. This indicate that a subscription within the tenant has lapsed, or that the "+ + "administrator for this tenant has disabled the application, preventing tokens from being "+ + "issued for it. Trace ID: dead78e1-0830-4edf-bea7-f0a445620100 Correlation ID: "+ + "deadbeef-7f1e-4578-8215-36004a2c935c Timestamp: 2023-12-05 19:31:01Z"), }, } for _, test := range table { @@ -323,18 +297,32 @@ func (suite *GroupsIntgSuite) TestGroups_GetByID_mockResourceLockedErrs() { defer flush() defer gock.Off() - test.setup(t) + interceptV1Path("groups", gID). + Reply(403). + JSON(graphTD.ParseableToMap(t, test.err)) + + interceptV1Path("groups"). + Reply(403). + JSON(graphTD.ParseableToMap(t, test.err)) // Verify both GetByID and GetTeamByID APIs handle this error _, err := suite.its.gockAC. Groups(). GetByID(ctx, test.id, CallConfig{}) - require.ErrorIs(t, err, graph.ErrResourceLocked, clues.ToCore(err)) + require.ErrorIs(t, err, core.ErrResourceNotAccessible, clues.ToCore(err)) + + interceptV1Path("teams", gID). + Reply(403). + JSON(graphTD.ParseableToMap(t, test.err)) + + interceptV1Path("teams"). + Reply(403). + JSON(graphTD.ParseableToMap(t, test.err)) _, err = suite.its.gockAC. Groups(). GetTeamByID(ctx, test.id, CallConfig{}) - require.ErrorIs(t, err, graph.ErrResourceLocked, clues.ToCore(err)) + require.ErrorIs(t, err, core.ErrResourceNotAccessible, clues.ToCore(err)) }) } } diff --git a/src/pkg/services/m365/api/helper_test.go b/src/pkg/services/m365/api/helper_test.go index 4708eaf38..2d993996e 100644 --- a/src/pkg/services/m365/api/helper_test.go +++ b/src/pkg/services/m365/api/helper_test.go @@ -15,7 +15,6 @@ import ( "github.com/alcionai/corso/src/pkg/control" "github.com/alcionai/corso/src/pkg/count" "github.com/alcionai/corso/src/pkg/services/m365/api/graph" - gmock "github.com/alcionai/corso/src/pkg/services/m365/api/graph/mock" ) // --------------------------------------------------------------------------- @@ -25,12 +24,12 @@ import ( // GockClient produces a new exchange api client that can be // mocked using gock. func gockClient(creds account.M365Config, counter *count.Bus) (Client, error) { - s, err := gmock.NewService(creds, counter) + s, err := graph.NewGockService(creds, counter) if err != nil { return Client{}, err } - li, err := gmock.NewService(creds, counter, graph.NoTimeout()) + li, err := graph.NewGockService(creds, counter, graph.NoTimeout()) if err != nil { return Client{}, err } diff --git a/src/pkg/services/m365/api/sites.go b/src/pkg/services/m365/api/sites.go index 7ce927239..6f4501656 100644 --- a/src/pkg/services/m365/api/sites.go +++ b/src/pkg/services/m365/api/sites.go @@ -157,10 +157,6 @@ func (c Sites) GetByID( err = clues.Stack(core.ErrResourceOwnerNotFound, err) } - if graph.IsErrResourceLocked(err) { - err = clues.Stack(graph.ErrResourceLocked, err) - } - return nil, err } @@ -203,10 +199,6 @@ func (c Sites) GetByID( err = clues.Stack(core.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 a48b250f0..41cce7f93 100644 --- a/src/pkg/services/m365/api/users.go +++ b/src/pkg/services/m365/api/users.go @@ -137,10 +137,6 @@ func (c Users) GetByID( ByUserId(identifier). Get(ctx, options) if err != nil { - if graph.IsErrResourceLocked(err) { - err = clues.Stack(graph.ErrResourceLocked, err) - } - return nil, graph.Stack(ctx, err) } @@ -198,12 +194,8 @@ func EvaluateMailboxError(err error) error { } // must occur before MailFolderNotFound, due to overlapping cases. - if graph.IsErrUserNotFound(err) { - return clues.Stack(core.ErrResourceOwnerNotFound, err) - } - - if graph.IsErrResourceLocked(err) { - return clues.Stack(graph.ErrResourceLocked, err) + if errors.Is(err, core.ErrResourceOwnerNotFound) || errors.Is(err, core.ErrResourceNotAccessible) { + return err } if graph.IsErrExchangeMailFolderNotFound(err) || graph.IsErrAuthenticationError(err) { diff --git a/src/pkg/services/m365/api/users_test.go b/src/pkg/services/m365/api/users_test.go index eab3520ab..d35eb78e5 100644 --- a/src/pkg/services/m365/api/users_test.go +++ b/src/pkg/services/m365/api/users_test.go @@ -81,16 +81,16 @@ func (suite *UsersUnitSuite) TestEvaluateMailboxError() { }, { name: "mail inbox err - user not found", - err: graphTD.ODataErr(string(graph.RequestResourceNotFound)), + err: core.ErrResourceOwnerNotFound, expect: func(t *testing.T, err error) { assert.ErrorIs(t, err, core.ErrResourceOwnerNotFound, clues.ToCore(err)) }, }, { name: "mail inbox err - resoruceLocked", - err: graphTD.ODataErr(string(graph.NotAllowed)), + err: core.ErrResourceNotAccessible, expect: func(t *testing.T, err error) { - assert.ErrorIs(t, err, graph.ErrResourceLocked, clues.ToCore(err)) + assert.ErrorIs(t, err, core.ErrResourceNotAccessible, clues.ToCore(err)) }, }, {