From 34081c4a39628d2fefa8ccfcca29657e05c4ee6f Mon Sep 17 00:00:00 2001 From: Binbin Li Date: Wed, 31 Jul 2024 08:27:52 +0000 Subject: [PATCH] feat: refactor error message format --- cmd/ratify/cmd/cmd_test.go | 4 +- errors/types.go | 86 +++++++++++++++++---------- errors/types_test.go | 117 +++++++++++++++++++++++++++---------- test/bats/plugin-test.bats | 2 +- 4 files changed, 144 insertions(+), 65 deletions(-) diff --git a/cmd/ratify/cmd/cmd_test.go b/cmd/ratify/cmd/cmd_test.go index 49149dcf56..9016198ab9 100644 --- a/cmd/ratify/cmd/cmd_test.go +++ b/cmd/ratify/cmd/cmd_test.go @@ -50,8 +50,8 @@ func TestDiscover(t *testing.T) { // TODO: make ratify cli more unit testable // unit test should not need to resolve real image - if !strings.Contains(err.Error(), "referrer store failure") { - t.Errorf("error expected") + if !strings.Contains(err.Error(), "REFERRER_STORE_FAILURE") { + t.Errorf("expected containing: %s, but got: %s", "REFERRER_STORE_FAILURE", err.Error()) } } diff --git a/errors/types.go b/errors/types.go index 3a211c80ad..9a685ea30b 100644 --- a/errors/types.go +++ b/errors/types.go @@ -66,9 +66,10 @@ type Error struct { Detail interface{} `json:"detail,omitempty"` ComponentType ComponentType `json:"componentType,omitempty"` PluginName string `json:"pluginName,omitempty"` - LinkToDoc string `json:"linkToDoc,omitempty"` + Remediation string `json:"remediation,omitempty"` Stack string `json:"stack,omitempty"` Description string `json:"description,omitempty"` + isRootError bool // isRootError is true if the originalError is either nil or not an Error type } // ErrorDescriptor provides relevant information about a given error code. @@ -152,7 +153,7 @@ func (ec ErrorCode) WithComponentType(componentType ComponentType) Error { // WithLinkToDoc returns a new Error object with attached link to the documentation. func (ec ErrorCode) WithLinkToDoc(link string) Error { - return newError(ec, ec.Message()).WithLinkToDoc(link) + return newError(ec, ec.Message()).WithRemediation(link) } func (ec ErrorCode) WithDescription() Error { @@ -165,7 +166,7 @@ func (ec ErrorCode) WithPluginName(pluginName string) Error { } // NewError returns a new Error object. -func (ec ErrorCode) NewError(componentType ComponentType, pluginName, link string, err error, detail interface{}, printStackTrace bool) Error { +func (ec ErrorCode) NewError(componentType ComponentType, pluginName, remediation string, err error, detail interface{}, printStackTrace bool) Error { stack := "" if printStackTrace { stack = getStackTrace() @@ -177,15 +178,17 @@ func (ec ErrorCode) NewError(componentType ComponentType, pluginName, link strin Detail: detail, ComponentType: componentType, PluginName: pluginName, - LinkToDoc: link, + Remediation: remediation, Stack: stack, + isRootError: err == nil || !errors.As(err, &Error{}), } } func newError(code ErrorCode, message string) Error { return Error{ - Code: code, - Message: message, + Code: code, + Message: message, + isRootError: true, } } @@ -209,39 +212,61 @@ func (e Error) Unwrap() error { } // Error returns a human readable representation of the error. +// An Error message includes the error code, detail from nested errors, root cause and remediation, all separated by ": ". func (e Error) Error() string { - var errStr string - if e.OriginalError != nil { - errStr += fmt.Sprintf("Original Error: (%s), ", e.OriginalError.Error()) + err, details := e.getRootError() + if err.Detail != nil { + details = append(details, fmt.Sprintf("%s", err.Detail)) } - - errStr += fmt.Sprintf("Error: %s, Code: %s", e.Message, e.Code.String()) - - if e.PluginName != "" { - errStr += fmt.Sprintf(", Plugin Name: %s", e.PluginName) + if err.OriginalError != nil { + details = append(details, err.OriginalError.Error()) } - if e.ComponentType != "" { - errStr += fmt.Sprintf(", Component Type: %s", e.ComponentType) + if err.Remediation != "" { + details = append(details, err.Remediation) } + return fmt.Sprintf("%s: %s", err.ErrorCode().Descriptor().Value, strings.Join(details, ": ")) +} - if e.LinkToDoc != "" { - errStr += fmt.Sprintf(", Documentation: %s", e.LinkToDoc) +// GetFullDetails returns details from all nested errors. +func (e Error) GetFullDetails() string { + err, details := e.getRootError() + if err.OriginalError != nil && err.Detail != nil { + details = append(details, fmt.Sprintf("%s", err.Detail)) } - if e.Detail != nil { - errStr += fmt.Sprintf(", Detail: %v", e.Detail) - } + return strings.Join(details, ": ") +} - if e.Description != "" { - errStr += fmt.Sprintf(", Description: %v", e.Description) +// GetRootCause returns the root cause of the error. +func (e Error) GetRootCause() string { + err, _ := e.getRootError() + if err.OriginalError != nil { + return err.OriginalError.Error() } + return fmt.Sprintf("%s", err.Detail) +} - if e.Stack != "" { - errStr += fmt.Sprintf(", Stack trace: %s", e.Stack) - } +func (e Error) GetRootRemediation() string { + err, _ := e.getRootError() + return err.Remediation +} - return errStr +func (e Error) getRootError() (err Error, details []string) { + err = e + for !err.isRootError { + if err.Detail != nil { + details = append(details, fmt.Sprintf("%s", err.Detail)) + } + var ratifyError Error + if errors.As(err.OriginalError, &ratifyError) { + err = ratifyError + } else { + // break is unnecessary, but added for safety + break + } + } + return err, details } // WithDetail will return a new Error, based on the current one, but with @@ -266,12 +291,13 @@ func (e Error) WithComponentType(componentType ComponentType) Error { // WithError returns a new Error object with original error. func (e Error) WithError(err error) Error { e.OriginalError = err + e.isRootError = err == nil || !errors.As(err, &Error{}) return e } -// WithLinkToDoc returns a new Error object attached with link to documentation. -func (e Error) WithLinkToDoc(link string) Error { - e.LinkToDoc = link +// WithRemediation returns a new Error object attached with remediation. +func (e Error) WithRemediation(remediation string) Error { + e.Remediation = remediation return e } diff --git a/errors/types_test.go b/errors/types_test.go index 5e97400d15..ac15df5a72 100644 --- a/errors/types_test.go +++ b/errors/types_test.go @@ -22,26 +22,31 @@ import ( ) const ( - testGroup = "test-group" - testValue = "test-value" - testMessage = "test-message" - testDescription = "test-description" - testDetail = "test-detail" - testComponentType = "test-component-type" - testLink = "test-link" - testPluginName = "test-plugin-name" - testErrorString = "Original Error: (Error: , Code: UNKNOWN), Error: test-message, Code: test-value, Plugin Name: test-plugin-name, Component Type: test-component-type, Documentation: test-link, Detail: test-detail" - nonexistentEC = 2000 + testGroup = "test-group" + testErrCode1 = "TEST_ERROR_CODE_1" + testErrCode2 = "TEST_ERROR_CODE_2" + testMessage = "test-message" + testDescription = "test-description" + testDetail1 = "test-detail-1" + testDetail2 = "test-detail-2" + testComponentType1 = "test-component-type-1" + testComponentType2 = "test-component-type-2" + testLink1 = "test-link-1" + testLink2 = "test-link-2" + testPluginName = "test-plugin-name" + nonexistentEC = 2000 ) var ( testEC = Register(testGroup, ErrorDescriptor{ - Value: testValue, + Value: testErrCode1, Message: testMessage, Description: testDescription, }) - testEC2 = Register(testGroup, ErrorDescriptor{}) + testEC2 = Register(testGroup, ErrorDescriptor{ + Value: testErrCode2, + }) ) func TestErrorCode(t *testing.T) { @@ -52,8 +57,9 @@ func TestErrorCode(t *testing.T) { } func TestError(t *testing.T) { - if testEC.Error() != testValue { - t.Fatalf("expected: %s, got: %s", testValue, testEC.Error()) + expectedStr := "test error code 1" + if testEC.Error() != expectedStr { + t.Fatalf("expected: %s, got: %s", expectedStr, testEC.Error()) } } @@ -66,7 +72,7 @@ func TestDescriptor(t *testing.T) { { name: "existing error code", ec: testEC, - expectedValue: testValue, + expectedValue: testErrCode1, }, { name: "nonexistent error code", @@ -92,9 +98,9 @@ func TestMessage(t *testing.T) { } func TestWithDetail(t *testing.T) { - err := testEC.WithDetail(testDetail) - if err.Detail != testDetail { - t.Fatalf("expected detail: %s, got: %s", testDetail, err.Detail) + err := testEC.WithDetail(testDetail1) + if err.Detail != testDetail1 { + t.Fatalf("expected detail: %s, got: %s", testDetail1, err.Detail) } } @@ -106,16 +112,16 @@ func TestWithError(t *testing.T) { } func TestWithComponentType(t *testing.T) { - err := testEC.WithComponentType(testComponentType) - if err.ComponentType != testComponentType { - t.Fatalf("expected component type: %s, got: %s", testComponentType, err.ComponentType) + err := testEC.WithComponentType(testComponentType1) + if err.ComponentType != testComponentType1 { + t.Fatalf("expected component type: %s, got: %s", testComponentType1, err.ComponentType) } } func TestWithLinkToDoc(t *testing.T) { - err := testEC.WithLinkToDoc(testLink) - if err.LinkToDoc != testLink { - t.Fatalf("expected link to doc: %s, got: %s", testLink, err.LinkToDoc) + err := testEC.WithLinkToDoc(testLink1) + if err.Remediation != testLink1 { + t.Fatalf("expected link to doc: %s, got: %s", testLink1, err.Remediation) } } @@ -134,7 +140,7 @@ func TestWithDescription(t *testing.T) { } func TestIs(t *testing.T) { - err := testEC.WithDetail(testDetail) + err := testEC.WithDetail(testDetail1) result := err.Is(err) if !result { t.Fatalf("expected true, got: %v", result) @@ -168,17 +174,64 @@ func TestIsEmpty(t *testing.T) { } func TestError_Error(t *testing.T) { - err := testEC.WithPluginName(testPluginName).WithComponentType(testComponentType).WithLinkToDoc(testLink).WithDetail(testDetail).WithError(Error{}).WithDescription() - result := err.Error() - if !strings.HasPrefix(result, testErrorString) { - t.Fatalf("expected string starts with: %s, but got: %s", testErrorString, result) + // Nested errors. + rootErr := testEC.NewError(testComponentType1, "", testLink1, errors.New(testMessage), testDetail1, false) + err := testEC2.WithPluginName(testPluginName).WithComponentType(testComponentType2).WithRemediation(testLink2).WithDetail(testDetail2).WithError(rootErr) + + expectedErrStr := strings.Join([]string{testErrCode1, testDetail2, testDetail1, testMessage, testLink1}, ": ") + if err.Error() != expectedErrStr { + t.Fatalf("expected string: %s, but got: %s", expectedErrStr, err.Error()) + } + + // Single error. + err = testEC.WithDetail(testDetail1) + expectedErrStr = "TEST_ERROR_CODE_1: test-detail-1" + if err.Error() != expectedErrStr { + t.Fatalf("expected string: %s, but got: %s", expectedErrStr, err.Error()) + } +} + +func TestError_GetRootCause(t *testing.T) { + // rootErr contains original error. + rootErr := testEC.NewError(testComponentType1, "", testLink1, errors.New(testMessage), testDetail1, false) + err := testEC.WithPluginName(testPluginName).WithComponentType(testComponentType2).WithRemediation(testLink2).WithDetail(testDetail2).WithError(rootErr) + + if err.GetRootCause() != testMessage { + t.Fatalf("expected root cause: %v, but got: %v", err.GetRootCause(), testMessage) + } + + // rootErr does not contain original error. + rootErr = testEC.NewError(testComponentType1, "", testLink1, nil, testDetail1, false) + err = testEC.WithPluginName(testPluginName).WithComponentType(testComponentType2).WithRemediation(testLink2).WithDetail(testDetail2).WithError(rootErr) + + if err.GetRootCause() != testDetail1 { + t.Fatalf("expected root cause: %v, but got: %v", err.GetRootCause(), testDetail1) + } +} + +func TestError_GetFullDetails(t *testing.T) { + rootErr := testEC.NewError(testComponentType1, "", testLink1, errors.New(testMessage), testDetail1, false) + err := testEC.WithPluginName(testPluginName).WithComponentType(testComponentType2).WithRemediation(testLink2).WithDetail(testDetail2).WithError(rootErr) + + expectedDetails := strings.Join([]string{testDetail2, testDetail1}, ": ") + if err.GetFullDetails() != expectedDetails { + t.Fatalf("expected full details: %v, but got: %v", expectedDetails, err.GetFullDetails()) + } +} + +func TestError_GetRootRemediation(t *testing.T) { + rootErr := testEC.NewError(testComponentType1, "", testLink1, errors.New(testMessage), testDetail1, false) + err := testEC.WithPluginName(testPluginName).WithComponentType(testComponentType2).WithRemediation(testLink2).WithDetail(testDetail2).WithError(rootErr) + + if err.GetRootRemediation() != testLink1 { + t.Fatalf("expected root remediation: %v, but got: %v", err.GetRootRemediation(), testLink1) } } func TestNewError(t *testing.T) { - err := testEC.NewError(testComponentType, testPluginName, testLink, Error{}, testDetail, false) + err := testEC.NewError(testComponentType1, testPluginName, testLink1, Error{}, testDetail1, false) - if err.ComponentType != testComponentType || err.PluginName != testPluginName || err.LinkToDoc != testLink || err.Detail != testDetail { - t.Fatalf("expected component type: %s, plugin name: %s, link to doc: %s, detail: %s, but got: %s, %s, %s, %s", testComponentType, testPluginName, testLink, testDetail, err.ComponentType, err.PluginName, err.LinkToDoc, err.Detail) + if err.ComponentType != testComponentType1 || err.PluginName != testPluginName || err.Remediation != testLink1 || err.Detail != testDetail1 { + t.Fatalf("expected component type: %s, plugin name: %s, link to doc: %s, detail: %s, but got: %s, %s, %s, %s", testComponentType1, testPluginName, testLink1, testDetail1, err.ComponentType, err.PluginName, err.Remediation, err.Detail) } } diff --git a/test/bats/plugin-test.bats b/test/bats/plugin-test.bats index 3d37bbd4ef..0cc9dea3af 100644 --- a/test/bats/plugin-test.bats +++ b/test/bats/plugin-test.bats @@ -312,7 +312,7 @@ RATIFY_NAMESPACE=gatekeeper-system sed 's/licensechecker/invalidlicensechecker/' ./config/samples/clustered/verifier/config_v1beta1_verifier_complete_licensechecker.yaml >invalidVerifier.yaml run kubectl apply -f invalidVerifier.yaml assert_success - run bash -c "kubectl describe verifiers.config.ratify.deislabs.io/verifier-license-checker -n ${RATIFY_NAMESPACE} | grep 'Brieferror: Original Error:'" + run bash -c "kubectl describe verifiers.config.ratify.deislabs.io/verifier-license-checker -n ${RATIFY_NAMESPACE} | grep 'Brieferror: PLUGIN_NOT_FOUND:'" assert_success # apply a valid verifier, validate status property shows success