diff --git a/registry/api/errcode/errors.go b/registry/api/errcode/errors.go index a68aaad5..d221cb67 100644 --- a/registry/api/errcode/errors.go +++ b/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, }) } } diff --git a/registry/api/errcode/errors_test.go b/registry/api/errcode/errors_test.go index 684e263a..1f0aaf91 100644 --- a/registry/api/errcode/errors_test.go +++ b/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) + } + } diff --git a/registry/client/blob_writer_test.go b/registry/client/blob_writer_test.go index e3c880e1..099dca4f 100644 --- a/registry/client/blob_writer_test.go +++ b/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) } } diff --git a/registry/client/errors.go b/registry/client/errors.go index e743533b..327fea6d 100644 --- a/registry/client/errors.go +++ b/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 } diff --git a/registry/client/repository_test.go b/registry/client/repository_test.go index 26d92d8e..bf02ce27 100644 --- a/registry/client/repository_test.go +++ b/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) } }