From b62116b3231598b4b3e30bcc462254320e766f85 Mon Sep 17 00:00:00 2001 From: sammyrnycreal Date: Mon, 8 Nov 2021 15:05:18 -0500 Subject: [PATCH] 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 | 7 ++++ zapcore/json_encoder_impl_test.go | 58 +++++++++++++++++++++++++++ zapcore/memory_encoder_test.go | 66 +++++++++++++++++++++++++++++++ 3 files changed, 131 insertions(+) diff --git a/zapcore/json_encoder.go b/zapcore/json_encoder.go index 7af00ddee..7c3faf6c5 100644 --- a/zapcore/json_encoder.go +++ b/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) { diff --git a/zapcore/json_encoder_impl_test.go b/zapcore/json_encoder_impl_test.go index 8db12b673..c1c4c629d 100644 --- a/zapcore/json_encoder_impl_test.go +++ b/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) { diff --git a/zapcore/memory_encoder_test.go b/zapcore/memory_encoder_test.go index 7a3f51acc..052bdb0c6 100644 --- a/zapcore/memory_encoder_test.go +++ b/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 {