From f4c6e65fa326031055b1232e38fb0c27f0db3602 Mon Sep 17 00:00:00 2001 From: Keepers Date: Thu, 9 Mar 2023 17:55:33 -0700 Subject: [PATCH] remove common errors (#2597) ## Description Now that clues is in place, we don't need the errors.As handling provided by common errors. Current errors.As can be replaced with either errors.Is checks, or HasLabel checks on the status code. ## Does this PR need a docs update or release note? - [x] :no_entry: No ## Type of change - [x] :broom: Tech Debt/Cleanup ## Issue(s) * #1970 ## Test Plan - [x] :zap: Unit test - [x] :green_heart: E2E --- src/internal/common/errors.go | 62 -------- src/internal/common/errors_test.go | 84 ----------- .../exchange/exchange_data_collection_test.go | 5 +- .../exchange/service_iterators_test.go | 3 +- src/internal/connector/graph/errors.go | 133 +++++------------- src/internal/connector/graph/errors_test.go | 121 ++++++---------- src/internal/connector/onedrive/drive_test.go | 4 +- src/internal/connector/onedrive/item.go | 21 +-- 8 files changed, 87 insertions(+), 346 deletions(-) delete mode 100644 src/internal/common/errors.go delete mode 100644 src/internal/common/errors_test.go diff --git a/src/internal/common/errors.go b/src/internal/common/errors.go deleted file mode 100644 index 82dea7b38..000000000 --- a/src/internal/common/errors.go +++ /dev/null @@ -1,62 +0,0 @@ -package common - -import ( - "fmt" - "io" -) - -// TODO: Remove in favor of clues.Stack() - -// Err provides boiler-plate functions that other types of errors can use -// if they wish to be compared with `errors.As()`. This struct ensures that -// stack traces are printed when requested (if present) and that Err -// chains `errors.As()`, `errors.Is()`, and `errors.Cause()` calls properly. -// -// When using errors.As, note that the variable that is passed as the second -// parameter must be a pointer to a type that exactly matches the returned type of the error previously. For -// example, if a struct was returned, the second parameter should be a pointer -// to said struct. If a pointer to a struct was returned, then a pointer to a -// pointer of the struct should be passed. -type Err struct { - Err error -} - -func EncapsulateError(e error) *Err { - return &Err{Err: e} -} - -func (e Err) Error() string { - return e.Err.Error() -} - -func (e Err) Cause() error { - return e.Err -} - -func (e Err) Unwrap() error { - return e.Err -} - -// Format complies with the Formatter interface and gives pretty printing when -// functions like `fmt.Printf("%+v")` are called. Implementing this allows Err -// to print stack traces from the encapsulated error. -func (e Err) Format(s fmt.State, verb rune) { - if f, ok := e.Err.(fmt.Formatter); ok { - f.Format(s, verb) - return - } - - // Formatting magic courtesy of github.com/pkg/errors. - switch verb { - case 'v': - if s.Flag('+') { - fmt.Fprintf(s, "%+v\n", e.Cause()) - return - } - - fallthrough - case 's', 'q': - // nolint:errcheck - _, _ = io.WriteString(s, e.Error()) - } -} diff --git a/src/internal/common/errors_test.go b/src/internal/common/errors_test.go deleted file mode 100644 index 767f1515b..000000000 --- a/src/internal/common/errors_test.go +++ /dev/null @@ -1,84 +0,0 @@ -package common_test - -import ( - "fmt" - "testing" - - "github.com/pkg/errors" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/suite" - - "github.com/alcionai/corso/src/internal/common" - "github.com/alcionai/corso/src/internal/tester" -) - -type testErr struct { - common.Err -} - -type testErr2 struct { - common.Err -} - -type ErrorsUnitSuite struct { - tester.Suite -} - -func TestErrorsUnitSuite(t *testing.T) { - s := &ErrorsUnitSuite{Suite: tester.NewUnitSuite(t)} - suite.Run(t, s) -} - -func (suite *ErrorsUnitSuite) TestPropagatesCause() { - err := assert.AnError - te := testErr{*common.EncapsulateError(err)} - te2 := testErr2{*common.EncapsulateError(te)} - - assert.Equal(suite.T(), assert.AnError, errors.Cause(te2)) -} - -func (suite *ErrorsUnitSuite) TestPropagatesIs() { - err := assert.AnError - te := testErr{*common.EncapsulateError(err)} - te2 := testErr2{*common.EncapsulateError(te)} - - assert.True(suite.T(), errors.Is(te2, err)) -} - -func (suite *ErrorsUnitSuite) TestPropagatesAs() { - err := assert.AnError - te := testErr{*common.EncapsulateError(err)} - te2 := testErr2{*common.EncapsulateError(te)} - - tmp := testErr{} - assert.True(suite.T(), errors.As(te2, &tmp)) -} - -func (suite *ErrorsUnitSuite) TestAs() { - err := assert.AnError - te := testErr{*common.EncapsulateError(err)} - te2 := testErr2{*common.EncapsulateError(te)} - - tmp := testErr2{} - assert.True(suite.T(), errors.As(te2, &tmp)) -} - -func (suite *ErrorsUnitSuite) TestAsIsUnique() { - err := assert.AnError - te := testErr{*common.EncapsulateError(err)} - - tmp := testErr2{} - assert.False(suite.T(), errors.As(te, &tmp)) -} - -func (suite *ErrorsUnitSuite) TestPrintsStack() { - err := assert.AnError - err = errors.Wrap(err, "wrapped error") - te := testErr{*common.EncapsulateError(err)} - te2 := testErr2{*common.EncapsulateError(te)} - - out := fmt.Sprintf("%+v", te2) - - // Stack trace should include a line noting that we're running testify. - assert.Contains(suite.T(), out, "testify") -} diff --git a/src/internal/connector/exchange/exchange_data_collection_test.go b/src/internal/connector/exchange/exchange_data_collection_test.go index 4c75e773d..37afe792c 100644 --- a/src/internal/connector/exchange/exchange_data_collection_test.go +++ b/src/internal/connector/exchange/exchange_data_collection_test.go @@ -10,7 +10,6 @@ import ( "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" - "github.com/alcionai/corso/src/internal/common" "github.com/alcionai/corso/src/internal/connector/graph" "github.com/alcionai/corso/src/internal/data" "github.com/alcionai/corso/src/internal/tester" @@ -219,9 +218,7 @@ func (suite *ExchangeDataCollectionSuite) TestGetItemWithRetries() { { name: "deleted in flight", items: &mockItemer{ - getErr: graph.ErrDeletedInFlight{ - Err: *common.EncapsulateError(assert.AnError), - }, + getErr: graph.ErrDeletedInFlight, }, expectErr: func(t *testing.T, err error) { assert.True(t, graph.IsErrDeletedInFlight(err), "is ErrDeletedInFlight") diff --git a/src/internal/connector/exchange/service_iterators_test.go b/src/internal/connector/exchange/service_iterators_test.go index b9755432d..31c6ba962 100644 --- a/src/internal/connector/exchange/service_iterators_test.go +++ b/src/internal/connector/exchange/service_iterators_test.go @@ -9,7 +9,6 @@ import ( "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" - "github.com/alcionai/corso/src/internal/common" "github.com/alcionai/corso/src/internal/connector/exchange/api" "github.com/alcionai/corso/src/internal/connector/graph" "github.com/alcionai/corso/src/internal/connector/support" @@ -140,7 +139,7 @@ func (suite *ServiceIteratorsSuite) TestFilterContainersAndFillCollections() { added: []string{"a1", "a2", "a3"}, removed: []string{"r1", "r2", "r3"}, newDelta: api.DeltaUpdate{URL: "delta_url"}, - err: graph.ErrDeletedInFlight{Err: *common.EncapsulateError(assert.AnError)}, + err: graph.ErrDeletedInFlight, } container1 = mockContainer{ id: strPtr("1"), diff --git a/src/internal/connector/graph/errors.go b/src/internal/connector/graph/errors.go index ee345030c..6561a58dd 100644 --- a/src/internal/connector/graph/errors.go +++ b/src/internal/connector/graph/errors.go @@ -14,7 +14,6 @@ import ( "golang.org/x/exp/slices" "github.com/alcionai/clues" - "github.com/alcionai/corso/src/internal/common" "github.com/alcionai/corso/src/internal/common/ptr" "github.com/alcionai/corso/src/pkg/logger" ) @@ -37,18 +36,7 @@ const ( errCodeMailboxNotEnabledForRESTAPI = "MailboxNotEnabledForRESTAPI" ) -var ( - Err401Unauthorized = errors.New("401 unauthorized") - // normally the graph client will catch this for us, but in case we - // run our own client Do(), we need to translate it to a timeout type - // failure locally. - Err429TooManyRequests = errors.New("429 too many requests") - Err503ServiceUnavailable = errors.New("503 Service Unavailable") - Err504GatewayTimeout = errors.New("504 Gateway Timeout") - Err500InternalServerError = errors.New("500 Internal Server Error") -) - -var ( +const ( mysiteURLNotFound = "unable to retrieve user's mysite url" mysiteNotFound = "user's mysite not found" ) @@ -58,15 +46,25 @@ const ( LabelsMysiteNotFound = "mysite_not_found" ) -// The folder or item was deleted between the time we identified -// it and when we tried to fetch data for it. -type ErrDeletedInFlight struct { - common.Err -} +var ( + // 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") + + // Delta tokens can be desycned or expired. In either case, the token + // becomes invalid, and cannot be used again. + // https://learn.microsoft.com/en-us/graph/errors#code-property + ErrInvalidDelta = clues.New("inalid delta token") + + // Timeout errors are identified for tracking the need to retry calls. + // Other delay errors, like throttling, are already handled by the + // graph client's built-in retries. + // https://github.com/microsoftgraph/msgraph-sdk-go/issues/302 + ErrTimeout = clues.New("communication timeout") +) func IsErrDeletedInFlight(err error) bool { - e := ErrDeletedInFlight{} - if errors.As(err, &e) { + if errors.Is(err, ErrDeletedInFlight) { return true } @@ -82,24 +80,9 @@ func IsErrDeletedInFlight(err error) bool { return false } -// Delta tokens can be desycned or expired. In either case, the token -// becomes invalid, and cannot be used again. -// https://learn.microsoft.com/en-us/graph/errors#code-property -type ErrInvalidDelta struct { - common.Err -} - func IsErrInvalidDelta(err error) bool { - e := ErrInvalidDelta{} - if errors.As(err, &e) { - return true - } - - if hasErrorCode(err, errCodeSyncStateNotFound, errCodeResyncRequired) { - return true - } - - return false + return hasErrorCode(err, errCodeSyncStateNotFound, errCodeResyncRequired) || + errors.Is(err, ErrInvalidDelta) } func IsErrExchangeMailFolderNotFound(err error) bool { @@ -110,79 +93,30 @@ func IsErrUserNotFound(err error) bool { return hasErrorCode(err, errCodeRequestResourceNotFound) } -// Timeout errors are identified for tracking the need to retry calls. -// Other delay errors, like throttling, are already handled by the -// graph client's built-in retries. -// https://github.com/microsoftgraph/msgraph-sdk-go/issues/302 -type ErrTimeout struct { - common.Err -} - func IsErrTimeout(err error) bool { - e := ErrTimeout{} - if errors.As(err, &e) { - return true - } - - if errors.Is(err, context.DeadlineExceeded) || os.IsTimeout(err) || errors.Is(err, http.ErrHandlerTimeout) { - return true - } - switch err := err.(type) { case *url.Error: return err.Timeout() - default: - return false - } -} - -type ErrThrottled struct { - common.Err -} - -func IsErrThrottled(err error) bool { - if errors.Is(err, Err429TooManyRequests) { - return true } - if hasErrorCode(err, errCodeActivityLimitReached) { - return true - } - - e := ErrThrottled{} - - return errors.As(err, &e) -} - -type ErrUnauthorized struct { - common.Err + return errors.Is(err, ErrTimeout) || + errors.Is(err, context.DeadlineExceeded) || + errors.Is(err, http.ErrHandlerTimeout) || + os.IsTimeout(err) } func IsErrUnauthorized(err error) bool { // TODO: refine this investigation. We don't currently know if // a specific item download url expired, or if the full connection // auth expired. - if errors.Is(err, Err401Unauthorized) { - return true - } - - e := ErrUnauthorized{} - - return errors.As(err, &e) + return clues.HasLabel(err, LabelStatus(http.StatusUnauthorized)) } -type ErrInternalServerError struct { - common.Err -} - -func IsInternalServerError(err error) bool { - if errors.Is(err, Err500InternalServerError) { - return true - } - - e := ErrInternalServerError{} - - return errors.As(err, &e) +// LabelStatus transforms the provided statusCode into +// a standard label that can be attached to a clues error +// and later reviewed when checking error statuses. +func LabelStatus(statusCode int) string { + return fmt.Sprintf("status_code_%d", statusCode) } // IsMalware is true if the graphAPI returns a "malware detected" error code. @@ -271,7 +205,12 @@ func Stack(ctx context.Context, e error) *clues.Err { } func setLabels(err *clues.Err, msg string) *clues.Err { - if strings.Contains(msg, mysiteNotFound) || strings.Contains(msg, mysiteURLNotFound) { + if err == nil { + return nil + } + + ml := strings.ToLower(msg) + if strings.Contains(ml, mysiteNotFound) || strings.Contains(ml, mysiteURLNotFound) { err = err.Label(LabelsMysiteNotFound) } diff --git a/src/internal/connector/graph/errors_test.go b/src/internal/connector/graph/errors_test.go index cafbdc35f..cbc0bbb87 100644 --- a/src/internal/connector/graph/errors_test.go +++ b/src/internal/connector/graph/errors_test.go @@ -2,13 +2,14 @@ package graph import ( "context" + "net/http" "testing" "github.com/microsoftgraph/msgraph-sdk-go/models/odataerrors" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/suite" - "github.com/alcionai/corso/src/internal/common" + "github.com/alcionai/clues" "github.com/alcionai/corso/src/internal/tester" ) @@ -47,7 +48,7 @@ func (suite *GraphErrorsUnitSuite) TestIsErrDeletedInFlight() { }, { name: "as", - err: ErrDeletedInFlight{Err: *common.EncapsulateError(assert.AnError)}, + err: ErrDeletedInFlight, expect: assert.True, }, { @@ -91,7 +92,7 @@ func (suite *GraphErrorsUnitSuite) TestIsErrInvalidDelta() { }, { name: "as", - err: ErrInvalidDelta{Err: *common.EncapsulateError(assert.AnError)}, + err: ErrInvalidDelta, expect: assert.True, }, { @@ -123,6 +124,40 @@ func (suite *GraphErrorsUnitSuite) TestIsErrInvalidDelta() { } } +func (suite *GraphErrorsUnitSuite) TestIsErrUserNotFound() { + 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: odErr("fnords"), + expect: assert.False, + }, + { + name: "request resource not found oDataErr", + err: odErr(errCodeRequestResourceNotFound), + expect: assert.True, + }, + } + for _, test := range table { + suite.Run(test.name, func() { + test.expect(suite.T(), IsErrUserNotFound(test.err)) + }) + } +} + func (suite *GraphErrorsUnitSuite) TestIsErrTimeout() { table := []struct { name string @@ -141,7 +176,7 @@ func (suite *GraphErrorsUnitSuite) TestIsErrTimeout() { }, { name: "as", - err: ErrTimeout{Err: *common.EncapsulateError(assert.AnError)}, + err: ErrTimeout, expect: assert.True, }, { @@ -157,40 +192,6 @@ func (suite *GraphErrorsUnitSuite) TestIsErrTimeout() { } } -func (suite *GraphErrorsUnitSuite) TestIsErrThrottled() { - 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: "as", - err: ErrThrottled{Err: *common.EncapsulateError(assert.AnError)}, - expect: assert.True, - }, - { - name: "is429", - err: Err429TooManyRequests, - expect: assert.True, - }, - } - for _, test := range table { - suite.Run(test.name, func() { - test.expect(suite.T(), IsErrThrottled(test.err)) - }) - } -} - func (suite *GraphErrorsUnitSuite) TestIsErrUnauthorized() { table := []struct { name string @@ -208,13 +209,9 @@ func (suite *GraphErrorsUnitSuite) TestIsErrUnauthorized() { expect: assert.False, }, { - name: "as", - err: ErrUnauthorized{Err: *common.EncapsulateError(assert.AnError)}, - expect: assert.True, - }, - { - name: "is429", - err: Err401Unauthorized, + name: "as", + err: clues.Stack(assert.AnError). + Label(LabelStatus(http.StatusUnauthorized)), expect: assert.True, }, } @@ -224,37 +221,3 @@ func (suite *GraphErrorsUnitSuite) TestIsErrUnauthorized() { }) } } - -func (suite *GraphErrorsUnitSuite) TestIsInternalServerError() { - 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: "as", - err: ErrInternalServerError{Err: *common.EncapsulateError(assert.AnError)}, - expect: assert.True, - }, - { - name: "is429", - err: Err500InternalServerError, - expect: assert.True, - }, - } - for _, test := range table { - suite.Run(test.name, func() { - test.expect(suite.T(), IsInternalServerError(test.err)) - }) - } -} diff --git a/src/internal/connector/onedrive/drive_test.go b/src/internal/connector/onedrive/drive_test.go index 226fee577..3023e9390 100644 --- a/src/internal/connector/onedrive/drive_test.go +++ b/src/internal/connector/onedrive/drive_test.go @@ -212,7 +212,7 @@ func (suite *OneDriveUnitSuite) TestDrives() { expectedResults: nil, }, { - name: "SiteURLNotFound", + name: "MySiteURLNotFound", pagerResults: []pagerResult{ { drives: nil, @@ -225,7 +225,7 @@ func (suite *OneDriveUnitSuite) TestDrives() { expectedResults: nil, }, { - name: "SiteNotFound", + name: "MySiteNotFound", pagerResults: []pagerResult{ { drives: nil, diff --git a/src/internal/connector/onedrive/item.go b/src/internal/connector/onedrive/item.go index ae7d82735..e861ec43d 100644 --- a/src/internal/connector/onedrive/item.go +++ b/src/internal/connector/onedrive/item.go @@ -180,23 +180,12 @@ func downloadItem(ctx context.Context, hc *http.Client, item models.DriveItemabl return nil, clues.New("malware detected").Label(graph.LabelsMalware) } - if resp.StatusCode == http.StatusTooManyRequests { - return resp, graph.Err429TooManyRequests - } + // upstream error checks can compare the status with + // clues.HasLabel(err, graph.LabelStatus(http.KnownStatusCode)) + cerr := clues.Wrap(clues.New(resp.Status), "non-2xx http response"). + Label(graph.LabelStatus(resp.StatusCode)) - if resp.StatusCode == http.StatusUnauthorized { - return resp, graph.Err401Unauthorized - } - - if resp.StatusCode == http.StatusInternalServerError { - return resp, graph.Err500InternalServerError - } - - if resp.StatusCode == http.StatusServiceUnavailable { - return resp, graph.Err503ServiceUnavailable - } - - return resp, clues.Wrap(clues.New(resp.Status), "non-2xx http response") + return resp, cerr } // oneDriveItemInfo will populate a details.OneDriveInfo struct