From 95c3c43072a3d613429ff5eb80b5fcf212dd9998 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Daniel=20Mart=C3=AD?= Date: Thu, 11 Jul 2019 21:27:19 +0900 Subject: [PATCH] encoding/json: fix the broken "overwriting of data" tests MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Because TestUnmarshal actually allocates a new value to decode into using ptr's pointer type, any existing data is thrown away. This was harmless in alomst all of the test cases, minus the "overwriting of data" ones added in 2015 in CL 12209. I spotted that nothing covered decoding a JSON array with few elements into a slice which already had many elements. I initially assumed that the code was buggy or that some code could be removed, when in fact there simply wasn't any code covering the edge case. Move those two tests to TestPrefilled, which already served a very similar purpose. Remove the map case, as TestPrefilled already has plenty of prefilled map cases. Moreover, we no longer reset an entire map when decoding, as per the godoc: To unmarshal a JSON object into a map, Unmarshal first establishes a map to use. If the map is nil, Unmarshal allocates a new map. Otherwise Unmarshal reuses the existing map, keeping existing entries. Finally, to ensure that ptr is used correctly in the future, make TestUnmarshal error if it's anything other than a pointer to a zero value. That is, the only correct use should be new(type). Don't rename the ptr field, as that would be extremely noisy and cause unwanted merge conflicts. Change-Id: I41e3ecfeae42d877ac5443a6bd622ac3d6c8120c Reviewed-on: https://go-review.googlesource.com/c/go/+/185738 Run-TryBot: Daniel Martí TryBot-Result: Gobot Gobot Reviewed-by: Emmanuel Odeke --- src/encoding/json/decode_test.go | 109 ++++++++++++++++++------------- 1 file changed, 65 insertions(+), 44 deletions(-) diff --git a/src/encoding/json/decode_test.go b/src/encoding/json/decode_test.go index 3f25893b41..d66be44d4e 100644 --- a/src/encoding/json/decode_test.go +++ b/src/encoding/json/decode_test.go @@ -145,23 +145,15 @@ func (u8 *u8marshal) UnmarshalText(b []byte) error { var _ encoding.TextUnmarshaler = (*u8marshal)(nil) var ( - um0, um1 unmarshaler // target2 of unmarshaling - ump = &um1 umtrue = unmarshaler{true} umslice = []unmarshaler{{true}} - umslicep = new([]unmarshaler) umstruct = ustruct{unmarshaler{true}} - um0T, um1T unmarshalerText // target2 of unmarshaling - umpType = &um1T - umtrueXY = unmarshalerText{"x", "y"} - umsliceXY = []unmarshalerText{{"x", "y"}} - umslicepType = new([]unmarshalerText) - umstructType = new(ustructText) - umstructXY = ustructText{unmarshalerText{"x", "y"}} + umtrueXY = unmarshalerText{"x", "y"} + umsliceXY = []unmarshalerText{{"x", "y"}} + umstructXY = ustructText{unmarshalerText{"x", "y"}} - ummapType = map[unmarshalerText]bool{} - ummapXY = map[unmarshalerText]bool{{"x", "y"}: true} + ummapXY = map[unmarshalerText]bool{{"x", "y"}: true} ) // Test data structures for anonymous fields. @@ -279,9 +271,6 @@ type unexportedWithMethods struct{} func (unexportedWithMethods) F() {} -func sliceAddr(x []int) *[]int { return &x } -func mapAddr(x map[string]int) *map[string]int { return &x } - type byteWithMarshalJSON byte func (b byteWithMarshalJSON) MarshalJSON() ([]byte, error) { @@ -400,7 +389,7 @@ type mapStringToStringData struct { type unmarshalTest struct { in string - ptr interface{} + ptr interface{} // new(type) out interface{} err error useNumber bool @@ -493,18 +482,18 @@ var unmarshalTests = []unmarshalTest{ {in: pallValueCompact, ptr: new(*All), out: &pallValue}, // unmarshal interface test - {in: `{"T":false}`, ptr: &um0, out: umtrue}, // use "false" so test will fail if custom unmarshaler is not called - {in: `{"T":false}`, ptr: &ump, out: &umtrue}, - {in: `[{"T":false}]`, ptr: &umslice, out: umslice}, - {in: `[{"T":false}]`, ptr: &umslicep, out: &umslice}, - {in: `{"M":{"T":"x:y"}}`, ptr: &umstruct, out: umstruct}, + {in: `{"T":false}`, ptr: new(unmarshaler), out: umtrue}, // use "false" so test will fail if custom unmarshaler is not called + {in: `{"T":false}`, ptr: new(*unmarshaler), out: &umtrue}, + {in: `[{"T":false}]`, ptr: new([]unmarshaler), out: umslice}, + {in: `[{"T":false}]`, ptr: new(*[]unmarshaler), out: &umslice}, + {in: `{"M":{"T":"x:y"}}`, ptr: new(ustruct), out: umstruct}, // UnmarshalText interface test - {in: `"x:y"`, ptr: &um0T, out: umtrueXY}, - {in: `"x:y"`, ptr: &umpType, out: &umtrueXY}, - {in: `["x:y"]`, ptr: &umsliceXY, out: umsliceXY}, - {in: `["x:y"]`, ptr: &umslicepType, out: &umsliceXY}, - {in: `{"M":"x:y"}`, ptr: umstructType, out: umstructXY}, + {in: `"x:y"`, ptr: new(unmarshalerText), out: umtrueXY}, + {in: `"x:y"`, ptr: new(*unmarshalerText), out: &umtrueXY}, + {in: `["x:y"]`, ptr: new([]unmarshalerText), out: umsliceXY}, + {in: `["x:y"]`, ptr: new(*[]unmarshalerText), out: &umsliceXY}, + {in: `{"M":"x:y"}`, ptr: new(ustructText), out: umstructXY}, // integer-keyed map test { @@ -579,15 +568,9 @@ var unmarshalTests = []unmarshalTest{ }, // Map keys can be encoding.TextUnmarshalers. - {in: `{"x:y":true}`, ptr: &ummapType, out: ummapXY}, + {in: `{"x:y":true}`, ptr: new(map[unmarshalerText]bool), out: ummapXY}, // If multiple values for the same key exists, only the most recent value is used. - {in: `{"x:y":false,"x:y":true}`, ptr: &ummapType, out: ummapXY}, - - // Overwriting of data. - // This is different from package xml, but it's what we've always done. - // Now documented and tested. - {in: `[2]`, ptr: sliceAddr([]int{1}), out: []int{2}}, - {in: `{"key": 2}`, ptr: mapAddr(map[string]int{"old": 0, "key": 1}), out: map[string]int{"key": 2}}, + {in: `{"x:y":false,"x:y":true}`, ptr: new(map[unmarshalerText]bool), out: ummapXY}, { in: `{ @@ -713,19 +696,19 @@ var unmarshalTests = []unmarshalTest{ // Used to be issue 8305, but time.Time implements encoding.TextUnmarshaler so this works now. { in: `{"2009-11-10T23:00:00Z": "hello world"}`, - ptr: &map[time.Time]string{}, + ptr: new(map[time.Time]string), out: map[time.Time]string{time.Date(2009, 11, 10, 23, 0, 0, 0, time.UTC): "hello world"}, }, // issue 8305 { in: `{"2009-11-10T23:00:00Z": "hello world"}`, - ptr: &map[Point]string{}, + ptr: new(map[Point]string), err: &UnmarshalTypeError{Value: "object", Type: reflect.TypeOf(map[Point]string{}), Offset: 1}, }, { in: `{"asdf": "hello world"}`, - ptr: &map[unmarshaler]string{}, + ptr: new(map[unmarshaler]string), err: &UnmarshalTypeError{Value: "object", Type: reflect.TypeOf(map[unmarshaler]string{}), Offset: 1}, }, @@ -1077,8 +1060,27 @@ func TestUnmarshal(t *testing.T) { continue } + typ := reflect.TypeOf(tt.ptr) + if typ.Kind() != reflect.Ptr { + t.Errorf("#%d: unmarshalTest.ptr %T is not a pointer type", i, tt.ptr) + continue + } + typ = typ.Elem() + // v = new(right-type) - v := reflect.New(reflect.TypeOf(tt.ptr).Elem()) + v := reflect.New(typ) + + if !reflect.DeepEqual(tt.ptr, v.Interface()) { + // There's no reason for ptr to point to non-zero data, + // as we decode into new(right-type), so the data is + // discarded. + // This can easily mean tests that silently don't test + // what they should. To test decoding into existing + // data, see TestPrefilled. + t.Errorf("#%d: unmarshalTest.ptr %#v is not a pointer to a zero value", i, tt.ptr) + continue + } + dec := NewDecoder(bytes.NewReader(in)) if tt.useNumber { dec.UseNumber() @@ -2086,11 +2088,10 @@ func TestSkipArrayObjects(t *testing.T) { } } -// Test semantics of pre-filled struct fields and pre-filled map fields. -// Issue 4900. +// Test semantics of pre-filled data, such as struct fields, map elements, +// slices, and arrays. +// Issues 4900 and 8837, among others. func TestPrefilled(t *testing.T) { - ptrToMap := func(m map[string]interface{}) *map[string]interface{} { return &m } - // Values here change, cannot reuse table across runs. var prefillTests = []struct { in string @@ -2104,8 +2105,28 @@ func TestPrefilled(t *testing.T) { }, { in: `{"X": 1, "Y": 2}`, - ptr: ptrToMap(map[string]interface{}{"X": float32(3), "Y": int16(4), "Z": 1.5}), - out: ptrToMap(map[string]interface{}{"X": float64(1), "Y": float64(2), "Z": 1.5}), + ptr: &map[string]interface{}{"X": float32(3), "Y": int16(4), "Z": 1.5}, + out: &map[string]interface{}{"X": float64(1), "Y": float64(2), "Z": 1.5}, + }, + { + in: `[2]`, + ptr: &[]int{1}, + out: &[]int{2}, + }, + { + in: `[2, 3]`, + ptr: &[]int{1}, + out: &[]int{2, 3}, + }, + { + in: `[2, 3]`, + ptr: &[...]int{1}, + out: &[...]int{2}, + }, + { + in: `[3]`, + ptr: &[...]int{1, 2}, + out: &[...]int{3, 0}, }, } -- 2.50.0