From 4563c50b91319c13e1d3cfb128a6d38dd753476e Mon Sep 17 00:00:00 2001 From: "Steven E. Harris" Date: Tue, 27 Sep 2016 17:38:43 -0400 Subject: [PATCH 1/3] Expose response HTTP status code in errors For various operations involving HTTP requests against the Eureka server, allow callers to interpret surfaced failures by way of inspecting the HTTP response's status code. --- errors.go | 23 +++++++++++++++++++++++ errors_test.go | 34 ++++++++++++++++++++++++++++++++++ net.go | 20 ++++++++++---------- tests/net_test.go | 26 +++++++++++++++++++++++++- 4 files changed, 92 insertions(+), 11 deletions(-) create mode 100644 errors_test.go diff --git a/errors.go b/errors.go index bdcd532..42b65a5 100644 --- a/errors.go +++ b/errors.go @@ -2,6 +2,29 @@ package fargo // MIT Licensed (see README.md) - Copyright (c) 2013 Hudl <@Hudl> +import ( + "fmt" +) + +type unsuccessfulHTTPResponse struct { + statusCode int + messagePrefix string +} + +func (u *unsuccessfulHTTPResponse) Error() string { + return fmt.Sprint(u.messagePrefix, ", rcode = ", u.statusCode) +} + +// HTTPResponseStatusCode extracts the HTTP status code for the response from Eureka that motivated +// the supplied error, if any. If the returned present value is true, the returned code is an HTTP +// status code. +func HTTPResponseStatusCode(err error) (code int, present bool) { + if u, ok := err.(*unsuccessfulHTTPResponse); ok { + return u.statusCode, true + } + return 0, false +} + type AppNotFoundError struct { specific string } diff --git a/errors_test.go b/errors_test.go new file mode 100644 index 0000000..b00af05 --- /dev/null +++ b/errors_test.go @@ -0,0 +1,34 @@ +package fargo + +// MIT Licensed (see README.md) - Copyright (c) 2013 Hudl <@Hudl> + +import ( + "errors" + "testing" +) + +func TestHTTPResponseStatusCode(t *testing.T) { + tests := []struct { + input error + present bool + code int + }{ + {nil, false, 0}, + {errors.New("other"), false, 0}, + {&unsuccessfulHTTPResponse{404, "missing"}, true, 404}, + } + for _, test := range tests { + code, present := HTTPResponseStatusCode(test.input) + if present { + if !test.present { + t.Errorf("input %v: want absent, got code %d", test.input, code) + continue + } + if code != test.code { + t.Errorf("input %v: want %d, got %d", test.input, test.code, code) + } + } else if test.present { + t.Errorf("input %v: want code %d, got absent", test.input, test.code) + } + } +} diff --git a/net.go b/net.go index b293eee..6d94221 100644 --- a/net.go +++ b/net.go @@ -131,7 +131,7 @@ func (e *EurekaConnection) RegisterInstance(ins *Instance) error { ins.Id(), ins.App, err.Error()) return err } - if rcode == 200 { + if rcode == http.StatusOK { log.Noticef("Instance=%s already exists in App=%s, aborting registration", ins.Id(), ins.App) return nil } @@ -164,7 +164,7 @@ func (e *EurekaConnection) ReregisterInstance(ins *Instance) error { if rcode != 204 { log.Warningf("HTTP returned %d registering Instance=%s App=%s Body=\"%s\"", rcode, ins.Id(), ins.App, string(body)) - return fmt.Errorf("http returned %d possible failure registering instance\n", rcode) + return &unsuccessfulHTTPResponse{rcode, "possible failure registering instance"} } // read back our registration to pick up eureka-supplied values @@ -182,8 +182,8 @@ func (e *EurekaConnection) GetInstance(app, insId string) (*Instance, error) { if err != nil { return nil, err } - if rcode != 200 { - return nil, fmt.Errorf("Error getting instance, rcode = %d", rcode) + if rcode != http.StatusOK { + return nil, &unsuccessfulHTTPResponse{rcode, "unable to retrieve instance"} } var ins *Instance if e.UseJson { @@ -219,9 +219,9 @@ func (e *EurekaConnection) DeregisterInstance(ins *Instance) error { } // Eureka promises to return HTTP status code upon deregistration success, but fargo used to accept status code 204 // here instead. Accommodate both for backward compatibility with any fake or proxy Eureka stand-ins. - if rcode != 200 && rcode != 204 { + if rcode != http.StatusOK && rcode != http.StatusNoContent { log.Warningf("HTTP returned %d deregistering Instance=%s App=%s", rcode, ins.Id(), ins.App) - return fmt.Errorf("http returned %d possible failure deregistering instance\n", rcode) + return &unsuccessfulHTTPResponse{rcode, "possible failure deregistering instance"} } return nil @@ -243,7 +243,7 @@ func (e EurekaConnection) AddMetadataString(ins *Instance, key, value string) er if rcode < 200 || rcode >= 300 { log.Warningf("HTTP returned %d updating metadata Instance=%s App=%s Body=\"%s\"", rcode, ins.Id(), ins.App, string(body)) - return fmt.Errorf("http returned %d possible failure updating instance metadata ", rcode) + return &unsuccessfulHTTPResponse{rcode, "possible failure updating instance metadata"} } ins.SetMetadataString(key, value) return nil @@ -265,7 +265,7 @@ func (e EurekaConnection) UpdateInstanceStatus(ins *Instance, status StatusType) if rcode < 200 || rcode >= 300 { log.Warningf("HTTP returned %d updating status Instance=%s App=%s Body=\"%s\"", rcode, ins.Id(), ins.App, string(body)) - return fmt.Errorf("http returned %d possible failure updating instance status ", rcode) + return &unsuccessfulHTTPResponse{rcode, "possible failure updating instance status"} } return nil } @@ -286,9 +286,9 @@ func (e *EurekaConnection) HeartBeatInstance(ins *Instance) error { log.Errorf("Error sending heartbeat for Instance=%s App=%s, error: %s", ins.Id(), ins.App, err.Error()) return err } - if rcode != 200 { + if rcode != http.StatusOK { log.Errorf("Sending heartbeat for Instance=%s App=%s returned code %d", ins.Id(), ins.App, rcode) - return fmt.Errorf("heartbeat returned code %d\n", rcode) + return &unsuccessfulHTTPResponse{rcode, "heartbeat failed"} } return nil } diff --git a/tests/net_test.go b/tests/net_test.go index 59def09..844d1f5 100644 --- a/tests/net_test.go +++ b/tests/net_test.go @@ -3,11 +3,33 @@ package fargo_test // MIT Licensed (see README.md) - Copyright (c) 2013 Hudl <@Hudl> import ( + "fmt" + "net/http" + "testing" + "github.com/hudl/fargo" . "github.com/smartystreets/goconvey/convey" - "testing" ) +func shouldNotBearAnHTTPStatusCode(actual interface{}, expected ...interface{}) string { + if code, present := fargo.HTTPResponseStatusCode(actual.(error)); present { + return fmt.Sprintf("Expected: no HTTP status code\nActual: %d", code) + } + return "" +} + +func shouldBearHTTPStatusCode(actual interface{}, expected ...interface{}) string { + expectedCode := expected[0] + code, present := fargo.HTTPResponseStatusCode(actual.(error)) + if !present { + return fmt.Sprintf("Expected: %d\nActual: no HTTP status code", expectedCode) + } + if code != expectedCode { + return fmt.Sprintf("Expected: %d\nActual: %d", expectedCode, code) + } + return "" +} + func TestConnectionCreation(t *testing.T) { Convey("Pull applications", t, func() { cfg, err := fargo.ReadConfig("./config_sample/local.gcfg") @@ -64,6 +86,7 @@ func TestRegistration(t *testing.T) { } err := e.HeartBeatInstance(&j) So(err, ShouldNotBeNil) + So(err, shouldBearHTTPStatusCode, http.StatusNotFound) }) Convey("Register an instance to TESTAPP", t, func() { Convey("Instance registers correctly", func() { @@ -149,6 +172,7 @@ func DontTestDeregistration(t *testing.T) { Convey("Instance cannot check in", func() { err := e.HeartBeatInstance(&i) So(err, ShouldNotBeNil) + So(err, shouldBearHTTPStatusCode, http.StatusNotFound) }) }) } From 4f48c58d4ce19c0a8526574fb36b07ec5d44827d Mon Sep 17 00:00:00 2001 From: "Steven E. Harris" Date: Wed, 28 Sep 2016 10:42:52 -0400 Subject: [PATCH 2/3] Treat the HTTP error message prefix as optional --- errors.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/errors.go b/errors.go index 42b65a5..4038d6e 100644 --- a/errors.go +++ b/errors.go @@ -12,7 +12,10 @@ type unsuccessfulHTTPResponse struct { } func (u *unsuccessfulHTTPResponse) Error() string { - return fmt.Sprint(u.messagePrefix, ", rcode = ", u.statusCode) + if len(u.messagePrefix) > 0 { + return fmt.Sprint(u.messagePrefix, ", rcode = ", u.statusCode) + } + return fmt.Sprint("rcode = ", u.statusCode) } // HTTPResponseStatusCode extracts the HTTP status code for the response from Eureka that motivated From 648f74ef5e30b65a22b165440f48407db9bd0b9e Mon Sep 17 00:00:00 2001 From: "Steven E. Harris" Date: Wed, 28 Sep 2016 10:43:53 -0400 Subject: [PATCH 3/3] Use GoConvey for HTTP error type tests --- errors_test.go | 62 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 40 insertions(+), 22 deletions(-) diff --git a/errors_test.go b/errors_test.go index b00af05..3154003 100644 --- a/errors_test.go +++ b/errors_test.go @@ -4,31 +4,49 @@ package fargo import ( "errors" + "strconv" "testing" + + . "github.com/smartystreets/goconvey/convey" ) func TestHTTPResponseStatusCode(t *testing.T) { - tests := []struct { - input error - present bool - code int - }{ - {nil, false, 0}, - {errors.New("other"), false, 0}, - {&unsuccessfulHTTPResponse{404, "missing"}, true, 404}, - } - for _, test := range tests { - code, present := HTTPResponseStatusCode(test.input) - if present { - if !test.present { - t.Errorf("input %v: want absent, got code %d", test.input, code) - continue - } - if code != test.code { - t.Errorf("input %v: want %d, got %d", test.input, test.code, code) - } - } else if test.present { - t.Errorf("input %v: want code %d, got absent", test.input, test.code) + Convey("An nil error should have no HTTP status code", t, func() { + _, present := HTTPResponseStatusCode(nil) + So(present, ShouldBeFalse) + }) + Convey("A foreign error should have no detectable HTTP status code", t, func() { + _, present := HTTPResponseStatusCode(errors.New("other")) + So(present, ShouldBeFalse) + }) + Convey("A fargo error generated from a response from Eureka", t, func() { + verify := func(err *unsuccessfulHTTPResponse) { + Convey("should have the given HTTP status code", func() { + code, present := HTTPResponseStatusCode(err) + So(present, ShouldBeTrue) + So(code, ShouldEqual, err.statusCode) + Convey("should produce a message", func() { + msg := err.Error() + if len(err.messagePrefix) == 0 { + Convey("that lacks a prefx", func() { + So(msg, ShouldNotStartWith, ",") + }) + } else { + Convey("that starts with the given prefix", func() { + So(msg, ShouldStartWith, err.messagePrefix) + }) + } + Convey("that contains the status code in decimal notation", func() { + So(msg, ShouldContainSubstring, strconv.Itoa(err.statusCode)) + }) + }) + }) } - } + Convey("with a message prefix", func() { + verify(&unsuccessfulHTTPResponse{500, "operation failed"}) + }) + Convey("without a message prefix", func() { + verify(&unsuccessfulHTTPResponse{statusCode: 500}) + }) + }) }