Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MapObjectEncoder: Empty arrays should not be nil #614

Merged
merged 2 commits into from
Jul 31, 2018
Merged

MapObjectEncoder: Empty arrays should not be nil #614

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
307d065
Replace null with empty slices for MapObjectEncoder AddArray
mh-park Jul 30, 2018
9e0ecf5
Add change to changelog, reflect code review
mh-park Jul 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
# Changelog

## Unreleased

Bugfixes:
* [#614][]: MapObjectEncoder should not ignore empty slices.

## v1.9.0 (19 Jul 2018)

Enhancements:
Expand Down Expand Up @@ -296,3 +301,4 @@ upgrade to the upcoming stable release.
[#602]: https://github.com/uber-go/zap/pull/602
[#572]: https://github.com/uber-go/zap/pull/572
[#606]: https://github.com/uber-go/zap/pull/606
[#614]: https://github.com/uber-go/zap/pull/614
40 changes: 20 additions & 20 deletions array_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,26 +55,26 @@ func TestArrayWrappers(t *testing.T) {
field Field
expected []interface{}
}{
{"empty bools", Bools("", []bool{}), []interface{}(nil)},
{"empty byte strings", ByteStrings("", [][]byte{}), []interface{}(nil)},
{"empty complex128s", Complex128s("", []complex128{}), []interface{}(nil)},
{"empty complex64s", Complex64s("", []complex64{}), []interface{}(nil)},
{"empty durations", Durations("", []time.Duration{}), []interface{}(nil)},
{"empty float64s", Float64s("", []float64{}), []interface{}(nil)},
{"empty float32s", Float32s("", []float32{}), []interface{}(nil)},
{"empty ints", Ints("", []int{}), []interface{}(nil)},
{"empty int64s", Int64s("", []int64{}), []interface{}(nil)},
{"empty int32s", Int32s("", []int32{}), []interface{}(nil)},
{"empty int16s", Int16s("", []int16{}), []interface{}(nil)},
{"empty int8s", Int8s("", []int8{}), []interface{}(nil)},
{"empty strings", Strings("", []string{}), []interface{}(nil)},
{"empty times", Times("", []time.Time{}), []interface{}(nil)},
{"empty uints", Uints("", []uint{}), []interface{}(nil)},
{"empty uint64s", Uint64s("", []uint64{}), []interface{}(nil)},
{"empty uint32s", Uint32s("", []uint32{}), []interface{}(nil)},
{"empty uint16s", Uint16s("", []uint16{}), []interface{}(nil)},
{"empty uint8s", Uint8s("", []uint8{}), []interface{}(nil)},
{"empty uintptrs", Uintptrs("", []uintptr{}), []interface{}(nil)},
{"empty bools", Bools("", []bool{}), []interface{}{}},
{"empty byte strings", ByteStrings("", [][]byte{}), []interface{}{}},
{"empty complex128s", Complex128s("", []complex128{}), []interface{}{}},
{"empty complex64s", Complex64s("", []complex64{}), []interface{}{}},
{"empty durations", Durations("", []time.Duration{}), []interface{}{}},
{"empty float64s", Float64s("", []float64{}), []interface{}{}},
{"empty float32s", Float32s("", []float32{}), []interface{}{}},
{"empty ints", Ints("", []int{}), []interface{}{}},
{"empty int64s", Int64s("", []int64{}), []interface{}{}},
{"empty int32s", Int32s("", []int32{}), []interface{}{}},
{"empty int16s", Int16s("", []int16{}), []interface{}{}},
{"empty int8s", Int8s("", []int8{}), []interface{}{}},
{"empty strings", Strings("", []string{}), []interface{}{}},
{"empty times", Times("", []time.Time{}), []interface{}{}},
{"empty uints", Uints("", []uint{}), []interface{}{}},
{"empty uint64s", Uint64s("", []uint64{}), []interface{}{}},
{"empty uint32s", Uint32s("", []uint32{}), []interface{}{}},
{"empty uint16s", Uint16s("", []uint16{}), []interface{}{}},
{"empty uint8s", Uint8s("", []uint8{}), []interface{}{}},
{"empty uintptrs", Uintptrs("", []uintptr{}), []interface{}{}},
{"bools", Bools("", []bool{true, false}), []interface{}{true, false}},
{"byte strings", ByteStrings("", [][]byte{{1, 2}, {3, 4}}), []interface{}{[]byte{1, 2}, []byte{3, 4}}},
{"complex128s", Complex128s("", []complex128{1 + 2i, 3 + 4i}), []interface{}{1 + 2i, 3 + 4i}},
Expand Down
2 changes: 1 addition & 1 deletion error_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ func TestErrorArrayConstructor(t *testing.T) {
field Field
expected []interface{}
}{
{"empty errors", Errors("", []error{}), []interface{}(nil)},
{"empty errors", Errors("", []error{}), []interface{}{}},
{
"errors",
Errors("", []error{nil, errors.New("foo"), nil, errors.New("bar")}),
Expand Down
2 changes: 1 addition & 1 deletion zapcore/field_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ func TestFieldAddingError(t *testing.T) {
t FieldType
want interface{}
}{
{ArrayMarshalerType, []interface{}(nil)},
{ArrayMarshalerType, []interface{}{}},
{ObjectMarshalerType, map[string]interface{}{}},
}
for _, tt := range tests {
Expand Down
2 changes: 1 addition & 1 deletion zapcore/memory_encoder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ func NewMapObjectEncoder() *MapObjectEncoder {

// AddArray implements ObjectEncoder.
func (m *MapObjectEncoder) AddArray(key string, v ArrayMarshaler) error {
arr := &sliceArrayEncoder{}
arr := &sliceArrayEncoder{elems: make([]interface{}, 0)}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 unnecessary in make(..) because it's the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially tried it without the 0 and zapcore/memory_encoder.go:46:39: missing len argument to make([]interface {}) popped up

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Derp, you're right.

err := v.MarshalLogArray(arr)
m.cur[key] = arr.elems
return err
Expand Down
7 changes: 7 additions & 0 deletions zapcore/memory_encoder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,13 @@ func TestMapObjectEncoderAdd(t *testing.T) {
},
expected: []interface{}{wantTurducken, wantTurducken},
},
{
desc: "AddArray (empty)",
f: func(e ObjectEncoder) {
assert.NoError(t, e.AddArray("k", turduckens(0)), "Expected AddArray to succeed.")
},
expected: []interface{}{},
},
{
desc: "AddBinary",
f: func(e ObjectEncoder) { e.AddBinary("k", []byte("foo")) },
Expand Down