Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Don't panic when a given field value is nil
Browse files Browse the repository at this point in the history
This commit ensures that zap.Stringer, zap.Object and zap.Any don't
cause a panic when the given field value is nil.
joa committed Mar 6, 2019
1 parent d2a364d commit 69c4aaa
Showing 2 changed files with 65 additions and 9 deletions.
24 changes: 24 additions & 0 deletions zapcore/field.go
Original file line number Diff line number Diff line change
@@ -107,6 +107,29 @@ type Field struct {
func (f Field) AddTo(enc ObjectEncoder) {
var err error

rec := true

defer func() {
if !rec {
return
}

if r := recover(); r != nil {
var msg string

switch v := r.(type) {
case error:
msg = v.Error()
case string:
msg = v
default:
msg = "<unknown>"
}

enc.AddString(fmt.Sprintf("%sError", f.Key), msg)
}
}()

switch f.Type {
case ArrayMarshalerType:
err = enc.AddArray(f.Key, f.Interface.(ArrayMarshaler))
@@ -166,6 +189,7 @@ func (f Field) AddTo(enc ObjectEncoder) {
case SkipType:
break
default:
rec = false
panic(fmt.Sprintf("unknown field type: %v", f))
}

50 changes: 41 additions & 9 deletions zapcore/field_test.go
Original file line number Diff line number Diff line change
@@ -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,29 @@ 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: "<unknown>"},
{t: StringerType, iface: (*url.URL)(nil), want: empty, err: "runtime error: invalid memory address or nil pointer dereference"},
{t: ArrayMarshalerType, iface: (ArrayMarshaler)(nil), want: empty, err: "interface conversion: interface is nil, not zapcore.ArrayMarshaler"},
{t: ObjectMarshalerType, iface: (ObjectMarshaler)(nil), want: empty, err: "interface conversion: interface is nil, not zapcore.ObjectMarshaler"},
}
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 +146,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)},
}

0 comments on commit 69c4aaa

Please sign in to comment.