From 24ca5e81d050c18ff120a7c87ddd895573bab04a Mon Sep 17 00:00:00 2001 From: Vlastimil Zeman Date: Thu, 10 Oct 2019 12:08:05 +0100 Subject: [PATCH 1/7] Lock current behaviour in unit tests --- errors_test.go | 239 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 239 insertions(+) create mode 100644 errors_test.go diff --git a/errors_test.go b/errors_test.go new file mode 100644 index 00000000..22f6acc2 --- /dev/null +++ b/errors_test.go @@ -0,0 +1,239 @@ +package typhon + +import ( + "bytes" + "errors" + "io/ioutil" + "net/http" + "testing" + + "github.com/monzo/terrors" + "github.com/monzo/terrors/stack" + "github.com/stretchr/testify/assert" +) + +func TestErrorFilter(t *testing.T) { + var nilReq Request + tests := []struct { + name string + req Request + svc Service + wantRsp Response + }{ + { + "nil request", + nilReq, + Service(func(req Request) Response { + return req.Response(map[string]string{"b": "a"}) + }), + Response{ + &http.Response{ + StatusCode: http.StatusOK, + Header: http.Header{"Content-Type": []string{"application/json"}}, + Body: &bufCloser{*bytes.NewBufferString(`"b":"a"`)}, + }, + nil, + &nilReq, + false, + }, + }, + { + "request with foo error", + Request{ + err: errors.New("foo error"), + }, + Service(func(req Request) Response { + return req.Response(map[string]string{"b": "a"}) + }), + Response{ + &http.Response{ + StatusCode: http.StatusInternalServerError, + Header: http.Header{ + "Content-Type": []string{"application/json"}, + "Terror": []string{"1"}, + }, + Body: &bufCloser{*bytes.NewBufferString(`"code":"internal_service","message":"foo error"`)}, + }, + errors.New("foo error"), + &Request{ + err: errors.New("foo error"), + }, + false, + }, + }, + { + "request with empty error", + Request{ + err: errors.New(""), + }, + Service(func(req Request) Response { + return Response{} + }), + Response{ + &http.Response{ + StatusCode: http.StatusInternalServerError, + Header: http.Header{ + "Content-Type": []string{"application/json"}, + "Terror": []string{"1"}, + }, + Body: &bufCloser{*bytes.NewBufferString(`"code":"internal_service"`)}, + }, + errors.New("Response error (500)"), + &Request{ + err: errors.New(""), + }, + false, + }, + }, + { + "service return empty response", + Request{}, + Service(func(req Request) Response { + return Response{} + }), + Response{ + &http.Response{ + StatusCode: http.StatusOK, + Header: http.Header{}, + }, + nil, + &Request{ + err: nil, + }, + false, + }, + }, + { + "service return response with 404 no Terror header", + Request{}, + Service(func(req Request) Response { + return Response{ + Response: &http.Response{ + Header: http.Header{}, + StatusCode: http.StatusNotFound, + Body: &bufCloser{*bytes.NewBufferString(`"message":"foo not found"`)}, + }, + Error: nil, + Request: &Request{}, + } + }), + Response{ + &http.Response{ + StatusCode: http.StatusNotFound, + Header: http.Header{}, + }, + errors.New(`"message":"foo not found"`), + &Request{ + err: nil, + }, + false, + }, + }, + { + "service return response with 404 with Terror header and no Terror body", + Request{}, + Service(func(req Request) Response { + return Response{ + Response: &http.Response{ + Header: http.Header{"Terror": []string{"1"}}, + StatusCode: http.StatusNotFound, + Body: &bufCloser{*bytes.NewBufferString("I am bad boy")}, + }, + Error: nil, + Request: &Request{}, + } + }), + Response{ + &http.Response{ + StatusCode: http.StatusNotFound, + Header: http.Header{"Terror": []string{"1"}}, + }, + errors.New("I am bad boy"), + &Request{ + err: nil, + }, + false, + }, + }, + { + "service return response with 404 with Terror header and Terror body", + Request{}, + Service(func(req Request) Response { + return Response{ + Response: &http.Response{ + Header: http.Header{"Terror": []string{"1"}}, + StatusCode: http.StatusNotFound, + Body: &bufCloser{*bytes.NewBufferString(`{"code":"not_found","message":"foo not found"}`)}, + }, + Error: nil, + Request: &Request{}, + } + }), + Response{ + &http.Response{ + StatusCode: http.StatusNotFound, + Header: http.Header{"Terror": []string{"1"}}, + }, + &terrors.Error{ + Code: "not_found", + Message: "foo not found", + StackFrames: stack.Stack{}, + Params: map[string]string{}, + }, + &Request{ + err: nil, + }, + false, + }, + }, + { + "service return non-nil response with empty non-nil error", + Request{}, + Service(func(req Request) Response { + return Response{ + Response: &http.Response{ + Header: http.Header{}, + StatusCode: http.StatusNoContent, + Body: &bufCloser{*bytes.NewBufferString(`"message":"no content"`)}, + }, + Error: errors.New(""), + Request: &Request{}, + } + }), + Response{ + &http.Response{ + Header: http.Header{}, + StatusCode: http.StatusNoContent, + }, + errors.New("Response error (204)"), + &Request{ + err: nil, + }, + false, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotRsp := ErrorFilter(tt.req, tt.svc) + + if tt.wantRsp.Response.Body != nil { + gotResBody, err := ioutil.ReadAll(gotRsp.Response.Body) + if err != nil { + t.Fatalf("cannot read gotRsp.Response.Body") + } + wantResBody, err := ioutil.ReadAll(tt.wantRsp.Response.Body) + if err != nil { + t.Fatalf("cannot read tt.wantRsp.Response.Body") + } + assert.Contains(t, string(gotResBody), string(wantResBody)) + } + + assert.Equal(t, tt.wantRsp.Response.Header, gotRsp.Response.Header) + assert.Equal(t, tt.wantRsp.Response.StatusCode, gotRsp.Response.StatusCode) + assert.Equal(t, tt.wantRsp.Request, gotRsp.Request) + assert.Equal(t, tt.wantRsp.hijacked, gotRsp.hijacked) + assert.EqualValues(t, tt.wantRsp.Error, gotRsp.Error) + }) + } +} From 997fc0a20ec6bd64c2667245c270c065b39debe5 Mon Sep 17 00:00:00 2001 From: Vlastimil Zeman Date: Thu, 10 Oct 2019 12:11:22 +0100 Subject: [PATCH 2/7] Use named return variable --- errors.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/errors.go b/errors.go index 832d670a..13720743 100644 --- a/errors.go +++ b/errors.go @@ -54,9 +54,8 @@ func status2TerrCode(code int) string { // ErrorFilter serialises and deserialises response errors. Without this filter, errors may not be passed across // the network properly so it is recommended to use this in most/all cases. -func ErrorFilter(req Request, svc Service) Response { +func ErrorFilter(req Request, svc Service) (rsp Response) { // If the request contains an error, short-circuit and return that directly - var rsp Response if req.err != nil { rsp = NewResponse(req) rsp.Error = req.err From 31b2231430bf279aad0d2f18c2991d43d840922e Mon Sep 17 00:00:00 2001 From: Vlastimil Zeman Date: Thu, 10 Oct 2019 13:17:59 +0100 Subject: [PATCH 3/7] Remove unreachable branch rsp.Response == nil checked and modified earlier --- errors.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/errors.go b/errors.go index 13720743..1620dbbc 100644 --- a/errors.go +++ b/errors.go @@ -103,8 +103,6 @@ func ErrorFilter(req Request, svc Service) (rsp Response) { if rsp.Error != nil && rsp.Error.Error() == "" { if rsp.Response != nil { rsp.Error = fmt.Errorf("Response error (%d)", rsp.StatusCode) - } else { - rsp.Error = fmt.Errorf("Response error") } } From de5c8b2a3d8be36efa67104c31264af032567923 Mon Sep 17 00:00:00 2001 From: Vlastimil Zeman Date: Thu, 10 Oct 2019 14:23:59 +0100 Subject: [PATCH 4/7] Move empty error decorator to defer so that it is possible to return early and remove else branches --- errors.go | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/errors.go b/errors.go index 1620dbbc..605ebceb 100644 --- a/errors.go +++ b/errors.go @@ -55,6 +55,12 @@ func status2TerrCode(code int) string { // ErrorFilter serialises and deserialises response errors. Without this filter, errors may not be passed across // the network properly so it is recommended to use this in most/all cases. func ErrorFilter(req Request, svc Service) (rsp Response) { + defer func() { + if rsp.Error != nil && rsp.Error.Error() == "" && rsp.Response != nil { + rsp.Error = fmt.Errorf("Response error (%d)", rsp.StatusCode) + } + }() + // If the request contains an error, short-circuit and return that directly if req.err != nil { rsp = NewResponse(req) @@ -82,29 +88,23 @@ func ErrorFilter(req Request, svc Service) (rsp Response) { rsp.StatusCode = ErrorStatusCode(terr) rsp.Header.Set("Terror", "1") } - } else if rsp.StatusCode >= 400 && rsp.StatusCode <= 599 { + return rsp + } + + if rsp.StatusCode >= 400 && rsp.StatusCode <= 599 { // There is an error in the underlying response; unmarshal b, _ := rsp.BodyBytes(false) - switch rsp.Header.Get("Terror") { - case "1": + if rsp.Header.Get("Terror") == "1" { tp := &terrorsproto.Error{} if err := json.Unmarshal(b, tp); err != nil { slog.Warn(rsp.Request, "Failed to unmarshal terror: %v", err) rsp.Error = errors.New(string(b)) - } else { - rsp.Error = terrors.Unmarshal(tp) + return rsp } - - default: - rsp.Error = errors.New(string(b)) + rsp.Error = terrors.Unmarshal(tp) + return rsp } + rsp.Error = errors.New(string(b)) } - - if rsp.Error != nil && rsp.Error.Error() == "" { - if rsp.Response != nil { - rsp.Error = fmt.Errorf("Response error (%d)", rsp.StatusCode) - } - } - return rsp } From 58232071aaec63080b6343c33a0ebd4dbff62ad5 Mon Sep 17 00:00:00 2001 From: Vlastimil Zeman Date: Thu, 10 Oct 2019 14:44:07 +0100 Subject: [PATCH 5/7] Invert ifs to return early and have less indented code. --- errors.go | 41 ++++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/errors.go b/errors.go index 605ebceb..07d98047 100644 --- a/errors.go +++ b/errors.go @@ -77,34 +77,37 @@ func ErrorFilter(req Request, svc Service) (rsp Response) { } if rsp.Error != nil { - if rsp.StatusCode == http.StatusOK { - // We got an error, but there is no error in the underlying response; marshal - if rsp.Body != nil { - rsp.Body.Close() - } - rsp.Body = &bufCloser{} - terr := terrors.Wrap(rsp.Error, nil).(*terrors.Error) - rsp.Encode(terrors.Marshal(terr)) - rsp.StatusCode = ErrorStatusCode(terr) - rsp.Header.Set("Terror", "1") + if rsp.StatusCode != http.StatusOK { + return rsp + } + // We got an error, but there is no error in the underlying response; marshal + if rsp.Body != nil { + rsp.Body.Close() } + rsp.Body = &bufCloser{} + terr := terrors.Wrap(rsp.Error, nil).(*terrors.Error) + rsp.Encode(terrors.Marshal(terr)) + rsp.StatusCode = ErrorStatusCode(terr) + rsp.Header.Set("Terror", "1") return rsp } if rsp.StatusCode >= 400 && rsp.StatusCode <= 599 { // There is an error in the underlying response; unmarshal b, _ := rsp.BodyBytes(false) - if rsp.Header.Get("Terror") == "1" { - tp := &terrorsproto.Error{} - if err := json.Unmarshal(b, tp); err != nil { - slog.Warn(rsp.Request, "Failed to unmarshal terror: %v", err) - rsp.Error = errors.New(string(b)) - return rsp - } - rsp.Error = terrors.Unmarshal(tp) + if rsp.Header.Get("Terror") != "1" { + rsp.Error = errors.New(string(b)) + return rsp + } + tp := &terrorsproto.Error{} + if err := json.Unmarshal(b, tp); err != nil { + slog.Warn(rsp.Request, "Failed to unmarshal terror: %v", err) + rsp.Error = errors.New(string(b)) return rsp } - rsp.Error = errors.New(string(b)) + rsp.Error = terrors.Unmarshal(tp) + return rsp } + return rsp } From f4b59dc8a054be86108183d842cf317fa398d341 Mon Sep 17 00:00:00 2001 From: Vlastimil Zeman Date: Thu, 10 Oct 2019 14:53:45 +0100 Subject: [PATCH 6/7] Remove unused status2TerrCode() --- errors.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/errors.go b/errors.go index 07d98047..0b72b6da 100644 --- a/errors.go +++ b/errors.go @@ -44,14 +44,6 @@ func ErrorStatusCode(err error) int { return http.StatusInternalServerError } -// terr2StatusCode converts HTTP status codes to a roughly equivalent terrors' code -func status2TerrCode(code int) string { - if c, ok := mapStatus2Terr[code]; ok { - return c - } - return terrors.ErrInternalService -} - // ErrorFilter serialises and deserialises response errors. Without this filter, errors may not be passed across // the network properly so it is recommended to use this in most/all cases. func ErrorFilter(req Request, svc Service) (rsp Response) { From 9c794c0d327c2a1abe6a6f082d9d7654db4b47e4 Mon Sep 17 00:00:00 2001 From: Vlastimil Zeman Date: Thu, 10 Oct 2019 15:17:19 +0100 Subject: [PATCH 7/7] Remove creation of NewResponse in request error case NewResponse duplicates action taken later in code. --- errors.go | 1 - 1 file changed, 1 deletion(-) diff --git a/errors.go b/errors.go index 0b72b6da..50ff5edd 100644 --- a/errors.go +++ b/errors.go @@ -55,7 +55,6 @@ func ErrorFilter(req Request, svc Service) (rsp Response) { // If the request contains an error, short-circuit and return that directly if req.err != nil { - rsp = NewResponse(req) rsp.Error = req.err } else { rsp = svc(req)