From 9160dc3c9b706b829814b4aa65efa538fb66a300 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Wed, 8 May 2024 14:52:49 +0200 Subject: [PATCH 1/4] clientdetails: add test for error reporting We have no tests for the clienterror, start with a trivial one. --- internal/worker/clienterrors/errors_test.go | 29 +++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 internal/worker/clienterrors/errors_test.go diff --git a/internal/worker/clienterrors/errors_test.go b/internal/worker/clienterrors/errors_test.go new file mode 100644 index 0000000000..b7c3ebbd65 --- /dev/null +++ b/internal/worker/clienterrors/errors_test.go @@ -0,0 +1,29 @@ +package clienterrors_test + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/osbuild/osbuild-composer/internal/worker/clienterrors" +) + +type customErr struct{} + +func (ce *customErr) Error() string { + return "customErr" +} + +func TestErrorInterface(t *testing.T) { + for _, tc := range []struct { + err error + expectedStr string + }{ + {fmt.Errorf("some error"), "some error"}, + {&customErr{}, "customErr"}, + } { + wce := clienterrors.WorkerClientError(2, "details", tc.err) + assert.Equal(t, fmt.Sprintf("Code: 2, Reason: details, Details: %s", tc.expectedStr), wce.String()) + } +} From 4165bf492add17ea9cafcabb1d1e696056317a08 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Wed, 8 May 2024 15:07:20 +0200 Subject: [PATCH 2/4] clienterrors: add (failing) test to reproduce error interface handling --- internal/worker/clienterrors/errors_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/internal/worker/clienterrors/errors_test.go b/internal/worker/clienterrors/errors_test.go index b7c3ebbd65..b492e7c25f 100644 --- a/internal/worker/clienterrors/errors_test.go +++ b/internal/worker/clienterrors/errors_test.go @@ -1,6 +1,7 @@ package clienterrors_test import ( + "encoding/json" "fmt" "testing" @@ -27,3 +28,11 @@ func TestErrorInterface(t *testing.T) { assert.Equal(t, fmt.Sprintf("Code: 2, Reason: details, Details: %s", tc.expectedStr), wce.String()) } } + +func TestErrorJSONMarshal(t *testing.T) { + err := fmt.Errorf("some-error") + + json, err := json.Marshal(clienterrors.WorkerClientError(2, "details", err)) + assert.NoError(t, err) + assert.Equal(t, `{"id":2,"reason":"details","details":{"some-error"}}`, string(json)) +} From d4300fcc60ffa69c025942289050b8b971700434 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Wed, 8 May 2024 15:19:23 +0200 Subject: [PATCH 3/4] clienterrors: add (one possible) way to fix the missing errors --- internal/worker/clienterrors/errors.go | 21 +++++++++++++++++++++ internal/worker/clienterrors/errors_test.go | 2 +- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/internal/worker/clienterrors/errors.go b/internal/worker/clienterrors/errors.go index e575c57412..ed3884a28e 100644 --- a/internal/worker/clienterrors/errors.go +++ b/internal/worker/clienterrors/errors.go @@ -1,6 +1,7 @@ package clienterrors import ( + "encoding/json" "fmt" ) @@ -58,6 +59,26 @@ func (e *Error) String() string { return fmt.Sprintf("Code: %d, Reason: %s, Details: %v", e.ID, e.Reason, e.Details) } +func (e *Error) MarshalJSON() ([]byte, error) { + var details interface{} + switch v := e.Details.(type) { + case error: + details = v.Error() + default: + details = v + } + + return json.Marshal(&struct { + ID ClientErrorCode `json:"id"` + Reason string `json:"reason"` + Details interface{} `json:"details,omitempty"` + }{ + ID: e.ID, + Reason: e.Reason, + Details: details, + }) +} + const ( JobStatusSuccess = "2xx" JobStatusUserInputError = "4xx" diff --git a/internal/worker/clienterrors/errors_test.go b/internal/worker/clienterrors/errors_test.go index b492e7c25f..2e7eb9401a 100644 --- a/internal/worker/clienterrors/errors_test.go +++ b/internal/worker/clienterrors/errors_test.go @@ -34,5 +34,5 @@ func TestErrorJSONMarshal(t *testing.T) { json, err := json.Marshal(clienterrors.WorkerClientError(2, "details", err)) assert.NoError(t, err) - assert.Equal(t, `{"id":2,"reason":"details","details":{"some-error"}}`, string(json)) + assert.Equal(t, `{"id":2,"reason":"details","details":"some-error"}`, string(json)) } From 7a862d4932f8d979674a24d2ae9a10eb0cf92029 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Wed, 8 May 2024 16:56:28 +0200 Subject: [PATCH 4/4] clienterrors: detect and error on deeply nested errors Florian pointed out that `clienterror.Error.Details` could contain arbitrarily deeply nested error values. Add code to detect this. Also deal with the (common?) case that `.Details` contains a list of errors. --- internal/worker/clienterrors/errors.go | 47 ++++++++++++++++++++- internal/worker/clienterrors/errors_test.go | 39 ++++++++++++++--- 2 files changed, 79 insertions(+), 7 deletions(-) diff --git a/internal/worker/clienterrors/errors.go b/internal/worker/clienterrors/errors.go index ed3884a28e..156ca87908 100644 --- a/internal/worker/clienterrors/errors.go +++ b/internal/worker/clienterrors/errors.go @@ -3,6 +3,7 @@ package clienterrors import ( "encoding/json" "fmt" + "reflect" ) const ( @@ -59,13 +60,57 @@ func (e *Error) String() string { return fmt.Sprintf("Code: %d, Reason: %s, Details: %v", e.ID, e.Reason, e.Details) } +func ensureNoErrs(v reflect.Value) error { + switch v.Kind() { + case reflect.Interface: + if errIf, ok := v.Interface().(error); ok { + if errIf != nil { + return fmt.Errorf("%v", errIf.Error()) + } + } + case reflect.Ptr: + if err := ensureNoErrs(v.Elem()); err != nil { + return err + } + case reflect.Struct: + for i := 0; i < v.NumField(); i++ { + if err := ensureNoErrs(v.Field(i)); err != nil { + return err + } + } + case reflect.Slice, reflect.Array: + for i := 0; i < v.Len(); i++ { + if err := ensureNoErrs(v.Index(i)); err != nil { + return err + } + } + case reflect.Map: + for _, key := range v.MapKeys() { + if err := ensureNoErrs(v.MapIndex(key)); err != nil { + return err + } + } + } + return nil +} + func (e *Error) MarshalJSON() ([]byte, error) { var details interface{} switch v := e.Details.(type) { case error: details = v.Error() + case []error: + details = make([]string, 0, len(v)) + for _, err := range v { + if err != nil { + details = append(details.([]string), err.Error()) + } + } default: - details = v + if err := ensureNoErrs(reflect.ValueOf(v)); err != nil { + return nil, fmt.Errorf("found nested error in %+v: %v", e.Details, err) + } + details = e.Details } return json.Marshal(&struct { diff --git a/internal/worker/clienterrors/errors_test.go b/internal/worker/clienterrors/errors_test.go index 2e7eb9401a..1f986558d7 100644 --- a/internal/worker/clienterrors/errors_test.go +++ b/internal/worker/clienterrors/errors_test.go @@ -24,15 +24,42 @@ func TestErrorInterface(t *testing.T) { {fmt.Errorf("some error"), "some error"}, {&customErr{}, "customErr"}, } { - wce := clienterrors.WorkerClientError(2, "details", tc.err) - assert.Equal(t, fmt.Sprintf("Code: 2, Reason: details, Details: %s", tc.expectedStr), wce.String()) + wce := clienterrors.WorkerClientError(2, "reason", tc.err) + assert.Equal(t, fmt.Sprintf("Code: 2, Reason: reason, Details: %s", tc.expectedStr), wce.String()) } } func TestErrorJSONMarshal(t *testing.T) { - err := fmt.Errorf("some-error") + for _, tc := range []struct { + err interface{} + expectedStr string + }{ + {fmt.Errorf("some-error"), `"some-error"`}, + {[]error{fmt.Errorf("err1"), fmt.Errorf("err2")}, `["err1","err2"]`}, + {"random detail", `"random detail"`}, + } { + json, err := json.Marshal(clienterrors.WorkerClientError(2, "reason", tc.err)) + assert.NoError(t, err) + assert.Equal(t, fmt.Sprintf(`{"id":2,"reason":"reason","details":%s}`, tc.expectedStr), string(json)) + } +} - json, err := json.Marshal(clienterrors.WorkerClientError(2, "details", err)) - assert.NoError(t, err) - assert.Equal(t, `{"id":2,"reason":"details","details":"some-error"}`, string(json)) +func TestErrorJSONMarshalDetectsNestedErrs(t *testing.T) { + details := struct { + Unrelated string + NestedErr error + Nested struct { + DeepErr error + } + }{ + Unrelated: "unrelated", + NestedErr: fmt.Errorf("some-nested-error"), + Nested: struct { + DeepErr error + }{ + DeepErr: fmt.Errorf("deep-err"), + }, + } + _, err := json.Marshal(clienterrors.WorkerClientError(2, "reason", details)) + assert.Equal(t, `json: error calling MarshalJSON for type *clienterrors.Error: found nested error in {Unrelated:unrelated NestedErr:some-nested-error Nested:{DeepErr:deep-err}}: some-nested-error`, err.Error()) }