From 241470c75c5240b8a9640b907a42c6311f85a746 Mon Sep 17 00:00:00 2001 From: marioiz Date: Tue, 9 Nov 2021 10:35:37 -0800 Subject: [PATCH] Improved Error Constructors --- errors.go | 66 ++++++++++++++++++--------- errors_test.go | 120 +++++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 150 insertions(+), 36 deletions(-) diff --git a/errors.go b/errors.go index 5dd73254..1674972d 100644 --- a/errors.go +++ b/errors.go @@ -73,6 +73,21 @@ type Error interface { Error() string } +// code.Error(msg) builds a new Twirp error with code and msg. Example: +// twirp.NotFound.Error("Resource not found") +// twirp.Internal.Error("Oops") +func (code ErrorCode) Error(msg string) Error { + return NewError(code, msg) +} + +// code.Errorf(msg, args...) builds a new Twirp error with code and formatted msg. +// The format may include "%w" to wrap other errors. Examples: +// twirp.Internal.Error("Oops: %w", originalErr) +// twirp.NotFound.Error("Resource not found with id: %q", resourceID) +func (code ErrorCode) Errorf(msgFmt string, a ...interface{}) Error { + return NewErrorf(code, msgFmt, a...) +} + // WrapError allows Twirp errors to wrap other errors. // The wrapped error can be extracted later with (github.com/pkg/errors).Unwrap // or errors.Is from the standard errors package on Go 1.13+. @@ -83,50 +98,57 @@ func WrapError(twerr Error, err error) Error { } } -// NewError is the generic constructor for a twirp.Error. The ErrorCode must be -// one of the valid predefined constants, otherwise it will be converted to an -// error {type: Internal, msg: "invalid error type {{code}}"}. If you need to -// add metadata, use .WithMeta(key, value) method after building the error. +// NewError builds a twirp.Error. The code must be one of the valid predefined constants. +// To add metadata, use .WithMeta(key, value) method after building the error. func NewError(code ErrorCode, msg string) Error { - if IsValidErrorCode(code) { - return &twerr{ - code: code, - msg: msg, - } - } - return &twerr{ - code: Internal, - msg: "invalid error type " + string(code), + if !IsValidErrorCode(code) { + return &twerr{code: Internal, msg: "invalid error type " + string(code)} } + return &twerr{code: code, msg: msg} } -// NotFoundError constructor for the common NotFound error. +// NewErrorf builds a twirp.Error with a formatted msg. +// The format may include "%w" to wrap other errors. Examples: +// twirp.NewErrorf(twirp.Internal, "Oops: %w", originalErr) +// twirp.NewErrorf(twirp.NotFound, "resource with id: %q", resourceID) +func NewErrorf(code ErrorCode, msgFmt string, a ...interface{}) Error { + err := fmt.Errorf(msgFmt, a...) // format error message, may include "%w" with an original error + twerr := NewError(code, err.Error()) // use the error as msg + return WrapError(twerr, err) // wrap so the original error can be identified with errors.Is +} + +// NotFoundError is a convenience constructor for NotFound errors. func NotFoundError(msg string) Error { return NewError(NotFound, msg) } -// InvalidArgumentError constructor for the common InvalidArgument error. Can be -// used when an argument has invalid format, is a number out of range, is a bad -// option, etc). +// InvalidArgumentError is a convenience constructor for InvalidArgument errors. +// The argument name is included on the "argument" metadata for convenience. func InvalidArgumentError(argument string, validationMsg string) Error { err := NewError(InvalidArgument, argument+" "+validationMsg) err = err.WithMeta("argument", argument) return err } -// RequiredArgumentError is a more specific constructor for InvalidArgument -// error. Should be used when the argument is required (expected to have a -// non-zero value). +// RequiredArgumentError builds an InvalidArgument error. +// Useful when a request argument is expected to have a non-zero value. func RequiredArgumentError(argument string) Error { return InvalidArgumentError(argument, "is required") } -// InternalError constructor for the common Internal error. Should be used to -// specify that something bad or unexpected happened. +// InternalError is a convenience constructor for Internal errors. func InternalError(msg string) Error { return NewError(Internal, msg) } +// InternalErrorf uses the formatted message as the internal error msg. +// The format may include "%w" to wrap other errors. Examples: +// twirp.InternalErrorf("database error: %w", err) +// twirp.InternalErrorf("failed to load resource %q: %w", resourceID, originalErr) +func InternalErrorf(msgFmt string, a ...interface{}) Error { + return NewErrorf(Internal, msgFmt, a...) +} + // InternalErrorWith makes an internal error, wrapping the original error and using it // for the error message, and with metadata "cause" with the original error type. // This function is used by Twirp services to wrap non-Twirp errors as internal errors. diff --git a/errors_test.go b/errors_test.go index c77b5879..fbbc1216 100644 --- a/errors_test.go +++ b/errors_test.go @@ -11,7 +11,7 @@ // express or implied. See the License for the specific language governing // permissions and limitations under the License. -package twirp +package twirp_test import ( "encoding/json" @@ -22,10 +22,77 @@ import ( "testing" pkgerrors "github.com/pkg/errors" + "github.com/twitchtv/twirp" ) +func TestErrorConstructors(t *testing.T) { + var twerr twirp.Error + err := errors.New("The OG error") + + // code.Error + + twerr = twirp.NotFound.Error("oops") + assertTwirpError(t, twerr, twirp.NotFound, "oops") + + // code.Errorf + + twerr = twirp.Aborted.Errorf("oops %d %s", 11, "times") + assertTwirpError(t, twerr, twirp.Aborted, "oops 11 times") + + twerr = twirp.Internal.Errorf("oops: %w", err) + assertTwirpError(t, twerr, twirp.Internal, "oops: The OG error") + if !errors.Is(twerr, err) { + t.Errorf("expected wrap the original error") + } + + // twirp.NewError + + twerr = twirp.NewError(twirp.NotFound, "oops") + assertTwirpError(t, twerr, twirp.NotFound, "oops") + + // twirp.NewErrorf + + twerr = twirp.NewErrorf(twirp.Aborted, "oops %d %s", 11, "times") + assertTwirpError(t, twerr, twirp.Aborted, "oops 11 times") + + twerr = twirp.NewErrorf(twirp.Internal, "oops: %w", err) + assertTwirpError(t, twerr, twirp.Internal, "oops: The OG error") + if !errors.Is(twerr, err) { + t.Errorf("expected wrap the original error") + } + + // Convenience constructors + + twerr = twirp.NotFoundError("oops") + assertTwirpError(t, twerr, twirp.NotFound, "oops") + + twerr = twirp.InvalidArgumentError("my_arg", "is invalid") + assertTwirpError(t, twerr, twirp.InvalidArgument, "my_arg is invalid") + assertTwirpErrorMeta(t, twerr, "argument", "my_arg") + + twerr = twirp.RequiredArgumentError("my_arg") + assertTwirpError(t, twerr, twirp.InvalidArgument, "my_arg is required") + assertTwirpErrorMeta(t, twerr, "argument", "my_arg") + + twerr = twirp.InternalError("oops") + assertTwirpError(t, twerr, twirp.Internal, "oops") + + twerr = twirp.InternalErrorf("oops: %w", err) + assertTwirpError(t, twerr, twirp.Internal, "oops: The OG error") + if !errors.Is(twerr, err) { + t.Errorf("expected wrap the original error") + } + + twerr = twirp.InternalErrorWith(err) + assertTwirpError(t, twerr, twirp.Internal, "The OG error") + if !errors.Is(twerr, err) { + t.Errorf("expected wrap the original error") + } + assertTwirpErrorMeta(t, twerr, "cause", "*errors.errorString") +} + func TestWithMetaRaces(t *testing.T) { - err := NewError(Internal, "msg") + err := twirp.NewError(twirp.Internal, "msg") err = err.WithMeta("k1", "v1") var wg sync.WaitGroup @@ -46,7 +113,7 @@ func TestWithMetaRaces(t *testing.T) { func TestPkgErrorCause(t *testing.T) { rootCause := pkgerrors.New("this is only a test") - twerr := InternalErrorWith(rootCause) + twerr := twirp.InternalErrorWith(rootCause) cause := pkgerrors.Cause(twerr) if cause != rootCause { t.Errorf("got wrong cause for err. have=%q, want=%q", cause, rootCause) @@ -55,8 +122,8 @@ func TestPkgErrorCause(t *testing.T) { func TestWrapError(t *testing.T) { rootCause := errors.New("cause") - twerr := NewError(NotFound, "it ain't there") - err := WrapError(twerr, rootCause) + twerr := twirp.NewError(twirp.NotFound, "it ain't there") + err := twirp.WrapError(twerr, rootCause) cause := pkgerrors.Cause(err) if cause != rootCause { t.Errorf("got wrong cause. got=%q, want=%q", cause, rootCause) @@ -76,7 +143,7 @@ func (e myError) Error() string { func TestInternalErrorWith_Unwrap(t *testing.T) { myErr := myError("myError") wrErr := fmt.Errorf("wrapped: %w", myErr) // double wrap - twerr := InternalErrorWith(wrErr) + twerr := twirp.InternalErrorWith(wrErr) if !errors.Is(twerr, myErr) { t.Errorf("expected errors.Is to match the error wrapped by twirp.InternalErrorWith") @@ -99,16 +166,22 @@ func (errorResponeWriter) Write([]byte) (int, error) { return 0, errors.New("this is only a test") } +type twerrJSON struct { + Code string `json:"code"` + Msg string `json:"msg"` + Meta map[string]string `json:"meta,omitempty"` +} + func TestWriteError(t *testing.T) { resp := httptest.NewRecorder() - twerr := NewError(Internal, "test middleware error") - err := WriteError(resp, twerr) + twerr := twirp.NewError(twirp.Internal, "test middleware error") + err := twirp.WriteError(resp, twerr) if err != nil { t.Errorf("got an error from WriteError when not expecting one: %s", err) return } - twerrCode := ServerHTTPStatusFromErrorCode(twerr.Code()) + twerrCode := twirp.ServerHTTPStatusFromErrorCode(twerr.Code()) if resp.Code != twerrCode { t.Errorf("got wrong status. have=%d, want=%d", resp.Code, twerrCode) return @@ -121,7 +194,7 @@ func TestWriteError(t *testing.T) { return } - if ErrorCode(gotTwerrJSON.Code) != twerr.Code() { + if twirp.ErrorCode(gotTwerrJSON.Code) != twerr.Code() { t.Errorf("got wrong error code. have=%s, want=%s", gotTwerrJSON.Code, twerr.Code()) return } @@ -134,7 +207,7 @@ func TestWriteError(t *testing.T) { errResp := &errorResponeWriter{ResponseRecorder: resp} // Writing again should error out as headers are being rewritten - err = WriteError(errResp, twerr) + err = twirp.WriteError(errResp, twerr) if err == nil { t.Errorf("did not get error on write. have=nil, want=some error") } @@ -143,7 +216,7 @@ func TestWriteError(t *testing.T) { func TestWriteError_WithNonTwirpError(t *testing.T) { resp := httptest.NewRecorder() nonTwerr := errors.New("not a twirp error") - err := WriteError(resp, nonTwerr) + err := twirp.WriteError(resp, nonTwerr) if err != nil { t.Errorf("got an error from WriteError when not expecting one: %s", err) return @@ -161,8 +234,8 @@ func TestWriteError_WithNonTwirpError(t *testing.T) { return } - if ErrorCode(gotTwerrJSON.Code) != Internal { - t.Errorf("got wrong error code. have=%s, want=%s", gotTwerrJSON.Code, Internal) + if twirp.ErrorCode(gotTwerrJSON.Code) != twirp.Internal { + t.Errorf("got wrong error code. have=%s, want=%s", gotTwerrJSON.Code, twirp.Internal) return } @@ -171,3 +244,22 @@ func TestWriteError_WithNonTwirpError(t *testing.T) { return } } + +// Test helpers + +func assertTwirpError(t *testing.T, twerr twirp.Error, code twirp.ErrorCode, msg string) { + t.Helper() + if twerr.Code() != code { + t.Errorf("wrong code. have=%q, want=%q", twerr.Code(), code) + } + if twerr.Msg() != msg { + t.Errorf("wrong msg. have=%q, want=%q", twerr.Msg(), msg) + } +} + +func assertTwirpErrorMeta(t *testing.T, twerr twirp.Error, key string, value string) { + t.Helper() + if twerr.Meta(key) != value { + t.Errorf("wrong meta. have=%q, want=%q", twerr.Meta(key), value) + } +}