larryh/corso
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

remove common errors (#2597)

Browse Source 
## 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 

## Type of change

- [x] 🧹 Tech Debt/Cleanup

## Issue(s)

* #1970

## Test Plan

- [x] ⚡ Unit test
- [x] 💚 E2E
This commit is contained in:
Keepers 2023-03-09 17:55:33 -07:00 committed by GitHub
parent f82837541f
commit f4c6e65fa3
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 87 additions and 346 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

62
src/internal/common/errors.go
View File

@ -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())
}
}

84
src/internal/common/errors_test.go
View File

@ -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")
}

5
src/internal/connector/exchange/exchange_data_collection_test.go
View File

@ -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")

3
src/internal/connector/exchange/service_iterators_test.go
View File

@ -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"),

133
src/internal/connector/graph/errors.go
View File

@ -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)
}

121
src/internal/connector/graph/errors_test.go
View File

@ -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))
})
}
}

4
src/internal/connector/onedrive/drive_test.go
View File

@ -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,

21
src/internal/connector/onedrive/item.go
View File

@ -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