From 8b1bdce01160f910e54cd8a3923b16ee64851eca Mon Sep 17 00:00:00 2001 From: Dominic Barnes Date: Mon, 1 Mar 2021 09:27:55 -0800 Subject: [PATCH 1/4] test(json): add test case for #63 --- json/json_test.go | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/json/json_test.go b/json/json_test.go index 5f3f8a2..d5426de 100644 --- a/json/json_test.go +++ b/json/json_test.go @@ -1578,7 +1578,6 @@ func TestGithubIssue41(t *testing.T) { "expected: ", expectedString, ) } - } func TestGithubIssue44(t *testing.T) { @@ -1602,6 +1601,40 @@ func (r *rawJsonString) UnmarshalJSON(b []byte) error { return nil } +// See https://github.com/segmentio/encoding/issues/63 +// In short, embedding a struct pointer resulted in an incorrect memory address +// as we were still looking to the parent struct to start and only using offsets +// which resulted in the wrong values being extracted. +func TestGithubIssue63(t *testing.T) { + expectedString := `{"my_field":"test","my_other_field":"testing","code":1}` + + type MyStruct struct { + MyField string `json:"my_field,omitempty"` + MyOtherField string `json:"my_other_field"` + MyEmptyField string `json:"my_empty_field,omitempty"` + } + + type MyStruct2 struct { + *MyStruct + Code int `json:"code"` + } + + input := MyStruct2{ + MyStruct: &MyStruct{ + MyField: "test", + MyOtherField: "testing", + }, + Code: 1, + } + + if b, err := Marshal(input); err != nil { + t.Error(err) + } else if string(b) != expectedString { + t.Errorf("got: %s", string(b)) + t.Errorf("expected: %s", expectedString) + } +} + func TestSetTrustRawMessage(t *testing.T) { buf := &bytes.Buffer{} enc := NewEncoder(buf) From c434071fcbcf01841a2546ca373250bb82c69e31 Mon Sep 17 00:00:00 2001 From: Dominic Barnes Date: Thu, 29 Apr 2021 00:46:19 -0700 Subject: [PATCH 2/4] hack: get test case to pass, will clean up next --- json/codec.go | 5 ++++- json/encode.go | 13 +++++++++++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/json/codec.go b/json/codec.go index 22d35fd..67cfda9 100644 --- a/json/codec.go +++ b/json/codec.go @@ -677,7 +677,9 @@ func appendStructFields(fields []structField, t reflect.Type, offset uintptr, se if embfield.pointer { subfield.codec = constructEmbeddedStructPointerCodec(embfield.subtype.typ, embfield.unexported, subfield.offset, subfield.codec) + subfield.ptrOffset = subfield.offset subfield.offset = embfield.offset + subfield.ptr = true } else { subfield.offset += embfield.offset } @@ -931,7 +933,6 @@ type structType struct { fieldsIndex map[string]*structField ficaseIndex map[string]*structField typ reflect.Type - inlined bool } type structField struct { @@ -946,6 +947,8 @@ type structField struct { typ reflect.Type zero reflect.Value index int + ptr bool + ptrOffset uintptr } func unmarshalTypeError(b []byte, t reflect.Type) error { diff --git a/json/encode.go b/json/encode.go index 1d54297..f584a3e 100644 --- a/json/encode.go +++ b/json/encode.go @@ -767,8 +767,17 @@ func (e encoder) encodeStruct(b []byte, p unsafe.Pointer, st *structType) ([]byt f := &st.fields[i] v := unsafe.Pointer(uintptr(p) + f.offset) - if f.omitempty && f.empty(v) { - continue + if f.omitempty { + if f.ptr { + v2 := *(*unsafe.Pointer)(v) + v3 := unsafe.Pointer(uintptr(v2) + f.ptrOffset) + + if f.empty(v3) { + continue + } + } else if f.empty(v) { + continue + } } if escapeHTML { From 04754cced2944e93524e59b40db8bb3c5ffcf70a Mon Sep 17 00:00:00 2001 From: Dominic Barnes Date: Tue, 11 May 2021 22:56:05 -0700 Subject: [PATCH 3/4] test(json): add further test cases for issue 63 bug --- json/json_test.go | 107 +++++++++++++++++++++++++++++++++++++--------- 1 file changed, 87 insertions(+), 20 deletions(-) diff --git a/json/json_test.go b/json/json_test.go index d5426de..2f8d386 100644 --- a/json/json_test.go +++ b/json/json_test.go @@ -1606,32 +1606,99 @@ func (r *rawJsonString) UnmarshalJSON(b []byte) error { // as we were still looking to the parent struct to start and only using offsets // which resulted in the wrong values being extracted. func TestGithubIssue63(t *testing.T) { - expectedString := `{"my_field":"test","my_other_field":"testing","code":1}` + spec := []struct { + description string + input func() interface{} + expected string + }{ + { + description: "original", + expected: `{"my_field":"test","code":0}`, + input: func() interface{} { + type MyStruct struct { + MyField string `json:"my_field,omitempty"` + } - type MyStruct struct { - MyField string `json:"my_field,omitempty"` - MyOtherField string `json:"my_other_field"` - MyEmptyField string `json:"my_empty_field,omitempty"` - } + type MyStruct2 struct { + *MyStruct + Code int `json:"code"` + } - type MyStruct2 struct { - *MyStruct - Code int `json:"code"` - } + return MyStruct2{ + MyStruct: &MyStruct{ + MyField: "test", + }, + Code: 0, + } + }, + }, + { + description: "additional fields", + expected: `{"my_field":"test","my_other_field":"testing","code":1}`, + input: func() interface{} { + type MyStruct struct { + MyField string `json:"my_field,omitempty"` + MyOtherField string `json:"my_other_field"` + MyEmptyField string `json:"my_empty_field,omitempty"` + } + + type MyStruct2 struct { + *MyStruct + Code int `json:"code"` + } + + return MyStruct2{ + MyStruct: &MyStruct{ + MyField: "test", + MyOtherField: "testing", + }, + Code: 1, + } + }, + }, + { + description: "multiple embed levels", + expected: `{"my_field":"test","my_other_field":"testing","code":1}`, + input: func() interface{} { + type MyStruct struct { + MyField string `json:"my_field,omitempty"` + MyOtherField string `json:"my_other_field"` + MyEmptyField string `json:"my_empty_field,omitempty"` + } - input := MyStruct2{ - MyStruct: &MyStruct{ - MyField: "test", - MyOtherField: "testing", + type MyStruct2 struct { + *MyStruct + Code int `json:"code"` + } + + type MyStruct3 struct { + *MyStruct2 + A int `json:"a"` + } + + return MyStruct3{ + MyStruct2: &MyStruct2{ + MyStruct: &MyStruct{ + MyField: "test", + MyOtherField: "testing", + }, + Code: 1, + }, + A: 2, + } + }, }, - Code: 1, } - if b, err := Marshal(input); err != nil { - t.Error(err) - } else if string(b) != expectedString { - t.Errorf("got: %s", string(b)) - t.Errorf("expected: %s", expectedString) + for _, test := range spec { + t.Run(test.description, func(t *testing.T) { + if b, err := Marshal(test.input()); err != nil { + t.Error(err) + } else if string(b) != test.expected { + t.Errorf("got: %s", string(b)) + t.Errorf("expected: %s", test.expected) + } + }) } } From b802decf082b20bc3f07fec969b3d3ebc7181947 Mon Sep 17 00:00:00 2001 From: Dominic Barnes Date: Tue, 11 May 2021 23:43:22 -0700 Subject: [PATCH 4/4] fix(json): update code to fix test cases --- json/codec.go | 2 +- json/json_test.go | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/json/codec.go b/json/codec.go index 67cfda9..ca429de 100644 --- a/json/codec.go +++ b/json/codec.go @@ -677,7 +677,7 @@ func appendStructFields(fields []structField, t reflect.Type, offset uintptr, se if embfield.pointer { subfield.codec = constructEmbeddedStructPointerCodec(embfield.subtype.typ, embfield.unexported, subfield.offset, subfield.codec) - subfield.ptrOffset = subfield.offset + subfield.ptrOffset += subfield.offset subfield.offset = embfield.offset subfield.ptr = true } else { diff --git a/json/json_test.go b/json/json_test.go index 2f8d386..db1081e 100644 --- a/json/json_test.go +++ b/json/json_test.go @@ -1673,7 +1673,6 @@ func TestGithubIssue63(t *testing.T) { type MyStruct3 struct { *MyStruct2 - A int `json:"a"` } return MyStruct3{ @@ -1684,7 +1683,6 @@ func TestGithubIssue63(t *testing.T) { }, Code: 1, }, - A: 2, } }, },