From 294ee7173ebd2635998cb2c124b9b39cc6cf1bad Mon Sep 17 00:00:00 2001 From: ryanfkeepers Date: Tue, 21 Feb 2023 10:42:41 -0700 Subject: [PATCH] remove common errors 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. --- 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 | 126 +++++------------- src/internal/connector/graph/errors_test.go | 110 +-------------- src/internal/connector/onedrive/item.go | 21 +-- 7 files changed, 40 insertions(+), 371 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 53673f2b1..09a1fd0a0 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" @@ -214,9 +213,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 e50058069..4c5ffacc5 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 e0d93db1d..b125349c3 100644 --- a/src/internal/connector/graph/errors.go +++ b/src/internal/connector/graph/errors.go @@ -13,7 +13,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" ) @@ -32,18 +31,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" ) @@ -54,15 +42,25 @@ var Labels = struct { MysiteNotFound: "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 } @@ -73,103 +71,39 @@ 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 { return hasErrorCode(err, errCodeResourceNotFound, errCodeMailboxNotEnabledForRESTAPI) } -// 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) } // --------------------------------------------------------------------------- diff --git a/src/internal/connector/graph/errors_test.go b/src/internal/connector/graph/errors_test.go index c7f889b83..222b6d037 100644 --- a/src/internal/connector/graph/errors_test.go +++ b/src/internal/connector/graph/errors_test.go @@ -7,8 +7,6 @@ import ( "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" ) type GraphErrorsUnitSuite struct { @@ -46,7 +44,7 @@ func (suite *GraphErrorsUnitSuite) TestIsErrDeletedInFlight() { }, { name: "as", - err: ErrDeletedInFlight{Err: *common.EncapsulateError(assert.AnError)}, + err: ErrDeletedInFlight, expect: assert.True, }, { @@ -90,7 +88,7 @@ func (suite *GraphErrorsUnitSuite) TestIsErrInvalidDelta() { }, { name: "as", - err: ErrInvalidDelta{Err: *common.EncapsulateError(assert.AnError)}, + err: ErrInvalidDelta, expect: assert.True, }, { @@ -129,7 +127,7 @@ func (suite *GraphErrorsUnitSuite) TestIsErrTimeout() { }, { name: "as", - err: ErrTimeout{Err: *common.EncapsulateError(assert.AnError)}, + err: ErrTimeout, expect: assert.True, }, { @@ -144,105 +142,3 @@ 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.T().Run(test.name, func(t *testing.T) { - test.expect(t, IsErrThrottled(test.err)) - }) - } -} - -func (suite *GraphErrorsUnitSuite) TestIsErrUnauthorized() { - 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: ErrUnauthorized{Err: *common.EncapsulateError(assert.AnError)}, - expect: assert.True, - }, - { - name: "is429", - err: Err401Unauthorized, - expect: assert.True, - }, - } - for _, test := range table { - suite.T().Run(test.name, func(t *testing.T) { - test.expect(t, IsErrUnauthorized(test.err)) - }) - } -} - -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.T().Run(test.name, func(t *testing.T) { - test.expect(t, IsInternalServerError(test.err)) - }) - } -} diff --git a/src/internal/connector/onedrive/item.go b/src/internal/connector/onedrive/item.go index bc7890a45..a4f180e1c 100644 --- a/src/internal/connector/onedrive/item.go +++ b/src/internal/connector/onedrive/item.go @@ -135,23 +135,12 @@ func downloadItem(ctx context.Context, hc *http.Client, item models.DriveItemabl return resp, nil } - 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