Stowage/go
2
0
Fork 0
mirror of https://github.com/golang/go.git synced 2026-06-28 03:40:37 +00:00
Code Issues Projects Releases Packages Wiki Activity

encoding/json/v2: remove recursion and error on string on unsupported type

Browse source 
Prior to this CL, `string` is the only struct tag that applies
recursively to all fields within a composite type. This is new behavior
in v2.

With typed struct tags coming in the horizon, it increasingly looks like
most tag options would be non-recursive in nature (i.e., they only
affect the immediate field value), perhaps we a specific typed option
modifier to make an option recursive.

Thus we have decided to revert back to non-recursive behavior similar to
v1.

One motivation for making `string` recursive was mistakes and
surprise from users that applied the tag to composite types without
realizing it has no effect.

To help address this, placing `string` on a field with an unsupported
type now reports a runtime error rather than simply ignoring the tag.

StringifyWithLegacySemantics controls whether `string` supports bool and
string (as before), and ReportErrorsWithLegacySemantics controls type
error reporting. When true, type errors are suppressed. Note that this
means that v1 can opt-in to strict errors while keeping bool/string
support, and that v2 can suppress errors without adding bool/string
support.

Interaction with `format` is pretty awkward. time.Time with
`string,format:unix` outputs a stringified number, in spite of `string`
documentating stating that it only applies to numeric Go types. #79451
tracks reconsidering the behavior. Until then I have maintained the
original behavior.

There are tons of new tests, primarily due to the new error behavior.
The tests previously used a large with many different Go types, all with
`string`. Now a single type error would preclude testing of the other
types. So each type is split into its own test. Additionally, we need to
test the different combinations of StringifyWithLegacySemantics and
ReportErrorsWithLegacySemantics.

Fixes #79065.
Updates #79451.

Change-Id: If970138d65e4164619358009b2d41df06a6a6964
Reviewed-on: https://go-review.googlesource.com/c/go/+/779300
LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Joseph Tsai <joetsai@digital-static.net>
Auto-Submit: Michael Pratt <mpratt@google.com>
This commit is contained in:
Michael Pratt 2026-05-15 11:48:47 -04:00 • committed by Gopher Robot
parent e8c1e370c9
commit e9edbced42
6 changed files with 856 additions and 563 deletions
Download patch file Download diff file Expand all files Collapse all files

109
src/encoding/json/v2/arshal_default.go
View file

@ -1076,6 +1076,34 @@ func makeStructArshaler(t reflect.Type) *arshaler {
if errInit != nil && !mo.Flags.Get(jsonflags.ReportErrorsWithLegacySemantics) {
return newMarshalErrorBefore(enc, errInit.GoType, errInit.Err)
}
// Validate that `string` struct tags only appear on valid
// field types.
//
// `string` tag type validation only occurs with new error
// semantics. Legacy semantics ignores errors.
//
// This validation is effectively a makeStructFields error that
// occurs before any marshalling begins, but since it depends
// on the marshal options it can't be part of the sync.Once.
if !mo.Flags.Get(jsonflags.ReportErrorsWithLegacySemantics) {
for i := range fields.flattened {
f := &fields.flattened[i]
if f.string {
if !mo.Flags.Get(jsonflags.StringifyWithLegacySemantics) {
if !canStringify(f.typ, f.format) {
st := va.Type() // Type of the enclosing struct.
return newMarshalErrorBefore(enc, st, newInvalidStringTagError(st.Field(f.index0).Name, false))
}
} else {
if !canLegacyStringify(f.typ, f.format) {
st := va.Type() // Type of the enclosing struct.
return newMarshalErrorBefore(enc, st, newInvalidStringTagError(st.Field(f.index0).Name, true))
}
}
}
}
}
if err := enc.WriteToken(jsontext.BeginObject); err != nil {
return err
}
@ -1163,8 +1191,11 @@ func makeStructArshaler(t reflect.Type) *arshaler {
flagsOriginal := mo.Flags
if f.string {
if !mo.Flags.Get(jsonflags.StringifyWithLegacySemantics) {
mo.Flags.Set(jsonflags.StringifyNumbers | 1)
} else if canLegacyStringify(f.typ) {
// Note that errors are reported above.
if canStringify(f.typ, f.format) {
mo.Flags.Set(jsonflags.StringifyNumbers | 1)
}
} else if canLegacyStringify(f.typ, f.format) {
mo.Flags.Set(jsonflags.StringifyNumbers | jsonflags.StringifyBoolsAndStrings | 1)
}
}
@ -1250,6 +1281,34 @@ func makeStructArshaler(t reflect.Type) *arshaler {
if errInit != nil && !uo.Flags.Get(jsonflags.ReportErrorsWithLegacySemantics) {
return newUnmarshalErrorAfter(dec, errInit.GoType, errInit.Err)
}
// Validate that `string` struct tags only appear on valid
// field types.
//
// `string` tag type validation only occurs with new error
// semantics. Legacy semantics ignores errors.
//
// This validation is effectively a makeStructFields error that
// occurs before any marshalling begins, but since it depends
// on the marshal options it can't be part of the sync.Once.
if !uo.Flags.Get(jsonflags.ReportErrorsWithLegacySemantics) {
for i := range fields.flattened {
f := &fields.flattened[i]
if f.string {
if !uo.Flags.Get(jsonflags.StringifyWithLegacySemantics) {
if !canStringify(f.typ, f.format) {
st := va.Type() // Type of the enclosing struct.
return newUnmarshalErrorAfter(dec, st, newInvalidStringTagError(st.Field(f.index0).Name, false))
}
} else {
if !canLegacyStringify(f.typ, f.format) {
st := va.Type() // Type of the enclosing struct.
return newUnmarshalErrorAfter(dec, st, newInvalidStringTagError(st.Field(f.index0).Name, true))
}
}
}
}
}
var seenIdxs uintSet
xd.Tokens.Last.DisableNamespace()
var errUnmarshal error
@ -1312,8 +1371,11 @@ func makeStructArshaler(t reflect.Type) *arshaler {
flagsOriginal := uo.Flags
if f.string {
if !uo.Flags.Get(jsonflags.StringifyWithLegacySemantics) {
uo.Flags.Set(jsonflags.StringifyNumbers | 1)
} else if canLegacyStringify(f.typ) {
// Note that errors are reported above.
if canStringify(f.typ, f.format) {
uo.Flags.Set(jsonflags.StringifyNumbers | 1)
}
} else if canLegacyStringify(f.typ, f.format) {
uo.Flags.Set(jsonflags.StringifyNumbers | jsonflags.StringifyBoolsAndStrings | 1)
}
}
@ -1399,11 +1461,39 @@ func isLegacyEmpty(v addressableValue) bool {
return false
}
// canStringify reports whether t can be stringified according to v2, where t
// is a number (or unnamed pointer to such).
// The `string` option does not apply recursively to nested types within
// a composite Go type (e.g., an array, slice, struct, map, or interface).
func canStringify(t reflect.Type, format string) bool {
// Based on encoding/json.typeFields#L1126-L1143@v1.23.0
if t.Name() == "" && t.Kind() == reflect.Ptr {
t = t.Elem()
}
switch t.Kind() {
case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64,
reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr,
reflect.Float32, reflect.Float64:
return true
}
// TODO(go.dev/issue/79451): Despite being defined in terms of the Go
// type system the `string` tag also applies to time.Time fields with a
// few different `format` tags. Thus we cannot determine validity
// solely from the field type like all other uses.
if internal.ExpJSONFormat && t == timeTimeType {
switch format {
case "unix", "unixmilli", "unixmicro", "unixnano":
return true
}
}
return false
}
// canLegacyStringify reports whether t can be stringified according to v1,
// where t is a bool, string, or number (or unnamed pointer to such).
// In v1, the `string` option does not apply recursively to nested types within
// The `string` option does not apply recursively to nested types within
// a composite Go type (e.g., an array, slice, struct, map, or interface).
func canLegacyStringify(t reflect.Type) bool {
func canLegacyStringify(t reflect.Type, format string) bool {
// Based on encoding/json.typeFields#L1126-L1143@v1.23.0
if t.Name() == "" && t.Kind() == reflect.Ptr {
t = t.Elem()
@ -1415,6 +1505,13 @@ func canLegacyStringify(t reflect.Type) bool {
reflect.Float32, reflect.Float64:
return true
}
// See above.
if internal.ExpJSONFormat && t == timeTimeType {
switch format {
case "unix", "unixmilli", "unixmicro", "unixnano":
return true
}
}
return false
}

1141
src/encoding/json/v2/arshal_test.go
View file

File diff suppressed because it is too large Load diff

18
src/encoding/json/v2/doc.go
View file

@ -78,13 +78,17 @@
// encoded as a JSON null, empty string, empty object, or empty array.
// This option has no effect when unmarshaling.
//
// - string: The "string" option specifies that [StringifyNumbers]
// be set when marshaling or unmarshaling a struct field value.
// This causes numeric types to be encoded as a JSON number
// within a JSON string, and to be decoded from a JSON string
// containing the JSON number without any surrounding whitespace.
// This extra level of encoding is often necessary since
// many JSON parsers cannot precisely represent 64-bit integers.
// - string: The "string" option specifies that [StringifyNumbers] be set
// when marshaling or unmarshaling a struct field value.
// This causes numeric types (or a pointer to a numeric type) to be encoded
// as a JSON number within a JSON string, and to be decoded from a JSON
// string containing the JSON number without any surrounding whitespace.
// The "string" option does not apply recursively. Specifically, `string`
// will not stringify bool, string, or numeric kinds within a composite
// data type (e.g., array, slice, struct, map, or interface).
// Applying this option to an invalid type causes a runtime error.
// This extra level of encoding is often necessary since many JSON parsers
// cannot precisely represent 64-bit integers.
//
// - case: When unmarshaling, the "case" option specifies how
// JSON object names are matched with the JSON name for Go struct fields.

18
src/encoding/json/v2/errors.go
View file

@ -16,6 +16,7 @@ import (
"strings"
"sync"
"encoding/json/internal"
"encoding/json/internal/jsonflags"
"encoding/json/internal/jsonopts"
"encoding/json/internal/jsonwire"
@ -450,3 +451,20 @@ func toUnexpectedEOF(err error) error {
}
return err
}
// newInvalidStringTagError returns an error for a `string` tag on a field with
// an invalid type. The error should be wrapped with appropriate context after
// creation.
func newInvalidStringTagError(field string, legacy bool) error {
if legacy {
if internal.ExpJSONFormat {
return fmt.Errorf("Go struct field %s has invalid `string` tag: field must be a numeric type, string, or bool (or pointer to such), or type with a format tag converting to a numeric type", field)
}
return fmt.Errorf("Go struct field %s has invalid `string` tag: field must be a numeric type, string, or bool (or pointer to such)", field)
}
if internal.ExpJSONFormat {
return fmt.Errorf("Go struct field %s has invalid `string` tag: field must be a numeric type (or pointer to such), or type with a format tag converting to a numeric type", field)
}
return fmt.Errorf("Go struct field %s has invalid `string` tag: field must be a numeric type (or pointer to such)", field)
}

112
src/encoding/json/v2_diff_test.go
View file

@ -257,14 +257,18 @@ func addr[T any](v T) *T {
// The "string" option is not applied recursively, and so does not affect
// strings, bools, and numeric values within a Go slice or map, but
// does have special handling to affect the underlying value within a pointer.
// If the "string" option is present on an unsupported type, it is simply ignored.
// When unmarshaling, the "string" option permits decoding from a JSON null
// escaped within a JSON string in some inconsistent cases.
//
// In v2, the "string" option specifies that only numeric values are encoded as
// a JSON number within a JSON string when marshaling and are unmarshaled
// from either a JSON number or a JSON string containing a JSON number.
// The "string" option is applied recursively to all numeric sub-values,
// and thus affects numeric values within a Go slice or map.
// The "string" option is still not applied recursively, and so does not affect
// within a Go slice or map, but it retains special handling to affect the
// underlying value within a pointer.
// If the "string" option is present on an unsupported type, a runtime error is
// reported.
// There is no support for escaped JSON nulls within a JSON string.
//
// The main utility for stringifying JSON numbers is because JSON parsers
@ -276,13 +280,6 @@ func addr[T any](v T) *T {
// to just numeric Go types. According to all code known by the Go module proxy,
// there are close to zero usages of the "string" option on a Go string or bool.
//
// Regarding the recursive application of the "string" option,
// there have been a number of issues filed about users being surprised that
// the "string" option does not recursively affect numeric values
// within a composite type like a Go map, slice, or interface value.
// In v1, specifying the "string" option on composite type has no effect
// and so this would be a largely backwards compatible change.
//
// The ability to decode from a JSON null wrapped within a JSON string
// is removed in v2 because this behavior was surprising and inconsistent in v1.
//
@ -294,8 +291,9 @@ func addr[T any](v T) *T {
// https://go.dev/issue/32055
// https://go.dev/issue/32117
// https://go.dev/issue/50997
// https://go.dev/issue/79065
func TestStringOption(t *testing.T) {
type Types struct {
type AllTypes struct {
String string `json:",string"`
Bool bool `json:",string"`
Int int `json:",string"`
@ -311,9 +309,15 @@ func TestStringOption(t *testing.T) {
InterfaceB any `json:",string"`
}
type V2Types struct {
Int int `json:",string"`
Float float64 `json:",string"`
PointerA *int `json:",string"`
}
for _, json := range jsonPackages {
t.Run(path.Join("Marshal", json.Version), func(t *testing.T) {
in := Types{
in := AllTypes{
String: "string",
Bool: true,
Int: 1,
@ -338,29 +342,32 @@ func TestStringOption(t *testing.T) {
}
return s
}
quoteOnlyV2 := func(s string) string {
if json.Version == "v2" {
s = quote(s)
}
return s
}
want := strings.Join([]string{
`{`,
`"String":` + quoteOnlyV1(`"string"`) + `,`, // in v1, Go strings are also stringified
`"Bool":` + quoteOnlyV1("true") + `,`, // in v1, Go bools are also stringified
`"Int":` + quote("1") + `,`,
`"Float":` + quote("1") + `,`,
`"Map":{"Name":` + quoteOnlyV2("1") + `},`, // in v2, numbers are recursively stringified
`"Struct":{"Field":` + quoteOnlyV2("1") + `},`, // in v2, numbers are recursively stringified
`"Slice":[` + quoteOnlyV2("1") + `],`, // in v2, numbers are recursively stringified
`"Array":[` + quoteOnlyV2("1") + `],`, // in v2, numbers are recursively stringified
`"Map":{"Name":1},`, // No recursive stringification
`"Struct":{"Field":1},`, // No recursive stringification
`"Slice":[1],`, // No recursive stringification
`"Array":[1],`, // No recursive stringification
`"PointerA":null,`,
`"PointerB":` + quote("1") + `,`, // in v1, numbers are stringified after a single pointer indirection
`"PointerC":` + quoteOnlyV2("1") + `,`, // in v2, numbers are recursively stringified
`"PointerB":` + quote("1") + `,`, // numbers are stringified after a single pointer indirection
`"PointerC":1,`, // No recursive stringification
`"InterfaceA":null,`,
`"InterfaceB":` + quoteOnlyV2("1") + ``, // in v2, numbers are recursively stringified
`"InterfaceB":1`, // No recursive stringification
`}`}, "")
got, err := json.Marshal(in)
var got []byte
var err error
if json.Version == "v2" {
// Suppress type errors in v2, so we can
// compare the affects regardless of type
// errors.
got, err = jsonv2.Marshal(in, jsonv1.ReportErrorsWithLegacySemantics(true))
} else {
got, err = json.Marshal(in)
}
if err != nil {
t.Fatalf("json.Marshal error: %v", err)
}
@ -372,84 +379,51 @@ func TestStringOption(t *testing.T) {
for _, json := range jsonPackages {
t.Run(path.Join("Unmarshal/Null", json.Version), func(t *testing.T) {
var got Types
var got AllTypes
err := json.Unmarshal([]byte(`{
"Bool": "null",
"Int": "null",
"PointerA": "null"
}`), &got)
switch {
case !reflect.DeepEqual(got, Types{}):
t.Fatalf("json.Unmarshal = %v, want %v", got, Types{})
case json.Version == "v1" && err != nil:
t.Fatalf("json.Unmarshal error: %v", err)
case json.Version == "v2" && err == nil:
t.Fatal("json.Unmarshal error is nil, want non-nil")
case !reflect.DeepEqual(got, AllTypes{}):
t.Fatalf("json.Unmarshal = %+v, want %+v", got, AllTypes{})
}
})
t.Run(path.Join("Unmarshal/Bool", json.Version), func(t *testing.T) {
var got Types
want := map[string]Types{
var got AllTypes
want := map[string]AllTypes{
"v1": {Bool: true},
"v2": {Bool: false},
}[json.Version]
err := json.Unmarshal([]byte(`{"Bool": "true"}`), &got)
switch {
case !reflect.DeepEqual(got, want):
t.Fatalf("json.Unmarshal = %v, want %v", got, want)
case json.Version == "v1" && err != nil:
t.Fatalf("json.Unmarshal error: %v", err)
case json.Version == "v2" && err == nil:
t.Fatal("json.Unmarshal error is nil, want non-nil")
case !reflect.DeepEqual(got, want):
t.Fatalf("json.Unmarshal = %v, want %v", got, want)
}
})
t.Run(path.Join("Unmarshal/Shallow", json.Version), func(t *testing.T) {
var got Types
want := Types{Int: 1, PointerB: addr(1)}
var got V2Types
want := V2Types{Int: 1, PointerA: addr(1)}
err := json.Unmarshal([]byte(`{
"Int": "1",
"PointerB": "1"
"PointerA": "1"
}`), &got)
switch {
case !reflect.DeepEqual(got, want):
t.Fatalf("json.Unmarshal = %v, want %v", got, want)
case err != nil:
t.Fatalf("json.Unmarshal error: %v", err)
}
})
t.Run(path.Join("Unmarshal/Deep", json.Version), func(t *testing.T) {
var got Types
want := map[string]Types{
"v1": {
Map: map[string]int{"Name": 0},
Slice: []int{0},
PointerC: addr(addr(0)),
},
"v2": {
Map: map[string]int{"Name": 1},
Struct: struct{ Field int }{1},
Slice: []int{1},
Array: [1]int{1},
PointerC: addr(addr(1)),
},
}[json.Version]
err := json.Unmarshal([]byte(`{
"Map": {"Name":"1"},
"Struct": {"Field":"1"},
"Slice": ["1"],
"Array": ["1"],
"PointerC": "1"
}`), &got)
switch {
case !reflect.DeepEqual(got, want):
t.Fatalf("json.Unmarshal =\n%v, want\n%v", got, want)
case json.Version == "v1" && err == nil:
t.Fatal("json.Unmarshal error is nil, want non-nil")
case json.Version == "v2" && err != nil:
t.Fatalf("json.Unmarshal error: %v", err)
t.Fatalf("json.Unmarshal =\n%+v, want\n%+v", got, want)
}
})
}

21
src/encoding/json/v2_options.go
View file

@ -44,10 +44,9 @@
// `omitzero` instead (which is identically supported in both v1 and v2).
//
// - In v1, a Go struct field marked as `string` can be used to quote a
// Go string, bool, or number as a JSON string. It does not recursively
// take effect on composite Go types. In contrast, v2 restricts
// the `string` option to only quote a Go number as a JSON string.
// It does recursively take effect on Go numbers within a composite Go type.
// Go string, bool, number, or pointer to such as a JSON string.
// In contrast, v2 restricts the `string` option to only quote a Go number
// or pointer to number as a JSON string.
// The [StringifyWithLegacySemantics] option controls this behavior difference.
//
// - In v1, a nil Go slice or Go map is marshaled as a JSON null.
@ -500,15 +499,13 @@ func ReportErrorsWithLegacySemantics(v bool) Options {
// StringifyWithLegacySemantics specifies that the `string` tag option
// may stringify bools and string values. It only takes effect on fields
// where the top-level type is a bool, string, numeric kind, or a pointer to
// such a kind. Specifically, `string` will not stringify bool, string,
// or numeric kinds within a composite data type
// (e.g., array, slice, struct, map, or interface).
// such a kind.
//
// When marshaling, such Go values are serialized as their usual
// JSON representation, but quoted within a JSON string.
// When unmarshaling, such Go values must be deserialized from
// a JSON string containing their usual JSON representation or
// Go number representation for that numeric kind.
// When marshaling, such Go values are serialized as their usual JSON
// representation, but quoted within a JSON string.
// When unmarshaling, such Go values must be deserialized from a JSON string
// containing their usual JSON representation or Go number representation for
// that numeric kind.
// Note that the Go number grammar is a superset of the JSON number grammar.
// A JSON null quoted in a JSON string is a valid substitute for JSON null
// while unmarshaling into a Go value that `string` takes effect on.