-
Notifications
You must be signed in to change notification settings - Fork 188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Access] Add draft of access api error framework #7106
Open
peterargue
wants to merge
11
commits into
master
Choose a base branch
from
peter/access-add-common-error-handling
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
e176fe3
add initial error framework
peterargue 049bc5f
rename rest error converter
peterargue e4c2ea3
improve comments in network endpoints
peterargue e4a33d9
add catchall error declaration
peterargue 4a7ba95
update error handling comments
peterargue f99e889
improve error converter godocs
peterargue e269179
add require access error
peterargue 6b039eb
throw exception if converting snapshot fails
peterargue df07206
fix error names and add sentinels for ctx errors
peterargue 56845e7
Merge branch 'master' into peter/access-add-common-error-handling
peterargue cb319c2
fix lint
peterargue File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,154 @@ | ||
package access | ||
|
||
import ( | ||
"context" | ||
"errors" | ||
"fmt" | ||
|
||
"github.com/onflow/flow-go/module/irrecoverable" | ||
) | ||
|
||
// RequireNoError returns nil if error is nil, otherwise throws an irrecoverable exception | ||
func RequireNoError(ctx context.Context, err error) error { | ||
if err == nil { | ||
return nil | ||
} | ||
|
||
irrecoverable.Throw(ctx, err) | ||
return irrecoverable.NewException(err) | ||
} | ||
|
||
// RequireErrorIs returns the error if it unwraps to any of the provided target error types | ||
// Otherwise, it throws an irrecoverable exception | ||
func RequireErrorIs(ctx context.Context, err error, targetErrs ...error) error { | ||
if err == nil { | ||
return nil | ||
} | ||
|
||
for _, targetErr := range targetErrs { | ||
if errors.Is(err, targetErr) { | ||
return err | ||
} | ||
} | ||
|
||
irrecoverable.Throw(ctx, err) | ||
return irrecoverable.NewException(err) | ||
} | ||
|
||
// InvalidRequest indicates that the client's request was malformed or invalid | ||
type InvalidRequest struct { | ||
err error | ||
} | ||
|
||
func NewInvalidRequest(err error) InvalidRequest { | ||
return InvalidRequest{err: err} | ||
} | ||
|
||
func (e InvalidRequest) Error() string { | ||
return fmt.Sprintf("invalid argument: %v", e.err) | ||
} | ||
|
||
func (e InvalidRequest) Unwrap() error { | ||
return e.err | ||
} | ||
|
||
func IsInvalidRequest(err error) bool { | ||
var errInvalidRequest InvalidRequest | ||
return errors.As(err, &errInvalidRequest) | ||
} | ||
|
||
// DataNotFound indicates that the requested data was not found on the system | ||
type DataNotFound struct { | ||
dataType string | ||
err error | ||
} | ||
|
||
func NewDataNotFound(dataType string, err error) DataNotFound { | ||
return DataNotFound{dataType: dataType, err: err} | ||
} | ||
|
||
func (e DataNotFound) Error() string { | ||
return fmt.Sprintf("data not found for %s: %v", e.dataType, e.err) | ||
} | ||
|
||
func (e DataNotFound) Unwrap() error { | ||
return e.err | ||
} | ||
|
||
func IsDataNotFound(err error) bool { | ||
var errDataNotFound DataNotFound | ||
return errors.As(err, &errDataNotFound) | ||
} | ||
|
||
// InternalError indicates that a non-fatal internal error occurred | ||
// IMPORTANT: this should only be used for benign internal errors. Fatal or irrecoverable system | ||
// errors must be handled explicitly. | ||
type InternalError struct { | ||
err error | ||
} | ||
|
||
func NewInternalError(err error) InternalError { | ||
return InternalError{err: err} | ||
} | ||
|
||
func (e InternalError) Error() string { | ||
return fmt.Sprintf("internal error: %v", e.err) | ||
} | ||
|
||
func (e InternalError) Unwrap() error { | ||
return e.err | ||
} | ||
|
||
func IsInternalError(err error) bool { | ||
var errInternalError InternalError | ||
return errors.As(err, &errInternalError) | ||
} | ||
|
||
// OutOfRangeError indicates that the request was for data that is outside of the available range. | ||
// This is a more specific version of DataNotFound, where the data is known to eventually exist, but | ||
// currently is not known. | ||
// For example, querying data for a height above the current finalized height. | ||
type OutOfRangeError struct { | ||
err error | ||
} | ||
|
||
func NewOutOfRangeError(err error) OutOfRangeError { | ||
return OutOfRangeError{err: err} | ||
} | ||
|
||
func (e OutOfRangeError) Error() string { | ||
return fmt.Sprintf("out of range: %v", e.err) | ||
} | ||
|
||
func (e OutOfRangeError) Unwrap() error { | ||
return e.err | ||
} | ||
|
||
func IsOutOfRangeError(err error) bool { | ||
var errOutOfRangeError OutOfRangeError | ||
return errors.As(err, &errOutOfRangeError) | ||
} | ||
|
||
// FailedPrecondition indicates that a precondition for the operation was not met | ||
// This is a more specific version of InvalidRequest, where the request is valid, but the system | ||
// is not currently in a state to fulfill the request (but may be in the future). | ||
type FailedPrecondition struct { | ||
err error | ||
} | ||
|
||
func NewFailedPrecondition(err error) FailedPrecondition { | ||
return FailedPrecondition{err: err} | ||
} | ||
|
||
func (e FailedPrecondition) Error() string { | ||
return fmt.Sprintf("precondition failed: %v", e.err) | ||
} | ||
|
||
func (e FailedPrecondition) Unwrap() error { | ||
return e.err | ||
} | ||
|
||
func IsPreconditionFailed(err error) bool { | ||
var errPreconditionFailed FailedPrecondition | ||
return errors.As(err, &errPreconditionFailed) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,92 @@ | ||
package access_test | ||
|
||
import ( | ||
"context" | ||
"fmt" | ||
"testing" | ||
|
||
"github.com/stretchr/testify/require" | ||
|
||
"github.com/onflow/flow-go/access" | ||
"github.com/onflow/flow-go/module/irrecoverable" | ||
) | ||
|
||
func TestRequireNoError(t *testing.T) { | ||
t.Parallel() | ||
|
||
t.Run("no error", func(t *testing.T) { | ||
t.Parallel() | ||
|
||
signalerCtx := irrecoverable.NewMockSignalerContext(t, context.Background()) | ||
ctx := irrecoverable.WithSignalerContext(context.Background(), signalerCtx) | ||
|
||
err := access.RequireNoError(ctx, nil) | ||
require.NoError(t, err) | ||
}) | ||
|
||
t.Run("with error", func(t *testing.T) { | ||
t.Parallel() | ||
|
||
expectedErr := fmt.Errorf("expected error") | ||
|
||
signalerCtx := irrecoverable.NewMockSignalerContextExpectError(t, context.Background(), expectedErr) | ||
ctx := irrecoverable.WithSignalerContext(context.Background(), signalerCtx) | ||
|
||
err := access.RequireNoError(ctx, expectedErr) | ||
require.NotErrorIs(t, err, expectedErr, "expected error should be overridden and explicitly not wrapped") | ||
require.Containsf(t, err.Error(), expectedErr.Error(), "expected returned error message to contain original error message") | ||
}) | ||
} | ||
|
||
func TestRequireErrorIs(t *testing.T) { | ||
t.Parallel() | ||
|
||
targetErr := fmt.Errorf("target error") | ||
|
||
t.Run("no error", func(t *testing.T) { | ||
t.Parallel() | ||
|
||
signalerCtx := irrecoverable.NewMockSignalerContext(t, context.Background()) | ||
ctx := irrecoverable.WithSignalerContext(context.Background(), signalerCtx) | ||
|
||
err := access.RequireErrorIs(ctx, nil, targetErr) | ||
require.NoError(t, err) | ||
}) | ||
|
||
t.Run("with expected error", func(t *testing.T) { | ||
t.Parallel() | ||
|
||
expectedErr := fmt.Errorf("got err: %w", targetErr) | ||
|
||
signalerCtx := irrecoverable.NewMockSignalerContext(t, context.Background()) | ||
ctx := irrecoverable.WithSignalerContext(context.Background(), signalerCtx) | ||
|
||
err := access.RequireErrorIs(ctx, expectedErr, targetErr) | ||
require.ErrorIs(t, err, expectedErr) | ||
}) | ||
|
||
t.Run("with multiple expected error", func(t *testing.T) { | ||
t.Parallel() | ||
|
||
expectedErr := fmt.Errorf("got err: %w", targetErr) | ||
|
||
signalerCtx := irrecoverable.NewMockSignalerContext(t, context.Background()) | ||
ctx := irrecoverable.WithSignalerContext(context.Background(), signalerCtx) | ||
|
||
err := access.RequireErrorIs(ctx, expectedErr, fmt.Errorf("target error2"), targetErr) | ||
require.ErrorIs(t, err, expectedErr) | ||
}) | ||
|
||
t.Run("with unexpected error", func(t *testing.T) { | ||
t.Parallel() | ||
|
||
expectedErr := fmt.Errorf("expected error") | ||
|
||
signalerCtx := irrecoverable.NewMockSignalerContextExpectError(t, context.Background(), expectedErr) | ||
ctx := irrecoverable.WithSignalerContext(context.Background(), signalerCtx) | ||
|
||
err := access.RequireErrorIs(ctx, expectedErr, targetErr) | ||
require.NotErrorIs(t, err, expectedErr, "expected error should be overridden and explicitly not wrapped") | ||
require.Containsf(t, err.Error(), expectedErr.Error(), "expected returned error message to contain original error message") | ||
}) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,12 @@ | ||
package common | ||
|
||
import "net/http" | ||
import ( | ||
"context" | ||
"errors" | ||
"net/http" | ||
|
||
"github.com/onflow/flow-go/access" | ||
) | ||
|
||
// StatusError provides custom error with http status. | ||
type StatusError interface { | ||
|
@@ -56,3 +62,37 @@ func (e *Error) Status() int { | |
func (e *Error) Error() string { | ||
return e.err.Error() | ||
} | ||
|
||
// ErrorToStatusError converts an Access API error into a grpc status error. The input may either | ||
// be a status.Error already, or an access sentinel error. | ||
func ErrorToStatusError(err error) StatusError { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. each API has unique error reporting protocols (grpc vs rest vs websockets). Create API specific converters that return the appropriate mapping from access sentinels. |
||
if err == nil { | ||
return nil | ||
} | ||
|
||
var converted StatusError | ||
if errors.As(err, &converted) { | ||
return converted | ||
} | ||
|
||
switch { | ||
case access.IsInvalidRequest(err): | ||
return NewBadRequestError(err) | ||
case access.IsDataNotFound(err): | ||
return NewNotFoundError(err.Error(), err) | ||
case access.IsPreconditionFailed(err): | ||
return NewRestError(http.StatusPreconditionFailed, err.Error(), err) | ||
case access.IsOutOfRangeError(err): | ||
return NewNotFoundError(err.Error(), err) | ||
case access.IsInternalError(err): | ||
return NewRestError(http.StatusInternalServerError, err.Error(), err) | ||
case errors.Is(err, context.Canceled): | ||
return NewRestError(http.StatusRequestTimeout, "Request canceled", err) | ||
case errors.Is(err, context.DeadlineExceeded): | ||
return NewRestError(http.StatusRequestTimeout, "Request deadline exceeded", err) | ||
default: | ||
// TODO: ideally we would throw an exception in this case. For now, report it as Unknown so we | ||
// can more easily identify any missed code paths and fix them while transitioning to this pattern. | ||
return NewRestError(http.StatusInternalServerError, err.Error(), err) | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[optional] since you are mentioning it:
flow-go/access/errors.go
Lines 139 to 140 in df07206
we could also implement it accordingly
similarly for the
PreconditionFailedError
below, which you are identifying as special sub-cases ofInvalidRequestError