Skip to content

Commit

Permalink
Demonstrate jsonEncoder and MapObjectEncoder differences
Browse files Browse the repository at this point in the history 
Add test cases to demonstrate uber-go#554,
how jsonEncoder and MapObjectEncoder handle namespaces differently.

Specifically: When foo.MarshalLogObject uses OpenNamespace,
jsonEncoder.AddObject does not close the namespace.
  • Loading branch information
@jkanywhere
jkanywhere committed Feb 4, 2018
1 parent cb3eb92 commit 367a5f7
Show file tree
Hide file tree
Showing 2 changed files with 110 additions and 0 deletions.
44 changes: 44 additions & 0 deletions zapcore/json_encoder_impl_test.go
View file
Expand Up @@ -220,6 +220,22 @@ func TestJSONEncoderObjectFields(t *testing.T) {
e.OpenNamespace("innermost")
},
},
{
desc: "object (no nested namespace)",
expected: `"obj":{"obj-out":"obj-outside-namespace"},"not-obj":"should-be-outside-obj"`,
f: func(e Encoder) {
e.AddObject("obj", maybeNamespace{false})
e.AddString("not-obj", "should-be-outside-obj")
},
},
{
desc: "object (with nested namespace)",
expected: `"obj":{"obj-out":"obj-outside-namespace","obj-namespace":{"obj-in":"obj-inside-namespace"}},"not-obj":"should-be-outside-obj"`,

This comment has been minimized.

Sign in to view

Copy link
@jkanywhere

jkanywhere Feb 4, 2018

Author Owner

Upon reflection I think this expected output is correct, and the fact the test fails represents a bug in zapcore.jsonEncoder. jsonEncoder.AddObject adds an } to close the object itself, but not any nested namespaces.
Fields logged after are added to the object, but not the nested namespace.

    --- PASS: TestJSONEncoderArrays/object_(no_nested_namespace)_then_string (0.00s)
    --- FAIL: TestJSONEncoderArrays/object_(with_nested_namespace)_then_string (0.00s)
        Error Trace:    1: 
    	Error:  	Not equal: "\"array\":[{\"obj-out\":\"obj-outside-namespace\",\"obj-namespace\":{\"obj-in\":\"obj-inside-namespace\"}},\"should-be-outside-obj\",{\"obj-out\":\"obj-outside-namespace\",\"obj-namespace\":{\"obj-in\":\"obj-inside-namespace\"}},\"should-be-outside-obj\"]" (expected)
    			        != "\"array\":[{\"obj-out\":\"obj-outside-namespace\",\"obj-namespace\":{\"obj-in\":\"obj-inside-namespace\"},\"should-be-outside-obj\",{\"obj-out\":\"obj-outside-namespace\",\"obj-namespace\":{\"obj-in\":\"obj-inside-namespace\"},\"should-be-outside-obj\"]" (actual)
    	Messages:	Unexpected encoder output after adding a object (with nested namespace) then string.
    		
        Error Trace:    1: 
    	Error:  	Not equal: "\"foo\":\"bar\",\"array\":[{\"obj-out\":\"obj-outside-namespace\",\"obj-namespace\":{\"obj-in\":\"obj-inside-namespace\"}},\"should-be-outside-obj\",{\"obj-out\":\"obj-outside-namespace\",\"obj-namespace\":{\"obj-in\":\"obj-inside-namespace\"}},\"should-be-outside-obj\"]" (expected)
    			        != "\"foo\":\"bar\",\"array\":[{\"obj-out\":\"obj-outside-namespace\",\"obj-namespace\":{\"obj-in\":\"obj-inside-namespace\"},\"should-be-outside-obj\",{\"obj-out\":\"obj-outside-namespace\",\"obj-namespace\":{\"obj-in\":\"obj-inside-namespace\"},\"should-be-outside-obj\"]" (actual)
    	Messages:	Unexpected encoder output after adding a object (with nested namespace) then string as a second field.
f: func(e Encoder) {
e.AddObject("obj", maybeNamespace{true})
e.AddString("not-obj", "should-be-outside-obj")
},
},
}

for _, tt := range tests {
Expand Down Expand Up @@ -313,6 +329,22 @@ func TestJSONEncoderArrays(t *testing.T) {
)
},
},
{
desc: "object (no nested namespace) then string",
expected: `[{"obj-out":"obj-outside-namespace"},"should-be-outside-obj",{"obj-out":"obj-outside-namespace"},"should-be-outside-obj"]`,
f: func(arr ArrayEncoder) {
arr.AppendObject(maybeNamespace{false})
arr.AppendString("should-be-outside-obj")
},
},
{
desc: "object (with nested namespace) then string",
expected: `[{"obj-out":"obj-outside-namespace","obj-namespace":{"obj-in":"obj-inside-namespace"}},"should-be-outside-obj",{"obj-out":"obj-outside-namespace","obj-namespace":{"obj-in":"obj-inside-namespace"}},"should-be-outside-obj"]`,
f: func(arr ArrayEncoder) {
arr.AppendObject(maybeNamespace{true})
arr.AppendString("should-be-outside-obj")
},
},
}

for _, tt := range tests {
Expand Down Expand Up @@ -400,6 +432,18 @@ func (l loggable) MarshalLogArray(enc ArrayEncoder) error {
return nil
}

// maybeNamespace is an ObjectMarhsaler that sometimes opens a namespace
type maybeNamespace struct{ bool }

func (m maybeNamespace) MarshalLogObject(enc ObjectEncoder) error {
enc.AddString("obj-out", "obj-outside-namespace")
if m.bool {
enc.OpenNamespace("obj-namespace")
enc.AddString("obj-in", "obj-inside-namespace")
}
return nil
}

type noJSON struct{}

func (nj noJSON) MarshalJSON() ([]byte, error) {
Expand Down
66 changes: 66 additions & 0 deletions zapcore/memory_encoder_test.go
View file
Expand Up @@ -202,6 +202,37 @@ func TestMapObjectEncoderAdd(t *testing.T) {
},
},
},
{
desc: "object (no nested namespace) then string",
f: func(e ObjectEncoder) {
e.OpenNamespace("k")
e.AddObject("obj", maybeNamespace{false})
e.AddString("not-obj", "should-be-outside-obj")
},
expected: map[string]interface{}{
"obj": map[string]interface{}{
"obj-out": "obj-outside-namespace",
},
"not-obj": "should-be-outside-obj",
},
},
{
desc: "object (with nested namespace) then string",
f: func(e ObjectEncoder) {
e.OpenNamespace("k")
e.AddObject("obj", maybeNamespace{true})
e.AddString("not-obj", "should-be-outside-obj")
},
expected: map[string]interface{}{
"obj": map[string]interface{}{
"obj-out": "obj-outside-namespace",
"obj-namespace": map[string]interface{}{
"obj-in": "obj-inside-namespace",
},
},
"not-obj": "should-be-outside-obj",
},
},
}

for _, tt := range tests {
Expand Down Expand Up @@ -255,6 +286,41 @@ func TestSliceArrayEncoderAppend(t *testing.T) {
},
expected: []interface{}{true, false},
},
{
desc: "object (no nested namespace) then string",
f: func(e ArrayEncoder) {
e.AppendArray(ArrayMarshalerFunc(func(inner ArrayEncoder) error {
inner.AppendObject(maybeNamespace{false})
inner.AppendString("should-be-outside-obj")
return nil
}))
},
expected: []interface{}{
map[string]interface{}{
"obj-out": "obj-outside-namespace",
},
"should-be-outside-obj",
},
},
{
desc: "object (with nested namespace) then string",
f: func(e ArrayEncoder) {
e.AppendArray(ArrayMarshalerFunc(func(inner ArrayEncoder) error {
inner.AppendObject(maybeNamespace{true})
inner.AppendString("should-be-outside-obj")
return nil
}))
},
expected: []interface{}{
map[string]interface{}{
"obj-out": "obj-outside-namespace",
"obj-namespace": map[string]interface{}{
"obj-in": "obj-inside-namespace",
},
},
"should-be-outside-obj",
},
},
}

for _, tt := range tests {
Expand Down

0 comments on commit 367a5f7

Please sign in to comment.