golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
122.15k stars 17.46k forks source link

encoding/json: UnmarshalJSON methods of embedded fields are ignored #64847

Open zephyrtronium opened 8 months ago

zephyrtronium commented 8 months ago

Go version

go version go1.21.4 linux/amd64

What operating system and processor architecture are you using (go env)?

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/branden/.cache/go-build'
GOENV='/home/branden/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/branden/go/pkg/mod'
GONOPROXY='*.redacted.net'
GONOSUMDB='*.redacted.net'
GOOS='linux'
GOPATH='/home/branden/go'
GOPRIVATE='*.redacted.net'
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.21.4'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build2911026133=/tmp/go-build -gno-record-gcc-switches'

What did you do?

https://go.dev/play/p/bNXLi9KmwHA

type (
    Bocchi struct {
        Bocchi string `json:"bocchi"`
    }
    Ryou struct {
        Ryou string `json:"ryou"`
    }
    Kessoku struct {
        Bocchi
        Ryou
    }
)

func (b *Bocchi) UnmarshalJSON(p []byte) error { return errors.New("kita") }
func (r *Ryou) UnmarshalJSON(p []byte) error   { return errors.New("nijika") }

func main() {
    var k Kessoku
    err := json.Unmarshal([]byte(`{"bocchi":"bocchi","ryou":"ryou"}`), &k)
    fmt.Printf("%#v\n", k)
    fmt.Println(err)
}

I.e., define a type embedding two other types that each implement json.Unmarshaler, then unmarshal into the containing type, which does not implement json.Unmarshaler.

What did you expect to see?

Empty fields in k and a non-nil error. The decoder unmarshals each field according to its respective rules.

I would also accept empty fields and a nil error as if the decoder unmarshals each embedded field as if it had its type as its field name, such that {"Bocchi":{"bocchi":"kita"}} would unmarshal to an object equal to Kessoku{Bocchi: Bocchi{"kita"}}. (Unmarshalling that document instead results in an error mentioning "Go struct field Kessoku.bocchi", which doesn't actually exist, but I'm not certain I'd call that a bug.)

What did you see instead?

Filled fields and a nil error. The decoder unmarshals the containing type as if it has exactly the list of promoted fields on the type, without regard to any json.Unmarshaler.

39175 is related, but there the report was that the behavior between one and two embedded fields was inconsistent (which is due to Go's language rules) rather than that the behavior with two is surprising per se. In my case, the containing type not implementing UnmarshalJSON by promotion is the point, but I want the embedded fields which are unmarshalers to have their logic respected.

The practical goal in my project is to have the equivalents of Bocchi and Ryou unmarshal according to default rules and then each perform validation before returning from their respective UnmarshalJSON. I sometimes need to handle a Bocchi explicitly without a Ryou, hence using separate types. It's straightforward to get the behavior I want by defining an UnmarshalJSON on Kessoku which unmarshals each field explicitly; I'm reporting as an issue because I got what I consider the most surprising behavior when I tried without.

The most flexible solution for my problem is probably #6213, which is addressed by #63397, so there might not be anything to do here.

seankhliao commented 8 months ago

see

https://go.googlesource.com/go/+/9b4b3e5acca2dabe107fa2c3ed963097d78a4562/src/encoding/json/encode.go#1175 https://go.googlesource.com/go/+/9b4b3e5acca2dabe107fa2c3ed963097d78a4562/src/encoding/json/encode.go#1202

dr2chase commented 7 months ago

@rsc @dsnet either of you care to take a look? I also wish @seankhliao's pair of links abovecame with at least a brief translation.

seankhliao commented 7 months ago

my understanding is that fields of embedded structs get promoted, so they're treated as fields in the parent struct, triggering a direct match based on tag/name, so there's no reason to call UnmarshalJSON

dsnet commented 7 months ago

The rules that the "json" package follows are based on the embedding rules of the Go language itself.

Both Bocchi and Ryou are embedded in Kessoku, which means that both the fields and methods are promoted to Kessoku. Since both Bocchi and Ryou have UnmarshalJSON methods, they cancel each other out. Thus, semantically Kessoku has both promoted Bocchi string and Ryou string fields, but not UnmarshalJSON method.

The currently observed behavior is a natural outflow of the above semantic structure. I don't think we should change these rules.

dsnet commented 7 months ago

The practical goal in my project is to have the equivalents of Bocchi and Ryou unmarshal according to default rules and then each perform validation before returning from their respective UnmarshalJSON.

I'm not sure what "default rules" means since in my mind the current behavior is a sensible application of the embedding rules in Go.

There are two possible workarounds I can suggest:

type Kessoku struct {
    Bocchi Bocchi
    Ryou Ryou
}

In this example, we avoid embedding, and thus avoid the promotion of fields.

Another example:

type Kessoku struct {
    Bocchi `json:"Bocchi"`
    Ryou `json:"Ryou"`
}

while embedding is used, we explicitly name the embedded fields, which makes it behave as above.

zephyrtronium commented 7 months ago

Looking back through the documentation, I think this is closer to #27031 than anything. I skipped the paragraph about how encoding/json handles "anonymous" struct fields and didn't find anything about embedded fields. Since the CLs there have stalled, I can send another one to address that and to clarify that the same rules apply to methods as well as fields for this issue.

gopherbot commented 7 months ago

Change https://go.dev/cl/552959 mentions this issue: encoding/json: clarify handling of embedded fields' methods