Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

encoding/json: UnmarshalJSON not working consistently with struct embedding #39175

Closed
magodo opened this issue May 20, 2020 · 3 comments
Closed

encoding/json: UnmarshalJSON not working consistently with struct embedding #39175

magodo opened this issue May 20, 2020 · 3 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.

Comments

@magodo
Copy link

magodo commented May 20, 2020

What version of Go are you using (go version)?

$ go version
go version go1.14.2 linux/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/magodo/.cache/go-build"
GOENV="/home/magodo/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/magodo/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go"
GOSUMDB="off"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/tmp/gofoo/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build012822801=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Case 1

Run following code:

package main

import (
	"encoding/json"
	"log"

	"github.com/davecgh/go-spew/spew"
)

type Foo struct {
	A string
	Bar
	Baz
}

type Bar struct {
	B string
}

type Baz struct {
	C string
}

func (v *Foo) UnmarshalJSON(data []byte) error {
	type t Foo
	return json.Unmarshal(data, (*t)(v))
}

func (v *Bar) UnmarshalJSON(data []byte) error {
	type t Bar
	return json.Unmarshal(data, (*t)(v))
}

func (v *Baz) UnmarshalJSON(data []byte) error {
	type t Baz
	return json.Unmarshal(data, (*t)(v))
}

func main() {
	var foo Foo
	if err := json.Unmarshal([]byte(`{
  "a": "foo",
  "b": "bar",
  "c": "baz"
}`), &foo); err != nil {
		log.Fatal(err)
	}
	spew.Dump(foo)
}

Case 2

Run above code, but comment out line 13 (i.e. remove the Baz embedded field from Foo).

What did you expect to see?

Case 1

(main.Foo) {
 A: (string) (len=3) "foo",
 Bar: (main.Bar) {
  B: (string) (len=3) "bar"
 },
 Baz: (main.Baz) {
  C: (string) (len=3) "baz"
 }
}

Case 2

(main.Foo) {
 A: (string) (len=3) "foo",
 Bar: (main.Bar) {
  B: (string) (len=3) "bar"
 }
}

What did you see instead?

Case 1

As expected.

Case 2

(main.Foo) {
 A: (string) "",
 Bar: (main.Bar) {
  B: (string) (len=3) "bar"
 }
}

Summary

The behavior of nested field seems not consistent between only one nested filed and having multiple nested fields.
@mvdan mvdan added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 20, 2020
@mvdan mvdan changed the title JSON custom unmarshal not working consistently encoding/json: UnmarshalJSON not working consistently with struct embedding May 20, 2020
@magodo
Copy link
Author

magodo commented May 20, 2020 •
edited

I got some answer from the community:

Case 2 when you embed a struct it's methods can be promoted if they don't conflict. While for case 1, if you embed multiple structs each with the same method (UnmarshalJSON), Go can't know which one you intend to call so none of them are available.

That means neither Bar nor Baz will promote their methods into the method set of Foo in case 1.
While I'm wondering if we can be consistent. I mean, maybe in the implementation of indirect() here, is it possible to add more check to avoid to use the promoted method at all? Though this seems to be a big breaking change, and not sure if it is feasible.

@mvdan
Copy link
Member

mvdan commented May 20, 2020

We briefly discussed this on Slack, and I agree with @seankhliao that this is intended behavior, which is what he described in the quote above.

We can't make encoding/json ignore promoted methods altogether; that would break existing programs that were following the docs just fine. It could also be a precedent for ignoring promoted methods similarly for other packages like fmt.Stringer, xml.Marshaler, encoding.TextMarshaler, and so on.

I think the general answer here is - if you don't know what methods a named type has, or if it's maintained in an external module out of your control, you want to be careful about embedding it and gaining its methods. Because it might "take over" methods you don't declare yourself and affect printing, encoding, decoding, etc.

@magodo should we close this as "working as intended"?

@andybons andybons added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label May 20, 2020
@magodo magodo closed this as completed May 20, 2020
@magodo
Copy link
Author

magodo commented May 20, 2020

Closed it as it works as expected.
@mvdan Thank you for your support!

@magodo magodo mentioned this issue May 21, 2020
Closed
@radeksimko radeksimko mentioned this issue May 26, 2020
Closed
@mvdan mvdan mentioned this issue Jun 29, 2020
Closed
@golang golang locked and limited conversation to collaborators May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@andybons @mvdan @magodo @gopherbot