Merge pull request #9673 from steven-zou/fix/issue_#9668_status_conflicts

return more clear error message for scan related API

src/core/api/scan.go

@@ -18,14 +18,14 @@ import (
 	"net/http"
 	"strconv"
 
-	"github.com/goharbor/harbor/src/pkg/scan/report"
-
 	"github.com/goharbor/harbor/src/common/models"
 	"github.com/goharbor/harbor/src/common/rbac"
 	"github.com/goharbor/harbor/src/common/utils"
 	coreutils "github.com/goharbor/harbor/src/core/utils"
 	"github.com/goharbor/harbor/src/jobservice/logger"
 	"github.com/goharbor/harbor/src/pkg/scan/api/scan"
+	"github.com/goharbor/harbor/src/pkg/scan/errs"
+	"github.com/goharbor/harbor/src/pkg/scan/report"
 	v1 "github.com/goharbor/harbor/src/pkg/scan/rest/v1"
 	"github.com/pkg/errors"
 )
@@ -94,7 +94,19 @@ func (sa *ScanAPI) Scan() {
 	}
 
 	if err := scan.DefaultController.Scan(sa.artifact); err != nil {
-		sa.SendInternalServerError(errors.Wrap(err, "scan API: scan"))
+		e := errors.Wrap(err, "scan API: scan")
+
+		if errs.AsError(err, errs.PreconditionFailed) {
+			sa.SendPreconditionFailedError(e)
+			return
+		}
+
+		if errs.AsError(err, errs.Conflict) {
+			sa.SendConflictError(e)
+			return
+		}
+
+		sa.SendInternalServerError(e)
 		return
 	}
 
@@ -117,7 +129,14 @@ func (sa *ScanAPI) Report() {
 	// Get the reports
 	reports, err := scan.DefaultController.GetReport(sa.artifact, producesMimes)
 	if err != nil {
-		sa.SendInternalServerError(errors.Wrap(err, "scan API: get report"))
+		e := errors.Wrap(err, "scan API: get report")
+
+		if errs.AsError(err, errs.PreconditionFailed) {
+			sa.SendPreconditionFailedError(e)
+			return
+		}
+
+		sa.SendInternalServerError(e)
 		return
 	}
 

src/pkg/scan/api/scan/base_controller.go

@@ -34,6 +34,7 @@ import (
 	sc "github.com/goharbor/harbor/src/pkg/scan/api/scanner"
 	"github.com/goharbor/harbor/src/pkg/scan/dao/scan"
 	"github.com/goharbor/harbor/src/pkg/scan/dao/scanner"
+	"github.com/goharbor/harbor/src/pkg/scan/errs"
 	"github.com/goharbor/harbor/src/pkg/scan/report"
 	v1 "github.com/goharbor/harbor/src/pkg/scan/rest/v1"
 	"github.com/google/uuid"
@@ -124,9 +125,14 @@ func (bc *basicController) Scan(artifact *v1.Artifact) error {
 		return errors.Wrap(err, "scan controller: scan")
 	}
 
+	// In case it does not exist
+	if r == nil {
+		return errs.WithCode(errs.PreconditionFailed, errs.Errorf("no available scanner for project: %d", artifact.NamespaceID))
+	}
+
 	// Check if it is disabled
 	if r.Disabled {
-		return errors.Errorf("scanner %s is disabled", r.Name)
+		return errs.WithCode(errs.PreconditionFailed, errs.Errorf("scanner %s is disabled", r.Name))
 	}
 
 	// Check the health of the registration by ping.
@@ -145,6 +151,7 @@ func (bc *basicController) Scan(artifact *v1.Artifact) error {
 
 	producesMimes := make([]string, 0)
 	matched := false
+	statusConflict := false
 	for _, ca := range meta.Capabilities {
 		for _, cm := range ca.ConsumesMimeTypes {
 			if cm == artifact.MimeType {
@@ -166,6 +173,12 @@ func (bc *basicController) Scan(artifact *v1.Artifact) error {
 				}
 				_, e := bc.manager.Create(reportPlaceholder)
 				if e != nil {
+					// Check if it is a status conflict error with common error format.
+					// Common error returned if and only if status conflicts.
+					if !statusConflict {
+						statusConflict = errs.AsError(e, errs.Conflict)
+					}
+
 					// Recorded by error wrap and logged at the same time.
 					if err == nil {
 						err = e
@@ -192,6 +205,10 @@ func (bc *basicController) Scan(artifact *v1.Artifact) error {
 	// If all the record are created failed.
 	if len(producesMimes) == 0 {
 		// Return the last error
+		if statusConflict {
+			return errs.WithCode(errs.Conflict, errs.Wrap(err, "scan controller: scan"))
+		}
+
 		return errors.Wrap(err, "scan controller: scan")
 	}
 
@@ -241,7 +258,7 @@ func (bc *basicController) GetReport(artifact *v1.Artifact, mimeTypes []string)
 	}
 
 	if r == nil {
-		return nil, errors.New("no scanner registration configured")
+		return nil, errs.WithCode(errs.PreconditionFailed, errs.Errorf("no scanner registration configured for project: %d", artifact.NamespaceID))
 	}
 
 	return bc.manager.GetBy(artifact.Digest, r.UUID, mimes)

src/pkg/scan/errs/error.go

@@ -0,0 +1,132 @@
+// Copyright Project Harbor Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package errs
+
+import (
+	"encoding/json"
+	"fmt"
+)
+
+const (
+	// Common error code
+	Common uint16 = 10000
+	// Conflict error code
+	Conflict uint16 = 10409
+	// PreconditionFailed error code
+	PreconditionFailed uint16 = 10412
+)
+
+// codeTexts behaviors as a hash map to look for the text for the given error code.
+func codeTexts(code uint16) string {
+	switch code {
+	case Common:
+		return "common"
+	case Conflict:
+		return "conflict"
+	case PreconditionFailed:
+		return "Precondition failed"
+	default:
+		return "unknown"
+	}
+}
+
+// Error with code
+type Error struct {
+	// Code of error
+	Code uint16 `json:"code"`
+	// Code represented by meaningful text
+	TextCode string `json:"text_code"`
+	// Message of error
+	Message string `json:"message"`
+	// Cause for error
+	Cause error `json:"cause"`
+}
+
+// Error message
+func (e *Error) Error() string {
+	emsg := fmt.Sprintf("error: %d(%s) : %s", e.Code, e.TextCode, e.Message)
+	if e.Cause != nil {
+		emsg = fmt.Sprintf("%s : cause: %s", emsg, e.Cause.Error())
+	}
+
+	return emsg
+}
+
+// String outputs the error with well-formatted string.
+func (e *Error) String() string {
+	bytes, err := json.Marshal(e)
+	if err != nil {
+		// Fallback to normal string
+		return e.Error()
+	}
+
+	return string(bytes)
+}
+
+// New common error.
+func New(message string) error {
+	return &Error{
+		Code:     Common,
+		TextCode: codeTexts(Common),
+		Message:  message,
+	}
+}
+
+// Wrap error with message.
+func Wrap(err error, message string) error {
+	return &Error{
+		Code:     Common,
+		TextCode: codeTexts(Common),
+		Message:  message,
+		Cause:    err,
+	}
+}
+
+// Errorf new a message with the specified format and arguments
+func Errorf(format string, args ...interface{}) error {
+	return &Error{
+		Code:     Common,
+		TextCode: codeTexts(Common),
+		Message:  fmt.Sprintf(format, args...),
+	}
+}
+
+// WithCode sets specified code for the error
+func WithCode(code uint16, err error) error {
+	if err == nil {
+		return err
+	}
+
+	e, ok := err.(*Error)
+	if !ok {
+		return err
+	}
+
+	e.Code = code
+	e.TextCode = codeTexts(code)
+
+	return e
+}
+
+// AsError checks if the given error has the given code
+func AsError(err error, code uint16) bool {
+	if err == nil {
+		return false
+	}
+
+	e, ok := err.(*Error)
+
+	return ok && e.Code == code
+}

src/pkg/scan/errs/error_test.go

@@ -0,0 +1,118 @@
+// Copyright Project Harbor Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package errs
+
+import (
+	"encoding/json"
+	"strings"
+	"testing"
+
+	"github.com/pkg/errors"
+	"github.com/stretchr/testify/require"
+	"github.com/stretchr/testify/suite"
+)
+
+// ErrorSuite is a test suite for testing Error.
+type ErrorSuite struct {
+	suite.Suite
+}
+
+// TestError is the entry point of ErrorSuite.
+func TestError(t *testing.T) {
+	suite.Run(t, &ErrorSuite{})
+}
+
+// TestErrorNew ...
+func (suite *ErrorSuite) TestErrorNew() {
+	err := New("error-testing")
+	require.Error(suite.T(), err)
+
+	suite.Equal(true, AsError(err, Common))
+	suite.Condition(func() (success bool) {
+		return -1 != strings.Index(err.Error(), "error-testing")
+	})
+	suite.Condition(func() (success bool) {
+		success = strings.Contains(err.Error(), codeTexts(Common))
+		return
+	})
+}
+
+// TestErrorWrap ...
+func (suite *ErrorSuite) TestErrorWrap() {
+	err := errors.New("error-stack")
+	e := Wrap(err, "wrap-message")
+	require.Error(suite.T(), e)
+
+	suite.Equal(true, AsError(e, Common))
+	suite.Condition(func() (success bool) {
+		success = -1 != strings.Index(e.Error(), "error-stack")
+		return
+	})
+	suite.Condition(func() (success bool) {
+		success = -1 != strings.Index(e.Error(), "wrap-message")
+		return
+	})
+	suite.Condition(func() (success bool) {
+		success = strings.Contains(e.Error(), codeTexts(Common))
+		return
+	})
+}
+
+// TestErrorErrorf ...
+func (suite *ErrorSuite) TestErrorErrorf() {
+	err := Errorf("a=%d", 1000)
+	require.Error(suite.T(), err)
+
+	suite.Equal(true, AsError(err, Common))
+	suite.Condition(func() (success bool) {
+		success = strings.Contains(err.Error(), "a=1000")
+		return
+	})
+	suite.Condition(func() (success bool) {
+		success = strings.Contains(err.Error(), codeTexts(Common))
+		return
+	})
+}
+
+// TestErrorString ...
+func (suite *ErrorSuite) TestErrorString() {
+	err := New("well-formatted-error")
+	require.Error(suite.T(), err)
+
+	str := err.(*Error).String()
+	require.Condition(suite.T(), func() (success bool) {
+		success = len(str) > 0
+		return
+	})
+
+	e := &Error{}
+	er := json.Unmarshal([]byte(str), e)
+	suite.NoError(er)
+	suite.Equal(e.Message, "well-formatted-error")
+}
+
+// TestErrorWithCode ...
+func (suite *ErrorSuite) TestErrorWithCode() {
+	err := New("error-with-code")
+	require.Error(suite.T(), err)
+
+	err = WithCode(Conflict, err)
+	require.Error(suite.T(), err)
+	suite.Equal(true, AsError(err, Conflict))
+	suite.Condition(func() (success bool) {
+		success = strings.Contains(err.Error(), codeTexts(Conflict))
+		return
+	})
+}

src/pkg/scan/report/base_manager.go

@@ -20,6 +20,7 @@ import (
 	"github.com/goharbor/harbor/src/jobservice/job"
 	"github.com/goharbor/harbor/src/pkg/q"
 	"github.com/goharbor/harbor/src/pkg/scan/dao/scan"
+	"github.com/goharbor/harbor/src/pkg/scan/errs"
 	"github.com/google/uuid"
 	"github.com/pkg/errors"
 )
@@ -78,7 +79,7 @@ func (bm *basicManager) Create(r *scan.Report) (string, error) {
 		// Status conflict
 		if theCopy.StartTime.Add(reportTimeout).After(time.Now()) {
 			if theStatus.Compare(job.RunningStatus) <= 0 {
-				return "", errors.Errorf("conflict: a previous scanning is %s", theCopy.Status)
+				return "", errs.WithCode(errs.Conflict, errs.Errorf("a previous scan process is %s", theCopy.Status))
 			}
 		}