Merge pull request #11070 from ywk253100/200313_unify_error_format

United error response format for management APIs (legacy and v2.0 APIs)

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

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)]
 	}

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,

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)