Skip to content

Commit

Permalink
Fix for Issue:
Browse files Browse the repository at this point in the history 
MapObjectEncoder does not match jsonEncoder
(uber-go#554)

 Problem was that new openNamespaces that are created during AppendObject() call were not closed.

 Note, ONLY new openNamespaces that are created during AppendObject() are closed. If they were created at prior to AppendObject() call, it will remain open till EncodeEntry() call completes.
  • Loading branch information
@sammyrnycreal
sammyrnycreal committed Oct 3, 2021
1 parent 10d89a7 commit 82a8da2
Show file tree
Hide file tree
Showing 6 changed files with 126 additions and 4 deletions.
3 changes: 1 addition & 2 deletions benchmarks/go.sum
View file
Expand Up @@ -69,8 +69,7 @@ github.com/yuin/goldmark v1.3.5/go.mod h1:mwnBkeHKe2W/ZEtQ+71ViKU8L12m81fl3OWwC1
github.com/zenazn/goji v0.9.0/go.mod h1:7S9M489iMyHBNxwZnk9/EHS098H4/F6TATF2mIxtB1Q=
go.uber.org/atomic v1.7.0 h1:ADUqmZGgLDDfbSL9ZmPxKTybcoEYHgpYfELNoN+7hsw=
go.uber.org/atomic v1.7.0/go.mod h1:fEN4uk6kAWBTFdckzkM89CLk9XfWZrxpCo0nPH17wJc=
go.uber.org/goleak v1.1.11-0.20210813005559-691160354723 h1:sHOAIxRGBp443oHZIPB+HsUGaksVCXVQENPxwTfQdH4=
go.uber.org/goleak v1.1.11-0.20210813005559-691160354723/go.mod h1:cwTWslyiVhfpKIDGSZEM2HlOvcqm+tG4zioyIeLoqMQ=
go.uber.org/goleak v1.1.11 h1:wy28qYRKZgnJTxGxvye5/wgWr1EKjmUDGYox5mGlRlI=
go.uber.org/goleak v1.1.11/go.mod h1:cwTWslyiVhfpKIDGSZEM2HlOvcqm+tG4zioyIeLoqMQ=
go.uber.org/multierr v1.6.0 h1:y6IPFStTAIT5Ytl7/XYmHvzXQ7S3g/IeZW9hyZ5thw4=
go.uber.org/multierr v1.6.0/go.mod h1:cdWPpRnG4AhwMwsgIHip0KRBQjJy5kYEpYjJxpXp9iU=
Expand Down
8 changes: 8 additions & 0 deletions go.sum
View file
Expand Up @@ -5,17 +5,20 @@ github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c
github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/kr/pretty v0.1.0 h1:L/CwN0zerZDmRFUapSPitk6f+Q3+0za1rQkzVuMiMFI=
github.com/kr/pretty v0.1.0/go.mod h1:dAy3ld7l9f0ibDNOQOHHMYYIIbhfbHSm3C4ZsoJORNo=
github.com/kr/pty v1.1.1 h1:VkoXIwSboBpnk99O/KFauAEILuNHv5DVFKZMBN/gUgw=
github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ=
github.com/kr/text v0.1.0 h1:45sCR5RtlFHMR4UwH9sdQ5TC8v0qDQCHnXt+kaKSTVE=
github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI=
github.com/pkg/errors v0.8.1 h1:iURUrRGxPUNPdy5/HRSm+Yj6okJ6UtLINN0Q9M4+h3I=
github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
github.com/stretchr/objx v0.1.0 h1:4G4v2dO3VZwixGIRoQ5Lfboy6nUhCyYzaqnIAPPhYs4=
github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=
github.com/stretchr/testify v1.7.0 h1:nwc3DEeHmmLAfoZucVR881uASk0Mfjw8xYJ99tb5CcY=
github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
github.com/yuin/goldmark v1.3.5 h1:dPmz1Snjq0kmkz159iL7S6WzdahUTHnHB5M56WFVifs=
github.com/yuin/goldmark v1.3.5/go.mod h1:mwnBkeHKe2W/ZEtQ+71ViKU8L12m81fl3OWwC1Zlc8k=
go.uber.org/atomic v1.7.0 h1:ADUqmZGgLDDfbSL9ZmPxKTybcoEYHgpYfELNoN+7hsw=
go.uber.org/atomic v1.7.0/go.mod h1:fEN4uk6kAWBTFdckzkM89CLk9XfWZrxpCo0nPH17wJc=
Expand All @@ -24,6 +27,7 @@ go.uber.org/goleak v1.1.11/go.mod h1:cwTWslyiVhfpKIDGSZEM2HlOvcqm+tG4zioyIeLoqMQ
go.uber.org/multierr v1.6.0 h1:y6IPFStTAIT5Ytl7/XYmHvzXQ7S3g/IeZW9hyZ5thw4=
go.uber.org/multierr v1.6.0/go.mod h1:cdWPpRnG4AhwMwsgIHip0KRBQjJy5kYEpYjJxpXp9iU=
golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=
golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550 h1:ObdrDkeb4kJdCP557AjRjq69pTHfNouLtWZG7j9rPN8=
golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI=
golang.org/x/lint v0.0.0-20190930215403-16217165b5de h1:5hukYrvBGR8/eNkX5mdUezrA6JiaEZDtJb9Ei+1LlBs=
golang.org/x/lint v0.0.0-20190930215403-16217165b5de/go.mod h1:6SW0HCj/g11FgYtHlgUYUwCkIfeOF89ocIRzGO/8vkc=
Expand All @@ -32,17 +36,21 @@ golang.org/x/mod v0.4.2/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA=
golang.org/x/net v0.0.0-20190311183353-d8887717615a/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg=
golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg=
golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s=
golang.org/x/net v0.0.0-20210405180319-a5a99cb37ef4 h1:4nGaVu0QrbjT/AK2PRLuQfQuh6DJve+pELhqTdAj3x0=
golang.org/x/net v0.0.0-20210405180319-a5a99cb37ef4/go.mod h1:p54w0d4576C0XHj96bSt6lcn1PtDYWL6XObtHCRCNQM=
golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/sync v0.0.0-20210220032951-036812b2e83c h1:5KslGYwFpkhGh+Q16bwMP3cOontH8FOep7tGV86Y7SQ=
golang.org/x/sync v0.0.0-20210220032951-036812b2e83c/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20201119102817-f84b799fce68/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20210330210617-4fbd30eecc44/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20210510120138-977fb7262007 h1:gG67DSER+11cZvqIMb8S8bt0vZtiN6xWYARwirrOSfE=
golang.org/x/sys v0.0.0-20210510120138-977fb7262007/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1 h1:v+OssWQX+hTHEmOBgwxdZxK4zHq3yOs8F9J7mk0PY8E=
golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=
golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
golang.org/x/text v0.3.3 h1:cokOdA+Jmi5PJGXLlLllQSgYigAEfHXJAERHVMaCc2k=
golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ=
golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
golang.org/x/tools v0.0.0-20190311212946-11955173bddd/go.mod h1:LCzVGOaR6xXOjkQ3onu1FJEFr0SW1gC7cKk1uF8kGRs=
Expand Down
6 changes: 6 additions & 0 deletions zapcore/json_encoder.go
View file
Expand Up @@ -211,10 +211,15 @@ func (enc *jsonEncoder) AppendArray(arr ArrayMarshaler) error {
}

func (enc *jsonEncoder) AppendObject(obj ObjectMarshaler) error {
// close ONLY new openNamespaces that are created during AppendObject()
i := enc.openNamespaces
enc.openNamespaces = 0
enc.addElementSeparator()
enc.buf.AppendByte('{')
err := obj.MarshalLogObject(enc)
enc.buf.AppendByte('}')
enc.closeOpenNamespaces()
enc.openNamespaces = i
return err
}

Expand Down Expand Up @@ -431,6 +436,7 @@ func (enc *jsonEncoder) closeOpenNamespaces() {
for i := 0; i < enc.openNamespaces; i++ {
enc.buf.AppendByte('}')
}
enc.openNamespaces = 0
}

func (enc *jsonEncoder) addKey(key string) {
Expand Down
44 changes: 44 additions & 0 deletions zapcore/json_encoder_impl_test.go
View file
Expand Up @@ -245,6 +245,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"`,
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 @@ -386,6 +402,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 @@ -522,6 +554,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) {
Expand Down
66 changes: 66 additions & 0 deletions zapcore/memory_encoder_test.go
View file
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
3 changes: 1 addition & 2 deletions zapgrpc/internal/test/go.sum
View file
Expand Up @@ -47,8 +47,7 @@ github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/
github.com/yuin/goldmark v1.3.5/go.mod h1:mwnBkeHKe2W/ZEtQ+71ViKU8L12m81fl3OWwC1Zlc8k=
go.uber.org/atomic v1.7.0 h1:ADUqmZGgLDDfbSL9ZmPxKTybcoEYHgpYfELNoN+7hsw=
go.uber.org/atomic v1.7.0/go.mod h1:fEN4uk6kAWBTFdckzkM89CLk9XfWZrxpCo0nPH17wJc=
go.uber.org/goleak v1.1.11-0.20210813005559-691160354723 h1:sHOAIxRGBp443oHZIPB+HsUGaksVCXVQENPxwTfQdH4=
go.uber.org/goleak v1.1.11-0.20210813005559-691160354723/go.mod h1:cwTWslyiVhfpKIDGSZEM2HlOvcqm+tG4zioyIeLoqMQ=
go.uber.org/goleak v1.1.11 h1:wy28qYRKZgnJTxGxvye5/wgWr1EKjmUDGYox5mGlRlI=
go.uber.org/goleak v1.1.11/go.mod h1:cwTWslyiVhfpKIDGSZEM2HlOvcqm+tG4zioyIeLoqMQ=
go.uber.org/multierr v1.6.0 h1:y6IPFStTAIT5Ytl7/XYmHvzXQ7S3g/IeZW9hyZ5thw4=
go.uber.org/multierr v1.6.0/go.mod h1:cdWPpRnG4AhwMwsgIHip0KRBQjJy5kYEpYjJxpXp9iU=
Expand Down

0 comments on commit 82a8da2

Please sign in to comment.