From b1af13b7b67099fc877dddbf095e2bff20c0531a Mon Sep 17 00:00:00 2001 From: Keepers Date: Wed, 10 Jan 2024 10:25:25 -0700 Subject: [PATCH] centralize core err interpretation (#4972) now that we have a core error library, we can start swapping out the percolation of graph api errors with core error sentinels. This PR begins that process by adding a centralized transformer to the graph service handlers that ensures all calls produce the same set of core errors according to some standard categorizations. This is just an initial step, and does not transfer all graph errors into core sentinels. --- #### 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/helpers_test.go | 5 +- src/internal/m365/controller.go | 10 +-- .../m365/service/exchange/enabled_test.go | 4 +- src/internal/m365/service/onedrive/enabled.go | 9 --- src/internal/operations/test/m365/helper.go | 5 +- src/pkg/errs/errs.go | 5 +- src/pkg/errs/errs_test.go | 22 ------ src/pkg/services/m365/api/graph/errors.go | 42 +++++++++-- .../services/m365/api/graph/errors_test.go | 8 +-- .../services/m365/api/graph/http_wrapper.go | 38 +++++----- .../m365/api/graph/http_wrapper_test.go | 1 - src/pkg/services/m365/api/graph/service.go | 49 +++++++------ .../{mock/service.go => service_mock.go} | 30 ++++---- src/pkg/services/m365/api/groups.go | 17 ++--- src/pkg/services/m365/api/groups_test.go | 72 ++++++++----------- src/pkg/services/m365/api/helper_test.go | 5 +- src/pkg/services/m365/api/sites.go | 8 --- src/pkg/services/m365/api/users.go | 12 +--- src/pkg/services/m365/api/users_test.go | 6 +- 19 files changed, 158 insertions(+), 190 deletions(-) rename src/pkg/services/m365/api/graph/{mock/service.go => service_mock.go} (55%) 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)) }, }, {