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.

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

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

Comments

@magodo
Copy link

@magodo 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 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 magodo commented May 20, 2020

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 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"?

@magodo
Copy link
Author

@magodo magodo commented May 20, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.