From 7dd9c40f285141acdf8829fc13e911cbc8c7222d Mon Sep 17 00:00:00 2001 From: Dmitry Zenovich Date: Wed, 21 Aug 2024 02:45:43 +0300 Subject: [PATCH] encoding/json: introduce the GODEBUG setting "jsoninconsistentmarshal" allowing to revert the new consistent JSON marshalling, rework marshaling-related tests --- doc/godebug.md | 5 + src/encoding/json/encode.go | 44 +++++- src/encoding/json/encode_test.go | 258 +++++++++++++++++++++++++------ src/internal/godebugs/table.go | 1 + src/runtime/metrics/doc.go | 5 + 5 files changed, 260 insertions(+), 53 deletions(-) diff --git a/doc/godebug.md b/doc/godebug.md index fdc26f7b9e3..03618ed9f41 100644 --- a/doc/godebug.md +++ b/doc/godebug.md @@ -151,6 +151,11 @@ see the [runtime documentation](/pkg/runtime#hdr-Environment_Variables) and the [go command documentation](/cmd/go#hdr-Build_and_test_caching). ### Go 1.24 +Go 1.24 made JSON marshaling consistent: custom marshalers ([`MarshalJSON`](/pkg/encoding/json#Marshaler) and [`MarshalText`](/pkg/encoding#TextMarshaler)) +are now always called when appropriate no matter if their receivers are pointers or values +even if the related data fields are non-addressable. +This behavior can be reverted with the [`jsoninconsistentmarshal` setting](/pkg/encoding/json/#Marshal). + Go 1.24 made XML marshaling consistent: custom marshalers ([`MarshalXML`](/pkg/encoding/xml#Marshaler), [`MarshalXMLAttr`](/pkg/encoding/xml#MarshalerAttr), [`MarshalText`](/pkg/encoding#TextMarshaler)) are now always called when appropriate no matter if their receivers are pointers or values diff --git a/src/encoding/json/encode.go b/src/encoding/json/encode.go index 6b8b9134fa9..09ecc9093ea 100644 --- a/src/encoding/json/encode.go +++ b/src/encoding/json/encode.go @@ -16,6 +16,7 @@ import ( "encoding" "encoding/base64" "fmt" + "internal/godebug" "math" "reflect" "slices" @@ -157,6 +158,13 @@ import ( // JSON cannot represent cyclic data structures and Marshal does not // handle them. Passing cyclic structures to Marshal will result in // an error. +// +// Before Go 1.24, the marshaling was inconsistent: custom marshalers +// (MarshalJSON and MarshalText methods) defined with pointer receivers +// were not called for non-addressable values. As of Go 1.24, the marshaling is consistent. +// +// The GODEBUG setting jsoninconsistentmarshal=1 restores pre-Go 1.24 +// inconsistent marshaling. func Marshal(v any) ([]byte, error) { e := newEncodeState() defer encodeStatePool.Put(e) @@ -363,7 +371,7 @@ func typeEncoder(t reflect.Type) encoderFunc { } // Compute the real encoder and replace the indirect func with it. - f = newTypeEncoder(t) + f = newTypeEncoder(t, true) wg.Done() encoderCache.Store(t, f) return f @@ -375,19 +383,19 @@ var ( ) // newTypeEncoder constructs an encoderFunc for a type. -func newTypeEncoder(t reflect.Type) encoderFunc { - // If we have a non-pointer value whose type implements +// The returned encoder only checks CanAddr when allowAddr is true. +func newTypeEncoder(t reflect.Type, allowAddr bool) encoderFunc { // If we have a non-pointer value whose type implements // Marshaler with a value receiver, then we're better off taking // the address of the value - otherwise we end up with an // allocation as we cast the value to an interface. - if t.Kind() != reflect.Pointer && reflect.PointerTo(t).Implements(marshalerType) { - return addrMarshalerEncoder + if t.Kind() != reflect.Pointer && allowAddr && reflect.PointerTo(t).Implements(marshalerType) { + return newCondAddrEncoder(addrMarshalerEncoder, newTypeEncoder(t, false)) } if t.Implements(marshalerType) { return marshalerEncoder } - if t.Kind() != reflect.Pointer && reflect.PointerTo(t).Implements(textMarshalerType) { - return addrTextMarshalerEncoder + if t.Kind() != reflect.Pointer && allowAddr && reflect.PointerTo(t).Implements(textMarshalerType) { + return newCondAddrEncoder(addrTextMarshalerEncoder, newTypeEncoder(t, false)) } if t.Implements(textMarshalerType) { return textMarshalerEncoder @@ -904,6 +912,28 @@ func newPtrEncoder(t reflect.Type) encoderFunc { return enc.encode } +type condAddrEncoder struct { + canAddrEnc, elseEnc encoderFunc +} + +var jsoninconsistentmarshal = godebug.New("jsoninconsistentmarshal") + +func (ce condAddrEncoder) encode(e *encodeState, v reflect.Value, opts encOpts) { + if v.CanAddr() || jsoninconsistentmarshal.Value() != "1" { + ce.canAddrEnc(e, v, opts) + } else { + jsoninconsistentmarshal.IncNonDefault() + ce.elseEnc(e, v, opts) + } +} + +// newCondAddrEncoder returns an encoder that checks whether its value +// CanAddr and delegates to canAddrEnc if so, else to elseEnc. +func newCondAddrEncoder(canAddrEnc, elseEnc encoderFunc) encoderFunc { + enc := condAddrEncoder{canAddrEnc: canAddrEnc, elseEnc: elseEnc} + return enc.encode +} + func isValidTag(s string) bool { if s == "" { return false diff --git a/src/encoding/json/encode_test.go b/src/encoding/json/encode_test.go index ac12880109e..325866ff97a 100644 --- a/src/encoding/json/encode_test.go +++ b/src/encoding/json/encode_test.go @@ -10,9 +10,11 @@ import ( "fmt" "log" "math" + "os" "reflect" "regexp" "runtime/debug" + "runtime/metrics" "strconv" "testing" ) @@ -1228,36 +1230,8 @@ func (s *structWithMarshalJSON) MarshalJSON() ([]byte, error) { var _ = Marshaler(&structWithMarshalJSON{}) -type embedderJ struct { - V structWithMarshalJSON -} - -func TestMarshalJSONWithPointerJSONMarshalers(t *testing.T) { - for _, test := range []struct { - name string - v interface{} - expected string - }{ - {name: "a value with MarshalJSON", v: structWithMarshalJSON{v: 1}, expected: `"marshalled(1)"`}, - {name: "pointer to a value with MarshalJSON", v: &structWithMarshalJSON{v: 1}, expected: `"marshalled(1)"`}, - {name: "a map with a value with MarshalJSON", v: map[string]interface{}{"v": structWithMarshalJSON{v: 1}}, expected: `{"v":"marshalled(1)"}`}, - {name: "a map with a pointer to a value with MarshalJSON", v: map[string]interface{}{"v": &structWithMarshalJSON{v: 1}}, expected: `{"v":"marshalled(1)"}`}, - {name: "a slice of maps with a value with MarshalJSON", v: []map[string]interface{}{{"v": structWithMarshalJSON{v: 1}}}, expected: `[{"v":"marshalled(1)"}]`}, - {name: "a slice of maps with a pointer to a value with MarshalJSON", v: []map[string]interface{}{{"v": &structWithMarshalJSON{v: 1}}}, expected: `[{"v":"marshalled(1)"}]`}, - {name: "a struct with a value with MarshalJSON", v: embedderJ{V: structWithMarshalJSON{v: 1}}, expected: `{"V":"marshalled(1)"}`}, - {name: "a slice of structs with a value with MarshalJSON", v: []embedderJ{{V: structWithMarshalJSON{v: 1}}}, expected: `[{"V":"marshalled(1)"}]`}, - } { - test := test - t.Run(test.name, func(t *testing.T) { - result, err := Marshal(test.v) - if err != nil { - t.Fatalf("Marshal error: %v", err) - } - if string(result) != test.expected { - t.Errorf("Marshal:\n\tgot: %s\n\twant: %s", result, test.expected) - } - }) - } +type embedder struct { + V interface{} } type structWithMarshalText struct{ v int } @@ -1268,31 +1242,223 @@ func (s *structWithMarshalText) MarshalText() ([]byte, error) { var _ = encoding.TextMarshaler(&structWithMarshalText{}) -type embedderT struct { - V structWithMarshalText -} - -func TestMarshalJSONWithPointerTextMarshalers(t *testing.T) { +func TestMarshalJSONWithPointerMarshalers(t *testing.T) { for _, test := range []struct { - name string - v interface{} - expected string + name string + jsoninconsistentmarshal bool + v interface{} + expected string + expectedOldBehaviorCount uint64 + expectedError string }{ + // MarshalJSON + {name: "a value with MarshalJSON", v: structWithMarshalJSON{v: 1}, expected: `"marshalled(1)"`}, + {name: "pointer to a value with MarshalJSON", v: &structWithMarshalJSON{v: 1}, expected: `"marshalled(1)"`}, + { + name: "a map with a value with MarshalJSON", + v: map[string]interface{}{"v": structWithMarshalJSON{v: 1}}, + expected: `{"v":"marshalled(1)"}`, + }, + { + name: "a map with a pointer to a value with MarshalJSON", + v: map[string]interface{}{"v": &structWithMarshalJSON{v: 1}}, + expected: `{"v":"marshalled(1)"}`, + }, + { + name: "a slice of maps with a value with MarshalJSON", + v: []map[string]interface{}{{"v": structWithMarshalJSON{v: 1}}}, + expected: `[{"v":"marshalled(1)"}]`, + }, + { + name: "a slice of maps with a pointer to a value with MarshalJSON", + v: []map[string]interface{}{{"v": &structWithMarshalJSON{v: 1}}}, + expected: `[{"v":"marshalled(1)"}]`, + }, + { + name: "a struct with a value with MarshalJSON", + v: embedder{V: structWithMarshalJSON{v: 1}}, + expected: `{"V":"marshalled(1)"}`, + }, + { + name: "a slice of structs with a value with MarshalJSON", + v: []embedder{{V: structWithMarshalJSON{v: 1}}}, + expected: `[{"V":"marshalled(1)"}]`, + }, + { + name: "a value with MarshalJSON (only addressable)", + jsoninconsistentmarshal: true, + v: structWithMarshalJSON{v: 1}, + expected: `{}`, + expectedOldBehaviorCount: 1, + }, + { + name: "pointer to a value with MarshalJSON (only addressable)", + jsoninconsistentmarshal: true, + v: &structWithMarshalJSON{v: 1}, + expected: `"marshalled(1)"`, + }, + { + name: "a map with a value with MarshalJSON (only addressable)", + jsoninconsistentmarshal: true, + v: map[string]interface{}{"v": structWithMarshalJSON{v: 1}}, + expected: `{"v":{}}`, + expectedOldBehaviorCount: 1, + }, + { + name: "a map with a pointer to a value with MarshalJSON (only addressable)", + jsoninconsistentmarshal: true, + v: map[string]interface{}{"v": &structWithMarshalJSON{v: 1}}, + expected: `{"v":"marshalled(1)"}`, + }, + { + name: "a slice of maps with a value with MarshalJSON (only addressable)", + jsoninconsistentmarshal: true, + v: []map[string]interface{}{{"v": structWithMarshalJSON{v: 1}}}, + expected: `[{"v":{}}]`, + expectedOldBehaviorCount: 1, + }, + { + name: "a slice of maps with a pointer to a value with MarshalJSON (only addressable)", + jsoninconsistentmarshal: true, + v: []map[string]interface{}{{"v": &structWithMarshalJSON{v: 1}}}, + expected: `[{"v":"marshalled(1)"}]`, + }, + { + name: "a struct with a value with MarshalJSON (only addressable)", + jsoninconsistentmarshal: true, + v: embedder{V: structWithMarshalJSON{v: 1}}, + expected: `{"V":{}}`, + expectedOldBehaviorCount: 1, + }, + { + name: "a slice of structs with a value with MarshalJSON (only addressable)", + jsoninconsistentmarshal: true, + v: []embedder{{V: structWithMarshalJSON{v: 1}}}, + expected: `[{"V":{}}]`, + expectedOldBehaviorCount: 1, + }, + { + name: "a slice of structs with a value with MarshalJSON with two elements (only addressable)", + jsoninconsistentmarshal: true, + v: []embedder{{V: structWithMarshalJSON{v: 1}}, {V: structWithMarshalJSON{v: 2}}}, + expected: `[{"V":{}},{"V":{}}]`, + expectedOldBehaviorCount: 2, + }, + // MarshalText {name: "a value with MarshalText", v: structWithMarshalText{v: 1}, expected: `"marshalled(1)"`}, {name: "pointer to a value with MarshalText", v: &structWithMarshalText{v: 1}, expected: `"marshalled(1)"`}, {name: "a map with a value with MarshalText", v: map[string]interface{}{"v": structWithMarshalText{v: 1}}, expected: `{"v":"marshalled(1)"}`}, - {name: "a map with a pointer to a value with MarshalText", v: map[string]interface{}{"v": &structWithMarshalText{v: 1}}, expected: `{"v":"marshalled(1)"}`}, - {name: "a slice of maps with a value with MarshalText", v: []map[string]interface{}{{"v": structWithMarshalText{v: 1}}}, expected: `[{"v":"marshalled(1)"}]`}, - {name: "a slice of maps with a pointer to a value with MarshalText", v: []map[string]interface{}{{"v": &structWithMarshalText{v: 1}}}, expected: `[{"v":"marshalled(1)"}]`}, - {name: "a struct with a value with MarshalText", v: embedderT{V: structWithMarshalText{v: 1}}, expected: `{"V":"marshalled(1)"}`}, - {name: "a slice of structs with a value with MarshalText", v: []embedderT{{V: structWithMarshalText{v: 1}}}, expected: `[{"V":"marshalled(1)"}]`}, + { + name: "a map with a pointer to a value with MarshalText", + v: map[string]interface{}{"v": &structWithMarshalText{v: 1}}, + expected: `{"v":"marshalled(1)"}`, + }, + { + name: "a slice of maps with a value with MarshalText", + v: []map[string]interface{}{{"v": structWithMarshalText{v: 1}}}, + expected: `[{"v":"marshalled(1)"}]`, + }, + { + name: "a slice of maps with a pointer to a value with MarshalText", + v: []map[string]interface{}{{"v": &structWithMarshalText{v: 1}}}, + expected: `[{"v":"marshalled(1)"}]`, + }, + { + name: "a struct with a value with MarshalText", + v: embedder{V: structWithMarshalText{v: 1}}, + expected: `{"V":"marshalled(1)"}`, + }, + { + name: "a slice of structs with a value with MarshalText", + v: []embedder{{V: structWithMarshalText{v: 1}}}, + expected: `[{"V":"marshalled(1)"}]`, + }, + { + name: "a value with MarshalText (only addressable)", + jsoninconsistentmarshal: true, + v: structWithMarshalText{v: 1}, + expected: `{}`, + expectedOldBehaviorCount: 1, + }, + { + name: "pointer to a value with MarshalText (only addressable)", + jsoninconsistentmarshal: true, + v: &structWithMarshalText{v: 1}, + expected: `"marshalled(1)"`, + }, + { + name: "a map with a value with MarshalText (only addressable)", + jsoninconsistentmarshal: true, + v: map[string]interface{}{"v": structWithMarshalText{v: 1}}, + expected: `{"v":{}}`, + expectedOldBehaviorCount: 1, + }, + { + name: "a map with a pointer to a value with MarshalText (only addressable)", + jsoninconsistentmarshal: true, + v: map[string]interface{}{"v": &structWithMarshalText{v: 1}}, + expected: `{"v":"marshalled(1)"}`, + }, + { + name: "a slice of maps with a value with MarshalText (only addressable)", + jsoninconsistentmarshal: true, + v: []map[string]interface{}{{"v": structWithMarshalText{v: 1}}}, + expected: `[{"v":{}}]`, + expectedOldBehaviorCount: 1, + }, + { + name: "a slice of maps with a pointer to a value with MarshalText (only addressable)", + jsoninconsistentmarshal: true, + v: []map[string]interface{}{{"v": &structWithMarshalText{v: 1}}}, + expected: `[{"v":"marshalled(1)"}]`, + }, + { + name: "a struct with a value with MarshalText (only addressable)", + jsoninconsistentmarshal: true, + v: embedder{V: structWithMarshalText{v: 1}}, + expected: `{"V":{}}`, + expectedOldBehaviorCount: 1, + }, + { + name: "a slice of structs with a value with MarshalText (only addressable)", + jsoninconsistentmarshal: true, + v: []embedder{{V: structWithMarshalText{v: 1}}}, + expected: `[{"V":{}}]`, + expectedOldBehaviorCount: 1, + }, } { test := test t.Run(test.name, func(t *testing.T) { - result, err := Marshal(test.v) - if err != nil { - t.Fatalf("Marshal error: %v", err) + const metricName = "/godebug/non-default-behavior/jsoninconsistentmarshal:events" + sample := make([]metrics.Sample, 1) + sample[0].Name = metricName + metrics.Read(sample) + metricOldValue := sample[0].Value.Uint64() + + if test.jsoninconsistentmarshal { + os.Setenv("GODEBUG", "jsoninconsistentmarshal=1") + defer os.Unsetenv("GODEBUG") } + result, err := Marshal(test.v) + + metrics.Read(sample) + metricNewValue := sample[0].Value.Uint64() + oldBehaviorCount := metricNewValue - metricOldValue + + if oldBehaviorCount != test.expectedOldBehaviorCount { + t.Errorf("The old behavior count is %d, want %d", oldBehaviorCount, test.expectedOldBehaviorCount) + } + + if err != nil { + if test.expectedError != "" { + if err.Error() != test.expectedError { + t.Errorf("Unexpected Marshal error: %s, expected: %s", err.Error(), test.expectedError) + } + return + } + t.Fatalf("Unexpected Marshal error: %v", err) + } + if string(result) != test.expected { t.Errorf("Marshal:\n\tgot: %s\n\twant: %s", result, test.expected) } diff --git a/src/internal/godebugs/table.go b/src/internal/godebugs/table.go index d4121a807de..f002115999f 100644 --- a/src/internal/godebugs/table.go +++ b/src/internal/godebugs/table.go @@ -38,6 +38,7 @@ var All = []Info{ {Name: "httpmuxgo121", Package: "net/http", Changed: 22, Old: "1"}, {Name: "httpservecontentkeepheaders", Package: "net/http", Changed: 23, Old: "1"}, {Name: "installgoroot", Package: "go/build"}, + {Name: "jsoninconsistentmarshal", Package: "encoding/json"}, {Name: "jstmpllitinterp", Package: "html/template", Opaque: true}, // bug #66217: remove Opaque //{Name: "multipartfiles", Package: "mime/multipart"}, {Name: "multipartmaxheaders", Package: "mime/multipart"}, diff --git a/src/runtime/metrics/doc.go b/src/runtime/metrics/doc.go index e10020aebe4..077ea701c87 100644 --- a/src/runtime/metrics/doc.go +++ b/src/runtime/metrics/doc.go @@ -280,6 +280,11 @@ Below is the full list of supported metrics, ordered lexicographically. The number of non-default behaviors executed by the go/build package due to a non-default GODEBUG=installgoroot=... setting. + /godebug/non-default-behavior/jsoninconsistentmarshal:events + The number of non-default behaviors executed by + the encoding/json package due to a non-default + GODEBUG=jsoninconsistentmarshal=... setting. + /godebug/non-default-behavior/multipartmaxheaders:events The number of non-default behaviors executed by the mime/multipart package due to a non-default