Skip to content

Commit

Permalink
clienterrors: detect and error on deeply nested errors
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
mvo5 committed May 8, 2024
1 parent 78bff73 commit c7f6fdf
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 7 deletions.
42 changes: 41 additions & 1 deletion internal/worker/clienterrors/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package clienterrors
import (
"encoding/json"
"fmt"
"reflect"
)

const (
Expand Down Expand Up @@ -59,13 +60,52 @@ 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 = fmt.Sprintf("%v", v)
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 {
Expand Down
39 changes: 33 additions & 6 deletions internal/worker/clienterrors/errors_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}

0 comments on commit c7f6fdf

Please sign in to comment.