Merge pull request #651 from duglin/ErrArgs

Add ability to pass in substitution args into an Error

registry/api/errcode/errors.go

@@ -69,20 +69,28 @@ func (ec *ErrorCode) UnmarshalText(text []byte) error {
 // WithDetail creates a new Error struct based on the passed-in info and
 // set the Detail property appropriately
 func (ec ErrorCode) WithDetail(detail interface{}) Error {
-	if err, ok := detail.(error); ok {
-		detail = err.Error()
-	}
-
 	return Error{
-		Code:   ec,
-		Detail: detail,
-	}
+		Code:    ec,
+		Message: ec.Message(),
+	}.WithDetail(detail)
+}
+
+// WithArgs creates a new Error struct and sets the Args slice
+func (ec ErrorCode) WithArgs(args ...interface{}) Error {
+	return Error{
+		Code:    ec,
+		Message: ec.Message(),
+	}.WithArgs(args...)
 }
 
 // Error provides a wrapper around ErrorCode with extra Details provided.
 type Error struct {
-	Code   ErrorCode   `json:"code"`
-	Detail interface{} `json:"detail,omitempty"`
+	Code    ErrorCode   `json:"code"`
+	Message string      `json:"message"`
+	Detail  interface{} `json:"detail,omitempty"`
+
+	// TODO(duglin): See if we need an "args" property so we can do the
+	// variable substitution right before showing the message to the user
 }
 
 // ErrorCode returns the ID/Value of this Error
@@ -97,9 +105,24 @@ func (e Error) Error() string {
 		e.Code.Message())
 }
 
-// Message returned the human-readable error message for this Error
-func (e Error) Message() string {
-	return e.Code.Message()
+// WithDetail will return a new Error, based on the current one, but with
+// some Detail info added
+func (e Error) WithDetail(detail interface{}) Error {
+	return Error{
+		Code:    e.Code,
+		Message: e.Message,
+		Detail:  detail,
+	}
+}
+
+// WithArgs uses the passed-in list of interface{} as the substitution
+// variables in the Error's Message string, but returns a new Error
+func (e Error) WithArgs(args ...interface{}) Error {
+	return Error{
+		Code:    e.Code,
+		Message: fmt.Sprintf(e.Code.Message(), args...),
+		Detail:  e.Detail,
+	}
 }
 
 // ErrorDescriptor provides relevant information about a given error code.
@@ -160,20 +183,11 @@ func (errs Errors) Len() int {
 	return len(errs)
 }
 
-// jsonError extends Error with 'Message' so that we can include the
-// error text, just in case the receiver of the JSON doesn't have this
-// particular ErrorCode registered
-type jsonError struct {
-	Code    ErrorCode   `json:"code"`
-	Message string      `json:"message"`
-	Detail  interface{} `json:"detail,omitempty"`
-}
-
 // MarshalJSON converts slice of error, ErrorCode or Error into a
 // slice of Error - then serializes
 func (errs Errors) MarshalJSON() ([]byte, error) {
 	var tmpErrs struct {
-		Errors []jsonError `json:"errors,omitempty"`
+		Errors []Error `json:"errors,omitempty"`
 	}
 
 	for _, daErr := range errs {
@@ -189,9 +203,16 @@ func (errs Errors) MarshalJSON() ([]byte, error) {
 
 		}
 
-		tmpErrs.Errors = append(tmpErrs.Errors, jsonError{
+		// If the Error struct was setup and they forgot to set the
+		// Message field (meaning its "") then grab it from the ErrCode
+		msg := err.Message
+		if msg == "" {
+			msg = err.Code.Message()
+		}
+
+		tmpErrs.Errors = append(tmpErrs.Errors, Error{
 			Code:    err.Code,
-			Message: err.Message(),
+			Message: msg,
 			Detail:  err.Detail,
 		})
 	}
@@ -203,7 +224,7 @@ func (errs Errors) MarshalJSON() ([]byte, error) {
 // Error or ErrorCode
 func (errs *Errors) UnmarshalJSON(data []byte) error {
 	var tmpErrs struct {
-		Errors []jsonError
+		Errors []Error
 	}
 
 	if err := json.Unmarshal(data, &tmpErrs); err != nil {
@@ -212,14 +233,17 @@ func (errs *Errors) UnmarshalJSON(data []byte) error {
 
 	var newErrs Errors
 	for _, daErr := range tmpErrs.Errors {
-		if daErr.Detail == nil {
+		// If Message is empty or exactly matches the Code's message string
+		// then just use the Code, no need for a full Error struct
+		if daErr.Detail == nil && (daErr.Message == "" || daErr.Message == daErr.Code.Message()) {
 			// Error's w/o details get converted to ErrorCode
 			newErrs = append(newErrs, daErr.Code)
 		} else {
 			// Error's w/ details are untouched
 			newErrs = append(newErrs, Error{
-				Code:   daErr.Code,
-				Detail: daErr.Detail,
+				Code:    daErr.Code,
+				Message: daErr.Message,
+				Detail:  daErr.Detail,
 			})
 		}
 	}

registry/api/errcode/errors_test.go

@@ -76,12 +76,21 @@ var ErrorCodeTest2 = Register("v2.errors", ErrorDescriptor{
 	HTTPStatusCode: http.StatusNotFound,
 })
 
+var ErrorCodeTest3 = Register("v2.errors", ErrorDescriptor{
+	Value:          "TEST3",
+	Message:        "Sorry %q isn't valid",
+	Description:    `Just a test message #3.`,
+	HTTPStatusCode: http.StatusNotFound,
+})
+
 func TestErrorsManagement(t *testing.T) {
 	var errs Errors
 
 	errs = append(errs, ErrorCodeTest1)
 	errs = append(errs, ErrorCodeTest2.WithDetail(
 		map[string]interface{}{"digest": "sometestblobsumdoesntmatter"}))
+	errs = append(errs, ErrorCodeTest3.WithArgs("BOOGIE"))
+	errs = append(errs, ErrorCodeTest3.WithArgs("BOOGIE").WithDetail("data"))
 
 	p, err := json.Marshal(errs)
 
@@ -89,7 +98,12 @@ func TestErrorsManagement(t *testing.T) {
 		t.Fatalf("error marashaling errors: %v", err)
 	}
 
-	expectedJSON := "{\"errors\":[{\"code\":\"TEST1\",\"message\":\"test error 1\"},{\"code\":\"TEST2\",\"message\":\"test error 2\",\"detail\":{\"digest\":\"sometestblobsumdoesntmatter\"}}]}"
+	expectedJSON := `{"errors":[` +
+		`{"code":"TEST1","message":"test error 1"},` +
+		`{"code":"TEST2","message":"test error 2","detail":{"digest":"sometestblobsumdoesntmatter"}},` +
+		`{"code":"TEST3","message":"Sorry \"BOOGIE\" isn't valid"},` +
+		`{"code":"TEST3","message":"Sorry \"BOOGIE\" isn't valid","detail":"data"}` +
+		`]}`
 
 	if string(p) != expectedJSON {
 		t.Fatalf("unexpected json:\ngot:\n%q\n\nexpected:\n%q", string(p), expectedJSON)
@@ -105,6 +119,13 @@ func TestErrorsManagement(t *testing.T) {
 		t.Fatalf("errors not equal after round trip:\nunmarshaled:\n%#v\n\nerrs:\n%#v", unmarshaled, errs)
 	}
 
+	// Test the arg substitution stuff
+	e1 := unmarshaled[3].(Error)
+	exp1 := `Sorry "BOOGIE" isn't valid`
+	if e1.Message != exp1 {
+		t.Fatalf("Wrong msg, got:\n%q\n\nexpected:\n%q", e1.Message, exp1)
+	}
+
 	// Test again with a single value this time
 	errs = Errors{ErrorCodeUnknown}
 	expectedJSON = "{\"errors\":[{\"code\":\"UNKNOWN\",\"message\":\"unknown error\"}]}"
@@ -128,4 +149,26 @@ func TestErrorsManagement(t *testing.T) {
 		t.Fatalf("errors not equal after round trip:\nunmarshaled:\n%#v\n\nerrs:\n%#v", unmarshaled, errs)
 	}
 
+	// Verify that calling WithArgs() more than once does the right thing.
+	// Meaning creates a new Error and uses the ErrorCode Message
+	e1 = ErrorCodeTest3.WithArgs("test1")
+	e2 := e1.WithArgs("test2")
+	if &e1 == &e2 {
+		t.Fatalf("args: e2 and e1 should not be the same, but they are")
+	}
+	if e2.Message != `Sorry "test2" isn't valid` {
+		t.Fatalf("e2 had wrong message: %q", e2.Message)
+	}
+
+	// Verify that calling WithDetail() more than once does the right thing.
+	// Meaning creates a new Error and overwrites the old detail field
+	e1 = ErrorCodeTest3.WithDetail("stuff1")
+	e2 = e1.WithDetail("stuff2")
+	if &e1 == &e2 {
+		t.Fatalf("detail: e2 and e1 should not be the same, but they are")
+	}
+	if e2.Detail != `stuff2` {
+		t.Fatalf("e2 had wrong detail: %q", e2.Detail)
+	}
+
 }

registry/client/blob_writer_test.go

@@ -90,7 +90,7 @@ func TestUploadReadFrom(t *testing.T) {
 						[
 							{
 								"code": "BLOB_UPLOAD_INVALID",
-								"message": "invalid upload identifier",
+								"message": "blob upload invalid",
 								"detail": "more detail"
 							}
 						]
@@ -174,11 +174,11 @@ func TestUploadReadFrom(t *testing.T) {
 		if v2Err.Code != v2.ErrorCodeBlobUploadInvalid {
 			t.Fatalf("Unexpected error code: %s, expected %d", v2Err.Code.String(), v2.ErrorCodeBlobUploadInvalid)
 		}
-		if expected := "blob upload invalid"; v2Err.Message() != expected {
-			t.Fatalf("Unexpected error message: %s, expected %s", v2Err.Message(), expected)
+		if expected := "blob upload invalid"; v2Err.Message != expected {
+			t.Fatalf("Unexpected error message: %q, expected %q", v2Err.Message, expected)
 		}
 		if expected := "more detail"; v2Err.Detail.(string) != expected {
-			t.Fatalf("Unexpected error message: %s, expected %s", v2Err.Detail.(string), expected)
+			t.Fatalf("Unexpected error message: %q, expected %q", v2Err.Detail.(string), expected)
 		}
 	}
 

registry/client/errors.go

@@ -52,10 +52,14 @@ func handleErrorResponse(resp *http.Response) error {
 	if resp.StatusCode == 401 {
 		err := parseHTTPErrorResponse(resp.Body)
 		if uErr, ok := err.(*UnexpectedHTTPResponseError); ok {
-			return &errcode.Error{
-				Code:   v2.ErrorCodeUnauthorized,
-				Detail: uErr.Response,
-			}
+			return v2.ErrorCodeUnauthorized.WithDetail(uErr.Response)
+			/*
+				return &errcode.Error{
+					Code:    v2.ErrorCodeUnauthorized,
+					Message: v2.ErrorCodeUnauthorized.Message(),
+					Detail:  uErr.Response,
+				}
+			*/
 		}
 		return err
 	}

registry/client/repository_test.go

@@ -697,14 +697,14 @@ func TestManifestUnauthorized(t *testing.T) {
 	if err == nil {
 		t.Fatal("Expected error fetching manifest")
 	}
-	v2Err, ok := err.(*errcode.Error)
+	v2Err, ok := err.(errcode.Error)
 	if !ok {
 		t.Fatalf("Unexpected error type: %#v", err)
 	}
 	if v2Err.Code != v2.ErrorCodeUnauthorized {
 		t.Fatalf("Unexpected error code: %s", v2Err.Code.String())
 	}
-	if expected := errcode.ErrorCode(v2.ErrorCodeUnauthorized).Message(); v2Err.Message() != expected {
-		t.Fatalf("Unexpected message value: %s, expected %s", v2Err.Message(), expected)
+	if expected := v2.ErrorCodeUnauthorized.Message(); v2Err.Message != expected {
+		t.Fatalf("Unexpected message value: %q, expected %q", v2Err.Message, expected)
 	}
 }