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: clarify Unmarshal behavior for string keys that implement encoding.TextUnmarshaler #33298

Closed
oncilla opened this issue Jul 26, 2019 · 4 comments
Labels
Documentation FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@oncilla
Copy link

oncilla commented Jul 26, 2019

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

$ go version
go version go1.11.9 linux/amd64

Does this issue reproduce with the latest release?

yes

What did you do?

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

What did you expect to see?

I expected json.Unmarshal to always use the UnmarshalText method on all key types.

The documentation is not very clear about this:
(from https://tip.golang.org/pkg/encoding/json/#Unmarshal)

To unmarshal a JSON object into a map, Unmarshal first establishes a map to use. If the map is nil, Unmarshal allocates a new map. Otherwise Unmarshal reuses the existing map, keeping existing entries. Unmarshal then stores key-value pairs from the JSON object into the map. The map's key type must either be a string, an integer, or implement encoding.TextUnmarshaler.

From that description, I would expect json.Unmarshal to use UnmarshalText on string types that implement encoding.TextUnmarshaler.
Also, I would expect it to call UnmarshalText and not UnmarshalJSON on int keys that implement json.Unmarshler.

I gather from the related issue #28827 that this cannot be changed for compatibility reasons.
Thus, the documentation should be more explicit what the expected outcome is.

What did you see instead?

When unmarshalling string types that implement the encoding.TextUnmarshaler, json.Unmarshal does not call the UnmarshalText method.

Also, json.Unmarshal happily uses UnmarshalJSON on int keys

oncilla added a commit to oncilla/scion that referenced this issue Jul 29, 2019
Currently some make keys are string types that implement the
`json.Unmarshaler` interface. Unfortunately, json.Unmarshal will ignore
string types' UnmarshalJSON method and simply set the string.

(see: golang/go#33298)

This PR makes those keys of integer type to ensure the parsing will
catch malformed map inputs.
oncilla added a commit to oncilla/scion that referenced this issue Jul 29, 2019
Currently some map keys are string types that implement the
`json.Unmarshaler` interface. Unfortunately, json.Unmarshal will ignore
string types' UnmarshalJSON method and simply set the string.
The same holds for the `encoding.TextUnmarshaler`.

(see: golang/go#33298)

This PR makes those keys of integer type to ensure the parsing will
catch malformed map inputs.
oncilla added a commit to oncilla/scion that referenced this issue Jul 30, 2019
Currently some map keys are string types that implement the
`json.Unmarshaler` interface. Unfortunately, json.Unmarshal will ignore
string types' UnmarshalJSON method and simply set the string.
The same holds for the `encoding.TextUnmarshaler`.

(see: golang/go#33298)

This PR makes those keys of integer type to ensure the parsing will
catch malformed map inputs.
oncilla added a commit to scionproto/scion that referenced this issue Jul 30, 2019
Currently some map keys are string types that implement the
`json.Unmarshaler` interface. Unfortunately, json.Unmarshal will ignore
string types' UnmarshalJSON method and simply set the string.
The same holds for the `encoding.TextUnmarshaler`.

(see: golang/go#33298)

This PR makes those keys of integer type to ensure the parsing will
catch malformed map inputs.
@FiloSottile FiloSottile added Documentation NeedsFix The path to resolution is known, but the work has not been done. labels Jul 30, 2019
@FiloSottile FiloSottile added this to the Go1.14 milestone Jul 30, 2019
@FiloSottile
Copy link
Contributor

We'd probably take CLs to make the docs clearer, both here and in #28827.

@eliben
Copy link
Member

eliben commented Jul 31, 2019

I sent https://golang.org/cl/188417 to fix #28827

If that seems reasonable, I can send a fix for this one too

@eliben
Copy link
Member

eliben commented Aug 2, 2019

Looking at the code implementing this in json/decode.go, it seems like the usage of UnmarshalJSON for types that aren't string types has been there from very early on. 7e88674 added support for encoding.TextUnmarshaler and the check for json.Unmarshaler was already there.

I'll send a CL to clarify this behavior in the documentation

@gopherbot
Copy link

Change https://golang.org/cl/188821 mentions this issue: encoding/json: clarify Unmarshal behavior for map keys

@golang golang locked and limited conversation to collaborators Sep 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants