From 6a4971d4ceb1b19d01e0bcbf0b63db7e35ccd3d1 Mon Sep 17 00:00:00 2001 From: Justin Hwang Date: Wed, 7 Feb 2024 12:52:40 -0500 Subject: [PATCH] zapcore: Support ObjectMarshaler for error types Previously, implementing `ObjectMarshaler` on an error type was ignored unless the error was logged with `zap.Any` or `zap.Object`. We should respect the `ObjectMarshaler` instead as this is more flexible than the basic `error` or `fmt.Formatter` interfaces. Resolves #1365 --- zapcore/error.go | 23 ++++++++-- zapcore/error_test.go | 102 ++++++++++++++++++++++++++++-------------- 2 files changed, 88 insertions(+), 37 deletions(-) diff --git a/zapcore/error.go b/zapcore/error.go index c40df1326..4e526f6a1 100644 --- a/zapcore/error.go +++ b/zapcore/error.go @@ -27,8 +27,12 @@ import ( "go.uber.org/zap/internal/pool" ) -// Encodes the given error into fields of an object. A field with the given -// name is added for the error message. +// Encodes the given error into fields of an object. +// +// If the error implements ObjectMarshaler, this takes precedence and a field +// with the given name is added with the corresponding MarshalLogObject. +// +// Otherwise, a field with the given name is added for the error message. // // If the error implements fmt.Formatter, a field with the name ${key}Verbose // is also added with the full verbose error message. @@ -37,6 +41,16 @@ import ( // causer (from github.com/pkg/errors), a ${key}Causes field is added with an // array of objects containing the errors this error was comprised of. // +// In the case of a ObjectMarshaler, +// +// { +// "error": { +// ... +// } +// } +// +// Otherwise, +// // { // "error": err.Error(), // "errorVerbose": fmt.Sprintf("%+v", err), @@ -60,6 +74,9 @@ func encodeError(key string, err error, enc ObjectEncoder) (retErr error) { retErr = fmt.Errorf("PANIC=%v", rerr) } }() + if obj, ok := err.(ObjectMarshaler); ok { + return enc.AddObject(key, obj) + } basic := err.Error() enc.AddString(key, basic) @@ -79,7 +96,7 @@ func encodeError(key string, err error, enc ObjectEncoder) (retErr error) { } type errorGroup interface { - // Provides read-only access to the underlying list of errors, preferably + // Errors provides read-only access to the underlying list of errors, preferably // without causing any allocs. Errors() []error } diff --git a/zapcore/error_test.go b/zapcore/error_test.go index 41f243ac4..458b40b6f 100644 --- a/zapcore/error_test.go +++ b/zapcore/error_test.go @@ -43,7 +43,20 @@ func (e errTooManyUsers) Format(s fmt.State, verb rune) { // Implement fmt.Formatter, but don't add any information beyond the basic // Error method. if verb == 'v' && s.Flag('+') { - io.WriteString(s, e.Error()) + _, _ = io.WriteString(s, e.Error()) + } +} + +type errTooFewUsers int + +func (e errTooFewUsers) Error() string { + return fmt.Sprintf("%d too few users", int(e)) +} + +func (e errTooFewUsers) Format(s fmt.State, verb rune) { + if verb == 'v' && s.Flag('+') { + _, _ = io.WriteString(s, "verbose: ") + _, _ = io.WriteString(s, e.Error()) } } @@ -64,62 +77,87 @@ func (e customMultierr) Errors() []error { } } +type customObjMarshalerErr struct{} + +func (e customObjMarshalerErr) Error() string { return "object marshaler string" } + +func (e customObjMarshalerErr) MarshalLogObject(enc ObjectEncoder) error { + enc.AddString("nested", "object marshaler nested") + return nil +} + func TestErrorEncoding(t *testing.T) { tests := []struct { - k string - t FieldType // defaults to ErrorType - iface interface{} - want map[string]interface{} + key string + iface any + want map[string]any }{ { - k: "k", + key: "k", + iface: customObjMarshalerErr{}, + want: map[string]any{ + "k": map[string]any{ + "nested": "object marshaler nested", + }, + }, + }, + { + key: "k", iface: errTooManyUsers(2), - want: map[string]interface{}{ + want: map[string]any{ "k": "2 too many users", }, }, { - k: "err", + key: "k", + iface: errTooFewUsers(2), + want: map[string]any{ + "k": "2 too few users", + "kVerbose": "verbose: 2 too few users", + }, + }, + { + key: "err", iface: multierr.Combine( errors.New("foo"), errors.New("bar"), errors.New("baz"), ), - want: map[string]interface{}{ + want: map[string]any{ "err": "foo; bar; baz", - "errCauses": []interface{}{ - map[string]interface{}{"error": "foo"}, - map[string]interface{}{"error": "bar"}, - map[string]interface{}{"error": "baz"}, + "errCauses": []any{ + map[string]any{"error": "foo"}, + map[string]any{"error": "bar"}, + map[string]any{"error": "baz"}, }, }, }, { - k: "e", + key: "e", iface: customMultierr{}, - want: map[string]interface{}{ + want: map[string]any{ "e": "great sadness", - "eCauses": []interface{}{ - map[string]interface{}{"error": "foo"}, - map[string]interface{}{ + "eCauses": []any{ + map[string]any{"error": "foo"}, + map[string]any{ "error": "bar; baz", - "errorCauses": []interface{}{ - map[string]interface{}{"error": "bar"}, - map[string]interface{}{"error": "baz"}, + "errorCauses": []any{ + map[string]any{"error": "bar"}, + map[string]any{"error": "baz"}, }, }, }, }, }, { - k: "k", + key: "k", iface: fmt.Errorf("failed: %w", errors.New("egad")), - want: map[string]interface{}{ + want: map[string]any{ "k": "failed: egad", }, }, { - k: "error", + key: "error", iface: multierr.Combine( fmt.Errorf("hello: %w", multierr.Combine(errors.New("foo"), errors.New("bar")), @@ -127,26 +165,22 @@ func TestErrorEncoding(t *testing.T) { errors.New("baz"), fmt.Errorf("world: %w", errors.New("qux")), ), - want: map[string]interface{}{ + want: map[string]any{ "error": "hello: foo; bar; baz; world: qux", - "errorCauses": []interface{}{ - map[string]interface{}{ + "errorCauses": []any{ + map[string]any{ "error": "hello: foo; bar", }, - map[string]interface{}{"error": "baz"}, - map[string]interface{}{"error": "world: qux"}, + map[string]any{"error": "baz"}, + map[string]any{"error": "world: qux"}, }, }, }, } for _, tt := range tests { - if tt.t == UnknownType { - tt.t = ErrorType - } - enc := NewMapObjectEncoder() - f := Field{Key: tt.k, Type: tt.t, Interface: tt.iface} + f := Field{Key: tt.key, Type: ErrorType, Interface: tt.iface} f.AddTo(enc) assert.Equal(t, tt.want, enc.Fields, "Unexpected output from field %+v.", f) }