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