jsonEncoder: Close namespaces opened in an object (#1017)

A bug in the implementation of jsonEncoder did not close namespaces
opened inside a MarshalLogObject method when the method exited. It
waited until the entire entry was encoded.

For example,

    type foo struct{ ... }

    func (*foo) MarshalLogObject(enc zapcore.ObjectEncoder) error {
        enc.OpenNamespace("ns")
        enc.AddString("x", "y")
        return nil
    }

    log.Info("msg", zap.Object("foo", &foo{..}), zap.Int("bar", 1))

We expect,

    {"msg": "msg", "foo": {"ns": {"x": "y"}}, "bar": 1}

But with the JSON encoder, we currently get,

    {"msg": "msg", "foo": {"ns": {"x": "y"}, "bar": 1}}

(`"bar"` has become part of the "ns" object created in "foo".)

This resolves the issue by tracking the number of namespaces for each
nested object separately, and closing only those namespaces that were
created during that object's MarshalLogObject call.

Resolves #554

zapcore/json_encoder.go

@@ -211,10 +211,16 @@ func (enc *jsonEncoder) AppendArray(arr ArrayMarshaler) error {
 }
 
 func (enc *jsonEncoder) AppendObject(obj ObjectMarshaler) error {
+	// Close ONLY new openNamespaces that are created during
+	// AppendObject().
+	old := enc.openNamespaces
+	enc.openNamespaces = 0
 	enc.addElementSeparator()
 	enc.buf.AppendByte('{')
 	err := obj.MarshalLogObject(enc)
 	enc.buf.AppendByte('}')
+	enc.closeOpenNamespaces()
+	enc.openNamespaces = old
 	return err
 }
 
@@ -431,6 +437,7 @@ func (enc *jsonEncoder) closeOpenNamespaces() {
 	for i := 0; i < enc.openNamespaces; i++ {
 		enc.buf.AppendByte('}')
 	}
+	enc.openNamespaces = 0
 }
 
 func (enc *jsonEncoder) addKey(key string) {

zapcore/json_encoder_impl_test.go

@@ -245,6 +245,36 @@ 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"`,
+			f: func(e Encoder) {
+				e.AddObject("obj", maybeNamespace{true})
+				e.AddString("not-obj", "should-be-outside-obj")
+			},
+		},
+		{
+			desc:     "multiple open namespaces",
+			expected: `"k":{"foo":1,"middle":{"foo":2,"inner":{"foo":3}}}`,
+			f: func(e Encoder) {
+				e.AddObject("k", ObjectMarshalerFunc(func(enc ObjectEncoder) error {
+					e.AddInt("foo", 1)
+					e.OpenNamespace("middle")
+					e.AddInt("foo", 2)
+					e.OpenNamespace("inner")
+					e.AddInt("foo", 3)
+					return nil
+				}))
+			},
+		},
 	}
 
 	for _, tt := range tests {
@@ -386,6 +416,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 {
@@ -522,6 +568,18 @@ func (l loggable) MarshalLogArray(enc ArrayEncoder) error {
 	return nil
 }
 
+// maybeNamespace is an ObjectMarshaler 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) {

zapcore/memory_encoder_test.go

@@ -214,6 +214,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 {
@@ -268,6 +299,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 {