Skip to content
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

Expose response HTTP status code in errors #52

Merged
merged 3 commits into from
Sep 28, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,32 @@ package fargo

// MIT Licensed (see README.md) - Copyright (c) 2013 Hudl <@Hudl>

import (
"fmt"
)

type unsuccessfulHTTPResponse struct {
statusCode int
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would unify order with string output. Maybe message prefix first and rcode second?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote the struct in order from most important to least important, from required to optional, thinking that the response code is the type's _raison d'être _. I tried to keep the string format consistent with the rest of fargo's error messages. I don't think you're advocating changing the string format.

I could swap the field order. I just wanted you to know that its current arrangement was a deliberate choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please take a look at commit 4f48c58, in which I make sure to treat the message prefix as optional. Given that, let me know if you'd still prefer the field order to be swapped.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. If you feel it's more appropriate leave it. I was just feeling that's it might look better when creating new error to have message first, so it would be similar to printf function. I'll leave decision to you. Thanks for clarifying.

messagePrefix string
}

func (u *unsuccessfulHTTPResponse) Error() string {
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
// 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
}
Expand Down
52 changes: 52 additions & 0 deletions errors_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
package fargo

// MIT Licensed (see README.md) - Copyright (c) 2013 Hudl <@Hudl>

import (
"errors"
"strconv"
"testing"

. "github.com/smartystreets/goconvey/convey"
)

func TestHTTPResponseStatusCode(t *testing.T) {
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})
})
})
}
20 changes: 10 additions & 10 deletions net.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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
Expand All @@ -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 {
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
}
Expand All @@ -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
}
Expand Down
26 changes: 25 additions & 1 deletion tests/net_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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)
})
})
}
Expand Down