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: Unmarshal doesn't work with embedded structs #29777

Closed
mirtchovski opened this issue Jan 17, 2019 · 7 comments

Comments

Projects
None yet
5 participants
@mirtchovski
Copy link
Contributor

commented Jan 17, 2019

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

$ go1.12beta2 version
go version go1.12beta2 darwin/amd64

Does this issue reproduce with the latest release?

Yes, also on the playground

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

go env Output
$ go1.12beta2 env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/aam/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/aam/go"
GOPROXY=""
GORACE=""
GOROOT="/Users/aam/sdk/go1.12beta2"
GOTMPDIR=""
GOTOOLDIR="/Users/aam/sdk/go1.12beta2/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/tmp/t/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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/sp/06p28g2d0vs7gd2vhf26wl9m0000gn/T/go-build025782431=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I compiled and ran this program:

https://play.golang.org/p/toQBPltN0Lk

What did you expect to see?

I expected to see the field "Criteria" populated.

What did you see instead?

The field "Criteria" was not populated. If I, for example, remove the Q field entirely and instead embed OQ I get correct results (both Criteria and S are populated from the same json):

https://play.golang.org/p/ZoqJlTStEWF

I was expecting the same behaviour with Q as with OQ.

@agnivade

This comment has been minimized.

Copy link
Member

commented Jan 17, 2019

@aslrousta

This comment has been minimized.

Copy link

commented Jan 18, 2019

This is presumably a bug in json.indirect function (at encoding/json/decode.go). If a struct has an Unmarshal function—whether explicit or inherited—it returns that function which is further used to unmarshal the entire struct.

if u, ok := v.Interface().(Unmarshaler); ok {
return u, nil, reflect.Value{}
}

The obvious workaround is to redefine Unmarshal function for Q struct, however if the above behavior is intended, the compiler must warn about it.

@as

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2019

I don't think this is a bug.

  • You've implemented json.Marshaler on type Q
  • Embedding Q to req also attaches that method to req.
  • The req type of *struct{...} does not implement json.Marshaler

To fix, write:

	var req struct {
		Criteria string `json:"criteria"`
		Q        Q
	}

Note: I currently rely on the current behavior of Q hijacking the marshaling process for some functions.

@aslrousta

This comment has been minimized.

Copy link

commented Jan 18, 2019

You've implemented json.Marshaler on type Q

You'd mention json.Unmarshaler, I think. BTW, the godoc specifies:

To unmarshal JSON into a value implementing the Unmarshaler interface, Unmarshal calls that value's UnmarshalJSON method, including when the input is a JSON null.

So, yes! According to the document, since req is implementing json.Unmarshaler (borrowing it from Q), the behavior is expected. However, I'd recommend to raise a warning or be more specific in the document, since this behavior is prone to errors.

@aslrousta

This comment has been minimized.

Copy link

commented Jan 18, 2019

I've just looked at an example with encoding/xml for a similar scenario. The xml.Unmarshal would complain with an error:

xml: (*struct {...}).UnmarshalXML did not consume entire <X> element

This is much better, and the same can happen with encoding/json, e.g.:

json: (*struct {...}).UnmarshalJSON did not consume entire JSON object

And, if someone knows exactly what he/she is doing, can simply ignore it, using:

err := json.Unmarshal(data, &obj)
if err != nil && !json.IsNotEntireJSON(err) {
    panic(err)
}
@bcmills

This comment has been minimized.

Copy link
Member

commented Jan 29, 2019

CC @rsc @dsnet @bradfitz @mvdan for encoding/json.

As @as notes, this is working as documented.

On top of that, the encoding/json package has no way to know, in general, whether your UnmarshalJSON method consumed the entire JSON object or not. (For all we know, you're parsing some of the fields via a recursive call to json.Unmarshal and others by pulling out the fields directly.)

@bcmills

This comment has been minimized.

Copy link
Member

commented Jan 29, 2019

I don't think there is anything more to be done here. (You're welcome to propose a change to the behavior of the package, but that should be filed as a separate proposal.)

@bcmills bcmills closed this Jan 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.