Skip to content

Commit 7dd9c40

Browse files
zenovichzenovich
committed
encoding/json: introduce the GODEBUG setting "jsoninconsistentmarshal" allowing to revert the new consistent JSON marshalling, rework marshaling-related tests
1 parent 8241a96 commit 7dd9c40

File tree

5 files changed

+258
-51
lines changed

5 files changed

+258
-51
lines changed

‎doc/godebug.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,11 @@ see the [runtime documentation](/pkg/runtime#hdr-Environment_Variables)
151151
and the [go command documentation](/cmd/go#hdr-Build_and_test_caching).
152152

153153
### Go 1.24
154+
Go 1.24 made JSON marshaling consistent: custom marshalers ([`MarshalJSON`](/pkg/encoding/json#Marshaler) and [`MarshalText`](/pkg/encoding#TextMarshaler))
155+
are now always called when appropriate no matter if their receivers are pointers or values
156+
even if the related data fields are non-addressable.
157+
This behavior can be reverted with the [`jsoninconsistentmarshal` setting](/pkg/encoding/json/#Marshal).
158+
154159
Go 1.24 made XML marshaling consistent: custom marshalers ([`MarshalXML`](/pkg/encoding/xml#Marshaler),
155160
[`MarshalXMLAttr`](/pkg/encoding/xml#MarshalerAttr), [`MarshalText`](/pkg/encoding#TextMarshaler))
156161
are now always called when appropriate no matter if their receivers are pointers or values

‎src/encoding/json/encode.go

Lines changed: 37 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
"encoding"
1717
"encoding/base64"
1818
"fmt"
19+
"internal/godebug"
1920
"math"
2021
"reflect"
2122
"slices"
@@ -157,6 +158,13 @@ import (
157158
// JSON cannot represent cyclic data structures and Marshal does not
158159
// handle them. Passing cyclic structures to Marshal will result in
159160
// an error.
161+
//
162+
// Before Go 1.24, the marshaling was inconsistent: custom marshalers
163+
// (MarshalJSON and MarshalText methods) defined with pointer receivers
164+
// were not called for non-addressable values. As of Go 1.24, the marshaling is consistent.
165+
//
166+
// The GODEBUG setting jsoninconsistentmarshal=1 restores pre-Go 1.24
167+
// inconsistent marshaling.
160168
func Marshal(v any) ([]byte, error) {
161169
e := newEncodeState()
162170
defer encodeStatePool.Put(e)
@@ -363,7 +371,7 @@ func typeEncoder(t reflect.Type) encoderFunc {
363371
}
364372

365373
// Compute the real encoder and replace the indirect func with it.
366-
f = newTypeEncoder(t)
374+
f = newTypeEncoder(t, true)
367375
wg.Done()
368376
encoderCache.Store(t, f)
369377
return f
@@ -375,19 +383,19 @@ var (
375383
)
376384

377385
// newTypeEncoder constructs an encoderFunc for a type.
378-
func newTypeEncoder(t reflect.Type) encoderFunc {
379-
// If we have a non-pointer value whose type implements
386+
// The returned encoder only checks CanAddr when allowAddr is true.
387+
func newTypeEncoder(t reflect.Type, allowAddr bool) encoderFunc { // If we have a non-pointer value whose type implements
380388
// Marshaler with a value receiver, then we're better off taking
381389
// the address of the value - otherwise we end up with an
382390
// allocation as we cast the value to an interface.
383-
if t.Kind() != reflect.Pointer && reflect.PointerTo(t).Implements(marshalerType) {
384-
return addrMarshalerEncoder
391+
if t.Kind() != reflect.Pointer && allowAddr && reflect.PointerTo(t).Implements(marshalerType) {
392+
return newCondAddrEncoder(addrMarshalerEncoder, newTypeEncoder(t, false))
385393
}
386394
if t.Implements(marshalerType) {
387395
return marshalerEncoder
388396
}
389-
if t.Kind() != reflect.Pointer && reflect.PointerTo(t).Implements(textMarshalerType) {
390-
return addrTextMarshalerEncoder
397+
if t.Kind() != reflect.Pointer && allowAddr && reflect.PointerTo(t).Implements(textMarshalerType) {
398+
return newCondAddrEncoder(addrTextMarshalerEncoder, newTypeEncoder(t, false))
391399
}
392400
if t.Implements(textMarshalerType) {
393401
return textMarshalerEncoder
@@ -904,6 +912,28 @@ func newPtrEncoder(t reflect.Type) encoderFunc {
904912
return enc.encode
905913
}
906914

915+
type condAddrEncoder struct {
916+
canAddrEnc, elseEnc encoderFunc
917+
}
918+
919+
var jsoninconsistentmarshal = godebug.New("jsoninconsistentmarshal")
920+
921+
func (ce condAddrEncoder) encode(e *encodeState, v reflect.Value, opts encOpts) {
922+
if v.CanAddr() || jsoninconsistentmarshal.Value() != "1" {
923+
ce.canAddrEnc(e, v, opts)
924+
} else {
925+
jsoninconsistentmarshal.IncNonDefault()
926+
ce.elseEnc(e, v, opts)
927+
}
928+
}
929+
930+
// newCondAddrEncoder returns an encoder that checks whether its value
931+
// CanAddr and delegates to canAddrEnc if so, else to elseEnc.
932+
func newCondAddrEncoder(canAddrEnc, elseEnc encoderFunc) encoderFunc {
933+
enc := condAddrEncoder{canAddrEnc: canAddrEnc, elseEnc: elseEnc}
934+
return enc.encode
935+
}
936+
907937
func isValidTag(s string) bool {
908938
if s == "" {
909939
return false

‎src/encoding/json/encode_test.go

Lines changed: 210 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,11 @@ import (
1010
"fmt"
1111
"log"
1212
"math"
13+
"os"
1314
"reflect"
1415
"regexp"
1516
"runtime/debug"
17+
"runtime/metrics"
1618
"strconv"
1719
"testing"
1820
)
@@ -1228,36 +1230,8 @@ func (s *structWithMarshalJSON) MarshalJSON() ([]byte, error) {
12281230

12291231
var _ = Marshaler(&structWithMarshalJSON{})
12301232

1231-
type embedderJ struct {
1232-
V structWithMarshalJSON
1233-
}
1234-
1235-
func TestMarshalJSONWithPointerJSONMarshalers(t *testing.T) {
1236-
for _, test := range []struct {
1237-
name string
1238-
v interface{}
1239-
expected string
1240-
}{
1241-
{name: "a value with MarshalJSON", v: structWithMarshalJSON{v: 1}, expected: `"marshalled(1)"`},
1242-
{name: "pointer to a value with MarshalJSON", v: &structWithMarshalJSON{v: 1}, expected: `"marshalled(1)"`},
1243-
{name: "a map with a value with MarshalJSON", v: map[string]interface{}{"v": structWithMarshalJSON{v: 1}}, expected: `{"v":"marshalled(1)"}`},
1244-
{name: "a map with a pointer to a value with MarshalJSON", v: map[string]interface{}{"v": &structWithMarshalJSON{v: 1}}, expected: `{"v":"marshalled(1)"}`},
1245-
{name: "a slice of maps with a value with MarshalJSON", v: []map[string]interface{}{{"v": structWithMarshalJSON{v: 1}}}, expected: `[{"v":"marshalled(1)"}]`},
1246-
{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)"}]`},
1247-
{name: "a struct with a value with MarshalJSON", v: embedderJ{V: structWithMarshalJSON{v: 1}}, expected: `{"V":"marshalled(1)"}`},
1248-
{name: "a slice of structs with a value with MarshalJSON", v: []embedderJ{{V: structWithMarshalJSON{v: 1}}}, expected: `[{"V":"marshalled(1)"}]`},
1249-
} {
1250-
test := test
1251-
t.Run(test.name, func(t *testing.T) {
1252-
result, err := Marshal(test.v)
1253-
if err != nil {
1254-
t.Fatalf("Marshal error: %v", err)
1255-
}
1256-
if string(result) != test.expected {
1257-
t.Errorf("Marshal:\n\tgot: %s\n\twant: %s", result, test.expected)
1258-
}
1259-
})
1260-
}
1233+
type embedder struct {
1234+
V interface{}
12611235
}
12621236

12631237
type structWithMarshalText struct{ v int }
@@ -1268,31 +1242,223 @@ func (s *structWithMarshalText) MarshalText() ([]byte, error) {
12681242

12691243
var _ = encoding.TextMarshaler(&structWithMarshalText{})
12701244

1271-
type embedderT struct {
1272-
V structWithMarshalText
1273-
}
1274-
1275-
func TestMarshalJSONWithPointerTextMarshalers(t *testing.T) {
1245+
func TestMarshalJSONWithPointerMarshalers(t *testing.T) {
12761246
for _, test := range []struct {
1277-
name string
1278-
v interface{}
1279-
expected string
1247+
name string
1248+
jsoninconsistentmarshal bool
1249+
v interface{}
1250+
expected string
1251+
expectedOldBehaviorCount uint64
1252+
expectedError string
12801253
}{
1254+
// MarshalJSON
1255+
{name: "a value with MarshalJSON", v: structWithMarshalJSON{v: 1}, expected: `"marshalled(1)"`},
1256+
{name: "pointer to a value with MarshalJSON", v: &structWithMarshalJSON{v: 1}, expected: `"marshalled(1)"`},
1257+
{
1258+
name: "a map with a value with MarshalJSON",
1259+
v: map[string]interface{}{"v": structWithMarshalJSON{v: 1}},
1260+
expected: `{"v":"marshalled(1)"}`,
1261+
},
1262+
{
1263+
name: "a map with a pointer to a value with MarshalJSON",
1264+
v: map[string]interface{}{"v": &structWithMarshalJSON{v: 1}},
1265+
expected: `{"v":"marshalled(1)"}`,
1266+
},
1267+
{
1268+
name: "a slice of maps with a value with MarshalJSON",
1269+
v: []map[string]interface{}{{"v": structWithMarshalJSON{v: 1}}},
1270+
expected: `[{"v":"marshalled(1)"}]`,
1271+
},
1272+
{
1273+
name: "a slice of maps with a pointer to a value with MarshalJSON",
1274+
v: []map[string]interface{}{{"v": &structWithMarshalJSON{v: 1}}},
1275+
expected: `[{"v":"marshalled(1)"}]`,
1276+
},
1277+
{
1278+
name: "a struct with a value with MarshalJSON",
1279+
v: embedder{V: structWithMarshalJSON{v: 1}},
1280+
expected: `{"V":"marshalled(1)"}`,
1281+
},
1282+
{
1283+
name: "a slice of structs with a value with MarshalJSON",
1284+
v: []embedder{{V: structWithMarshalJSON{v: 1}}},
1285+
expected: `[{"V":"marshalled(1)"}]`,
1286+
},
1287+
{
1288+
name: "a value with MarshalJSON (only addressable)",
1289+
jsoninconsistentmarshal: true,
1290+
v: structWithMarshalJSON{v: 1},
1291+
expected: `{}`,
1292+
expectedOldBehaviorCount: 1,
1293+
},
1294+
{
1295+
name: "pointer to a value with MarshalJSON (only addressable)",
1296+
jsoninconsistentmarshal: true,
1297+
v: &structWithMarshalJSON{v: 1},
1298+
expected: `"marshalled(1)"`,
1299+
},
1300+
{
1301+
name: "a map with a value with MarshalJSON (only addressable)",
1302+
jsoninconsistentmarshal: true,
1303+
v: map[string]interface{}{"v": structWithMarshalJSON{v: 1}},
1304+
expected: `{"v":{}}`,
1305+
expectedOldBehaviorCount: 1,
1306+
},
1307+
{
1308+
name: "a map with a pointer to a value with MarshalJSON (only addressable)",
1309+
jsoninconsistentmarshal: true,
1310+
v: map[string]interface{}{"v": &structWithMarshalJSON{v: 1}},
1311+
expected: `{"v":"marshalled(1)"}`,
1312+
},
1313+
{
1314+
name: "a slice of maps with a value with MarshalJSON (only addressable)",
1315+
jsoninconsistentmarshal: true,
1316+
v: []map[string]interface{}{{"v": structWithMarshalJSON{v: 1}}},
1317+
expected: `[{"v":{}}]`,
1318+
expectedOldBehaviorCount: 1,
1319+
},
1320+
{
1321+
name: "a slice of maps with a pointer to a value with MarshalJSON (only addressable)",
1322+
jsoninconsistentmarshal: true,
1323+
v: []map[string]interface{}{{"v": &structWithMarshalJSON{v: 1}}},
1324+
expected: `[{"v":"marshalled(1)"}]`,
1325+
},
1326+
{
1327+
name: "a struct with a value with MarshalJSON (only addressable)",
1328+
jsoninconsistentmarshal: true,
1329+
v: embedder{V: structWithMarshalJSON{v: 1}},
1330+
expected: `{"V":{}}`,
1331+
expectedOldBehaviorCount: 1,
1332+
},
1333+
{
1334+
name: "a slice of structs with a value with MarshalJSON (only addressable)",
1335+
jsoninconsistentmarshal: true,
1336+
v: []embedder{{V: structWithMarshalJSON{v: 1}}},
1337+
expected: `[{"V":{}}]`,
1338+
expectedOldBehaviorCount: 1,
1339+
},
1340+
{
1341+
name: "a slice of structs with a value with MarshalJSON with two elements (only addressable)",
1342+
jsoninconsistentmarshal: true,
1343+
v: []embedder{{V: structWithMarshalJSON{v: 1}}, {V: structWithMarshalJSON{v: 2}}},
1344+
expected: `[{"V":{}},{"V":{}}]`,
1345+
expectedOldBehaviorCount: 2,
1346+
},
1347+
// MarshalText
12811348
{name: "a value with MarshalText", v: structWithMarshalText{v: 1}, expected: `"marshalled(1)"`},
12821349
{name: "pointer to a value with MarshalText", v: &structWithMarshalText{v: 1}, expected: `"marshalled(1)"`},
12831350
{name: "a map with a value with MarshalText", v: map[string]interface{}{"v": structWithMarshalText{v: 1}}, expected: `{"v":"marshalled(1)"}`},
1284-
{name: "a map with a pointer to a value with MarshalText", v: map[string]interface{}{"v": &structWithMarshalText{v: 1}}, expected: `{"v":"marshalled(1)"}`},
1285-
{name: "a slice of maps with a value with MarshalText", v: []map[string]interface{}{{"v": structWithMarshalText{v: 1}}}, expected: `[{"v":"marshalled(1)"}]`},
1286-
{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)"}]`},
1287-
{name: "a struct with a value with MarshalText", v: embedderT{V: structWithMarshalText{v: 1}}, expected: `{"V":"marshalled(1)"}`},
1288-
{name: "a slice of structs with a value with MarshalText", v: []embedderT{{V: structWithMarshalText{v: 1}}}, expected: `[{"V":"marshalled(1)"}]`},
1351+
{
1352+
name: "a map with a pointer to a value with MarshalText",
1353+
v: map[string]interface{}{"v": &structWithMarshalText{v: 1}},
1354+
expected: `{"v":"marshalled(1)"}`,
1355+
},
1356+
{
1357+
name: "a slice of maps with a value with MarshalText",
1358+
v: []map[string]interface{}{{"v": structWithMarshalText{v: 1}}},
1359+
expected: `[{"v":"marshalled(1)"}]`,
1360+
},
1361+
{
1362+
name: "a slice of maps with a pointer to a value with MarshalText",
1363+
v: []map[string]interface{}{{"v": &structWithMarshalText{v: 1}}},
1364+
expected: `[{"v":"marshalled(1)"}]`,
1365+
},
1366+
{
1367+
name: "a struct with a value with MarshalText",
1368+
v: embedder{V: structWithMarshalText{v: 1}},
1369+
expected: `{"V":"marshalled(1)"}`,
1370+
},
1371+
{
1372+
name: "a slice of structs with a value with MarshalText",
1373+
v: []embedder{{V: structWithMarshalText{v: 1}}},
1374+
expected: `[{"V":"marshalled(1)"}]`,
1375+
},
1376+
{
1377+
name: "a value with MarshalText (only addressable)",
1378+
jsoninconsistentmarshal: true,
1379+
v: structWithMarshalText{v: 1},
1380+
expected: `{}`,
1381+
expectedOldBehaviorCount: 1,
1382+
},
1383+
{
1384+
name: "pointer to a value with MarshalText (only addressable)",
1385+
jsoninconsistentmarshal: true,
1386+
v: &structWithMarshalText{v: 1},
1387+
expected: `"marshalled(1)"`,
1388+
},
1389+
{
1390+
name: "a map with a value with MarshalText (only addressable)",
1391+
jsoninconsistentmarshal: true,
1392+
v: map[string]interface{}{"v": structWithMarshalText{v: 1}},
1393+
expected: `{"v":{}}`,
1394+
expectedOldBehaviorCount: 1,
1395+
},
1396+
{
1397+
name: "a map with a pointer to a value with MarshalText (only addressable)",
1398+
jsoninconsistentmarshal: true,
1399+
v: map[string]interface{}{"v": &structWithMarshalText{v: 1}},
1400+
expected: `{"v":"marshalled(1)"}`,
1401+
},
1402+
{
1403+
name: "a slice of maps with a value with MarshalText (only addressable)",
1404+
jsoninconsistentmarshal: true,
1405+
v: []map[string]interface{}{{"v": structWithMarshalText{v: 1}}},
1406+
expected: `[{"v":{}}]`,
1407+
expectedOldBehaviorCount: 1,
1408+
},
1409+
{
1410+
name: "a slice of maps with a pointer to a value with MarshalText (only addressable)",
1411+
jsoninconsistentmarshal: true,
1412+
v: []map[string]interface{}{{"v": &structWithMarshalText{v: 1}}},
1413+
expected: `[{"v":"marshalled(1)"}]`,
1414+
},
1415+
{
1416+
name: "a struct with a value with MarshalText (only addressable)",
1417+
jsoninconsistentmarshal: true,
1418+
v: embedder{V: structWithMarshalText{v: 1}},
1419+
expected: `{"V":{}}`,
1420+
expectedOldBehaviorCount: 1,
1421+
},
1422+
{
1423+
name: "a slice of structs with a value with MarshalText (only addressable)",
1424+
jsoninconsistentmarshal: true,
1425+
v: []embedder{{V: structWithMarshalText{v: 1}}},
1426+
expected: `[{"V":{}}]`,
1427+
expectedOldBehaviorCount: 1,
1428+
},
12891429
} {
12901430
test := test
12911431
t.Run(test.name, func(t *testing.T) {
1432+
const metricName = "/godebug/non-default-behavior/jsoninconsistentmarshal:events"
1433+
sample := make([]metrics.Sample, 1)
1434+
sample[0].Name = metricName
1435+
metrics.Read(sample)
1436+
metricOldValue := sample[0].Value.Uint64()
1437+
1438+
if test.jsoninconsistentmarshal {
1439+
os.Setenv("GODEBUG", "jsoninconsistentmarshal=1")
1440+
defer os.Unsetenv("GODEBUG")
1441+
}
12921442
result, err := Marshal(test.v)
1443+
1444+
metrics.Read(sample)
1445+
metricNewValue := sample[0].Value.Uint64()
1446+
oldBehaviorCount := metricNewValue - metricOldValue
1447+
1448+
if oldBehaviorCount != test.expectedOldBehaviorCount {
1449+
t.Errorf("The old behavior count is %d, want %d", oldBehaviorCount, test.expectedOldBehaviorCount)
1450+
}
1451+
12931452
if err != nil {
1294-
t.Fatalf("Marshal error: %v", err)
1453+
if test.expectedError != "" {
1454+
if err.Error() != test.expectedError {
1455+
t.Errorf("Unexpected Marshal error: %s, expected: %s", err.Error(), test.expectedError)
1456+
}
1457+
return
1458+
}
1459+
t.Fatalf("Unexpected Marshal error: %v", err)
12951460
}
1461+
12961462
if string(result) != test.expected {
12971463
t.Errorf("Marshal:\n\tgot: %s\n\twant: %s", result, test.expected)
12981464
}

‎src/internal/godebugs/table.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ var All = []Info{
3838
{Name: "httpmuxgo121", Package: "net/http", Changed: 22, Old: "1"},
3939
{Name: "httpservecontentkeepheaders", Package: "net/http", Changed: 23, Old: "1"},
4040
{Name: "installgoroot", Package: "go/build"},
41+
{Name: "jsoninconsistentmarshal", Package: "encoding/json"},
4142
{Name: "jstmpllitinterp", Package: "html/template", Opaque: true}, // bug #66217: remove Opaque
4243
//{Name: "multipartfiles", Package: "mime/multipart"},
4344
{Name: "multipartmaxheaders", Package: "mime/multipart"},

0 commit comments

Comments
 (0)