From f0ff5985bfbe0aee4444bd8633b9af848004c0a4 Mon Sep 17 00:00:00 2001 From: Vitalii Levitskii Date: Fri, 15 Mar 2024 11:35:13 +0100 Subject: [PATCH] Changed encoding/decoding error code Signed-off-by: Vitalii Levitskii --- CHANGELOG.md | 1 + .../client/errorshttpclient/behavior.go | 2 +- .../client/errorstchclient/behavior.go | 2 +- pkg/errors/client.go | 10 +++++----- pkg/errors/errors_test.go | 19 ++++++++++--------- pkg/errors/server.go | 14 ++++++++------ transport/tchannel/handler_test.go | 4 ++-- 7 files changed, 28 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index da56c06ce..7e24e038e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ## [Unreleased] - Upgraded go version to 1.21, set toolchain version. - Reverted rpc-caller-procedure value setting. +- errors: encoding/decoding (marshalling/unmarshalling) errors are now returned as CodeInternal (13) instead of CodeInvalidArgument (3). ## [1.72.1] - 2024-03-14 - tchannel: Renamed caller-procedure header from `$rpc$-caller-procedure` to `rpc-caller-procedure`. diff --git a/internal/crossdock/client/errorshttpclient/behavior.go b/internal/crossdock/client/errorshttpclient/behavior.go index 0183fe87a..fd5392259 100644 --- a/internal/crossdock/client/errorshttpclient/behavior.go +++ b/internal/crossdock/client/errorshttpclient/behavior.go @@ -283,7 +283,7 @@ func Run(t crossdock.T) { "Context-TTL-MS": "100", }, body: "{}", - wantStatus: 400, + wantStatus: 500, wantBodyContains: `failed to encode "json" response ` + `body for procedure "bad-response" of service "yarpc-test" ` + `from caller "yarpc-test":`, diff --git a/internal/crossdock/client/errorstchclient/behavior.go b/internal/crossdock/client/errorstchclient/behavior.go index 1e154c673..6cd7c8544 100644 --- a/internal/crossdock/client/errorstchclient/behavior.go +++ b/internal/crossdock/client/errorstchclient/behavior.go @@ -131,7 +131,7 @@ func Run(t crossdock.T) { return } code := tchannel.GetSystemErrorCode(err) - assert.Equal(tchannel.ErrCodeBadRequest, code, "bad-response must produce unexpected error") + assert.Equal(tchannel.ErrCodeUnexpected, code, "bad-response must produce unexpected error") assert.Contains(err.Error(), `failed to encode "json"`, "must mention failure to encode JSON in error message") }, }, diff --git a/pkg/errors/client.go b/pkg/errors/client.go index 793ba8176..e48ae292d 100644 --- a/pkg/errors/client.go +++ b/pkg/errors/client.go @@ -29,28 +29,28 @@ import ( ) // RequestBodyEncodeError builds a YARPC error with code -// yarpcerrors.CodeInvalidArgument that represents a failure to encode +// yarpcerrors.CodeInternal that represents a failure to encode // the request body. func RequestBodyEncodeError(req *transport.Request, err error) error { return newClientEncodingError(req, false /*isResponse*/, false /*isHeader*/, err) } // ResponseBodyDecodeError builds a YARPC error with code -// yarpcerrors.CodeInvalidArgument that represents a failure to decode +// yarpcerrors.CodeInternal that represents a failure to decode // the response body. func ResponseBodyDecodeError(req *transport.Request, err error) error { return newClientEncodingError(req, true /*isResponse*/, false /*isHeader*/, err) } // RequestHeadersEncodeError builds a YARPC error with code -// yarpcerrors.CodeInvalidArgument that represents a failure to +// yarpcerrors.CodeInternal that represents a failure to // encode the request headers. func RequestHeadersEncodeError(req *transport.Request, err error) error { return newClientEncodingError(req, false /*isResponse*/, true /*isHeader*/, err) } // ResponseHeadersDecodeError builds a YARPC error with code -// yarpcerrors.CodeInvalidArgument that represents a failure to +// yarpcerrors.CodeInternal that represents a failure to // decode the response headers. func ResponseHeadersDecodeError(req *transport.Request, err error) error { return newClientEncodingError(req, true /*isResponse*/, true /*isHeader*/, err) @@ -71,5 +71,5 @@ func newClientEncodingError(req *transport.Request, isResponse bool, isHeader bo parts = append(parts, fmt.Sprintf("for procedure %q of service %q: %v", req.Procedure, req.Service, err)) - return yarpcerrors.Newf(yarpcerrors.CodeInvalidArgument, strings.Join(parts, " ")) + return yarpcerrors.Newf(yarpcerrors.CodeInternal, strings.Join(parts, " ")) } diff --git a/pkg/errors/errors_test.go b/pkg/errors/errors_test.go index 3f9436cd5..f08ba6766 100644 --- a/pkg/errors/errors_test.go +++ b/pkg/errors/errors_test.go @@ -32,15 +32,16 @@ import ( func TestWrapHandlerError(t *testing.T) { assert.Nil(t, WrapHandlerError(nil, "foo", "bar")) assert.Equal(t, yarpcerrors.CodeInvalidArgument, yarpcerrors.FromError(WrapHandlerError(yarpcerrors.Newf(yarpcerrors.CodeInvalidArgument, ""), "foo", "bar")).Code()) + assert.Equal(t, yarpcerrors.CodeInternal, yarpcerrors.FromError(WrapHandlerError(yarpcerrors.Newf(yarpcerrors.CodeInternal, ""), "foo", "bar")).Code()) assert.Equal(t, yarpcerrors.CodeUnknown, yarpcerrors.FromError(WrapHandlerError(errors.New(""), "foo", "bar")).Code()) } func TestExpectEncodings(t *testing.T) { - assert.Error(t, ExpectEncodings(&transport.Request{}, "foo")) + assertError(t, ExpectEncodings(&transport.Request{}, "foo"), yarpcerrors.CodeInvalidArgument, "request", "encoding") assert.NoError(t, ExpectEncodings(&transport.Request{Encoding: "foo"}, "foo")) assert.NoError(t, ExpectEncodings(&transport.Request{Encoding: "foo"}, "foo", "bar")) - assert.Error(t, ExpectEncodings(&transport.Request{Encoding: "foo"}, "bar")) - assert.Error(t, ExpectEncodings(&transport.Request{Encoding: "foo"}, "bar", "baz")) + assertError(t, ExpectEncodings(&transport.Request{Encoding: "foo"}, "bar"), yarpcerrors.CodeInvalidArgument, "request", "encoding") + assertError(t, ExpectEncodings(&transport.Request{Encoding: "foo"}, "bar", "baz"), yarpcerrors.CodeInvalidArgument, "request", "encoding") } func TestEncodeErrors(t *testing.T) { @@ -51,12 +52,12 @@ func TestEncodeErrors(t *testing.T) { }{ { errorFunc: RequestBodyEncodeError, - expectedCode: yarpcerrors.CodeInvalidArgument, + expectedCode: yarpcerrors.CodeInternal, expectedWords: []string{"request", "body", "encode"}, }, { errorFunc: RequestHeadersEncodeError, - expectedCode: yarpcerrors.CodeInvalidArgument, + expectedCode: yarpcerrors.CodeInternal, expectedWords: []string{"request", "headers", "encode"}, }, { @@ -71,22 +72,22 @@ func TestEncodeErrors(t *testing.T) { }, { errorFunc: ResponseBodyEncodeError, - expectedCode: yarpcerrors.CodeInvalidArgument, + expectedCode: yarpcerrors.CodeInternal, expectedWords: []string{"response", "body", "encode"}, }, { errorFunc: ResponseHeadersEncodeError, - expectedCode: yarpcerrors.CodeInvalidArgument, + expectedCode: yarpcerrors.CodeInternal, expectedWords: []string{"response", "headers", "encode"}, }, { errorFunc: ResponseBodyDecodeError, - expectedCode: yarpcerrors.CodeInvalidArgument, + expectedCode: yarpcerrors.CodeInternal, expectedWords: []string{"response", "body", "decode"}, }, { errorFunc: ResponseHeadersDecodeError, - expectedCode: yarpcerrors.CodeInvalidArgument, + expectedCode: yarpcerrors.CodeInternal, expectedWords: []string{"response", "headers", "decode"}, }, } diff --git a/pkg/errors/server.go b/pkg/errors/server.go index 0e4a5c38b..752f5afd2 100644 --- a/pkg/errors/server.go +++ b/pkg/errors/server.go @@ -29,28 +29,28 @@ import ( ) // RequestBodyDecodeError builds a YARPC error with code -// yarpcerrors.CodeInvalidArgument that represents a failure to decode +// yarpcerrors.CodeInternal that represents a failure to decode // the request body. func RequestBodyDecodeError(req *transport.Request, err error) error { return newServerEncodingError(req, nil, false /*isResponse*/, false /*isHeader*/, err) } // ResponseBodyEncodeError builds a YARPC error with code -// yarpcerrors.CodeInvalidArgument that represents a failure to encode +// yarpcerrors.CodeInternal that represents a failure to encode // the response body. func ResponseBodyEncodeError(req *transport.Request, err error) error { return newServerEncodingError(req, nil, true /*isResponse*/, false /*isHeader*/, err) } // RequestHeadersDecodeError builds a YARPC error with code -// yarpcerrors.CodeInvalidArgument that represents a failure to +// yarpcerrors.CodeInternal that represents a failure to // decode the request headers. func RequestHeadersDecodeError(req *transport.Request, err error) error { return newServerEncodingError(req, nil, false /*isResponse*/, true /*isHeader*/, err) } // ResponseHeadersEncodeError builds a YARPC error with code -// yarpcerrors.CodeInvalidArgument that represents a failure to +// yarpcerrors.CodeInternal that represents a failure to // encode the response headers. func ResponseHeadersEncodeError(req *transport.Request, err error) error { return newServerEncodingError(req, nil, true /*isResponse*/, true /*isHeader*/, err) @@ -58,7 +58,7 @@ func ResponseHeadersEncodeError(req *transport.Request, err error) error { // ExpectEncodings verifies that the given request has one of the given // encodings, otherwise it returns a YARPC error with code -// yarpcerrors.CodeInvalidArgument. +// yarpcerrors.CodeInternal. func ExpectEncodings(req *transport.Request, want ...transport.Encoding) error { got := req.Encoding for _, w := range want { @@ -74,6 +74,7 @@ func newServerEncodingError(req *transport.Request, encodings []transport.Encodi if len(encodings) == 0 { encodings = []transport.Encoding{req.Encoding} } + errCode := yarpcerrors.CodeInternal parts := []string{"failed to"} if isResponse { switch len(encodings) { @@ -83,6 +84,7 @@ func newServerEncodingError(req *transport.Request, encodings []transport.Encodi parts = append(parts, fmt.Sprintf("encode %v response", encodings)) } } else { + errCode = yarpcerrors.CodeInvalidArgument switch len(encodings) { case 1: parts = append(parts, fmt.Sprintf("decode %q request", string(encodings[0]))) @@ -98,7 +100,7 @@ func newServerEncodingError(req *transport.Request, encodings []transport.Encodi parts = append(parts, fmt.Sprintf("for procedure %q of service %q from caller %q: %v", req.Procedure, req.Service, req.Caller, err)) - return yarpcerrors.Newf(yarpcerrors.CodeInvalidArgument, strings.Join(parts, " ")) + return yarpcerrors.Newf(errCode, strings.Join(parts, " ")) } func newEncodingMismatchError(want []transport.Encoding, got transport.Encoding) error { diff --git a/transport/tchannel/handler_test.go b/transport/tchannel/handler_test.go index cae62a451..e2de73ee4 100644 --- a/transport/tchannel/handler_test.go +++ b/transport/tchannel/handler_test.go @@ -306,7 +306,7 @@ func TestHandlerFailures(t *testing.T) { "serialization derp", ))) }, - wantStatus: tchannel.ErrCodeBadRequest, + wantStatus: tchannel.ErrCodeUnexpected, newResponseWriter: newHandlerWriter, recorder: newResponseRecorder(), wantLogLevel: zapcore.ErrorLevel, @@ -391,7 +391,7 @@ func TestHandlerFailures(t *testing.T) { arg2: nil, arg3: []byte{0x00}, }, - wantStatus: tchannel.ErrCodeBadRequest, + wantStatus: tchannel.ErrCodeBadRequest, // This is an actual error for request, but we're testing problem with response writing newResponseWriter: newHandlerWriter, recorder: newFaultyResponseRecorder(), wantLogLevel: zapcore.ErrorLevel,