diff --git a/zapcore/field.go b/zapcore/field.go index 6a5e33e2f..6a6e4d731 100644 --- a/zapcore/field.go +++ b/zapcore/field.go @@ -160,7 +160,7 @@ func (f Field) AddTo(enc ObjectEncoder) { case NamespaceType: enc.OpenNamespace(f.Key) case StringerType: - enc.AddString(f.Key, f.Interface.(fmt.Stringer).String()) + encodeStringer(f.Key, f.Interface, enc) case ErrorType: encodeError(f.Key, f.Interface.(error), enc) case SkipType: @@ -199,3 +199,25 @@ func addFields(enc ObjectEncoder, fields []Field) { fields[i].AddTo(enc) } } + +func encodeStringer(key string, stringer interface{}, enc ObjectEncoder) { + defer catchPanic(key, enc) + enc.AddString(key, stringer.(fmt.Stringer).String()) +} + +func catchPanic(key string, enc ObjectEncoder) { + if err := recover(); err != nil { + var msg string + + switch v := err.(type) { + case error: + msg = v.Error() + case string: + msg = v + default: + msg = "" + } + + enc.AddString(fmt.Sprintf("%sError", key), msg) + } +} diff --git a/zapcore/field_test.go b/zapcore/field_test.go index 9a5fe0189..d0b8b8d97 100644 --- a/zapcore/field_test.go +++ b/zapcore/field_test.go @@ -24,13 +24,12 @@ import ( "errors" "fmt" "math" + "net/url" "testing" "time" - "go.uber.org/zap" - "github.com/stretchr/testify/assert" - + "go.uber.org/zap" . "go.uber.org/zap/zapcore" ) @@ -58,6 +57,27 @@ func (u users) MarshalLogArray(enc ArrayEncoder) error { return nil } +type obj struct { + kind int +} + +func (o *obj) String() string { + if o == nil { + return "nil obj" + } + + if o.kind == 1 { + panic("panic with string") + } else if o.kind == 2 { + panic(errors.New("panic with error")) + } else if o.kind == 3 { + // recursive - causes a panic when trying to print the panic value + panic(o) + } + + return "obj" +} + func TestUnknownFieldType(t *testing.T) { unknown := Field{Key: "k", String: "foo"} assert.Equal(t, UnknownType, unknown.Type, "Expected zero value of FieldType to be UnknownType.") @@ -67,19 +87,27 @@ func TestUnknownFieldType(t *testing.T) { } func TestFieldAddingError(t *testing.T) { + var empty interface{} tests := []struct { - t FieldType - want interface{} + t FieldType + iface interface{} + want interface{} + err string }{ - {ArrayMarshalerType, []interface{}{}}, - {ObjectMarshalerType, map[string]interface{}{}}, + {t: ArrayMarshalerType, iface: users(-1), want: []interface{}{}, err: "too few users"}, + {t: ObjectMarshalerType, iface: users(-1), want: map[string]interface{}{}, err: "too few users"}, + {t: StringerType, iface: obj{}, want: empty, err: "interface conversion: zapcore_test.obj is not fmt.Stringer: missing method String"}, + {t: StringerType, iface: &obj{1}, want: empty, err: "panic with string"}, + {t: StringerType, iface: &obj{2}, want: empty, err: "panic with error"}, + {t: StringerType, iface: &obj{3}, want: empty, err: ""}, + {t: StringerType, iface: (*url.URL)(nil), want: empty, err: "runtime error: invalid memory address or nil pointer dereference"}, } for _, tt := range tests { - f := Field{Key: "k", Interface: users(-1), Type: tt.t} + f := Field{Key: "k", Interface: tt.iface, Type: tt.t} enc := NewMapObjectEncoder() assert.NotPanics(t, func() { f.AddTo(enc) }, "Unexpected panic when adding fields returns an error.") assert.Equal(t, tt.want, enc.Fields["k"], "On error, expected zero value in field.Key.") - assert.Equal(t, "too few users", enc.Fields["kError"], "Expected error message in log context.") + assert.Equal(t, tt.err, enc.Fields["kError"], "Expected error message in log context.") } } @@ -116,6 +144,8 @@ func TestFields(t *testing.T) { {t: ReflectType, iface: users(2), want: users(2)}, {t: NamespaceType, want: map[string]interface{}{}}, {t: StringerType, iface: users(2), want: "2 users"}, + {t: StringerType, iface: &obj{}, want: "obj"}, + {t: StringerType, iface: (*obj)(nil), want: "nil obj"}, {t: SkipType, want: interface{}(nil)}, }