apierrors: error API cleanup

api/v1/lib/httpcli/apierrors/apierrors.go

@@ -1,14 +1,15 @@
 package apierrors
 
 import (
+	"io"
 	"io/ioutil"
 	"net/http"
 )
 
 // Code is a Mesos HTTP v1 API response status code
 type Code int
 
-var (
+const (
 	// MsgNotLeader is returned by Do calls that are sent to a non leading Mesos master.
 	MsgNotLeader = "call sent to a non-leading master"
 	// MsgAuth is returned by Do calls that are not successfully authenticated.
@@ -40,6 +41,10 @@ var (
 	CodeMesosUnavailable     = Code(http.StatusServiceUnavailable)
 	CodeNotFound             = Code(http.StatusNotFound)
 
+	MaxSizeDetails = 4 * 1024 // MaxSizeDetails limits the length of the details message read from a response body
+)
+
+var (
 	// ErrorTable maps HTTP response codes to their respective Mesos v1 API error messages.
 	ErrorTable = map[Code]string{
 		CodeNotLeader:            MsgNotLeader,
@@ -56,9 +61,8 @@ var (
 
 // Error captures HTTP v1 API error codes and messages generated by Mesos.
 type Error struct {
-	Code    Code   // Code is the HTTP response status code generated by Mesos
-	Message string // Message briefly summarizes the nature of the error
-	Details string // Details captures the HTTP response entity, if any, supplied by Mesos
+	code    Code   // code is the HTTP response status code generated by Mesos
+	message string // message briefly summarizes the nature of the error, possibly includes details from Mesos
 }
 
 // IsErrorCode returns true for all HTTP status codes that are not considered informational or successful.
@@ -81,22 +85,42 @@ func FromResponse(res *http.Response) error {
 		return nil
 	}
 
-	err := &Error{Code: code}
+	var details string
 
 	if res.Body != nil {
 		defer res.Body.Close()
-		buf, _ := ioutil.ReadAll(res.Body)
-		err.Details = string(buf)
+		buf, _ := ioutil.ReadAll(io.LimitReader(res.Body, MaxSizeDetails))
+		details = string(buf)
 	}
 
-	err.Message = ErrorTable[code]
+	return code.Error(details)
+}
+
+// Error generates an error from the given status code and detail string.
+func (code Code) Error(details string) error {
+	if !code.IsError() {
+		return nil
+	}
+	err := &Error{
+		code:    code,
+		message: ErrorTable[code],
+	}
+	if details != "" {
+		err.message = err.message + ": " + details
+	}
 	return err
 }
 
 // Error implements error interface
-func (err *Error) Error() string {
-	if err.Details != "" {
-		return err.Message + ": " + err.Details
+func (err *Error) Error() string { return err.message }
+
+func (err *Error) Code() Code { return err.code }
+
+// IsNotLeader returns true if the given error is an apierror for CodeNotLeader
+func IsNotLeader(err error) bool {
+	if err == nil {
+		return false
 	}
-	return err.Message 
+	apiErr, ok := err.(*Error)
+	return ok && apiErr.code == CodeNotLeader
 }

api/v1/lib/httpcli/apierrors/apierrors_test.go

@@ -0,0 +1,52 @@
+package apierrors
+
+import (
+	"bytes"
+	"io/ioutil"
+	"net/http"
+	"reflect"
+	"testing"
+)
+
+func TestFromResponse(t *testing.T) {
+	for _, tt := range []struct {
+		r *http.Response
+		e error
+	}{
+		{nil, nil},
+		{
+			&http.Response{StatusCode: 200},
+			nil,
+		},
+		{
+			&http.Response{StatusCode: 400, Body: ioutil.NopCloser(bytes.NewBufferString("missing framework id"))},
+			&Error{400, ErrorTable[CodeMalformedRequest] + ": missing framework id"},
+		},
+	} {
+		rr := FromResponse(tt.r)
+		if !reflect.DeepEqual(tt.e, rr) {
+			t.Errorf("Expected: %v, got: %v", tt.e, rr)
+		}
+	}
+}
+
+func TestError(t *testing.T) {
+	for _, tt := range []struct {
+		code         Code
+		isErr        bool
+		details      string
+		wantsMessage string
+	}{
+		{200, false, "", ""},
+		{400, true, "", "malformed request"},
+		{400, true, "foo", "malformed request: foo"},
+	} {
+		err := tt.code.Error(tt.details)
+		if tt.isErr != (err != nil) {
+			t.Errorf("expected isErr %v but error was %q", tt.isErr, err)
+		}
+		if err != nil && err.Error() != tt.wantsMessage {
+			t.Errorf("Expected: %s, got: %s", tt.wantsMessage, err.Error())
+		}
+	}
+}

api/v1/lib/httpcli/httpsched/httpsched.go

@@ -163,22 +163,14 @@ func (mre *mesosRedirectionError) Error() string {
 	return "mesos server sent redirect to: " + mre.newURL
 }
 
-func isErrNotLeader(err error) bool {
-	if err == nil {
-		return false
-	}
-	apiErr, ok := err.(*apierrors.Error)
-	return ok && apiErr.Code == apierrors.CodeNotLeader
-}
-
 // redirectHandler returns a config options that decorates the default response handling routine;
 // it transforms normal Mesos redirect "errors" into mesosRedirectionErrors by parsing the Location
 // header and computing the address of the next endpoint that should be used to replay the failed
 // HTTP request.
 func (cli *client) redirectHandler() httpcli.Opt {
 	return httpcli.HandleResponse(func(hres *http.Response, err error) (mesos.Response, error) {
 		resp, err := cli.HandleResponse(hres, err) // default response handler
-		if err == nil || !isErrNotLeader(err) {
+		if err == nil || !apierrors.IsNotLeader(err) {
 			return resp, err
 		}
 		// TODO(jdef) for now, we're tightly coupled to the httpcli package's Response type

api/v1/lib/httpcli/httpsched/state.go

@@ -113,7 +113,7 @@ func disconnectedFn(ctx context.Context, state *state) stateFn {
 	// (a) validate call = SUBSCRIBE
 	if state.call.GetType() != scheduler.Call_SUBSCRIBE {
 		state.resp = nil
-		state.err = &apierrors.Error{Code: apierrors.CodeUnsubscribed}
+		state.err = apierrors.CodeUnsubscribed.Error("")
 		return disconnectedFn
 	}
 
@@ -193,7 +193,7 @@ var CodesIndicatingSubscriptionLoss = func(codes ...apierrors.Code) map[apierror
 
 func errorIndicatesSubscriptionLoss(err error) (result bool) {
 	if apiError, ok := err.(*apierrors.Error); ok {
-		_, result = CodesIndicatingSubscriptionLoss[apiError.Code]
+		_, result = CodesIndicatingSubscriptionLoss[apiError.Code()]
 	}
 	// TODO(jdef) should other error types be considered here as well?
 	return