From ed5197f840e45950717d1161096150e8ca777ef9 Mon Sep 17 00:00:00 2001 From: Joe Tsai Date: Tue, 5 Sep 2023 16:27:05 -0700 Subject: [PATCH] Legacy support for v1 --- arshal_default.go | 28 +++++++++++++++++++++------- v1/breaking_test.go | 37 +++++++++++++++++++++++++++++++++++++ v1/decode.go | 4 +++- v1/diff.sh | 3 +++ v1/failing.txt | 1 - v1/options.go | 42 +++++++++++++++++++++++++++++++++--------- 6 files changed, 97 insertions(+), 18 deletions(-) create mode 100644 v1/breaking_test.go create mode 100755 v1/diff.sh diff --git a/arshal_default.go b/arshal_default.go index c6c5c96..c29169e 100644 --- a/arshal_default.go +++ b/arshal_default.go @@ -157,7 +157,9 @@ func makeBoolArshaler(t reflect.Type) *arshaler { k := tok.Kind() switch k { case 'n': - va.SetBool(false) + if !uo.Flags.Get(jsonflags.MergeWithLegacySemantics) { + va.SetBool(false) + } return nil case 't', 'f': if !uo.Flags.Get(jsonflags.StringifyBoolsAndStrings) { @@ -229,7 +231,9 @@ func makeStringArshaler(t reflect.Type) *arshaler { k := val.Kind() switch k { case 'n': - va.SetString("") + if !uo.Flags.Get(jsonflags.MergeWithLegacySemantics) { + va.SetString("") + } return nil case '"': val = jsonwire.UnquoteMayCopy(val, flags.IsVerbatim()) @@ -351,7 +355,9 @@ func makeBytesArshaler(t reflect.Type, fncs *arshaler) *arshaler { k := val.Kind() switch k { case 'n': - va.SetZero() + if !uo.Flags.Get(jsonflags.MergeWithLegacySemantics) || va.Kind() == reflect.Slice { + va.SetZero() + } return nil case '"': val = jsonwire.UnquoteMayCopy(val, flags.IsVerbatim()) @@ -437,7 +443,9 @@ func makeIntArshaler(t reflect.Type) *arshaler { k := val.Kind() switch k { case 'n': - va.SetInt(0) + if !uo.Flags.Get(jsonflags.MergeWithLegacySemantics) { + va.SetInt(0) + } return nil case '"': if !uo.Flags.Get(jsonflags.StringifyNumbers) { @@ -515,7 +523,9 @@ func makeUintArshaler(t reflect.Type) *arshaler { k := val.Kind() switch k { case 'n': - va.SetUint(0) + if !uo.Flags.Get(jsonflags.MergeWithLegacySemantics) { + va.SetUint(0) + } return nil case '"': if !uo.Flags.Get(jsonflags.StringifyNumbers) { @@ -603,7 +613,9 @@ func makeFloatArshaler(t reflect.Type) *arshaler { k := val.Kind() switch k { case 'n': - va.SetFloat(0) + if !uo.Flags.Get(jsonflags.MergeWithLegacySemantics) { + va.SetFloat(0) + } return nil case '"': val = jsonwire.UnquoteMayCopy(val, flags.IsVerbatim()) @@ -1463,7 +1475,9 @@ func makeArrayArshaler(t reflect.Type) *arshaler { k := tok.Kind() switch k { case 'n': - va.SetZero() + if !uo.Flags.Get(jsonflags.MergeWithLegacySemantics) { + va.SetZero() + } return nil case '[': once.Do(init) diff --git a/v1/breaking_test.go b/v1/breaking_test.go new file mode 100644 index 0000000..b82e5d4 --- /dev/null +++ b/v1/breaking_test.go @@ -0,0 +1,37 @@ +package json + +// List of breaking v1 changes. +// TODO: Some of these might need to be a v1 compatibility option +// or alternatively a GODEBUG option (see https://go.dev/doc/godebug). +const ( + // avoidNilPointerMethods means that implementation always avoids + // calling MarshalJSON and MarshalText methods on nil pointers. + // This is a break from historical v1 behavior. + // In particular, v1 would call the method only if the type is an interface, + // and the interface implements the method. However, if the type is a + // concrete pointer type or an interface (without the method) but contains + // a nil pointer, then the method is not called. This is inconsistent. + // + // See https://go.dev/play/p/WaTuxCmqDGL + // See https://go.dev/play/p/9WeUhxXYiMH + avoidNilPointerMethods = true + + // namedByteInSliceAsJSONArray means that a named byte in a slice + // (e.g., []MyByte) is treated as JSON array of MyByte, + // rather than as a JSON string that contains base64-encoded binary data. + // The v1 behavior has always been relying on a bug in reflect, + // where reflect.Value.Bytes can be called on such types even though + // the Go language specifies that []MyByte cannot be directly converted + // into a []byte (i.e., the two types are invariant). + // + // See https://go.dev/issue/24746 + namedByteInSliceAsJSONArray = true +) + +func ternary[T any](boolVal bool, trueVal, falseVal T) T { + if boolVal { + return trueVal + } else { + return falseVal + } +} diff --git a/v1/decode.go b/v1/decode.go index 659e17f..b5267c0 100644 --- a/v1/decode.go +++ b/v1/decode.go @@ -223,7 +223,9 @@ func (n *Number) UnmarshalJSONV2(dec *jsontext.Decoder, opts jsonv2.Options) err k := val.Kind() switch k { case 'n': - *n = "" // TODO: Should we merge with legacy semantics? + if legacy, _ := jsonv2.GetOption(opts, MergeWithLegacySemantics); !legacy { + *n = "" + } return nil case '"': if !stringify { diff --git a/v1/diff.sh b/v1/diff.sh new file mode 100755 index 0000000..c2bc7ba --- /dev/null +++ b/v1/diff.sh @@ -0,0 +1,3 @@ +#!/bin/bash + +for X in $(ls *_test.go); do diff -u ../../go/src/encoding/json/$X $X; done diff --git a/v1/failing.txt b/v1/failing.txt index 95f462a..fd026bd 100644 --- a/v1/failing.txt +++ b/v1/failing.txt @@ -5,7 +5,6 @@ TestUnmarshal/#109 TestUnmarshal/#111 TestUnmarshal/#113 TestUnmarshal/#138 -TestNullString TestInterfaceSet TestInterfaceSet/#01 TestInterfaceSet/#02 diff --git a/v1/options.go b/v1/options.go index a626723..79e46ee 100644 --- a/v1/options.go +++ b/v1/options.go @@ -2,11 +2,6 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -// Package json implements legacy support for v1 [encoding/json]. -// -// The entirety of the v1 API will eventually live in this package. -// For the time being, it only contains options needed to configure the v2 API -// to operate under v1 semantics. package json import ( @@ -38,9 +33,11 @@ type Options = jsonopts.Options // - [FormatTimeDurationAsNanosecond] // - [IgnoreStructErrors] // - [MatchCaseSensitiveDelimiter] +// - [MergeWithLegacySemantics] // - [OmitEmptyWithLegacyDefinition] // - [RejectFloatOverflow] // - [ReportLegacyErrorValues] +// - [SkipUnaddressableMethods] // - [StringifyWithLegacySemantics] // - [UnmarshalArrayFromAnyLength] // - [jsonv2.Deterministic] @@ -146,6 +143,17 @@ func MatchCaseSensitiveDelimiter(v bool) Options { } } +// TODO: null being ignored instead of clear. +// TODO: other odd behavior? +// The v1 default is true. +func MergeWithLegacySemantics(v bool) Options { + if v { + return jsonflags.MergeWithLegacySemantics | 1 + } else { + return jsonflags.MergeWithLegacySemantics | 0 + } +} + // OmitEmptyWithLegacyDefinition specifies that the `omitempty` tag option // follows a definition of empty where a field is omitted if the Go value is // false, 0, a nil pointer, a nil interface value, @@ -199,12 +207,28 @@ func ReportLegacyErrorValues(v bool) Options { } } +// SkipUnaddressableMethods specifies that [Marshaler] and [Unmarshaler] +// methods declared on a pointer receiver are not called on values that +// are not addressable (e.g., values derived from a map or interface value). +// This option exists to maintain compatibility for behavior that +// has been widely regarded as a bug. Unfortunately, the effect of this option +// is not consistent and more heavily applies to marshaling than unmarshaling. +// +// The v1 default is true. +func SkipUnaddressableMethods(v bool) Options { + if v { + return jsonflags.SkipUnaddressableMethods | 1 + } else { + return jsonflags.SkipUnaddressableMethods | 0 + } +} + // 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). +// where the top-level type is a bool, string, or numeric type. +// Specifically, `string` will not stringify bool, string, or numeric types +// within a composite data type (e.g., array, slice, struct, map, or interface). +// It treats a quoted "null" as if it were a JSON null. TODO: keep this behavior? // // This affects either marshaling or unmarshaling. // The v1 default is true.