From ce2c1ffbb39af943c0cbda8f8f8eeb515c68279e Mon Sep 17 00:00:00 2001 From: Keepers Date: Thu, 26 Jan 2023 21:24:06 -0700 Subject: [PATCH] clean up graph errors (#2286) ## Description The current interface makes no sense. This change refactors the funcs to standard boolean comparators. ## Does this PR need a docs update or release note? - [x] :no_entry: No ## Type of change - [x] :broom: Tech Debt/Cleanup --- .../connector/exchange/api/contacts.go | 2 +- src/internal/connector/exchange/api/events.go | 2 +- src/internal/connector/exchange/api/mail.go | 2 +- src/internal/connector/exchange/api/shared.go | 6 +- .../exchange/exchange_data_collection.go | 24 ++--- .../connector/exchange/service_iterators.go | 3 +- src/internal/connector/graph/errors.go | 100 +++++------------- src/internal/connector/onedrive/collection.go | 6 +- 8 files changed, 44 insertions(+), 101 deletions(-) diff --git a/src/internal/connector/exchange/api/contacts.go b/src/internal/connector/exchange/api/contacts.go index e12f4b795..85fea19fc 100644 --- a/src/internal/connector/exchange/api/contacts.go +++ b/src/internal/connector/exchange/api/contacts.go @@ -215,7 +215,7 @@ func (c Contacts) GetAddedAndRemovedItemIDs( } // only return on error if it is NOT a delta issue. // on bad deltas we retry the call with the regular builder - if graph.IsErrInvalidDelta(err) == nil { + if !graph.IsErrInvalidDelta(err) { return nil, nil, DeltaUpdate{}, err } diff --git a/src/internal/connector/exchange/api/events.go b/src/internal/connector/exchange/api/events.go index f78aef76b..cdb74ad19 100644 --- a/src/internal/connector/exchange/api/events.go +++ b/src/internal/connector/exchange/api/events.go @@ -216,7 +216,7 @@ func (c Events) GetAddedAndRemovedItemIDs( } // only return on error if it is NOT a delta issue. // on bad deltas we retry the call with the regular builder - if graph.IsErrInvalidDelta(err) == nil { + if !graph.IsErrInvalidDelta(err) { return nil, nil, DeltaUpdate{}, err } diff --git a/src/internal/connector/exchange/api/mail.go b/src/internal/connector/exchange/api/mail.go index 59085ba96..f29d64bd8 100644 --- a/src/internal/connector/exchange/api/mail.go +++ b/src/internal/connector/exchange/api/mail.go @@ -215,7 +215,7 @@ func (c Mail) GetAddedAndRemovedItemIDs( } // only return on error if it is NOT a delta issue. // on bad deltas we retry the call with the regular builder - if graph.IsErrInvalidDelta(err) == nil { + if !graph.IsErrInvalidDelta(err) { return nil, nil, DeltaUpdate{}, err } diff --git a/src/internal/connector/exchange/api/shared.go b/src/internal/connector/exchange/api/shared.go index c77e21fa8..dfe64d716 100644 --- a/src/internal/connector/exchange/api/shared.go +++ b/src/internal/connector/exchange/api/shared.go @@ -72,11 +72,7 @@ func getItemsAddedAndRemovedFromContainer( // get the next page of data, check for standard errors resp, err := pager.getPage(ctx) if err != nil { - if err := graph.IsErrDeletedInFlight(err); err != nil { - return nil, nil, deltaURL, err - } - - if err := graph.IsErrInvalidDelta(err); err != nil { + if graph.IsErrDeletedInFlight(err) || graph.IsErrInvalidDelta(err) { return nil, nil, deltaURL, err } diff --git a/src/internal/connector/exchange/exchange_data_collection.go b/src/internal/connector/exchange/exchange_data_collection.go index 1a628a28d..44418e9bd 100644 --- a/src/internal/connector/exchange/exchange_data_collection.go +++ b/src/internal/connector/exchange/exchange_data_collection.go @@ -260,17 +260,13 @@ func (col *Collection) streamItems(ctx context.Context) { // If the data is no longer available just return here and chalk it up // as a success. There's no reason to retry and no way we can backup up // enough information to restore the item anyway. - if e := graph.IsErrDeletedInFlight(err); e != nil { + if graph.IsErrDeletedInFlight(err) { atomic.AddInt64(&success, 1) logger.Ctx(ctx).Infow( "Graph reported item not found", - "error", - e, - "service", - path.ExchangeService.String(), - "category", - col.category.String, - ) + "error", err, + "service", path.ExchangeService.String(), + "category", col.category.String) return } @@ -286,17 +282,13 @@ func (col *Collection) streamItems(ctx context.Context) { // there's really nothing we can do and not reporting it will make the // status code upset cause we won't have the same number of results as // attempted items. - if e := graph.IsErrDeletedInFlight(err); e != nil { + if graph.IsErrDeletedInFlight(err) { atomic.AddInt64(&success, 1) logger.Ctx(ctx).Infow( "Graph reported item not found", - "error", - e, - "service", - path.ExchangeService.String(), - "category", - col.category.String, - ) + "error", err, + "service", path.ExchangeService.String(), + "category", col.category.String) return } diff --git a/src/internal/connector/exchange/service_iterators.go b/src/internal/connector/exchange/service_iterators.go index 0880ad233..b59f37877 100644 --- a/src/internal/connector/exchange/service_iterators.go +++ b/src/internal/connector/exchange/service_iterators.go @@ -93,8 +93,7 @@ func filterContainersAndFillCollections( added, removed, newDelta, err := getter.GetAddedAndRemovedItemIDs(ctx, qp.ResourceOwner, cID, prevDelta) if err != nil { - // note == nil check; only catches non-inFlight error cases. - if graph.IsErrDeletedInFlight(err) == nil { + if !graph.IsErrDeletedInFlight(err) { errs = support.WrapAndAppend(qp.ResourceOwner, err, errs) continue } diff --git a/src/internal/connector/graph/errors.go b/src/internal/connector/graph/errors.go index 049660425..c75e4a6cb 100644 --- a/src/internal/connector/graph/errors.go +++ b/src/internal/connector/graph/errors.go @@ -41,21 +41,17 @@ type ErrDeletedInFlight struct { common.Err } -func IsErrDeletedInFlight(err error) error { - if asDeletedInFlight(err) { - return err +func IsErrDeletedInFlight(err error) bool { + e := ErrDeletedInFlight{} + if errors.As(err, &e) { + return true } if hasErrorCode(err, errCodeItemNotFound, errCodeSyncFolderNotFound) { - return ErrDeletedInFlight{*common.EncapsulateError(err)} + return true } - return nil -} - -func asDeletedInFlight(err error) bool { - e := ErrDeletedInFlight{} - return errors.As(err, &e) + return false } // Delta tokens can be desycned or expired. In either case, the token @@ -65,21 +61,17 @@ type ErrInvalidDelta struct { common.Err } -func IsErrInvalidDelta(err error) error { - if asInvalidDelta(err) { - return err +func IsErrInvalidDelta(err error) bool { + e := ErrInvalidDelta{} + if errors.As(err, &e) { + return true } if hasErrorCode(err, errCodeSyncStateNotFound, errCodeResyncRequired) { - return ErrInvalidDelta{*common.EncapsulateError(err)} + return true } - return nil -} - -func asInvalidDelta(err error) bool { - e := ErrInvalidDelta{} - return errors.As(err, &e) + return false } func IsErrExchangeMailFolderNotFound(err error) bool { @@ -94,28 +86,12 @@ type ErrTimeout struct { common.Err } -func IsErrTimeout(err error) error { - if asTimeout(err) { - return err - } - - if isTimeoutErr(err) { - return ErrTimeout{*common.EncapsulateError(err)} - } - - return nil -} - -func asTimeout(err error) bool { +func IsErrTimeout(err error) bool { e := ErrTimeout{} - return errors.As(err, &e) -} + if errors.As(err, &e) { + return true + } -// isTimeoutErr is used to determine if the Graph error returned is -// because of Timeout. This is used to restrict retries to just -// timeouts as other errors are handled within a middleware in the -// client. -func isTimeoutErr(err error) bool { if errors.Is(err, context.DeadlineExceeded) || os.IsTimeout(err) { return true } @@ -132,20 +108,13 @@ type ErrThrottled struct { common.Err } -func IsErrThrottled(err error) error { +func IsErrThrottled(err error) bool { if errors.Is(err, Err429TooManyRequests) { - return err + return true } - if asThrottled(err) { - return err - } - - return nil -} - -func asThrottled(err error) bool { e := ErrThrottled{} + return errors.As(err, &e) } @@ -153,23 +122,16 @@ type ErrUnauthorized struct { common.Err } -func IsErrUnauthorized(err error) error { +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 err + return true } - if asUnauthorized(err) { - return err - } - - return nil -} - -func asUnauthorized(err error) bool { e := ErrUnauthorized{} + return errors.As(err, &e) } @@ -177,21 +139,17 @@ type ErrServiceUnavailable struct { common.Err } -func IsSericeUnavailable(err error) error { +func IsSericeUnavailable(err error) bool { if errors.Is(err, Err503ServiceUnavailable) { - return err + return true } - if asServiceUnavailable(err) { - return err - } - - return nil -} - -func asServiceUnavailable(err error) bool { e := ErrUnauthorized{} - return errors.As(err, &e) + if errors.As(err, &e) { + return true + } + + return true } // --------------------------------------------------------------------------- diff --git a/src/internal/connector/onedrive/collection.go b/src/internal/connector/onedrive/collection.go index 77ac88c63..a786de0ab 100644 --- a/src/internal/connector/onedrive/collection.go +++ b/src/internal/connector/onedrive/collection.go @@ -256,7 +256,7 @@ func (oc *Collection) populateItems(ctx context.Context) { break } - if graph.IsErrUnauthorized(err) != nil { + if graph.IsErrUnauthorized(err) { // assume unauthorized requests are a sign of an expired // jwt token, and that we've overrun the available window // to download the actual file. Re-downloading the item @@ -271,9 +271,7 @@ func (oc *Collection) populateItems(ctx context.Context) { continue - } else if graph.IsErrTimeout(err) == nil && - graph.IsErrThrottled(err) == nil && - graph.IsSericeUnavailable(err) == nil { + } else if !graph.IsErrTimeout(err) && !graph.IsErrThrottled(err) && !graph.IsSericeUnavailable(err) { // TODO: graphAPI will provides headers that state the duration to wait // in order to succeed again. The one second sleep won't cut it here. //