diff --git a/src/common/api/base.go b/src/common/api/base.go index 859bc0dac..f6fe36140 100644 --- a/src/common/api/base.go +++ b/src/common/api/base.go @@ -16,12 +16,11 @@ package api import ( "encoding/json" + "errors" "fmt" "net/http" "strconv" - "errors" - "github.com/astaxie/beego" "github.com/astaxie/beego/validation" commonhttp "github.com/goharbor/harbor/src/common/http" @@ -65,18 +64,10 @@ func (b *BaseAPI) Render() error { // RenderError provides shortcut to render http error func (b *BaseAPI) RenderError(code int, text string) { - http.Error(b.Ctx.ResponseWriter, text, code) -} - -// RenderFormattedError renders errors with well formatted style -func (b *BaseAPI) RenderFormattedError(errorCode int, errorMsg string) { - error := commonhttp.Error{ - Code: errorCode, - Message: errorMsg, - } - formattedErrMsg := error.String() - log.Errorf("%s %s failed with error: %s", b.Ctx.Request.Method, b.Ctx.Request.URL.String(), formattedErrMsg) - b.RenderError(error.Code, formattedErrMsg) + serror.SendError(b.Ctx.ResponseWriter, &commonhttp.Error{ + Code: code, + Message: text, + }) } // DecodeJSONReq decodes a json request @@ -204,7 +195,7 @@ func (b *BaseAPI) ParseAndHandleError(text string, err error) { } log.Errorf("%s: %v", text, err) if e, ok := err.(*commonhttp.Error); ok { - b.RenderFormattedError(e.Code, e.Message) + b.RenderError(e.Code, e.Message) return } b.SendInternalServerError(errors.New("")) @@ -212,22 +203,22 @@ func (b *BaseAPI) ParseAndHandleError(text string, err error) { // SendUnAuthorizedError sends unauthorized error to the client. func (b *BaseAPI) SendUnAuthorizedError(err error) { - b.RenderFormattedError(http.StatusUnauthorized, err.Error()) + b.RenderError(http.StatusUnauthorized, err.Error()) } // SendConflictError sends conflict error to the client. func (b *BaseAPI) SendConflictError(err error) { - b.RenderFormattedError(http.StatusConflict, err.Error()) + b.RenderError(http.StatusConflict, err.Error()) } // SendNotFoundError sends not found error to the client. func (b *BaseAPI) SendNotFoundError(err error) { - b.RenderFormattedError(http.StatusNotFound, err.Error()) + b.RenderError(http.StatusNotFound, err.Error()) } // SendBadRequestError sends bad request error to the client. func (b *BaseAPI) SendBadRequestError(err error) { - b.RenderFormattedError(http.StatusBadRequest, err.Error()) + b.RenderError(http.StatusBadRequest, err.Error()) } // SendInternalServerError sends internal server error to the client. @@ -235,23 +226,22 @@ func (b *BaseAPI) SendBadRequestError(err error) { // When you send an internal server error to the client, you expect user to check the log // to find out the root cause. func (b *BaseAPI) SendInternalServerError(err error) { - log.Error(err.Error()) - b.RenderFormattedError(http.StatusInternalServerError, http.StatusText(http.StatusInternalServerError)) + b.RenderError(http.StatusInternalServerError, http.StatusText(http.StatusInternalServerError)) } // SendForbiddenError sends forbidden error to the client. func (b *BaseAPI) SendForbiddenError(err error) { - b.RenderFormattedError(http.StatusForbidden, err.Error()) + b.RenderError(http.StatusForbidden, err.Error()) } // SendPreconditionFailedError sends conflict error to the client. func (b *BaseAPI) SendPreconditionFailedError(err error) { - b.RenderFormattedError(http.StatusPreconditionFailed, err.Error()) + b.RenderError(http.StatusPreconditionFailed, err.Error()) } // SendStatusServiceUnavailableError sends service unavailable error to the client. func (b *BaseAPI) SendStatusServiceUnavailableError(err error) { - b.RenderFormattedError(http.StatusServiceUnavailable, err.Error()) + b.RenderError(http.StatusServiceUnavailable, err.Error()) } // SendError return the error defined in OCI spec: https://github.com/opencontainers/distribution-spec/blob/master/spec.md#errors diff --git a/src/server/error/error.go b/src/server/error/error.go index a4c7ea392..c4bb11c9b 100644 --- a/src/server/error/error.go +++ b/src/server/error/error.go @@ -20,6 +20,7 @@ import ( openapi "github.com/go-openapi/errors" "github.com/go-openapi/runtime" "github.com/go-openapi/runtime/middleware" + commonhttp "github.com/goharbor/harbor/src/common/http" "github.com/goharbor/harbor/src/common/utils/log" ierror "github.com/goharbor/harbor/src/internal/error" "net/http" @@ -42,8 +43,6 @@ var ( } ) -// TODO use "SendError" instead in the v1 APIs? - // SendError tries to parse the HTTP status code from the specified error, envelops it into // an error array as the error payload and returns the code and payload to the response. // And the error is logged as well @@ -76,6 +75,11 @@ func apiError(err error) (statusCode int, errPayload string) { code = int(openAPIErr.Code()) errCode := strings.Replace(strings.ToUpper(http.StatusText(code)), " ", "_", -1) err = ierror.New(nil).WithCode(errCode).WithMessage(openAPIErr.Error()) + } else if legacyErr, ok := err.(*commonhttp.Error); ok { + // make sure the legacy error format is align with the new one + code = legacyErr.Code + errCode := strings.Replace(strings.ToUpper(http.StatusText(code)), " ", "_", -1) + err = ierror.New(nil).WithCode(errCode).WithMessage(legacyErr.Message) } else { code = codeMap[ierror.ErrCode(err)] } diff --git a/src/server/error/error_test.go b/src/server/error/error_test.go index 2a2a3a9ad..94be36c4d 100644 --- a/src/server/error/error_test.go +++ b/src/server/error/error_test.go @@ -17,6 +17,7 @@ package error import ( "errors" openapi "github.com/go-openapi/errors" + commonhttp "github.com/goharbor/harbor/src/common/http" ierror "github.com/goharbor/harbor/src/internal/error" "github.com/stretchr/testify/assert" "net/http" @@ -55,6 +56,15 @@ func TestAPIError(t *testing.T) { assert.Equal(t, http.StatusBadRequest, statusCode) assert.Equal(t, `{"errors":[{"code":"BAD_REQUEST","message":"bad request"}]}`, payload) + // legacy error + err = &commonhttp.Error{ + Code: http.StatusNotFound, + Message: "not found", + } + statusCode, payload = apiError(err) + assert.Equal(t, http.StatusNotFound, statusCode) + assert.Equal(t, `{"errors":[{"code":"NOT_FOUND","message":"not found"}]}`, payload) + // ierror.Error err = &ierror.Error{ Cause: nil, diff --git a/tests/apitests/python/test_edit_project_creation.py b/tests/apitests/python/test_edit_project_creation.py index 7243a264b..c815f8a1a 100644 --- a/tests/apitests/python/test_edit_project_creation.py +++ b/tests/apitests/python/test_edit_project_creation.py @@ -59,7 +59,7 @@ class TestProjects(unittest.TestCase): #3. Create a new project(PA) by user(UA), and fail to create a new project; self.project.create_project(metadata = {"public": "false"}, expect_status_code = 403, - expect_response_body = "{\"code\":403,\"message\":\"Only system admin can create project\"}", **TestProjects.USER_edit_project_creation_CLIENT) + expect_response_body = "{\"errors\":[{\"code\":\"FORBIDDEN\",\"message\":\"Only system admin can create project\"}]}", **TestProjects.USER_edit_project_creation_CLIENT) #4. Set project creation to "everyone"; self.conf.set_configurations_of_project_creation_restriction("everyone", **ADMIN_CLIENT)